diff --git a/substrate/bin/node-template/node/src/service.rs b/substrate/bin/node-template/node/src/service.rs index 9be91e7ef9..c5b9bf336e 100644 --- a/substrate/bin/node-template/node/src/service.rs +++ b/substrate/bin/node-template/node/src/service.rs @@ -145,44 +145,41 @@ pub fn new_full(config: Configuration) gossip_duration: Duration::from_millis(333), justification_period: 512, name: Some(name), - observer_enabled: true, + observer_enabled: false, keystore, is_authority, }; - match (is_authority, disable_grandpa) { - (false, false) => { - // start the lightweight GRANDPA observer - service.spawn_task("grandpa-observer", grandpa::run_grandpa_observer( - grandpa_config, - grandpa_link, - service.network(), - service.on_exit(), - )?); - }, - (true, false) => { - // start the full GRANDPA voter - let voter_config = grandpa::GrandpaParams { - config: grandpa_config, - link: grandpa_link, - network: service.network(), - inherent_data_providers: inherent_data_providers.clone(), - on_exit: service.on_exit(), - telemetry_on_connect: Some(service.telemetry_on_connect_stream()), - voting_rule: grandpa::VotingRulesBuilder::default().build(), - }; + let enable_grandpa = !disable_grandpa; + if enable_grandpa { + // start the full GRANDPA voter + // NOTE: non-authorities could run the GRANDPA observer protocol, but at + // this point the full voter should provide better guarantees of block + // and vote data availability than the observer. The observer has not + // been tested extensively yet and having most nodes in a network run it + // could lead to finality stalls. + let grandpa_config = grandpa::GrandpaParams { + config: grandpa_config, + link: grandpa_link, + network: service.network(), + inherent_data_providers: inherent_data_providers.clone(), + on_exit: service.on_exit(), + telemetry_on_connect: Some(service.telemetry_on_connect_stream()), + voting_rule: grandpa::VotingRulesBuilder::default().build(), + }; - // the GRANDPA voter task is considered infallible, i.e. - // if it fails we take down the service with it. - service.spawn_essential_task("grandpa", grandpa::run_grandpa_voter(voter_config)?); - }, - (_, true) => { - grandpa::setup_disabled_grandpa( - service.client(), - &inherent_data_providers, - service.network(), - )?; - }, + // the GRANDPA voter task is considered infallible, i.e. + // if it fails we take down the service with it. + service.spawn_essential_task( + "grandpa-voter", + grandpa::run_grandpa_voter(grandpa_config)? + ); + } else { + grandpa::setup_disabled_grandpa( + service.client(), + &inherent_data_providers, + service.network(), + )?; } Ok(service) diff --git a/substrate/bin/node/cli/src/service.rs b/substrate/bin/node/cli/src/service.rs index 79e1f97169..c60e150a4f 100644 --- a/substrate/bin/node/cli/src/service.rs +++ b/substrate/bin/node/cli/src/service.rs @@ -215,46 +215,41 @@ macro_rules! new_full { gossip_duration: std::time::Duration::from_millis(333), justification_period: 512, name: Some(name), - observer_enabled: true, + observer_enabled: false, keystore, is_authority, }; - match (is_authority, disable_grandpa) { - (false, false) => { - // start the lightweight GRANDPA observer - service.spawn_task("grandpa-observer", grandpa::run_grandpa_observer( - config, - grandpa_link, - service.network(), - service.on_exit(), - )?); - }, - (true, false) => { - // start the full GRANDPA voter - let grandpa_config = grandpa::GrandpaParams { - config: config, - link: grandpa_link, - network: service.network(), - inherent_data_providers: inherent_data_providers.clone(), - on_exit: service.on_exit(), - telemetry_on_connect: Some(service.telemetry_on_connect_stream()), - voting_rule: grandpa::VotingRulesBuilder::default().build(), - }; - // the GRANDPA voter task is considered infallible, i.e. - // if it fails we take down the service with it. - service.spawn_essential_task( - "grandpa-voter", - grandpa::run_grandpa_voter(grandpa_config)? - ); - }, - (_, true) => { - grandpa::setup_disabled_grandpa( - service.client(), - &inherent_data_providers, - service.network(), - )?; - }, + let enable_grandpa = !disable_grandpa; + if enable_grandpa { + // start the full GRANDPA voter + // NOTE: non-authorities could run the GRANDPA observer protocol, but at + // this point the full voter should provide better guarantees of block + // and vote data availability than the observer. The observer has not + // been tested extensively yet and having most nodes in a network run it + // could lead to finality stalls. + let grandpa_config = grandpa::GrandpaParams { + config, + link: grandpa_link, + network: service.network(), + inherent_data_providers: inherent_data_providers.clone(), + on_exit: service.on_exit(), + telemetry_on_connect: Some(service.telemetry_on_connect_stream()), + voting_rule: grandpa::VotingRulesBuilder::default().build(), + }; + + // the GRANDPA voter task is considered infallible, i.e. + // if it fails we take down the service with it. + service.spawn_essential_task( + "grandpa-voter", + grandpa::run_grandpa_voter(grandpa_config)? + ); + } else { + grandpa::setup_disabled_grandpa( + service.client(), + &inherent_data_providers, + service.network(), + )?; } Ok((service, inherent_data_providers)) diff --git a/substrate/client/finality-grandpa/src/lib.rs b/substrate/client/finality-grandpa/src/lib.rs index e931271df9..36b57024c9 100644 --- a/substrate/client/finality-grandpa/src/lib.rs +++ b/substrate/client/finality-grandpa/src/lib.rs @@ -96,7 +96,6 @@ mod voting_rule; pub use finality_proof::FinalityProofProvider; pub use justification::GrandpaJustification; pub use light_import::light_block_import; -pub use observer::run_grandpa_observer; pub use voting_rule::{ BeforeBestBlockBy, ThreeQuartersOfTheUnfinalizedChain, VotingRule, VotingRulesBuilder }; @@ -551,7 +550,7 @@ pub fn run_grandpa_voter( Client: AuxStore, { let GrandpaParams { - config, + mut config, link, network, inherent_data_providers, @@ -560,6 +559,12 @@ pub fn run_grandpa_voter( voting_rule, } = grandpa_params; + // NOTE: we have recently removed `run_grandpa_observer` from the public + // API, I felt it is easier to just ignore this field rather than removing + // it from the config temporarily. This should be removed after #5013 is + // fixed and we re-add the observer to the public API. + config.observer_enabled = false; + let LinkHalf { client, select_chain, diff --git a/substrate/client/finality-grandpa/src/observer.rs b/substrate/client/finality-grandpa/src/observer.rs index 77227909dc..8345a34209 100644 --- a/substrate/client/finality-grandpa/src/observer.rs +++ b/substrate/client/finality-grandpa/src/observer.rs @@ -150,6 +150,9 @@ fn grandpa_observer( /// listening for and validating GRANDPA commits instead of following the full /// protocol. Provide configuration and a link to a block import worker that has /// already been instantiated with `block_import`. +/// NOTE: this is currently not part of the crate's public API since we don't consider +/// it stable enough to use on a live network. +#[allow(unused)] pub fn run_grandpa_observer( config: Config, link: LinkHalf, diff --git a/substrate/client/finality-grandpa/src/tests.rs b/substrate/client/finality-grandpa/src/tests.rs index 24ab0dd41c..5d01b257b6 100644 --- a/substrate/client/finality-grandpa/src/tests.rs +++ b/substrate/client/finality-grandpa/src/tests.rs @@ -1376,7 +1376,7 @@ fn finalize_3_voters_1_light_observer() { run_to_completion_with(&mut runtime, 20, net.clone(), authorities, |executor| { executor.spawn( - run_grandpa_observer( + observer::run_grandpa_observer( Config { gossip_duration: TEST_GOSSIP_DURATION, justification_period: 32,