feat: merge-train/avm #19377
Merged
feat: merge-train/avm #19377
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
This PR * Makes BB's `info`/`vinfo`/etc customizable by a logging function * Updates the TS side of CPP simulation to optionally take a logger function. The TS-side of the CPP simulator now uses the pino logger.  I've also refactored the `avm_simulate_napi.cpp` file to avoid referencing the parameter indices many times, and using them in a very "local" way. Otherwise it was getting a bit dangerous that we would use some magic index in places many lines away. This shouldn't cause any overhead if logging is not active (as expected in block building, because ANY logging might take a `ms` - e.g., showing the transaction hash). If you are doing any logging, you will pay dearly. Closes [AVM-182](https://linear.app/aztec-labs/issue/AVM-182/optionally-use-ts-logger-in-c-simulation).
AztecBot
requested review from
IlyasRidhuan,
dbanks12,
fcarreiro,
jeanmon and
sirasistant
as code owners
Fixes [AVM-165](https://linear.app/aztec-labs/issue/AVM-165/instruction-fetching-error-soundness-bug). The fix was just to uncomment a line.
Biggest change is that now the calldata providers should never throw. I think this was already the case (an OOB memory access gives you `FF::zero()`). Also, the providers will only be used when callstack collection is on, which shouldnt be in prod. Related to [AVM-171](https://linear.app/aztec-labs/issue/AVM-171/vm2-move-assert-bb-assert).
Finish moving usages of ContractsDbProxy to instruction mutators, ready for removal
Adds mutation for enqueued calls including teardown. Also restructuring and cleaning up the `tx_data` file
`BB_ASSERT` or `BB_ASSERT_DEBUG` was used depending on whether the code was in the hot path. The biggest semantic change is that `assert` crashes, and `BB_ASSERT*` throws. I made a change in the assertion macros because the previous implementation was potentially making copies of the input objects. The original implementation was (probably) chosen so that when the inputs where expressions (e.g., `obj + 7`) they were only evaluated once. This is reasonable. But... when the input is not an expression a copy might be made. The chosen solution was to use `const auto&`, which would in general be dangerous but it works for temporary values (like the expression) thanks to the C++ feature called "lifetime extension" for this very particular case (`const&`). The change was discussed with Adam. * https://en.cppreference.com/w/cpp/language/reference_initialization.html#Lifetime_of_a_temporary * https://stackoverflow.com/questions/39718268/why-do-const-references-extend-the-lifetime-of-rvalues Fixes [AVM-171](https://linear.app/aztec-labs/issue/AVM-171/vm2-move-assert-bb-assert).
… sha256 mem PILs (#19367) Found by claude
This PR moves us from `compute_calldata_hash` -> `assert_calldata_hash` which essentially adds a degree of validation to the calldata hash in the AVM cpp code. ### Why tho? 1. Aligns it with other "infallible" pre-requisites to proving, e.g., `assert_address_derivation`. 2. Somewhat minimises (although not completely) the following prover DOS vector. - A sequencer generates proving hints with a bad calldata hash (i.e. `H(calldata) != calldata_hash`) - Gives the hints to a prover agent, since there is no validation the prover agent constructs the entire trace. - The proof construction fails because the lookup constraining calldata hash is invalid, all the prover work is wasted 3. It only minimises the attack vector because the prover agent still wastes some work during re-execution, although this should be significantly less than when it gets to tracegen. ### Sharp Edges ~In TS, cd hash validation is done at the tx validation level. I don't think it is worthwhile to implement the cd hash validation in the TS simulator - so instead I've implemented a validation in the TS fuzzer entrypoint~
…ints (#19371) Instead of making every audit PR bigger, I thought it would be helpful to knock many of these all out at once. This also standardizes the comment formats a bit for boolean selectors.
And annotated some explicitly constrained booleans in these files while i was at it. Also we had a duplicated boolean constraint in keccak_mem
…d a misnamed relation (#19404) due to typo where addr was constrained instead of size
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
feat(avm)!: optionally use TS logger in C++ simulation (#19305)
chore(avm): bytecode caching comments
chore(avm): disable VK hash checking in tests
fix(avm)!: instr_fetching soundness bug (#19381)
fix(avm): dont catch wide exceptions (#19388)
refactor(avm): Refactor get contract instance fuzzer backfill (#19387)
feat(avm): mutate enqueued calls (#19315)
chore(avm): migrate to BB asserts (#19395)
fix!: more missing boolean constraints in calldata, calldata hashing, sha256 mem PILs (#19367)
feat(avm): defensively assert cd hashes (#19346)
chore: annotate booleans in PIL, and add some missing boolean constraints (#19371)
fix!: missing boolean constraints on zero checks targets (#19401)
fix!: context did not constrain returndata size to 0 at start, and had a misnamed relation (#19404)
END_COMMIT_OVERRIDE