trie/bintrie: fix grouped InternalNode serialization path mismatch#569
Conversation
gballet
force-pushed
the
binary-tree-with-pages
branch
from
95a6188 to
2ffd285
Compare
gballet
left a comment
gballet
left a comment
There was a problem hiding this comment.
leaving some comments ahead of your cleanup. The PR name should be changed, imo, as it seems to concern InternalNodes and not StemNodes, iiuc.
There was a problem hiding this comment.
this shouldn't happen, and so I recommend either returning an error or panicking, because this is a criticial flaw if that happens.
There was a problem hiding this comment.
Agreed — this should be a hard error. However, this code is from the base branch (binary-tree-with-pages), not from this PR's fix. I've trimmed this PR to only the serialization path fix. Happy to open a separate PR for the groupDepth validation if you'd like.
There was a problem hiding this comment.
same here, it should panic because whatever situation brought this turn of events should be immediately flagged
There was a problem hiding this comment.
Same as above — this is base branch code. Removed from this PR's scope. Can address in a follow-up.
| serializeSubtree(n.left, remainingDepth-1, leftPos, bitmap, hashes) | ||
| serializeSubtree(n.right, remainingDepth-1, rightPos, bitmap, hashes) | ||
| serializeSubtree(n.left, remainingDepth-1, leftPos, absoluteDepth+1, bitmap, hashes) | ||
| serializeSubtree(n.right, remainingDepth-1, rightPos, absoluteDepth+1, bitmap, hashes) | ||
| case Empty: | ||
| // Empty subtree: all positions in this subtree are empty (bits already 0) | ||
| return | ||
| default: |
There was a problem hiding this comment.
aren't you handling HashedNode ?
There was a problem hiding this comment.
You're right that HashedNode was implicit. Refactored the default case into an explicit type switch: *StemNode extends using stem key bits, and the fallback (HashedNode/unknown) extends all-left via position << remainingDepth. The all-left extension is correct for HashedNode because we don't have key bits to follow — it matches the all-zero path that resolveNode would take.
CPerezz
force-pushed
the
fix/stemnode-group-path-mismatch
branch
from
371cb9e to
3f9bb49
Compare
CPerezz
changed the title
…tion When a StemNode appears at an intermediate depth within a grouped InternalNode, collectChildGroups stored it at its actual depth path (e.g., [0,0] for depth 2), but serializeSubtree projected its hash to the group leaf boundary using stem key bits. After deserialization, the HashedNode ended up at the projected depth, causing lookup path mismatches and "missing trie node" errors. Two fixes: 1. serializeSubtree: use stem key bits (not all-left) to project StemNodes to their correct bitmap leaf position 2. collectChildGroups: extend the storage path to match the projected leaf position using the same stem-bit extension This ensures the storage path matches the lookup path generated by keyToPath in InsertValuesAtStem/GetValuesAtStem after deserialization. Fixes missing trie node errors for groupDepth values 1-3 with large state databases.
- Add comment explaining why groupDepth byte is serialized (sparse bottom layer makes bitmap size variable) - Make HashedNode handling explicit with named type switch case instead of hiding it in the if/else fallthrough
CPerezz
force-pushed
the
fix/stemnode-group-path-mismatch
branch
from
3f9bb49 to
ada64c9
Compare
2 participants
When a StemNode appears at an intermediate depth within a grouped InternalNode,
collectChildGroupsstored it at its actual depth path (e.g., [0,0] for depth 2), butserializeSubtreeprojected its hash to the group leaf boundary using stem key bits. After deserialization, the HashedNode ended up at the projected depth, causing lookup path mismatches and "missing trie node" errors.Two fixes:
serializeSubtree: use stem key bits (not all-left) to project StemNodes to their correct bitmap leaf positioncollectChildGroups: extend the storage path to match the projected leaf position using the same stem-bit extensionThis ensures the storage path matches the lookup path generated by
keyToPathinInsertValuesAtStem/GetValuesAtStemafter deserialization.Fixes missing trie node errors for groupDepth values 1-3 with large state databases.
Review updates: