-
Notifications
You must be signed in to change notification settings - Fork 246
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
Create invalid extrinsics fraud proof storage, and move block randomness into it #3314
Conversation
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.
Rest LGTM
231e06c
to
ae33c81
Compare
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.
Rest LGTM
// Used by caller | ||
block_randomness: _, |
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.
This field was introduced recently and is not exist in the consensus runtime in mainnet, so we should also remove it
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.
Specifically in #3312, where it wasn’t caught in review. Is there an automated test we could add to catch accidental changes like this?
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.
It is hard to test compatibility with an existing network in unit/integration test, usually we will test runtime upgrade before release (by simulating the existing network in a local network).
#[derive(Clone, Debug, Decode, Encode, Eq, PartialEq, TypeInfo)] | ||
pub enum FraudProofStorageKeyRequest<Number> { | ||
BlockRandomness, | ||
InvalidInherentExtrinsicData, |
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.
Hang on, is this a breaking runtime API change?
This enum is passed to a runtime API here:
subspace/crates/sp-domains-fraud-proof/src/lib.rs
Lines 157 to 166 in 5675113
sp_api::decl_runtime_apis! { | |
/// API necessary for fraud proof. | |
pub trait FraudProofApi<DomainHeader: HeaderT> { | |
/// Submit the fraud proof via an unsigned extrinsic. | |
fn submit_fraud_proof_unsigned(fraud_proof: FraudProof<NumberFor<Block>, Block::Hash, DomainHeader, H256>); | |
/// Reture the storage key used in fraud proof | |
fn fraud_proof_storage_key(req: FraudProofStorageKeyRequest<NumberFor<Block>>) -> Vec<u8>; | |
} | |
} |
If it is, I think we'll need to bump the runtime version in this PR as well.
Edit: Or since it already got bumped in #3303, maybe that's ok.
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.
This should be fine as runtime API only fail when we actually call it and it has never been called yet as there is no domain initialized (while the host function change will crash the node upon runtime/client upgrade).
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.
Yep, in this case it would produce wrong results (because enum variants have changed indexes) or fail (because the expected data does not accompany the variant at that index). So that’s fine as long as most nodes are upgraded before then.
We might want to bump the runtime version for Taurus though.
#[derive(Clone, Debug, Decode, Encode, Eq, PartialEq, TypeInfo)] | ||
pub enum FraudProofStorageKeyRequest<Number> { | ||
BlockRandomness, | ||
InvalidInherentExtrinsicData, |
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.
This should be fine as runtime API only fail when we actually call it and it has never been called yet as there is no domain initialized (while the host function change will crash the node upon runtime/client upgrade).
// Used by caller | ||
block_randomness: _, |
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.
It is hard to test compatibility with an existing network in unit/integration test, usually we will test runtime upgrade before release (by simulating the existing network in a local network).
This PR:
InvalidInherentExtrinsicData
fraud proof storage, andextrinsics_shuffling_seed
derived fromblock_randomness
into itIt also fixes some typos, and simplifies some of the code.
Part of #3281.
Eventually, all of
InvalidInherentExtrinsicProof
and most ofDomainInherentExtrinsicData
(except the sudo call) will be moved intoInvalidInherentExtrinsicData
.Code contributor checklist: