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

Conversation

@bkchr
Copy link
Member

@bkchr bkchr commented Mar 17, 2021

This pr introduces Aura for parachains. It is working, but at least one thing is missing to make it secure. We don't verify yet that the slot is "valid", based on the time of the relay block. This will come in some later pr, when we are able to read the relay chain storage more easily.

The pr is also not yet mergable:

  • It still depends on some non-merged Substrate branch that I need to clean up.
  • Tests are missing and others are maybe not compiling/failing.
  • Some docs are missing.

Fixes: #115

pepyakin and others added 30 commits February 2, 2021 18:46
Co-authored-by: Bastian Köcher <[email protected]>
force_authoring,
slot_duration,
// We got around 500ms for proposing
block_proposal_slot_portion: SlotProportion::new(1f32 / 24f32),
Copy link
Member

Choose a reason for hiding this comment

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

1 / 24 * 12 sec = 500ms?

Somehow we should pull in the 12 second block times here? else some change in the config will have unintended results.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a proportion, not a duration in milliseconds.

It expresses that we can use 1/24 of the slot duration for proposing. Which in our case is 500ms

/// but we require the old authorities to verify the seal when validating a PoV. This will always
/// be updated to the latest AuRa authorities in `on_finalize`.
#[pallet::storage]
pub(crate) type Authorities<T: Config> = StorageValue<_, Vec<T::AuthorityId>, ValueQuery>;
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to introduce bounded vec here so we dont need to change it later?

Copy link
Member Author

Choose a reason for hiding this comment

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

Aura is not yet using BoundedVec. It will start to fail with a compilation error when this change this done. After that we can change it here.

I don't want to introduce a new upper bound here, when we can just fetch it from AURA later.

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.

Only minor nits the overall approach seems correct to me.

Comment on lines 11 to 16
sc-client-api = { git = "https://github.com/paritytech/substrate", optional = true , branch = "master" }
sp-runtime = { git = "https://github.com/paritytech/substrate", optional = true , branch = "master" }
sp-std = { git = "https://github.com/paritytech/substrate", default-features = false, branch = "master" }
sp-state-machine = { git = "https://github.com/paritytech/substrate", branch = "master", optional = true }
sp-state-machine = { git = "https://github.com/paritytech/substrate", optional = true , branch = "master" }
sp-trie = { git = "https://github.com/paritytech/substrate", default-features = false, branch = "master" }
sp-api = { git = "https://github.com/paritytech/substrate", branch = "master", optional = true }
sp-api = { git = "https://github.com/paritytech/substrate", optional = true , branch = "master" }
Copy link
Contributor

Choose a reason for hiding this comment

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

Revert?

IDP::InherentDataProviders: InherentDataProviderExt + Send + Sync,
{
sc_consensus_aura::import_queue::<P, _, _, _, _, _, _>(sc_consensus_aura::ImportQueueParams {
block_import: crate::ParachainBlockImport(block_import),
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the only difference from regular aura import queue right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah

@bkchr
Copy link
Member Author

bkchr commented May 10, 2021

bot merge

@ghost
Copy link

ghost commented May 10, 2021

Trying merge.

@bkchr bkchr merged commit d01bc24 into master May 10, 2021
@bkchr bkchr deleted the bkchr-aura-the-long-way branch May 10, 2021 12:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support Aura

5 participants