Skip to content

Commit

Permalink
fixes incorrect sibling hash caching during witness creation (#4849)
Browse files Browse the repository at this point in the history
when calculating witnesses at past tree sizes we cache the sibling hashes of
left nodes at the past tree size

the merkle tree has separate datastores for nodes and leaves. the cache is keyed
by node index, not leaf index.

when building the cache we incorrectly set a leaf index to a sibling hash. if
that index is visited as a node index when creating the witness, then the cached
value may be used incorrectly resulting in an invalid commitment.

the commitment may still be correct if that node index corresponds to a right
node or if the node index is overwritten by the correct sibling hash in the
cache while building the past root

there are four criteria for the merkle tree, past size, and witness index that
produce an incorrect commitment:
1. the leaf at leaf index `pastSize - 1` is a right leaf
2. the node at node index `pastSize - 2` is a left node
3. the node at node index `pastSize - 2` is _not_ on the path from the leaf
`pastSize - 1` to the root (or the incorrect cache would be overwritten
4. the node at node index `pastSize - 2` is on the path from the witness leaf
index to the root

these criteria depend on the tree size, past size, and witness index, so
reproducing the incorrect commitment is somewhat random. an example that
produces the incorrect commitment is a tree size of 128, a pasts ize of 74, and
witness indexes of 68 or 69

adds regression test for witness creation issue

treeSize 128
pastSize 74
index 68 (or 69)
  • Loading branch information
hughy authored Mar 14, 2024
1 parent e6d6b00 commit 264df7b
Show file tree
Hide file tree
Showing 2 changed files with 47 additions and 1 deletion.
47 changes: 47 additions & 0 deletions ironfish/src/merkletree/merkletree.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -662,6 +662,53 @@ describe('Merkle tree', function () {
}
})

it('calculates witnesses without past sibling hash caching regression', async () => {
/* eslint-disable prettier/prettier */
// construct tree of size 128
const leaves = [
'aa', 'ab', 'ac', 'ad', 'ae', 'af', 'ag', 'ah',
'ba', 'bb', 'bc', 'bd', 'be', 'bf', 'bg', 'bh',
'ca', 'cb', 'cc', 'cd', 'ce', 'cf', 'cg', 'ch',
'da', 'db', 'dc', 'dd', 'de', 'df', 'dg', 'dh',
'ea', 'eb', 'ec', 'ed', 'ee', 'ef', 'eg', 'eh',
'fa', 'fb', 'fc', 'fd', 'fe', 'ff', 'fg', 'fh',
'ga', 'gb', 'gc', 'gd', 'ge', 'gf', 'gg', 'gh',
'ha', 'hb', 'hc', 'hd', 'he', 'hf', 'hg', 'hh',
'ia', 'ib', 'ic', 'id', 'ie', 'if', 'ig', 'ih',
'ja', 'jb', 'jc', 'jd', 'je', 'jf', 'jg', 'jh',
'ka', 'kb', 'kc', 'kd', 'ke', 'kf', 'kg', 'kh',
'la', 'lb', 'lc', 'ld', 'le', 'lf', 'lg', 'lh',
'ma', 'mb', 'mc', 'md', 'me', 'mf', 'mg', 'mh',
'na', 'nb', 'nc', 'nd', 'ne', 'nf', 'ng', 'nh',
'oa', 'ob', 'oc', 'od', 'oe', 'of', 'og', 'oh',
'pa', 'pb', 'pc', 'pd', 'pe', 'pf', 'pg', 'ph',
]
/* eslint-enable prettier/prettier */

const tree = await makeTree({ depth: 7 })
for (const leaf of leaves) {
await tree.add(leaf)
}

// a tree size of 128, a past tree size of 74, and an index of 68 created an
// issue with pastRightSiblingHashes where the cache incorrectly stored a
// hash for the parent of leaf index 68 (at node index 72)
// the conditions to produce this issue:
// 1. leaf at pastSize - 1 is a right leaf node
// 2. node at pastSize - 2 is a left node
// 3. node at pastSize - 2 is not on path from leaf pastSize - 1 to root
// 4. node at pastSize - 2 is on path from witness index to root
const pastSize = 74
const pastRootHash = await tree.pastRoot(pastSize)

const index = 68
const witness = await tree.witness(index, pastSize)
if (witness === null) {
throw new Error('Witness should not be null')
}
expect(witness.rootHash).toEqual(pastRootHash)
})

it('witness rootHash should equal the tree rootHash', async () => {
const tree = await makeTree({ depth: 3, leaves: 'abcdefgh' })

Expand Down
1 change: 0 additions & 1 deletion ironfish/src/merkletree/merkletree.ts
Original file line number Diff line number Diff line change
Expand Up @@ -598,7 +598,6 @@ export class MerkleTree<
const sibling = await this.getLeaf(leafIndex - 1, tx)
const siblingHash = sibling.merkleHash
currentHash = this.hasher.combineHash(0, siblingHash, currentHash)
pastHashes.set(leafIndex - 1, currentHash)
} else {
currentHash = this.hasher.combineHash(0, currentHash, currentHash)
}
Expand Down

0 comments on commit 264df7b

Please sign in to comment.