Prepare Polkadot to be used by Cumulus #1697
Conversation
This begins to make Polkadot usable from Cumulus.
bkchr
added
A0-please_review
parachain/src/primitives.rs
Outdated
| /// collate candidates that satisfy the criteria implied these transient validation data. | ||
| #[derive(PartialEq, Eq, Clone, Encode, Decode)] | ||
| #[cfg_attr(feature = "std", derive(Debug, Default))] | ||
| pub struct ValidationData<N = RelayChainBlockNumber> { |
There was a problem hiding this comment.
@rphmeier Essentially I will need all this data when validating the parachain PovBlock, so I copied that here. You think that this is okay? Or should I create some new struct for this? (Which would essentially contain all this data anyway).
There was a problem hiding this comment.
You should not need the TransientValidationData within the parachain runtime, at least not directly via ground truth.
node/subsystem/src/messages.rs
Outdated
| ValidateFromExhaustive( | ||
| PersistedValidationData, | ||
| Option<TransientValidationData>, | ||
| TransientValidationData, |
There was a problem hiding this comment.
The transient validation data is by definition transient. It literally won't be guaranteed available when this is called. That is why it is an Option.
Why does Cumulus need this? The role of this function should never need to cross the polkadot/cumulus boundary.
node/subsystem/src/messages.rs
Outdated
| /// validation without needing to access the state of the relay-chain. Optionally provide the | ||
| /// `TransientValidationData` for further checks on the outputs. | ||
| /// Explicitly provide the `PersistedValidationData`, `TransientValidationData` and `ValidationCode` | ||
| /// so this can do full validation without needing to access the state of the relay-chain. |
There was a problem hiding this comment.
This docs change doesn't make sense, because the only way to access TransientValidationData is to access the state of the relay-chain.
parachain/src/primitives.rs
Outdated
| pub hrmp_mqc_heads: Vec<(Id, Hash)>, | ||
| /// The validation data that was also available to the collator when | ||
| /// building the PoVBlock. | ||
| pub validation_data: ValidationData, |
There was a problem hiding this comment.
I don't believe all of this info should go into ground truth.
rphmeier
left a comment
rphmeier
left a comment
There was a problem hiding this comment.
This completely erodes the distinction between Persisted and Transient validation data and alters the abstract parachain execution model without a clear reason.
| Arc<FullClient<RuntimeApi, Executor>>, | ||
| Arc<sc_network::NetworkService<Block, <Block as BlockT>::Hash>>, | ||
| RpcHandlers, | ||
| OverseerHandler, |
runtime/kusama/src/lib.rs
Outdated
| .collect() | ||
| impl primitives::v1::ParachainHost<Block, Hash, BlockNumber> for Runtime { | ||
| fn validators() -> Vec<ValidatorId> { | ||
| unimplemented!() |
There was a problem hiding this comment.
I'm kind of worried what will happen if this is pushed to the chain and we have subsystems that query this runtime API. I think those subsystems may just fail and the node will go down. At the very least logs will get spammed a lot.
Yet we do need this implementation to actually get service to compile.
A different implementation where it just returns Vec::new() and None wherever possible would be better I think.
rphmeier
left a comment
rphmeier
left a comment
There was a problem hiding this comment.
Looks great except I'm worried about the unimplemented!() in the runtime.
I have done that, but now I'm more worried than you :P There will be 100% a timeframe where the new service will need to work with the old runtimes and if that is just for syncing. The services should not stop or print tons of data. We already had this with the availability system which printed a lot of data because it could not find required data and that is really bad for node operators, because they get spammed with useless warnings etc. |
This begins to make Polkadot usable from Cumulus.