feat(consensus)!: add balanced binary merkle tree #5189
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
SWvheerden
previously approved these changes
Feb 20, 2023
jorgeantonio21
previously approved these changes
Feb 20, 2023
Cifko
dismissed stale reviews from jorgeantonio21 and SWvheerden
via
d69971a
Cifko
force-pushed
the
balanced-binary-merkle-tree
branch
from
a3d9495
to
fb95396
Compare
stringhandler
changed the title
Cifko
force-pushed
the
balanced-binary-merkle-tree
branch
from
fb95396
to
1550b39
Compare
AaronFeickert
requested changes
Feb 24, 2023
Comment on lines
+622
to
+626
| let vn_bmt = ValidatorNodeBMT::create(vec![Blake256::new() | ||
| .chain(public_key.as_bytes()) | ||
| .chain(shard_key.as_slice()) | ||
| .finalize() | ||
| .to_vec()]); |
There was a problem hiding this comment.
Unless there's a compelling reason not to, this should use a domain-separated hasher.
There was a problem hiding this comment.
This is just a test. The balanced tree is using domain-separated hasher. But as I'm looking at the code for the hash of VNs we are not using the domain-separated hasher there. But that's not part of this PR.
There was a problem hiding this comment.
Whoops, made this comment in the wrong spot. Sorry for the confusion.
There was a problem hiding this comment.
Opened an issue for this, so another PR can address it.
stringhandler
pushed a commit
that referenced
this pull request
Mar 3, 2023
Description --- Removes dead validator node Merkle mountain range (MMR) code left over from [another PR](#5189) and renames a few things for clarity. Motivation and Context --- An earlier PR moves validator node data from an MMR to a balanced binary Merkle tree, but leaves some old MMR code around. This PR removes that code and does some minor renaming for clarity. How Has This Been Tested? --- Existing tests pass. It should be separately confirmed that none of the removed or renamed functionality needs to be externally available.
stringhandler
pushed a commit
that referenced
this pull request
Mar 8, 2023
### ⚠ BREAKING CHANGES * **peer_db:** more accurate peer stats per address (5142) * use consensus hashing API for validator node MMR (5207) * **consensus:** add balanced binary merkle tree (5189) ### Features * add favourite flag to contact ([5217](#5217)) ([0371b60](0371b60)) * add indexer config ([5210](#5210)) ([cf95601](cf95601)) * add merge proof for balanced binary merkle tree ([5193](#5193)) ([8962909](8962909)) * **consensus:** add balanced binary merkle tree ([5189](#5189)) ([8d34e8a](8d34e8a)) * log to base dir ([#5197](#5197)) ([5147b5c](5147b5c)) * **peer_db:** more accurate peer stats per address ([5142](#5142)) ([fdad1c6](fdad1c6)) ### Bug Fixes * add grpc commitment signature proto type ([5200](#5200)) ([d523f1e](d523f1e)) * peer seeds for esme/igor ([5202](#5202)) ([1bc226c](1bc226c)) * remove panics from merged BBMT verification ([5221](#5221)) ([a4c5fce](a4c5fce)) * source coverage ci failure ([5209](#5209)) ([80294a1](80294a1)) * use consensus hashing API for validator node MMR ([5207](#5207)) ([de28115](de28115)) * wallet reuse existing tor address ([5092](#5092)) ([576f44e](576f44e)) * **wallet:** avoids empty addresses in node identity ([5224](#5224)) ([1a66312](1a66312))
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Implement balanced binary merkle tree and use it instead of mmr for vns. It has no effect on the base layer functionality apart for being a breaking change (it changes the vn merkle root). In the base layer only the generation is used. The proofs are used only in
tari-dan(signing votes).The advantages of MMR over binary tree is the time complexity when adding an element. But that's not our case. We regenerate the tree everytime. So it's much simpler to generate balanced binary tree. Which is the best option for this. The proof time/size for binary tree is always log n. For MMR the worst case scenario is 2*log n (when there is peak of every possible height).
The merged proofs are part of another PR.
Some comparison between balanced binary merkle tree vs merkle mountain range.
The data set I used was number of VNs of 1000 to 1M (step by 1000), committee size from 10 to 200 (step by 10) (In reality the committee can be bigger, this is representing the number of proofs).
For generating the proofs the random subset of VNs were selected (same for BMT and MMR).
Generating merged proofs takes 85.96% of the time
In the graph below the green represents only the merging time. But in reality we need to generate the proof anyway and then merge them (figure above takes this into account)
Verifying merged proofs takes 54.77% of the time
Merged proof size is 9.47% of the size
How Has This Been Tested?
There are couple cargo tests.
This is a breaking change : The Merkle root for VNs is different. It's equal in the genesis block, but once there is a single vn registration it starts to differ.