trees/subtree: Add MTC subtree primitives#8808
Conversation
jsha
left a comment
There was a problem hiding this comment.
After a first look, my high level comments:
There are a number of places where subtree.go panics. I notice that the upstream tlog.go panics in a bunch of places where the match is wrong too, but I'd like to see if we can have fewer panic sites to reason about. For instance, what if ValidSubtree() returns a Subtree() object, which has accessors for Hi(), Lo(), Size(), and Height(). We can guarantee in the contract for Size() and Height(), for instance, that they are positive (non-negative and non-zero). This might also let us consolidate the various //nolint: gosec // G115 correctness comments into a smaller number of places.
We could make this into a constructor:
type Subtree struct {
start, len, treeSize uint64
}
func New(start, end, treeSize int64) (Subtree, error) {}I notice that both internal callers of ValidSubtree() check end > treeSize immediately after. That suggests to me that ValidSubtree() should be a function of not just start and end, but also of tree size, so that we can check that property as part of it.
| // rangeHash returns MTH(D[lo:hi)), the RFC 6962 section 2.1 Merkle Tree Hash | ||
| // over the leaves in [lo, hi) as an independent list, read through the provided | ||
| // reader. It decomposes [lo, hi) into its maximal aligned perfect subtrees and | ||
| // reads all of their roots in a single ReadHashes call before folding them | ||
| // together. | ||
| func rangeHash(lo, hi int64, reader xtlog.HashReader) (xtlog.Hash, error) { |
There was a problem hiding this comment.
I think this can also be implemented in terms of tlog.TreeHash(hi-lo, someReader).
The distinction is that TreeHash will make storage calls starting from zero, but we need to make sure our storage layer is correctly indexing based on the subtree we want. I think we can achieve that with a tlog.HashReaderFunc:
func subtreeHash(lo, hi int64, reader xtlog.HashReader) (xtlog.Hash, error) {
return tlog.TreeHash(hi-lo, func(zeroBasedIndexes []int64) []Hash {
var absoluteIndexes []int64
for _, i := range zeroBasedIndexes {
absoluteIndexes = append(absoluteIndexes, i+lo)
}
return reader.ReadHashes(absoluteIndexes)
}
}There was a problem hiding this comment.
Using the HashReaderFunc is very clever, but the way you're calculating the offset is subtly wrong. The indexes TreeHash hands our reader aren't leaf numbers. They're stored hash indexes, a flat numbering of every node in the tree, with leaves and internal nodes interleaved. So i + lo is adding a leaf count to something that isn't a leaf position. The offset has to account for the node's level. A leaf may be lo away from another leaf, but a node spanning 8 leaves moves by only lo / 8.
So instead of offsetting the index, we decode it, shift its position by lo scaled to that level, and re-encode:
level, n := tlog.SplitStoredHashIndex(idx)
absolute := tlog.StoredHashIndex(level, (lo>>level)+n)Which gives us:
func rangeHash(lo, hi int64, reader tlog.HashReader) (tlog.Hash, error) {
return tlog.TreeHash(hi-lo, tlog.HashReaderFunc(
func(zeroBasedIndexes []int64) ([]tlog.Hash, error) {
absoluteIndexes := make([]int64, len(zeroBasedIndexes))
for i, idx := range zeroBasedIndexes {
level, n := tlog.SplitStoredHashIndex(idx)
absoluteIndexes[i] = tlog.StoredHashIndex(level, (lo>>level)+n)
}
return reader.ReadHashes(absoluteIndexes)
}))
}But fixing the offset surfaces our next problem. lo>>level is only exact when lo is a multiple of 2^level, which holds for every range we currently pass to rangeHash(). If a later change passes a lo that isn't, the shift drops bits, the index points to a different node, and we silently fold a wrong hash.
So we would need to add a guard:
func rangeHash(lo, hi int64, reader tlog.HashReader) (tlog.Hash, error) {
return tlog.TreeHash(hi-lo, tlog.HashReaderFunc(
func(zeroBasedIndexes []int64) ([]tlog.Hash, error) {
absoluteIndexes := make([]int64, len(zeroBasedIndexes))
for i, idx := range zeroBasedIndexes {
level, n := tlog.SplitStoredHashIndex(idx)
loAtLevel := lo >> level
if loAtLevel<<level != lo {
return nil, fmt.Errorf("subtreeHash: lo=%d is not a multiple of 2^%d", lo, level)
}
absoluteIndexes[i] = tlog.StoredHashIndex(level, loAtLevel+n)
}
return reader.ReadHashes(absoluteIndexes)
}))
}For what it's worth, your idea does work. That said, the version we have doesn't require this awkward translation layer that's somewhat difficult to reason about.
jsha
left a comment
There was a problem hiding this comment.
Looking great! I had some optional comment tweaks, but the code looks spec-compliant to me.
One high-level thing: I noticed ConsistencyProof emits one ReadHashes call per proof entry. This is clearly intentional - there's a test that checks the number of calls. But given how hashSubtree (and the tlog infrastructure) emphasizes coalescing into a single call, it's a surprise. I think it's fine to land as-is, particularly since a reorganization to coalesce all the ReadHashes calls would probably result in more complicated code. But maybe we should file a follow-up issue to look at performance and see if we can get the best of both worlds.
|
|
||
| // [start, end) covers only part of the node, so split the node at splitPoint. | ||
| // The switch routes by where the subtree falls (left child, right child, or | ||
| // straddle) and names the other child as the sibling the shared tail appends. |
There was a problem hiding this comment.
I'm not sure what the last four words in this sentence mean - "the shared tail appends"?
Maybe this was meant to say something like:
| // straddle) and names the other child as the sibling the shared tail appends. | |
| // straddle) and names the other child as the sibling. The hash of the sibling | |
| // will be appended to `proof` and returned. |
| // subtree's right edge meets the tree's right edge (sn == tn) or not. | ||
| if sn == tn { | ||
| // A flush subtree has no outside sibling to combine on the way up to | ||
| // nodeHash, so climb every level. |
There was a problem hiding this comment.
| // nodeHash, so climb every level. | |
| // nodeHash, so climb to the subtree root. |
| sr = nodeHash | ||
| rest = proof | ||
| } else { | ||
| // The subtree is larger, so the seed is proof[0], the largest complete |
| // fr and sr climb together from a shared seed: fr rebuilds the subtree | ||
| // hash, sr the tree root. |
There was a problem hiding this comment.
Nit:
| // fr and sr climb together from a shared seed: fr rebuilds the subtree | |
| // hash, sr the tree root. | |
| // fr and sr climb together from a shared seed: fr rebuilds the subtree | |
| // hash, sr the root hash. |
(uses parallel terms, also more closely matches the description in 4.4.3)
| // fr only combines while fn < sn. Freezing it at fn == sn is | ||
| // what makes the final fr == nodeHash check meaningful. |
There was a problem hiding this comment.
Optional:
| // fr only combines while fn < sn. Freezing it at fn == sn is | |
| // what makes the final fr == nodeHash check meaningful. | |
| // fr only combines while fn < sn. Once fn == sn, we've finished | |
| // with nodes below the subtree root and fr should have its final value. |
Reasoning: I wasn't sure what "meaningful" meant in the second sentence. I've tried to give an explanation that made sense to me. Please let me know if you think it's correct!
| // At the ragged right edge (sn == tn) the just-combined node is | ||
| // shorter than its left sibling, so skip its empty levels here, | ||
| // consuming no proof hash, until sn is odd again. |
There was a problem hiding this comment.
I don't understand this comment. Could you clarify? We could also drop it; the code matches the spec.
Implement the generation and verification of MTC subtree consistency proofs, which prove that an aligned interval of the log (a subtree) sits at that position in the tree under a given root hash. The MTC draft defines these in section 4.
x/mod's sumdb/tlog provides the hash primitives and the stored-hash reader these functions use. Its proofs cover only two cases: an inclusion proof for a single entry and a consistency proof for a prefix of the log. MTC requires a proof for any subtree. x/mod has no such proof.
Export two functions:
A note to reviewers: this package and its tests were written with assistance from Claude Opus 4.8 and Fable 5. That being said, it has been reviewed and extensively revised by myself before submission.