Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Support MMR Pruning #9700

Merged

Conversation

AurevoirXavier
Copy link
Contributor

@AurevoirXavier AurevoirXavier commented Sep 6, 2021

Add pruning process to the MMR pallet.

But I have encountered a problem with the verify_leaf part.
should_verify_on_the_next_block_since_there_is_no_pruning_yet this failed.
We can't get the root at some moment (except the latest one) if we perform pruning on the Nodes.

@tomusdrw Any idea on that?

Closes paritytech/grandpa-bridge-gadget#64

@bkchr bkchr requested a review from tomusdrw September 6, 2021 07:26
@bkchr bkchr added A0-please_review Pull request needs code review. B7-runtimenoteworthy labels Sep 6, 2021
@bkchr bkchr added the C1-low PR touches the given topic and has a low impact on builders. label Sep 6, 2021
@hackfisher
Copy link
Contributor

hackfisher commented Sep 6, 2021

But I have encountered a problem with the verify_leaf part.
should_verify_on_the_next_block_since_there_is_no_pruning_yet this failed.
We can't get the root at some moment (except the latest one) if we perform pruning on the Nodes.

After support pruning, ModuleMmr<mmr::storage::RuntimeStorage, T, I> can only be used to verify proof generated agains latest root, proof generated agains previous roots(and its leaves) will require previous roots(or leaves) for verification.

In this test, root in runtime storage has already been changed, and leaves for generating that root have already been pruned after new_block();.

OffchainStorage might be need to be used for generating and verifying proof.

Copy link
Contributor

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

Cool stuff, thanks!

I think the idea of removing nodes on every insertion is sensible - it's going to give us much more predictable on_initialize weight compared to defered pruning.

We have to be careful though, to asses what the actual weight is going to be. The computation time is now (slowly) growing depending on the MMR size (more/less peaks), so the formula and benchmarking needs to be updated accordingly.

Comment on lines 103 to 119
let elems = elems
.into_iter()
.enumerate()
.map(|(i, elem)| (size + i as NodeIndex, elem))
.collect::<Vec<_>>();
let leaves = elems.iter().fold(leaves, |acc, (pos, elem)| {
// Indexing API is used to store the full leaf content.
let key = Pallet::<T, I>::offchain_key(size);
elem.using_encoded(|elem| sp_io::offchain_index::set(&key, elem));
size += 1;
elem.using_encoded(|elem| {
offchain_index::set(&Pallet::<T, I>::offchain_key(*pos), elem)
});

if let Node::Data(..) = elem {
leaves += 1;
acc + 1
} else {
acc
}
}
});
Copy link
Contributor

Choose a reason for hiding this comment

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

These changes seem unnecessary and imho increase both code complexity and the number of iterations + extra allocation.

Copy link
Contributor Author

@AurevoirXavier AurevoirXavier Sep 9, 2021

Choose a reason for hiding this comment

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

I don't like mutable variable. IMO this simplify the code.
But yes, a little bit more allocation here. Could you accept this?

frame/merkle-mountain-range/src/mmr/storage.rs Outdated Show resolved Hide resolved
frame/merkle-mountain-range/src/mmr/storage.rs Outdated Show resolved Hide resolved
frame/merkle-mountain-range/src/mmr/storage.rs Outdated Show resolved Hide resolved
frame/merkle-mountain-range/src/mmr/storage.rs Outdated Show resolved Hide resolved
@AurevoirXavier
Copy link
Contributor Author

Cool stuff, thanks!

I think the idea of removing nodes on every insertion is sensible - it's going to give us much more predictable on_initialize weight compared to defered pruning.

We have to be careful though, to asses what the actual weight is going to be. The computation time is now (slowly) growing depending on the MMR size (more/less peaks), so the formula and benchmarking needs to be updated accordingly.

How do we benchmark this?

@AurevoirXavier
Copy link
Contributor Author

@tomusdrw Please review.

@AurevoirXavier
Copy link
Contributor Author

