-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Feat: Add API and mechanism to retrieve additional top-level and child proofs via the relay state proof #10678
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
bkchr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly looking good, just some small improvements.
| // Not implemented: requires relay chain RPC to expose child trie proof method. | ||
| tracing::warn!( | ||
| target: "relay-chain-rpc-interface", | ||
| "prove_child_read not implemented for RPC interface, returning empty proof" | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And it is not doing this?
state_getChildReadProof.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, missed that one, done.
cumulus/primitives/core/src/lib.rs
Outdated
| /// | ||
| /// This API allows parachains to request both top-level relay chain storage keys | ||
| /// and child trie storage keys to be included in the relay chain state proof. | ||
| pub trait KeyToIncludeInRelayProofApi { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| pub trait KeyToIncludeInRelayProofApi { | |
| pub trait KeyToIncludeInRelayProof { |
cumulus/primitives/core/src/lib.rs
Outdated
| /// The returned `RelayProofRequest` contains a list of storage keys where each key | ||
| /// can be either: | ||
| /// - `RelayStorageKey::Top`: Top-level relay chain storage key | ||
| /// - `RelayStorageKey::Child`: Child trie storage, containing the child trie identifier | ||
| /// and the key to prove from that child trie | ||
| /// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /// The returned `RelayProofRequest` contains a list of storage keys where each key | |
| /// can be either: | |
| /// - `RelayStorageKey::Top`: Top-level relay chain storage key | |
| /// - `RelayStorageKey::Child`: Child trie storage, containing the child trie identifier | |
| /// and the key to prove from that child trie | |
| /// |
cumulus/primitives/core/src/lib.rs
Outdated
| /// - `RelayStorageKey::Child`: Child trie storage, containing the child trie identifier | ||
| /// and the key to prove from that child trie | ||
| /// | ||
| /// The collator generates proofs for these and includes them in the relay chain state proof. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /// The collator generates proofs for these and includes them in the relay chain state proof. | |
| /// The collator will include them in the relay chain proof that is passed alongside the parachain inherent into the runtime. |
| async fn collect_additional_storage_proofs( | ||
| relay_chain_interface: &impl RelayChainInterface, | ||
| relay_parent: PHash, | ||
| relay_proof_request: RelayProofRequest, | ||
| ) -> Option<StorageProof> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't get why we have two functions doing almost the same.
IMO we should have one function that returns us the "static keys" (aka what we used so far and then we will join them with the additional keys to download all at once)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did it just for the sake of keeping logics more isolated but I have unified with the suggested approach and we reduced a prove_read call which is nice.
| /// Processor for relay chain proof keys. | ||
| /// | ||
| /// This allows parachains to process data from the relay chain state proof, | ||
| /// including both child trie keys and main trie keys that were requested | ||
| /// via `KeyToIncludeInRelayProofApi`. | ||
| type RelayProofKeysProcessor: relay_state_snapshot::ProcessRelayProofKeys; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just merge this with OnSystemEvent. Then we don't need a new type here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done :)
cumulus/test/runtime/src/lib.rs
Outdated
|
|
||
| impl cumulus_primitives_core::KeyToIncludeInRelayProofApi<Block> for Runtime { | ||
| fn keys_to_prove() -> RelayProofRequest { | ||
| Default::default() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you return here some key that isn't read up to now. And then add some logic to the test_pallet that asserts that the key is part of the proof.
As we use the test runtime in zombienet tests, it will be automatically tested there for us.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
ceed7ce to
d40f024
Compare
bkchr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some last nitpicks, otherwise looks good.
| .unwrap_or_else(|e| { | ||
| tracing::warn!( | ||
| target: crate::LOG_TARGET, | ||
| error = ?e, | ||
| "Failed to fetch relay proof requests from runtime, using empty request" | ||
| ); | ||
| Default::default() | ||
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| .unwrap_or_else(|e| { | |
| tracing::warn!( | |
| target: crate::LOG_TARGET, | |
| error = ?e, | |
| "Failed to fetch relay proof requests from runtime, using empty request" | |
| ); | |
| Default::default() | |
| }) | |
| .unwrap_or_else(|e| { | |
| tracing::debug!( | |
| target: crate::LOG_TARGET, | |
| error = ?e, | |
| "Failed to fetch relay proof requests from runtime, using empty request" | |
| ); | |
| Default::default() | |
| }) |
The API may not exists and then we clearly don't want to see these warnings :)
| total_weight.saturating_accrue( | ||
| <T::OnSystemEvent as OnSystemEvent>::on_relay_state_proof(&relay_state_proof), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FMT
cumulus/primitives/core/src/lib.rs
Outdated
| pub trait KeyToIncludeInRelayProof { | ||
| /// Returns relay chain storage proof requests. | ||
| /// | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
|
||
| // Expect the requested key to be part of the proof. | ||
| relay_state_proof | ||
| .read_optional_entry::<u64>(RELAY_EPOCH_INDEX_KEY) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My point was to use a key that isn't by default part of the proof, this is already part of the proof. Let's take the balance key for ALICE for example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I understand it, I picked EPOCH_INDEX because I didn't realize it was already included because it was not part of the static keys. Already switched to ALICE System key and had to add to sproof builder for it not to break any unit test. Also tested and working as expected :)
|
Review required! Latest push from author must always be reviewed |
Purpose
This pull request introduces a new runtime API and implements the full feature pipeline for requesting additional relay-chain storage proofs in lookahead collators. The API allows parachain runtimes to specify extra top-level storage keys or child-trie data that must be included in the relay-chain state proof. The collator collects these additional proofs and merges them into the relay-chain state proof provided to the runtime during block execution, enabling the runtime to later process custom relay-chain data.
Rationale
Immediate application in pubsub mechanism proposed in #9994
This is a narrow down of scope for easier review of PR #10679
Due to early exits when defaulted it adds no significant overhead to current flows.
What this PR adds
Runtime API
Introduces
KeyToIncludeInRelayProofApi. (Suggestions for better naming are very welcome.)Adds supporting types
RelayProofRequestandRelayStorageKey.Allows runtimes to declare which relay-chain storage entries must be included in the relay state proof.
Collator integration
The lookahead collator calls the runtime API before block production.
Requested relay-chain proofs are collected, batched, and merged in a single operation.
The additional proofs are merged into the existing relay-chain state proof and passed to the runtime via parachain inherent data.
Proof extraction
parachain-systemexposes an extraction method for processing this additional proofs.Uses a handler pattern:
parachain-systemmanages proof lifecycle and initial validation.Application pallets consume proofs (data extraction or additional validation) by implementing
ProcessRelayProofKeys.Keeps extra proofs processing logic out of parachain-system.
About RelayStorageKey
RelayStorageKeyis an enum with two variants:Top: aVec<u8>representing a top-level relay-chain storage key.Child, which contains:storage_key: an unprefixed identifier of the child trie root (the default :child_storage:default: prefix is applied automatically),key: the specific key within that child trie.On the client side, child trie access is performed via ChildInfo::new_default(&storage_key).
Why
storage_keyinstead ofChildInfo:ChildInfofromsp-storagedoes not implementTypeInfo, which runtime APIs require.Adding
TypeInfotosp-storage(or introducing a wrapper to avoid bloating a critical core component likesp-storage) would significantly expand the scope of this PR.As a result, the current design:
Uses raw
storage_keybytes.Is limited to child tries using the default prefix.
Future improvements
Full
ChildInfosupport ifTypeInfois added tosp-storage(directly or via a wrapper), enabling arbitrary child-trie prefixes.Possible unification with
additional_relay_state_keysfor top-level proofs, subject to careful analysis of semantics and backward compatibility.Integration with additional collator implementations beyond lookahead collators.