refactor: Use unsigned int for branch-related operations by bthomee · Pull Request #7938 · XRPLF/rippled

bthomee · 2026-08-03T21:10:19Z

Part 1/8 of a stack. Base: develop.

Branch indices, depths, and array positions in the SHAMap code were typed
int despite never legitimately going negative, so several call sites carried
a manual runtime check purely to rule out a value the type system could have
ruled out on its own: SHAMapNodeID::createID asserted depth >= 0,
SHAMapInnerNode::setChild asserted m >= 0. Switched these, and the branch
parameters on descend/descendThrow/descendAsync, getChildNodeID,
getChildIndex, and the loop indices iterating a node's 16 branches, to
unsigned int, dropping the now-redundant half of each assert and replacing a
few bare 16s with SHAMapInnerNode::kBranchFactor.

belowHelper's int branch parameter is replaced together with the mechanism
it fed: a std::tuple<int, std::function<bool(int)>, std::function<void(int&)>>
describing which direction to scan is replaced by a BelowDirection enum.

Also fixes two srcDstNumAllocated - 1 subtractions in
TaggedPointer.ipp's sparse-array shift logic that this same cleanup should
have caught, a stray while child to which child typo in the StackEntry
tuple comment, and closes a gap in hasLeafNode: its do/while had no
bound on how deep it could descend, so a malformed map with an inner node at
kLeafDepth would reach getChildNodeID and throw uncaught instead of
failing cleanly.

Copilot

Pull request overview

This PR refactors SHAMap branch/depth/index handling to use unsigned types (and kBranchFactor) where values are not expected to go negative, and it tightens a few traversal/proof-path edge cases in SHAMap-related code.

Changes:

Switch SHAMap branch/depth parameters and loop indices from int to unsigned int (and replace hard-coded 16 with kBranchFactor in multiple places).
Refactor belowHelper’s scan-direction plumbing from a tuple-of-functions to a BelowDirection enum.
Add a depth bound in hasLeafNode to avoid uncaught throws on malformed maps; adjust proof-path maximum length to use kLeafDepth + 1.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/xrpld/app/ledger/detail/LedgerNodeHelpers.cpp	Updates leaf ID creation call to the new unsigned-depth `createID` signature.
src/libxrpl/shamap/SHAMapSync.cpp	Converts traversal indices/branches to unsigned and updates proof-path constraints/types.
src/libxrpl/shamap/SHAMapNodeID.cpp	Updates `createID` to unsigned depth and derives mask table size from `kLeafDepth`.
src/libxrpl/shamap/SHAMapInnerNode.cpp	Converts child/branch APIs and internal loops/bitmasks to unsigned-safe forms.
src/libxrpl/shamap/SHAMapDelta.cpp	Replaces hard-coded branch-factor constants and loop index types in delta walkers (incl. parallel walker sizing).
src/libxrpl/shamap/SHAMap.cpp	Converts descent/below-helper branch types to unsigned; refactors below scan logic and adjusts some branch-iteration loops.
include/xrpl/shamap/SHAMapNodeID.h	Public header signature changes for `getChildNodeID`/`createID` to unsigned.
include/xrpl/shamap/SHAMapInnerNode.h	Public header signature changes for branch-related APIs and inline bitmask logic.
include/xrpl/shamap/SHAMap.h	Updates SHAMap public/private method signatures for unsigned branches and introduces `BelowDirection`.
include/xrpl/shamap/detail/TaggedPointer.ipp	Updates sparse/dense iteration/indexing logic to unsigned types and unsigned bitmasks.
include/xrpl/shamap/detail/TaggedPointer.h	Updates `getChildIndex` and `popcnt16` related code to unsigned types/bitmasks.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

bthomee · 2026-08-03T21:29:44Z

@@ -466,32 +468,24 @@ SHAMap::belowHelper(
            }
            inner = intr_ptr::staticPointerCast<SHAMapInnerNode>(node);
            stack.emplace(inner, stack.top().second.getChildNodeID(branch));


Correct, and that is fixed in #7942.

✅ Auto-resolved: the latest review no longer flags this issue, so it appears to have been addressed. If that is not correct, reopen this thread and it will be re-checked on the next review.

bthomee · 2026-08-04T00:09:58Z

 SHAMapNodeID
-SHAMapNodeID::createID(int depth, uint256 const& key)
+SHAMapNodeID::createID(unsigned int depth, uint256 const& key)
 {
-    XRPL_ASSERT(
-        depth >= 0 && depth <= SHAMap::kLeafDepth, "xrpl::SHAMapNodeID::createID : valid depth");
+    XRPL_ASSERT(depth <= SHAMap::kLeafDepth, "xrpl::SHAMapNodeID::createID : valid depth");
    return SHAMapNodeID(depth, key & depthMask(depth));


Fixed in #7941. That PR already contains the "clamp the depth" commit for selectBranch in this same file. The unguarded index is in depthMask's 65-entry table, so the constructor and isPrefixOfAtDepth share the exposure. Both places that set a depth now clamp: the constructor (the single point every depth_ passes through) and createID, which needs its own bound because it selects the mask while evaluating the constructor's argument; the constructor body runs too late to help.

Clamping is used rather than a throw, so this deliberately differs from getChildNodeID, since createID is reached from getSHAMapNodeID with a peer-supplied depth, and two of that function's three callers — InboundTransactions::gotData and PeerImp::onMessage — sit on paths with no handler between them and a thread boundary (JobQueue::processTask/Job::doJob and asio's ioContext_.run() both lack one). A logic_error there would terminate the process instead of dropping the message, so throwing would trade an OOB read for a remote crash vector.

Clamping also repairs id_ alongside depth_, since a node ID whose id and depth disagree breaks the invariant every read of id_ depends on. Leaving the depth unclamped would also let getRawString narrow it to one byte, turning depth 256 into a node claiming to be the root.

It's worth noting no current caller can reach this — verifyProofPath and getSHAMapNodeID both bound the depth first, so it's hardening, not a live bug.

✅ Auto-resolved: the latest review no longer flags this issue, so it appears to have been addressed. If that is not correct, reopen this thread and it will be re-checked on the next review.

codecov · 2026-08-03T21:52:52Z

Codecov Report

❌ Patch coverage is 94.06780% with 7 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/libxrpl/shamap/SHAMapSync.cpp	81.8%	4 Missing ⚠️
include/xrpl/shamap/detail/TaggedPointer.ipp	92.3%	2 Missing ⚠️
src/libxrpl/shamap/SHAMapDelta.cpp	90.0%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

xrplf-ai-reviewer

This is a clean, largely mechanical refactor from int to unsigned int for branch/depth indices in the SHAMap code, replacing now-redundant >= 0 asserts, swapping bare 16s for SHAMapInnerNode::kBranchFactor, and replacing the belowHelper tuple-of-lambdas mechanism with a BelowDirection enum. I traced the belowHelper/firstBelow/lastBelow rewrite and the lowerBound unsigned-decrement loop carefully since those are the highest-risk spots for off-by-one/underflow bugs, and both preserve the original iteration semantics correctly. The added depth bound in hasLeafNode correctly closes the described gap. No correctness or security issues found in the changed lines.

Branch indices, depths, and array positions in the SHAMap code were typed `int` despite never legitimately going negative, so several call sites carried a manual runtime check purely to rule out a value the type system could have ruled out on its own: `SHAMapNodeID::createID` asserted `depth >= 0`, `SHAMapInnerNode::setChild` asserted `m >= 0`. Switched these, and the branch parameters on `descend`/`descendThrow`/`descendAsync`, `getChildNodeID`, `getChildIndex`, and the loop indices iterating a node's 16 branches, to `unsigned int`, dropping the now-redundant half of each assert and replacing a few bare `16`s with `SHAMapInnerNode::kBranchFactor`. `belowHelper`'s `int` branch parameter is replaced together with the mechanism it fed: a `std::tuple<int, std::function<bool(int)>, std::function<void(int&)>>` describing which direction to scan is replaced by a `BelowDirection` enum. Also fixes two `srcDstNumAllocated - 1` subtractions in `TaggedPointer.ipp`'s sparse-array shift logic that this same cleanup should have caught, a stray `while child` to `which child` typo in the `StackEntry` tuple comment, and closes a gap in `hasLeafNode`: its `do`/`while` had no bound on how deep it could descend, so a malformed map with an inner node at `kLeafDepth` would reach `getChildNodeID` and throw uncaught instead of failing cleanly. That new bound is marked `UNREACHABLE` rather than left as a bare check, since `addKnownNode` already rejects such a map as invalid before it could be reached: no test can cover the line, so it is excluded from coverage rather than reported as an untested branch.