@tomusdrw We really need this feature! 🙏🏻

@tomusdrw
Copy link
Contributor

@AurevoirXavier apologies for the delay, reviewing this PR is high on my list, hoping to get this done this week.

@AurevoirXavier
Copy link
Contributor Author

AurevoirXavier commented Oct 25, 2021

@AurevoirXavier apologies for the delay, reviewing this PR is high on my list, hoping to get this done this week.

@tomusdrw Any progress?

@adoerr adoerr self-requested a review November 11, 2021 06:51
@AurevoirXavier
Copy link
Contributor Author

@adoerr Could this be merged?

@adoerr
Copy link
Contributor

adoerr commented Nov 16, 2021

@tomusdrw is giving it another look right now

@tomusdrw
Copy link
Contributor

@tomusdrw is giving it another look right now

Proposed my changes here: darwinia-network#123
Main ares of focus:

  • Distinction between NodeIndex and LeafIndex types to avoid confusion
  • Code reduction, removing extra loops and allocations

Simplification of insertion & pruning code
@tomusdrw
Copy link
Contributor

@AurevoirXavier could you run cargo fmt to make the CI happy?

@adoerr adoerr self-requested a review November 18, 2021 16:06
@tomusdrw tomusdrw added the D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit. label Nov 23, 2021
@tomusdrw
Copy link
Contributor

bot merge

@paritytech-processbot
Copy link

Waiting for commit status.

@paritytech-processbot paritytech-processbot bot merged commit 7a531b0 into paritytech:master Nov 23, 2021
@AurevoirXavier
Copy link
Contributor Author

AurevoirXavier commented Nov 23, 2021

Just a note here, the weights were not getting updated.


@tomusdrw

Could you mention me while updating the weights?
I'm very interested in how to bench this.

AurevoirXavier added a commit to darwinia-network/substrate that referenced this pull request Jan 13, 2022
* Use `0.3.2`

* Replace `u64` with `NodeIndex`

* Fix Typo

* Add Pruning Logic

* Fix Some Tests

* Remove Comment

* Log Only Under STD

* Return while No Element to Append

* Optimize Pruning Algorithm

* Update Doc

* Update Doc

* Zero Copy Algorithm

* Import Missing Type

* Fix Merge Mistake

* Import Missing Item

* Make `verify` Off-Chain

* `cargo fmt`

* Avoid using NodeIndex in incorrect places.

* Simplify pruning.

* Format

Co-authored-by: Tomasz Drwięga <[email protected]>
@AurevoirXavier AurevoirXavier deleted the xavier-mmr-pruning branch March 2, 2022 06:42
grishasobol pushed a commit to gear-tech/substrate that referenced this pull request Mar 28, 2022
* Use `0.3.2`

* Replace `u64` with `NodeIndex`

* Fix Typo

* Add Pruning Logic

* Fix Some Tests

* Remove Comment

* Log Only Under STD

* Return while No Element to Append

* Optimize Pruning Algorithm

* Update Doc

* Update Doc

* Zero Copy Algorithm

* Import Missing Type

* Fix Merge Mistake

* Import Missing Item

* Make `verify` Off-Chain

* `cargo fmt`

* Avoid using NodeIndex in incorrect places.

* Simplify pruning.

* Format

Co-authored-by: Tomasz Drwięga <[email protected]>
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
* Use `0.3.2`

* Replace `u64` with `NodeIndex`

* Fix Typo

* Add Pruning Logic

* Fix Some Tests

* Remove Comment

* Log Only Under STD

* Return while No Element to Append

* Optimize Pruning Algorithm

* Update Doc

* Update Doc

* Zero Copy Algorithm

* Import Missing Type

* Fix Merge Mistake

* Import Missing Item

* Make `verify` Off-Chain

* `cargo fmt`

* Avoid using NodeIndex in incorrect places.

* Simplify pruning.

* Format

Co-authored-by: Tomasz Drwięga <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MMR non-peak nodes pruning
5 participants