feat: merge-train/avm #19571
Merged
feat: merge-train/avm #19571
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Resolves https://linear.app/aztec-labs/issue/AVM-198/fix-relative-addressing Relative addressing was pretty broken in the fuzzer: we created references to addresses with different base pointers for the same instruction. This made it so the last base pointer "won", rendering all the other references to addresses invalid, triggering more tag mismatch errors. With this fix we execute roughly 50% more opcodes in the same fuzzer iterations. The fix is to have a common base pointer for all instructions in an instruction block, and to generate code aware of that base pointer. When generating bytecode for that instruction block, we write the base pointer first and then we generate the bytecode for the recorded instructions.
Collaborator
Author
Flakey Tests🤖 says: This CI run detected 2 tests that failed, but were tolerated due to a .test_patterns.yml entry. |
Introduces bytecode mutation using the standard `LLVMFuzzerMutate`. We allow the mutated bytecode to expand up to 2x the original size. The mutation itself then utilises the contract upgrade path, this way we do not need to modify other classes or instances that may be used by other enqueued calls. This does require the addition of public data writes as part of the setup to the fuzzer state (that also needs to happen in TS)
IlyasRidhuan
requested review from
dbanks12,
fcarreiro and
sirasistant
as code owners
### ECC Pre-Audit - Normalise infinities Closes [AVM-193](https://linear.app/aztec-labs/issue/AVM-193/normalise-input-infinity-points-to-ecc-add) NOTE: The issue of no operation being set is now addressed in #19471 => this PR only normalises `inf`s so they always have `(0, 0)` coordinates in `ecc.pil`. Though the circuit no longer fails, it does incorrectly set the predicates (i.e. `double = 0` when doubling `inf`), which could be a footgun. In pre-audit it was found that our C++ elliptic curve point class `StandardAffinePoint` accepts different representations of the infinity (`O`) point - this makes sense as in noir we use `(0, 0)` and in BB we use `((P+1)/2, 0)`. This would be fine if any ECC calculations in the AVM _first_ checked `is_inf` and overrided any subsequent coordinate-based operations. But the `ecc.pil` trace sets whether we have a `double` or `add` operation based on coordinates without gating by any `is_inf` checks: ``` bool x_match = p.x() == q.x(); bool y_match = p.y() == q.y(); bool double_predicate = (x_match && y_match); bool add_predicate = (!x_match && !y_match); // If x match but the y's don't, the result is the infinity point when adding; bool infinity_predicate = (x_match && !y_match); ``` Assuming (understandably!) that two infinity points would share the same coordinates, the above works fine, but we can input a BB-standard `O` as `p` and a Noir-standard `O` as `q` from as early as the simulation call without any errors/checks. ~This results in `!x_match && y_match` which means none of `double_predicate`, `add_predicate`, or `infinity_predicate` are true and the circuit falls over. See c9da1ea for repro of this.~ ~Though there is no current flow that throws here, I think it's worth addressing as we can make the circuit fail with what should be valid inputs.~ ### Fix The current approach is to normalise any _input_ infinity points (we already normalise _resulting_ infinity points in `ecc` and `scalar_mul`) at the simulation stage, so events with any infs always have `(0, 0)` coordinates. This means: - No changes required to `ecc.pil` or the `execution.pil` dispatch - No changes to memory reads/writes - The ecc simulator sets `x = 0, y = 0` if the point `is_inf` for standard add and scalar mul events - `ecc_mem.pil` now constrains that `ecc.pil` has `x = 0, y = 0` if the point `is_inf` via the existing lookup (requires 4 more columns) We could alternatively normalise the points in tracegen, but this means we don't use the values emitted in the events, which feels a bit gross. We could also add many gating constraints to `ecc.pil` to handle `inf` edge cases, but this would be expensive and defeats the point a bit of the trace assuming that the input points are on the curve and validated. So this approach felt the smoothest!
Check that all columns are in one graph The only finding is the duplicate of #19557
Adds mutation for contract classses. This requires changing from simple `ContractClass` to `ContractClassWithCommitment` within the fuzzer. We need the commitment so that when the contract class is mutated, we can re-compute: the class id and the contract address.
What it says on the tin.
separator unused
We now just sanity check the operand address: we were sanity checking that we didn't try to overflow via relative but that's actually a valuable thing to test.
This allowed you to tamper with sha256 inputs on rows after start
Collaborator
Author
|
🤖 Auto-merge enabled after 4 hours of inactivity. This PR will be merged automatically once all checks pass. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
BEGIN_COMMIT_OVERRIDE
fix(avm): Fix relative addressing in fuzzer (#19550)
feat(avm): avm fuzzer bytecode mutation (#19378)
chore(avm): there is automatic conversion from uint128_t to FF
chore(avm): ECC pre-audit - normalise infinity points (#19462)
feat(bb-pilcom): single-component graph check (#19578)
feat(avm): contract class mutation (#19498)
chore: support uint128_t in uint256_t construction (#19581)
fix!: remove unused column in update_check.pil (#19557)
fix(avm)!: pre-audit review of context.pil (#19549)
fix(avm): Relax fuzzer memory manager asserts (#19591)
fix!: sha256.pil missing input propagation constraints (#19590)
END_COMMIT_OVERRIDE