PR 7944 converted the XRPL_ASSERT guards in NodePathStack to UNREACHABLE + return false,
so the pairing checks now fail in both Debug and Release builds, not just Debug. Update
the comment to reflect this change.
Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
The stack asserted its preconditions and then went ahead regardless. Asserts
expand to `assert`, so in a release build every one of those was a no-op in
front of the operation it was guarding: reading or popping an empty
`std::stack` is undefined, `pushChild`'s out-of-range branch check was missing
entirely, and `getChildNodeID` throws `std::logic_error` at leaf depth. None
of these conditions are reachable through any of `SHAMap`'s public entry
points today, but the failure modes if they ever did happen would be
disproportionate: an out-of-range branch would silently corrupt a node ID
instead of failing loudly, and a `logic_error` reaching an unguarded call
chain would abort the process, since nothing in this codebase catches it.
Pushes now return false instead of throwing or silently corrupting the ID, and
reads degrade to a null node rather than undefined behavior. `[[nodiscard]]`
makes an unchecked push a compile error. Each new guard is marked
`UNREACHABLE` rather than left implicitly untested, since no test fixture in
this suite can reach these paths without building a deliberately corrupt map.
`pushRoot` gets the same conversion as every sibling method; it was the one
push still asserting instead of returning false.
`walkTowardsKey`'s two modes (with and without a caller-supplied stack) must
fail at the same node and leave the stack in a state every caller already
knows how to handle; on failure the stack is now cleared via a restored
`clear()`, and a restored `pushCurrent` lambda keeps the loop-entry and
post-loop push-and-clear logic from being duplicated. It also stops deriving
each node ID twice: a caller-supplied stack now reads the ID `pushNode` just
computed off `stack->top().second`, instead of a redundant local copy that
additionally went stale once the loop exited. `belowHelper` and
`peekNextItem` finally get the fallback `top()`'s own docstring promises: both
read `stack.top()` right after an assert-only emptiness check, with no
fallback for release builds, so both now return early on an empty stack
instead of dereferencing a null `SHAMapTreeNodePtr`.
`pushChild`'s hard guard also only checked the parent's depth against
`kLeafDepth`, one level too permissive for an inner child: a parent at 63
passed the check, then pushed an inner child at 64 with only a debug-only
assert catching it, the exact gap this commit exists to close. Tightened to
require depth + 1 below `kLeafDepth` for an inner child, with
`walkTowardsKey`'s no-stack path given the identical tightening so a
malformed map fails at the same node in both modes.
`walkTowardsKey`'s own empty-stack precondition had the same gap: only an
`XRPL_ASSERT` enforced it, so a non-empty caller-supplied stack would silently
be appended to instead of failing closed. Guarded the same way as everything
else here, and `peekFirstItem`'s comment on the failure path is corrected to
match: it clears the stack unconditionally, not only for an empty map.
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, the smallest map that reaches that
branch, 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. The same comment
also claimed the leaf branch settles every probe before `root_`'s inner-node
scan could run, which only holds for a probe the leaf qualifies against: for
the rest the leaf is popped and that scan is exactly what reaches `end()`.
The test name said `leaf root` for the same reason, and now names the leaf
below the root.
`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.
