From 05c71381c027b5be25918be7164ba9d734b4a77d Mon Sep 17 00:00:00 2001 From: Robert Hambrock Date: Thu, 21 Jul 2022 17:09:21 +0200 Subject: [PATCH] pallet-mmr: fix batch proof failures (#11840) * pallet-mmr: extend batch proof verification test covers all possible 2-leaf combinations now, including current verification failures that batch proof item count limit is too low sometimes. * raise upper bound on proof item number as described in https://github.com/paritytech/substrate/issues/11753#issuecomment-1179838174 * test for powerset of leaves * refactor batch proof verification test * test all batch proofs for mmr sizes up to n=13 * limit mmr size to reduce batch proof test duration * use saturating integer addition for proof check * extract common chain building in batch proof tests note: right now, since not killing old chain, it keeps growing by 7 blocks for every leaf selection (added after proof generation), hence heavier to compute. * only add blocks after a proof generation once * increase batch proof testing range * register offchain extensions only once * fmt & remove unused util --- substrate/Cargo.lock | 1 + .../frame/merkle-mountain-range/Cargo.toml | 1 + .../frame/merkle-mountain-range/src/lib.rs | 2 +- .../merkle-mountain-range/src/mmr/storage.rs | 2 +- .../merkle-mountain-range/src/mmr/utils.rs | 22 +----- .../frame/merkle-mountain-range/src/tests.rs | 70 ++++++++++++++----- 6 files changed, 58 insertions(+), 40 deletions(-) diff --git a/substrate/Cargo.lock b/substrate/Cargo.lock index 279864e056..3d5bfe5e51 100644 --- a/substrate/Cargo.lock +++ b/substrate/Cargo.lock @@ -5871,6 +5871,7 @@ dependencies = [ "frame-support", "frame-system", "hex-literal", + "itertools", "parity-scale-codec", "scale-info", "sp-core", diff --git a/substrate/frame/merkle-mountain-range/Cargo.toml b/substrate/frame/merkle-mountain-range/Cargo.toml index f5f8bdee08..75301afed0 100644 --- a/substrate/frame/merkle-mountain-range/Cargo.toml +++ b/substrate/frame/merkle-mountain-range/Cargo.toml @@ -27,6 +27,7 @@ sp-std = { version = "4.0.0", default-features = false, path = "../../primitives [dev-dependencies] env_logger = "0.9" hex-literal = "0.3" +itertools = "0.10.3" [features] default = ["std"] diff --git a/substrate/frame/merkle-mountain-range/src/lib.rs b/substrate/frame/merkle-mountain-range/src/lib.rs index 9274e8e72c..4644ebcb7d 100644 --- a/substrate/frame/merkle-mountain-range/src/lib.rs +++ b/substrate/frame/merkle-mountain-range/src/lib.rs @@ -351,7 +351,7 @@ impl, I: 'static> Pallet { ) -> Result<(), primitives::Error> { if proof.leaf_count > Self::mmr_leaves() || proof.leaf_count == 0 || - proof.items.len() as u32 > mmr::utils::NodesUtils::new(proof.leaf_count).depth() + (proof.items.len().saturating_add(leaves.len())) as u64 > proof.leaf_count { return Err(primitives::Error::Verify .log_debug("The proof has incorrect number of leaves or proof items.")) diff --git a/substrate/frame/merkle-mountain-range/src/mmr/storage.rs b/substrate/frame/merkle-mountain-range/src/mmr/storage.rs index d31262d4d7..8b623edf56 100644 --- a/substrate/frame/merkle-mountain-range/src/mmr/storage.rs +++ b/substrate/frame/merkle-mountain-range/src/mmr/storage.rs @@ -136,7 +136,7 @@ where let window_size = ::BlockHashCount::get().unique_saturated_into(); if leaves >= window_size { - // Move the rolling window towards the end of `block_num->hash` mappings available + // Move the rolling window towards the end of `block_num->hash` mappings available // in the runtime: we "canonicalize" the leaf at the end, let to_canon_leaf = leaves.saturating_sub(window_size); // and all the nodes added by that leaf. diff --git a/substrate/frame/merkle-mountain-range/src/mmr/utils.rs b/substrate/frame/merkle-mountain-range/src/mmr/utils.rs index 3734ea5147..0b8e88a928 100644 --- a/substrate/frame/merkle-mountain-range/src/mmr/utils.rs +++ b/substrate/frame/merkle-mountain-range/src/mmr/utils.rs @@ -46,15 +46,6 @@ impl NodesUtils { 2 * self.no_of_leaves - self.number_of_peaks() } - /// Calculate maximal depth of the MMR. - pub fn depth(&self) -> u32 { - if self.no_of_leaves == 0 { - return 0 - } - - 64 - self.no_of_leaves.next_power_of_two().leading_zeros() - } - /// Calculate `LeafIndex` for the leaf that added `node_index` to the MMR. pub fn leaf_index_that_added_node(node_index: NodeIndex) -> LeafIndex { let rightmost_leaf_pos = Self::rightmost_leaf_node_index_from_pos(node_index); @@ -128,18 +119,7 @@ mod tests { } #[test] - fn should_calculate_number_of_leaves_correctly() { - assert_eq!( - vec![0, 1, 2, 3, 4, 9, 15, 21] - .into_iter() - .map(|n| NodesUtils::new(n).depth()) - .collect::>(), - vec![0, 1, 2, 3, 3, 5, 5, 6] - ); - } - - #[test] - fn should_calculate_depth_correclty() { + fn should_calculate_depth_correctly() { assert_eq!( vec![0, 1, 2, 3, 4, 9, 15, 21] .into_iter() diff --git a/substrate/frame/merkle-mountain-range/src/tests.rs b/substrate/frame/merkle-mountain-range/src/tests.rs index 53226f8419..566a051823 100644 --- a/substrate/frame/merkle-mountain-range/src/tests.rs +++ b/substrate/frame/merkle-mountain-range/src/tests.rs @@ -347,28 +347,64 @@ fn should_verify() { } #[test] -fn should_verify_batch_proof() { +fn should_verify_batch_proofs() { + fn generate_and_verify_batch_proof( + ext: &mut sp_io::TestExternalities, + leaves: &Vec, + blocks_to_add: usize, + ) { + let (leaves, proof) = ext + .execute_with(|| crate::Pallet::::generate_batch_proof(leaves.to_vec()).unwrap()); + + ext.execute_with(|| { + add_blocks(blocks_to_add); + // then + assert_eq!(crate::Pallet::::verify_leaves(leaves, proof), Ok(())); + }) + } + let _ = env_logger::try_init(); - // Start off with chain initialisation and storing indexing data off-chain - // (MMR Leafs) + use itertools::Itertools; + let mut ext = new_test_ext(); - ext.execute_with(|| add_blocks(7)); - ext.persist_offchain_overlay(); - - // Try to generate proof now. This requires the offchain extensions to be present - // to retrieve full leaf data. + // require the offchain extensions to be present + // to retrieve full leaf data when generating proofs register_offchain_ext(&mut ext); - let (leaves, proof) = ext.execute_with(|| { - // when - crate::Pallet::::generate_batch_proof(vec![0, 4, 5]).unwrap() - }); - ext.execute_with(|| { - add_blocks(7); - // then - assert_eq!(crate::Pallet::::verify_leaves(leaves, proof), Ok(())); - }); + // verify that up to n=10, valid proofs are generated for all possible leaf combinations + for n in 0..10 { + ext.execute_with(|| new_block()); + ext.persist_offchain_overlay(); + + // generate powerset (skipping empty set) of all possible leaf combinations for mmr size n + let leaves_set: Vec> = (0..n).into_iter().powerset().skip(1).collect(); + + leaves_set.iter().for_each(|leaves_subset| { + generate_and_verify_batch_proof(&mut ext, leaves_subset, 0); + ext.persist_offchain_overlay(); + }); + } + + // verify that up to n=15, valid proofs are generated for all possible 2-leaf combinations + for n in 10..15 { + // (MMR Leafs) + ext.execute_with(|| new_block()); + ext.persist_offchain_overlay(); + + // generate all possible 2-leaf combinations for mmr size n + let leaves_set: Vec> = (0..n).into_iter().combinations(2).collect(); + + leaves_set.iter().for_each(|leaves_subset| { + generate_and_verify_batch_proof(&mut ext, leaves_subset, 0); + ext.persist_offchain_overlay(); + }); + } + + generate_and_verify_batch_proof(&mut ext, &vec![7, 11], 20); + ext.execute_with(|| add_blocks(1000)); + ext.persist_offchain_overlay(); + generate_and_verify_batch_proof(&mut ext, &vec![7, 11, 100, 800], 100); } #[test]