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

Switch node template to use AuRa #3790

Merged
merged 22 commits into from
Oct 18, 2019

Conversation

shawntabrizi
Copy link
Member

Fixes #3779

This PR updates the Substrate Node Template to use AuRa consensus rather than Grandpa/BABE.

Right now Substrate chains brick when a chain is offline for more than one epoch. This is not a very friendly developer experience, so we think it will be better to use AuRa for now.

Tested on --dev chain and --alice/--bob local network.

@shawntabrizi shawntabrizi changed the title Shawntabrizi switch aura Switch node template to use AuRa Oct 10, 2019
@shawntabrizi shawntabrizi added the A0-please_review Pull request needs code review. label Oct 10, 2019
Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

Small problem with crypto; maybe a look from someone who knows service code better; looks good otherwise 👍


EDIT:

Small problem with crypto; maybe a look from someone who knows service code better(seeing Pierre's commit); looks good otherwise 👍

shawntabrizi and others added 3 commits October 11, 2019 09:15
Co-Authored-By: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
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.

Changes LGTM. Why remove GRANDPA though? It's unrelated to the BABE issue and I expect some users will start asking why blocks aren't being finalized.

babe_link.clone(),
babe_block_import,
.with_import_queue_and_fprb(|_config, client, _backend, _fetcher, _select_chain, _tx_pool| {
let finality_proof_request_builder = Box::new(DummyFinalityProofRequestBuilder::default()) as Box<_>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the explicit cast necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

Removing as Box<_> gives me an error:

Shawns-MacBook-Pro:substrate shawntabrizi$ cargo build -p node-template
   Compiling node-template v2.0.0 (/Users/shawntabrizi/Documents/GitHub/substrate/node-template)
error[E0599]: no method named `build` found for type `substrate_service::builder::ServiceBuilder<sr_primitives::generic::block::Block<sr_primitives::generic::header::Header<u32, sr_primitives::traits::BlakeTwo256>, sr_primitives::OpaqueExtrinsic>, node_template_runtime::RuntimeApi, C, node_template_runtime::GenesisConfig, std::option::Option<()>, substrate_client::client::Client<substrate_client::light::backend::Backend<substrate_client_db::light::LightStorage<sr_primitives::generic::block::Block<sr_primitives::generic::header::Header<u32, sr_primitives::traits::BlakeTwo256>, sr_primitives::OpaqueExtrinsic>>, substrate_primitives::hasher::blake2::Blake2Hasher>, substrate_client::light::call_executor::GenesisCallExecutor<substrate_client::light::backend::Backend<substrate_client_db::light::LightStorage<sr_primitives::generic::block::Block<sr_primitives::generic::header::Header<u32, sr_primitives::traits::BlakeTwo256>, sr_primitives::OpaqueExtrinsic>>, substrate_primitives::hasher::blake2::Blake2Hasher>, substrate_client::call_executor::LocalCallExecutor<substrate_client::light::backend::Backend<substrate_client_db::light::LightStorage<sr_primitives::generic::block::Block<sr_primitives::generic::header::Header<u32, sr_primitives::traits::BlakeTwo256>, sr_primitives::OpaqueExtrinsic>>, substrate_primitives::hasher::blake2::Blake2Hasher>, substrate_executor::native_executor::NativeExecutor<service::Executor>>>, sr_primitives::generic::block::Block<sr_primitives::generic::header::Header<u32, sr_primitives::traits::BlakeTwo256>, sr_primitives::OpaqueExtrinsic>, node_template_runtime::RuntimeApi>, alloc::sync::Arc<substrate_network::on_demand_layer::OnDemand<sr_primitives::generic::block::Block<sr_primitives::generic::header::Header<u32, sr_primitives::traits::BlakeTwo256>, sr_primitives::OpaqueExtrinsic>>>, substrate_client::client::LongestChain<substrate_client::light::backend::Backend<substrate_client_db::light::LightStorage<sr_primitives::generic::block::Block<sr_primitives::generic::header::Header<u32, sr_primitives::traits::BlakeTwo256>, sr_primitives::OpaqueExtrinsic>>, substrate_primitives::hasher::blake2::Blake2Hasher>, sr_primitives::generic::block::Block<sr_primitives::generic::header::Header<u32, sr_primitives::traits::BlakeTwo256>, sr_primitives::OpaqueExtrinsic>>, substrate_consensus_common::import_queue::basic_queue::BasicQueue<sr_primitives::generic::block::Block<sr_primitives::generic::header::Header<u32, sr_primitives::traits::BlakeTwo256>, sr_primitives::OpaqueExtrinsic>>, std::boxed::Box<substrate_network::config::DummyFinalityProofRequestBuilder>, alloc::sync::Arc<dyn substrate_network::chain::FinalityProofProvider<sr_primitives::generic::block::Block<sr_primitives::generic::header::Header<u32, sr_primitives::traits::BlakeTwo256>, sr_primitives::OpaqueExtrinsic>>>, service::NodeProtocol, substrate_transaction_graph::pool::Pool<substrate_transaction_pool::api::FullChainApi<substrate_client::client::Client<substrate_client::light::backend::Backend<substrate_client_db::light::LightStorage<sr_primitives::generic::block::Block<sr_primitives::generic::header::Header<u32, sr_primitives::traits::BlakeTwo256>, sr_primitives::OpaqueExtrinsic>>, substrate_primitives::hasher::blake2::Blake2Hasher>, substrate_client::light::call_executor::GenesisCallExecutor<substrate_client::light::backend::Backend<substrate_client_db::light::LightStorage<sr_primitives::generic::block::Block<sr_primitives::generic::header::Header<u32, sr_primitives::traits::BlakeTwo256>, sr_primitives::OpaqueExtrinsic>>, substrate_primitives::hasher::blake2::Blake2Hasher>, substrate_client::call_executor::LocalCallExecutor<substrate_client::light::backend::Backend<substrate_client_db::light::LightStorage<sr_primitives::generic::block::Block<sr_primitives::generic::header::Header<u32, sr_primitives::traits::BlakeTwo256>, sr_primitives::OpaqueExtrinsic>>, substrate_primitives::hasher::blake2::Blake2Hasher>, substrate_executor::native_executor::NativeExecutor<service::Executor>>>, sr_primitives::generic::block::Block<sr_primitives::generic::header::Header<u32, sr_primitives::traits::BlakeTwo256>, sr_primitives::OpaqueExtrinsic>, node_template_runtime::RuntimeApi>, sr_primitives::generic::block::Block<sr_primitives::generic::header::Header<u32, sr_primitives::traits::BlakeTwo256>, sr_primitives::OpaqueExtrinsic>>>, (), substrate_service::builder::LightRpcBuilder<sr_primitives::generic::block::Block<sr_primitives::generic::header::Header<u32, sr_primitives::traits::BlakeTwo256>, sr_primitives::OpaqueExtrinsic>, node_template_runtime::RuntimeApi, service::Executor>, substrate_client::light::backend::Backend<substrate_client_db::light::LightStorage<sr_primitives::generic::block::Block<sr_primitives::generic::header::Header<u32, sr_primitives::traits::BlakeTwo256>, sr_primitives::OpaqueExtrinsic>>, substrate_primitives::hasher::blake2::Blake2Hasher>>` in the current scope
   --> node-template/src/service.rs:135:4
    |
135 |         .build()
    |          ^^^^^

error: aborting due to previous error

node-template/runtime/src/lib.rs Show resolved Hide resolved
node-template/src/service.rs Show resolved Hide resolved
node-template/src/service.rs Outdated Show resolved Hide resolved
node-template/src/service.rs Outdated Show resolved Hide resolved
node-template/src/service.rs Outdated Show resolved Hide resolved
shawntabrizi and others added 6 commits October 13, 2019 12:43
Co-Authored-By: André Silva <andre.beat@gmail.com>
Co-Authored-By: André Silva <andre.beat@gmail.com>
Co-Authored-By: André Silva <andre.beat@gmail.com>
Co-Authored-By: André Silva <andre.beat@gmail.com>
Co-Authored-By: André Silva <andre.beat@gmail.com>
@shawntabrizi
Copy link
Member Author

@andresilva TBH I didn't think that hard about including or not including Grandpa. I just thought it too wasn't that stable ATM, so better not to add it...

If you think it should be here, then we can add it back, but I will probably need help writing the service file.

@kianenigma
Copy link
Contributor

If you think it should be here, then we can add it back, but I will probably need help writing the service file.

Maybe looking at previous revisions would help?

I agree that the we want some finalization in node-template, otherwise people will keep complaining about it.

@kianenigma kianenigma added A5-grumble and removed A0-please_review Pull request needs code review. labels Oct 14, 2019
@shawntabrizi shawntabrizi added A0-please_review Pull request needs code review. and removed A5-grumble labels Oct 16, 2019
@shawntabrizi
Copy link
Member Author

@andresilva @kianenigma this is ready for a final review.

Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

LGTM 👍

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.

BlockImport is how we allow consensus engines to hook into the block import pipeline. The client also implements BlockImport and it's where actual block import happens (execution, updating db, etc.). Both BABE and GRANDPA need to hook into the import pipeline in order to check for authority change digests, do some bookkeeping on pending changes and properly validate them. The way this works is that we wrap BlockImports passing a final composite one to the import queue. When we have both BABE and GRANDPA the import queue sees a BabeBlockImport, which wraps a GrandpaBlockImport, which wraps a client::BlockImport. Aura doesn't need to hook into the import pipeline, so we just need to pass a GrandpaBlockImport to the import queue and to the aura proposer task.

node-template/src/chain_spec.rs Outdated Show resolved Hide resolved
node-template/src/chain_spec.rs Outdated Show resolved Hide resolved
node-template/src/chain_spec.rs Outdated Show resolved Hide resolved
node-template/src/cli.rs Show resolved Hide resolved
node-template/src/service.rs Outdated Show resolved Hide resolved
node-template/src/service.rs Outdated Show resolved Hide resolved
node-template/src/service.rs Outdated Show resolved Hide resolved
shawntabrizi and others added 7 commits October 16, 2019 20:31
Co-Authored-By: André Silva <andre.beat@gmail.com>
Co-Authored-By: André Silva <andre.beat@gmail.com>
Co-Authored-By: André Silva <andre.beat@gmail.com>
Co-Authored-By: André Silva <andre.beat@gmail.com>
Co-Authored-By: André Silva <andre.beat@gmail.com>
@@ -105,9 +105,9 @@ pub fn new_full<C: Send + Default + 'static>(config: Configuration<C, GenesisCon

let aura = aura::start_aura::<_, _, _, _, _, AuraPair, _, _, _>(
aura::SlotDuration::get_or_compute(&*client)?,
client.clone(),
select_chain,
client,
Copy link
Contributor

Choose a reason for hiding this comment

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

the fact that it's so easy to get this wrong means that we should probably not implement BlockImport on Client.

@andresilva andresilva merged commit 5e406b1 into paritytech:master Oct 18, 2019
@shawntabrizi shawntabrizi deleted the shawntabrizi-switch-aura branch October 18, 2019 13:27
geigerzaehler pushed a commit to radicle-dev/radicle-registry that referenced this pull request Oct 22, 2019
Aura is better suited for development. See paritytech/substrate#3790
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Switch Substrate Node Template to use AuRa
5 participants