Reduce provisioner work #12718
Reduce provisioner work #12718
Conversation
github-actions
bot
added
the
A3-in_progress
| @@ -22,6 +22,7 @@ log = "0.4.17" | |||
| thiserror = "1.0.30" | |||
| sc-client-api = { version = "4.0.0-dev", path = "../../api" } | |||
| sc-consensus = { version = "0.10.0-dev", path = "../../../client/consensus/common" } | |||
| sp-consensus-babe = { version = "0.10.0-dev", path = "../../../primitives/consensus/babe" } | |||
| fn get_slot(&mut self) -> Option<InherentType> { | ||
| let timestamp = sp_timestamp::InherentDataProvider::from_system_time(); | ||
| let slot_duration = SlotDuration::from_duration(self.slot_duration)?; | ||
| let slot = | ||
| sp_consensus_babe::inherents::InherentDataProvider::from_timestamp_and_slot_duration( | ||
| *timestamp, | ||
| slot_duration, | ||
| ) | ||
| .slot(); | ||
|
|
||
| Some(slot) | ||
| } |
There was a problem hiding this comment.
This is also wrong. There are reasons why it is generic.
| pub fn from_duration(duration: sp_std::time::Duration) -> Option<Self> { | ||
| let millis: u64 = duration.as_millis().try_into().ok()?; | ||
| Some(Self::from_millis(millis)) | ||
| } |
There was a problem hiding this comment.
Can be From<Duration>.
And as_millis() as u64 is fine here. Until this overflows, there still seems to be a lot of time :P
There was a problem hiding this comment.
Someone likes to live dangerously :P I agree though!
There was a problem hiding this comment.
Whoever reads this in the future with Substrate still running. Sorry :P
| /// The inherent data. | ||
| pub inherent_data: InherentData, |
There was a problem hiding this comment.
Should be replaced by Box<InherentdataProvider>
There was a problem hiding this comment.
Can you explain why? So far, it looks like it's not needed at all. In case I can fix the changes to make them generic without this field, of course.
There was a problem hiding this comment.
We should still create the inherent data providers inside here and just pass this around to create the inherent data. The function create_inherent_data should be made async. Then you can wait there for the provisioner result.
There was a problem hiding this comment.
But if we keep creating the inherent data providers here, does that not mean that we still do call the provisioner to do all the work for non block building validators? At least in polkadot as polkadot_node_core_parachains_inherent::ParachainsInherentDataProvider::createis called in create_inherent_data_providers https://github.com/paritytech/polkadot/blob/master/node/service/src/lib.rs#L1155-L1159. Which seems to be gathering the inherent data https://github.com/paritytech/polkadot/blob/master/node/core/parachains-inherent/src/lib.rs#L53.
I was thinking of factoring out the slotand timestampcalculation in to a different async in order to get these here and later on call create_inherent_data_providers.
There was a problem hiding this comment.
Yeah, you will then just move the call to the provisioner to create_inherent_data. I don't see the problem.
|
Closed in favor of #12749 |
ghost
closed this
ghost
deleted the
✄ -----------------------------------------------------------------------------
Thank you for your Pull Request! 🙏
Before you submit, please check that:
A*for PR status (one required)B*for changelog (one required)C*for release notes (exactly one required)D*for various implications/requirementsFixes #228orRelated #1337.Refer to the contributing guide for details.
After you've read this notice feel free to remove it.
Thank you!
✄ -----------------------------------------------------------------------------