Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Make consensus SlotWorker don't assume a slot is time / duration #7441

Merged
merged 4 commits into from
Oct 27, 2020

Conversation

bkchr
Copy link
Member

@bkchr bkchr commented Oct 27, 2020

This removes the last bit of assumption that a slot is always time / duration. This will be required by parachains where a slot will be the
relay chain block number. Besides this there are also some other drive
by changes. One more notable is that on_slot now returns a
SlotResult that holds the block and a potential storage proof.

To simplify the implementation and usage of the SimpleSlotWorker the
SlotWorker trait is now implemented for each type that implements
SimpleSlotWorker.

This removes the last bit of assumption that a slot is always `time /
duration`. This will be required by parachains where a slot will be the
relay chain block number. Besides this there are also some other drive
by changes. One more notable is that `on_slot` now returns a
`SlotResult` that holds the block and a potential storage proof.

To simplify the implementation and usage of the `SimpleSlotWorker` the
`SlotWorker` trait is now implemented for each type that implements
`SimpleSlotWorker`.
@bkchr bkchr added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Oct 27, 2020
@bkchr bkchr requested a review from andresilva October 27, 2020 20:37
@bkchr
Copy link
Member Author

bkchr commented Oct 27, 2020

Contributes towards: paritytech/cumulus#216

Copy link
Contributor

@andresilva andresilva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. did you test block production locally? I don't think a burn-in is needed since local block production should test this well.

@@ -37,30 +37,6 @@ pub fn duration_now() -> Duration {
))
}


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this was added in order to be used with BABE's relative time protocol, but since that wasn't implemented we might as well remove this for now anyway.

client/consensus/slots/src/lib.rs Outdated Show resolved Hide resolved
client/consensus/slots/src/lib.rs Outdated Show resolved Hide resolved
@bkchr bkchr merged commit c55797b into master Oct 27, 2020
@bkchr bkchr deleted the bkchr-slots-no-time branch October 27, 2020 23:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants