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

Merkle Mountain Range pallet improvements #7891

Merged
16 commits merged into from
Jan 28, 2021
Merged

Merkle Mountain Range pallet improvements #7891

16 commits merged into from
Jan 28, 2021

Conversation

tomusdrw
Copy link
Contributor

Follow up on #7312

This PR adds runtime APIs to make it easy to generate and verify MMR proofs if pallet is included in the runtime.

Additionally it adds a verify_leaf_proof standalone function, which allows stateless verification of MMR proofs that are coming from differently configured pallets (i.e. other chains). This functionality is exposed in the runtime api as well for convenience.

Since the exact layout of the leaf type is most likely unknown, we use OpaqueLeaf structure, which maintains scale-compatibility with the type the proof originates from.

  • Next possible stop: expose MMR functions as custom RPCs to make it easier to use.

@tomusdrw tomusdrw added A0-please_review Pull request needs code review. B3-apinoteworthy C1-low PR touches the given topic and has a low impact on builders. labels Jan 13, 2021
@cheme
Copy link
Contributor

cheme commented Jan 15, 2021

Did you consider moving frame/merkle-mountain-range/mmr/lib.rs to primitive (+ Instance and Config traits)?

@tomusdrw
Copy link
Contributor Author

Did you consider moving frame/merkle-mountain-range/mmr/lib.rs to primitive (+ Instance and Config traits)?

I'd rather keep the primitives as minimal as possible, right now it's:

  1. RuntimeAPI (will for instance be needed by the custom RPC)
  2. traits - needed for downstream crates that would like to implement the leaf type.

I guess your idea would be to provide stateless proof verification in the primitives crate as well? It should be possible even without Config traits (or other frame-deps), but I'm not convinced yet it would be that useful. Probably something to re-visit when we start building an actual bridge using the MMR.

Comment on lines 338 to 350
pub fn from_leaf<T: FullLeaf>(leaf: T) -> Self {
let encoded_leaf = leaf.using_encoded(|d| d.to_vec(), true);
// OpaqueLeaf must be SCALE-compatible with `Vec<u8>`.
// Simply using raw encoded bytes don't work, cause we don't know the
// length of the expected data.
let encoded_vec = codec::Encode::encode(&encoded_leaf);
OpaqueLeaf(encoded_vec)
}
}

impl FullLeaf for OpaqueLeaf {
fn using_encoded<R, F: FnOnce(&[u8]) -> R>(&self, f: F, _compact: bool) -> R {
f(&self.0)
Copy link
Contributor

Choose a reason for hiding this comment

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

this type encoding/decoding is bit confusing to me.
The encoding is identity but the decoding is not.
e.g. encode(decode(encode(OpaqueLeaf(something))) != encode(OpaqueLeaf(something))

I think it would make more sense to have a manual implementation of Decode.
Or to change to something like this:

Suggested change
pub fn from_leaf<T: FullLeaf>(leaf: T) -> Self {
let encoded_leaf = leaf.using_encoded(|d| d.to_vec(), true);
// OpaqueLeaf must be SCALE-compatible with `Vec<u8>`.
// Simply using raw encoded bytes don't work, cause we don't know the
// length of the expected data.
let encoded_vec = codec::Encode::encode(&encoded_leaf);
OpaqueLeaf(encoded_vec)
}
}
impl FullLeaf for OpaqueLeaf {
fn using_encoded<R, F: FnOnce(&[u8]) -> R>(&self, f: F, _compact: bool) -> R {
f(&self.0)
pub fn from_leaf<T: FullLeaf>(leaf: T) -> Self {
let encoded_leaf = leaf.using_encoded(|d| d.to_vec(), true);
OpaqueLeaf(encoded_leaf)
}
}
impl FullLeaf for OpaqueLeaf {
fn using_encoded<R, F: FnOnce(&[u8]) -> R>(&self, f: F, _compact: bool) -> R {
f(&self.0.encode())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was writing an elaborate answer why this is correct, just to learn that it's all wrong :) Thanks for being persistent with this.

So my initialy motivation was to have OpaqueLeaf be Vec<u8> SCALE-compatible, but pretend to be the "unwraped" (no length prefix) encoding for MMR struct. I've realised that this is not really needed, and I can relax the FullLeaf requirement and have OpaqueLeaf be completely independent from SCALE.

Please see the updated version now. The runtime API just accepts a raw Vec<u8>, and we just wrap this into OpaqueLeaf to get custom FullLeaf implementation (the default one (blanket impl for codec::Encode) would re-encode the vec, which is what I wanted to avoid).

Copy link
Contributor

@HCastano HCastano 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!

frame/merkle-mountain-range/primitives/src/lib.rs Outdated Show resolved Hide resolved
frame/merkle-mountain-range/primitives/src/lib.rs Outdated Show resolved Hide resolved
frame/merkle-mountain-range/primitives/src/lib.rs Outdated Show resolved Hide resolved
frame/merkle-mountain-range/src/lib.rs Outdated Show resolved Hide resolved
frame/merkle-mountain-range/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@gui1117 gui1117 left a comment

Choose a reason for hiding this comment

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

looks good to me, does it need an audit ?


fn verify_proof_stateless(
root: mmr::Hash,
leaf: Vec<u8>,
Copy link
Contributor

Choose a reason for hiding this comment

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

by curiosity: am I correct saying that user could also give the hash instead of the encoded leaf here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the leaf has to be encoded in it's compact form, so depending on the types used in the pallet it either has to be the hash (if it's wrapped in Compact) or the full leaf. I don't think passing a hash here would work in case your MMR leaf is configured without Compact (i.e. the "compact" encoding and regular encoding is the same).

@tomusdrw
Copy link
Contributor Author

looks good to me, does it need an audit ?

The MMR code was not audited yet, it will be though as a part of a bigger bridge project when we stabilise the code a bit more.

@tomusdrw
Copy link
Contributor Author

bot merge

@ghost
Copy link

ghost commented Jan 28, 2021

Trying merge.

@ghost ghost merged commit 2d597fc into master Jan 28, 2021
@ghost ghost deleted the td-mmr2 branch January 28, 2021 10:58
This pull request was closed.
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants