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

babe: pass epoch data via intermediates#4807

Merged
andresilva merged 12 commits intomasterfrom
sp-babe-intermediate
Feb 6, 2020
Merged

babe: pass epoch data via intermediates#4807
andresilva merged 12 commits intomasterfrom
sp-babe-intermediate

Conversation

@sorpaas
Copy link
Copy Markdown
Contributor

@sorpaas sorpaas commented Feb 2, 2020 •
edited
Loading

Fixes #4652 (comment)

This passes the epoch data via block import intermediates, avoiding a second call to epoch_changes.epoch_for_child_of. It does require encoding/decoding of the data when passing, which, under the current design of BlockImportParams, I don't think is avoidable. (Fixed in #4809)

@sorpaas sorpaas added the A0-please_review Pull request needs code review. label Feb 2, 2020
@sorpaas sorpaas requested a review from andresilva February 2, 2020 15:25
@sorpaas sorpaas requested a review from Demi-Marie as a code owner February 2, 2020 15:25
@sorpaas sorpaas added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. labels Feb 3, 2020
@sorpaas sorpaas added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Feb 3, 2020
sorpaas
sorpaas commented Feb 3, 2020
View reviewed changes
Comment thread client/consensus/babe/src/lib.rs Outdated

let epoch = match intermediate.epoch {
Some(epoch) => epoch,
None => {
Copy link
Copy Markdown
Contributor Author

@sorpaas sorpaas Feb 3, 2020

Choose a reason for hiding this comment

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

This branch is needed because when proposing blocks, it won't invoke the verifier. In those cases the epoch must be re-fetched.

@gavofyork gavofyork added the B0-silent Changes should not be mentioned in any release notes label Feb 4, 2020
@sorpaas
Copy link
Copy Markdown
Contributor Author

sorpaas commented Feb 4, 2020

Hmm looks like there's an error in tests. Let me debug.

@sorpaas sorpaas added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. labels Feb 4, 2020
andresilva
andresilva reviewed Feb 5, 2020
View reviewed changes
Copy link
Copy Markdown
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.

When authoring blocks we still fetch epoch data from the slot worker in order to author the block. We could change block_import_params to also take EpochData, which we could then use to populate the intermediate. We could then change BabeIntermediate to not use Option. WDYT?

@bkchr
Copy link
Copy Markdown
Member

bkchr commented Feb 6, 2020

When authoring blocks we still fetch epoch data from the slot worker in order to author the block. We could change block_import_params to also take EpochData, which we could then use to populate the intermediate. We could then change BabeIntermediate to not use Option. WDYT?

Sounds like a good idea to me. 👍

@sorpaas
Copy link
Copy Markdown
Contributor Author

sorpaas commented Feb 6, 2020

Integration test fixed!

@sorpaas sorpaas added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. A0-please_review Pull request needs code review. labels Feb 6, 2020
@sorpaas sorpaas added the A3-in_progress Pull request is in progress. No review needed at this stage. label Feb 6, 2020
@sorpaas
Copy link
Copy Markdown
Contributor Author

sorpaas commented Feb 6, 2020

PR is ready, but I want to merge it after #4785 is merged. So marking it in-progress for now.

@sorpaas sorpaas added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Feb 6, 2020
andresilva
andresilva approved these changes Feb 6, 2020
View reviewed changes
Copy link
Copy Markdown
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!

@andresilva andresilva merged commit dec1bb7 into master Feb 6, 2020
@andresilva andresilva deleted the sp-babe-intermediate branch February 6, 2020 17:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@Demi-Marie Demi-Marie Awaiting requested review from Demi-Marie

2 more reviewers

@andresilva andresilva andresilva approved these changes

@rphmeier rphmeier rphmeier approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@sorpaas @bkchr @andresilva @rphmeier @gavofyork