The two functions were near duplicates: walk to the key, then look for the
nearest leaf on one side. Only the scan direction, the comparison deciding a
leaf qualifies, and whether to take the first or last leaf below the subtree
differed, exactly the distinction `BelowDirection` already draws for
`belowHelper`, so the pair collapse into one parameterised walk. Also drops
the stale `// TODO: what to return here?` above `lowerBound`'s `return end()`:
no predecessor is the correct answer for the smallest key, and the tests pin
it.
Existing coverage only exercised `boundHelper`'s inner-node branch, every test
map had at least three items, so the root was always an inner node and the
leaf branch at the top of the function was never reached with a real answer to
give. Adds coverage for a single-item map, where the leaf branch alone decides
the outcome, and an empty map, where the scan must find nothing on every
branch before falling through to `end()`.
Fixes the single-item test's own comment, which claimed `root_` becomes a
leaf, when in fact `root_` stays the inner node it was constructed with for
any map built via `addItem`; only a single-item map synced from a peer
(`addRootNode`) ever replaces `root_` with a leaf directly.
`belowHelper` built each stack entry's `SHAMapNodeID` from `branch`, the branch
used to reach the subtree root, rather than `childBranch`, the branch it had
just descended. The resulting IDs carried a correct depth but named a
different subtree, and nothing rejected them: such an ID has a legal depth and
a legal mask, so only comparing it against an actual leaf key exposes the
mismatch. The affected stacks feed read-only traversals whose consumers use
only the depth, so no ledger state, hash, or peer message was affected, but
any future consumer of `getNodeID()` would have silently received the wrong
position.
Rather than fix the one call, make the mistake unrepresentable.
`NodePathStack` replaces the bare `std::stack` and refuses to accept an ID at
all: every push takes the branch being descended and derives the ID itself, so
a node and its ID cannot disagree. `isPrefixOf` assertions on each push catch a
wrong branch at the point it happens rather than wherever the ID is later
read. Leaf entries now keep the depth they were reached at instead of a
normalized `kLeafDepth`, which is what lets those assertions hold:
`addGiveItem` splits a leaf from the depth it actually sits at.
The new traversal tests fail on the previous code: reverting the branch
derivation trips the leaf-key assertion on the first iteration. Also adds a
`deepFanOutKeysAtLeafDepth` helper and mirrors them against it, since the
existing `deepFanOutKeys`'s fan-out at the 6th nibble keeps its tree only
about 6 levels deep and never exercised the depth-63/64 code these tests are
meant to protect, plus a case that collapses the entire depth-63 chain of
single-child inner nodes into a leaf on the final delete, which the
every-other-key deletion pattern the other new tests use never triggers.
`selectBranch` reads the key byte at `depth / 2`, which is out of bounds for a
32-byte key once the depth reaches 64. Every branch selection in the map
funnels through here, so this is the one place a stray depth can turn into a
bad read. Assert the precondition for callers, then clamp anyway: a wrong
answer for an input that should never occur is better than reading past the
buffer. Verified under ASan that the unclamped form reads one byte past a
32-byte allocation while the clamped form does not.
`depthMask`'s own 65-entry table had the same exposure one level up, reachable
through the public `createID` factory rather than only from inside the map. A
depth past `kLeafDepth` indexed that table out of bounds, confirmed under ASan
as a 4-byte `global-buffer-overflow` immediately after `kMasks`. Both places
that can set a depth now clamp it: the constructor, which is the single point
every `SHAMapNodeID`'s `depth_` passes through, and `createID`, which needs its
own bound because it picks the mask while evaluating the constructor's
argument, before the constructor body could correct anything.
Clamping rather than throwing, which is what `getChildNodeID` does for the
analogous case: `createID` is reached from `getSHAMapNodeID` with a
peer-supplied depth, and two of that function's three callers
(`InboundTransactions::gotData`, `PeerImp::onMessage`) sit on paths with no
handler between them and a thread boundary, so a throw there would end the
process rather than the message. Clamping also has to fix up `id_` alongside
`depth_`, since a node ID whose id and depth disagree fails the invariant every
read of `id_` relies on. Leaving the depth unclamped would additionally let
`getRawString` narrow it to a byte, turning depth 256 into a node claiming to
be the root.
`deserializeSHAMapNodeID` gets the same mask check `isPrefixOf` already
performs, pulled into a shared `isPrefixOfAtDepth` helper, and the masking both
it and `createID` perform is now a named `maskedToDepth` rather than a repeated
bitwise-and.
Tests cover the depth-sensitivity of `isPrefixOf`, the guards that must hold
with asserts stripped, that `deserializeSHAMapNodeID` rejects an out-of-range
depth, and the clamp itself under both build configurations (`EXPECT_DEATH`
in a forked process when the assert is live, and the clamped result compared
against depth 63 when it is not). The clamp test also gates on
`ENABLE_VOIDSTAR`, not just `NDEBUG`: under Antithesis instrumentation
`XRPL_ASSERT` routes to a handler that records the hit but never aborts, so a
Debug build with voidstar enabled has `NDEBUG` undefined yet still hits the
same non-fatal assert as a release build, and without this gate would send
that configuration into the `EXPECT_DEATH` arm, where the forked child never
dies and the test fails, breaking the CI job that runs this suite under
`-Dvoidstar=ON`.
A SHAMap has 65 levels, and nibbles run out at level 64: `selectBranch` indexes
the key byte at `depth / 2`, so at depth 64 it reads byte 32 of a 32-byte key.
Only the leaf terminating a path may sit at that depth, but proof path nodes
come off the wire, so a peer could send 65 hash-chained inner nodes and drive
that read past the end of the buffer.
The existing length bound cannot be tightened to catch this: a path for two
keys sharing all 63 leading nibbles legitimately holds 64 inner nodes plus a
leaf, so 65 elements is valid. The claimed node type at the final depth is the
thing to reject, using `>=` rather than `==` to match the convention every
other `kLeafDepth` comparison in this codebase already follows.
Reachable from `TMProofPathResponse` via `LedgerReplayMsgHandler`; confirmed
under ASan with asserts compiled out that the unguarded read lands one byte
past a 32-byte heap allocation. Also closes a second gap: nothing checked the
terminal leaf's own key against `key`, so the hash chain alone let a peer
substitute any leaf whose subtree hashes matched at every level above it.
Pinned by tests: the 65-element path that must verify for both keys sharing
the deep prefix, and the forged all-inner path that must not.
Also guards `visitDifferences` against an inner node claimed at leaf depth:
`hasLeafNode` only checked the comparison map, not the map being walked, so a
corrupt node in the map under `visitDifferences` itself could still throw
uncaught. Skip such a node's children instead.
