Reduce zero-padding and cache zero-hashes in k-ary MerkleTree #2407
Conversation
Signed-off-by: ljedrz <ljedrz@gmail.com>
Signed-off-by: ljedrz <ljedrz@gmail.com>
|
The regressions turned out to be simpler to iron out than expected: The empty tree case is still a bit worse than before, but it's a tiny difference measured in µs and not the typical scenario. Also, that one seems likely to fluctuate - I just re-ran it (without any changes), and saw a -6% difference. |
| @@ -170,7 +170,7 @@ fn check_merkle_tree_depth_3_arity_3_padded<LH: LeafHash<Hash = PH::Hash>, PH: P | |||
|
|
|||
| // Construct the Merkle tree for the given leaves. | |||
| let merkle_tree = KaryMerkleTree::<LH, PH, 3, ARITY>::new(leaf_hasher, path_hasher, &leaves)?; | |||
| assert_eq!(40, merkle_tree.tree.len()); | |||
| assert_eq!(25, merkle_tree.tree.len()); | |||
There was a problem hiding this comment.
With this PR, the nodes that would only contain empty hashes (padding) no longer need to allocate them at the leaf level, which means that the tree ends up being smaller; you can see a calculation of the difference in the current number of leaves - padded for the entire leaf level - and the one this PR proposes (only padded up to the arity) here. In case of this particular test case (arity = 3, depth = 3, leaves = 10), the number of tree members (at the leaf level) that are just padding is currently 17, which this PR takes down to 2, which is a reduction of 15, leading to the corresponding reduction in tree size.
There was a problem hiding this comment.
Does this change apply to the merkle-tree too? (non-k-ary one) If so we shoudl optimize and profile that one too.
There was a problem hiding this comment.
I've already done that; the wins aren't as great, but I'm happy to file a PR soon if this direction is desirable.
There was a problem hiding this comment.
|
@ljedrz is this a breaking PR change? Does the Merkle tree's expected outputs and roots change in any way? |
|
I don't think so - hashes for the nodes containing empty hashes are calculated as if the empty hashes were there, they are only not physically present in the tree; the number of leaves field is not affected either; are there any test vectors I could double-check against? |
|
For those interested, the equivalence tests can be found in this repo. (currently private but the AleoHQ/snarkVM team has access) A sample equivalence test for the use snarkvm_old::ledger::puzzle::Puzzle as OldPuzzle;
use snarkvm_old::ledger::store::helpers::memory::ConsensusMemory as OldMemory;
use snarkvm_old::prelude::{
Address as OldAddress, MainnetV0 as OldNetwork, Network as OldNetworkTrait, Uniform,
};
use snarkvm_old::synthesizer::vm::VM as OldVM;
use snarkvm_new::ledger::puzzle::Puzzle as NewPuzzle;
use snarkvm_new::ledger::store::helpers::memory::ConsensusMemory as NewMemory;
use snarkvm_new::prelude::{
Address as NewAddress, MainnetV0 as NewNetwork, Network as NewNetworkTrait,
};
use snarkvm_new::synthesizer::vm::VM as NewVM;
use anyhow::Result;
use std::str::FromStr;
const ITERATIONS: u128 = 100;
fn check_equivalency(
old_puzzle: &OldPuzzle<OldNetwork>,
new_puzzle: &NewPuzzle<NewNetwork>,
) -> Result<()> {
// Initialize an RNG.
let rng = &mut rand::thread_rng();
// Check that the puzzles produce equivalent solutions.
for _ in 0..ITERATIONS {
// Generate a random epoch hash.
let old_epoch_hash = <OldNetwork as OldNetworkTrait>::BlockHash::rand(rng);
let new_epoch_hash =
<NewNetwork as NewNetworkTrait>::BlockHash::from_str(&format!("{old_epoch_hash}"))?;
// Generate a random address and counter.
let old_address = OldAddress::<OldNetwork>::rand(rng);
let new_address = NewAddress::<NewNetwork>::from_str(&format!("{old_address}"))?;
let old_counter = u64::rand(rng);
let new_counter = u64::from_str(&format!("{old_counter}"))?;
let old_solution = old_puzzle.prove(old_epoch_hash, old_address, old_counter, None)?;
let new_solution = new_puzzle.prove(new_epoch_hash, new_address, new_counter, None)?;
let old_proof_target = old_puzzle.get_proof_target(&old_solution)?;
let new_proof_target = new_puzzle.get_proof_target(&new_solution)?;
assert_eq!(old_proof_target, new_proof_target);
}
Ok(())
}
#[test]
fn test_puzzles_are_equivalent() -> Result<()> {
// Construct the old puzzle.
let old_puzzle = OldVM::<OldNetwork, OldMemory<OldNetwork>>::new_puzzle()?;
// Construct the new puzzle.
let new_puzzle = NewVM::<NewNetwork, NewMemory<NewNetwork>>::new_puzzle()?;
// Check that the old and new puzzles produce equivalent solutions.
check_equivalency(&old_puzzle, &new_puzzle)?;
Ok(())
} |
Signed-off-by: ljedrz <ljedrz@gmail.com>
Signed-off-by: ljedrz <ljedrz@gmail.com>
Signed-off-by: ljedrz <ljedrz@gmail.com>
Signed-off-by: ljedrz <ljedrz@gmail.com>
Signed-off-by: ljedrz <ljedrz@gmail.com>
|
I'm going to check if any of these review comments apply to https://github.com/AleoHQ/snarkVM/pull/2415 as well. Edit: it seems that way, but let's finish this PR first so as to make sure that everything is in good shape. |
There was a problem hiding this comment.
As far as I can tell: this PR does not impact completeness/soundness of the merkle tree opening proof, because it produces merkle tree nodes which are identical to before.
Note that due to the particular imlementation, we can't prove membership anymore of empty leaves outside of range of the originally supplied leaves, which seems like a desirable bonus.
Signed-off-by: ljedrz <ljedrz@gmail.com>
Signed-off-by: ljedrz <ljedrz@gmail.com>
|
I'll rerun the benchmarks for this and the other PR shortly. |
|
The updated benchmark results (skipping the Which is a further improvement across the board. |
When creating k-ary Merkle trees, we currently pad all the "missing" leaves, which can result in a large number of empty hashes that all end up being allocated, and then repeatedly included in the calculation of further hashes. In order to improve performance for all such scenarios, we can omit some of the padding, and cache the hashes that derive from the zero hash, which is what this PR proposes.
While there are still ways in which the performance can be improved further, these changes aren't exactly trivial already, and it seems like a good moment to open them to discussion. Below are some benchmarks that I'll be referring to when outlining the potential for future improvement.
These are all wins, with a significantly lower both maximum and current (once the tree is created) heap use.
The creation of an empty and a full tree see regressions, as those scenarios don't benefit from the caching of empty hashes. I have an idea on how to improve them, which will be posted as an additional commit if the diff isn't too large.