Skip to content

Add MIP-8 page commit tests.#27

Merged
pdobacz merged 2 commits into
forks/monad_ninefrom
mip8-page-commit-tests
Jul 17, 2026
Merged

Add MIP-8 page commit tests.#27
pdobacz merged 2 commits into
forks/monad_ninefrom
mip8-page-commit-tests

Conversation

@mijovic

@mijovic mijovic commented Jun 22, 2026

Copy link
Copy Markdown

ENG-1132

🗒️ Description

🔗 Related Issues or PRs

N/A.

✅ Checklist

  • All: Ran fast static checks to avoid unnecessary CI fails, see also Code Standards and Enabling Pre-commit Checks:
    just static
  • All: PR title adheres to the repo standard - it will be used as the squash commit message and should start type(scope):.
  • All: Considered updating the online docs in the ./docs/ directory.
  • All: Set appropriate labels for the changes (only maintainers can apply labels).
  • Tests: Ran mkdocs serve locally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.
  • Tests: For PRs implementing a missed test case, update the post-mortem document to add an entry the list.
  • Ported Tests: All converted JSON/YML tests from ethereum/tests or tests/static have been assigned @ported_from marker.

Cute Animal Picture

Put a link to a cute animal picture inside the parenthesis-->

Greptile Summary

This PR adds a new test file, test_paged_storage_trie.py, that covers the MIP-8 page-based storage commitment primitives page_commit and storage_root_paged. The tests are split into two categories: self-referential known-answer vectors for page_commit (with an appropriate disclaimer in the module docstring) and structural property tests.

  • Known-answer vectors pin six page_commit outputs (single slot, pair, multi-level, full page, sparse even) to catch accidental ISMC regressions, with a clear warning that they must be cross-checked against the Monad reference client.
  • Property tests cover zero-page rejection, length invariants, determinism, slot-value sensitivity, insertion-order independence, multi-page root distinguishability, and a manual keccak-MPT reconstruction against the single-page codepath.

Confidence Score: 5/5

This is a test-only addition with no changes to production code; safe to merge.

The PR adds only a new test file. No production logic is modified. The known-answer vectors carry a clear self-referential disclaimer, and all structural property tests are independently correct. The two pre-existing review thread concerns (the functionally duplicate empty-storage test and the dead < 32 branch in the manual reconstruction) are style/coverage issues already on the author's radar and do not introduce incorrect behaviour.

No files require special attention.

Important Files Changed

Filename Overview
packages/testing/src/execution_testing/forks/tests/test_paged_storage_trie.py New test file adding characterization vectors and property tests for MIP-8 page-based storage commitment; no logic bugs introduced, known limitations flagged in prior review threads.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[_storage helper\nslot→Bytes32 key mapping] --> B[storage_root_paged]
    C[_page helper\noffset→4096-byte page image] --> D[page_commit]

    subgraph "page_commit tests"
        D --> E[test_page_commit_known_vectors\n6 parametrized vectors]
        D --> F[test_page_commit_length_is_32]
        D --> G[test_page_commit_rejects_all_zero_page]
        D --> H[test_page_commit_deterministic]
        D --> I[test_page_commit_sensitive_to_value]
        D --> J[test_page_commit_sensitive_to_offset]
    end

    subgraph "storage_root_paged tests"
        B --> K[test_empty_storage_is_empty_trie_root]
        B --> L[test_storage_root_length_is_32]
        B --> M[test_storage_root_order_independent]
        B --> N[test_storage_root_distinguishes_pages]
        B --> O[test_storage_root_clears_to_empty_when_slots_omitted]
        D --> P[test_single_page_root_matches_manual_reconstruction\nmanual keccak-MPT cross-check]
        B --> P
    end
Loading
%%{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
    A[_storage helper\nslot→Bytes32 key mapping] --> B[storage_root_paged]
    C[_page helper\noffset→4096-byte page image] --> D[page_commit]

    subgraph "page_commit tests"
        D --> E[test_page_commit_known_vectors\n6 parametrized vectors]
        D --> F[test_page_commit_length_is_32]
        D --> G[test_page_commit_rejects_all_zero_page]
        D --> H[test_page_commit_deterministic]
        D --> I[test_page_commit_sensitive_to_value]
        D --> J[test_page_commit_sensitive_to_offset]
    end

    subgraph "storage_root_paged tests"
        B --> K[test_empty_storage_is_empty_trie_root]
        B --> L[test_storage_root_length_is_32]
        B --> M[test_storage_root_order_independent]
        B --> N[test_storage_root_distinguishes_pages]
        B --> O[test_storage_root_clears_to_empty_when_slots_omitted]
        D --> P[test_single_page_root_matches_manual_reconstruction\nmanual keccak-MPT cross-check]
        B --> P
    end
Loading

Reviews (4): Last reviewed commit: "Fix lint in paged storage trie tests" | Re-trigger Greptile

@mijovic
mijovic requested review from QEDK and pdobacz as code owners June 22, 2026 13:21
@mijovic
mijovic changed the base branch from execute-with-eestnet-mip8 to forks/monad_nine June 22, 2026 13:22
Comment on lines +187 to +203
) -> 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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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.

Fix in Claude Code

@mijovic
mijovic changed the base branch from forks/monad_nine to forks/monad_ten June 22, 2026 13:26
@mijovic
mijovic changed the base branch from forks/monad_ten to execute-with-eestnet-mip8 June 22, 2026 13:27
@mijovic
mijovic force-pushed the mip8-page-commit-tests branch from 083df74 to a7da928 Compare June 22, 2026 13:29
@mijovic
mijovic changed the base branch from execute-with-eestnet-mip8 to forks/monad_nine June 22, 2026 13:29

@pdobacz pdobacz left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

@pdobacz

pdobacz commented Jun 23, 2026

Copy link
Copy Markdown
Collaborator

the CI failure, seems like it needs a formatter run:

uv run ruff format src packages tests && uv run ruff check src packages tests --exclude tests/fixtures --fix --unsafe-fixes

is my go-to line for this

just static

to check afterwards

mijovic and others added 2 commits July 17, 2026 12:40
@pdobacz
pdobacz force-pushed the mip8-page-commit-tests branch from ba4e4f3 to 57dbd06 Compare July 17, 2026 12:40
@pdobacz
pdobacz merged commit e38dd53 into forks/monad_nine Jul 17, 2026
5 checks passed
@pdobacz
pdobacz deleted the mip8-page-commit-tests branch July 17, 2026 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants