Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Create invalid extrinsics fraud proof storage, and move block randomness into it #3314
Changes from 3 commits
0694218
2d31849
ae33c81
5675113
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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).
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
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.