[Ergologica]

Closes #89.

The problem

InternalNode.computeLabel (BatchNode.scala:45-47) hashes the balance and the two children labels:

override protected def computeLabel: D = {
  hf.hash(Array(InternalNodePrefix, b), left.label, right.label)
}

The key is not in there. InternalProverNode.getNewKey says so outright — "label doesn't change when key of an internal node changes".

So a manifest or a subtree carries keys its own digest does not authenticate, and both verify() methods checked only labels. An adversary can hand over a subtree with altered internal keys and every check passes: the digest is unchanged, verify() returns true, combine() attaches it (the lookup is by label), and the resulting prover routes lookups down the wrong branch. nodesFromBytes has a local left.key < key <= right.key guard for materialized internal nodes (BatchAVLProverSerializer.scala:135-137), but it constrains the key only against its immediate children, and the ProxyInternalNode branch (:140-146) has no key check at all.

The approach

The issue offers two routes — "avoid storing keys for internal nodes at all, or implement check". This implements the check, because dropping the keys changes the serialization format and downstream consumers would have to move in lockstep (ergo vendors its own copies of ManifestSerializer / SubtreeSerializer). No wire format change.

The keys do not need to be trusted, because they are recomputable. In this tree the key of an internal node is the smallest key of its right subtree — the invariant BatchAVLProver.checkTree already enforces at BatchAVLProver.scala:463:

myRequire(minRight.key sameElements r.key, "min of right subtree doesn't match")

So a materialized subtree can be checked against itself, with no extra data.

The change

New NodeKeyChecks (package-private to serialization), applied at the three points where trust was being extended:

• BatchAVLProverSubtree.verify — full check. A subtree is fully materialized, so every internal key is recomputed from the leaves and compared. A subtree passing this cannot contain a forged internal key.
• BatchAVLProverManifest.verify — ordering check. A manifest ends at ProxyInternalNodes whose children are only labels, so the keys below are not available yet and the full check is impossible here. What is checkable is the search-tree ordering: since every internal key is the smallest key of its right subtree, an in-order walk of the manifest's internal nodes must yield strictly increasing keys, terminal proxy nodes included. This is the check available "assuming bottom keys of subtrees are correct".
• BatchAVLProverSerializer.combine — full check on the assembled tree. This is what actually closes the hole: once manifest and subtrees are joined everything is materialized, so a forged key fails recovery even if a caller skipped verify(). combine already returns Try, so the failure surfaces normally.

Tests

Four new properties in AVLBatchSerializationSpecification:

• a label does not commit to the key of an internal node — pins the underlying fact: same children, different key, identical label. It documents why the rest exists, and fails loudly if labels ever start committing to keys.
• subtree with a forged internal key does not verify — asserts the forged subtree still passes the digest comparison it used to be accepted on, then that verify rejects it anyway.
• manifest with an internal key out of order does not verify — same shape, via a root key smaller than every key in the tree.
• combining a tree with a forged internal key fails — end to end: the label is unchanged, so the forged subtree is still found and attached where the real one belongs, and recovery fails.

Each asserts the honest case passes too, so the checks cannot be satisfied by rejecting everything.

sbt scryptoJVM/test — 139 tests green. Cross-compiled on 2.11, 2.12, 2.13 and 3.3.5.

Two notes

SparseMerkleTreeSpecification's "Updated tree has valid proof" failed once for me across full-suite runs and passes otherwise. It is unrelated to this change — this PR touches nothing under merkle/ — but it looks genuinely flaky: the properties are forAll { (height: Byte) => ... } over an unconstrained Byte. Happy to open a separate issue.

ergo vendors its own avldb/.../ManifestSerializer.scala and SubtreeSerializer.scala, which have no key check either. This PR does not touch ergo; say the word and I'll open the matching one there.

[Ergologica]

Closing this in favour of ergoplatform#3.

@kushti pointed out that this master is protected and that the Ergo side now works from ergoplatform/scrypto, which is where the recent work on BatchAVLProverSerializer has been happening. I have rebased the change there.

Worth recording for anyone who finds this PR later: the problem it describes is still present on that fork. Matching subtrees by digest label, which the hardening there now does, does not cover it - InternalNode.computeLabel hashes the balance and the two children labels only, so two trees differing solely in an internal key have identical labels.