xrplf-ai-reviewer

Clean, well-scoped mechanical refactor from int to unsigned int for branch/depth indices across the SHAMap code. I traced the trickiest transformations by hand: the lowerBound reverse-scan loop (branch - 1 >= 0 → branch > 0u; --branch), the belowHelper tuple-of-lambdas → BelowDirection enum rewrite, and the two srcDstNumAllocated - 1 fixes in TaggedPointer.ipp — all preserve original semantics given the invariant that the affected branch index is always present in the relevant bitset before the subtraction (so no unsigned-underflow path is reachable). The added hasLeafNode depth-bound check and the SHAMapNodeID.h/.cpp param renames are correct and match the stated intent. No newly-introduced correctness or security issues found in the changed (+) lines.

godexsoft

The code looks fine to me, largely a mechanical change.
I wonder why we use 'unsigned int' instead of 'uint32_t' and whether we should switch?

bthomee · 2026-08-04T12:02:34Z

I wonder why we use 'unsigned int' instead of 'uint32_t' and whether we should switch?

That's a larger question beyond the scope of this PR I think - there are pros and cons to each data type.

bthomee marked this pull request as ready for review August 3, 2026 21:19

Copilot AI review requested due to automatic review settings August 3, 2026 21:19

Copilot started reviewing on behalf of bthomee August 3, 2026 21:19 View session

Copilot AI reviewed Aug 3, 2026

View reviewed changes

bthomee force-pushed the bthomee/shamap-stack-01-unsigned-int-branch-ops branch from 080c08b to 72936bc Compare August 4, 2026 00:12

xrplf-ai-reviewer Bot approved these changes Aug 4, 2026

View reviewed changes

bthomee force-pushed the bthomee/shamap-stack-01-unsigned-int-branch-ops branch from 72936bc to 561ca50 Compare August 4, 2026 00:49

bthomee requested a review from vlntb August 4, 2026 00:49

bthomee mentioned this pull request Aug 4, 2026

test: Cover visitDifferences node skipping #7946

Draft

bthomee requested a review from godexsoft August 4, 2026 00:49

xrplf-ai-reviewer Bot approved these changes Aug 4, 2026

View reviewed changes

godexsoft approved these changes Aug 4, 2026

View reviewed changes

Navigation Menu

Uh oh!

refactor: Use unsigned int for branch-related operations - #7938

refactor: Use unsigned int for branch-related operations#7938
bthomee wants to merge 1 commit into
developfrom
bthomee/shamap-stack-01-unsigned-int-branch-ops

bthomee commented Aug 3, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

bthomee Aug 3, 2026

Uh oh!

xrplf-ai-reviewer Bot Aug 4, 2026

Uh oh!

xrplf-ai-reviewer Bot Aug 4, 2026

Uh oh!

bthomee Aug 4, 2026

Uh oh!

xrplf-ai-reviewer Bot Aug 4, 2026

Uh oh!

xrplf-ai-reviewer Bot Aug 4, 2026

Uh oh!

Uh oh!

codecov Bot commented Aug 3, 2026 •

edited

Loading

Uh oh!

xrplf-ai-reviewer Bot left a comment

Uh oh!

xrplf-ai-reviewer Bot left a comment

Uh oh!

godexsoft left a comment

Uh oh!

bthomee commented Aug 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

bthomee commented Aug 3, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

bthomee Aug 3, 2026

Choose a reason for hiding this comment

Uh oh!

xrplf-ai-reviewer Bot Aug 4, 2026

Choose a reason for hiding this comment

Uh oh!

xrplf-ai-reviewer Bot Aug 4, 2026

Choose a reason for hiding this comment

Uh oh!

bthomee Aug 4, 2026

Choose a reason for hiding this comment

Uh oh!

xrplf-ai-reviewer Bot Aug 4, 2026

Choose a reason for hiding this comment

Uh oh!

xrplf-ai-reviewer Bot Aug 4, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov Bot commented Aug 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

xrplf-ai-reviewer Bot left a comment

Choose a reason for hiding this comment

Uh oh!

xrplf-ai-reviewer Bot left a comment

Choose a reason for hiding this comment

Uh oh!

godexsoft left a comment

Choose a reason for hiding this comment

Uh oh!

bthomee commented Aug 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented Aug 3, 2026 •

edited

Loading