Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

blockchain: rolling merkle root #1421

Closed
wants to merge 5 commits into from

Conversation

cfromknecht
Copy link
Contributor

@cfromknecht cfromknecht commented Apr 18, 2019

This PR adds a new RollingMerkleTree type for efficiently computing merkle roots. The leaves
of the tree are iteratively pushed onto the tree, and any complete substrees are evaluated and
stored so that subsequent pushes may consume them when their sibling tree is complete. As a
result, this requires O(log n) additional memory be allocated. The current BuildMerkleTreeStore
allocates an array of up to 4 times the number of leaves, which is used to store internal nodes and the root.

For 2000 leaves, the benchmarks show that this method is about 7% slower (~90 microseconds), but uses 99.04% less memory and 99.85% fewer allocations. These allocations reduce gc pressure, especially during IBD when the large slices of hashes currently used are almost immediately discarded.

BenchmarkMerkle/rolling-1000-8              2000            694382 ns/op             858 B/op          3 allocs/op
BenchmarkMerkle/rolling-2000-8              1000           1367763 ns/op             923 B/op          3 allocs/op
BenchmarkMerkle/rolling-4000-8               500           2900103 ns/op            1210 B/op          6 allocs/op
BenchmarkMerkle/rolling-8000-8               300           5799806 ns/op            2034 B/op         10 allocs/op
BenchmarkMerkle/rolling-16000-8              100          11572403 ns/op            1578 B/op          3 allocs/op
BenchmarkMerkle/rolling-32000-8               50          22949069 ns/op            1720 B/op          5 allocs/op
BenchmarkMerkle/old-1000-8                  2000            655435 ns/op           48416 B/op       1002 allocs/op
BenchmarkMerkle/old-2000-8                  1000           1277192 ns/op           96800 B/op       2002 allocs/op
BenchmarkMerkle/old-4000-8                   500           2533821 ns/op          193568 B/op       4002 allocs/op
BenchmarkMerkle/old-8000-8                   300           5029310 ns/op          387104 B/op       8002 allocs/op              
BenchmarkMerkle/old-16000-8                  200          10135631 ns/op          774176 B/op      16002 allocs/op                     
BenchmarkMerkle/old-32000-8                  100          20442194 ns/op         1548340 B/op      32002 allocs/op

alternative to #1377

@cfromknecht cfromknecht force-pushed the rolling-merkle branch 3 times, most recently from 39b1775 to bd42fc6 Compare April 19, 2019 02:31
This commit adds an explicit assertion that the merkle root
is properly computed when an odd number of leaves are present, and a
left node must be hashed with itself to create a valid root.
type RollingMerkleTree struct {
// nodes contains all nodes in the tree. The nodes may correspond to
// interior nodes if their children have already been pruned.
nodes map[uint32]chainhash.Hash
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't this be better served as a slice? So we just index in rather than having the overhead of a map. We'd also gain memory locality for all the items as well.

merkleStoreTree := BuildMerkleTreeStore(block.Transactions(), false)
merkleStoreRoot := merkleStoreTree[len(merkleStoreTree)-1]

if calcMerkleRoot != *merkleStoreRoot {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@jakesylvestre
Copy link
Collaborator

@jcvernaleo (as per #1530)

@Rjected
Copy link
Collaborator

Rjected commented Jun 8, 2020

@cfromknecht mind rebasing so the actions can run?

@Roasbeef
Copy link
Member

Superseded by #1979

@Roasbeef Roasbeef closed this Aug 10, 2023
@moza88
Copy link

moza88 commented Jun 18, 2024

I noticed BuildMerkleTreeStore is still in the codebase even though it's replaced with RollingMerkleTree and CalcMerkleRoot. Should we remove it from blockchain/merkle.go and blockchain/merkle_test.go?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants