docs: avm docs#19603
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Flakey Tests🤖 says: This CI run detected 2 tests that failed, but were tolerated due to a .test_patterns.yml entry. |
fcarreiro
left a comment
fcarreiro
left a comment
There was a problem hiding this comment.
This is REALLY REALLY good! I think you should merge it.
You can still pass it around and people can comment. Someone else can take over and make the changes.
| @@ -0,0 +1,79 @@ | |||
| # Aztec Virtual Machine (AVM) | |||
There was a problem hiding this comment.
I feel this is a bit too defensive. It's like "I'm telling you why a VM and not circuits", while it could be "hello! this is the AVM, by the way, no circuits because of XX". By now I think L2s with VMs are quite standard and understood.
There was a problem hiding this comment.
Left this as-is for now, but we can rethink. Honestly I pasted verbatim from the technical whitepaper. I personally think this content is useful somewhere, but maybe it doesn't need to be multiple paragraphs in the intro.
| - `argsOffset` — Memory offset where arguments start | ||
| - `argsSize` — Number of field elements to pass | ||
|
|
||
| The callee receives this memory region as its calldata. |
There was a problem hiding this comment.
The types are reinterpreted as FIELDS.... are they? (now I dont remember)
| - `retOffset` — Memory offset where return data starts | ||
| - `retSize` — Number of field elements to return | ||
|
|
||
| On `REVERT`, the same parameters specify revert data. |
There was a problem hiding this comment.
Maybe a note here (or in the ISA, or both) that this only SETS the params, but does not try to access or copy the returndata. in particular this means that setting this to some OOB memory location access will not fail here, but on the caller (which, thinking about it, is actually quite bad from a programming/defensive PoV...)
There was a problem hiding this comment.
Oof honestly I don't think I realized that. That's a bummer. Ideally the fact that returndata really lives in child's memory should be a circuit/cpp implementation detail. IMO both TS and C++ implementations aren't great. maybe they should really both just pad with 0s for memory OOB like they do if you copy a size larger than the calldata/returndata.
There was a problem hiding this comment.
This means we need to officially define calldata/returndata as living in the parent's/child's memory in the docs 😢
There was a problem hiding this comment.
And if it would be an out-of-bounds access, but it is also a read past the end of calldata/returndata size, it's NOT an OOB error.....
There was a problem hiding this comment.
I'm documenting it as the C++ and PIL implement it. I updated the opcode pages and calldata-returndata.md
|
we really need to revisit terms like revert vs rollback, revert vs error, exception / exceptional, halt,.... We need to define these terms and then have claude change the docs to use them consistently. My proposal:
|
BEGIN_COMMIT_OVERRIDE feat(avm): contract instance mutation (#19499) fix(avm): Fix note hash exists fuzzing (#19616) fix(avm): Build trace on coverage prover runs (#19627) chore(avm): Use PC alias type consistently (#19625) feat(avm): mutate global gas fees and timestamp (#19500) docs: avm docs (#19603) fix(avm): Increase chances of fuzzer finding limits (#19656) fix(avm)!: de-risk memory injection attacks (#19620) fix(avm): Fix TS ECC add infinity handling (#19657) fix(avm): Fix jumpif in fuzzer (#19655) feat(avm): protocol contractg mutations (#19586) chore(avm): analyze fuzzer corpus distribution (#19614) feat(avm): fuzzer treats enqueued call size as coverage (#19615) refactor(avm): Refactor calldata copy and return data copy fuzzing (#19666) feat(avm): boundary values for mutations (#19617) END_COMMIT_OVERRIDE
3 participants
Review via github's markdown viewer: https://github.com/AztecProtocol/aztec-packages/blob/db/avm-docs/yarn-project/simulator/docs/avm/README.md