fix(brillig_vm): Remove slice padding for foreign call inputs#9321
fix(brillig_vm): Remove slice padding for foreign call inputs#9321
Conversation
…ng of slice foreign call inputs
|
I like this idea. I have a similar conundrum in #9320 where I can't think for a reason not to truncate the extra elements during slice operations. |
I'll be able to take a deeper look at #9320 tomorrow. For the interpreter I agree that we should be able to remove the padding as we have access to the actual dynamic length. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'Test Suite Duration'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.
| Benchmark suite | Current: 78e0665 | Previous: 1748cbe | Ratio |
|---|---|---|---|
test_report_zkpassport_noir-ecdsa_ |
3 s |
2 s |
1.50 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
|
I checked on the parent of this PR what ACIR passes to the oracle when the hybrid slice is being popped/pushed (appended to the PR description in the interpreter fix), and it's definitely not what the interpreter would pass, so these values should not be relied on by anybody. Removing would be the cleanest approach 👏 |
|
I imagine this could still be breaking if there's code in aztec-packages which is assuming that it needs to do this truncation itself. |
|
I thought that wouldn’t be a problem, because if the slices are passed as a separate ForeignCallParam then they would just truncate something that is already shorter, which is a no-op. If they instead dealt with flattened data, then they would have had to hardcode some value for the capacity on their side, since nothing indicates it in the data, which would be tight coupling to an implementation detail that I imagine wasn’t meant to be part of the interface, and would have been fragile anyway. |
…n-vm-rather-than-decode
aakoshh
force-pushed
the
mv/remove-padding-in-vm-rather-than-decode
branch
from
c29790a to
78e0665
Compare
Automated pull of nightly from the [noir](https://github.com/noir-lang/noir) programming language, a dependency of Aztec. BEGIN_COMMIT_OVERRIDE fix(LSP): don't crash on broken function definition (noir-lang/noir#9441) feat(opt): Don't clone on array_len (noir-lang/noir#9440) feat(ssa_fuzzer): mode without instruction simplification + array instructions mutations + limits fix (noir-lang/noir#9438) feat(fuzz): Push and pop for slices (noir-lang/noir#9262) fix(brillig_vm): Remove slice padding for foreign call inputs (noir-lang/noir#9321) fix: allow calling private impl method defined on another module from… (noir-lang/noir#9449) fix: guard `Eq` for `[T]` to avoid OOB on length mismatch (noir-lang/noir#9453) chore: bump external pinned commits (noir-lang/noir#9454) END_COMMIT_OVERRIDE Co-authored-by: AztecBot <tech@aztecprotocol.com>
4 participants
Description
Problem*
Alternative to #9288
Summary*
Changes
brillig_vm::VM::get_memory_valuesto remove any extra items fromHeapVectors that are beyond itslength, as indicated by the precedingu32field in the parent data structure or list of inputs. This is so that when we pass the data as aForeignCallParameter, we don't have to deal with any padding while decoding the data.Unlike #9288 this is a backwards compatible change.
Additional Context
Slice length and vector size can differ when we use
IfElseto merge slices of different lengths. The array backing the slice is created so that it can fit both outcomes, and if we end up filling it with data from the shorter slice, the rest is padding. Arguably there is no reason for the client to receive those extra elements.Documentation*
Check one:
PR Checklist*
cargo fmton default settings.