refactor: Add SHAMapNodeID::isPrefixOf - #7939
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a small helper API on SHAMapNodeID—isPrefixOf(uint256 const&)—to centralize the “node ID matches the prefix implied by depth” check, and refactors existing call sites to use it instead of open-coding depthMask comparisons or rebuilding IDs via createID.
Changes:
- Add
SHAMapNodeID::isPrefixOf(uint256 const&)to test whether a leaf key shares this node’s depth-prefix. - Replace open-coded prefix/ID consistency checks with
isPrefixOfin ledger node deserialization and SHAMap sync logic. - Reuse the helper internally to simplify invariant checks in
SHAMapNodeIDconstruction and child derivation.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/xrpld/app/ledger/detail/LedgerNodeHelpers.cpp | Uses isPrefixOf to validate legacy leaf node IDs against their leaf keys. |
| src/libxrpl/shamap/SHAMapSync.cpp | Switches leaf-position assertion to XRPL_ASSERT_IF(..., nodeID.isPrefixOf(leafKey(...))). |
| src/libxrpl/shamap/SHAMapNodeID.cpp | Implements isPrefixOf and reuses it for existing depth-mask invariant checks. |
| include/xrpl/shamap/SHAMapNodeID.h | Declares and documents the new isPrefixOf public API. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
080c08b to
72936bc
Compare
ef6fda7 to
cc83dd3
Compare
A node at depth d names the tree path spelled by the first d nibbles of its key, so any leaf beneath it must agree on that prefix. Three call sites open-coded this check against `depthMask`, and a fourth rebuilt a whole node ID with `createID` just to compare it back. No behavior change: each converted site tests the same condition it did before.
72936bc to
561ca50
Compare
cc83dd3 to
bb0a62e
Compare
bthomee
added
the
Trivial
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean, mechanical refactor extracting a shared isPrefixOf helper. Verified each of the four call sites: the new isPrefixOf(id_) / isPrefixOf(key) checks are algebraically equivalent to the mask comparisons and createID(...).getNodeID() == ... comparisons they replace, so the 'no behavior change' claim holds. No issues found in the added lines.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Part 2/8 of a stack. Base: part 1 (
bthomee/shamap-stack-01-unsigned-int-branch-ops).A node at depth d names the tree path spelled by the first d nibbles of its
key, so any leaf beneath it must agree on that prefix. Three call sites
open-coded this check against
depthMask, and a fourth rebuilt a whole nodeID with
createIDjust to compare it back.No behavior change: each converted site tests the same condition it did
before.