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

Refactor: extract most aura logic out to standalone module, make use of these #13764

Merged
merged 5 commits into from
Mar 31, 2023
Merged

Refactor: extract most aura logic out to standalone module, make use of these #13764

merged 5 commits into from
Mar 31, 2023

Conversation

rphmeier
Copy link
Contributor

@rphmeier rphmeier commented Mar 29, 2023 •
edited
Loading

Related to paritytech/cumulus#2382

The changeset here is simply to create a standalone module in sc_consensus_aura and extract out useful behaviors such as slot claiming, seal generation, header verification to it. Then the parts of sc_consensus_aura that use these behaviors have been changed to invoke these functions.

This is also an experiment in refactoring Substrate consensus code - as @bkchr and I have discussed in the past, we've introduced abstractions at many of the wrong layers. By breaking up the consensus code into batches of standalone functions (@tomaka may cry tears of joy at this), it makes integration, modification, and customization easier.

The motivation here is to do the same in Cumulus. I am currently breaking out many primitives in Cumulus to make consensus drive the collator, and as per paritytech/cumulus#2301 we may need modifications to the meaning of Aura slots to prepare for asynchronous backing.

This is intended to be fully backwards-compatible in both API and behavior.

@rphmeier rphmeier added A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. T0-node This PR/Issue is related to the topic “node”. labels Mar 29, 2023
@rphmeier rphmeier added the B0-silent Changes should not be mentioned in any release notes label Mar 29, 2023
@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2614187

@rphmeier rphmeier force-pushed the rh-aura-standalone-functions branch from 288f0e8 to 2686d95 Compare March 30, 2023 04:15
bkchr
bkchr approved these changes Mar 30, 2023
View reviewed changes
client/consensus/aura/src/standalone.rs Outdated Show resolved Hide resolved
client/consensus/aura/src/import_queue.rs
Comment on lines +91 to +97
Err(SealVerificationError::Deferred(header, slot)) =>
Ok(CheckedHeader::Deferred(header, slot)),
Err(SealVerificationError::Unsealed) => Err(Error::HeaderUnsealed(hash)),
Err(SealVerificationError::BadSeal) => Err(Error::HeaderBadSeal(hash)),
Err(SealVerificationError::BadSignature) => Err(Error::BadSignature(hash)),
Err(SealVerificationError::SlotAuthorNotFound) => Err(Error::SlotAuthorNotFound),
Err(SealVerificationError::InvalidPreDigest(e)) => Err(Error::from(e)),
Copy link
Member

Choose a reason for hiding this comment

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

Please add SealVerificationError as variant to Error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would break the API, so I don't think it's a good idea.

SealVerificationError is basically just a copy of the Error variants that could be produced within seal verification.

client/consensus/aura/src/standalone.rs Outdated
{
client
.runtime_api()
.slot_duration(client.usage_info().chain.best_hash)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should pass the bloc hash to the slot_duration function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, but unfortunately, there was already a pub fn slot_duration with this exact interface exported from the lib.rs. To maintain backwards compatibility the interface needs to stay the same.

I will add another function which takes a block hash explicitly.

davxy
davxy approved these changes Mar 30, 2023
View reviewed changes
client/consensus/aura/src/import_queue.rs Show resolved Hide resolved
client/consensus/aura/src/lib.rs Show resolved Hide resolved
rphmeier and others added 2 commits March 30, 2023 08:53
@rphmeier @bkchr
Update client/consensus/aura/src/standalone.rs
af4182b 
improve docs

Co-authored-by: Bastian Köcher <git@kchr.de>
@rphmeier
add slot_duration_at
e1a46dc
@rphmeier
Copy link
Contributor Author

bot merge

@paritytech-processbot
Copy link

This PR cannot be merged at the moment due to: 11 of 14 required status checks are expected.

processbot expects that the problem will be solved automatically later and so the auto-merge process will be started. You can simply wait for now.

@rphmeier
Copy link
Contributor Author

Ci looks stuck, either that or it somehow hasn't run in several hours. mildly blocking

@bkchr
Copy link
Member

bkchr commented Mar 31, 2023

bot rebase

@paritytech-processbot
Copy link

Rebased

@bkchr
Copy link
Member

bkchr commented Mar 31, 2023

bot merge

@paritytech-processbot
Copy link

Waiting for commit status.

@paritytech-processbot
Copy link

Merge cancelled due to error. Error: Statuses failed for a18f21e

@bkchr
Copy link
Member

bkchr commented Mar 31, 2023

bot fmt

@command-bot
Copy link

command-bot bot commented Mar 31, 2023 •
edited
Loading

@bkchr https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2625236 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/substrate/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 30-8c7a1214-8ed4-43c3-aed9-cc85cd5eabbd to cancel this command or bot cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Mar 31, 2023

@bkchr Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2625236 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2625236/artifacts/download.

@bkchr
Copy link
Member

bkchr commented Mar 31, 2023

bot merge

@paritytech-processbot
Copy link

Waiting for commit status.

@paritytech-processbot
Copy link

Merge cancelled due to error. Error: Statuses failed for b5e4271

@bkchr bkchr merged commit a7bc9c2 into master Mar 31, 2023
@bkchr bkchr deleted the rh-aura-standalone-functions branch March 31, 2023 12:00
pgherveou pushed a commit that referenced this pull request Apr 4, 2023
@rphmeier @bkchr @pgherveou
Refactor: extract most aura logic out to standalone module, make use …
0e99aba 
…of these (#13764)

* Extract most aura logic out to standalone module, make use of these

* Update client/consensus/aura/src/standalone.rs

improve docs

Co-authored-by: Bastian Köcher <git@kchr.de>

* add slot_duration_at

* ".git/.scripts/commands/fmt/fmt.sh"

---------

Co-authored-by: Bastian Köcher <git@kchr.de>
Co-authored-by: parity-processbot <>
@rphmeier rphmeier mentioned this pull request Apr 19, 2023
Merged
7 tasks
gpestana pushed a commit that referenced this pull request Apr 23, 2023
@rphmeier @bkchr @gpestana
Refactor: extract most aura logic out to standalone module, make use …
668d61b 
…of these (#13764)

* Extract most aura logic out to standalone module, make use of these

* Update client/consensus/aura/src/standalone.rs

improve docs

Co-authored-by: Bastian Köcher <git@kchr.de>

* add slot_duration_at

* ".git/.scripts/commands/fmt/fmt.sh"

---------

Co-authored-by: Bastian Köcher <git@kchr.de>
Co-authored-by: parity-processbot <>
@github-actions github-actions bot mentioned this pull request Jun 15, 2023
Closed
@kacperzuk-neti kacperzuk-neti mentioned this pull request Jun 23, 2023
Merged
15 tasks
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
@rphmeier @bkchr @nathanwhit
Refactor: extract most aura logic out to standalone module, make use …
8dc4542 
…of these (paritytech#13764)

* Extract most aura logic out to standalone module, make use of these

* Update client/consensus/aura/src/standalone.rs

improve docs

Co-authored-by: Bastian Köcher <git@kchr.de>

* add slot_duration_at

* ".git/.scripts/commands/fmt/fmt.sh"

---------

Co-authored-by: Bastian Köcher <git@kchr.de>
Co-authored-by: parity-processbot <>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@davxy davxy davxy approved these changes

@mrcnski mrcnski mrcnski approved these changes

@bkchr bkchr bkchr approved these changes

@andresilva andresilva Awaiting requested review from andresilva

Assignees
No one assigned
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. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. T0-node This PR/Issue is related to the topic “node”.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@rphmeier @paritytech-cicd-pr @bkchr @mrcnski @davxy