From 4f21cc7e5cf06856a93884493c928bd80e5842d4 Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Mon, 15 Feb 2021 22:37:13 -0600 Subject: [PATCH] Integrate Approval Voting into Overseer / Service / GRANDPA (#2412) * integrate approval voting into overseer * expose public API and make keystore arc * integrate overseer in service * guide: `ApprovedAncestor` returns block number * return block number along with hash from ApprovedAncestor * introduce a voting rule for reporting on approval checking * integrate the delay voting rule * Rococo configuration * fix compilation and add slack * fix web-wasm build * tweak parameterization * migrate voting rules to asycn * remove hack comment --- polkadot/Cargo.lock | 2 + polkadot/node/core/approval-voting/src/lib.rs | 32 +++-- .../node/core/approval-voting/src/tests.rs | 6 +- polkadot/node/overseer/src/lib.rs | 128 +++++++++++++---- polkadot/node/service/Cargo.toml | 3 + polkadot/node/service/src/chain_spec.rs | 12 +- polkadot/node/service/src/grandpa_support.rs | 136 +++++++++++++++++- polkadot/node/service/src/lib.rs | 60 ++++++-- polkadot/node/subsystem/src/messages.rs | 2 +- .../src/node/approval/approval-voting.md | 4 +- .../src/types/overseer-protocol.md | 2 +- 11 files changed, 321 insertions(+), 66 deletions(-) diff --git a/polkadot/Cargo.lock b/polkadot/Cargo.lock index dee985c5d6..056ea9e8b8 100644 --- a/polkadot/Cargo.lock +++ b/polkadot/Cargo.lock @@ -5851,6 +5851,7 @@ dependencies = [ "polkadot-collator-protocol", "polkadot-network-bridge", "polkadot-node-collation-generation", + "polkadot-node-core-approval-voting", "polkadot-node-core-av-store", "polkadot-node-core-backing", "polkadot-node-core-bitfield-signing", @@ -5883,6 +5884,7 @@ dependencies = [ "sc-executor", "sc-finality-grandpa", "sc-finality-grandpa-warp-sync", + "sc-keystore", "sc-network", "sc-service", "sc-telemetry", diff --git a/polkadot/node/core/approval-voting/src/lib.rs b/polkadot/node/core/approval-voting/src/lib.rs index 16d9c6810d..3f7db15727 100644 --- a/polkadot/node/core/approval-voting/src/lib.rs +++ b/polkadot/node/core/approval-voting/src/lib.rs @@ -76,11 +76,23 @@ const LOG_TARGET: &str = "approval_voting"; /// The approval voting subsystem. pub struct ApprovalVotingSubsystem { - keystore: LocalKeystore, + keystore: Arc, slot_duration_millis: u64, db: Arc, } +impl ApprovalVotingSubsystem { + /// Create a new approval voting subsystem with the given keystore, slot duration, + /// and underlying DB. + pub fn new(keystore: Arc, slot_duration_millis: u64, db: Arc) -> Self { + ApprovalVotingSubsystem { + keystore, + slot_duration_millis, + db, + } + } +} + impl Subsystem for ApprovalVotingSubsystem where T: AuxStore + Send + Sync + 'static, C: SubsystemContext { fn start(self, ctx: C) -> SpawnedSubsystem { @@ -229,7 +241,7 @@ use approval_db_v1_reader::ApprovalDBV1Reader; struct State { session_window: import::RollingSessionWindow, - keystore: LocalKeystore, + keystore: Arc, slot_duration_millis: u64, db: T, clock: Box, @@ -529,10 +541,10 @@ async fn handle_approved_ancestor( db: &impl DBReader, target: Hash, lower_bound: BlockNumber, -) -> SubsystemResult> { +) -> SubsystemResult> { let mut all_approved_max = None; - let block_number = { + let target_number = { let (tx, rx) = oneshot::channel(); ctx.send_message(ChainApiMessage::BlockNumber(target, tx).into()).await; @@ -544,17 +556,17 @@ async fn handle_approved_ancestor( } }; - if block_number <= lower_bound { return Ok(None) } + if target_number <= lower_bound { return Ok(None) } // request ancestors up to but not including the lower bound, // as a vote on the lower bound is implied if we cannot find // anything else. - let ancestry = if block_number > lower_bound + 1 { + let ancestry = if target_number > lower_bound + 1 { let (tx, rx) = oneshot::channel(); ctx.send_message(ChainApiMessage::Ancestors { hash: target, - k: (block_number - (lower_bound + 1)) as usize, + k: (target_number - (lower_bound + 1)) as usize, response_channel: tx, }.into()).await; @@ -566,7 +578,7 @@ async fn handle_approved_ancestor( Vec::new() }; - for block_hash in std::iter::once(target).chain(ancestry) { + for (i, block_hash) in std::iter::once(target).chain(ancestry).enumerate() { // Block entries should be present as the assumption is that // nothing here is finalized. If we encounter any missing block // entries we can fail. @@ -577,7 +589,9 @@ async fn handle_approved_ancestor( if entry.is_fully_approved() { if all_approved_max.is_none() { - all_approved_max = Some(block_hash); + // First iteration of the loop is target, i = 0. After that, + // ancestry is moving backwards. + all_approved_max = Some((block_hash, target_number - i as BlockNumber)); } } else { all_approved_max = None; diff --git a/polkadot/node/core/approval-voting/src/tests.rs b/polkadot/node/core/approval-voting/src/tests.rs index 95e23998f1..7c8c9f3d94 100644 --- a/polkadot/node/core/approval-voting/src/tests.rs +++ b/polkadot/node/core/approval-voting/src/tests.rs @@ -184,7 +184,7 @@ impl DBReader for TestStore { fn blank_state() -> State { State { session_window: import::RollingSessionWindow::default(), - keystore: LocalKeystore::in_memory(), + keystore: Arc::new(LocalKeystore::in_memory()), slot_duration_millis: SLOT_DURATION_MILLIS, db: TestStore::default(), clock: Box::new(MockClock::default()), @@ -1490,7 +1490,7 @@ fn approved_ancestor_all_approved() { let test_fut = Box::pin(async move { assert_eq!( handle_approved_ancestor(&mut ctx, &state.db, block_hash_4, 0).await.unwrap(), - Some(block_hash_4), + Some((block_hash_4, 4)), ) }); @@ -1572,7 +1572,7 @@ fn approved_ancestor_missing_approval() { let test_fut = Box::pin(async move { assert_eq!( handle_approved_ancestor(&mut ctx, &state.db, block_hash_4, 0).await.unwrap(), - Some(block_hash_2), + Some((block_hash_2, 2)), ) }); diff --git a/polkadot/node/overseer/src/lib.rs b/polkadot/node/overseer/src/lib.rs index c7f2c36a04..198bcc4133 100644 --- a/polkadot/node/overseer/src/lib.rs +++ b/polkadot/node/overseer/src/lib.rs @@ -86,6 +86,7 @@ use polkadot_subsystem::messages::{ ProvisionerMessage, PoVDistributionMessage, RuntimeApiMessage, AvailabilityStoreMessage, NetworkBridgeMessage, AllMessages, CollationGenerationMessage, CollatorProtocolMessage, AvailabilityRecoveryMessage, ApprovalDistributionMessage, + ApprovalVotingMessage, }; pub use polkadot_subsystem::{ Subsystem, SubsystemContext, OverseerSignal, FromOverseer, SubsystemError, SubsystemResult, @@ -561,6 +562,9 @@ pub struct Overseer { /// An Approval Distribution subsystem. approval_distribution_subsystem: OverseenSubsystem, + /// An Approval Voting subsystem. + approval_voting_subsystem: OverseenSubsystem, + /// Spawner to spawn tasks to. s: S, @@ -601,7 +605,7 @@ pub struct Overseer { /// subsystems are implemented and the rest can be mocked with the [`DummySubsystem`]. pub struct AllSubsystems< CV = (), CB = (), CS = (), SD = (), AD = (), AR = (), BS = (), BD = (), P = (), - PoVD = (), RA = (), AS = (), NB = (), CA = (), CG = (), CP = (), ApD = (), + PoVD = (), RA = (), AS = (), NB = (), CA = (), CG = (), CP = (), ApD = (), ApV = (), > { /// A candidate validation subsystem. pub candidate_validation: CV, @@ -637,10 +641,12 @@ pub struct AllSubsystems< pub collator_protocol: CP, /// An Approval Distribution subsystem. pub approval_distribution: ApD, + /// An Approval Voting subsystem. + pub approval_voting: ApV, } -impl - AllSubsystems +impl + AllSubsystems { /// Create a new instance of [`AllSubsystems`]. /// @@ -672,6 +678,7 @@ impl DummySubsystem, DummySubsystem, DummySubsystem, + DummySubsystem, > { AllSubsystems { candidate_validation: DummySubsystem, @@ -691,6 +698,7 @@ impl collation_generation: DummySubsystem, collator_protocol: DummySubsystem, approval_distribution: DummySubsystem, + approval_voting: DummySubsystem, } } @@ -698,7 +706,7 @@ impl pub fn replace_candidate_validation( self, candidate_validation: NEW, - ) -> AllSubsystems { + ) -> AllSubsystems { AllSubsystems { candidate_validation, candidate_backing: self.candidate_backing, @@ -717,6 +725,7 @@ impl collation_generation: self.collation_generation, collator_protocol: self.collator_protocol, approval_distribution: self.approval_distribution, + approval_voting: self.approval_voting, } } @@ -724,7 +733,7 @@ impl pub fn replace_candidate_backing( self, candidate_backing: NEW, - ) -> AllSubsystems { + ) -> AllSubsystems { AllSubsystems { candidate_validation: self.candidate_validation, candidate_backing, @@ -743,6 +752,7 @@ impl collation_generation: self.collation_generation, collator_protocol: self.collator_protocol, approval_distribution: self.approval_distribution, + approval_voting: self.approval_voting, } } @@ -750,7 +760,7 @@ impl pub fn replace_candidate_selection( self, candidate_selection: NEW, - ) -> AllSubsystems { + ) -> AllSubsystems { AllSubsystems { candidate_validation: self.candidate_validation, candidate_backing: self.candidate_backing, @@ -769,6 +779,7 @@ impl collation_generation: self.collation_generation, collator_protocol: self.collator_protocol, approval_distribution: self.approval_distribution, + approval_voting: self.approval_voting, } } @@ -776,7 +787,7 @@ impl pub fn replace_statement_distribution( self, statement_distribution: NEW, - ) -> AllSubsystems { + ) -> AllSubsystems { AllSubsystems { candidate_validation: self.candidate_validation, candidate_backing: self.candidate_backing, @@ -795,6 +806,7 @@ impl collation_generation: self.collation_generation, collator_protocol: self.collator_protocol, approval_distribution: self.approval_distribution, + approval_voting: self.approval_voting, } } @@ -802,7 +814,7 @@ impl pub fn replace_availability_distribution( self, availability_distribution: NEW, - ) -> AllSubsystems { + ) -> AllSubsystems { AllSubsystems { candidate_validation: self.candidate_validation, candidate_backing: self.candidate_backing, @@ -821,6 +833,7 @@ impl collation_generation: self.collation_generation, collator_protocol: self.collator_protocol, approval_distribution: self.approval_distribution, + approval_voting: self.approval_voting, } } @@ -828,7 +841,7 @@ impl pub fn replace_availability_recovery( self, availability_recovery: NEW, - ) -> AllSubsystems { + ) -> AllSubsystems { AllSubsystems { candidate_validation: self.candidate_validation, candidate_backing: self.candidate_backing, @@ -847,6 +860,7 @@ impl collation_generation: self.collation_generation, collator_protocol: self.collator_protocol, approval_distribution: self.approval_distribution, + approval_voting: self.approval_voting, } } @@ -854,7 +868,7 @@ impl pub fn replace_bitfield_signing( self, bitfield_signing: NEW, - ) -> AllSubsystems { + ) -> AllSubsystems { AllSubsystems { candidate_validation: self.candidate_validation, candidate_backing: self.candidate_backing, @@ -873,6 +887,7 @@ impl collation_generation: self.collation_generation, collator_protocol: self.collator_protocol, approval_distribution: self.approval_distribution, + approval_voting: self.approval_voting, } } @@ -880,7 +895,7 @@ impl pub fn replace_bitfield_distribution( self, bitfield_distribution: NEW, - ) -> AllSubsystems { + ) -> AllSubsystems { AllSubsystems { candidate_validation: self.candidate_validation, candidate_backing: self.candidate_backing, @@ -899,6 +914,7 @@ impl collation_generation: self.collation_generation, collator_protocol: self.collator_protocol, approval_distribution: self.approval_distribution, + approval_voting: self.approval_voting, } } @@ -906,7 +922,7 @@ impl pub fn replace_provisioner( self, provisioner: NEW, - ) -> AllSubsystems { + ) -> AllSubsystems { AllSubsystems { candidate_validation: self.candidate_validation, candidate_backing: self.candidate_backing, @@ -925,6 +941,7 @@ impl collation_generation: self.collation_generation, collator_protocol: self.collator_protocol, approval_distribution: self.approval_distribution, + approval_voting: self.approval_voting, } } @@ -932,7 +949,7 @@ impl pub fn replace_pov_distribution( self, pov_distribution: NEW, - ) -> AllSubsystems { + ) -> AllSubsystems { AllSubsystems { candidate_validation: self.candidate_validation, candidate_backing: self.candidate_backing, @@ -951,6 +968,7 @@ impl collation_generation: self.collation_generation, collator_protocol: self.collator_protocol, approval_distribution: self.approval_distribution, + approval_voting: self.approval_voting, } } @@ -958,7 +976,7 @@ impl pub fn replace_runtime_api( self, runtime_api: NEW, - ) -> AllSubsystems { + ) -> AllSubsystems { AllSubsystems { candidate_validation: self.candidate_validation, candidate_backing: self.candidate_backing, @@ -977,6 +995,7 @@ impl collation_generation: self.collation_generation, collator_protocol: self.collator_protocol, approval_distribution: self.approval_distribution, + approval_voting: self.approval_voting, } } @@ -984,7 +1003,7 @@ impl pub fn replace_availability_store( self, availability_store: NEW, - ) -> AllSubsystems { + ) -> AllSubsystems { AllSubsystems { candidate_validation: self.candidate_validation, candidate_backing: self.candidate_backing, @@ -1003,6 +1022,7 @@ impl collation_generation: self.collation_generation, collator_protocol: self.collator_protocol, approval_distribution: self.approval_distribution, + approval_voting: self.approval_voting, } } @@ -1010,7 +1030,7 @@ impl pub fn replace_network_bridge( self, network_bridge: NEW, - ) -> AllSubsystems { + ) -> AllSubsystems { AllSubsystems { candidate_validation: self.candidate_validation, candidate_backing: self.candidate_backing, @@ -1029,6 +1049,7 @@ impl collation_generation: self.collation_generation, collator_protocol: self.collator_protocol, approval_distribution: self.approval_distribution, + approval_voting: self.approval_voting, } } @@ -1036,7 +1057,7 @@ impl pub fn replace_chain_api( self, chain_api: NEW, - ) -> AllSubsystems { + ) -> AllSubsystems { AllSubsystems { candidate_validation: self.candidate_validation, candidate_backing: self.candidate_backing, @@ -1055,6 +1076,7 @@ impl collation_generation: self.collation_generation, collator_protocol: self.collator_protocol, approval_distribution: self.approval_distribution, + approval_voting: self.approval_voting, } } @@ -1062,7 +1084,7 @@ impl pub fn replace_collation_generation( self, collation_generation: NEW, - ) -> AllSubsystems { + ) -> AllSubsystems { AllSubsystems { candidate_validation: self.candidate_validation, candidate_backing: self.candidate_backing, @@ -1081,6 +1103,7 @@ impl collation_generation, collator_protocol: self.collator_protocol, approval_distribution: self.approval_distribution, + approval_voting: self.approval_voting, } } @@ -1088,7 +1111,7 @@ impl pub fn replace_collator_protocol( self, collator_protocol: NEW, - ) -> AllSubsystems { + ) -> AllSubsystems { AllSubsystems { candidate_validation: self.candidate_validation, candidate_backing: self.candidate_backing, @@ -1107,6 +1130,7 @@ impl collation_generation: self.collation_generation, collator_protocol, approval_distribution: self.approval_distribution, + approval_voting: self.approval_voting, } } @@ -1114,7 +1138,7 @@ impl pub fn replace_approval_distribution( self, approval_distribution: NEW, - ) -> AllSubsystems { + ) -> AllSubsystems { AllSubsystems { candidate_validation: self.candidate_validation, candidate_backing: self.candidate_backing, @@ -1133,6 +1157,34 @@ impl collation_generation: self.collation_generation, collator_protocol: self.collator_protocol, approval_distribution, + approval_voting: self.approval_voting, + } + } + + /// Replace the `approval_voting` instance in `self`. + pub fn replace_approval_voting( + self, + approval_voting: NEW, + ) -> AllSubsystems { + AllSubsystems { + candidate_validation: self.candidate_validation, + candidate_backing: self.candidate_backing, + candidate_selection: self.candidate_selection, + statement_distribution: self.statement_distribution, + availability_distribution: self.availability_distribution, + availability_recovery: self.availability_recovery, + bitfield_signing: self.bitfield_signing, + bitfield_distribution: self.bitfield_distribution, + provisioner: self.provisioner, + pov_distribution: self.pov_distribution, + runtime_api: self.runtime_api, + availability_store: self.availability_store, + network_bridge: self.network_bridge, + chain_api: self.chain_api, + collation_generation: self.collation_generation, + collator_protocol: self.collator_protocol, + approval_distribution: self.approval_distribution, + approval_voting } } } @@ -1343,9 +1395,9 @@ where /// # /// # }); } /// ``` - pub fn new( + pub fn new( leaves: impl IntoIterator, - all_subsystems: AllSubsystems, + all_subsystems: AllSubsystems, prometheus_registry: Option<&prometheus::Registry>, mut s: S, ) -> SubsystemResult<(Self, OverseerHandler)> @@ -1367,6 +1419,7 @@ where CG: Subsystem> + Send, CP: Subsystem> + Send, ApD: Subsystem> + Send, + ApV: Subsystem> + Send, { let (events_tx, events_rx) = metered::channel(CHANNEL_CAPACITY, "overseer_events"); @@ -1551,6 +1604,15 @@ where &mut seed, )?; + let approval_voting_subsystem = spawn( + &mut s, + &mut running_subsystems, + metered::UnboundedMeteredSender::<_>::clone(&to_overseer_tx), + all_subsystems.approval_voting, + &metrics, + &mut seed, + )?; + let leaves = leaves .into_iter() .map(|BlockInfo { hash, parent_hash: _, number }| (hash, number)) @@ -1577,6 +1639,7 @@ where collation_generation_subsystem, collator_protocol_subsystem, approval_distribution_subsystem, + approval_voting_subsystem, s, running_subsystems, to_overseer_rx: to_overseer_rx.fuse(), @@ -1610,6 +1673,7 @@ where let _ = self.collator_protocol_subsystem.send_signal(OverseerSignal::Conclude).await; let _ = self.collation_generation_subsystem.send_signal(OverseerSignal::Conclude).await; let _ = self.approval_distribution_subsystem.send_signal(OverseerSignal::Conclude).await; + let _ = self.approval_voting_subsystem.send_signal(OverseerSignal::Conclude).await; let mut stop_delay = Delay::new(Duration::from_secs(STOP_DELAY)).fuse(); @@ -1778,7 +1842,8 @@ where self.chain_api_subsystem.send_signal(signal.clone()).await?; self.collator_protocol_subsystem.send_signal(signal.clone()).await?; self.collation_generation_subsystem.send_signal(signal.clone()).await?; - self.approval_distribution_subsystem.send_signal(signal).await?; + self.approval_distribution_subsystem.send_signal(signal.clone()).await?; + self.approval_voting_subsystem.send_signal(signal).await?; Ok(()) } @@ -1804,7 +1869,7 @@ where self.availability_distribution_subsystem.send_message(msg).await?; }, AllMessages::AvailabilityRecovery(msg) => { - let _ = self.availability_recovery_subsystem.send_message(msg).await; + self.availability_recovery_subsystem.send_message(msg).await?; }, AllMessages::BitfieldDistribution(msg) => { self.bitfield_distribution_subsystem.send_message(msg).await?; @@ -1837,10 +1902,10 @@ where self.collator_protocol_subsystem.send_message(msg).await?; }, AllMessages::ApprovalDistribution(msg) => { - let _ = self.approval_distribution_subsystem.send_message(msg).await; + self.approval_distribution_subsystem.send_message(msg).await?; }, - AllMessages::ApprovalVoting(_msg) => { - // FIXME: https://github.com/paritytech/polkadot/issues/2321 + AllMessages::ApprovalVoting(msg) => { + self.approval_voting_subsystem.send_message(msg).await?; }, } @@ -2696,6 +2761,11 @@ mod tests { ApprovalDistributionMessage::NewBlocks(Default::default()) } + fn test_approval_voting_msg() -> ApprovalVotingMessage { + let (sender, _) = oneshot::channel(); + ApprovalVotingMessage::ApprovedAncestor(Default::default(), 0, sender) + } + // Checks that `stop`, `broadcast_signal` and `broadcast_message` are implemented correctly. #[test] fn overseer_all_subsystems_receive_signals_and_messages() { @@ -2730,6 +2800,7 @@ mod tests { network_bridge: subsystem.clone(), chain_api: subsystem.clone(), approval_distribution: subsystem.clone(), + approval_voting: subsystem.clone(), }; let (overseer, mut handler) = Overseer::new( vec![], @@ -2767,13 +2838,14 @@ mod tests { handler.send_msg(AllMessages::NetworkBridge(test_network_bridge_msg())).await; handler.send_msg(AllMessages::ChainApi(test_chain_api_msg())).await; handler.send_msg(AllMessages::ApprovalDistribution(test_approval_distribution_msg())).await; + handler.send_msg(AllMessages::ApprovalVoting(test_approval_voting_msg())).await; // send a stop signal to each subsystems handler.stop().await; select! { res = overseer_fut => { - const NUM_SUBSYSTEMS: usize = 17; + const NUM_SUBSYSTEMS: usize = 18; assert_eq!(stop_signals_received.load(atomic::Ordering::SeqCst), NUM_SUBSYSTEMS); // x2 because of broadcast_signal on startup diff --git a/polkadot/node/service/Cargo.toml b/polkadot/node/service/Cargo.toml index a32e25a945..58f5c5c235 100644 --- a/polkadot/node/service/Cargo.toml +++ b/polkadot/node/service/Cargo.toml @@ -19,6 +19,7 @@ sc-executor = { git = "https://github.com/paritytech/substrate", branch = "maste sc-finality-grandpa-warp-sync = { git = "https://github.com/paritytech/substrate", branch = "master", optional = true } sc-network = { git = "https://github.com/paritytech/substrate", branch = "master" } sc-transaction-pool = { git = "https://github.com/paritytech/substrate", branch = "master" } +sc-keystore = { git = "https://github.com/paritytech/substrate", branch = "master" } service = { package = "sc-service", git = "https://github.com/paritytech/substrate", branch = "master", default-features = false } telemetry = { package = "sc-telemetry", git = "https://github.com/paritytech/substrate", branch = "master" } @@ -95,6 +96,7 @@ polkadot-node-core-runtime-api = { path = "../core/runtime-api", optional = true polkadot-pov-distribution = { path = "../network/pov-distribution", optional = true } polkadot-statement-distribution = { path = "../network/statement-distribution", optional = true } polkadot-approval-distribution = { path = "../network/approval-distribution", optional = true } +polkadot-node-core-approval-voting = { path = "../core/approval-voting", optional = true } [dev-dependencies] polkadot-test-client = { path = "../test/client" } @@ -132,4 +134,5 @@ real-overseer = [ "polkadot-pov-distribution", "polkadot-statement-distribution", "polkadot-approval-distribution", + "polkadot-node-core-approval-voting", ] diff --git a/polkadot/node/service/src/chain_spec.rs b/polkadot/node/service/src/chain_spec.rs index 4908ff717e..b099cd5d36 100644 --- a/polkadot/node/service/src/chain_spec.rs +++ b/polkadot/node/service/src/chain_spec.rs @@ -870,7 +870,6 @@ fn rococo_staging_testnet_config_genesis(wasm_binary: &[u8]) -> rococo_runtime:: group_rotation_frequency: 20, chain_availability_period: 4, thread_availability_period: 4, - no_show_slots: 10, max_upward_queue_count: 8, max_upward_queue_size: 8 * 1024, max_downward_message_size: 1024, @@ -893,6 +892,11 @@ fn rococo_staging_testnet_config_genesis(wasm_binary: &[u8]) -> rococo_runtime:: hrmp_max_parachain_outbound_channels: 4, hrmp_max_parathread_outbound_channels: 4, hrmp_max_message_num_per_candidate: 5, + no_show_slots: 2, + n_delay_tranches: 25, + needed_approvals: 2, + relay_vrf_modulo_samples: 10, + zeroth_delay_tranche_width: 0, ..Default::default() }, }), @@ -1376,7 +1380,6 @@ pub fn rococo_testnet_genesis( group_rotation_frequency: 20, chain_availability_period: 4, thread_availability_period: 4, - no_show_slots: 10, max_upward_queue_count: 8, max_upward_queue_size: 8 * 1024, max_downward_message_size: 1024, @@ -1399,6 +1402,11 @@ pub fn rococo_testnet_genesis( hrmp_max_parachain_outbound_channels: 4, hrmp_max_parathread_outbound_channels: 4, hrmp_max_message_num_per_candidate: 5, + no_show_slots: 2, + n_delay_tranches: 25, + needed_approvals: 2, + relay_vrf_modulo_samples: 10, + zeroth_delay_tranche_width: 0, ..Default::default() }, }), diff --git a/polkadot/node/service/src/grandpa_support.rs b/polkadot/node/service/src/grandpa_support.rs index dc4a81a619..d411c8ed81 100644 --- a/polkadot/node/service/src/grandpa_support.rs +++ b/polkadot/node/service/src/grandpa_support.rs @@ -18,9 +18,138 @@ use std::sync::Arc; -#[cfg(feature = "full-node")] -use polkadot_primitives::v1::Hash; +use polkadot_primitives::v1::{Block as PolkadotBlock, Header as PolkadotHeader, BlockNumber, Hash}; +use polkadot_subsystem::messages::ApprovalVotingMessage; + use sp_runtime::traits::{Block as BlockT, NumberFor}; +use sp_runtime::generic::BlockId; +use sp_runtime::traits::Header as _; + +use prometheus_endpoint::{self, Registry}; +use polkadot_overseer::OverseerHandler; + +use futures::channel::oneshot; + +/// A custom GRANDPA voting rule that acts as a diagnostic for the approval +/// voting subsystem's desired votes. +/// +/// The practical effect of this voting rule is to implement a fixed delay of +/// blocks and to issue a prometheus metric on the lag behind the head that +/// approval checking would indicate. +#[cfg(feature = "full-node")] +#[derive(Clone)] +pub(crate) struct ApprovalCheckingDiagnostic { + checking_lag: Option, + overseer: OverseerHandler, +} + +#[cfg(feature = "full-node")] +impl ApprovalCheckingDiagnostic { + /// Create a new approval checking diagnostic voting rule. + pub fn new(overseer: OverseerHandler, registry: Option<&Registry>) + -> Result + { + Ok(ApprovalCheckingDiagnostic { + checking_lag: if let Some(registry) = registry { + Some(prometheus_endpoint::register( + prometheus_endpoint::Histogram::with_opts( + prometheus_endpoint::HistogramOpts::new( + "approval_checking_finality_lag", + "How far behind the head of the chain the Approval Checking protocol wants to vote", + ).buckets(vec![1.0, 2.0, 3.0, 4.0, 5.0, 10.0, 20.0, 30.0, 40.0, 50.0]) + )?, + registry, + )?) + } else { + None + }, + overseer, + }) + } +} + +#[cfg(feature = "full-node")] +impl grandpa::VotingRule for ApprovalCheckingDiagnostic + where B: sp_blockchain::HeaderBackend +{ + fn restrict_vote( + &self, + backend: Arc, + base: &PolkadotHeader, + _best_target: &PolkadotHeader, + current_target: &PolkadotHeader, + ) -> grandpa::VotingRuleResult { + // always wait 50 blocks behind the head to finalize. + const DIAGNOSTIC_GRANDPA_DELAY: BlockNumber = 50; + + let aux = || { + let find_target = |target_number: BlockNumber, current_header: &PolkadotHeader| { + let mut target_hash = current_header.hash(); + let mut target_header = current_header.clone(); + + loop { + if *target_header.number() < target_number { + unreachable!( + "we are traversing backwards from a known block; \ + blocks are stored contiguously; \ + qed" + ); + } + + if *target_header.number() == target_number { + return Some((target_hash, target_number)); + } + + target_hash = *target_header.parent_hash(); + target_header = backend.header(BlockId::Hash(target_hash)).ok()? + .expect("Header known to exist due to the existence of one of its descendents; qed"); + } + }; + + let target_number = std::cmp::max( + current_target.number().saturating_sub(DIAGNOSTIC_GRANDPA_DELAY), + base.number().clone(), + ); + + find_target(target_number, current_target) + }; + + let actual_vote_target = aux(); + + // Query approval checking and issue metrics. + let mut overseer = self.overseer.clone(); + let checking_lag = self.checking_lag.clone(); + + let current_hash = current_target.hash(); + let current_number = current_target.number.clone(); + + let base_number = base.number; + + Box::pin(async move { + let (tx, rx) = oneshot::channel(); + let approval_checking_subsystem_vote = { + overseer.send_msg(ApprovalVotingMessage::ApprovedAncestor( + current_hash, + base_number, + tx, + )).await; + + rx.await.ok().and_then(|v| v) + }; + + let approval_checking_subsystem_lag = approval_checking_subsystem_vote.map_or( + current_number - base_number, + |(_h, n)| current_number - n, + ); + + if let Some(ref checking_lag) = checking_lag { + checking_lag.observe(approval_checking_subsystem_lag as _); + } + + actual_vote_target + }) + } +} /// A custom GRANDPA voting rule that "pauses" voting (i.e. keeps voting for the /// same last finalized block) after a given block at height `N` has been @@ -42,9 +171,6 @@ where current_target: &Block::Header, ) -> grandpa::VotingRuleResult { let aux = || { - use sp_runtime::generic::BlockId; - use sp_runtime::traits::Header as _; - // walk backwards until we find the target block let find_target = |target_number: NumberFor, current_header: &Block::Header| { let mut target_hash = current_header.hash(); diff --git a/polkadot/node/service/src/lib.rs b/polkadot/node/service/src/lib.rs index 2f720f1f11..abdb303219 100644 --- a/polkadot/node/service/src/lib.rs +++ b/polkadot/node/service/src/lib.rs @@ -22,7 +22,6 @@ pub mod chain_spec; mod grandpa_support; mod client; -use grandpa::{self, FinalityProofProvider as GrandpaFinalityProofProvider}; #[cfg(feature = "full-node")] use { std::convert::TryInto, @@ -35,10 +34,11 @@ use { polkadot_primitives::v1::ParachainHost, sc_authority_discovery::Service as AuthorityDiscoveryService, sp_blockchain::HeaderBackend, - sp_keystore::SyncCryptoStorePtr, sp_trie::PrefixedMemoryDB, - sc_client_api::ExecutorProvider, + sc_client_api::{AuxStore, ExecutorProvider}, + sc_keystore::LocalKeystore, babe_primitives::BabeApi, + grandpa::{self, FinalityProofProvider as GrandpaFinalityProofProvider}, }; #[cfg(feature = "real-overseer")] use polkadot_network_bridge::RequestMultiplexer; @@ -348,7 +348,7 @@ fn new_partial(config: &mut Configuration, jaeger_agent: O #[cfg(all(feature="full-node", not(feature = "real-overseer")))] fn real_overseer( leaves: impl IntoIterator, - _: SyncCryptoStorePtr, + _: Arc, _: Arc, _: AvailabilityConfig, _: Arc>, @@ -361,8 +361,8 @@ fn real_overseer( _: u64, ) -> Result<(Overseer, OverseerHandler), Error> where - RuntimeClient: 'static + ProvideRuntimeApi + HeaderBackend, - RuntimeClient::Api: ParachainHost, + RuntimeClient: 'static + ProvideRuntimeApi + HeaderBackend + AuxStore, + RuntimeClient::Api: ParachainHost + BabeApi, Spawner: 'static + SpawnNamed + Clone + Unpin, { Overseer::new( @@ -376,7 +376,7 @@ where #[cfg(all(feature = "full-node", feature = "real-overseer"))] fn real_overseer( leaves: impl IntoIterator, - keystore: SyncCryptoStorePtr, + keystore: Arc, runtime_client: Arc, availability_config: AvailabilityConfig, network_service: Arc>, @@ -386,10 +386,10 @@ fn real_overseer( spawner: Spawner, is_collator: IsCollator, isolation_strategy: IsolationStrategy, - _slot_duration: u64, // TODO [now]: instantiate approval voting. + slot_duration: u64, ) -> Result<(Overseer, OverseerHandler), Error> where - RuntimeClient: 'static + ProvideRuntimeApi + HeaderBackend, + RuntimeClient: 'static + ProvideRuntimeApi + HeaderBackend + AuxStore, RuntimeClient::Api: ParachainHost + BabeApi, Spawner: 'static + SpawnNamed + Clone + Unpin, { @@ -412,6 +412,7 @@ where use polkadot_statement_distribution::StatementDistribution as StatementDistributionSubsystem; use polkadot_availability_recovery::AvailabilityRecoverySubsystem; use polkadot_approval_distribution::ApprovalDistribution as ApprovalDistributionSubsystem; + use polkadot_node_core_approval_voting::ApprovalVotingSubsystem; let all_subsystems = AllSubsystems { availability_distribution: AvailabilityDistributionSubsystem::new( @@ -477,7 +478,7 @@ where Metrics::register(registry)?, ), runtime_api: RuntimeApiSubsystem::new( - runtime_client, + runtime_client.clone(), Metrics::register(registry)?, spawner.clone(), ), @@ -487,6 +488,11 @@ where approval_distribution: ApprovalDistributionSubsystem::new( Metrics::register(registry)?, ), + approval_voting: ApprovalVotingSubsystem::new( + keystore.clone(), + slot_duration, + runtime_client.clone(), + ), }; Overseer::new( @@ -563,7 +569,12 @@ pub fn new_full( let role = config.role.clone(); let force_authoring = config.force_authoring; let backoff_authoring_blocks = - Some(sc_consensus_slots::BackoffAuthoringOnFinalizedHeadLagging::default()); + Some(sc_consensus_slots::BackoffAuthoringOnFinalizedHeadLagging { + #[cfg(feature = "real-overseer")] + unfinalized_slack: 100, + ..Default::default() + }); + let disable_grandpa = config.disable_grandpa; let name = config.network.node_name.clone(); @@ -705,10 +716,18 @@ pub fn new_full( // we'd say let overseer_handler = authority_discovery_service.map(|authority_discovery_service|, ...), // but in that case we couldn't use ? to propagate errors - let overseer_handler = if let Some(authority_discovery_service) = authority_discovery_service { + let local_keystore = keystore_container.local_keystore(); + if local_keystore.is_none() { + tracing::info!("Cannot run as validator without local keystore."); + } + + let maybe_params = local_keystore + .and_then(move |k| authority_discovery_service.map(|a| (a, k))); + + let overseer_handler = if let Some((authority_discovery_service, keystore)) = maybe_params { let (overseer, overseer_handler) = real_overseer( leaves, - keystore_container.sync_keystore(), + keystore, overseer_client.clone(), availability_config, network.clone(), @@ -803,6 +822,17 @@ pub fn new_full( // add a custom voting rule to temporarily stop voting for new blocks // after the given pause block is finalized and restarting after the // given delay. + let builder = grandpa::VotingRulesBuilder::default(); + + let builder = if let Some(ref overseer) = overseer_handler { + builder.add(grandpa_support::ApprovalCheckingDiagnostic::new( + overseer.clone(), + prometheus_registry.as_ref(), + )?) + } else { + builder + }; + let voting_rule = match grandpa_pause { Some((block, delay)) => { info!( @@ -813,11 +843,11 @@ pub fn new_full( delay, ); - grandpa::VotingRulesBuilder::default() + builder .add(grandpa_support::PauseAfterBlockFor(block, delay)) .build() } - None => grandpa::VotingRulesBuilder::default().build(), + None => builder.build(), }; let grandpa_config = grandpa::GrandpaParams { diff --git a/polkadot/node/subsystem/src/messages.rs b/polkadot/node/subsystem/src/messages.rs index f670fdd5af..c240b14ba0 100644 --- a/polkadot/node/subsystem/src/messages.rs +++ b/polkadot/node/subsystem/src/messages.rs @@ -661,7 +661,7 @@ pub enum ApprovalVotingMessage { /// /// It can also return the same block hash, if that is acceptable to vote upon. /// Return `None` if the input hash is unrecognized. - ApprovedAncestor(Hash, BlockNumber, oneshot::Sender>), + ApprovedAncestor(Hash, BlockNumber, oneshot::Sender>), } /// Message to the Approval Distribution subsystem. diff --git a/polkadot/roadmap/implementers-guide/src/node/approval/approval-voting.md b/polkadot/roadmap/implementers-guide/src/node/approval/approval-voting.md index e00a1525cf..db71720e4c 100644 --- a/polkadot/roadmap/implementers-guide/src/node/approval/approval-voting.md +++ b/polkadot/roadmap/implementers-guide/src/node/approval/approval-voting.md @@ -218,9 +218,9 @@ On receiving a `CheckAndImportApproval(indirect_approval_vote, response_channel) On receiving an `ApprovedAncestor(Hash, BlockNumber, response_channel)`: * Iterate over the ancestry of the hash all the way back to block number given, starting from the provided block hash. - * Keep track of an `all_approved_max: Option`. + * Keep track of an `all_approved_max: Option<(Hash, BlockNumber)>`. * For each block hash encountered, load the `BlockEntry` associated. If any are not found, return `None` on the response channel and conclude. - * If the block entry's `approval_bitfield` has all bits set to 1 and `all_approved_max == None`, set `all_approved_max = Some(current_hash)`. + * If the block entry's `approval_bitfield` has all bits set to 1 and `all_approved_max == None`, set `all_approved_max = Some((current_hash, current_number))`. * If the block entry's `approval_bitfield` has any 0 bits, set `all_approved_max = None`. * After iterating all ancestry, return `all_approved_max`. diff --git a/polkadot/roadmap/implementers-guide/src/types/overseer-protocol.md b/polkadot/roadmap/implementers-guide/src/types/overseer-protocol.md index a9aa308983..9e740dde47 100644 --- a/polkadot/roadmap/implementers-guide/src/types/overseer-protocol.md +++ b/polkadot/roadmap/implementers-guide/src/types/overseer-protocol.md @@ -81,7 +81,7 @@ enum ApprovalVotingMessage { /// /// It can also return the same block hash, if that is acceptable to vote upon. /// Return `None` if the input hash is unrecognized. - ApprovedAncestor(Hash, BlockNumber, ResponseChannel>), + ApprovedAncestor(Hash, BlockNumber, ResponseChannel>), } ```