[Storehouse] 001 - Part 2 Test cases#8609
Conversation
[Storehouse] 001 Part2 - Add payloadless trie test cases
📝 WalkthroughWalkthroughAdds extensive tests for payloadless trie roots, updates, pruning, reads, full-trie equivalence, JSON output, interim-node hashing, and single and batch proof behavior. ChangesPayloadless trie validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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 |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
ledger/complete/payloadless/trie_test.go (1)
610-621: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueOff-by-one in removal sampling loop.
i > samplesNeededallows one extra iteration beyond the intended count. WhensamplesNeededis 3, the loop executes 4 removals (i = 0, 1, 2, 3) before breaking. This is a long-running smoke test so the impact is low, but the logic should use>=for correctness.♻️ Proposed fix
- if i > samplesNeeded { + if i >= samplesNeeded { break }🤖 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/trie_test.go` around lines 610 - 621, Fix the off-by-one error in the removal sampling loop by changing the break condition after incrementing i from i > samplesNeeded to i >= samplesNeeded, ensuring exactly samplesNeeded paths are removed. Update the loop in the test that builds updatePaths and updateValues.
🤖 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/trie_test.go`:
- Around line 82-85: Move require.NoError(t, err) immediately after the
NewTrieWithUpdatedRegisters call and before any assertions dereferencing
leftPopulatedTrie, including AllocatedRegCount(), so errors are reported without
a nil-pointer panic.
- Line 804: Update the assertion in the relevant no-pruning subtest to compare
rootHash with updatedTrieNoPruning.RootHash(), replacing the incorrect
updatedTrie reference so the test validates the trie being modified and
re-populated.
- Around line 363-369: Update the multiset equality assertion after sorting in
the affected test so it compares every element of paths with sortedPaths,
including indices 0 through index-1; replace the loop starting at index with a
full-range comparison while preserving the existing length assumptions and
assert.Equal usage.
---
Nitpick comments:
In `@ledger/complete/payloadless/trie_test.go`:
- Around line 610-621: Fix the off-by-one error in the removal sampling loop by
changing the break condition after incrementing i from i > samplesNeeded to i >=
samplesNeeded, ensuring exactly samplesNeeded paths are removed. Update the loop
in the test that builds updatePaths and updateValues.
🪄 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: 3478c3f8-6b69-4e79-a89a-b1abd5ca5028
📒 Files selected for processing (5)
ledger/complete/payloadless/equivalence_test.goledger/complete/payloadless/json_test.goledger/complete/payloadless/node_test.goledger/complete/payloadless/trie_test.goledger/payloadless_proof_test.go
AlexHentschel
left a comment
AlexHentschel
left a comment
There was a problem hiding this comment.
Looks good.
- suggested the addition of clarifying Ascii diagrams in a few places
- AI identified some untested side-cases and proposed extension of tests (these were mostly untested already in the prior version of mTrie). The extension should be straight forward for an AI to implement 🤞 .
Overall, nice work consolidating the tests.
| // (old value empty and new value present) | ||
| updatedTrieNoPruning, maxDepthTouched, err = payloadless.NewTrieWithUpdatedRegisters(updatedTrieNoPruning, paths, values, false) | ||
| require.NoError(t, err) | ||
| require.Equal(t, rootHash, updatedTrie.RootHash()) |
There was a problem hiding this comment.
This assertion re-checks updatedTrie, which is unchanged since line 739 — so it is trivially true, and the actual property (re-allocating the removed registers restores the original root hash on the no-pruning trie) is unverified.
| require.Equal(t, rootHash, updatedTrie.RootHash()) | |
| require.Equal(t, rootHash, updatedTrieNoPruning.RootHash()) |
I verified locally that the test passes with the corrected assertion. Note the mtrie original has the identical bug (ledger/complete/mtrie/trie/trie_test.go line 1044) — candidate for a small follow-up fix there.
| sort.Slice(paths, func(i, j int) bool { | ||
| return bytes.Compare(paths[i][:], paths[j][:]) < 0 | ||
| }) | ||
| for i := index; i < len(paths); i++ { |
There was a problem hiding this comment.
The loop starts at index, so the first partition paths[0:index] is never compared against sortedPaths — corruption of the zero-bit partition would go undetected. Both slices are fully sorted at this point, so the check can simply cover the whole range:
| for i := index; i < len(paths); i++ { | |
| for i := 0; i < len(paths); i++ { |
Same issue in the mtrie original (ledger/complete/mtrie/trie/trie_test.go line 413).
| const pathsNumber = 100 | ||
| const redundantPaths = 10 | ||
| const pathsSize = 32 | ||
| randomIndex := rand.Intn(pathsSize) |
There was a problem hiding this comment.
pathsSize = 32 is the path length in bytes, but it bounds the bit index passed to SplitPaths. The test therefore only ever splits on bit indices 0–31 (the first 4 bytes); the remaining 224 bit positions — including the byte-boundary cases inside bitutils.ReadBit — are never exercised.
| randomIndex := rand.Intn(pathsSize) | |
| randomIndex := rand.Intn(pathsSize * 8) |
(Inherited from the mtrie original.)
| leftPopulatedTrie, maxDepthTouched, err := payloadless.NewTrieWithUpdatedRegisters(emptyTrie, []ledger.Path{path}, [][]byte{value}, true) | ||
| require.Equal(t, uint16(0), maxDepthTouched) | ||
| require.Equal(t, uint64(1), leftPopulatedTrie.AllocatedRegCount()) | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
Nit: require.NoError(t, err) should precede the assertions on the other return values — if the update errors, the maxDepthTouched / reg-count failures mask the root cause. Same ordering at lines 220–222, 227–229, and 435–438.
| require.NoError(t, err) | ||
|
|
||
| // and now we override the first 99 registers with default values, i.e. unallocate them | ||
| emptyValues0 := make([][]byte, len(values1)) |
There was a problem hiding this comment.
All unallocation tests use nil values. The spec's boundary condition is len(value) == 0 (see NewLeaf), which the empty non-nil slice []byte{} also satisfies — but that representation is never tested anywhere in the suite. Suggest setting a few of these entries to []byte{} so both representations of "unallocated" are pinned as equivalent.
| n3 := NewLeaf(path, v, 1) | ||
| n4 := NewInterimNode(1, n1, n2) | ||
| n5 := NewInterimNode(2, n4, n3) | ||
| require.Equal(t, 3, len(n5.AllLeafHashes())) |
There was a problem hiding this comment.
Two weaknesses here:
- Only the count is asserted — an implementation returning the wrong hashes (or the same hash three times) would pass. Suggest asserting the returned set equals the three expected leaf hashes.
n1andn2are the same register (identical path and value) attached as both children ofn4, which is not a well-formed trie shape. Distinct registers keep the fixture representative and make the content assertion from (1) meaningful.
Adding a default leaf (unallocated register) into the subtree would also pin the "empty leaves are skipped" clause of the contract, which is currently untested at the node level.
| paths := []ledger.Path{path1, path2, path3} | ||
| values := [][]byte{value1, value2, value3} | ||
|
|
||
| tr, _, err := payloadless.NewTrieWithUpdatedRegisters(payloadless.NewEmptyMTrie(), paths, values, true) |
There was a problem hiding this comment.
Both dump tests use pruned tries, so DumpAsJSON on an explicit default leaf (as occurs in unpruned tries) is un-tested: it currently emits {"path": ..., "leafHash": null}. If that is intended, worth a test case (build with prune=false, unallocate one register); if unallocated registers should rather be skipped (as AllLeafHashes does), that's a production-code question — either way a test would force the decision to be explicit.
| // Test_InterimNodeWithOneChild verifies that the hash value of an interim node with | ||
| // only one child (left or right) is computed correctly. |
There was a problem hiding this comment.
A small diagram would make this hash-pinning test self-explanatory and, more importantly, document why the two constructions hash differently (the nil sibling contributes DefaultHashForHeight(h-1)):
| // Test_InterimNodeWithOneChild verifies that the hash value of an interim node with | |
| // only one child (left or right) is computed correctly. | |
| // Test_InterimNodeWithOneChild verifies that the hash value of an interim node with | |
| // only one child (left or right) is computed correctly. The nil sibling contributes | |
| // DefaultHashForHeight(h-1), which is why the left-only and right-only nodes below hash | |
| // differently: | |
| // | |
| // NewInterimNode(1, c, nil) NewInterimNode(1, nil, c) | |
| // (h=1) (h=1) | |
| // / \ / \ | |
| // c=Leaf(56809) nil=D(0) nil=D(0) c=Leaf(56809) | |
| // (h=0) (h=0) |
| // Test_InterimNodeWithBothChildren verifies that the hash value of an interim node with | ||
| // both children (left and right) is computed correctly. |
There was a problem hiding this comment.
A diagram clarifies the shape under test. Worth also noting the construction is synthetic, so a reader doesn't puzzle over why 56809 (MSB=1) sits on the left:
| // Test_InterimNodeWithBothChildren verifies that the hash value of an interim node with | |
| // both children (left and right) is computed correctly. | |
| // Test_InterimNodeWithBothChildren verifies that the hash value of an interim node with | |
| // both children (left and right) is computed correctly: | |
| // | |
| // NewInterimNode(1, leftChild, rightChild) | |
| // (h=1) | |
| // / \ | |
| // leftChild=Leaf rightChild=Leaf | |
| // (56809, h=0) (2, h=0) | |
| // | |
| // Note: the construction is synthetic — the child paths are not consistent with their branch | |
| // positions (NewInterimNode does not validate this); only the resulting node hash (a function | |
| // of the two child hashes) is under test. |
| // Test_AllLeafHashes verifies that AllLeafHashes collects the leaf hash of every allocated register in | ||
| // the subtree (here: three leaves across a small two-level interim structure). |
There was a problem hiding this comment.
The two-level shape is the one non-obvious structure among these tests — a diagram helps. This suggestion only touches the doc comment; it composes with my other comment on the fixture (using three distinct registers), which should be applied to the body:
| // Test_AllLeafHashes verifies that AllLeafHashes collects the leaf hash of every allocated register in | |
| // the subtree (here: three leaves across a small two-level interim structure). | |
| // Test_AllLeafHashes verifies that AllLeafHashes collects the leaf hash of every allocated register in | |
| // the subtree (here: three leaves across a small two-level interim structure). Structure under test | |
| // (apply the fixture fix from my other comment first, so n1/n2/n3 are three DISTINCT registers): | |
| // | |
| // n5 (h=2) | |
| // / \ | |
| // n4 (h=1) n3 (h=1) | |
| // / \ | |
| // n1 (h=0) n2 (h=0) |
janezpodhostnik
left a comment
janezpodhostnik
left a comment
There was a problem hiding this comment.
Looks like Alex already covered everything I had
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/equivalence_test.go`:
- Around line 374-396: Update the equivalence assertions around mByPath and
plByPath to derive the expected proof category from each path’s allocation
state, asserting Inclusion=true for allocated and explicitly-unallocated paths
and Inclusion=false for never-allocated paths before comparing structural
fields. Do not use mp.Inclusion as the oracle; retain the existing payloadless
LeafHash checks for the allocated versus explicitly-unallocated cases.
- Around line 480-484: In the live-path loop over live, add an assertion that
mByPath[p].Value() equals expectedValue before validating the payloadless hash.
Keep the existing non-empty, payload presence, and expectedLeafHash assertions
unchanged.
🪄 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: 77a343be-c347-4e02-b2aa-6307671b7239
📒 Files selected for processing (4)
ledger/complete/payloadless/equivalence_test.goledger/complete/payloadless/json_test.goledger/complete/payloadless/node_test.goledger/complete/payloadless/trie_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- ledger/complete/payloadless/node_test.go
- ledger/complete/payloadless/trie_test.go
| for _, p := range queryPaths { | ||
| mp := mByPath[p] | ||
| plp := plByPath[p] | ||
|
|
||
| require.Equalf(t, mp.Inclusion, plp.Inclusion, "Inclusion mismatch for path %x", p[:]) | ||
| require.Equalf(t, mp.Steps, plp.Steps, "Steps mismatch for path %x", p[:]) | ||
| require.Equalf(t, mp.Flags, plp.Flags, "Flags mismatch for path %x", p[:]) | ||
| require.Equalf(t, mp.Interims, plp.Interims, "Interims mismatch for path %x", p[:]) | ||
| require.Equal(t, mp.Path, plp.Path) | ||
|
|
||
| if mp.Inclusion { | ||
| if mp.Payload.IsEmpty() { | ||
| // Explicitly-unallocated register (default leaf kept by prune=false): | ||
| // payloadless reports Inclusion=true with LeafHash=nil. | ||
| require.Nilf(t, plp.LeafHash, | ||
| "payloadless should report nil LeafHash for explicitly-unallocated path %x", p[:]) | ||
| } else { | ||
| require.NotNilf(t, plp.LeafHash, | ||
| "payloadless should report non-nil LeafHash for allocated path %x", p[:]) | ||
| expected := hash.HashLeaf(hash.Hash(mp.Path), []byte(mp.Payload.Value())) | ||
| require.Equalf(t, expected, *plp.LeafHash, "leaf hash mismatch for path %x", p[:]) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert each query category’s expected proof semantics independently.
Using mp.Inclusion as the oracle lets both implementations incorrectly return non-inclusion for explicitly unallocated leaves without failing. Assert allocated and explicitly unallocated paths have Inclusion=true, while never-allocated paths have Inclusion=false; then compare structural fields.
This is the unresolved case identified in the earlier review.
🤖 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/equivalence_test.go` around lines 374 - 396,
Update the equivalence assertions around mByPath and plByPath to derive the
expected proof category from each path’s allocation state, asserting
Inclusion=true for allocated and explicitly-unallocated paths and
Inclusion=false for never-allocated paths before comparing structural fields. Do
not use mp.Inclusion as the oracle; retain the existing payloadless LeafHash
checks for the allocated versus explicitly-unallocated cases.
| for p, expectedValue := range live { | ||
| require.Falsef(t, mByPath[p].IsEmpty(), "mtrie should return non-empty payload for live path at step %d", step) | ||
| require.NotNilf(t, plByPath[p], "payloadless missing live path at step %d", step) | ||
| expectedLeafHash := hash.HashLeaf(hash.Hash(p), expectedValue) | ||
| require.Equalf(t, expectedLeafHash, *plByPath[p], "leaf hash mismatch for live path at step %d", step) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Verify the full-trie payload value, not only its presence.
A wrong non-empty value from m.UnsafeRead currently passes. Compare mByPath[p].Value() with expectedValue before validating the payloadless hash.
Proposed assertion
for p, expectedValue := range live {
require.Falsef(t, mByPath[p].IsEmpty(), "mtrie should return non-empty payload for live path at step %d", step)
+ require.Equalf(t, expectedValue, []byte(mByPath[p].Value()), "mtrie value mismatch for live path at step %d", step)
require.NotNilf(t, plByPath[p], "payloadless missing live path at step %d", step)🤖 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/equivalence_test.go` around lines 480 - 484, In
the live-path loop over live, add an assertion that mByPath[p].Value() equals
expectedValue before validating the payloadless hash. Keep the existing
non-empty, payload presence, and expectedLeafHash assertions unchanged.
This comment has been minimized.
This comment has been minimized.
4 participants
Accidentally merged #8572.
Now since #8569 has been merged, I'm opening this PR again to add test cases for payload node and trie implementation.
Summary by CodeRabbit