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

Merkle Mountain Range & BEEFY integration #2101

Merged
merged 99 commits into from
Apr 1, 2021
Merged

Merkle Mountain Range & BEEFY integration #2101

merged 99 commits into from
Apr 1, 2021

Conversation

tomusdrw
Copy link
Contributor

@tomusdrw tomusdrw commented Dec 9, 2020

This is a draft PR of a potential MMR & BEEFY integration with Rococo.

There is a lot of things to polish and finish up here, but opening it up for discussion opportunity and visibility for external teams.

Open questions:

  1. Shall the ETH bridge support Parathreads?
  2. Can we efficiently update the Merkle Tree of ParaHeads?
  3. Shall we lift a particular parachain header digest item to the MMR leaf directly. Currently the bridge needs to include entire ParaHead to prove anything about a parachain. To make these proofs smaller we could create another entry in MmrLeaf and extract a particular digest item from each ParaHead, then merkelize them in addition to headers.

Future Work:

  • Start BEEFY service
  • Cache BEEFY merkle tree
  • Session Keys migration
  • Westend integration
  • Update parachains merkle tree efficiently (only changed parachains)

@github-actions github-actions bot added the A3-in_progress Pull request is in progress. No review needed at this stage. label Dec 9, 2020
@tomusdrw tomusdrw removed A1-onice A3-in_progress Pull request is in progress. No review needed at this stage. labels Jan 11, 2021
@tomusdrw tomusdrw added the C1-low PR touches the given topic and has a low impact on builders. label Jan 11, 2021
@tomusdrw tomusdrw marked this pull request as ready for review January 12, 2021 12:37
@tomusdrw
Copy link
Contributor Author

@bkchr @rphmeier feel free to take a look already, but AFAIU to deploy to Rococo we need to have the session keys migration implemented, right?

The parachain merkle tree construction optimization is something I'd left out for a separate PR, I feel this is not going to be critical for Rococo.

@rphmeier
Copy link
Contributor

rphmeier commented Jan 12, 2021

we need to have the session keys migration implemented, right?

I wrote a Session::update_keys function which makes this pretty easy now, if that helps. I am also open to restarting Rococo after we merge this.

To make these proofs smaller we could create another entry in MmrLeaf and extract a particular digest item from each ParaHead

The parachain headers don't have any particular format. They are just Vec<u8>, so I don't think we can do that, unfortunately.

Can you give some more info on the para-heads merkle tree and what it will be used for?

@tomusdrw
Copy link
Contributor Author

tomusdrw commented Jan 14, 2021

The parachain headers don't have any particular format. They are just Vec, so I don't think we can do that, unfortunately.

Ah, fair enough.

Can you give some more info on the para-heads merkle tree and what it will be used for?

A light client tracking latest MMR root hash, will be able to receive:

  1. A MMR proof of a particular leaf (i.e. one particular relay chain block)
  2. A Merkle Proof of Parachain Header
  3. A parachain-specific proof (for instance storage proof).

Hence a light client with only MMR root hash will be able to efficiently verify any parachain state data if needed.

@adoerr adoerr added the D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit. label Mar 24, 2021
@tomusdrw tomusdrw added D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit. and removed D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit. labels Mar 25, 2021
Copy link
Contributor

@andresilva andresilva left a comment

Choose a reason for hiding this comment

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

Initial review, mostly just nits about the search-replace on dependencies.

cli/Cargo.toml Outdated
@@ -33,7 +33,7 @@ browser-utils = { package = "substrate-browser-utils", git = "https://github.com

# this crate is used only to enable `trie-memory-tracker` feature
# see https://github.com/paritytech/substrate/pull/6745
sp-trie = { git = "https://github.com/paritytech/substrate", branch = "master", default-features = false }
sp-trie = { git = "https://github.com/paritytech/substrate", default-features = false , branch = "master" }
Copy link
Contributor

Choose a reason for hiding this comment

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

There are a bunch of changes like this which I assume came from some automated search and replace. Can we revert this?

Comment on lines 8 to 10
sp-core = { git = "https://github.com/paritytech/substrate", default-features = false , branch = "master" }
sp-std = { git = "https://github.com/paritytech/substrate", default-features = false , branch = "master" }
sp-runtime = { git = "https://github.com/paritytech/substrate", default-features = false , branch = "master" }
Copy link
Contributor

Choose a reason for hiding this comment

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

Revert?

Comment on lines 26 to 31
sc-client-api = { git = "https://github.com/paritytech/substrate", default-features = false , branch = "master" }
sc-keystore = { git = "https://github.com/paritytech/substrate", default-features = false , branch = "master" }
sp-consensus-slots = { git = "https://github.com/paritytech/substrate", default-features = false , branch = "master" }
sp-blockchain = { git = "https://github.com/paritytech/substrate", default-features = false , branch = "master" }
sp-application-crypto = { git = "https://github.com/paritytech/substrate", default-features = false, features = ["full_crypto"] , branch = "master" }
sp-runtime = { git = "https://github.com/paritytech/substrate", default-features = false , branch = "master" }
Copy link
Contributor

Choose a reason for hiding this comment

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

Revert?

@@ -21,7 +21,7 @@ polkadot-overseer = { path = "../../overseer" }
polkadot-primitives = { path = "../../../primitives" }
polkadot-node-primitives = { path = "../../primitives" }

sc-service = { git = "https://github.com/paritytech/substrate", branch = "master", default-features = false }
sc-service = { git = "https://github.com/paritytech/substrate", default-features = false , branch = "master" }
Copy link
Contributor

Choose a reason for hiding this comment

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

Revert?

@@ -15,7 +15,7 @@ futures = "0.3.8"
tracing = "0.1.25"

[dev-dependencies]
sp-core = { git = "https://github.com/paritytech/substrate", branch = "master", features = ["std"] }
sp-core = { git = "https://github.com/paritytech/substrate", features = ["std"] , branch = "master" }
Copy link
Contributor

Choose a reason for hiding this comment

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

Revert?

@@ -48,6 +49,7 @@ pallet-identity = { git = "https://github.com/paritytech/substrate", branch = "m
pallet-im-online = { git = "https://github.com/paritytech/substrate", branch = "master", default-features = false }
pallet-indices = { git = "https://github.com/paritytech/substrate", branch = "master", default-features = false }
pallet-membership = { git = "https://github.com/paritytech/substrate", branch = "master", default-features = false }
pallet-mmr-primitives = { git = "https://github.com/paritytech/substrate", default-features = false , branch = "master" }
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing from std features list below?

Copy link
Contributor

Choose a reason for hiding this comment

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

Done

Comment on lines 18 to 25
sp-api = { git = "https://github.com/paritytech/substrate", default-features = false , branch = "master" }
inherents = { package = "sp-inherents", git = "https://github.com/paritytech/substrate", default-features = false , branch = "master" }
sp-std = { package = "sp-std", git = "https://github.com/paritytech/substrate", default-features = false , branch = "master" }
sp-io = { git = "https://github.com/paritytech/substrate", default-features = false , branch = "master" }
sp-runtime = { git = "https://github.com/paritytech/substrate", default-features = false , branch = "master" }
sp-session = { git = "https://github.com/paritytech/substrate", default-features = false , branch = "master" }
sp-staking = { git = "https://github.com/paritytech/substrate", default-features = false , branch = "master" }
sp-core = { git = "https://github.com/paritytech/substrate", default-features = false , branch = "master" }
Copy link
Contributor

Choose a reason for hiding this comment

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

Revert?

@@ -35,11 +36,14 @@ pallet-offences = { git = "https://github.com/paritytech/substrate", branch = "m
pallet-transaction-payment = { git = "https://github.com/paritytech/substrate", branch = "master", default-features = false }
pallet-treasury = { git = "https://github.com/paritytech/substrate", branch = "master", default-features = false }

pallet-beefy = { git = "https://github.com/paritytech/grandpa-bridge-gadget", default-features = false , branch = "master" }
pallet-mmr = { git = "https://github.com/paritytech/substrate", default-features = false , branch = "master" }
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing from std feature list below?

sp-session = { git = "https://github.com/paritytech/substrate", branch = "master", default-features = false }
sp-staking = { git = "https://github.com/paritytech/substrate", branch = "master", default-features = false }
sp-core = { git = "https://github.com/paritytech/substrate", branch = "master", default-features = false }
beefy-primitives = { git = "https://github.com/paritytech/grandpa-bridge-gadget", default-features = false , branch = "master" }
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing from std feature list below?

@@ -955,6 +967,19 @@ pub fn new_full<RuntimeApi, Executor>(
task_manager.spawn_essential_handle().spawn_blocking("babe", babe);
}

// Start BEEFY
task_manager.spawn_essential_handle().spawn_blocking(
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we do this only on rococo/westend? Otherwise if BEEFY crashes for some reason the whole node goes down.

@adoerr adoerr requested a review from andresilva March 30, 2021 16:10
Comment on lines 696 to 699
/// Priviledged origin used by propose parachain.
pub struct PriviledgedOrigin;
// A wrapper around `babe::CurrentBlockRandomness` that does not return `Option<Random>`.
pub struct CurrentBlockRandomness;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
/// Priviledged origin used by propose parachain.
pub struct PriviledgedOrigin;
// A wrapper around `babe::CurrentBlockRandomness` that does not return `Option<Random>`.
pub struct CurrentBlockRandomness;
/// Priviledged origin used by propose parachain.
pub struct PriviledgedOrigin;
// A wrapper around `babe::CurrentBlockRandomness` that does not return `Option<Random>`.
pub struct CurrentBlockRandomness;

Seems unneeded.

Copy link
Contributor

@andresilva andresilva left a comment

Choose a reason for hiding this comment

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

I pushed some minor fixes. LGTM.

@tomusdrw tomusdrw merged commit e805045 into master Apr 1, 2021
@tomusdrw tomusdrw deleted the td-mmr branch April 1, 2021 10:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
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.

9 participants