[Storehouse] 001 - Add payloadless node and trie implementation#8569
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a payloadless Merkle trie with cached nodes, copy-on-write updates, register accounting, proof generation, hashing helpers, extensive tests, and revised MTrie algorithm documentation. ChangesPayloadless trie foundation
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Client
participant MTrie
participant Node
participant BatchProof
Client->>MTrie: Update registers
MTrie->>Node: Rebuild affected paths
Node-->>MTrie: Return cached hashes
Client->>MTrie: Request proofs
MTrie->>BatchProof: Populate inclusion and sibling hashes
BatchProof-->>Client: Return proof batch
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
| hasValue := len(values[i]) > 0 | ||
| var newLeafHash hash.Hash | ||
| if hasValue { | ||
| newLeafHash = hash.HashLeaf(hash.Hash(paths[i]), values[i]) |
There was a problem hiding this comment.
leaf hash is computed here to be shared in the next if and else branches
| root *node.Node | ||
| root *Node | ||
| regCount uint64 // number of registers allocated in the trie | ||
| regSize uint64 // size of registers allocated in the trie |
There was a problem hiding this comment.
regSize is removed from the trie, because we won't be able to know the actual size. When updating an existing register, we know the new size, but we don't know the old size, so we can't update the regSize with correct value. Given that, this field is removed. Currently the regSize is used by metrics reporting only.
| // allocatedRegCountDelta and allocatedRegSizeDelta should both be 0 | ||
| return n, 0, 0, nodeHeight | ||
| // no compact leaf node from its ancester, it means we are storing a value on a new path, | ||
| n = NewLeaf(paths[0], values[0], nodeHeight) |
There was a problem hiding this comment.
Why DeepCopy is not needed here?
The original mtrie needs DeepCopy to protect the payload value slice from being mutated. But for the payloadless trie, the leaf hash is computed from the payload value (HashLeaf), so the leaf no longer reference any shared byte slices with the caller. A DeepCopy would be wasteful.
There was a problem hiding this comment.
Isn't it a problem that LeafHash() returns a *hash.Hash which then gets publically surfaced via MTrie.ReadSingleLeafHash and MTrie.UnsafeRead. should we coppy the hash when returning from LeafHash().
There was a problem hiding this comment.
Good flag, beyond discussing it in the PR a comment in the code is useful. I have added respective documentation to the NewLeaf constructor:
flow-go/ledger/complete/payloadless/node.go
Lines 125 to 128 in d89f3a6
There was a problem hiding this comment.
The defensive copy (copyLeafHash) is added to the forest.
This is better than adding the copy in trie's ReadSingleLeafHash. Because 1) the execution node interact with trie through the forest, which has the defensive copy. 2) the trie's API is lower level, and other methods like "UnsafeRead" is also under a "do no modify" contract".
But it's a good flag. I will add comments to the ReadSingleLeafHash as well as the AllLeafHashes (which isn't being used anywhere) to highlight they should not be modified.
| // for retrieving the original value separately if needed. | ||
| type PayloadlessTrieProof struct { | ||
| Path Path // path | ||
| LeafHash *hash.Hash // leaf hash HashLeaf(path, value); nil for empty leaves and non-inclusion proofs |
There was a problem hiding this comment.
The PayloadlessTrieProof is copied from ledger/trie.go#TrieProof.
The difference is the Payload field is changed to LeafHash field
zhangchiqing
requested review from
AlexHentschel and
janezpodhostnik
and removed request for
a team
zhangchiqing
changed the title
zhangchiqing
changed the title
| // is moved to a higher position in the trie. | ||
| // UNCHECKED requirement: fromHeight < toHeight | ||
| // UNCHECKED requirement: fromHeight >= 0 | ||
| func ExtendHashToHeight(path hash.Hash, baseHash hash.Hash, fromHeight, toHeight int) hash.Hash { |
There was a problem hiding this comment.
fromHeight looks like an unnecessary parameter
|
|
||
| // NewMTrie returns a Mtrie given the root | ||
| func NewMTrie(root *node.Node, regCount uint64, regSize uint64) (*MTrie, error) { | ||
| func NewMTrie(root *Node, regCount uint64) (*MTrie, error) { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
zhangchiqing
force-pushed
the
leo/payloadless-base
branch
from
5cbb377 to
c51de07
Compare
zhangchiqing
force-pushed
the
leo/payloadless
branch
from
dd66c68 to
f42a6d8
Compare
…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>
Co-authored-by: Leo Zhang <zhangchiqing@gmail.com>
Co-authored-by: Leo Zhang <zhangchiqing@gmail.com>
Suggested amendments for PR #8569: `[Storehouse] 001 - Add payloadless node and trie implementation`
There was a problem hiding this comment.
Thanks. Very happy with the result.
Disclaimer: some portion of changes in this PR has been introduced by my suggested changes (see my merged changed #8599 targeting the branch of this PR). For other reviewers, I have provided a variety of comments motivating and explaining my changes; hope that simplifies further reviews.
@janez if you were happy with Leo's PR before, I pretty much didn't change any behaviour, just documentation and encapsulation of logic.
Only a single minor nit-pick for Leo to address:
👉 my comment #8569 (comment)
All other comments I left are purely informational to explain my changes.
Happy for this to be merged. Thanks for your patience Leo 🙇
| // allocatedRegCountDelta and allocatedRegSizeDelta should both be 0 | ||
| return n, 0, 0, nodeHeight | ||
| // no compact leaf node from its ancester, it means we are storing a value on a new path, | ||
| n = NewLeaf(paths[0], values[0], nodeHeight) |
There was a problem hiding this comment.
Good flag, beyond discussing it in the PR a comment in the code is useful. I have added respective documentation to the NewLeaf constructor:
flow-go/ledger/complete/payloadless/node.go
Lines 125 to 128 in d89f3a6
| if len(paths) == 1 { | ||
| // check if the only path to update has the same payload. | ||
| // if payload is the same, we could skip the update to avoid creating duplicated node | ||
| if !currentNode.Payload().ValueEquals(&payloads[i]) { | ||
| n = node.NewLeaf(paths[i], payloads[i].DeepCopy(), nodeHeight) | ||
|
|
||
| allocatedRegCountDelta, allocatedRegSizeDelta = | ||
| computeAllocatedRegDeltas(currentNode.Payload(), &payloads[i]) | ||
|
|
||
| return n, allocatedRegCountDelta, allocatedRegSizeDelta, nodeHeight | ||
| } | ||
| // avoid creating a new node when the same payload is written | ||
| return currentNode, 0, 0, nodeHeight | ||
| } |
There was a problem hiding this comment.
This is one of the few algorithmic changes I made in my PR #8599 (already applied here).
Essentially I have moved this code block outside of the loop, for logical clarity. Less checks of the unchanging condition len(paths) == 1 is a nice side effect but was not the goal of the change. Moving the code block is purely aesthetically (hopefully meaningfully improving clarity in conjunction with the extended documentation) - the algorithmic behaviour is entirely unchanged in regards to to the conditions handled by this code block.
The if len(paths) == 1 condition is now paired with requiring paths[0] == currentPath which is true if and only if the code block lines 426 - 439 was reached in the old version 👇 [new version]
flow-go/ledger/complete/payloadless/trie.go
Lines 332 to 344 in d89f3a6
| found = true | ||
|
|
||
| allocatedRegCountDelta, allocatedRegSizeDelta = | ||
| computeAllocatedRegDeltasFromHigherHeight(currentNode.Payload()) | ||
|
|
||
| break |
There was a problem hiding this comment.
The iteration over paths and the check whether p is found in paths is contained has now been consolidated into
flow-go/ledger/complete/payloadless/trie.go
Lines 348 to 361 in d89f3a6
Previously, those logic conceptually belonging together was more separated in the old code version.
| n = node.NewInterimNode(nodeHeight, newLeftChild, newRightChild) | ||
| return n, allocatedRegCountDelta, allocatedRegSizeDelta, lowestHeightTouched | ||
| } | ||
|
|
||
| // computeAllocatedRegDeltasFromHigherHeight returns the deltas | ||
| // needed to compute the allocated reg count and reg size when | ||
| // a payload is updated or unallocated at a lower height. | ||
| func computeAllocatedRegDeltasFromHigherHeight(oldPayload *ledger.Payload) (allocatedRegCountDelta, allocatedRegSizeDelta int64) { | ||
| if !oldPayload.IsEmpty() { | ||
| // Allocated register will be updated or unallocated at lower height. | ||
| allocatedRegCountDelta-- | ||
| } | ||
| oldPayloadSize := oldPayload.Size() | ||
| allocatedRegSizeDelta -= int64(oldPayloadSize) | ||
| return | ||
| n = NewInterimNode(nodeHeight, newLeftChild, newRightChild) | ||
| return n, allocatedRegCountDelta, lowestHeightTouched | ||
| } | ||
|
|
||
| // computeAllocatedRegDeltas returns the allocated reg count | ||
| // and reg size deltas computed from old payload and new payload. | ||
| // PRECONDITION: !oldPayload.Equals(newPayload) | ||
| func computeAllocatedRegDeltas(oldPayload, newPayload *ledger.Payload) (allocatedRegCountDelta, allocatedRegSizeDelta int64) { | ||
| allocatedRegCountDelta = 0 | ||
| if newPayload.IsEmpty() { | ||
| // Old payload is not empty while new payload is empty. | ||
| // Allocated register will be unallocated. | ||
| allocatedRegCountDelta = -1 | ||
| } else if oldPayload.IsEmpty() { | ||
| // Old payload is empty while new payload is not empty. | ||
| // Unallocated register will be allocated. | ||
| allocatedRegCountDelta = 1 | ||
| } | ||
|
|
||
| oldPayloadSize := oldPayload.Size() | ||
| newPayloadSize := newPayload.Size() | ||
| allocatedRegSizeDelta = int64(newPayloadSize - oldPayloadSize) | ||
| return |
There was a problem hiding this comment.
consolidated into computeAllocatedRegCountDelta method
There was a problem hiding this comment.
The AI applied this clarification also to this file, increasing change surface. If that results in conflicts we could drop the changes here and re-apply them in the end. @zhangchiqing
| // | ||
| // The function extends the leaf hash from height 0 to nodeHeight by hashing upward | ||
| // through the trie structure, combining with default hashes at each level. | ||
| func ComputeCompactValueFromLeafHash(path hash.Hash, leafHash hash.Hash, nodeHeight int) hash.Hash { |
There was a problem hiding this comment.
| func ComputeCompactValueFromLeafHash(path hash.Hash, leafHash hash.Hash, nodeHeight int) hash.Hash { | |
| // TODO: tentative method; should eventually be replaced by node-provided methods, as the node as | |
| // the node technically defined how to compute hashes, and we should encapsulate the logic there. | |
| func ComputeCompactValueFromLeafHash(path hash.Hash, leafHash hash.Hash, nodeHeight int) hash.Hash { |
| @@ -354,120 +253,146 @@ func NewTrieWithUpdatedRegisters( | |||
| // updateResult is a wrapper of return values from update(). | |||
| // It's used to communicate values from goroutine. | |||
| type updateResult struct { | |||
There was a problem hiding this comment.
my PR #8599 extended the documentation of this method. When reviewing, you'll see some changes, but none of them change the higher-level behaviour of the update method (at least the changes should not 😅 ).
My main goal was to increase encapsulation, by moving everything hash-computation related into node, which already is the authority on how the leaf hashes are computed in detail.
| return i | ||
| } | ||
|
|
||
| func minInt(a, b int) int { |
There was a problem hiding this comment.
now provided natively by go
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
This comment has been minimized.
This comment has been minimized.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ledger/complete/payloadless/node.go`:
- Around line 384-391: Update the Node.Path method to guard against a nil
receiver before accessing n.path; return nil when n is nil or not a leaf, while
preserving the existing pointer return for leaf nodes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0c785d32-b9fa-43a7-976f-aa6139361cc6
📒 Files selected for processing (10)
ledger/common/hash/sha3.goledger/common/hash/sha3_internal_test.goledger/complete/mtrie/README.mdledger/complete/mtrie/trie/trie_test.goledger/complete/payloadless/node.goledger/complete/payloadless/node_test.goledger/complete/payloadless/trie.goledger/complete/payloadless/trie_test.goledger/payloadless_proof.goledger/trie.go
| // Path returns a pointer to the Node's register storage path. | ||
| // If the node is not a leaf, the function returns `nil`. | ||
| func (n *Node) Path() *ledger.Path { | ||
| if n.IsLeaf() { | ||
| return &n.path | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Path() panics on nil receiver despite IsLeaf() being nil-safe.
IsLeaf() returns true for a nil node, but Path() then dereferences n.path, causing a nil pointer panic. All current callers check for nil before calling Path(), but the nil-safety of IsLeaf() creates a trap for future callers.
🛡️ Proposed fix: add nil guard to `Path()`
func (n *Node) Path() *ledger.Path {
- if n.IsLeaf() {
+ if n == nil || !n.IsLeaf() {
+ return nil
+ }
+ if n.IsLeaf() {
return &n.path
}
return nil
}Or more concisely:
func (n *Node) Path() *ledger.Path {
- if n.IsLeaf() {
+ if n == nil || n.lChild != nil || n.rChild != nil {
+ return nil
+ }
+ if n.IsLeaf() {
return &n.path
}
return nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Path returns a pointer to the Node's register storage path. | |
| // If the node is not a leaf, the function returns `nil`. | |
| func (n *Node) Path() *ledger.Path { | |
| if n.IsLeaf() { | |
| return &n.path | |
| } | |
| return nil | |
| } | |
| // Path returns a pointer to the Node's register storage path. | |
| // If the node is not a leaf, the function returns `nil`. | |
| func (n *Node) Path() *ledger.Path { | |
| if n == nil || !n.IsLeaf() { | |
| return nil | |
| } | |
| if n.IsLeaf() { | |
| return &n.path | |
| } | |
| return nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ledger/complete/payloadless/node.go` around lines 384 - 391, Update the
Node.Path method to guard against a nil receiver before accessing n.path; return
nil when n is nil or not a leaf, while preserving the existing pointer return
for leaf nodes.
4 participants
This PR adds the payloadless node and trie implementation. The base branch already contains a copy of the mtrie code in the payloadless package, so this PR's diff shows only the payloadless-specific changes, making review easier.
Test cases are created in #8572
Find Spec
Summary by CodeRabbit