fix: Report a map-invalidating node as invalid data

SHAMap::addKnownNode() reports invalid() for the two kinds of node it does not hook into the map: an
inner node arriving at kLeafDepth, a depth only a leaf may occupy, and a node whose ID does not match
the position the descent stopped at. That is the verdict callers already handle as bad data, so
neither counts as forward progress. Each emits one warning naming the node and where the descent
stopped, matching the sibling branches beside them, and carries a SOMETIMES() hint for the fuzzer.
The depth rule is spelled once, as a file-local isLeafDepth() that hasLeafNode() reads too. The
verdict on a map-invalidating node belongs to the root hash that was asked for rather than to this
copy of the tree, since every node from the root down hash-verified to get there: no peer can satisfy
such a hash, retrying is futile, and it cannot arise by accident. A charge for it is a deterrent
rather than a control even so, which the comment says, because the same node can reach a map through a
fetch pack or an unsolicited object reply and neither passes through here.

The depth test precedes the full-below cache lookup on the way down. That cache is keyed by node hash
and shared by every map of a family, and a hash covers a node's children but not its depth, so the
same subtree hash can be cached as complete at one depth and reached at kLeafDepth here. Testing the
depth first is what keeps the verdict independent of whatever an unrelated map cached, which is the
determinism the acquisition paths need from it. Skipping the shortcut at the deepest level only
forgoes an optimization, and the depth it guards cannot occur in a real tree.

Three tests drive the map through DeepChain's fill() and addOffendingNode(), so a case names the
position no valid tree can occupy without spelling out the descent. They cover the map-invalidating
node; the three ways a node cannot be hooked anywhere while leaving the map sound; and the same
offending node with a full-below entry already in place, so the depth test is what has to reach the
verdict. A file-local tallyIs() reads each verdict as counts, which leaves get()'s wording pinned in
one place rather than at every site with a verdict to check.
This commit is contained in:
Bart
2026-08-22 22:15:47 -04:00
parent adb772ef2f
commit f45cce80e6
3 changed files with 172 additions and 16 deletions

View File

@@ -393,6 +393,11 @@ public:
* @param filter Optional sync filter to track received nodes.
* @return Status indicating whether the node was useful, duplicate, or invalid.
*
* A node no valid tree could hold makes the map Invalid, which is
* terminal: the root hash committed to an impossible shape, so no peer
* can satisfy it. An acquisition reaching this verdict must give up
* rather than retry; nothing may promote the map back to a valid state.
*
* @note This function expects the treeNode to be a valid, deserialized SHAMapTreeNode. The
* caller is responsible for deserialization and basic validation before calling this
* function. This also means that the nodeID must be consistent with the node's content.