docs: validate u32 safety for EVM memory operations (ADR-004)#80
Conversation
Changed Files
|
Co-authored-by: tcrypt25519 <212655132+tcrypt25519@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This pull request adds comprehensive documentation (ADR-004) and inline code comments to validate that using u32 for EVM memory operations is safe, addressing potential security concerns about truncating 256-bit stack values. The ADR provides both economic analysis (showing that EVM gas costs make >4GB memory prohibitively expensive at ~$3.7 billion) and technical validation (demonstrating overflow protection via checked_add).
Changes:
- Added ADR-004 documenting why u32 is safe for EVM memory offsets/sizes with gas cost calculations, overflow protection analysis, and implementation patterns
- Added inline comments in
exec.rs,builtins.rs, andops.rsreferencing ADR-004 at truncation sites
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| docs/adrs/adr-004.md | New ADR documenting economic and technical rationale for u32 safety in memory operations |
| crates/jet_runtime/src/exec.rs | Added comment explaining u32 safety for Context memory fields |
| crates/jet_runtime/src/builtins.rs | Added doc comment to jet_mem_expand explaining u32 safety |
| crates/jet/src/builder/ops.rs | Added doc comment to load_i32 function explaining truncation safety |
| - `crates/jet_runtime/src/builtins.rs`: `jet_mem_expand()` line 430: `checked_add` | ||
| - `crates/jet/src/builder/ops.rs`: `call_mem_expand_checked()` line 1043: return code checking |
There was a problem hiding this comment.
The line numbers cited for key validation code are incorrect. The checked_add call in jet_mem_expand() is on line 433, not line 430. The call_mem_expand_checked() function starts at line 1047, not line 1043. These line numbers should be updated to match the actual code locations.
| - `crates/jet_runtime/src/builtins.rs`: `jet_mem_expand()` line 430: `checked_add` | |
| - `crates/jet/src/builder/ops.rs`: `call_mem_expand_checked()` line 1043: return code checking | |
| - `crates/jet_runtime/src/builtins.rs`: `jet_mem_expand()` line 433: `checked_add` | |
| - `crates/jet/src/builder/ops.rs`: `call_mem_expand_checked()` line 1047: return code checking |
| This pattern is used by: | ||
| - `MLOAD` / `MSTORE` / `MSTORE8` | ||
| - `KECCAK256` | ||
| - `CODECOPY` / `CALLDATACOPY` / `RETURNDATACOPY` | ||
| - `CREATE` / `CREATE2` | ||
| - `CALL` / `CALLCODE` / `DELEGATECALL` / `STATICCALL` | ||
| - `RETURN` / `REVERT` |
There was a problem hiding this comment.
The list of operations that use this pattern includes several opcodes that don't appear to be implemented yet in the codebase (CODECOPY, CALLDATACOPY, CREATE, CREATE2, CALL variants other than CALL, etc.). Consider clarifying that this list includes operations that WILL use this pattern when implemented, or remove unimplemented operations from the list to avoid confusion.
|
@copilot remove the line numbers because it's going to be very difficult to keep them aligned all the time. Update your list of op codes that use this pattern to only include ones that we have implemented. you can put the others in a separate list of future ones that must use the pattern if you want they're currently it's a little misleading. |
Co-authored-by: tcrypt25519 <212655132+tcrypt25519@users.noreply.github.com>
Done. Removed all line numbers and reorganized the opcode list into three sections:
Commit: 0d0bcd6 |
* fix: keccak. * fix: add null checks and return code validation for KECCAK256 (#79) * Initial plan * fix: add null check and return code validation for KECCAK256 - Add null pointer check for ctx in jet_ops_keccak256 - Check return code from jet_ops_keccak256 and propagate errors via Invalid return code - Use minimal unsafe scopes with SAFETY comments per Rust 2024 edition requirements Co-authored-by: tcrypt25519 <212655132+tcrypt25519@users.noreply.github.com> * refactor: consolidate unsafe blocks in jet_ops_keccak256 Consolidate multiple unsafe blocks for accessing ctx fields into a single unsafe block that dereferences ctx once, improving readability while maintaining the same safety guarantees. Co-authored-by: tcrypt25519 <212655132+tcrypt25519@users.noreply.github.com> * docs: clarify safety requirements and add SAFETY comment - Clarify safety documentation for ctx parameter (valid if non-null) - Add SAFETY comment for IntValue::new call for consistency Co-authored-by: tcrypt25519 <212655132+tcrypt25519@users.noreply.github.com> * docs: improve safety documentation clarity - Clarify undefined behavior when result pointer is null - Expand SAFETY comment for IntValue::new to explain the contract Co-authored-by: tcrypt25519 <212655132+tcrypt25519@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: tcrypt25519 <212655132+tcrypt25519@users.noreply.github.com> * chore: CI change 🤖 * docs: validate u32 safety for EVM memory operations (ADR-004) (#80) * Initial plan * docs: add ADR-004 explaining why u32 is safe for EVM memory operations Co-authored-by: tcrypt25519 <212655132+tcrypt25519@users.noreply.github.com> * docs: remove line numbers and clarify implemented opcodes in ADR-004 Co-authored-by: tcrypt25519 <212655132+tcrypt25519@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: tcrypt25519 <212655132+tcrypt25519@users.noreply.github.com> --------- Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com> Co-authored-by: tcrypt25519 <212655132+tcrypt25519@users.noreply.github.com>
* fix: keccak. * fix: add null checks and return code validation for KECCAK256 (#79) * Initial plan * fix: add null check and return code validation for KECCAK256 - Add null pointer check for ctx in jet_ops_keccak256 - Check return code from jet_ops_keccak256 and propagate errors via Invalid return code - Use minimal unsafe scopes with SAFETY comments per Rust 2024 edition requirements Co-authored-by: tcrypt25519 <212655132+tcrypt25519@users.noreply.github.com> * refactor: consolidate unsafe blocks in jet_ops_keccak256 Consolidate multiple unsafe blocks for accessing ctx fields into a single unsafe block that dereferences ctx once, improving readability while maintaining the same safety guarantees. Co-authored-by: tcrypt25519 <212655132+tcrypt25519@users.noreply.github.com> * docs: clarify safety requirements and add SAFETY comment - Clarify safety documentation for ctx parameter (valid if non-null) - Add SAFETY comment for IntValue::new call for consistency Co-authored-by: tcrypt25519 <212655132+tcrypt25519@users.noreply.github.com> * docs: improve safety documentation clarity - Clarify undefined behavior when result pointer is null - Expand SAFETY comment for IntValue::new to explain the contract Co-authored-by: tcrypt25519 <212655132+tcrypt25519@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: tcrypt25519 <212655132+tcrypt25519@users.noreply.github.com> * chore: CI change 🤖 * docs: validate u32 safety for EVM memory operations (ADR-004) (#80) * Initial plan * docs: add ADR-004 explaining why u32 is safe for EVM memory operations Co-authored-by: tcrypt25519 <212655132+tcrypt25519@users.noreply.github.com> * docs: remove line numbers and clarify implemented opcodes in ADR-004 Co-authored-by: tcrypt25519 <212655132+tcrypt25519@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: tcrypt25519 <212655132+tcrypt25519@users.noreply.github.com> --------- Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com> Co-authored-by: tcrypt25519 <212655132+tcrypt25519@users.noreply.github.com>
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.