-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Bulletin as parachain missing features #10662
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
Conversation
… to polkadot execute/prepare workers
…time_dev node_dev --bump patch'
…bko-bulletin-para-support
Signed-off-by: Iulian Barbu <[email protected]>
Signed-off-by: Iulian Barbu <[email protected]>
Signed-off-by: Iulian Barbu <[email protected]>
Signed-off-by: Iulian Barbu <[email protected]>
Signed-off-by: Iulian Barbu <[email protected]>
Co-authored-by: Branislav Kontur <[email protected]>
Signed-off-by: Iulian Barbu <[email protected]>
…to bko-bulletin-support
…to bko-bulletin-support
| } | ||
|
|
||
| #[async_trait::async_trait] | ||
| impl<T: InherentDataProvider> InherentDataProvider for Vec<T> { |
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.
Iulian is currently OOO until next year.
| /// data corresponding to block `N - RetentionPeriod` when producing block `N`. | ||
| #[pallet::storage] | ||
| pub type StoragePeriod<T: Config> = StorageValue<_, BlockNumberFor<T>, ValueQuery>; | ||
| pub type RetentionPeriod<T: Config> = StorageValue<_, BlockNumberFor<T>, ValueQuery>; |
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.
Hmm this will change the key and break solo-bulletin on upgrade without migration right?
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.
Hmm this will change the key and break solo-bulletin on upgrade without migration right?
Good point, but no — we’re safe. The Bulletin repo (historically) has its own customized copy of pallet-transaction-storage, where StoragePeriod is just a Config const type and not a storage item:
https://github.com/paritytech/polkadot-bulletin-chain/blob/0f5ff2cb1f6b079a990e623ff67156f169a42a2a/pallets/transaction-storage/src/lib.rs#L201. So no migration is needed for the solo chain.
Also, we don’t want to maintain both pallets long term, so in parallel we’re working on merging them into a single pallet:
paritytech/polkadot-bulletin-chain#86.
skunert
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.
LGTM
I am a bit paranoid about these NOOP HFs in validate_block, but also double checked and saw that its just writing some DB columns which are not going to the state, so should be fine.
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.
One last nitpick.
|
All GitHub workflows were cancelled due to failure one of the required jobs. |
|
/cmd fmt |
Relates to: paritytech/polkadot-bulletin-chain#74
This PR adds the required support and features for running Bulletin as a parachain. It is a top-level PR that merges three partial features/PRs, which can also be reviewed/merged separately:
transaction_index::HostFunctionswith NO-OP impl to the cumulus ParachainSystemvalidate_blockfor polkadot-prepare/execute-worker - Addtransaction_index::HostFunctionswith NO-OP impl to the polkadot-prepare/execute-worker #10370