Custom RPC for Merkle Mountain Range pallet #8137
Conversation
tomusdrw
added
A0-please_review
|
@bkchr could you take a look at the externalities extension? Is there any other way how you'd see this being done? |
| let leaf: mmr::Leaf = leaf | ||
| .into_opaque_leaf() | ||
| .try_decode() | ||
| .ok_or(mmr::Error::Verify)?; | ||
| Mmr::verify_leaf(leaf, proof) | ||
| } | ||
|
|
||
| fn verify_proof_stateless( |
There was a problem hiding this comment.
Perhaps verify_ephemeral_proof in order to emphasize, that it is not based on persistent data?
There was a problem hiding this comment.
I thought that stateless is enough emphasis :) "Ephemeral proof" sounds like a special kind of a proof to me, while here we just don't query the state, hence stateless.
|
|
||
| impl EncodableOpaqueLeaf { | ||
| /// Convert a concrete leaf into encodable opaque version. | ||
| pub fn from_leaf<T: FullLeaf>(leaf: &T) -> Self { |
There was a problem hiding this comment.
Shouldn't this throw some kind of error, if T doesn't meet the encoding requirements?
There was a problem hiding this comment.
Hmm, but there isn't really any requirements other than FullLeaf implementation. And the encoding is infallible.
So EncodableOpqaueLeaf and OpaqueLeaf serve a bit different purposes.
Both are meant for leaves that we don't necessarily know the concrete type of (i.e. we know it's a leaf, but we don't have the struct/type to encode it anywhere in the code).
The first one is a wrapper type to send such "unknown leaves" to the RuntimeApi or over RPC.
The second one is for MMR implementation - i.e. it implements FullLeaf, but can't implement Encode/Decode because of conflicting trait implementations.
from_leaf is just a convenience method to convert a known leaf type into its encodable and opaque form - so there is no extra requirements here.
| /// Generate MMR proof for a leaf under given index. | ||
| fn generate_proof(leaf_index: u64) -> Result<(Leaf, Proof<Hash>), Error>; | ||
| #[skip_initialize_block] |
There was a problem hiding this comment.
Just curious, why's this okay?
There was a problem hiding this comment.
It's not only okay, it's actually required :) By default the RuntimeApi will run block initialisation, which triggers on_initialize hooks of every modules.
MMR module adds new leaf in on_initialize, so the proofs are generated for "the next block" instead of for the current one.
|
Sorry for the delay, I will review this pr tomorrow. |
| @@ -84,6 +96,7 @@ impl ExtensionsFactory for () { | |||
| pub struct ExecutionExtensions<Block: traits::Block> { | |||
| strategies: ExecutionStrategies, | |||
| keystore: Option<SyncCryptoStorePtr>, | |||
| offchain_db: Option<Box<dyn DbExternalitiesFactory>>, | |||
There was a problem hiding this comment.
Why isn't this just a generic ExtensionFactory?
There was a problem hiding this comment.
Generic parameter would propagate up to the Client type, and the actual implementation of the DbExternalitiesFactory is in sc_offchain, so I think it would be quite inconvenient to use.
| @@ -119,19 +108,73 @@ impl<Storage: OffchainStorage> OffchainExt for Api<Storage> { | |||
| old_value: Option<&[u8]>, | |||
| new_value: &[u8], | |||
| ) -> bool { | |||
| log::debug!( | |||
There was a problem hiding this comment.
For these kind of logging, I can really recommend tracing::debug!/* in the future. They provide some nice "structured logging"
General comment, a target would be nice.
|
bot merge |
|
Checks failed; merge aborted. |
|
bot merge force |
|
Trying merge. |
ghost
merged commit 
Follow up on #7891.
Initially I thought that it would be possible to use
state_callRPC to use MMR RuntimeApi and be able to generate proofs.However, RPC-originated calls DO NOT have access to Offchain DB, hence proof generation is not really possible. I think it's unreasonable to blanket-enable Offchain DB access for all runtime calls over RPC, hence this PR introduces a custom RPC for MMR pallet, which performs RuntimeApi call but with OffchainDB access enabled (
OffchainCallwith default execution extensions enabled).Unfortunately I had to extract OffchainDB access from
OffchainWorkercontext and add yet another execution extension for this to work (I'm sorry @bkchr :))Companion PR: paritytech/polkadot#2463