Skip to content

offset trie improvments - #2496

Open
goodlyrottenapple wants to merge 1 commit into
mainfrom
sam/offset-trie-improvements
Open

offset trie improvments#2496
goodlyrottenapple wants to merge 1 commit into
mainfrom
sam/offset-trie-improvements

Conversation

@goodlyrottenapple

@goodlyrottenapple goodlyrottenapple commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

offset trie fixes/improvements:

  • constructor validates the data forms a tree, i.e. each child has
    exactly one parent and no nodes are unreachable
  • Bound path length at MAX_PATH_NIBBLES as the decoder assumes the
    encoding is always a short string
  • Rework checked_end onto offsets, fixing an out-of-bounds read
  • hashes_ insert is now guarded on the RLP length, enforcing the
    invariant that only hash-referenced nodes get cached in the map

Copilot AI lite review requested due to automatic review settings August 20, 2026 11:38

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR cleans up RLP encoding paths in the offset-format witness trie by making “hash-as-RLP” handling explicit and reusable, reducing ad-hoc memcpy/encode patterns and improving type-safety around fixed-size hash RLP payloads.

Changes:

  • Define HASH_RLP_LEN in terms of KECCAK256_SIZE and introduce hash_rlp_view for fixed-size RLP-encoded hashes.
  • Add DigestView::hash_rlp() and update account leaf code-hash accessors to return a dedicated hash RLP view.
  • Centralize hash RLP encoding/copying via OffsetTrie::encode_rlp(...) overloads and use them at call sites.

Verdict: CORRECT

Generated with Claude Code

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
category/execution/ethereum/db/offset_trie.hpp Introduces hash_rlp_view, adds digest/hash RLP view helpers, and centralizes hash RLP emission via encode_rlp overloads.
category/execution/ethereum/db/offset_trie.cpp Switches account-leaf encoding to use the new encode_rlp(l.code_hash_rlp(), ...) helper instead of manual memcpy.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread category/execution/ethereum/db/offset_trie.cpp

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Clean, semantically-equivalent refactor: hash_rlp_view encodes the fixed 33-byte RLP-hash extent in the type, DigestView::hash_rlp() leans on the DIGEST == 0x80 + KECCAK256_SIZE invariant (guarded by the existing static_assert), and the two new OffsetTrie::encode_rlp overloads collapse three ad-hoc memcpy/shrink sites into one call. One P3 nit inline about an adjacent open-coded storage_root write in AccountLeafView encoding that the same helper could take over.

Verdict: CORRECT

🤖 Generated with Claude Code

@goodlyrottenapple
goodlyrottenapple force-pushed the sam/offset-trie-improvements branch 2 times, most recently from c176f08 to a7b72bc Compare August 24, 2026 08:56
@goodlyrottenapple
goodlyrottenapple force-pushed the sam/offset-trie-improvements branch from a7b72bc to 2ea99fa Compare August 24, 2026 08:57
@goodlyrottenapple
goodlyrottenapple changed the base branch from main to sam/crypto-unify-keccak August 24, 2026 08:57
@goodlyrottenapple
goodlyrottenapple force-pushed the sam/offset-trie-improvements branch 2 times, most recently from babfad7 to e5b92a8 Compare August 24, 2026 09:07
@goodlyrottenapple
goodlyrottenapple force-pushed the sam/crypto-unify-keccak branch 4 times, most recently from 30b3103 to a46eaa3 Compare August 24, 2026 11:07
@goodlyrottenapple
goodlyrottenapple force-pushed the sam/offset-trie-improvements branch from e5b92a8 to 073157e Compare August 24, 2026 11:07
@goodlyrottenapple
goodlyrottenapple force-pushed the sam/offset-trie-improvements branch from 073157e to 0d6d7ff Compare August 24, 2026 13:01
@goodlyrottenapple
goodlyrottenapple force-pushed the sam/offset-trie-improvements branch from 0d6d7ff to 57abbae Compare August 24, 2026 14:56
@Baltoli
Baltoli force-pushed the sam/crypto-unify-keccak branch from 9cdadf2 to d5cde85 Compare August 25, 2026 09:25
@goodlyrottenapple
goodlyrottenapple force-pushed the sam/offset-trie-improvements branch from 57abbae to f07ffab Compare August 25, 2026 12:21
Base automatically changed from sam/crypto-unify-keccak to main August 25, 2026 12:39
@goodlyrottenapple
goodlyrottenapple force-pushed the sam/offset-trie-improvements branch 2 times, most recently from 1593c6d to ceb0ebc Compare August 26, 2026 12:14
mkolosick
mkolosick previously approved these changes Aug 26, 2026
dhil
dhil previously approved these changes Aug 27, 2026
Comment thread category/execution/ethereum/db/offset_trie.cpp Outdated
Comment thread category/execution/ethereum/db/offset_trie.hpp
Comment thread category/execution/ethereum/db/offset_trie.hpp
- constructor validates the data forms a tree, i.e. each child has
  exactly one parent and no nodes are unreachable
- Bound path length at MAX_PATH_NIBBLES as the decoder assumes the
  encoding is always a short string
- Rework checked_end onto offsets, fixing an out-of-bounds read
- hashes_ insert is now guarded on the RLP length, enforcing the
  invariant that only hash-referenced nodes get cached in the map

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@goodlyrottenapple
goodlyrottenapple dismissed stale reviews from dhil and mkolosick via b7056a6 August 28, 2026 14:37
@goodlyrottenapple
goodlyrottenapple force-pushed the sam/offset-trie-improvements branch from ceb0ebc to b7056a6 Compare August 28, 2026 14:37
@mkolosick
mkolosick self-requested a review August 28, 2026 14:57
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.

4 participants