Add MIP-8 page commit tests.#27
Conversation
| ) -> None: | ||
| """``storage_root_paged`` matches a keccak-MPT built by hand. | ||
|
|
||
| Cross-checks the trie-assembly layer for a single page (page 0): slot | ||
| grouping, the ``keccak256(page_index)`` trie key, and the | ||
| ``rlp.encode(commitment)`` leaf framing — independently of the BLAKE3 | ||
| internals of ``page_commit``. | ||
| """ | ||
| commitment = page_commit(_page(slot_values)) | ||
| key = keccak256(U256(0).to_be_bytes32()) | ||
| obj = {bytes_to_nibble_list(key): rlp.encode(commitment)} | ||
|
|
||
| root_node = encode_internal_node(patricialize(obj, Uint(0))) | ||
| encoded = rlp.encode(root_node) | ||
| expected = keccak256(encoded) if len(encoded) < 32 else Hash32(root_node) | ||
|
|
||
| assert storage_root_paged(_storage(slot_values)) == expected |
There was a problem hiding this comment.
if len(encoded) < 32 branch is dead for all parametrized cases
For a single-page leaf the MPT root_node encodes to a [hex-prefix-path, rlp(commitment)] structure where the path alone is 33 bytes (0x20 || 32-byte keccak key) and the value is 33 bytes. rlp.encode(root_node) is therefore always well above 32 bytes, so the keccak256(encoded) path on line 200 is never exercised by this test. The assertion on line 203 ends up testing only the else / Hash32(root_node) branch. This is consistent with what the production code does for these inputs, but if the threshold condition was accidentally inverted in either place the test would not catch it.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/testing/src/execution_testing/forks/tests/test_paged_storage_trie.py
Line: 187-203
Comment:
**`if len(encoded) < 32` branch is dead for all parametrized cases**
For a single-page leaf the MPT `root_node` encodes to a `[hex-prefix-path, rlp(commitment)]` structure where the path alone is 33 bytes (`0x20` || 32-byte keccak key) and the value is 33 bytes. `rlp.encode(root_node)` is therefore always well above 32 bytes, so the `keccak256(encoded)` path on line 200 is never exercised by this test. The assertion on line 203 ends up testing only the `else` / `Hash32(root_node)` branch. This is consistent with what the production code does for these inputs, but if the threshold condition was accidentally inverted in either place the test would not catch it.
How can I resolve this? If you propose a fix, please make it concise.083df74 to
a7da928
Compare
pdobacz
left a comment
There was a problem hiding this comment.
LGTM, this is a great addition to the tests of the mEELS implementation of MIP-8, but note it is not a new spec test (the remaining 3 points from your list you've sent me seem to be new spec tests)
|
the CI failure, seems like it needs a formatter run: is my go-to line for this to check afterwards |
🗒️ Description
🔗 Related Issues or PRs
N/A.
✅ Checklist
just statictype(scope):.mkdocs servelocally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.@ported_frommarker.Cute Animal Picture
Greptile Summary
This PR adds a new test module for MIP-8 paged storage commitment, covering both
page_commitandstorage_root_pagedfromethereum.paged_storage_trie.page_commitdigests are included with a clear warning that they were generated from the spec itself and must be cross-checked against the Monad reference client before being treated as canonical.test_single_page_root_matches_manual_reconstructionrebuilds the keccak-MPT root by hand and compares it tostorage_root_paged, independently verifying slot grouping, trie key derivation, andrlp.encode(commitment)leaf framing.Confidence Score: 5/5
Test-only addition; no production logic is changed and the new tests are consistent with the existing paged_storage_trie implementation.
The test helpers, known-answer vectors, and property assertions all match the production code exactly. Two pre-existing comment threads cover the two noted test gaps; no new correctness issues were found.
No files require special attention.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD subgraph Helpers H1["_page(slot_values)"] H2["_storage(slot_values)"] end subgraph page_commit Tests PC1["known_vectors"] PC2["length_is_32"] PC3["rejects_all_zero_page"] PC4["deterministic"] PC5["sensitive_to_value"] PC6["sensitive_to_offset"] end subgraph storage_root_paged Tests SR1["empty_storage_is_empty_trie_root"] SR2["storage_root_length_is_32"] SR3["order_independent"] SR4["distinguishes_pages"] SR5["clears_to_empty"] SR6["single_page_manual_reconstruction"] end H1 --> PC1 & PC2 & PC3 & PC4 & PC5 & PC6 & SR6 H2 --> SR2 & SR3 & SR4 & SR5 & SR6 PC1 & SR6 --> page_commit["page_commit() BLAKE3 ISMC"] SR1 & SR2 & SR3 & SR4 & SR5 & SR6 --> storage_root_paged["storage_root_paged() keccak-MPT"]%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% flowchart TD subgraph Helpers H1["_page(slot_values)"] H2["_storage(slot_values)"] end subgraph page_commit Tests PC1["known_vectors"] PC2["length_is_32"] PC3["rejects_all_zero_page"] PC4["deterministic"] PC5["sensitive_to_value"] PC6["sensitive_to_offset"] end subgraph storage_root_paged Tests SR1["empty_storage_is_empty_trie_root"] SR2["storage_root_length_is_32"] SR3["order_independent"] SR4["distinguishes_pages"] SR5["clears_to_empty"] SR6["single_page_manual_reconstruction"] end H1 --> PC1 & PC2 & PC3 & PC4 & PC5 & PC6 & SR6 H2 --> SR2 & SR3 & SR4 & SR5 & SR6 PC1 & SR6 --> page_commit["page_commit() BLAKE3 ISMC"] SR1 & SR2 & SR3 & SR4 & SR5 & SR6 --> storage_root_paged["storage_root_paged() keccak-MPT"]Reviews (2): Last reviewed commit: "Add MIP-8 page commit tests." | Re-trigger Greptile