Skip to content

Suggested amendments for PR #8569: [Storehouse] 001 - Add payloadless node and trie implementation#8599

Merged
AlexHentschel merged 3 commits into
leo/payloadlessfrom
alex/payloadless-suggestions
Jul 9, 2026
Merged

Suggested amendments for PR #8569: [Storehouse] 001 - Add payloadless node and trie implementation#8599
AlexHentschel merged 3 commits into
leo/payloadlessfrom
alex/payloadless-suggestions

Conversation

@AlexHentschel

@AlexHentschel AlexHentschel commented Jul 8, 2026

Copy link
Copy Markdown
Member

This PR description is AI-authored (High-Assurance Audit Agent, via Cursor). The code and documentation changes were reviewed and hand-polished line-by-line by @AlexHentschel.

Status: draft

Opened as a draft on purpose. The code-level changes are reviewed and I am happy with them, but the motivation behind them deserves a fuller write-up — this description is a first pass at that motivation, to be refined.

Summary

Suggested amendments stacked directly on leo/payloadless (the head of #8569). The central change improves encapsulation and separation of concerns for constructing and re-leveling compact leaves, so the trie update logic no longer has to reason whether internal implementation details of node represent allocated- vs unallocated-registers. Also added several documentation and spec refinements with node/trie unit tests for the code I modified.

Encapsulating leaf re-leveling

In #8569, moving an existing compact leaf to a new height uses NewLeafWithHash (node.go L96–106). Its contract places an unchecked requirement on the caller: leafHash must be HashLeaf(path, originalValue). Unlike NewLeaf (node.go L72), which handles nil/empty values internally, NewLeafWithHash does not — so the higher-level trie code must itself distinguish allocated vs unallocated registers and select the right constructor. That created the need for additional if branching in trie code (further driving complexity): trie.go L294 and trie.go L348.

Consequences:

  • Separation-of-concerns leak: leaf-representation rules bleed into the trie update logic, increasing the mental load on engineers reading/maintaining update.
  • Fragility: a future caller passing HashLeaf(path, []) to NewLeafWithHash would silently mint a non-default leaf for an empty value — an invalid leaf by our construction convention. Today's callers avoid this, but the guard lives on the caller side, not in the constructor.

(Background: internal Slack discussion + PR review comment #8569 (discussion_r3464907588).)

Change: self-contained constructor

Introduce NewRelevelledLeaf(leaf, relevellingHeight) (node.go L188–211): a single constructor that re-levels an existing leaf and performs the consistency checks internally, so valid input ⇒ valid output regardless of whether the leaf represents an allocated or unallocated register. nil input yields nil output (a default leaf must carry the representative path of the register it stands for). The trie then delegates via a single call — trie.go L300–308 — and the caller-side split disappears.

Also included:

Notes for reviewers

…ne docs, add node/trie tests

Encapsulate the "move a compact leaf to a new height" operation inside node.go
(NewRelevelledLeaf) so trie.go business logic no longer branches on the allocated-
vs-unallocated-register distinction; refine node/trie documentation and the mtrie
README Update case taxonomy; add exhaustive node_test.go / trie_test.go coverage.

Co-authored-by: Cursor <cursoragent@cursor.com>
@coderabbitai

coderabbitai Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 7a786285-6db2-41d8-a4f7-a22d95b5178b

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch alex/payloadless-suggestions

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@AlexHentschel AlexHentschel changed the title Suggested amendments for PR [Storehouse] #8569, "[Storehouse] 001 - Add payloadless node and trie implementation" Suggested amendments for PR #8569: [Storehouse] 001 - Add payloadless node and trie implementation Jul 8, 2026
@AlexHentschel AlexHentschel added Documentation Improvements or additions to documentation Performance S-BFT Protocol Team: Issues assigned to the Protocol Pillar. Execution Cadence Execution Team labels Jul 8, 2026
@AlexHentschel AlexHentschel marked this pull request as ready for review July 8, 2026 03:18
@AlexHentschel AlexHentschel requested a review from a team as a code owner July 8, 2026 03:18

@zhangchiqing zhangchiqing left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good to me 👍

Comment thread ledger/complete/payloadless/node.go Outdated
Comment thread ledger/complete/payloadless/trie.go Outdated
AlexHentschel and others added 2 commits July 8, 2026 22:55
Co-authored-by: Leo Zhang <zhangchiqing@gmail.com>
Co-authored-by: Leo Zhang <zhangchiqing@gmail.com>
@AlexHentschel AlexHentschel merged commit d89f3a6 into leo/payloadless Jul 9, 2026
@AlexHentschel AlexHentschel deleted the alex/payloadless-suggestions branch July 9, 2026 05:58
@AlexHentschel AlexHentschel restored the alex/payloadless-suggestions branch July 9, 2026 06:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Documentation Improvements or additions to documentation Execution Cadence Execution Team Performance Protocol Team: Issues assigned to the Protocol Pillar. S-BFT

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants