From 46a4231abecf9ec149826ecb0bc21e1f81062095 Mon Sep 17 00:00:00 2001 From: acatangiu Date: Thu, 25 Aug 2022 18:58:09 +0300 Subject: [PATCH 01/31] client/beefy: create communication module and move gossip there --- .../beefy/src/{ => communication}/gossip.rs | 0 client/beefy/src/communication/mod.rs | 21 +++++++++++++++++++ client/beefy/src/lib.rs | 4 ++-- client/beefy/src/worker.rs | 4 ++-- 4 files changed, 25 insertions(+), 4 deletions(-) rename client/beefy/src/{ => communication}/gossip.rs (100%) create mode 100644 client/beefy/src/communication/mod.rs diff --git a/client/beefy/src/gossip.rs b/client/beefy/src/communication/gossip.rs similarity index 100% rename from client/beefy/src/gossip.rs rename to client/beefy/src/communication/gossip.rs diff --git a/client/beefy/src/communication/mod.rs b/client/beefy/src/communication/mod.rs new file mode 100644 index 0000000000000..35baafff5780d --- /dev/null +++ b/client/beefy/src/communication/mod.rs @@ -0,0 +1,21 @@ +// This file is part of Substrate. + +// Copyright (C) 2022 Parity Technologies (UK) Ltd. +// SPDX-License-Identifier: GPL-3.0-or-later WITH Classpath-exception-2.0 + +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. + +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License +// along with this program. If not, see . + +//! Communication streams for the BEEFY networking protocols. + +pub mod gossip; diff --git a/client/beefy/src/lib.rs b/client/beefy/src/lib.rs index bdf44e056786c..62261e8bfaa5e 100644 --- a/client/beefy/src/lib.rs +++ b/client/beefy/src/lib.rs @@ -29,8 +29,8 @@ use sp_mmr_primitives::MmrApi; use sp_runtime::traits::Block; use std::sync::Arc; +mod communication; mod error; -mod gossip; mod keystore; mod metrics; mod round; @@ -232,7 +232,7 @@ where } = beefy_params; let sync_oracle = network.clone(); - let gossip_validator = Arc::new(gossip::GossipValidator::new()); + let gossip_validator = Arc::new(communication::gossip::GossipValidator::new()); let gossip_engine = sc_network_gossip::GossipEngine::new( network, protocol_name, diff --git a/client/beefy/src/worker.rs b/client/beefy/src/worker.rs index 9f1938fa91c33..6b7686c331b21 100644 --- a/client/beefy/src/worker.rs +++ b/client/beefy/src/worker.rs @@ -47,8 +47,8 @@ use beefy_primitives::{ }; use crate::{ + communication::gossip::{topic, GossipValidator}, error::Error, - gossip::{topic, GossipValidator}, justification::BeefyVersionedFinalityProof, keystore::BeefyKeystore, metric_inc, metric_set, @@ -881,7 +881,7 @@ pub(crate) mod tests { let api = Arc::new(TestApi {}); let network = peer.network_service().clone(); let sync_oracle = network.clone(); - let gossip_validator = Arc::new(crate::gossip::GossipValidator::new()); + let gossip_validator = Arc::new(crate::communication::gossip::GossipValidator::new()); let gossip_engine = GossipEngine::new(network, BEEFY_PROTOCOL_NAME, gossip_validator.clone(), None); let worker_params = crate::worker::WorkerParams { From cda48fcc332c8848f00f84058a30ed4462155afc Mon Sep 17 00:00:00 2001 From: acatangiu Date: Thu, 25 Aug 2022 19:05:19 +0300 Subject: [PATCH 02/31] client/beefy: move beefy_protocol_name module to communication --- client/beefy/src/communication/mod.rs | 51 ++++++++++++++++++++++++++- client/beefy/src/lib.rs | 40 ++------------------- client/beefy/src/tests.rs | 15 +++----- 3 files changed, 57 insertions(+), 49 deletions(-) diff --git a/client/beefy/src/communication/mod.rs b/client/beefy/src/communication/mod.rs index 35baafff5780d..68f3a539f7814 100644 --- a/client/beefy/src/communication/mod.rs +++ b/client/beefy/src/communication/mod.rs @@ -18,4 +18,53 @@ //! Communication streams for the BEEFY networking protocols. -pub mod gossip; +pub(crate) mod gossip; + +pub(crate) mod beefy_protocol_name { + /// BEEFY votes gossip protocol name suffix. + const GOSSIP_NAME: &str = "/beefy/1"; + /// Old names for the gossip protocol, used for backward compatibility. + pub(super) const LEGACY_NAMES: [&str; 1] = ["/paritytech/beefy/1"]; + + /// BEEFY justifications protocol name suffix. + const JUSTIFICATIONS_NAME: &str = "/beefy/justifications/1"; + + /// Name of the votes gossip protocol used by BEEFY. + /// + /// Must be registered towards the networking in order for BEEFY voter to properly function. + pub fn gossip_protocol_name>( + genesis_hash: Hash, + fork_id: Option<&str>, + ) -> std::borrow::Cow<'static, str> { + if let Some(fork_id) = fork_id { + format!("/{}/{}{}", hex::encode(genesis_hash), fork_id, GOSSIP_NAME).into() + } else { + format!("/{}{}", hex::encode(genesis_hash), GOSSIP_NAME).into() + } + } + + /// Name of the BEEFY justifications request-response protocol. + pub fn justifications_protocol_name>( + genesis_hash: Hash, + fork_id: Option<&str>, + ) -> std::borrow::Cow<'static, str> { + if let Some(fork_id) = fork_id { + format!("/{}/{}{}", hex::encode(genesis_hash), fork_id, JUSTIFICATIONS_NAME).into() + } else { + format!("/{}{}", hex::encode(genesis_hash), JUSTIFICATIONS_NAME).into() + } + } +} + +/// Returns the configuration value to put in +/// [`sc_network::config::NetworkConfiguration::extra_sets`]. +/// For standard protocol name see [`beefy_protocol_name::gossip_protocol_name`]. +pub fn beefy_peers_set_config( + gossip_protocol_name: std::borrow::Cow<'static, str>, +) -> sc_network::config::NonDefaultSetConfig { + let mut cfg = sc_network::config::NonDefaultSetConfig::new(gossip_protocol_name, 1024 * 1024); + + cfg.allow_non_reserved(25, 25); + cfg.add_fallback_names(beefy_protocol_name::LEGACY_NAMES.iter().map(|&n| n.into()).collect()); + cfg +} diff --git a/client/beefy/src/lib.rs b/client/beefy/src/lib.rs index 62261e8bfaa5e..0da9eac6e9dec 100644 --- a/client/beefy/src/lib.rs +++ b/client/beefy/src/lib.rs @@ -29,13 +29,13 @@ use sp_mmr_primitives::MmrApi; use sp_runtime::traits::Block; use std::sync::Arc; -mod communication; mod error; mod keystore; mod metrics; mod round; mod worker; +pub mod communication; pub mod import; pub mod justification; pub mod notification; @@ -51,42 +51,8 @@ use crate::{ }, }; -pub use beefy_protocol_name::standard_name as protocol_standard_name; - -pub(crate) mod beefy_protocol_name { - use sc_chain_spec::ChainSpec; - - const NAME: &str = "/beefy/1"; - /// Old names for the notifications protocol, used for backward compatibility. - pub(crate) const LEGACY_NAMES: [&str; 1] = ["/paritytech/beefy/1"]; - - /// Name of the notifications protocol used by BEEFY. - /// - /// Must be registered towards the networking in order for BEEFY to properly function. - pub fn standard_name>( - genesis_hash: &Hash, - chain_spec: &Box, - ) -> std::borrow::Cow<'static, str> { - let chain_prefix = match chain_spec.fork_id() { - Some(fork_id) => format!("/{}/{}", hex::encode(genesis_hash), fork_id), - None => format!("/{}", hex::encode(genesis_hash)), - }; - format!("{}{}", chain_prefix, NAME).into() - } -} - -/// Returns the configuration value to put in -/// [`sc_network::config::NetworkConfiguration::extra_sets`]. -/// For standard protocol name see [`beefy_protocol_name::standard_name`]. -pub fn beefy_peers_set_config( - protocol_name: std::borrow::Cow<'static, str>, -) -> sc_network::config::NonDefaultSetConfig { - let mut cfg = sc_network::config::NonDefaultSetConfig::new(protocol_name, 1024 * 1024); - - cfg.allow_non_reserved(25, 25); - cfg.add_fallback_names(beefy_protocol_name::LEGACY_NAMES.iter().map(|&n| n.into()).collect()); - cfg -} +pub use communication::beefy_protocol_name::gossip_protocol_name as gossip_protocol_name; +pub use communication::beefy_protocol_name::justifications_protocol_name as justifs_protocol_name; /// A convenience BEEFY client trait that defines all the type bounds a BEEFY client /// has to satisfy. Ideally that should actually be a trait alias. Unfortunately as diff --git a/client/beefy/src/tests.rs b/client/beefy/src/tests.rs index f0257d179cb33..18074016e23a7 100644 --- a/client/beefy/src/tests.rs +++ b/client/beefy/src/tests.rs @@ -24,7 +24,6 @@ use serde::{Deserialize, Serialize}; use std::{collections::HashMap, sync::Arc, task::Poll}; use tokio::{runtime::Runtime, time::Duration}; -use sc_chain_spec::{ChainSpec, GenericChainSpec}; use sc_client_api::HeaderBackend; use sc_consensus::{ BlockImport, BlockImportParams, BoxJustificationImport, ForkChoiceStrategy, ImportResult, @@ -60,8 +59,8 @@ use sp_runtime::{ use substrate_test_runtime_client::{runtime::Header, ClientExt}; use crate::{ - beefy_block_import_and_links, beefy_protocol_name, justification::*, - keystore::tests::Keyring as BeefyKeyring, BeefyRPCLinks, BeefyVoterLinks, + beefy_block_import_and_links, communication::beefy_protocol_name::gossip_protocol_name, + justification::*, keystore::tests::Keyring as BeefyKeyring, BeefyRPCLinks, BeefyVoterLinks, }; pub(crate) const BEEFY_PROTOCOL_NAME: &'static str = "/beefy/1"; @@ -91,16 +90,10 @@ impl BuildStorage for Genesis { #[test] fn beefy_protocol_name() { - let chain_spec = GenericChainSpec::::from_json_bytes( - &include_bytes!("../../chain-spec/res/chain_spec.json")[..], - ) - .unwrap() - .cloned_box(); - // Create protocol name using random genesis hash. let genesis_hash = H256::random(); let expected = format!("/{}/beefy/1", hex::encode(genesis_hash)); - let proto_name = beefy_protocol_name::standard_name(&genesis_hash, &chain_spec); + let proto_name = gossip_protocol_name(&genesis_hash, None); assert_eq!(proto_name.to_string(), expected); // Create protocol name using hardcoded genesis hash. Verify exact representation. @@ -110,7 +103,7 @@ fn beefy_protocol_name() { ]; let expected = "/32043c7b3a6ad8f6c2bc8bc121d4caab09377b5e082b0cfbbb39ad13bc4acd93/beefy/1".to_string(); - let proto_name = beefy_protocol_name::standard_name(&genesis_hash, &chain_spec); + let proto_name = gossip_protocol_name(&genesis_hash, None); assert_eq!(proto_name.to_string(), expected); } From e8157d7f592bd5361704f17716f9e4737dcd1693 Mon Sep 17 00:00:00 2001 From: acatangiu Date: Thu, 25 Aug 2022 19:11:15 +0300 Subject: [PATCH 03/31] client/beefy: move notification module under communication --- client/beefy/rpc/src/lib.rs | 6 +----- client/beefy/src/communication/mod.rs | 2 ++ .../src/{ => communication}/notification.rs | 0 client/beefy/src/import.rs | 2 +- client/beefy/src/lib.rs | 16 ++++++++-------- client/beefy/src/worker.rs | 2 +- 6 files changed, 13 insertions(+), 15 deletions(-) rename client/beefy/src/{ => communication}/notification.rs (100%) diff --git a/client/beefy/rpc/src/lib.rs b/client/beefy/rpc/src/lib.rs index 3be182ceb8f39..77a1c5d751be9 100644 --- a/client/beefy/rpc/src/lib.rs +++ b/client/beefy/rpc/src/lib.rs @@ -35,7 +35,7 @@ use jsonrpsee::{ }; use log::warn; -use beefy_gadget::notification::{BeefyBestBlockStream, BeefyVersionedFinalityProofStream}; +use beefy_gadget::communication::notification::{BeefyBestBlockStream, BeefyVersionedFinalityProofStream}; mod notification; @@ -164,10 +164,6 @@ where mod tests { use super::*; - use beefy_gadget::{ - justification::BeefyVersionedFinalityProof, - notification::{BeefyBestBlockStream, BeefyVersionedFinalityProofSender}, - }; use beefy_primitives::{known_payload_ids, Payload, SignedCommitment}; use codec::{Decode, Encode}; use jsonrpsee::{types::EmptyParams, RpcModule}; diff --git a/client/beefy/src/communication/mod.rs b/client/beefy/src/communication/mod.rs index 68f3a539f7814..5ed5cd26eb3ef 100644 --- a/client/beefy/src/communication/mod.rs +++ b/client/beefy/src/communication/mod.rs @@ -18,6 +18,8 @@ //! Communication streams for the BEEFY networking protocols. +pub mod notification; + pub(crate) mod gossip; pub(crate) mod beefy_protocol_name { diff --git a/client/beefy/src/notification.rs b/client/beefy/src/communication/notification.rs similarity index 100% rename from client/beefy/src/notification.rs rename to client/beefy/src/communication/notification.rs diff --git a/client/beefy/src/import.rs b/client/beefy/src/import.rs index 129484199de89..1920790dc34ea 100644 --- a/client/beefy/src/import.rs +++ b/client/beefy/src/import.rs @@ -34,8 +34,8 @@ use sc_client_api::backend::Backend; use sc_consensus::{BlockCheckParams, BlockImport, BlockImportParams, ImportResult}; use crate::{ + communication::notification::BeefyVersionedFinalityProofSender, justification::{decode_and_verify_finality_proof, BeefyVersionedFinalityProof}, - notification::BeefyVersionedFinalityProofSender, }; /// A block-import handler for BEEFY. diff --git a/client/beefy/src/lib.rs b/client/beefy/src/lib.rs index 0da9eac6e9dec..7cd0a9bf8f3e6 100644 --- a/client/beefy/src/lib.rs +++ b/client/beefy/src/lib.rs @@ -38,21 +38,21 @@ mod worker; pub mod communication; pub mod import; pub mod justification; -pub mod notification; #[cfg(test)] mod tests; use crate::{ - import::BeefyBlockImport, - notification::{ + communication::notification::{ BeefyBestBlockSender, BeefyBestBlockStream, BeefyVersionedFinalityProofSender, BeefyVersionedFinalityProofStream, }, + import::BeefyBlockImport, }; -pub use communication::beefy_protocol_name::gossip_protocol_name as gossip_protocol_name; -pub use communication::beefy_protocol_name::justifications_protocol_name as justifs_protocol_name; +pub use communication::beefy_protocol_name::{ + gossip_protocol_name, justifications_protocol_name as justifs_protocol_name, +}; /// A convenience BEEFY client trait that defines all the type bounds a BEEFY client /// has to satisfy. Ideally that should actually be a trait alias. Unfortunately as @@ -122,13 +122,13 @@ where { // Voter -> RPC links let (to_rpc_justif_sender, from_voter_justif_stream) = - notification::BeefyVersionedFinalityProofStream::::channel(); + BeefyVersionedFinalityProofStream::::channel(); let (to_rpc_best_block_sender, from_voter_best_beefy_stream) = - notification::BeefyBestBlockStream::::channel(); + BeefyBestBlockStream::::channel(); // BlockImport -> Voter links let (to_voter_justif_sender, from_block_import_justif_stream) = - notification::BeefyVersionedFinalityProofStream::::channel(); + BeefyVersionedFinalityProofStream::::channel(); // BlockImport let import = diff --git a/client/beefy/src/worker.rs b/client/beefy/src/worker.rs index 6b7686c331b21..5eda41ef9a1b9 100644 --- a/client/beefy/src/worker.rs +++ b/client/beefy/src/worker.rs @@ -834,8 +834,8 @@ where pub(crate) mod tests { use super::*; use crate::{ + communication::notification::{BeefyBestBlockStream, BeefyVersionedFinalityProofStream}, keystore::tests::Keyring, - notification::{BeefyBestBlockStream, BeefyVersionedFinalityProofStream}, tests::{ create_beefy_keystore, get_beefy_streams, make_beefy_ids, two_validators::TestApi, BeefyPeer, BeefyTestNet, BEEFY_PROTOCOL_NAME, From f9bf81bd6605d05f707bf094087f477cdd40f883 Mon Sep 17 00:00:00 2001 From: acatangiu Date: Fri, 26 Aug 2022 15:39:12 +0300 Subject: [PATCH 04/31] client/beefy: add incoming request_response protocol handler --- client/beefy/src/communication/mod.rs | 7 +- .../request_response/incoming_handler.rs | 98 ++++++++++ .../src/communication/request_response/mod.rs | 172 ++++++++++++++++++ 3 files changed, 274 insertions(+), 3 deletions(-) create mode 100644 client/beefy/src/communication/request_response/incoming_handler.rs create mode 100644 client/beefy/src/communication/request_response/mod.rs diff --git a/client/beefy/src/communication/mod.rs b/client/beefy/src/communication/mod.rs index 5ed5cd26eb3ef..0a497dcbb4735 100644 --- a/client/beefy/src/communication/mod.rs +++ b/client/beefy/src/communication/mod.rs @@ -19,18 +19,19 @@ //! Communication streams for the BEEFY networking protocols. pub mod notification; +pub mod request_response; pub(crate) mod gossip; pub(crate) mod beefy_protocol_name { /// BEEFY votes gossip protocol name suffix. const GOSSIP_NAME: &str = "/beefy/1"; - /// Old names for the gossip protocol, used for backward compatibility. - pub(super) const LEGACY_NAMES: [&str; 1] = ["/paritytech/beefy/1"]; - /// BEEFY justifications protocol name suffix. const JUSTIFICATIONS_NAME: &str = "/beefy/justifications/1"; + /// Old names for the gossip protocol, used for backward compatibility. + pub(super) const LEGACY_NAMES: [&str; 1] = ["/paritytech/beefy/1"]; + /// Name of the votes gossip protocol used by BEEFY. /// /// Must be registered towards the networking in order for BEEFY voter to properly function. diff --git a/client/beefy/src/communication/request_response/incoming_handler.rs b/client/beefy/src/communication/request_response/incoming_handler.rs new file mode 100644 index 0000000000000..55c01972f73ea --- /dev/null +++ b/client/beefy/src/communication/request_response/incoming_handler.rs @@ -0,0 +1,98 @@ +// Copyright 2022 Parity Technologies (UK) Ltd. +// This file is part of Substrate. + +// Substrate is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. + +// Substrate is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License +// along with Substrate. If not, see . + +//! Helper for handling (i.e. answering) BEEFY justifications requests from a remote peer. + +use beefy_primitives::BEEFY_ENGINE_ID; +use log::debug; +use sc_client_api::BlockBackend; +use sc_network::{config as netconfig, config::RequestResponseConfig}; +use sp_runtime::{generic::BlockId, traits::Block}; +use std::{marker::PhantomData, sync::Arc}; + +use crate::communication::request_response::{ + justif_protocol_config, Error, IncomingRequest, IncomingRequestReceiver, +}; + +/// Handler for incoming BEEFY justifications requests from a remote peer. +pub struct BeefyJustifsRequestHandler { + request_receiver: IncomingRequestReceiver, + /// Blockchain client. + client: Arc, + _block: PhantomData, +} + +impl BeefyJustifsRequestHandler +where + B: Block, + Client: BlockBackend + Send + Sync + 'static, +{ + /// Create a new [`BeefyJustifsRequestHandler`]. + pub fn new(fork_id: Option<&str>, client: Arc) -> (Self, RequestResponseConfig) { + let genesis_hash = client + .block_hash(0u32.into()) + .ok() + .flatten() + .expect("Genesis block exists; qed"); + let (request_receiver, config) = justif_protocol_config(genesis_hash, fork_id); + + (Self { client, request_receiver, _block: PhantomData }, config) + } + + async fn handle_request(&self, request: IncomingRequest) -> Result<(), Error> { + // TODO: validate `request.begin` and change peer reputation for invalid requests. + + let encoded_proof = self + .client + .justifications(&BlockId::Number(request.payload.begin)) + .map_err(Error::Client)? + .map(|justifs| justifs.get(BEEFY_ENGINE_ID).cloned()) + .flatten() + // No BEEFY justification present. + .ok_or(()); + + request + .pending_response + .send(netconfig::OutgoingResponse { + result: encoded_proof, + reputation_changes: Vec::new(), + sent_feedback: None, + }) + .map_err(|_| Error::SendResponse) + } + + /// Run [`BeefyJustifsRequestHandler`]. + pub async fn run(mut self) { + while let Ok(request) = self.request_receiver.recv(|| vec![]).await { + let peer = request.peer; + match self.handle_request(request).await { + Ok(()) => { + debug!( + target: "beefy::sync", + "Handled BEEFY justification request from {}.", peer + ) + }, + Err(e) => { + // TODO: handle reputation changes here + debug!( + target: "beefy::sync", + "Failed to handle BEEFY justification request from {}: {}", peer, e, + ) + }, + } + } + } +} diff --git a/client/beefy/src/communication/request_response/mod.rs b/client/beefy/src/communication/request_response/mod.rs new file mode 100644 index 0000000000000..01005cd0a23f7 --- /dev/null +++ b/client/beefy/src/communication/request_response/mod.rs @@ -0,0 +1,172 @@ +// This file is part of Substrate. + +// Copyright (C) 2022 Parity Technologies (UK) Ltd. +// SPDX-License-Identifier: GPL-3.0-or-later WITH Classpath-exception-2.0 + +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. + +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License +// along with this program. If not, see . + +//! Request/response protocol for syncing BEEFY justifications. + +pub(crate) mod incoming_handler; + +use futures::{ + channel::{mpsc, oneshot}, + StreamExt, +}; +use std::time::Duration; + +use codec::{Decode, Encode, Error as CodecError}; +use sc_network::{config as netconfig, config::RequestResponseConfig, PeerId, ReputationChange}; +use sp_runtime::traits::{Block, NumberFor}; + +use crate::communication::beefy_protocol_name::justifications_protocol_name; + +// 10 seems reasonable, considering justifs are explicitly requested only +// for mandatory blocks, by nodes that are syncing/catching-up. +const JUSTIF_CHANNEL_SIZE: usize = 10; + +const MAX_RESPONSE_SIZE: u64 = 1024 * 1024; +const JUSTIF_REQUEST_TIMEOUT: Duration = Duration::from_secs(3); + +/// Get the configuration for the BEEFY justifications Request/response protocol. +/// +/// Returns a receiver for messages received on this protocol and the requested +/// `ProtocolConfig`. +pub fn justif_protocol_config>( + genesis_hash: Hash, + fork_id: Option<&str>, +) -> (IncomingRequestReceiver, RequestResponseConfig) { + let name = justifications_protocol_name(genesis_hash, fork_id); + let fallback_names = vec![]; + let (tx, rx) = mpsc::channel(JUSTIF_CHANNEL_SIZE); + let rx = IncomingRequestReceiver { raw: rx }; + let cfg = RequestResponseConfig { + name, + fallback_names, + max_request_size: 32, + max_response_size: MAX_RESPONSE_SIZE, + // We are connected to all validators: + request_timeout: JUSTIF_REQUEST_TIMEOUT, + inbound_queue: Some(tx), + }; + (rx, cfg) +} + +/// BEEFY justification request. +#[derive(Debug, Clone, Encode, Decode)] +pub struct JustificationRequest { + /// Start collecting proofs from this block. + pub begin: NumberFor, +} + +/// A request coming in, including a sender for sending responses. +#[derive(Debug)] +pub struct IncomingRequest { + /// `PeerId` of sending peer. + pub peer: PeerId, + /// The sent request. + pub payload: JustificationRequest, + /// Sender for sending response back. + pub pending_response: oneshot::Sender, +} + +impl IncomingRequest { + /// Create new `IncomingRequest`. + pub fn new( + peer: PeerId, + payload: JustificationRequest, + pending_response: oneshot::Sender, + ) -> Self { + Self { peer, payload, pending_response } + } + + /// Try building from raw network request. + /// + /// This function will fail if the request cannot be decoded and will apply passed in + /// reputation changes in that case. + /// + /// Params: + /// - The raw request to decode + /// - Reputation changes to apply for the peer in case decoding fails. + fn try_from_raw( + raw: netconfig::IncomingRequest, + reputation_changes: Vec, + ) -> std::result::Result { + let netconfig::IncomingRequest { payload, peer, pending_response } = raw; + let payload = match JustificationRequest::decode(&mut payload.as_ref()) { + Ok(payload) => payload, + Err(err) => { + let response = netconfig::OutgoingResponse { + result: Err(()), + reputation_changes, + sent_feedback: None, + }; + if let Err(_) = pending_response.send(response) { + return Err(Error::DecodingErrorNoReputationChange(peer, err)) + } + return Err(Error::DecodingError(peer, err)) + }, + }; + Ok(Self::new(peer, payload, pending_response)) + } +} + +/// Receiver for incoming requests. +/// +/// Takes care of decoding and handling of invalid encoded requests. +pub struct IncomingRequestReceiver { + raw: mpsc::Receiver, +} + +impl IncomingRequestReceiver { + /// Try to receive the next incoming request. + /// + /// Any received request will be decoded, on decoding errors the provided reputation changes + /// will be applied and an error will be reported. + pub async fn recv(&mut self, reputation_changes: F) -> Result> + where + B: Block, + F: FnOnce() -> Vec, + { + let req = match self.raw.next().await { + None => return Err(Error::RequestChannelExhausted), + Some(raw) => IncomingRequest::::try_from_raw(raw, reputation_changes())?, + }; + Ok(req) + } +} + +#[derive(Debug, thiserror::Error)] +pub enum Error { + #[error(transparent)] + Client(#[from] sp_blockchain::Error), + + /// Decoding failed, we were able to change the peer's reputation accordingly. + #[error("Decoding request failed for peer {0}.")] + DecodingError(PeerId, #[source] CodecError), + + /// Decoding failed, but sending reputation change failed. + #[error("Decoding request failed for peer {0}, and changing reputation failed.")] + DecodingErrorNoReputationChange(PeerId, #[source] CodecError), + + /// Incoming request stream exhausted. Should only happen on shutdown. + #[error("Incoming request channel got closed.")] + RequestChannelExhausted, + + #[error("Failed to send response.")] + SendResponse, +} + +/// General result based on above `Error`. +pub type Result = std::result::Result; From f68920e7ea6a1908710f27d7728d6aa3bd926b1b Mon Sep 17 00:00:00 2001 From: acatangiu Date: Fri, 26 Aug 2022 17:00:07 +0300 Subject: [PATCH 05/31] client/beefy: keep track of connected peers and their progress --- Cargo.lock | 1 + client/beefy/Cargo.toml | 1 + client/beefy/src/communication/gossip.rs | 20 +++--- client/beefy/src/communication/mod.rs | 1 + client/beefy/src/communication/peers.rs | 61 +++++++++++++++++ .../request_response/incoming_handler.rs | 4 +- .../src/communication/request_response/mod.rs | 2 +- client/beefy/src/lib.rs | 20 ++++-- client/beefy/src/worker.rs | 67 ++++++++++++++----- 9 files changed, 142 insertions(+), 35 deletions(-) create mode 100644 client/beefy/src/communication/peers.rs diff --git a/Cargo.lock b/Cargo.lock index 64e84fe4710af..34d10df90490b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -456,6 +456,7 @@ dependencies = [ "sc-finality-grandpa", "sc-keystore", "sc-network", + "sc-network-common", "sc-network-gossip", "sc-network-test", "sc-utils", diff --git a/client/beefy/Cargo.toml b/client/beefy/Cargo.toml index 5cc2952e26ba5..a634b91f2113c 100644 --- a/client/beefy/Cargo.toml +++ b/client/beefy/Cargo.toml @@ -27,6 +27,7 @@ sc-consensus = { version = "0.10.0-dev", path = "../consensus/common" } sc-finality-grandpa = { version = "0.10.0-dev", path = "../../client/finality-grandpa" } sc-keystore = { version = "4.0.0-dev", path = "../keystore" } sc-network = { version = "0.10.0-dev", path = "../network" } +sc-network-common = { version = "0.10.0-dev", path = "../network/common" } sc-network-gossip = { version = "0.10.0-dev", path = "../network-gossip" } sc-utils = { version = "4.0.0-dev", path = "../utils" } sp-api = { version = "4.0.0-dev", path = "../../primitives/api" } diff --git a/client/beefy/src/communication/gossip.rs b/client/beefy/src/communication/gossip.rs index 02d5efe9e0e58..6c41a2e48932a 100644 --- a/client/beefy/src/communication/gossip.rs +++ b/client/beefy/src/communication/gossip.rs @@ -16,7 +16,7 @@ // You should have received a copy of the GNU General Public License // along with this program. If not, see . -use std::{collections::BTreeMap, time::Duration}; +use std::{collections::BTreeMap, sync::Arc, time::Duration}; use sc_network::PeerId; use sc_network_gossip::{MessageIntent, ValidationResult, Validator, ValidatorContext}; @@ -28,13 +28,12 @@ use log::{debug, trace}; use parking_lot::{Mutex, RwLock}; use wasm_timer::Instant; +use crate::{communication::peers::KnownPeers, keystore::BeefyKeystore}; use beefy_primitives::{ crypto::{Public, Signature}, VoteMessage, }; -use crate::keystore::BeefyKeystore; - // Timeout for rebroadcasting messages. const REBROADCAST_AFTER: Duration = Duration::from_secs(60 * 5); @@ -103,17 +102,19 @@ where topic: B::Hash, known_votes: RwLock>, next_rebroadcast: Mutex, + known_peers: Arc>>, } impl GossipValidator where B: Block, { - pub fn new() -> GossipValidator { + pub fn new(known_peers: Arc>>) -> GossipValidator { GossipValidator { topic: topic::(), known_votes: RwLock::new(KnownVotes::new()), next_rebroadcast: Mutex::new(Instant::now() + REBROADCAST_AFTER), + known_peers, } } @@ -165,6 +166,7 @@ where if BeefyKeystore::verify(&msg.id, &msg.signature, &msg.commitment.encode()) { self.known_votes.write().add_known(&round, msg_hash); + self.known_peers.lock().note_vote_for(*sender, round); return ValidationResult::ProcessAndKeep(self.topic) } else { // TODO: report peer @@ -271,7 +273,7 @@ mod tests { #[test] fn note_and_drop_round_works() { - let gv = GossipValidator::::new(); + let gv = GossipValidator::::new(Arc::new(Mutex::new(KnownPeers::new()))); gv.note_round(1u64); @@ -298,7 +300,7 @@ mod tests { #[test] fn note_same_round_twice() { - let gv = GossipValidator::::new(); + let gv = GossipValidator::::new(Arc::new(Mutex::new(KnownPeers::new()))); gv.note_round(3u64); gv.note_round(7u64); @@ -355,7 +357,7 @@ mod tests { #[test] fn should_avoid_verifying_signatures_twice() { - let gv = GossipValidator::::new(); + let gv = GossipValidator::::new(Arc::new(Mutex::new(KnownPeers::new()))); let sender = sc_network::PeerId::random(); let mut context = TestContext; @@ -391,7 +393,7 @@ mod tests { #[test] fn messages_allowed_and_expired() { - let gv = GossipValidator::::new(); + let gv = GossipValidator::::new(Arc::new(Mutex::new(KnownPeers::new()))); let sender = sc_network::PeerId::random(); let topic = Default::default(); let intent = MessageIntent::Broadcast; @@ -434,7 +436,7 @@ mod tests { #[test] fn messages_rebroadcast() { - let gv = GossipValidator::::new(); + let gv = GossipValidator::::new(Arc::new(Mutex::new(KnownPeers::new()))); let sender = sc_network::PeerId::random(); let topic = Default::default(); diff --git a/client/beefy/src/communication/mod.rs b/client/beefy/src/communication/mod.rs index 0a497dcbb4735..350dbc67ae5ec 100644 --- a/client/beefy/src/communication/mod.rs +++ b/client/beefy/src/communication/mod.rs @@ -22,6 +22,7 @@ pub mod notification; pub mod request_response; pub(crate) mod gossip; +pub(crate) mod peers; pub(crate) mod beefy_protocol_name { /// BEEFY votes gossip protocol name suffix. diff --git a/client/beefy/src/communication/peers.rs b/client/beefy/src/communication/peers.rs new file mode 100644 index 0000000000000..411a61183e128 --- /dev/null +++ b/client/beefy/src/communication/peers.rs @@ -0,0 +1,61 @@ +// This file is part of Substrate. + +// Copyright (C) 2022 Parity Technologies (UK) Ltd. +// SPDX-License-Identifier: GPL-3.0-or-later WITH Classpath-exception-2.0 + +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. + +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License +// along with this program. If not, see . + +//! Logic for keeping track of BEEFY peers. + +use sc_network::PeerId; +use sp_runtime::traits::{Block, NumberFor, Zero}; +use std::collections::HashMap; + +struct PeerData { + last_voted_on: NumberFor, +} + +impl Default for PeerData { + fn default() -> Self { + PeerData { last_voted_on: Zero::zero() } + } +} + +/// Keep a simple map of connected peers +/// and the most recent voting round they participated in. +pub struct KnownPeers { + live: HashMap>, +} + +impl KnownPeers { + pub fn new() -> Self { + Self { live: HashMap::new() } + } + + /// Add new connected `peer`. + pub fn add_new(&mut self, peer: PeerId) { + self.live.entry(peer).or_default(); + } + + /// Note vote round number for `peer`. + pub fn note_vote_for(&mut self, peer: PeerId, round: NumberFor) { + let data = self.live.entry(peer).or_default(); + data.last_voted_on = round.max(data.last_voted_on); + } + + /// Remove connected `peer`. + pub fn remove(&mut self, peer: &PeerId) { + self.live.remove(peer); + } +} diff --git a/client/beefy/src/communication/request_response/incoming_handler.rs b/client/beefy/src/communication/request_response/incoming_handler.rs index 55c01972f73ea..122e7d17ad830 100644 --- a/client/beefy/src/communication/request_response/incoming_handler.rs +++ b/client/beefy/src/communication/request_response/incoming_handler.rs @@ -55,7 +55,7 @@ where async fn handle_request(&self, request: IncomingRequest) -> Result<(), Error> { // TODO: validate `request.begin` and change peer reputation for invalid requests. - let encoded_proof = self + let maybe_encoded_proof = self .client .justifications(&BlockId::Number(request.payload.begin)) .map_err(Error::Client)? @@ -67,7 +67,7 @@ where request .pending_response .send(netconfig::OutgoingResponse { - result: encoded_proof, + result: maybe_encoded_proof, reputation_changes: Vec::new(), sent_feedback: None, }) diff --git a/client/beefy/src/communication/request_response/mod.rs b/client/beefy/src/communication/request_response/mod.rs index 01005cd0a23f7..76955ed276bce 100644 --- a/client/beefy/src/communication/request_response/mod.rs +++ b/client/beefy/src/communication/request_response/mod.rs @@ -18,7 +18,7 @@ //! Request/response protocol for syncing BEEFY justifications. -pub(crate) mod incoming_handler; +pub mod incoming_handler; use futures::{ channel::{mpsc, oneshot}, diff --git a/client/beefy/src/lib.rs b/client/beefy/src/lib.rs index 7cd0a9bf8f3e6..c2801e83a5f31 100644 --- a/client/beefy/src/lib.rs +++ b/client/beefy/src/lib.rs @@ -17,6 +17,7 @@ // along with this program. If not, see . use beefy_primitives::{BeefyApi, MmrRootHash}; +use parking_lot::Mutex; use prometheus::Registry; use sc_client_api::{Backend, BlockchainEvents, Finalizer}; use sc_consensus::BlockImport; @@ -43,9 +44,12 @@ pub mod justification; mod tests; use crate::{ - communication::notification::{ - BeefyBestBlockSender, BeefyBestBlockStream, BeefyVersionedFinalityProofSender, - BeefyVersionedFinalityProofStream, + communication::{ + notification::{ + BeefyBestBlockSender, BeefyBestBlockStream, BeefyVersionedFinalityProofSender, + BeefyVersionedFinalityProofStream, + }, + peers::KnownPeers, }, import::BeefyBlockImport, }; @@ -197,10 +201,11 @@ where links, } = beefy_params; - let sync_oracle = network.clone(); - let gossip_validator = Arc::new(communication::gossip::GossipValidator::new()); + let known_peers = Arc::new(Mutex::new(KnownPeers::new())); + let gossip_validator = + Arc::new(communication::gossip::GossipValidator::new(known_peers.clone())); let gossip_engine = sc_network_gossip::GossipEngine::new( - network, + network.clone(), protocol_name, gossip_validator.clone(), None, @@ -224,8 +229,9 @@ where client, backend, runtime, - sync_oracle, + network, key_store: key_store.into(), + known_peers, gossip_engine, gossip_validator, links, diff --git a/client/beefy/src/worker.rs b/client/beefy/src/worker.rs index 5eda41ef9a1b9..37f5f85b22be6 100644 --- a/client/beefy/src/worker.rs +++ b/client/beefy/src/worker.rs @@ -26,8 +26,10 @@ use std::{ use codec::{Codec, Decode, Encode}; use futures::StreamExt; use log::{debug, error, info, log_enabled, trace, warn}; +use parking_lot::Mutex; use sc_client_api::{Backend, FinalityNotification}; +use sc_network_common::{protocol::event::Event as NetEvent, service::NetworkEventStream}; use sc_network_gossip::GossipEngine; use sp_api::{BlockId, ProvideRuntimeApi}; @@ -54,7 +56,7 @@ use crate::{ metric_inc, metric_set, metrics::Metrics, round::Rounds, - BeefyVoterLinks, Client, + BeefyVoterLinks, Client, KnownPeers, }; enum RoundAction { @@ -174,12 +176,13 @@ impl VoterOracle { } } -pub(crate) struct WorkerParams { +pub(crate) struct WorkerParams { pub client: Arc, pub backend: Arc, pub runtime: Arc, - pub sync_oracle: SO, + pub network: N, pub key_store: BeefyKeystore, + pub known_peers: Arc>>, pub gossip_engine: GossipEngine, pub gossip_validator: Arc>, pub links: BeefyVoterLinks, @@ -188,13 +191,16 @@ pub(crate) struct WorkerParams { } /// A BEEFY worker plays the BEEFY protocol -pub(crate) struct BeefyWorker { +pub(crate) struct BeefyWorker { // utilities client: Arc, backend: Arc, runtime: Arc, - sync_oracle: SO, + network: N, key_store: BeefyKeystore, + + // communication + known_peers: Arc>>, gossip_engine: GossipEngine, gossip_validator: Arc>, @@ -217,14 +223,14 @@ pub(crate) struct BeefyWorker { voting_oracle: VoterOracle, } -impl BeefyWorker +impl BeefyWorker where B: Block + Codec, BE: Backend, C: Client, R: ProvideRuntimeApi, R::Api: BeefyApi + MmrApi, - SO: SyncOracle + Send + Sync + Clone + 'static, + N: NetworkEventStream + SyncOracle + Send + Sync + Clone + 'static, { /// Return a new BEEFY worker instance. /// @@ -232,15 +238,16 @@ where /// BEEFY pallet has been deployed on-chain. /// /// The BEEFY pallet is needed in order to keep track of the BEEFY authority set. - pub(crate) fn new(worker_params: WorkerParams) -> Self { + pub(crate) fn new(worker_params: WorkerParams) -> Self { let WorkerParams { client, backend, runtime, key_store, - sync_oracle, + network, gossip_engine, gossip_validator, + known_peers, links, metrics, min_block_delta, @@ -254,7 +261,8 @@ where client: client.clone(), backend, runtime, - sync_oracle, + network, + known_peers, key_store, gossip_engine, gossip_validator, @@ -670,6 +678,7 @@ where info!(target: "beefy", "🥩 run BEEFY worker, best grandpa: #{:?}.", self.best_grandpa_block_header.number()); self.wait_for_runtime_pallet().await; + let mut network_events = self.network.event_stream("network-gossip").fuse(); let mut finality_notifications = self.client.finality_notification_stream().fuse(); let mut votes = Box::pin( self.gossip_engine @@ -696,6 +705,7 @@ where if let Some(notification) = notification { self.handle_finality_notification(¬ification); } else { + error!(target: "beefy", "🥩 Finality stream terminated, closing worker."); return; } }, @@ -709,6 +719,7 @@ where debug!(target: "beefy", "🥩 {}", err); } } else { + error!(target: "beefy", "🥩 Block import stream terminated, closing worker."); return; } }, @@ -719,17 +730,26 @@ where debug!(target: "beefy", "🥩 {}", err); } } else { + error!(target: "beefy", "🥩 Votes gossiping stream terminated, closing worker."); + return; + } + }, + net_event = network_events.next() => { + if let Some(net_event) = net_event { + self.handle_network_event(net_event); + } else { + error!(target: "beefy", "🥩 Network events stream terminated, closing worker."); return; } }, _ = gossip_engine => { - error!(target: "beefy", "🥩 Gossip engine has terminated."); + error!(target: "beefy", "🥩 Gossip engine has terminated, closing worker."); return; } } // Don't bother acting on 'state' changes during major sync. - if !self.sync_oracle.is_major_syncing() { + if !self.network.is_major_syncing() { // Handle pending justifications and/or votes for now GRANDPA finalized blocks. if let Err(err) = self.try_pending_justif_and_votes() { debug!(target: "beefy", "🥩 {}", err); @@ -742,6 +762,20 @@ where } } } + + /// Update known peers based on network events. + fn handle_network_event(&mut self, event: NetEvent) { + match event { + NetEvent::SyncConnected { remote } => { + self.known_peers.lock().add_new(remote); + }, + NetEvent::SyncDisconnected { remote } => { + self.known_peers.lock().remove(&remote); + }, + // We don't care about other events. + _ => (), + } + } } /// Extract the MMR root hash from a digest in the given header, if it exists. @@ -880,21 +914,22 @@ pub(crate) mod tests { let api = Arc::new(TestApi {}); let network = peer.network_service().clone(); - let sync_oracle = network.clone(); - let gossip_validator = Arc::new(crate::communication::gossip::GossipValidator::new()); + let known_peers = Arc::new(Mutex::new(KnownPeers::new())); + let gossip_validator = Arc::new(GossipValidator::new(known_peers.clone())); let gossip_engine = - GossipEngine::new(network, BEEFY_PROTOCOL_NAME, gossip_validator.clone(), None); + GossipEngine::new(network.clone(), BEEFY_PROTOCOL_NAME, gossip_validator.clone(), None); let worker_params = crate::worker::WorkerParams { client: peer.client().as_client(), backend: peer.client().as_backend(), runtime: api, key_store: Some(keystore).into(), + known_peers, links, gossip_engine, gossip_validator, min_block_delta, metrics: None, - sync_oracle, + network, }; BeefyWorker::<_, _, _, _, _>::new(worker_params) } From 5fc1bb7a998c8739ffd4553546c712445bbad469 Mon Sep 17 00:00:00 2001 From: acatangiu Date: Fri, 26 Aug 2022 18:57:08 +0300 Subject: [PATCH 06/31] client/beefy: add logic for generating Justif requests --- .../src/communication/request_response/mod.rs | 1 + .../request_response/outgoing_request.rs | 144 ++++++++++++++++++ client/beefy/src/lib.rs | 5 +- client/beefy/src/worker.rs | 35 ++++- 4 files changed, 178 insertions(+), 7 deletions(-) create mode 100644 client/beefy/src/communication/request_response/outgoing_request.rs diff --git a/client/beefy/src/communication/request_response/mod.rs b/client/beefy/src/communication/request_response/mod.rs index 76955ed276bce..ce232b7f0a9b5 100644 --- a/client/beefy/src/communication/request_response/mod.rs +++ b/client/beefy/src/communication/request_response/mod.rs @@ -19,6 +19,7 @@ //! Request/response protocol for syncing BEEFY justifications. pub mod incoming_handler; +pub(crate) mod outgoing_request; use futures::{ channel::{mpsc, oneshot}, diff --git a/client/beefy/src/communication/request_response/outgoing_request.rs b/client/beefy/src/communication/request_response/outgoing_request.rs new file mode 100644 index 0000000000000..4f5accbde678e --- /dev/null +++ b/client/beefy/src/communication/request_response/outgoing_request.rs @@ -0,0 +1,144 @@ +// This file is part of Substrate. + +// Copyright (C) 2022 Parity Technologies (UK) Ltd. +// SPDX-License-Identifier: GPL-3.0-or-later WITH Classpath-exception-2.0 + +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. + +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License +// along with this program. If not, see . + +//! Generating request logic for request/response protocol for syncing BEEFY justifications. + +use codec::{Decode, Encode}; +use futures::{channel::oneshot, stream::FusedStream, FutureExt, Stream}; +use sc_network::PeerId; +use sc_network_common::{ + request_responses::{IfDisconnected, RequestFailure}, + service::NetworkRequest, +}; +use sp_runtime::traits::{Block, NumberFor}; +use std::{ + marker::PhantomData, + pin::Pin, + task::{Context, Poll, Waker}, +}; + +use crate::{ + communication::request_response::JustificationRequest, + justification::BeefyVersionedFinalityProof, +}; + +/// Used to receive a response from the network. +type ResponseReceiver = oneshot::Receiver, RequestFailure>>; + +enum State { + Init, + Idle(Waker), + AwaitingResponse(ResponseReceiver), +} + +pub struct OnDemandJustififactionsEngine { + network: N, + protocol_name: std::borrow::Cow<'static, str>, + state: State, + _phantom: PhantomData, +} + +impl OnDemandJustififactionsEngine +where + B: Block, + N: NetworkRequest, +{ + pub fn new(network: N, protocol_name: std::borrow::Cow<'static, str>) -> Self { + Self { network, protocol_name, state: State::Init, _phantom: PhantomData } + } + + pub fn fire_request_for(&mut self, block: NumberFor) { + // TODO: do peer selection based on `known_peers`. + let peer = PeerId::random(); + + let payload = JustificationRequest:: { begin: block }.encode(); + + let (tx, rx) = oneshot::channel(); + + self.network.start_request( + peer, + self.protocol_name.clone(), + payload, + tx, + IfDisconnected::ImmediateError, + ); + + self.state = State::AwaitingResponse(rx); + } +} + +impl Stream for OnDemandJustififactionsEngine +where + B: Block, + N: NetworkRequest, +{ + type Item = BeefyVersionedFinalityProof; + + fn poll_next(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll> { + // TODO: I don't think we need this waker, should remove. + let waker = cx.waker().clone(); + + let response_receiver = match &mut self.state { + // If there's nothing to do, + State::Init | State::Idle(_) => { + self.state = State::Idle(waker); + // do nothing. + return Poll::Pending + }, + State::AwaitingResponse(receiver) => receiver, + }; + + match response_receiver.poll_unpin(cx) { + Poll::Ready(Ok(Ok(encoded))) => { + self.state = State::Idle(waker); + + match >::decode(&mut &*encoded) { + // TODO: Verify this proof is valid. + Ok(proof) => return Poll::Ready(Some(proof)), + Err(_) => { + // TODO: decode error, try another peer. + }, + } + }, + Poll::Ready(Err(_)) | Poll::Ready(Ok(Err(_))) => { + self.state = State::Idle(waker); + // TODO: this peer closed connection or couldn't/refused to answer, try another. + }, + Poll::Pending => (), + } + + Poll::Pending + } +} + +impl FusedStream for OnDemandJustififactionsEngine +where + B: Block, + N: NetworkRequest, +{ + fn is_terminated(&self) -> bool { + false + } +} + +impl Unpin for OnDemandJustififactionsEngine +where + B: Block, + N: NetworkRequest, +{ +} diff --git a/client/beefy/src/lib.rs b/client/beefy/src/lib.rs index c2801e83a5f31..6a2089fd99f60 100644 --- a/client/beefy/src/lib.rs +++ b/client/beefy/src/lib.rs @@ -57,6 +57,7 @@ use crate::{ pub use communication::beefy_protocol_name::{ gossip_protocol_name, justifications_protocol_name as justifs_protocol_name, }; +use sc_network_common::service::NetworkRequest; /// A convenience BEEFY client trait that defines all the type bounds a BEEFY client /// has to satisfy. Ideally that should actually be a trait alias. Unfortunately as @@ -155,7 +156,7 @@ where C: Client, R: ProvideRuntimeApi, R::Api: BeefyApi + MmrApi, - N: GossipNetwork + Clone + SyncOracle + Send + Sync + 'static, + N: GossipNetwork + NetworkRequest + Clone + SyncOracle + Send + Sync + 'static, { /// BEEFY client pub client: Arc, @@ -187,7 +188,7 @@ where C: Client, R: ProvideRuntimeApi, R::Api: BeefyApi + MmrApi, - N: GossipNetwork + Clone + SyncOracle + Send + Sync + 'static, + N: GossipNetwork + NetworkRequest + Clone + SyncOracle + Send + Sync + 'static, { let BeefyParams { client, diff --git a/client/beefy/src/worker.rs b/client/beefy/src/worker.rs index 37f5f85b22be6..ed541173bcc9a 100644 --- a/client/beefy/src/worker.rs +++ b/client/beefy/src/worker.rs @@ -29,7 +29,10 @@ use log::{debug, error, info, log_enabled, trace, warn}; use parking_lot::Mutex; use sc_client_api::{Backend, FinalityNotification}; -use sc_network_common::{protocol::event::Event as NetEvent, service::NetworkEventStream}; +use sc_network_common::{ + protocol::event::Event as NetEvent, + service::{NetworkEventStream, NetworkRequest}, +}; use sc_network_gossip::GossipEngine; use sp_api::{BlockId, ProvideRuntimeApi}; @@ -49,7 +52,10 @@ use beefy_primitives::{ }; use crate::{ - communication::gossip::{topic, GossipValidator}, + communication::{ + gossip::{topic, GossipValidator}, + request_response::outgoing_request::OnDemandJustififactionsEngine, + }, error::Error, justification::BeefyVersionedFinalityProof, keystore::BeefyKeystore, @@ -203,6 +209,7 @@ pub(crate) struct BeefyWorker { known_peers: Arc>>, gossip_engine: GossipEngine, gossip_validator: Arc>, + on_demand_justifications: OnDemandJustififactionsEngine, // channels /// Links between the block importer, the background voter and the RPC layer. @@ -230,7 +237,7 @@ where C: Client, R: ProvideRuntimeApi, R::Api: BeefyApi + MmrApi, - N: NetworkEventStream + SyncOracle + Send + Sync + Clone + 'static, + N: NetworkEventStream + NetworkRequest + SyncOracle + Send + Sync + Clone + 'static, { /// Return a new BEEFY worker instance. /// @@ -257,6 +264,12 @@ where .expect_header(BlockId::number(client.info().finalized_number)) .expect("latest block always has header available; qed."); + let on_demand_justifications = OnDemandJustififactionsEngine::new( + network.clone(), + // FIXME: use right protocol name. + "TODO: FIXME: proto-name-here".into(), + ); + BeefyWorker { client: client.clone(), backend, @@ -266,6 +279,7 @@ where key_store, gossip_engine, gossip_validator, + on_demand_justifications, links, metrics, best_grandpa_block_header: last_finalized_header, @@ -358,6 +372,7 @@ where self.init_session_at(new_validator_set, *header.number()); // TODO: when adding SYNC protocol, fire up a request for justification for this // mandatory block here. + self.on_demand_justifications.fire_request_for(*header.number()); } } } @@ -709,8 +724,6 @@ where return; } }, - // TODO: when adding SYNC protocol, join the on-demand justifications stream to - // this one, and handle them both here. justif = block_import_justif.next() => { if let Some(justif) = justif { // Block import justifications have already been verified to be valid @@ -723,6 +736,18 @@ where return; } }, + // TODO: join this stream's branch with the one above; how? .. ¯\_(ツ)_/¯ + justif = self.on_demand_justifications.next() => { + if let Some(justif) = justif { + // TODO: make sure proofs are verified before consuming. + if let Err(err) = self.triage_incoming_justif(justif) { + debug!(target: "beefy", "🥩 {}", err); + } + } else { + error!(target: "beefy", "🥩 On demand justifications stream terminated, closing worker."); + return; + } + }, vote = votes.next() => { if let Some(vote) = vote { // Votes have already been verified to be valid by the gossip validator. From 9f05d3ad9d82898348e6f0b60e20f8210f113853 Mon Sep 17 00:00:00 2001 From: acatangiu Date: Fri, 26 Aug 2022 19:54:08 +0300 Subject: [PATCH 07/31] client/beefy: cancel outdated on-demand justification requests --- .../request_response/outgoing_request.rs | 29 +++++++++++++------ client/beefy/src/worker.rs | 2 ++ 2 files changed, 22 insertions(+), 9 deletions(-) diff --git a/client/beefy/src/communication/request_response/outgoing_request.rs b/client/beefy/src/communication/request_response/outgoing_request.rs index 4f5accbde678e..ac79801b09510 100644 --- a/client/beefy/src/communication/request_response/outgoing_request.rs +++ b/client/beefy/src/communication/request_response/outgoing_request.rs @@ -27,7 +27,6 @@ use sc_network_common::{ }; use sp_runtime::traits::{Block, NumberFor}; use std::{ - marker::PhantomData, pin::Pin, task::{Context, Poll, Waker}, }; @@ -40,17 +39,16 @@ use crate::{ /// Used to receive a response from the network. type ResponseReceiver = oneshot::Receiver, RequestFailure>>; -enum State { +enum State { Init, Idle(Waker), - AwaitingResponse(ResponseReceiver), + AwaitingResponse(NumberFor, ResponseReceiver), } -pub struct OnDemandJustififactionsEngine { +pub struct OnDemandJustififactionsEngine { network: N, protocol_name: std::borrow::Cow<'static, str>, - state: State, - _phantom: PhantomData, + state: State, } impl OnDemandJustififactionsEngine @@ -59,7 +57,7 @@ where N: NetworkRequest, { pub fn new(network: N, protocol_name: std::borrow::Cow<'static, str>) -> Self { - Self { network, protocol_name, state: State::Init, _phantom: PhantomData } + Self { network, protocol_name, state: State::Init } } pub fn fire_request_for(&mut self, block: NumberFor) { @@ -78,7 +76,20 @@ where IfDisconnected::ImmediateError, ); - self.state = State::AwaitingResponse(rx); + self.state = State::AwaitingResponse(block, rx); + } + + /// Cancel any pending request for block numbers smaller or equal to `latest_block`. + pub fn cancel_requests_older_than(&mut self, latest_block: NumberFor) { + match &self.state { + State::Init | State::Idle(_) => (), + State::AwaitingResponse(block, _) if *block <= latest_block => { + // TODO: this should be `State::Idle` but I need to figure out if I need that + // `Waker` or not. + self.state = State::Init; + }, + _ => (), + } } } @@ -100,7 +111,7 @@ where // do nothing. return Poll::Pending }, - State::AwaitingResponse(receiver) => receiver, + State::AwaitingResponse(_block, receiver) => receiver, }; match response_receiver.poll_unpin(cx) { diff --git a/client/beefy/src/worker.rs b/client/beefy/src/worker.rs index ed541173bcc9a..643609c85df3a 100644 --- a/client/beefy/src/worker.rs +++ b/client/beefy/src/worker.rs @@ -482,6 +482,8 @@ where self.best_beefy_block = Some(block_num); metric_set!(self, beefy_best_block, block_num); + self.on_demand_justifications.cancel_requests_older_than(block_num); + self.client.hash(block_num).ok().flatten().map(|hash| { self.links .to_rpc_best_block_sender From ac5c3a5f79a9ca264528324eddd42b0dfc6f001d Mon Sep 17 00:00:00 2001 From: Adrian Catangiu Date: Fri, 9 Sep 2022 22:00:34 +0300 Subject: [PATCH 08/31] try Andre's suggestion for JustificationEngine --- .../request_response/outgoing_request.rs | 191 +++++++++++------- client/beefy/src/worker.rs | 26 +-- 2 files changed, 124 insertions(+), 93 deletions(-) diff --git a/client/beefy/src/communication/request_response/outgoing_request.rs b/client/beefy/src/communication/request_response/outgoing_request.rs index ac79801b09510..1e1c6366b8401 100644 --- a/client/beefy/src/communication/request_response/outgoing_request.rs +++ b/client/beefy/src/communication/request_response/outgoing_request.rs @@ -18,49 +18,55 @@ //! Generating request logic for request/response protocol for syncing BEEFY justifications. -use codec::{Decode, Encode}; -use futures::{channel::oneshot, stream::FusedStream, FutureExt, Stream}; +use beefy_primitives::BeefyApi; +use codec::Encode; +use futures::channel::oneshot; use sc_network::PeerId; use sc_network_common::{ request_responses::{IfDisconnected, RequestFailure}, service::NetworkRequest, }; -use sp_runtime::traits::{Block, NumberFor}; -use std::{ - pin::Pin, - task::{Context, Poll, Waker}, +use sp_api::ProvideRuntimeApi; +use sp_runtime::{ + generic::BlockId, + traits::{Block, NumberFor}, }; +use std::sync::Arc; use crate::{ communication::request_response::JustificationRequest, - justification::BeefyVersionedFinalityProof, + justification::{decode_and_verify_finality_proof, BeefyVersionedFinalityProof}, + ConsensusError, }; /// Used to receive a response from the network. type ResponseReceiver = oneshot::Receiver, RequestFailure>>; enum State { - Init, - Idle(Waker), + Idle, AwaitingResponse(NumberFor, ResponseReceiver), } -pub struct OnDemandJustififactionsEngine { +pub struct OnDemandJustificationsEngine { network: N, + runtime: Arc, protocol_name: std::borrow::Cow<'static, str>, state: State, } -impl OnDemandJustififactionsEngine +impl OnDemandJustificationsEngine where B: Block, N: NetworkRequest, + R: ProvideRuntimeApi, + R::Api: BeefyApi, { - pub fn new(network: N, protocol_name: std::borrow::Cow<'static, str>) -> Self { - Self { network, protocol_name, state: State::Init } + pub fn new(network: N, runtime: Arc, protocol_name: std::borrow::Cow<'static, str>) -> Self { + Self { network, runtime, protocol_name, state: State::Idle } } - pub fn fire_request_for(&mut self, block: NumberFor) { + /// Start requesting justification for `block` number. + pub fn request(&mut self, block: NumberFor) { // TODO: do peer selection based on `known_peers`. let peer = PeerId::random(); @@ -79,77 +85,106 @@ where self.state = State::AwaitingResponse(block, rx); } + /// FIXME (Andre): this to replace 'impl Stream' + pub async fn next(&mut self) -> BeefyVersionedFinalityProof { + let (number, rx) = match std::mem::replace(&mut self.state, State::Idle) { + State::AwaitingResponse(block, rx) => (block, rx), + _ => unimplemented!(), // return future that never finishes. + }; + + let result = match rx.await { + Ok(result) => result, + Err(_) => unimplemented!(), + }; + + let encoded = match result { + Ok(encoded) => encoded, + Err(_) => unimplemented!(), + }; + + let block_id = BlockId::number(number); + let validator_set = self + .runtime + .runtime_api() + .validator_set(&block_id) + .map_err(|e| ConsensusError::ClientImport(e.to_string())) + .unwrap_or_else(|_| unimplemented!()) + .ok_or_else(|| ConsensusError::ClientImport("Unknown validator set".to_string())) + .unwrap_or_else(|_| unimplemented!()); + + let proof = decode_and_verify_finality_proof::(&encoded[..], number, &validator_set); + // FIXME: handle error. + proof.unwrap() + } + /// Cancel any pending request for block numbers smaller or equal to `latest_block`. pub fn cancel_requests_older_than(&mut self, latest_block: NumberFor) { match &self.state { - State::Init | State::Idle(_) => (), State::AwaitingResponse(block, _) if *block <= latest_block => { - // TODO: this should be `State::Idle` but I need to figure out if I need that - // `Waker` or not. - self.state = State::Init; + self.state = State::Idle; }, _ => (), } } } -impl Stream for OnDemandJustififactionsEngine -where - B: Block, - N: NetworkRequest, -{ - type Item = BeefyVersionedFinalityProof; - - fn poll_next(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll> { - // TODO: I don't think we need this waker, should remove. - let waker = cx.waker().clone(); - - let response_receiver = match &mut self.state { - // If there's nothing to do, - State::Init | State::Idle(_) => { - self.state = State::Idle(waker); - // do nothing. - return Poll::Pending - }, - State::AwaitingResponse(_block, receiver) => receiver, - }; - - match response_receiver.poll_unpin(cx) { - Poll::Ready(Ok(Ok(encoded))) => { - self.state = State::Idle(waker); - - match >::decode(&mut &*encoded) { - // TODO: Verify this proof is valid. - Ok(proof) => return Poll::Ready(Some(proof)), - Err(_) => { - // TODO: decode error, try another peer. - }, - } - }, - Poll::Ready(Err(_)) | Poll::Ready(Ok(Err(_))) => { - self.state = State::Idle(waker); - // TODO: this peer closed connection or couldn't/refused to answer, try another. - }, - Poll::Pending => (), - } - - Poll::Pending - } -} - -impl FusedStream for OnDemandJustififactionsEngine -where - B: Block, - N: NetworkRequest, -{ - fn is_terminated(&self) -> bool { - false - } -} - -impl Unpin for OnDemandJustififactionsEngine -where - B: Block, - N: NetworkRequest, -{ -} +// impl Stream for OnDemandJustificationsEngine +// where +// B: Block, +// N: NetworkRequest, +// { +// type Item = BeefyVersionedFinalityProof; +// +// fn poll_next(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll> { +// // TODO: I don't think we need this waker, should remove. +// let waker = cx.waker().clone(); +// +// let response_receiver = match &mut self.state { +// // If there's nothing to do, +// State::Init | State::Idle(_) => { +// self.state = State::Idle(waker); +// // do nothing. +// return Poll::Pending +// }, +// State::AwaitingResponse(_block, receiver) => receiver, +// }; +// +// match response_receiver.poll_unpin(cx) { +// Poll::Ready(Ok(Ok(encoded))) => { +// self.state = State::Idle(waker); +// +// match >::decode(&mut &*encoded) { +// // TODO: Verify this proof is valid. +// Ok(proof) => return Poll::Ready(Some(proof)), +// Err(_) => { +// // TODO: decode error, try another peer. +// }, +// } +// }, +// Poll::Ready(Err(_)) | Poll::Ready(Ok(Err(_))) => { +// self.state = State::Idle(waker); +// // TODO: this peer closed connection or couldn't/refused to answer, try another. +// }, +// Poll::Pending => (), +// } +// +// Poll::Pending +// } +// } +// +// impl FusedStream for OnDemandJustificationsEngine +// where +// B: Block, +// N: NetworkRequest, +// { +// fn is_terminated(&self) -> bool { +// false +// } +// } +// +// impl Unpin for OnDemandJustificationsEngine +// where +// B: Block, +// N: NetworkRequest, +// { +// } diff --git a/client/beefy/src/worker.rs b/client/beefy/src/worker.rs index 643609c85df3a..972eb9c598bcf 100644 --- a/client/beefy/src/worker.rs +++ b/client/beefy/src/worker.rs @@ -24,7 +24,7 @@ use std::{ }; use codec::{Codec, Decode, Encode}; -use futures::StreamExt; +use futures::{FutureExt, StreamExt}; use log::{debug, error, info, log_enabled, trace, warn}; use parking_lot::Mutex; @@ -54,7 +54,7 @@ use beefy_primitives::{ use crate::{ communication::{ gossip::{topic, GossipValidator}, - request_response::outgoing_request::OnDemandJustififactionsEngine, + request_response::outgoing_request::OnDemandJustificationsEngine, }, error::Error, justification::BeefyVersionedFinalityProof, @@ -209,7 +209,7 @@ pub(crate) struct BeefyWorker { known_peers: Arc>>, gossip_engine: GossipEngine, gossip_validator: Arc>, - on_demand_justifications: OnDemandJustififactionsEngine, + on_demand_justifications: OnDemandJustificationsEngine, // channels /// Links between the block importer, the background voter and the RPC layer. @@ -264,8 +264,9 @@ where .expect_header(BlockId::number(client.info().finalized_number)) .expect("latest block always has header available; qed."); - let on_demand_justifications = OnDemandJustififactionsEngine::new( + let on_demand_justifications = OnDemandJustificationsEngine::new( network.clone(), + runtime.clone(), // FIXME: use right protocol name. "TODO: FIXME: proto-name-here".into(), ); @@ -370,9 +371,9 @@ where // Check for and enqueue potential new session. if let Some(new_validator_set) = find_authorities_change::(header) { self.init_session_at(new_validator_set, *header.number()); - // TODO: when adding SYNC protocol, fire up a request for justification for this + // TODO: when adding SYNC protocol, fire up request for justification for this // mandatory block here. - self.on_demand_justifications.fire_request_for(*header.number()); + self.on_demand_justifications.request(*header.number()); } } } @@ -739,15 +740,10 @@ where } }, // TODO: join this stream's branch with the one above; how? .. ¯\_(ツ)_/¯ - justif = self.on_demand_justifications.next() => { - if let Some(justif) = justif { - // TODO: make sure proofs are verified before consuming. - if let Err(err) = self.triage_incoming_justif(justif) { - debug!(target: "beefy", "🥩 {}", err); - } - } else { - error!(target: "beefy", "🥩 On demand justifications stream terminated, closing worker."); - return; + justif = self.on_demand_justifications.next().fuse() => { + // TODO: make sure proofs are verified before consuming. + if let Err(err) = self.triage_incoming_justif(justif) { + debug!(target: "beefy", "🥩 {}", err); } }, vote = votes.next() => { From e9ebea75d0666f509d3efcd908117b347f99bdff Mon Sep 17 00:00:00 2001 From: Adrian Catangiu Date: Fri, 9 Sep 2022 22:01:09 +0300 Subject: [PATCH 09/31] justif engine add justifs validation --- .../src/communication/request_response/mod.rs | 9 ++++ .../request_response/outgoing_request.rs | 45 +++++++++++++------ 2 files changed, 41 insertions(+), 13 deletions(-) diff --git a/client/beefy/src/communication/request_response/mod.rs b/client/beefy/src/communication/request_response/mod.rs index ce232b7f0a9b5..3c6c6dbb024fd 100644 --- a/client/beefy/src/communication/request_response/mod.rs +++ b/client/beefy/src/communication/request_response/mod.rs @@ -153,6 +153,12 @@ pub enum Error { #[error(transparent)] Client(#[from] sp_blockchain::Error), + #[error(transparent)] + RuntimeApi(#[from] sp_api::ApiError), + + #[error("BEEFY pallet not available.")] + Pallet, + /// Decoding failed, we were able to change the peer's reputation accordingly. #[error("Decoding request failed for peer {0}.")] DecodingError(PeerId, #[source] CodecError), @@ -167,6 +173,9 @@ pub enum Error { #[error("Failed to send response.")] SendResponse, + + #[error("Failed to send response.")] + TodoError, } /// General result based on above `Error`. diff --git a/client/beefy/src/communication/request_response/outgoing_request.rs b/client/beefy/src/communication/request_response/outgoing_request.rs index 1e1c6366b8401..f2ce8002f07cf 100644 --- a/client/beefy/src/communication/request_response/outgoing_request.rs +++ b/client/beefy/src/communication/request_response/outgoing_request.rs @@ -31,16 +31,18 @@ use sp_runtime::{ generic::BlockId, traits::{Block, NumberFor}, }; -use std::sync::Arc; +use std::{result::Result, sync::Arc}; +use log::debug; use crate::{ - communication::request_response::JustificationRequest, + communication::request_response::{Error, JustificationRequest}, justification::{decode_and_verify_finality_proof, BeefyVersionedFinalityProof}, - ConsensusError, }; +/// Response type received from network. +type Response = Result, RequestFailure>; /// Used to receive a response from the network. -type ResponseReceiver = oneshot::Receiver, RequestFailure>>; +type ResponseReceiver = oneshot::Receiver; enum State { Idle, @@ -92,12 +94,32 @@ where _ => unimplemented!(), // return future that never finishes. }; - let result = match rx.await { - Ok(result) => result, + let resp = match rx.await { + Ok(resp) => resp, Err(_) => unimplemented!(), }; - let encoded = match result { + match self.process_response(resp, number) { + Ok(proof) => proof, + Err(e) => { + debug!( + target: "beefy", + "🥩 on demand justification (block {:?}) response error: {:?}", + number, e + ); + self.request(number); + + unimplemented!(); // return future that never finishes. + } + } + } + + fn process_response( + &mut self, + resp: Response, + number: NumberFor, + ) -> Result, Error> { + let encoded = match resp { Ok(encoded) => encoded, Err(_) => unimplemented!(), }; @@ -107,14 +129,11 @@ where .runtime .runtime_api() .validator_set(&block_id) - .map_err(|e| ConsensusError::ClientImport(e.to_string())) - .unwrap_or_else(|_| unimplemented!()) - .ok_or_else(|| ConsensusError::ClientImport("Unknown validator set".to_string())) - .unwrap_or_else(|_| unimplemented!()); + .map_err(Error::RuntimeApi)? + .ok_or_else(|| Error::Pallet)?; let proof = decode_and_verify_finality_proof::(&encoded[..], number, &validator_set); - // FIXME: handle error. - proof.unwrap() + proof.map_err(|_| Error::TodoError) } /// Cancel any pending request for block numbers smaller or equal to `latest_block`. From 4ab5ab030c8a196f3dc9ca983e8cd7c72f0c5335 Mon Sep 17 00:00:00 2001 From: acatangiu Date: Mon, 12 Sep 2022 21:33:02 +0300 Subject: [PATCH 10/31] client/beefy: impl OnDemandJustificationsEngine async next() --- .../request_response/outgoing_request.rs | 144 +++++++----------- client/beefy/src/worker.rs | 7 +- 2 files changed, 63 insertions(+), 88 deletions(-) diff --git a/client/beefy/src/communication/request_response/outgoing_request.rs b/client/beefy/src/communication/request_response/outgoing_request.rs index 7b3d73465f92c..149fc3cb0d758 100644 --- a/client/beefy/src/communication/request_response/outgoing_request.rs +++ b/client/beefy/src/communication/request_response/outgoing_request.rs @@ -20,8 +20,12 @@ use beefy_primitives::BeefyApi; use codec::Encode; -use futures::channel::oneshot; -use log::debug; +use futures::{ + channel::{oneshot, oneshot::Canceled}, + future::Either, + stream::{FuturesUnordered, StreamExt}, +}; +use log::{debug, error}; use sc_network::{PeerId, ProtocolName}; use sc_network_common::{ request_responses::{IfDisconnected, RequestFailure}, @@ -46,7 +50,7 @@ type ResponseReceiver = oneshot::Receiver; enum State { Idle, - AwaitingResponse(NumberFor, ResponseReceiver), + AwaitingResponse(NumberFor), } pub struct OnDemandJustificationsEngine { @@ -54,6 +58,9 @@ pub struct OnDemandJustificationsEngine { runtime: Arc, protocol_name: ProtocolName, state: State, + engine: FuturesUnordered< + Either>, ResponseReceiver>, + >, } impl OnDemandJustificationsEngine @@ -64,7 +71,11 @@ where R::Api: BeefyApi, { pub fn new(network: N, runtime: Arc, protocol_name: ProtocolName) -> Self { - Self { network, runtime, protocol_name, state: State::Idle } + let engine = FuturesUnordered::new(); + let pending = Either::Left(std::future::pending::<_>()); + engine.push(pending); + + Self { network, runtime, protocol_name, state: State::Idle, engine } } /// Start requesting justification for `block` number. @@ -84,46 +95,69 @@ where IfDisconnected::ImmediateError, ); - self.state = State::AwaitingResponse(block, rx); + self.engine.push(Either::Right(rx)); + self.state = State::AwaitingResponse(block); } - /// FIXME (Andre): this to replace 'impl Stream' - pub async fn next(&mut self) -> BeefyVersionedFinalityProof { - let (number, rx) = match std::mem::replace(&mut self.state, State::Idle) { - State::AwaitingResponse(block, rx) => (block, rx), - _ => unimplemented!(), // return future that never finishes. + pub async fn next(&mut self) -> Option> { + // The `engine` contains at the very least a `Pending` future (future that never finishes). + // This "blocks" until: + // - we have a [ResponseReceiver] added to the engine, and + // - we also get a [Response] on it. + let resp = self.engine.next().await; + + let resp = resp.expect("Engine will never end because of inner `Pending` future, qed."); + let number = match self.state { + State::AwaitingResponse(block) => block, + // Programming error to have active [ResponseReceiver]s in the engine with no pending + // requests. + State::Idle => unreachable!(), }; - let resp = match rx.await { + let resp = match resp { Ok(resp) => resp, - Err(_) => unimplemented!(), + Err(e) => { + debug!( + target: "beefy", + "🥩 on demand justification (block {:?}) peer hung up: {:?}", + number, e + ); + self.request(number); + return None + }, + }; + + let encoded = match resp { + Ok(encoded) => encoded, + Err(e) => { + error!( + target: "beefy", + "🥩 on demand justification (block {:?}) peer responded with error: {:?}", + number, e + ); + return None + }, }; - match self.process_response(resp, number) { - Ok(proof) => proof, + match self.process_response(encoded, number) { + Ok(proof) => Some(proof), Err(e) => { debug!( target: "beefy", - "🥩 on demand justification (block {:?}) response error: {:?}", + "🥩 on demand justification (block {:?}) invalid proof: {:?}", number, e ); self.request(number); - - unimplemented!(); // return future that never finishes. + None }, } } fn process_response( &mut self, - resp: Response, + encoded: Vec, number: NumberFor, ) -> Result, Error> { - let encoded = match resp { - Ok(encoded) => encoded, - Err(_) => unimplemented!(), - }; - let block_id = BlockId::number(number); let validator_set = self .runtime @@ -139,71 +173,11 @@ where /// Cancel any pending request for block numbers smaller or equal to `latest_block`. pub fn cancel_requests_older_than(&mut self, latest_block: NumberFor) { match &self.state { - State::AwaitingResponse(block, _) if *block <= latest_block => { + State::AwaitingResponse(block) if *block <= latest_block => { self.state = State::Idle; + // TODO: reset `self.engine` }, _ => (), } } } - -// impl Stream for OnDemandJustificationsEngine -// where -// B: Block, -// N: NetworkRequest, -// { -// type Item = BeefyVersionedFinalityProof; -// -// fn poll_next(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll> { -// // TODO: I don't think we need this waker, should remove. -// let waker = cx.waker().clone(); -// -// let response_receiver = match &mut self.state { -// // If there's nothing to do, -// State::Init | State::Idle(_) => { -// self.state = State::Idle(waker); -// // do nothing. -// return Poll::Pending -// }, -// State::AwaitingResponse(_block, receiver) => receiver, -// }; -// -// match response_receiver.poll_unpin(cx) { -// Poll::Ready(Ok(Ok(encoded))) => { -// self.state = State::Idle(waker); -// -// match >::decode(&mut &*encoded) { -// // TODO: Verify this proof is valid. -// Ok(proof) => return Poll::Ready(Some(proof)), -// Err(_) => { -// // TODO: decode error, try another peer. -// }, -// } -// }, -// Poll::Ready(Err(_)) | Poll::Ready(Ok(Err(_))) => { -// self.state = State::Idle(waker); -// // TODO: this peer closed connection or couldn't/refused to answer, try another. -// }, -// Poll::Pending => (), -// } -// -// Poll::Pending -// } -// } -// -// impl FusedStream for OnDemandJustificationsEngine -// where -// B: Block, -// N: NetworkRequest, -// { -// fn is_terminated(&self) -> bool { -// false -// } -// } -// -// impl Unpin for OnDemandJustificationsEngine -// where -// B: Block, -// N: NetworkRequest, -// { -// } diff --git a/client/beefy/src/worker.rs b/client/beefy/src/worker.rs index b657746430bd7..189878cf497e1 100644 --- a/client/beefy/src/worker.rs +++ b/client/beefy/src/worker.rs @@ -836,9 +836,10 @@ where }, // TODO: join this stream's branch with the one above; how? .. ¯\_(ツ)_/¯ justif = self.on_demand_justifications.next().fuse() => { - // TODO: make sure proofs are verified before consuming. - if let Err(err) = self.triage_incoming_justif(justif) { - debug!(target: "beefy", "🥩 {}", err); + if let Some(justif) = justif { + if let Err(err) = self.triage_incoming_justif(justif) { + debug!(target: "beefy", "🥩 {}", err); + } } }, vote = votes.next() => { From 88f890432b32a016fae4716d2e4878cb251df6d2 Mon Sep 17 00:00:00 2001 From: acatangiu Date: Tue, 13 Sep 2022 11:35:59 +0300 Subject: [PATCH 11/31] move beefy proto name test --- client/beefy/src/communication/mod.rs | 27 +++++++++++++++++++++++++++ client/beefy/src/tests.rs | 23 ++--------------------- client/beefy/src/worker.rs | 4 ++-- 3 files changed, 31 insertions(+), 23 deletions(-) diff --git a/client/beefy/src/communication/mod.rs b/client/beefy/src/communication/mod.rs index fba2edee6088c..6f8cac2d58c3a 100644 --- a/client/beefy/src/communication/mod.rs +++ b/client/beefy/src/communication/mod.rs @@ -74,3 +74,30 @@ pub fn beefy_peers_set_config( cfg.add_fallback_names(beefy_protocol_name::LEGACY_NAMES.iter().map(|&n| n.into()).collect()); cfg } + +#[cfg(test)] +mod tests { + use super::*; + + use sp_core::H256; + + #[test] + fn beefy_gossip_protocol_name() { + use beefy_protocol_name::gossip_protocol_name; + // Create protocol name using random genesis hash. + let genesis_hash = H256::random(); + let expected = format!("/{}/beefy/1", hex::encode(genesis_hash)); + let proto_name = gossip_protocol_name(&genesis_hash, None); + assert_eq!(proto_name.to_string(), expected); + + // Create protocol name using hardcoded genesis hash. Verify exact representation. + let genesis_hash = [ + 50, 4, 60, 123, 58, 106, 216, 246, 194, 188, 139, 193, 33, 212, 202, 171, 9, 55, 123, + 94, 8, 43, 12, 251, 187, 57, 173, 19, 188, 74, 205, 147, + ]; + let expected = + "/32043c7b3a6ad8f6c2bc8bc121d4caab09377b5e082b0cfbbb39ad13bc4acd93/beefy/1".to_string(); + let proto_name = gossip_protocol_name(&genesis_hash, None); + assert_eq!(proto_name.to_string(), expected); + } +} diff --git a/client/beefy/src/tests.rs b/client/beefy/src/tests.rs index 25675d93c1f42..a6af43398275a 100644 --- a/client/beefy/src/tests.rs +++ b/client/beefy/src/tests.rs @@ -59,8 +59,8 @@ use sp_runtime::{ use substrate_test_runtime_client::{runtime::Header, ClientExt}; use crate::{ - beefy_block_import_and_links, communication::beefy_protocol_name::gossip_protocol_name, - justification::*, keystore::tests::Keyring as BeefyKeyring, BeefyRPCLinks, BeefyVoterLinks, + beefy_block_import_and_links, justification::*, keystore::tests::Keyring as BeefyKeyring, + BeefyRPCLinks, BeefyVoterLinks, }; pub(crate) const BEEFY_PROTOCOL_NAME: &'static str = "/beefy/1"; @@ -88,25 +88,6 @@ impl BuildStorage for Genesis { } } -#[test] -fn beefy_protocol_name() { - // Create protocol name using random genesis hash. - let genesis_hash = H256::random(); - let expected = format!("/{}/beefy/1", hex::encode(genesis_hash)); - let proto_name = gossip_protocol_name(&genesis_hash, None); - assert_eq!(proto_name.to_string(), expected); - - // Create protocol name using hardcoded genesis hash. Verify exact representation. - let genesis_hash = [ - 50, 4, 60, 123, 58, 106, 216, 246, 194, 188, 139, 193, 33, 212, 202, 171, 9, 55, 123, 94, - 8, 43, 12, 251, 187, 57, 173, 19, 188, 74, 205, 147, - ]; - let expected = - "/32043c7b3a6ad8f6c2bc8bc121d4caab09377b5e082b0cfbbb39ad13bc4acd93/beefy/1".to_string(); - let proto_name = gossip_protocol_name(&genesis_hash, None); - assert_eq!(proto_name.to_string(), expected); -} - #[derive(Default)] pub(crate) struct PeerData { pub(crate) beefy_rpc_links: Mutex>>, diff --git a/client/beefy/src/worker.rs b/client/beefy/src/worker.rs index 189878cf497e1..66e8c2f3f567f 100644 --- a/client/beefy/src/worker.rs +++ b/client/beefy/src/worker.rs @@ -993,7 +993,7 @@ pub(crate) mod tests { keystore::tests::Keyring, tests::{ create_beefy_keystore, get_beefy_streams, make_beefy_ids, two_validators::TestApi, - BeefyPeer, BeefyTestNet, BEEFY_PROTOCOL_NAME, + BeefyPeer, BeefyTestNet, }, BeefyRPCLinks, }; @@ -1039,7 +1039,7 @@ pub(crate) mod tests { let known_peers = Arc::new(Mutex::new(KnownPeers::new())); let gossip_validator = Arc::new(GossipValidator::new(known_peers.clone())); let gossip_engine = - GossipEngine::new(network.clone(), BEEFY_PROTOCOL_NAME, gossip_validator.clone(), None); + GossipEngine::new(network.clone(), "/beefy/1", gossip_validator.clone(), None); let worker_params = crate::worker::WorkerParams { client: peer.client().as_client(), backend: peer.client().as_backend(), From b5bf047ff111c6f29ee395aa41e9bd9dacf67f27 Mon Sep 17 00:00:00 2001 From: acatangiu Date: Tue, 13 Sep 2022 16:18:36 +0300 Subject: [PATCH 12/31] client/beefy: initialize OnDemandJustificationsEngine --- .../request_response/incoming_handler.rs | 26 ++++++---- .../src/communication/request_response/mod.rs | 8 +-- client/beefy/src/lib.rs | 49 ++++++++++++++----- client/beefy/src/worker.rs | 9 +--- 4 files changed, 60 insertions(+), 32 deletions(-) diff --git a/client/beefy/src/communication/request_response/incoming_handler.rs b/client/beefy/src/communication/request_response/incoming_handler.rs index 122e7d17ad830..c0795da1692c0 100644 --- a/client/beefy/src/communication/request_response/incoming_handler.rs +++ b/client/beefy/src/communication/request_response/incoming_handler.rs @@ -17,9 +17,10 @@ //! Helper for handling (i.e. answering) BEEFY justifications requests from a remote peer. use beefy_primitives::BEEFY_ENGINE_ID; -use log::debug; +use log::{debug, trace}; use sc_client_api::BlockBackend; use sc_network::{config as netconfig, config::RequestResponseConfig}; +use sc_network_common::protocol::ProtocolName; use sp_runtime::{generic::BlockId, traits::Block}; use std::{marker::PhantomData, sync::Arc}; @@ -29,16 +30,16 @@ use crate::communication::request_response::{ /// Handler for incoming BEEFY justifications requests from a remote peer. pub struct BeefyJustifsRequestHandler { - request_receiver: IncomingRequestReceiver, - /// Blockchain client. - client: Arc, - _block: PhantomData, + pub(crate) request_receiver: IncomingRequestReceiver, + pub(crate) justif_protocol_name: ProtocolName, + pub(crate) client: Arc, + pub(crate) _block: PhantomData, } impl BeefyJustifsRequestHandler where B: Block, - Client: BlockBackend + Send + Sync + 'static, + Client: BlockBackend + Send + Sync, { /// Create a new [`BeefyJustifsRequestHandler`]. pub fn new(fork_id: Option<&str>, client: Arc) -> (Self, RequestResponseConfig) { @@ -48,8 +49,13 @@ where .flatten() .expect("Genesis block exists; qed"); let (request_receiver, config) = justif_protocol_config(genesis_hash, fork_id); + let justif_protocol_name = config.name.clone(); - (Self { client, request_receiver, _block: PhantomData }, config) + (Self { request_receiver, justif_protocol_name, client, _block: PhantomData }, config) + } + + pub fn protocol_name(&self) -> ProtocolName { + self.justif_protocol_name.clone() } async fn handle_request(&self, request: IncomingRequest) -> Result<(), Error> { @@ -76,20 +82,22 @@ where /// Run [`BeefyJustifsRequestHandler`]. pub async fn run(mut self) { + trace!(target: "beefy::sync", "🥩 Running BeefyJustifsRequestHandler"); + while let Ok(request) = self.request_receiver.recv(|| vec![]).await { let peer = request.peer; match self.handle_request(request).await { Ok(()) => { debug!( target: "beefy::sync", - "Handled BEEFY justification request from {}.", peer + "🥩 Handled BEEFY justification request from {}.", peer ) }, Err(e) => { // TODO: handle reputation changes here debug!( target: "beefy::sync", - "Failed to handle BEEFY justification request from {}: {}", peer, e, + "🥩 Failed to handle BEEFY justification request from {}: {}", peer, e, ) }, } diff --git a/client/beefy/src/communication/request_response/mod.rs b/client/beefy/src/communication/request_response/mod.rs index 3c6c6dbb024fd..a319e68e2505b 100644 --- a/client/beefy/src/communication/request_response/mod.rs +++ b/client/beefy/src/communication/request_response/mod.rs @@ -44,7 +44,7 @@ const JUSTIF_REQUEST_TIMEOUT: Duration = Duration::from_secs(3); /// /// Returns a receiver for messages received on this protocol and the requested /// `ProtocolConfig`. -pub fn justif_protocol_config>( +pub(crate) fn justif_protocol_config>( genesis_hash: Hash, fork_id: Option<&str>, ) -> (IncomingRequestReceiver, RequestResponseConfig) { @@ -73,7 +73,7 @@ pub struct JustificationRequest { /// A request coming in, including a sender for sending responses. #[derive(Debug)] -pub struct IncomingRequest { +pub(crate) struct IncomingRequest { /// `PeerId` of sending peer. pub peer: PeerId, /// The sent request. @@ -123,10 +123,10 @@ impl IncomingRequest { } } -/// Receiver for incoming requests. +/// Receiver for incoming BEEFY justifications requests. /// /// Takes care of decoding and handling of invalid encoded requests. -pub struct IncomingRequestReceiver { +pub(crate) struct IncomingRequestReceiver { raw: mpsc::Receiver, } diff --git a/client/beefy/src/lib.rs b/client/beefy/src/lib.rs index 09804ae30adac..a74d7b0e57cc0 100644 --- a/client/beefy/src/lib.rs +++ b/client/beefy/src/lib.rs @@ -19,9 +19,10 @@ use beefy_primitives::{BeefyApi, MmrRootHash}; use parking_lot::Mutex; use prometheus::Registry; -use sc_client_api::{Backend, BlockchainEvents, Finalizer}; +use sc_client_api::{Backend, BlockBackend, BlockchainEvents, Finalizer}; use sc_consensus::BlockImport; use sc_network::ProtocolName; +use sc_network_common::service::NetworkRequest; use sc_network_gossip::Network as GossipNetwork; use sp_api::ProvideRuntimeApi; use sp_blockchain::HeaderBackend; @@ -29,7 +30,7 @@ use sp_consensus::{Error as ConsensusError, SyncOracle}; use sp_keystore::SyncCryptoStorePtr; use sp_mmr_primitives::MmrApi; use sp_runtime::traits::Block; -use std::sync::Arc; +use std::{marker::PhantomData, sync::Arc}; mod error; mod keystore; @@ -51,6 +52,7 @@ use crate::{ BeefyVersionedFinalityProofStream, }, peers::KnownPeers, + request_response::outgoing_request::OnDemandJustificationsEngine, }, import::BeefyBlockImport, }; @@ -58,7 +60,6 @@ use crate::{ pub use communication::beefy_protocol_name::{ gossip_protocol_name, justifications_protocol_name as justifs_protocol_name, }; -use sc_network_common::service::NetworkRequest; /// A convenience BEEFY client trait that defines all the type bounds a BEEFY client /// has to satisfy. Ideally that should actually be a trait alias. Unfortunately as @@ -149,6 +150,24 @@ where (import, voter_links, rpc_links) } +/// BEEFY gadget network parameters. +pub struct BeefyNetworkParams +where + B: Block, + N: GossipNetwork + NetworkRequest + Clone + SyncOracle + Send + Sync + 'static, +{ + /// Network implementing gossip, requests and sync-oracle. + pub network: N, + /// Chain specific BEEFY gossip protocol name. See + /// [`beefy_protocol_name::gossip_protocol_name`]. + pub gossip_protocol_name: ProtocolName, + /// Chain specific BEEFY on-demand justifications protocol name. See + /// [`beefy_protocol_name::justifications_protocol_name`]. + pub justifications_protocol_name: ProtocolName, + + _phantom: PhantomData, +} + /// BEEFY gadget initialization parameters. pub struct BeefyParams where @@ -167,15 +186,12 @@ where pub runtime: Arc, /// Local key store pub key_store: Option, - /// Gossip network - pub network: N, + /// BEEFY voter network params + pub network_params: BeefyNetworkParams, /// Minimal delta between blocks, BEEFY should vote for pub min_block_delta: u32, /// Prometheus metric registry pub prometheus_registry: Option, - /// Chain specific BEEFY gossip protocol name. See - /// [`beefy_protocol_name::gossip_protocol_name`]. - pub protocol_name: ProtocolName, /// Links between the block importer, the background voter and the RPC layer. pub links: BeefyVoterLinks, } @@ -187,7 +203,7 @@ pub async fn start_beefy_gadget(beefy_params: BeefyParams, - C: Client, + C: Client + BlockBackend, R: ProvideRuntimeApi, R::Api: BeefyApi + MmrApi, N: GossipNetwork + NetworkRequest + Clone + SyncOracle + Send + Sync + 'static, @@ -197,23 +213,31 @@ where backend, runtime, key_store, - network, + network_params, min_block_delta, prometheus_registry, - protocol_name, links, } = beefy_params; + let BeefyNetworkParams { network, gossip_protocol_name, justifications_protocol_name, .. } = + network_params; + let known_peers = Arc::new(Mutex::new(KnownPeers::new())); let gossip_validator = Arc::new(communication::gossip::GossipValidator::new(known_peers.clone())); let gossip_engine = sc_network_gossip::GossipEngine::new( network.clone(), - protocol_name, + gossip_protocol_name, gossip_validator.clone(), None, ); + let on_demand_justifications = OnDemandJustificationsEngine::new( + network.clone(), + runtime.clone(), + justifications_protocol_name, + ); + let metrics = prometheus_registry.as_ref().map(metrics::Metrics::register).and_then( |result| match result { @@ -237,6 +261,7 @@ where known_peers, gossip_engine, gossip_validator, + on_demand_justifications, links, metrics, min_block_delta, diff --git a/client/beefy/src/worker.rs b/client/beefy/src/worker.rs index 66e8c2f3f567f..6c43e8297dbe6 100644 --- a/client/beefy/src/worker.rs +++ b/client/beefy/src/worker.rs @@ -192,6 +192,7 @@ pub(crate) struct WorkerParams { pub known_peers: Arc>>, pub gossip_engine: GossipEngine, pub gossip_validator: Arc>, + pub on_demand_justifications: OnDemandJustificationsEngine, pub links: BeefyVoterLinks, pub metrics: Option, pub min_block_delta: u32, @@ -255,6 +256,7 @@ where network, gossip_engine, gossip_validator, + on_demand_justifications, known_peers, links, metrics, @@ -266,13 +268,6 @@ where .expect_header(BlockId::number(backend.blockchain().info().finalized_number)) .expect("latest block always has header available; qed."); - let on_demand_justifications = OnDemandJustificationsEngine::new( - network.clone(), - runtime.clone(), - // FIXME: use right protocol name. - "TODO: FIXME: proto-name-here".into(), - ); - BeefyWorker { client: client.clone(), backend, From 2e47d2c2ccdc8e424e9521d73b2112271a250ca5 Mon Sep 17 00:00:00 2001 From: acatangiu Date: Tue, 13 Sep 2022 18:20:05 +0300 Subject: [PATCH 13/31] client/tests: allow for custom req-resp protocols --- .../request_response/outgoing_request.rs | 21 +++- client/beefy/src/round.rs | 17 +-- client/beefy/src/tests.rs | 101 ++++++++++++------ client/beefy/src/worker.rs | 18 ++-- client/network/test/src/lib.rs | 4 + 5 files changed, 109 insertions(+), 52 deletions(-) diff --git a/client/beefy/src/communication/request_response/outgoing_request.rs b/client/beefy/src/communication/request_response/outgoing_request.rs index 149fc3cb0d758..4e0729a990f72 100644 --- a/client/beefy/src/communication/request_response/outgoing_request.rs +++ b/client/beefy/src/communication/request_response/outgoing_request.rs @@ -72,9 +72,7 @@ where { pub fn new(network: N, runtime: Arc, protocol_name: ProtocolName) -> Self { let engine = FuturesUnordered::new(); - let pending = Either::Left(std::future::pending::<_>()); - engine.push(pending); - + engine.push(Either::Left(std::future::pending::<_>())); Self { network, runtime, protocol_name, state: State::Idle, engine } } @@ -111,7 +109,14 @@ where State::AwaitingResponse(block) => block, // Programming error to have active [ResponseReceiver]s in the engine with no pending // requests. - State::Idle => unreachable!(), + State::Idle => { + error!( + target: "beefy", + "🥩 unexpected response received: {:?}", + resp + ); + return None + }, }; let resp = match resp { @@ -174,8 +179,14 @@ where pub fn cancel_requests_older_than(&mut self, latest_block: NumberFor) { match &self.state { State::AwaitingResponse(block) if *block <= latest_block => { + debug!( + target: "beefy", + "🥩 cancel pending request for block: {:?}", + block + ); self.state = State::Idle; - // TODO: reset `self.engine` + self.engine = FuturesUnordered::new(); + self.engine.push(Either::Left(std::future::pending::<_>())); }, _ => (), } diff --git a/client/beefy/src/round.rs b/client/beefy/src/round.rs index c96613eb38a95..04b825f21ed1d 100644 --- a/client/beefy/src/round.rs +++ b/client/beefy/src/round.rs @@ -135,7 +135,7 @@ where } } - pub(crate) fn try_conclude( + pub(crate) fn should_conclude( &mut self, round: &(P, NumberFor), ) -> Option>> { @@ -148,7 +148,6 @@ where if done { let signatures = self.rounds.remove(round)?.votes; - self.conclude(round.1); Some( self.validators() .iter() @@ -279,7 +278,7 @@ mod tests { true )); // round not concluded - assert!(rounds.try_conclude(&round).is_none()); + assert!(rounds.should_conclude(&round).is_none()); // self vote already present, should not self vote assert!(!rounds.should_self_vote(&round)); @@ -296,7 +295,7 @@ mod tests { (Keyring::Dave.public(), Keyring::Dave.sign(b"I am committed")), false )); - assert!(rounds.try_conclude(&round).is_none()); + assert!(rounds.should_conclude(&round).is_none()); // add 2nd good vote assert!(rounds.add_vote( @@ -305,7 +304,7 @@ mod tests { false )); // round not concluded - assert!(rounds.try_conclude(&round).is_none()); + assert!(rounds.should_conclude(&round).is_none()); // add 3rd good vote assert!(rounds.add_vote( @@ -314,7 +313,8 @@ mod tests { false )); // round concluded - assert!(rounds.try_conclude(&round).is_some()); + assert!(rounds.should_conclude(&round).is_some()); + rounds.conclude(round.1); // Eve is a validator, but round was concluded, adding vote disallowed assert!(!rounds.add_vote( @@ -432,11 +432,12 @@ mod tests { assert_eq!(3, rounds.rounds.len()); // conclude unknown round - assert!(rounds.try_conclude(&(H256::from_low_u64_le(5), 5)).is_none()); + assert!(rounds.should_conclude(&(H256::from_low_u64_le(5), 5)).is_none()); assert_eq!(3, rounds.rounds.len()); // conclude round 2 - let signatures = rounds.try_conclude(&(H256::from_low_u64_le(2), 2)).unwrap(); + let signatures = rounds.should_conclude(&(H256::from_low_u64_le(2), 2)).unwrap(); + rounds.conclude(2); assert_eq!(1, rounds.rounds.len()); assert_eq!( diff --git a/client/beefy/src/tests.rs b/client/beefy/src/tests.rs index a6af43398275a..43e3d7e29e712 100644 --- a/client/beefy/src/tests.rs +++ b/client/beefy/src/tests.rs @@ -21,7 +21,7 @@ use futures::{future, stream::FuturesUnordered, Future, StreamExt}; use parking_lot::Mutex; use serde::{Deserialize, Serialize}; -use std::{collections::HashMap, sync::Arc, task::Poll}; +use std::{collections::HashMap, marker::PhantomData, sync::Arc, task::Poll}; use tokio::{runtime::Runtime, time::Duration}; use sc_client_api::HeaderBackend; @@ -32,7 +32,7 @@ use sc_consensus::{ use sc_keystore::LocalKeystore; use sc_network_test::{ Block, BlockImportAdapter, FullPeerConfig, PassThroughVerifier, Peer, PeersClient, - TestNetFactory, + PeersFullClient, TestNetFactory, }; use sc_utils::notification::NotificationReceiver; @@ -41,6 +41,7 @@ use beefy_primitives::{ BeefyApi, ConsensusLog, MmrRootHash, ValidatorSet, VersionedFinalityProof, BEEFY_ENGINE_ID, KEY_TYPE as BeefyKeyType, }; +use sc_network::{config::RequestResponseConfig, ProtocolName}; use sp_mmr_primitives::{ BatchProof, EncodableOpaqueLeaf, Error as MmrError, LeafIndex, MmrApi, Proof, }; @@ -59,11 +60,21 @@ use sp_runtime::{ use substrate_test_runtime_client::{runtime::Header, ClientExt}; use crate::{ - beefy_block_import_and_links, justification::*, keystore::tests::Keyring as BeefyKeyring, + beefy_block_import_and_links, + communication::request_response::{ + incoming_handler::BeefyJustifsRequestHandler, justif_protocol_config, + }, + gossip_protocol_name, + justification::*, + keystore::tests::Keyring as BeefyKeyring, BeefyRPCLinks, BeefyVoterLinks, }; -pub(crate) const BEEFY_PROTOCOL_NAME: &'static str = "/beefy/1"; +const GENESIS_HASH: H256 = H256::zero(); +fn beefy_gossip_proto_name() -> ProtocolName { + gossip_protocol_name(GENESIS_HASH, None) +} + const GOOD_MMR_ROOT: MmrRootHash = MmrRootHash::repeat_byte(0xbf); const BAD_MMR_ROOT: MmrRootHash = MmrRootHash::repeat_byte(0x42); @@ -92,6 +103,8 @@ impl BuildStorage for Genesis { pub(crate) struct PeerData { pub(crate) beefy_rpc_links: Mutex>>, pub(crate) beefy_voter_links: Mutex>>, + pub(crate) beefy_justif_req_handler: + Mutex>>, } #[derive(Default)] @@ -100,23 +113,34 @@ pub(crate) struct BeefyTestNet { } impl BeefyTestNet { - pub(crate) fn new(n_authority: usize, n_full: usize) -> Self { - let mut net = BeefyTestNet { peers: Vec::with_capacity(n_authority + n_full) }; - for _ in 0..n_authority { - net.add_authority_peer(); - } - for _ in 0..n_full { - net.add_full_peer(); + pub(crate) fn new(n_authority: usize) -> Self { + let mut net = BeefyTestNet { peers: Vec::with_capacity(n_authority) }; + + for i in 0..n_authority { + let (rx, cfg) = justif_protocol_config(GENESIS_HASH, None); + let justif_protocol_name = cfg.name.clone(); + + net.add_authority_peer(vec![cfg]); + + let client = net.peers[i].client().as_client(); + let justif_handler = BeefyJustifsRequestHandler { + request_receiver: rx, + justif_protocol_name, + client, + _block: PhantomData, + }; + *net.peers[i].data.beefy_justif_req_handler.lock() = Some(justif_handler); } net } - pub(crate) fn add_authority_peer(&mut self) { + pub(crate) fn add_authority_peer(&mut self, req_resp_cfgs: Vec) { self.add_full_peer_with_config(FullPeerConfig { - notifications_protocols: vec![BEEFY_PROTOCOL_NAME.into()], + notifications_protocols: vec![beefy_gossip_proto_name()], + request_response_protocols: req_resp_cfgs, is_authority: true, ..Default::default() - }) + }); } pub(crate) fn generate_blocks_and_sync( @@ -172,6 +196,7 @@ impl TestNetFactory for BeefyTestNet { let peer_data = PeerData { beefy_rpc_links: Mutex::new(Some(rpc_links)), beefy_voter_links: Mutex::new(Some(voter_links)), + ..Default::default() }; (BlockImportAdapter::new(block_import), None, peer_data) } @@ -189,11 +214,8 @@ impl TestNetFactory for BeefyTestNet { } fn add_full_peer(&mut self) { - self.add_full_peer_with_config(FullPeerConfig { - notifications_protocols: vec![BEEFY_PROTOCOL_NAME.into()], - is_authority: false, - ..Default::default() - }) + // `add_authority_peer()` used instead. + unimplemented!() } } @@ -321,7 +343,7 @@ where API: ProvideRuntimeApi + Default + Sync + Send, API::Api: BeefyApi + MmrApi, { - let voters = FuturesUnordered::new(); + let tasks = FuturesUnordered::new(); for (peer_id, key, api) in peers.into_iter() { let peer = &net.peers[peer_id]; @@ -329,31 +351,44 @@ where let keystore = create_beefy_keystore(*key); let (_, _, peer_data) = net.make_block_import(peer.client().clone()); - let PeerData { beefy_rpc_links, beefy_voter_links } = peer_data; + let PeerData { beefy_rpc_links, beefy_voter_links, .. } = peer_data; let beefy_voter_links = beefy_voter_links.lock().take(); *peer.data.beefy_rpc_links.lock() = beefy_rpc_links.lock().take(); *peer.data.beefy_voter_links.lock() = beefy_voter_links.clone(); + let on_demand_justif_handler = peer.data.beefy_justif_req_handler.lock().take().unwrap(); + + let network_params = crate::BeefyNetworkParams { + network: peer.network_service().clone(), + gossip_protocol_name: beefy_gossip_proto_name(), + justifications_protocol_name: on_demand_justif_handler.protocol_name(), + _phantom: PhantomData, + }; + let beefy_params = crate::BeefyParams { client: peer.client().as_client(), backend: peer.client().as_backend(), runtime: api.clone(), key_store: Some(keystore), - network: peer.network_service().clone(), + network_params, links: beefy_voter_links.unwrap(), min_block_delta, prometheus_registry: None, - protocol_name: BEEFY_PROTOCOL_NAME.into(), }; - let gadget = crate::start_beefy_gadget::<_, _, _, _, _>(beefy_params); + let task = async move { + futures::join!( + crate::start_beefy_gadget::<_, _, _, _, _>(beefy_params), + on_demand_justif_handler.run() + ); + }; fn assert_send(_: &T) {} - assert_send(&gadget); - voters.push(gadget); + assert_send(&task); + tasks.push(task); } - voters.for_each(|_| async move {}) + tasks.for_each(|_| async move {}) } fn block_until(future: impl Future + Unpin, net: &Arc>, runtime: &mut Runtime) { @@ -496,7 +531,7 @@ fn beefy_finalizing_blocks() { let session_len = 10; let min_block_delta = 4; - let mut net = BeefyTestNet::new(2, 0); + let mut net = BeefyTestNet::new(2); let api = Arc::new(two_validators::TestApi {}); let beefy_peers = peers.iter().enumerate().map(|(id, key)| (id, key, api.clone())).collect(); @@ -535,7 +570,7 @@ fn lagging_validators() { let session_len = 30; let min_block_delta = 1; - let mut net = BeefyTestNet::new(2, 0); + let mut net = BeefyTestNet::new(2); let api = Arc::new(two_validators::TestApi {}); let beefy_peers = peers.iter().enumerate().map(|(id, key)| (id, key, api.clone())).collect(); runtime.spawn(initialize_beefy(&mut net, beefy_peers, min_block_delta)); @@ -584,7 +619,7 @@ fn lagging_validators() { // Bob catches up and also finalizes #60 (and should have buffered Alice's vote on #60) let (best_blocks, versioned_finality_proof) = get_beefy_streams(&mut net.lock(), peers); net.lock().peer(1).client().as_client().finalize_block(finalize, None).unwrap(); - // verify beefy skips intermediary votes, and successfully finalizes mandatory block #40 + // verify beefy skips intermediary votes, and successfully finalizes mandatory block #60 wait_for_best_beefy_blocks(best_blocks, &net, &mut runtime, &[60]); wait_for_beefy_signed_commitments(versioned_finality_proof, &net, &mut runtime, &[60]); } @@ -600,7 +635,7 @@ fn correct_beefy_payload() { let session_len = 20; let min_block_delta = 2; - let mut net = BeefyTestNet::new(4, 0); + let mut net = BeefyTestNet::new(4); // Alice, Bob, Charlie will vote on good payloads let good_api = Arc::new(four_validators::TestApi {}); @@ -674,11 +709,11 @@ fn beefy_importing_blocks() { sp_tracing::try_init_simple(); - let mut net = BeefyTestNet::new(2, 0); + let mut net = BeefyTestNet::new(2); let client = net.peer(0).client().clone(); let (mut block_import, _, peer_data) = net.make_block_import(client.clone()); - let PeerData { beefy_rpc_links: _, beefy_voter_links } = peer_data; + let PeerData { beefy_voter_links, .. } = peer_data; let justif_stream = beefy_voter_links.lock().take().unwrap().from_block_import_justif_stream; let params = |block: Block, justifications: Option| { diff --git a/client/beefy/src/worker.rs b/client/beefy/src/worker.rs index 6c43e8297dbe6..2925b605416b2 100644 --- a/client/beefy/src/worker.rs +++ b/client/beefy/src/worker.rs @@ -448,7 +448,7 @@ where let rounds = self.voting_oracle.rounds_mut().ok_or(Error::UninitSession)?; if rounds.add_vote(&round, vote, self_vote) { - if let Some(signatures) = rounds.try_conclude(&round) { + if let Some(signatures) = rounds.should_conclude(&round) { self.gossip_validator.conclude_round(round.1); let block_num = round.1; @@ -1035,6 +1035,11 @@ pub(crate) mod tests { let gossip_validator = Arc::new(GossipValidator::new(known_peers.clone())); let gossip_engine = GossipEngine::new(network.clone(), "/beefy/1", gossip_validator.clone(), None); + let on_demand_justifications = OnDemandJustificationsEngine::new( + network.clone(), + api.clone(), + "/beefy/justifs/1".into(), + ); let worker_params = crate::worker::WorkerParams { client: peer.client().as_client(), backend: peer.client().as_backend(), @@ -1047,6 +1052,7 @@ pub(crate) mod tests { min_block_delta, metrics: None, network, + on_demand_justifications, }; BeefyWorker::<_, _, _, _, _>::new(worker_params) } @@ -1298,7 +1304,7 @@ pub(crate) mod tests { fn keystore_vs_validator_set() { let keys = &[Keyring::Alice]; let validator_set = ValidatorSet::new(make_beefy_ids(keys), 0).unwrap(); - let mut net = BeefyTestNet::new(1, 0); + let mut net = BeefyTestNet::new(1); let mut worker = create_beefy_worker(&net.peer(0), &keys[0], 1); // keystore doesn't contain other keys than validators' @@ -1321,7 +1327,7 @@ pub(crate) mod tests { fn should_finalize_correctly() { let keys = &[Keyring::Alice]; let validator_set = ValidatorSet::new(make_beefy_ids(keys), 0).unwrap(); - let mut net = BeefyTestNet::new(1, 0); + let mut net = BeefyTestNet::new(1); let backend = net.peer(0).client().as_backend(); let mut worker = create_beefy_worker(&net.peer(0), &keys[0], 1); @@ -1408,7 +1414,7 @@ pub(crate) mod tests { fn should_init_session() { let keys = &[Keyring::Alice]; let validator_set = ValidatorSet::new(make_beefy_ids(keys), 0).unwrap(); - let mut net = BeefyTestNet::new(1, 0); + let mut net = BeefyTestNet::new(1); let mut worker = create_beefy_worker(&net.peer(0), &keys[0], 1); assert!(worker.voting_oracle.sessions.is_empty()); @@ -1442,7 +1448,7 @@ pub(crate) mod tests { fn should_triage_votes_and_process_later() { let keys = &[Keyring::Alice, Keyring::Bob]; let validator_set = ValidatorSet::new(make_beefy_ids(keys), 0).unwrap(); - let mut net = BeefyTestNet::new(1, 0); + let mut net = BeefyTestNet::new(1); let mut worker = create_beefy_worker(&net.peer(0), &keys[0], 1); fn new_vote( @@ -1503,7 +1509,7 @@ pub(crate) mod tests { fn should_initialize_correct_voter() { let keys = &[Keyring::Alice]; let validator_set = ValidatorSet::new(make_beefy_ids(keys), 1).unwrap(); - let mut net = BeefyTestNet::new(1, 0); + let mut net = BeefyTestNet::new(1); let backend = net.peer(0).client().as_backend(); // push 15 blocks with `AuthorityChange` digests every 10 blocks diff --git a/client/network/test/src/lib.rs b/client/network/test/src/lib.rs index 837cdeed0f3d1..df94a093ebac1 100644 --- a/client/network/test/src/lib.rs +++ b/client/network/test/src/lib.rs @@ -55,6 +55,7 @@ use sc_network::{ }, Multiaddr, NetworkService, NetworkWorker, }; +use sc_network::config::RequestResponseConfig; use sc_network_common::{ config::{MultiaddrWithPeerId, ProtocolId}, protocol::ProtocolName, @@ -683,6 +684,8 @@ pub struct FullPeerConfig { pub block_announce_validator: Option + Send + Sync>>, /// List of notification protocols that the network must support. pub notifications_protocols: Vec, + /// List of request-response protocols that the network must support. + pub request_response_protocols: Vec, /// The indices of the peers the peer should be connected to. /// /// If `None`, it will be connected to all other peers. @@ -785,6 +788,7 @@ where network_config.transport = TransportConfig::MemoryOnly; network_config.listen_addresses = vec![listen_addr.clone()]; network_config.allow_non_globals_in_dht = true; + network_config.request_response_protocols.extend(config.request_response_protocols); network_config.extra_sets = config .notifications_protocols .into_iter() From fdebcaa485943a4431558cc1799909be56d5b609 Mon Sep 17 00:00:00 2001 From: acatangiu Date: Wed, 14 Sep 2022 18:47:30 +0300 Subject: [PATCH 14/31] client/beefy: on-demand-justif: implement simple peer selection strategy --- client/beefy/src/communication/peers.rs | 16 +- .../request_response/incoming_handler.rs | 4 +- .../src/communication/request_response/mod.rs | 7 +- .../request_response/outgoing_request.rs | 233 +++++++++++------- client/beefy/src/lib.rs | 1 + client/beefy/src/worker.rs | 3 +- 6 files changed, 172 insertions(+), 92 deletions(-) diff --git a/client/beefy/src/communication/peers.rs b/client/beefy/src/communication/peers.rs index 411a61183e128..089e6c5a5bd1c 100644 --- a/client/beefy/src/communication/peers.rs +++ b/client/beefy/src/communication/peers.rs @@ -20,7 +20,7 @@ use sc_network::PeerId; use sp_runtime::traits::{Block, NumberFor, Zero}; -use std::collections::HashMap; +use std::collections::{HashMap, VecDeque}; struct PeerData { last_voted_on: NumberFor, @@ -58,4 +58,18 @@ impl KnownPeers { pub fn remove(&mut self, peer: &PeerId) { self.live.remove(peer); } + + /// Return _filtered and cloned_ list of peers that have voted on `block` or higher. + pub fn peers_at_least_at_block(&self, block: NumberFor) -> VecDeque { + self.live + .iter() + .filter_map(|(k, v)| if v.last_voted_on >= block { Some(k) } else { None }) + .cloned() + .collect() + } + + /// Answer whether `peer` is part of `KnownPeers` set. + pub fn contains(&self, peer: &PeerId) -> bool { + self.live.contains_key(peer) + } } diff --git a/client/beefy/src/communication/request_response/incoming_handler.rs b/client/beefy/src/communication/request_response/incoming_handler.rs index c0795da1692c0..04c644509c15a 100644 --- a/client/beefy/src/communication/request_response/incoming_handler.rs +++ b/client/beefy/src/communication/request_response/incoming_handler.rs @@ -90,14 +90,14 @@ where Ok(()) => { debug!( target: "beefy::sync", - "🥩 Handled BEEFY justification request from {}.", peer + "🥩 Handled BEEFY justification request from {:?}.", peer ) }, Err(e) => { // TODO: handle reputation changes here debug!( target: "beefy::sync", - "🥩 Failed to handle BEEFY justification request from {}: {}", peer, e, + "🥩 Failed to handle BEEFY justification request from {:?}: {}", peer, e, ) }, } diff --git a/client/beefy/src/communication/request_response/mod.rs b/client/beefy/src/communication/request_response/mod.rs index a319e68e2505b..80d6466dca493 100644 --- a/client/beefy/src/communication/request_response/mod.rs +++ b/client/beefy/src/communication/request_response/mod.rs @@ -156,9 +156,6 @@ pub enum Error { #[error(transparent)] RuntimeApi(#[from] sp_api::ApiError), - #[error("BEEFY pallet not available.")] - Pallet, - /// Decoding failed, we were able to change the peer's reputation accordingly. #[error("Decoding request failed for peer {0}.")] DecodingError(PeerId, #[source] CodecError), @@ -174,8 +171,8 @@ pub enum Error { #[error("Failed to send response.")] SendResponse, - #[error("Failed to send response.")] - TodoError, + #[error("Received invalid response.")] + InvalidResponse, } /// General result based on above `Error`. diff --git a/client/beefy/src/communication/request_response/outgoing_request.rs b/client/beefy/src/communication/request_response/outgoing_request.rs index 4e0729a990f72..3af5acd564b06 100644 --- a/client/beefy/src/communication/request_response/outgoing_request.rs +++ b/client/beefy/src/communication/request_response/outgoing_request.rs @@ -18,14 +18,15 @@ //! Generating request logic for request/response protocol for syncing BEEFY justifications. -use beefy_primitives::BeefyApi; +use beefy_primitives::{crypto::AuthorityId, BeefyApi, ValidatorSet}; use codec::Encode; use futures::{ channel::{oneshot, oneshot::Canceled}, future::Either, stream::{FuturesUnordered, StreamExt}, }; -use log::{debug, error}; +use log::{debug, error, warn}; +use parking_lot::Mutex; use sc_network::{PeerId, ProtocolName}; use sc_network_common::{ request_responses::{IfDisconnected, RequestFailure}, @@ -36,11 +37,12 @@ use sp_runtime::{ generic::BlockId, traits::{Block, NumberFor}, }; -use std::{result::Result, sync::Arc}; +use std::{collections::VecDeque, result::Result, sync::Arc}; use crate::{ communication::request_response::{Error, JustificationRequest}, justification::{decode_and_verify_finality_proof, BeefyVersionedFinalityProof}, + KnownPeers, }; /// Response type received from network. @@ -50,13 +52,17 @@ type ResponseReceiver = oneshot::Receiver; enum State { Idle, - AwaitingResponse(NumberFor), + AwaitingResponse(PeerId, NumberFor), } pub struct OnDemandJustificationsEngine { network: N, runtime: Arc, protocol_name: ProtocolName, + + live_peers: Arc>>, + peers_cache: VecDeque, + state: State, engine: FuturesUnordered< Either>, ResponseReceiver>, @@ -70,16 +76,47 @@ where R: ProvideRuntimeApi, R::Api: BeefyApi, { - pub fn new(network: N, runtime: Arc, protocol_name: ProtocolName) -> Self { + pub fn new( + network: N, + runtime: Arc, + protocol_name: ProtocolName, + live_peers: Arc>>, + ) -> Self { let engine = FuturesUnordered::new(); engine.push(Either::Left(std::future::pending::<_>())); - Self { network, runtime, protocol_name, state: State::Idle, engine } + Self { + network, + runtime, + protocol_name, + live_peers, + peers_cache: VecDeque::new(), + state: State::Idle, + engine, + } } - /// Start requesting justification for `block` number. - pub fn request(&mut self, block: NumberFor) { - // TODO: do peer selection based on `known_peers`. - let peer = PeerId::random(); + fn reset_state(&mut self) { + self.state = State::Idle; + self.engine = FuturesUnordered::new(); + self.engine.push(Either::Left(std::future::pending::<_>())); + } + + fn reset_peers_cache_for_block(&mut self, block: NumberFor) { + self.peers_cache = self.live_peers.lock().peers_at_least_at_block(block); + } + + pub fn try_next_peer(&mut self) -> Option { + let live = self.live_peers.lock(); + while let Some(peer) = self.peers_cache.pop_front() { + if live.contains(&peer) { + return Some(peer) + } + } + None + } + + fn request_from_peer(&mut self, peer: PeerId, block: NumberFor) { + debug!(target: "beefy", "🥩 requesting justif #{:?} from peer {:?}", block, peer); let payload = JustificationRequest:: { begin: block }.encode(); @@ -94,101 +131,131 @@ where ); self.engine.push(Either::Right(rx)); - self.state = State::AwaitingResponse(block); + self.state = State::AwaitingResponse(peer, block); } - pub async fn next(&mut self) -> Option> { - // The `engine` contains at the very least a `Pending` future (future that never finishes). - // This "blocks" until: - // - we have a [ResponseReceiver] added to the engine, and - // - we also get a [Response] on it. - let resp = self.engine.next().await; + /// Start requesting justification for `block` number. + pub fn start_new_request(&mut self, block: NumberFor) { + self.reset_state(); + self.reset_peers_cache_for_block(block); - let resp = resp.expect("Engine will never end because of inner `Pending` future, qed."); - let number = match self.state { - State::AwaitingResponse(block) => block, - // Programming error to have active [ResponseReceiver]s in the engine with no pending - // requests. - State::Idle => { - error!( + if let Some(peer) = self.try_next_peer() { + self.request_from_peer(peer, block); + } else { + // TODO: add some timeout mechanism to retry and not get stuck just because + // there was no connectivity when we first tried this. + debug!(target: "beefy", "🥩 No peers to request justif #{:?} from!", block); + } + } + + /// Cancel any pending request for block numbers smaller or equal to `block`. + pub fn cancel_requests_older_than(&mut self, block: NumberFor) { + match &self.state { + State::AwaitingResponse(_, number) if *number <= block => { + debug!( target: "beefy", - "🥩 unexpected response received: {:?}", - resp + "🥩 cancel pending request for block: {:?}", + number ); - return None + self.reset_state(); }, - }; + _ => (), + } + } - let resp = match resp { - Ok(resp) => resp, - Err(e) => { + fn process_response( + &mut self, + peer: PeerId, + block: NumberFor, + validator_set: &ValidatorSet, + response: std::result::Result, + ) -> std::result::Result, Error> { + response + .map_err(|e| { debug!( target: "beefy", - "🥩 on demand justification (block {:?}) peer hung up: {:?}", - number, e + "🥩 for on demand justification #{:?}, peer {:?} hung up: {:?}", + block, peer, e ); - self.request(number); - return None - }, - }; + Error::InvalidResponse + })? + .map_err(|e| { + debug!( + target: "beefy", + "🥩 for on demand justification #{:?}, peer {:?} hung up: {:?}", + block, peer, e + ); + Error::InvalidResponse + }) + .and_then(|encoded| { + decode_and_verify_finality_proof::(&encoded[..], block, &validator_set).map_err( + |e| { + debug!( + target: "beefy", + "🥩 for on demand justification #{:?}, peer {:?} responded with invalid proof: {:?}", + block, peer, e + ); + Error::InvalidResponse + }, + ) + }) + } + + pub async fn next(&mut self) -> Option> { + // The `engine` contains at the very least a `Pending` future (future that never finishes). + // This "blocks" until: + // - we have a [ResponseReceiver] added to the engine, and + // - we also get a [Response] on it. + let resp = self + .engine + .next() + .await + .expect("Engine will never end because of inner `Pending` future, qed."); - let encoded = match resp { - Ok(encoded) => encoded, - Err(e) => { + let (peer, block) = match self.state { + State::AwaitingResponse(peer, block) => (peer, block), + // Programming error to have active [ResponseReceiver]s in the engine with no pending + // requests. Just log an error for now. + State::Idle => { error!( target: "beefy", - "🥩 on demand justification (block {:?}) peer responded with error: {:?}", - number, e + "🥩 unexpected response received in 'Idle' state: {:?}", + resp ); + self.reset_state(); return None }, }; - match self.process_response(encoded, number) { - Ok(proof) => Some(proof), - Err(e) => { - debug!( - target: "beefy", - "🥩 on demand justification (block {:?}) invalid proof: {:?}", - number, e - ); - self.request(number); - None - }, - } - } - - fn process_response( - &mut self, - encoded: Vec, - number: NumberFor, - ) -> Result, Error> { - let block_id = BlockId::number(number); + let block_id = BlockId::number(block); let validator_set = self .runtime .runtime_api() .validator_set(&block_id) - .map_err(Error::RuntimeApi)? - .ok_or_else(|| Error::Pallet)?; - - let proof = decode_and_verify_finality_proof::(&encoded[..], number, &validator_set); - proof.map_err(|_| Error::TodoError) - } + .map_err(|e| { + error!(target: "beefy", "🥩 Runtime API error {:?} in on-demand justif engine.", e); + e + }) + .ok()? + .or_else(|| { + error!(target: "beefy", "🥩 BEEFY pallet not available for block {:?}.", block); + None + })?; - /// Cancel any pending request for block numbers smaller or equal to `latest_block`. - pub fn cancel_requests_older_than(&mut self, latest_block: NumberFor) { - match &self.state { - State::AwaitingResponse(block) if *block <= latest_block => { - debug!( - target: "beefy", - "🥩 cancel pending request for block: {:?}", - block - ); - self.state = State::Idle; - self.engine = FuturesUnordered::new(); - self.engine.push(Either::Left(std::future::pending::<_>())); - }, - _ => (), - } + self.process_response(peer, block, &validator_set, resp) + .map_err(|_| { + // No valid justification received, try next peer in our set. + if let Some(peer) = self.try_next_peer() { + self.request_from_peer(peer, block); + } else { + warn!(target: "beefy", "🥩 ran out of peers to request justif #{:?} from", block); + } + }) + .map(|proof| { + // Good proof received, go back to 'Idle'. + self.reset_state(); + proof + }) + .ok() } } diff --git a/client/beefy/src/lib.rs b/client/beefy/src/lib.rs index a74d7b0e57cc0..334a3de04ca08 100644 --- a/client/beefy/src/lib.rs +++ b/client/beefy/src/lib.rs @@ -236,6 +236,7 @@ where network.clone(), runtime.clone(), justifications_protocol_name, + known_peers.clone(), ); let metrics = diff --git a/client/beefy/src/worker.rs b/client/beefy/src/worker.rs index 2925b605416b2..f4f311f53ea33 100644 --- a/client/beefy/src/worker.rs +++ b/client/beefy/src/worker.rs @@ -386,7 +386,7 @@ where self.init_session_at(new_validator_set, *header.number()); // TODO (issue #12093): when adding SYNC protocol, // fire up a request for justification for this mandatory block here. - self.on_demand_justifications.request(*header.number()); + self.on_demand_justifications.start_new_request(*header.number()); } } } @@ -1039,6 +1039,7 @@ pub(crate) mod tests { network.clone(), api.clone(), "/beefy/justifs/1".into(), + known_peers.clone(), ); let worker_params = crate::worker::WorkerParams { client: peer.client().as_client(), From 2f02fba52557258685a3d1b8e3caa4edd4c80433 Mon Sep 17 00:00:00 2001 From: acatangiu Date: Thu, 15 Sep 2022 12:52:27 +0300 Subject: [PATCH 15/31] client/beefy: fix voter initialization Fix corner case where voter gets a single burst of finality notifications just when it starts. The notification stream was consumed by "wait_for_pallet" logic, then main loop would subscribe to finality notifications, but by that time some notifications might've been lost. Fix this by subscribing the main loop to notifications before waiting for pallet to become available. Share the same stream with the main loop so that notifications for blocks before pallet available are ignored, while _all_ notifications after pallet available are processed. Add regression test for this. Signed-off-by: acatangiu --- client/beefy/src/tests.rs | 34 ++++++++++++++++++++++++++++++++++ client/beefy/src/worker.rs | 15 ++++++++------- 2 files changed, 42 insertions(+), 7 deletions(-) diff --git a/client/beefy/src/tests.rs b/client/beefy/src/tests.rs index 43e3d7e29e712..52a6a65ff8f9a 100644 --- a/client/beefy/src/tests.rs +++ b/client/beefy/src/tests.rs @@ -820,3 +820,37 @@ fn beefy_importing_blocks() { })); } } + +#[test] +fn voter_initialization() { + sp_tracing::try_init_simple(); + // Regression test for voter initialization where finality notifications were dropped + // after waiting for BEEFY pallet availability. + + let mut runtime = Runtime::new().unwrap(); + let peers = &[BeefyKeyring::Alice, BeefyKeyring::Bob]; + let validator_set = ValidatorSet::new(make_beefy_ids(peers), 0).unwrap(); + let session_len = 5; + // Should vote on all mandatory blocks no matter the `min_block_delta`. + let min_block_delta = 10; + + let mut net = BeefyTestNet::new(2); + let api = Arc::new(two_validators::TestApi {}); + let beefy_peers = peers.iter().enumerate().map(|(id, key)| (id, key, api.clone())).collect(); + runtime.spawn(initialize_beefy(&mut net, beefy_peers, min_block_delta)); + + // push 30 blocks + net.generate_blocks_and_sync(26, session_len, &validator_set, false); + let net = Arc::new(Mutex::new(net)); + + // Finalize multiple blocks at once to get a burst of finality notifications right from start. + // Need to finalize at least one block in each session, choose randomly. + // Expect voters to pick up all of them and BEEFY-finalize the mandatory blocks of each session. + finalize_block_and_wait_for_beefy( + &net, + peers, + &mut runtime, + &[1, 6, 10, 17, 24, 26], + &[1, 5, 10, 15, 20, 25], + ); +} diff --git a/client/beefy/src/worker.rs b/client/beefy/src/worker.rs index f4f311f53ea33..814ae43d09ff9 100644 --- a/client/beefy/src/worker.rs +++ b/client/beefy/src/worker.rs @@ -24,11 +24,11 @@ use std::{ }; use codec::{Codec, Decode, Encode}; -use futures::{FutureExt, StreamExt}; +use futures::{stream::Fuse, FutureExt, StreamExt}; use log::{debug, error, info, log_enabled, trace, warn}; use parking_lot::Mutex; -use sc_client_api::{Backend, FinalityNotification, HeaderBackend}; +use sc_client_api::{Backend, FinalityNotification, FinalityNotifications, HeaderBackend}; use sc_network_common::{ protocol::event::Event as NetEvent, service::{NetworkEventStream, NetworkRequest}, @@ -744,12 +744,11 @@ where /// Wait for BEEFY runtime pallet to be available. /// Should be called only once during worker initialization. - async fn wait_for_runtime_pallet(&mut self) { + async fn wait_for_runtime_pallet(&mut self, finality: &mut Fuse>) { let mut gossip_engine = &mut self.gossip_engine; - let mut finality_stream = self.client.finality_notification_stream().fuse(); loop { futures::select! { - notif = finality_stream.next() => { + notif = finality.next() => { let notif = match notif { Some(notif) => notif, None => break @@ -783,12 +782,14 @@ where pub(crate) async fn run(mut self) { info!(target: "beefy", "🥩 run BEEFY worker, best grandpa: #{:?}.", self.best_grandpa_block_header.number()); let mut block_import_justif = self.links.from_block_import_justif_stream.subscribe().fuse(); + // Subscribe to finality notifications before waiting for runtime pallet and reuse stream, + // so we process notifications for all finalized blocks after pallet is available. + let mut finality_notifications = self.client.finality_notification_stream().fuse(); - self.wait_for_runtime_pallet().await; + self.wait_for_runtime_pallet(&mut finality_notifications).await; trace!(target: "beefy", "🥩 BEEFY pallet available, starting voter."); let mut network_events = self.network.event_stream("network-gossip").fuse(); - let mut finality_notifications = self.client.finality_notification_stream().fuse(); let mut votes = Box::pin( self.gossip_engine .messages_for(topic::()) From a9c60df5ea81605b9b8bd2f0f2dbde5eaf5ea4b0 Mon Sep 17 00:00:00 2001 From: acatangiu Date: Fri, 16 Sep 2022 14:03:09 +0300 Subject: [PATCH 16/31] client/beefy: make sure justif requests are always out for mandatory blocks --- .../request_response/outgoing_request.rs | 27 ++++++++++--------- client/beefy/src/worker.rs | 20 +++++++++++--- 2 files changed, 31 insertions(+), 16 deletions(-) diff --git a/client/beefy/src/communication/request_response/outgoing_request.rs b/client/beefy/src/communication/request_response/outgoing_request.rs index 3af5acd564b06..c8bc38343e7bf 100644 --- a/client/beefy/src/communication/request_response/outgoing_request.rs +++ b/client/beefy/src/communication/request_response/outgoing_request.rs @@ -134,17 +134,19 @@ where self.state = State::AwaitingResponse(peer, block); } - /// Start requesting justification for `block` number. - pub fn start_new_request(&mut self, block: NumberFor) { - self.reset_state(); + /// If no other request is in progress, start new justification request for `block`. + pub fn request(&mut self, block: NumberFor) { + // ignore new requests while there's already one pending + match &self.state { + State::AwaitingResponse(_, _) => return, + State::Idle => (), + } self.reset_peers_cache_for_block(block); + // Start the requests engine - each unsuccessful received response will automatically + // trigger a new request to the next peer in the `peers_cache` until there are none left. if let Some(peer) = self.try_next_peer() { self.request_from_peer(peer, block); - } else { - // TODO: add some timeout mechanism to retry and not get stuck just because - // there was no connectivity when we first tried this. - debug!(target: "beefy", "🥩 No peers to request justif #{:?} from!", block); } } @@ -154,7 +156,7 @@ where State::AwaitingResponse(_, number) if *number <= block => { debug!( target: "beefy", - "🥩 cancel pending request for block: {:?}", + "🥩 cancel pending request for justification #{:?}", number ); self.reset_state(); @@ -226,6 +228,10 @@ where return None }, }; + // We received the awaited response. All response processing success and error cases + // ultimately move the engine in `Idle` state, so just do it now to make things simple + // and make sure we don't miss any error short-circuit. + self.reset_state(); let block_id = BlockId::number(block); let validator_set = self @@ -251,11 +257,6 @@ where warn!(target: "beefy", "🥩 ran out of peers to request justif #{:?} from", block); } }) - .map(|proof| { - // Good proof received, go back to 'Idle'. - self.reset_state(); - proof - }) .ok() } } diff --git a/client/beefy/src/worker.rs b/client/beefy/src/worker.rs index 814ae43d09ff9..a65967f557708 100644 --- a/client/beefy/src/worker.rs +++ b/client/beefy/src/worker.rs @@ -121,6 +121,17 @@ impl VoterOracle { } } + /// Return current pending mandatory block, if any. + pub fn mandatory_pending(&self) -> Option> { + self.sessions.front().and_then(|round| { + if round.mandatory_done() { + None + } else { + Some(round.session_start()) + } + }) + } + /// Return `(A, B)` tuple representing inclusive [A, B] interval of votes to accept. pub fn accepted_interval( &self, @@ -384,9 +395,6 @@ where { if let Some(new_validator_set) = find_authorities_change::(&header) { self.init_session_at(new_validator_set, *header.number()); - // TODO (issue #12093): when adding SYNC protocol, - // fire up a request for justification for this mandatory block here. - self.on_demand_justifications.start_new_request(*header.number()); } } } @@ -870,6 +878,12 @@ where // Don't bother voting during major sync. if !self.network.is_major_syncing() { + // If the current target is a mandatory block, + // make sure there's also an on-demand justification request out for it. + if let Some(block) = self.voting_oracle.mandatory_pending() { + // This only starts new request if there isn't already an active one. + self.on_demand_justifications.request(block); + } // There were external events, 'state' is changed, author a vote if needed/possible. if let Err(err) = self.try_to_vote() { debug!(target: "beefy", "🥩 {}", err); From d48e19960590db8bef3a6730e8c71dd22fb90ae4 Mon Sep 17 00:00:00 2001 From: acatangiu Date: Fri, 16 Sep 2022 14:05:02 +0300 Subject: [PATCH 17/31] client/beefy: add test for on-demand justifications sync --- .../request_response/outgoing_request.rs | 30 ++-- client/beefy/src/round.rs | 3 +- client/beefy/src/tests.rs | 160 ++++++++++++++---- client/beefy/src/worker.rs | 18 +- 4 files changed, 162 insertions(+), 49 deletions(-) diff --git a/client/beefy/src/communication/request_response/outgoing_request.rs b/client/beefy/src/communication/request_response/outgoing_request.rs index c8bc38343e7bf..efbb50057794a 100644 --- a/client/beefy/src/communication/request_response/outgoing_request.rs +++ b/client/beefy/src/communication/request_response/outgoing_request.rs @@ -116,7 +116,7 @@ where } fn request_from_peer(&mut self, peer: PeerId, block: NumberFor) { - debug!(target: "beefy", "🥩 requesting justif #{:?} from peer {:?}", block, peer); + debug!(target: "beefy::sync", "🥩 requesting justif #{:?} from peer {:?}", block, peer); let payload = JustificationRequest:: { begin: block }.encode(); @@ -147,6 +147,8 @@ where // trigger a new request to the next peer in the `peers_cache` until there are none left. if let Some(peer) = self.try_next_peer() { self.request_from_peer(peer, block); + } else { + debug!(target: "beefy::sync", "🥩 no good peers to request justif #{:?} from", block); } } @@ -155,7 +157,7 @@ where match &self.state { State::AwaitingResponse(_, number) if *number <= block => { debug!( - target: "beefy", + target: "beefy::sync", "🥩 cancel pending request for justification #{:?}", number ); @@ -175,7 +177,7 @@ where response .map_err(|e| { debug!( - target: "beefy", + target: "beefy::sync", "🥩 for on demand justification #{:?}, peer {:?} hung up: {:?}", block, peer, e ); @@ -183,8 +185,8 @@ where })? .map_err(|e| { debug!( - target: "beefy", - "🥩 for on demand justification #{:?}, peer {:?} hung up: {:?}", + target: "beefy::sync", + "🥩 for on demand justification #{:?}, peer {:?} error: {:?}", block, peer, e ); Error::InvalidResponse @@ -193,7 +195,7 @@ where decode_and_verify_finality_proof::(&encoded[..], block, &validator_set).map_err( |e| { debug!( - target: "beefy", + target: "beefy::sync", "🥩 for on demand justification #{:?}, peer {:?} responded with invalid proof: {:?}", block, peer, e ); @@ -220,7 +222,7 @@ where // requests. Just log an error for now. State::Idle => { error!( - target: "beefy", + target: "beefy::sync", "🥩 unexpected response received in 'Idle' state: {:?}", resp ); @@ -239,12 +241,12 @@ where .runtime_api() .validator_set(&block_id) .map_err(|e| { - error!(target: "beefy", "🥩 Runtime API error {:?} in on-demand justif engine.", e); + error!(target: "beefy::sync", "🥩 Runtime API error {:?} in on-demand justif engine.", e); e }) .ok()? .or_else(|| { - error!(target: "beefy", "🥩 BEEFY pallet not available for block {:?}.", block); + error!(target: "beefy::sync", "🥩 BEEFY pallet not available for block {:?}.", block); None })?; @@ -254,9 +256,17 @@ where if let Some(peer) = self.try_next_peer() { self.request_from_peer(peer, block); } else { - warn!(target: "beefy", "🥩 ran out of peers to request justif #{:?} from", block); + warn!(target: "beefy::sync", "🥩 ran out of peers to request justif #{:?} from", block); } }) + .map(|proof| { + debug!( + target: "beefy::sync", + "🥩 received valid on-demand justif #{:?} from {:?}", + block, peer + ); + proof + }) .ok() } } diff --git a/client/beefy/src/round.rs b/client/beefy/src/round.rs index 04b825f21ed1d..45d346ccd85eb 100644 --- a/client/beefy/src/round.rs +++ b/client/beefy/src/round.rs @@ -33,7 +33,7 @@ use sp_runtime::traits::{Block, NumberFor}; /// whether the local `self` validator has voted/signed. /// /// Does not do any validation on votes or signatures, layers above need to handle that (gossip). -#[derive(Default)] +#[derive(Debug, Default)] struct RoundTracker { self_vote: bool, votes: HashMap, @@ -69,6 +69,7 @@ pub fn threshold(authorities: usize) -> usize { /// Only round numbers > `best_done` are of interest, all others are considered stale. /// /// Does not do any validation on votes or signatures, layers above need to handle that (gossip). +#[derive(Debug)] pub(crate) struct Rounds { rounds: BTreeMap<(Payload, NumberFor), RoundTracker>, session_start: NumberFor, diff --git a/client/beefy/src/tests.rs b/client/beefy/src/tests.rs index 52a6a65ff8f9a..31da9caf02533 100644 --- a/client/beefy/src/tests.rs +++ b/client/beefy/src/tests.rs @@ -406,18 +406,19 @@ fn run_for(duration: Duration, net: &Arc>, runtime: &mut Run pub(crate) fn get_beefy_streams( net: &mut BeefyTestNet, - peers: &[BeefyKeyring], + // peer index and key + peers: impl Iterator, ) -> (Vec>, Vec>>) { let mut best_block_streams = Vec::new(); let mut versioned_finality_proof_streams = Vec::new(); - for peer_id in 0..peers.len() { - let beefy_rpc_links = net.peer(peer_id).data.beefy_rpc_links.lock().clone().unwrap(); + peers.for_each(|(index, _)| { + let beefy_rpc_links = net.peer(index).data.beefy_rpc_links.lock().clone().unwrap(); let BeefyRPCLinks { from_voter_justif_stream, from_voter_best_beefy_stream } = beefy_rpc_links; best_block_streams.push(from_voter_best_beefy_stream.subscribe()); versioned_finality_proof_streams.push(from_voter_justif_stream.subscribe()); - } + }); (best_block_streams, versioned_finality_proof_streams) } @@ -495,18 +496,24 @@ fn streams_empty_after_timeout( fn finalize_block_and_wait_for_beefy( net: &Arc>, - peers: &[BeefyKeyring], + // peer index and key + peers: impl Iterator + Clone, runtime: &mut Runtime, finalize_targets: &[u64], expected_beefy: &[u64], ) { - let (best_blocks, versioned_finality_proof) = get_beefy_streams(&mut net.lock(), peers); + let (best_blocks, versioned_finality_proof) = get_beefy_streams(&mut net.lock(), peers.clone()); for block in finalize_targets { let finalize = BlockId::number(*block); - for i in 0..peers.len() { - net.lock().peer(i).client().as_client().finalize_block(finalize, None).unwrap(); - } + peers.clone().for_each(|(index, _)| { + net.lock() + .peer(index) + .client() + .as_client() + .finalize_block(finalize, None) + .unwrap(); + }) } if expected_beefy.is_empty() { @@ -526,8 +533,8 @@ fn beefy_finalizing_blocks() { sp_tracing::try_init_simple(); let mut runtime = Runtime::new().unwrap(); - let peers = &[BeefyKeyring::Alice, BeefyKeyring::Bob]; - let validator_set = ValidatorSet::new(make_beefy_ids(peers), 0).unwrap(); + let peers = [BeefyKeyring::Alice, BeefyKeyring::Bob]; + let validator_set = ValidatorSet::new(make_beefy_ids(&peers), 0).unwrap(); let session_len = 10; let min_block_delta = 4; @@ -544,17 +551,18 @@ fn beefy_finalizing_blocks() { // Minimum BEEFY block delta is 4. + let peers = peers.into_iter().enumerate(); // finalize block #5 -> BEEFY should finalize #1 (mandatory) and #5 from diff-power-of-two rule. - finalize_block_and_wait_for_beefy(&net, peers, &mut runtime, &[5], &[1, 5]); + finalize_block_and_wait_for_beefy(&net, peers.clone(), &mut runtime, &[5], &[1, 5]); // GRANDPA finalize #10 -> BEEFY finalize #10 (mandatory) - finalize_block_and_wait_for_beefy(&net, peers, &mut runtime, &[10], &[10]); + finalize_block_and_wait_for_beefy(&net, peers.clone(), &mut runtime, &[10], &[10]); // GRANDPA finalize #18 -> BEEFY finalize #14, then #18 (diff-power-of-two rule) - finalize_block_and_wait_for_beefy(&net, peers, &mut runtime, &[18], &[14, 18]); + finalize_block_and_wait_for_beefy(&net, peers.clone(), &mut runtime, &[18], &[14, 18]); // GRANDPA finalize #20 -> BEEFY finalize #20 (mandatory) - finalize_block_and_wait_for_beefy(&net, peers, &mut runtime, &[20], &[20]); + finalize_block_and_wait_for_beefy(&net, peers.clone(), &mut runtime, &[20], &[20]); // GRANDPA finalize #21 -> BEEFY finalize nothing (yet) because min delta is 4 finalize_block_and_wait_for_beefy(&net, peers, &mut runtime, &[21], &[]); @@ -565,8 +573,8 @@ fn lagging_validators() { sp_tracing::try_init_simple(); let mut runtime = Runtime::new().unwrap(); - let peers = &[BeefyKeyring::Alice, BeefyKeyring::Bob]; - let validator_set = ValidatorSet::new(make_beefy_ids(peers), 0).unwrap(); + let peers = [BeefyKeyring::Alice, BeefyKeyring::Bob]; + let validator_set = ValidatorSet::new(make_beefy_ids(&peers), 0).unwrap(); let session_len = 30; let min_block_delta = 1; @@ -580,13 +588,20 @@ fn lagging_validators() { let net = Arc::new(Mutex::new(net)); + let peers = peers.into_iter().enumerate(); // finalize block #15 -> BEEFY should finalize #1 (mandatory) and #9, #13, #14, #15 from // diff-power-of-two rule. - finalize_block_and_wait_for_beefy(&net, peers, &mut runtime, &[15], &[1, 9, 13, 14, 15]); + finalize_block_and_wait_for_beefy( + &net, + peers.clone(), + &mut runtime, + &[15], + &[1, 9, 13, 14, 15], + ); // Alice finalizes #25, Bob lags behind let finalize = BlockId::number(25); - let (best_blocks, versioned_finality_proof) = get_beefy_streams(&mut net.lock(), peers); + let (best_blocks, versioned_finality_proof) = get_beefy_streams(&mut net.lock(), peers.clone()); net.lock().peer(0).client().as_client().finalize_block(finalize, None).unwrap(); // verify nothing gets finalized by BEEFY let timeout = Some(Duration::from_millis(250)); @@ -594,21 +609,21 @@ fn lagging_validators() { streams_empty_after_timeout(versioned_finality_proof, &net, &mut runtime, None); // Bob catches up and also finalizes #25 - let (best_blocks, versioned_finality_proof) = get_beefy_streams(&mut net.lock(), peers); + let (best_blocks, versioned_finality_proof) = get_beefy_streams(&mut net.lock(), peers.clone()); net.lock().peer(1).client().as_client().finalize_block(finalize, None).unwrap(); // expected beefy finalizes block #17 from diff-power-of-two wait_for_best_beefy_blocks(best_blocks, &net, &mut runtime, &[23, 24, 25]); wait_for_beefy_signed_commitments(versioned_finality_proof, &net, &mut runtime, &[23, 24, 25]); // Both finalize #30 (mandatory session) and #32 -> BEEFY finalize #30 (mandatory), #31, #32 - finalize_block_and_wait_for_beefy(&net, peers, &mut runtime, &[30, 32], &[30, 31, 32]); + finalize_block_and_wait_for_beefy(&net, peers.clone(), &mut runtime, &[30, 32], &[30, 31, 32]); // Verify that session-boundary votes get buffered by client and only processed once // session-boundary block is GRANDPA-finalized (this guarantees authenticity for the new session // validator set). // Alice finalizes session-boundary mandatory block #60, Bob lags behind - let (best_blocks, versioned_finality_proof) = get_beefy_streams(&mut net.lock(), peers); + let (best_blocks, versioned_finality_proof) = get_beefy_streams(&mut net.lock(), peers.clone()); let finalize = BlockId::number(60); net.lock().peer(0).client().as_client().finalize_block(finalize, None).unwrap(); // verify nothing gets finalized by BEEFY @@ -629,9 +644,8 @@ fn correct_beefy_payload() { sp_tracing::try_init_simple(); let mut runtime = Runtime::new().unwrap(); - let peers = - &[BeefyKeyring::Alice, BeefyKeyring::Bob, BeefyKeyring::Charlie, BeefyKeyring::Dave]; - let validator_set = ValidatorSet::new(make_beefy_ids(peers), 0).unwrap(); + let peers = [BeefyKeyring::Alice, BeefyKeyring::Bob, BeefyKeyring::Charlie, BeefyKeyring::Dave]; + let validator_set = ValidatorSet::new(make_beefy_ids(&peers), 0).unwrap(); let session_len = 20; let min_block_delta = 2; @@ -651,15 +665,16 @@ fn correct_beefy_payload() { let bad_peers = vec![(3, &BeefyKeyring::Dave, bad_api)]; runtime.spawn(initialize_beefy(&mut net, bad_peers, min_block_delta)); - // push 10 blocks + // push 12 blocks net.generate_blocks_and_sync(12, session_len, &validator_set, false); let net = Arc::new(Mutex::new(net)); + let peers = peers.into_iter().enumerate(); // with 3 good voters and 1 bad one, consensus should happen and best blocks produced. finalize_block_and_wait_for_beefy(&net, peers, &mut runtime, &[10], &[1, 9]); let (best_blocks, versioned_finality_proof) = - get_beefy_streams(&mut net.lock(), &[BeefyKeyring::Alice]); + get_beefy_streams(&mut net.lock(), [(0, BeefyKeyring::Alice)].into_iter()); // now 2 good validators and 1 bad one are voting net.lock() @@ -688,7 +703,7 @@ fn correct_beefy_payload() { // 3rd good validator catches up and votes as well let (best_blocks, versioned_finality_proof) = - get_beefy_streams(&mut net.lock(), &[BeefyKeyring::Alice]); + get_beefy_streams(&mut net.lock(), [(0, BeefyKeyring::Alice)].into_iter()); net.lock() .peer(2) .client() @@ -828,8 +843,8 @@ fn voter_initialization() { // after waiting for BEEFY pallet availability. let mut runtime = Runtime::new().unwrap(); - let peers = &[BeefyKeyring::Alice, BeefyKeyring::Bob]; - let validator_set = ValidatorSet::new(make_beefy_ids(peers), 0).unwrap(); + let peers = [BeefyKeyring::Alice, BeefyKeyring::Bob]; + let validator_set = ValidatorSet::new(make_beefy_ids(&peers), 0).unwrap(); let session_len = 5; // Should vote on all mandatory blocks no matter the `min_block_delta`. let min_block_delta = 10; @@ -839,7 +854,7 @@ fn voter_initialization() { let beefy_peers = peers.iter().enumerate().map(|(id, key)| (id, key, api.clone())).collect(); runtime.spawn(initialize_beefy(&mut net, beefy_peers, min_block_delta)); - // push 30 blocks + // push 26 blocks net.generate_blocks_and_sync(26, session_len, &validator_set, false); let net = Arc::new(Mutex::new(net)); @@ -848,9 +863,90 @@ fn voter_initialization() { // Expect voters to pick up all of them and BEEFY-finalize the mandatory blocks of each session. finalize_block_and_wait_for_beefy( &net, - peers, + peers.into_iter().enumerate(), &mut runtime, &[1, 6, 10, 17, 24, 26], &[1, 5, 10, 15, 20, 25], ); } + +#[test] +fn on_demand_beefy_justification_sync() { + sp_tracing::try_init_simple(); + + let mut runtime = Runtime::new().unwrap(); + let all_peers = + [BeefyKeyring::Alice, BeefyKeyring::Bob, BeefyKeyring::Charlie, BeefyKeyring::Dave]; + let validator_set = ValidatorSet::new(make_beefy_ids(&all_peers), 0).unwrap(); + let session_len = 5; + let min_block_delta = 5; + + let mut net = BeefyTestNet::new(4); + + // Alice, Bob, Charlie start first and make progress through voting. + let api = Arc::new(four_validators::TestApi {}); + let fast_peers = [BeefyKeyring::Alice, BeefyKeyring::Bob, BeefyKeyring::Charlie]; + let voting_peers = + fast_peers.iter().enumerate().map(|(id, key)| (id, key, api.clone())).collect(); + runtime.spawn(initialize_beefy(&mut net, voting_peers, min_block_delta)); + + // Dave will start late and have to catch up using on-demand justification requests (since + // in this test there is no block import queue to automatically import justifications). + let dave = vec![(3, &BeefyKeyring::Dave, api)]; + // Instantiate but don't run Dave, yet. + let dave_task = initialize_beefy(&mut net, dave, min_block_delta); + let dave_index = 3; + + // push 30 blocks + net.generate_blocks_and_sync(30, session_len, &validator_set, false); + + let fast_peers = fast_peers.into_iter().enumerate(); + let net = Arc::new(Mutex::new(net)); + // With 3 active voters and one inactive, consensus should happen and blocks BEEFY-finalized. + // Need to finalize at least one block in each session, choose randomly. + finalize_block_and_wait_for_beefy( + &net, + fast_peers.clone(), + &mut runtime, + &[1, 6, 10, 17, 24], + &[1, 5, 10, 15, 20], + ); + + // Spawn Dave, he's now way behind voting and can only catch up through on-demand justif sync. + runtime.spawn(dave_task); + // give Dave a chance to spawn and init. + run_for(Duration::from_millis(400), &net, &mut runtime); + + let (dave_best_blocks, _) = + get_beefy_streams(&mut net.lock(), [(dave_index, BeefyKeyring::Dave)].into_iter()); + net.lock() + .peer(dave_index) + .client() + .as_client() + .finalize_block(BlockId::number(1), None) + .unwrap(); + // Give Dave task some cpu cycles to process the finality notification, + run_for(Duration::from_millis(100), &net, &mut runtime); + // freshly spun up Dave now needs to listen for gossip to figure out the state of his peers. + + // Have the other peers do some gossip so Dave finds out about their progress. + finalize_block_and_wait_for_beefy(&net, fast_peers, &mut runtime, &[25], &[25]); + + // Now verify Dave successfully finalized #1 (through on-demand justification request). + wait_for_best_beefy_blocks(dave_best_blocks, &net, &mut runtime, &[1]); + + // Give Dave all tasks some cpu cycles to burn through their events queues, + run_for(Duration::from_millis(100), &net, &mut runtime); + // then verify Dave catches up through on-demand justification requests. + finalize_block_and_wait_for_beefy( + &net, + [(dave_index, BeefyKeyring::Dave)].into_iter(), + &mut runtime, + &[6, 10, 17, 24, 26], + &[5, 10, 15, 20, 25], + ); + + let all_peers = all_peers.into_iter().enumerate(); + // Now that Dave has caught up, sanity check voting works for all of them. + finalize_block_and_wait_for_beefy(&net, all_peers, &mut runtime, &[30], &[30]); +} diff --git a/client/beefy/src/worker.rs b/client/beefy/src/worker.rs index a65967f557708..681187f0f0f8d 100644 --- a/client/beefy/src/worker.rs +++ b/client/beefy/src/worker.rs @@ -435,7 +435,10 @@ where let block_num = signed_commitment.commitment.block_number; let best_grandpa = *self.best_grandpa_block_header.number(); match self.voting_oracle.triage_round(block_num, best_grandpa)? { - RoundAction::Process => self.finalize(justification)?, + RoundAction::Process => { + debug!(target: "beefy", "🥩 Process justification for round: {:?}.", block_num); + self.finalize(justification)? + }, RoundAction::Enqueue => { debug!(target: "beefy", "🥩 Buffer justification for round: {:?}.", block_num); self.pending_justifications.entry(block_num).or_insert(justification); @@ -876,7 +879,7 @@ where debug!(target: "beefy", "🥩 {}", err); } - // Don't bother voting during major sync. + // Don't bother voting or requesting justifications during major sync. if !self.network.is_major_syncing() { // If the current target is a mandatory block, // make sure there's also an on-demand justification request out for it. @@ -1341,13 +1344,15 @@ pub(crate) mod tests { #[test] fn should_finalize_correctly() { - let keys = &[Keyring::Alice]; - let validator_set = ValidatorSet::new(make_beefy_ids(keys), 0).unwrap(); + let keys = [Keyring::Alice]; + let validator_set = ValidatorSet::new(make_beefy_ids(&keys), 0).unwrap(); let mut net = BeefyTestNet::new(1); let backend = net.peer(0).client().as_backend(); let mut worker = create_beefy_worker(&net.peer(0), &keys[0], 1); - let (mut best_block_streams, mut finality_proofs) = get_beefy_streams(&mut net, keys); + let keys = keys.iter().cloned().enumerate(); + let (mut best_block_streams, mut finality_proofs) = + get_beefy_streams(&mut net, keys.clone()); let mut best_block_stream = best_block_streams.drain(..).next().unwrap(); let mut finality_proof = finality_proofs.drain(..).next().unwrap(); @@ -1369,7 +1374,8 @@ pub(crate) mod tests { })); // unknown hash for block #1 - let (mut best_block_streams, mut finality_proofs) = get_beefy_streams(&mut net, keys); + let (mut best_block_streams, mut finality_proofs) = + get_beefy_streams(&mut net, keys.clone()); let mut best_block_stream = best_block_streams.drain(..).next().unwrap(); let mut finality_proof = finality_proofs.drain(..).next().unwrap(); let justif = create_finality_proof(1); From 3cb39f78dff9f1abc76bdc35419424679333c1bb Mon Sep 17 00:00:00 2001 From: acatangiu Date: Fri, 16 Sep 2022 16:34:01 +0300 Subject: [PATCH 18/31] client/beefy: tweak main loop event processing order --- client/beefy/src/worker.rs | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/client/beefy/src/worker.rs b/client/beefy/src/worker.rs index 681187f0f0f8d..f6020607cc6ef 100644 --- a/client/beefy/src/worker.rs +++ b/client/beefy/src/worker.rs @@ -821,6 +821,22 @@ where // The branches below only change 'state', actual voting happen afterwards, // based on the new resulting 'state'. futures::select_biased! { + // Use `select_biased!` to prioritize order below. + // Make sure to pump gossip engine. + _ = gossip_engine => { + error!(target: "beefy", "🥩 Gossip engine has terminated, closing worker."); + return; + }, + // Keep track of connected peers. + net_event = network_events.next() => { + if let Some(net_event) = net_event { + self.handle_network_event(net_event); + } else { + error!(target: "beefy", "🥩 Network events stream terminated, closing worker."); + return; + } + }, + // Process finality notifications first since these drive the voter. notification = finality_notifications.next() => { if let Some(notification) = notification { self.handle_finality_notification(¬ification); @@ -829,6 +845,7 @@ where return; } }, + // Process incoming justifications as these can make some in-flight votes obsolete. justif = block_import_justif.next() => { if let Some(justif) = justif { // Block import justifications have already been verified to be valid @@ -841,7 +858,6 @@ where return; } }, - // TODO: join this stream's branch with the one above; how? .. ¯\_(ツ)_/¯ justif = self.on_demand_justifications.next().fuse() => { if let Some(justif) = justif { if let Err(err) = self.triage_incoming_justif(justif) { @@ -849,6 +865,7 @@ where } } }, + // Finally process incoming votes. vote = votes.next() => { if let Some(vote) = vote { // Votes have already been verified to be valid by the gossip validator. @@ -860,18 +877,6 @@ where return; } }, - net_event = network_events.next() => { - if let Some(net_event) = net_event { - self.handle_network_event(net_event); - } else { - error!(target: "beefy", "🥩 Network events stream terminated, closing worker."); - return; - } - }, - _ = gossip_engine => { - error!(target: "beefy", "🥩 Gossip engine has terminated, closing worker."); - return; - } } // Handle pending justifications and/or votes for now GRANDPA finalized blocks. From f76dd028e62863d237cd6be293ef71a9465cd0e1 Mon Sep 17 00:00:00 2001 From: acatangiu Date: Mon, 19 Sep 2022 11:51:14 +0300 Subject: [PATCH 19/31] client/beefy: run on-demand-justif-handler under same async task as voter --- client/beefy/src/lib.rs | 10 ++++++++-- client/beefy/src/tests.rs | 8 ++------ 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/client/beefy/src/lib.rs b/client/beefy/src/lib.rs index 334a3de04ca08..6c7d8b45d2e46 100644 --- a/client/beefy/src/lib.rs +++ b/client/beefy/src/lib.rs @@ -52,7 +52,10 @@ use crate::{ BeefyVersionedFinalityProofStream, }, peers::KnownPeers, - request_response::outgoing_request::OnDemandJustificationsEngine, + request_response::{ + incoming_handler::BeefyJustifsRequestHandler, + outgoing_request::OnDemandJustificationsEngine, + }, }, import::BeefyBlockImport, }; @@ -194,6 +197,8 @@ where pub prometheus_registry: Option, /// Links between the block importer, the background voter and the RPC layer. pub links: BeefyVoterLinks, + /// Handler for incoming BEEFY justifications requests from a remote peer. + pub on_demand_justifications_handler: BeefyJustifsRequestHandler, } /// Start the BEEFY gadget. @@ -217,6 +222,7 @@ where min_block_delta, prometheus_registry, links, + on_demand_justifications_handler, } = beefy_params; let BeefyNetworkParams { network, gossip_protocol_name, justifications_protocol_name, .. } = @@ -270,5 +276,5 @@ where let worker = worker::BeefyWorker::<_, _, _, _, _>::new(worker_params); - worker.run().await + futures::future::join(worker.run(), on_demand_justifications_handler.run()).await; } diff --git a/client/beefy/src/tests.rs b/client/beefy/src/tests.rs index 31da9caf02533..979b833877877 100644 --- a/client/beefy/src/tests.rs +++ b/client/beefy/src/tests.rs @@ -375,13 +375,9 @@ where links: beefy_voter_links.unwrap(), min_block_delta, prometheus_registry: None, + on_demand_justifications_handler: on_demand_justif_handler, }; - let task = async move { - futures::join!( - crate::start_beefy_gadget::<_, _, _, _, _>(beefy_params), - on_demand_justif_handler.run() - ); - }; + let task = crate::start_beefy_gadget::<_, _, _, _, _>(beefy_params); fn assert_send(_: &T) {} assert_send(&task); From 5496ea8761743141cde580605a1545ec307bbcb9 Mon Sep 17 00:00:00 2001 From: acatangiu Date: Mon, 19 Sep 2022 12:12:06 +0300 Subject: [PATCH 20/31] client/beefy: add test for known-peers --- client/beefy/src/communication/peers.rs | 55 ++++++++++++++++++- .../request_response/outgoing_request.rs | 2 +- 2 files changed, 55 insertions(+), 2 deletions(-) diff --git a/client/beefy/src/communication/peers.rs b/client/beefy/src/communication/peers.rs index 089e6c5a5bd1c..4554ff4b992ac 100644 --- a/client/beefy/src/communication/peers.rs +++ b/client/beefy/src/communication/peers.rs @@ -60,7 +60,7 @@ impl KnownPeers { } /// Return _filtered and cloned_ list of peers that have voted on `block` or higher. - pub fn peers_at_least_at_block(&self, block: NumberFor) -> VecDeque { + pub fn at_least_at_block(&self, block: NumberFor) -> VecDeque { self.live .iter() .filter_map(|(k, v)| if v.last_voted_on >= block { Some(k) } else { None }) @@ -73,3 +73,56 @@ impl KnownPeers { self.live.contains_key(peer) } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn should_track_known_peers_progress() { + let (alice, bob, charlie) = (PeerId::random(), PeerId::random(), PeerId::random()); + let mut peers = KnownPeers::::new(); + assert!(peers.live.is_empty()); + + // Alice and Bob new connected peers. + peers.add_new(alice); + peers.add_new(bob); + // 'Tracked' Bob seen voting for 5. + peers.note_vote_for(bob, 5); + // Previously unseen Charlie now seen voting for 10. + peers.note_vote_for(charlie, 10); + + assert_eq!(peers.live.len(), 3); + assert!(peers.contains(&alice)); + assert!(peers.contains(&bob)); + assert!(peers.contains(&charlie)); + + // Get peers at block >= 5 + let at_5 = peers.at_least_at_block(5); + // Should be Bob and Charlie + assert_eq!(at_5.len(), 2); + assert!(at_5.contains(&bob)); + assert!(at_5.contains(&charlie)); + + // 'Tracked' Alice seen voting for 10. + peers.note_vote_for(alice, 10); + + // Get peers at block >= 9 + let at_9 = peers.at_least_at_block(9); + // Should be Charlie and Alice + assert_eq!(at_9.len(), 2); + assert!(at_9.contains(&charlie)); + assert!(at_9.contains(&alice)); + + // Remove Alice + peers.remove(&alice); + assert_eq!(peers.live.len(), 2); + assert!(!peers.contains(&alice)); + + // Get peers at block >= 9 + let at_9 = peers.at_least_at_block(9); + // Now should be just Charlie + assert_eq!(at_9.len(), 1); + assert!(at_9.contains(&charlie)); + } +} diff --git a/client/beefy/src/communication/request_response/outgoing_request.rs b/client/beefy/src/communication/request_response/outgoing_request.rs index efbb50057794a..2958f7fab032e 100644 --- a/client/beefy/src/communication/request_response/outgoing_request.rs +++ b/client/beefy/src/communication/request_response/outgoing_request.rs @@ -102,7 +102,7 @@ where } fn reset_peers_cache_for_block(&mut self, block: NumberFor) { - self.peers_cache = self.live_peers.lock().peers_at_least_at_block(block); + self.peers_cache = self.live_peers.lock().at_least_at_block(block); } pub fn try_next_peer(&mut self) -> Option { From 940aaf204614ced2bd9e25ce3ca63adbfb0986ec Mon Sep 17 00:00:00 2001 From: acatangiu Date: Mon, 19 Sep 2022 13:33:38 +0300 Subject: [PATCH 21/31] client/beefy: reorg request-response module --- ...andler.rs => incoming_requests_handler.rs} | 96 ++++++++++++++++- .../src/communication/request_response/mod.rs | 100 ++---------------- ...request.rs => outgoing_requests_engine.rs} | 0 client/beefy/src/lib.rs | 3 +- client/beefy/src/tests.rs | 4 +- client/beefy/src/worker.rs | 2 +- 6 files changed, 107 insertions(+), 98 deletions(-) rename client/beefy/src/communication/request_response/{incoming_handler.rs => incoming_requests_handler.rs} (52%) rename client/beefy/src/communication/request_response/{outgoing_request.rs => outgoing_requests_engine.rs} (100%) diff --git a/client/beefy/src/communication/request_response/incoming_handler.rs b/client/beefy/src/communication/request_response/incoming_requests_handler.rs similarity index 52% rename from client/beefy/src/communication/request_response/incoming_handler.rs rename to client/beefy/src/communication/request_response/incoming_requests_handler.rs index 04c644509c15a..95bfb6cd6edec 100644 --- a/client/beefy/src/communication/request_response/incoming_handler.rs +++ b/client/beefy/src/communication/request_response/incoming_requests_handler.rs @@ -17,17 +17,103 @@ //! Helper for handling (i.e. answering) BEEFY justifications requests from a remote peer. use beefy_primitives::BEEFY_ENGINE_ID; +use codec::Decode; +use futures::{ + channel::{mpsc, oneshot}, + StreamExt, +}; use log::{debug, trace}; use sc_client_api::BlockBackend; -use sc_network::{config as netconfig, config::RequestResponseConfig}; +use sc_network::{config as netconfig, config::RequestResponseConfig, PeerId, ReputationChange}; use sc_network_common::protocol::ProtocolName; use sp_runtime::{generic::BlockId, traits::Block}; use std::{marker::PhantomData, sync::Arc}; use crate::communication::request_response::{ - justif_protocol_config, Error, IncomingRequest, IncomingRequestReceiver, + on_demand_justifications_protocol_config, Error, JustificationRequest, }; +/// A request coming in, including a sender for sending responses. +#[derive(Debug)] +pub(crate) struct IncomingRequest { + /// `PeerId` of sending peer. + pub peer: PeerId, + /// The sent request. + pub payload: JustificationRequest, + /// Sender for sending response back. + pub pending_response: oneshot::Sender, +} + +impl IncomingRequest { + /// Create new `IncomingRequest`. + pub fn new( + peer: PeerId, + payload: JustificationRequest, + pending_response: oneshot::Sender, + ) -> Self { + Self { peer, payload, pending_response } + } + + /// Try building from raw network request. + /// + /// This function will fail if the request cannot be decoded and will apply passed in + /// reputation changes in that case. + /// + /// Params: + /// - The raw request to decode + /// - Reputation changes to apply for the peer in case decoding fails. + pub fn try_from_raw( + raw: netconfig::IncomingRequest, + reputation_changes: Vec, + ) -> Result { + let netconfig::IncomingRequest { payload, peer, pending_response } = raw; + let payload = match JustificationRequest::decode(&mut payload.as_ref()) { + Ok(payload) => payload, + Err(err) => { + let response = netconfig::OutgoingResponse { + result: Err(()), + reputation_changes, + sent_feedback: None, + }; + if let Err(_) = pending_response.send(response) { + return Err(Error::DecodingErrorNoReputationChange(peer, err)) + } + return Err(Error::DecodingError(peer, err)) + }, + }; + Ok(Self::new(peer, payload, pending_response)) + } +} + +/// Receiver for incoming BEEFY justifications requests. +/// +/// Takes care of decoding and handling of invalid encoded requests. +pub(crate) struct IncomingRequestReceiver { + raw: mpsc::Receiver, +} + +impl IncomingRequestReceiver { + pub fn new(inner: mpsc::Receiver) -> Self { + Self { raw: inner } + } + + /// Try to receive the next incoming request. + /// + /// Any received request will be decoded, on decoding errors the provided reputation changes + /// will be applied and an error will be reported. + pub async fn recv(&mut self, reputation_changes: F) -> Result, Error> + where + B: Block, + F: FnOnce() -> Vec, + { + let req = match self.raw.next().await { + None => return Err(Error::RequestChannelExhausted), + Some(raw) => IncomingRequest::::try_from_raw(raw, reputation_changes())?, + }; + Ok(req) + } +} + /// Handler for incoming BEEFY justifications requests from a remote peer. pub struct BeefyJustifsRequestHandler { pub(crate) request_receiver: IncomingRequestReceiver, @@ -43,21 +129,23 @@ where { /// Create a new [`BeefyJustifsRequestHandler`]. pub fn new(fork_id: Option<&str>, client: Arc) -> (Self, RequestResponseConfig) { - let genesis_hash = client + let genesis = client .block_hash(0u32.into()) .ok() .flatten() .expect("Genesis block exists; qed"); - let (request_receiver, config) = justif_protocol_config(genesis_hash, fork_id); + let (request_receiver, config) = on_demand_justifications_protocol_config(genesis, fork_id); let justif_protocol_name = config.name.clone(); (Self { request_receiver, justif_protocol_name, client, _block: PhantomData }, config) } + /// Network request-response protocol name used by this handler. pub fn protocol_name(&self) -> ProtocolName { self.justif_protocol_name.clone() } + // Sends back justification response if justification found in client backend. async fn handle_request(&self, request: IncomingRequest) -> Result<(), Error> { // TODO: validate `request.begin` and change peer reputation for invalid requests. diff --git a/client/beefy/src/communication/request_response/mod.rs b/client/beefy/src/communication/request_response/mod.rs index 80d6466dca493..c83bb9d57e91b 100644 --- a/client/beefy/src/communication/request_response/mod.rs +++ b/client/beefy/src/communication/request_response/mod.rs @@ -18,20 +18,20 @@ //! Request/response protocol for syncing BEEFY justifications. -pub mod incoming_handler; -pub(crate) mod outgoing_request; +mod incoming_requests_handler; +pub(crate) mod outgoing_requests_engine; -use futures::{ - channel::{mpsc, oneshot}, - StreamExt, -}; +pub use incoming_requests_handler::BeefyJustifsRequestHandler; + +use futures::channel::mpsc; use std::time::Duration; use codec::{Decode, Encode, Error as CodecError}; -use sc_network::{config as netconfig, config::RequestResponseConfig, PeerId, ReputationChange}; +use sc_network::{config::RequestResponseConfig, PeerId}; use sp_runtime::traits::{Block, NumberFor}; use crate::communication::beefy_protocol_name::justifications_protocol_name; +use incoming_requests_handler::IncomingRequestReceiver; // 10 seems reasonable, considering justifs are explicitly requested only // for mandatory blocks, by nodes that are syncing/catching-up. @@ -44,14 +44,16 @@ const JUSTIF_REQUEST_TIMEOUT: Duration = Duration::from_secs(3); /// /// Returns a receiver for messages received on this protocol and the requested /// `ProtocolConfig`. -pub(crate) fn justif_protocol_config>( +/// +/// Consider using [`BeefyJustifsRequestHandler`] instead of this low-level function. +pub(crate) fn on_demand_justifications_protocol_config>( genesis_hash: Hash, fork_id: Option<&str>, ) -> (IncomingRequestReceiver, RequestResponseConfig) { let name = justifications_protocol_name(genesis_hash, fork_id); let fallback_names = vec![]; let (tx, rx) = mpsc::channel(JUSTIF_CHANNEL_SIZE); - let rx = IncomingRequestReceiver { raw: rx }; + let rx = IncomingRequestReceiver::new(rx); let cfg = RequestResponseConfig { name, fallback_names, @@ -71,83 +73,6 @@ pub struct JustificationRequest { pub begin: NumberFor, } -/// A request coming in, including a sender for sending responses. -#[derive(Debug)] -pub(crate) struct IncomingRequest { - /// `PeerId` of sending peer. - pub peer: PeerId, - /// The sent request. - pub payload: JustificationRequest, - /// Sender for sending response back. - pub pending_response: oneshot::Sender, -} - -impl IncomingRequest { - /// Create new `IncomingRequest`. - pub fn new( - peer: PeerId, - payload: JustificationRequest, - pending_response: oneshot::Sender, - ) -> Self { - Self { peer, payload, pending_response } - } - - /// Try building from raw network request. - /// - /// This function will fail if the request cannot be decoded and will apply passed in - /// reputation changes in that case. - /// - /// Params: - /// - The raw request to decode - /// - Reputation changes to apply for the peer in case decoding fails. - fn try_from_raw( - raw: netconfig::IncomingRequest, - reputation_changes: Vec, - ) -> std::result::Result { - let netconfig::IncomingRequest { payload, peer, pending_response } = raw; - let payload = match JustificationRequest::decode(&mut payload.as_ref()) { - Ok(payload) => payload, - Err(err) => { - let response = netconfig::OutgoingResponse { - result: Err(()), - reputation_changes, - sent_feedback: None, - }; - if let Err(_) = pending_response.send(response) { - return Err(Error::DecodingErrorNoReputationChange(peer, err)) - } - return Err(Error::DecodingError(peer, err)) - }, - }; - Ok(Self::new(peer, payload, pending_response)) - } -} - -/// Receiver for incoming BEEFY justifications requests. -/// -/// Takes care of decoding and handling of invalid encoded requests. -pub(crate) struct IncomingRequestReceiver { - raw: mpsc::Receiver, -} - -impl IncomingRequestReceiver { - /// Try to receive the next incoming request. - /// - /// Any received request will be decoded, on decoding errors the provided reputation changes - /// will be applied and an error will be reported. - pub async fn recv(&mut self, reputation_changes: F) -> Result> - where - B: Block, - F: FnOnce() -> Vec, - { - let req = match self.raw.next().await { - None => return Err(Error::RequestChannelExhausted), - Some(raw) => IncomingRequest::::try_from_raw(raw, reputation_changes())?, - }; - Ok(req) - } -} - #[derive(Debug, thiserror::Error)] pub enum Error { #[error(transparent)] @@ -174,6 +99,3 @@ pub enum Error { #[error("Received invalid response.")] InvalidResponse, } - -/// General result based on above `Error`. -pub type Result = std::result::Result; diff --git a/client/beefy/src/communication/request_response/outgoing_request.rs b/client/beefy/src/communication/request_response/outgoing_requests_engine.rs similarity index 100% rename from client/beefy/src/communication/request_response/outgoing_request.rs rename to client/beefy/src/communication/request_response/outgoing_requests_engine.rs diff --git a/client/beefy/src/lib.rs b/client/beefy/src/lib.rs index 6c7d8b45d2e46..7f080dfb361ca 100644 --- a/client/beefy/src/lib.rs +++ b/client/beefy/src/lib.rs @@ -53,8 +53,7 @@ use crate::{ }, peers::KnownPeers, request_response::{ - incoming_handler::BeefyJustifsRequestHandler, - outgoing_request::OnDemandJustificationsEngine, + outgoing_requests_engine::OnDemandJustificationsEngine, BeefyJustifsRequestHandler, }, }, import::BeefyBlockImport, diff --git a/client/beefy/src/tests.rs b/client/beefy/src/tests.rs index 979b833877877..21dd5a07cae4a 100644 --- a/client/beefy/src/tests.rs +++ b/client/beefy/src/tests.rs @@ -62,7 +62,7 @@ use substrate_test_runtime_client::{runtime::Header, ClientExt}; use crate::{ beefy_block_import_and_links, communication::request_response::{ - incoming_handler::BeefyJustifsRequestHandler, justif_protocol_config, + on_demand_justifications_protocol_config, BeefyJustifsRequestHandler, }, gossip_protocol_name, justification::*, @@ -117,7 +117,7 @@ impl BeefyTestNet { let mut net = BeefyTestNet { peers: Vec::with_capacity(n_authority) }; for i in 0..n_authority { - let (rx, cfg) = justif_protocol_config(GENESIS_HASH, None); + let (rx, cfg) = on_demand_justifications_protocol_config(GENESIS_HASH, None); let justif_protocol_name = cfg.name.clone(); net.add_authority_peer(vec![cfg]); diff --git a/client/beefy/src/worker.rs b/client/beefy/src/worker.rs index f6020607cc6ef..4c7bb593631af 100644 --- a/client/beefy/src/worker.rs +++ b/client/beefy/src/worker.rs @@ -55,7 +55,7 @@ use beefy_primitives::{ use crate::{ communication::{ gossip::{topic, GossipValidator}, - request_response::outgoing_request::OnDemandJustificationsEngine, + request_response::outgoing_requests_engine::OnDemandJustificationsEngine, }, error::Error, justification::BeefyVersionedFinalityProof, From dbc29645ac35bc00d87435c20289abb03f39d9a5 Mon Sep 17 00:00:00 2001 From: acatangiu Date: Mon, 19 Sep 2022 14:44:31 +0300 Subject: [PATCH 22/31] client/beefy: add issue references for future work todos --- client/beefy/src/communication/peers.rs | 3 +++ .../request_response/incoming_requests_handler.rs | 4 ++-- .../request_response/outgoing_requests_engine.rs | 4 +++- 3 files changed, 8 insertions(+), 3 deletions(-) diff --git a/client/beefy/src/communication/peers.rs b/client/beefy/src/communication/peers.rs index 4554ff4b992ac..4027465480369 100644 --- a/client/beefy/src/communication/peers.rs +++ b/client/beefy/src/communication/peers.rs @@ -18,6 +18,9 @@ //! Logic for keeping track of BEEFY peers. +// TODO (issue #12296): replace this naive peer tracking with generic one that infers data +// from multiple network protocols. + use sc_network::PeerId; use sp_runtime::traits::{Block, NumberFor, Zero}; use std::collections::{HashMap, VecDeque}; diff --git a/client/beefy/src/communication/request_response/incoming_requests_handler.rs b/client/beefy/src/communication/request_response/incoming_requests_handler.rs index 95bfb6cd6edec..c5a485e89a532 100644 --- a/client/beefy/src/communication/request_response/incoming_requests_handler.rs +++ b/client/beefy/src/communication/request_response/incoming_requests_handler.rs @@ -147,7 +147,7 @@ where // Sends back justification response if justification found in client backend. async fn handle_request(&self, request: IncomingRequest) -> Result<(), Error> { - // TODO: validate `request.begin` and change peer reputation for invalid requests. + // TODO (issue #12293): validate `request` and change peer reputation for invalid requests. let maybe_encoded_proof = self .client @@ -182,7 +182,7 @@ where ) }, Err(e) => { - // TODO: handle reputation changes here + // TODO (issue #12293): apply reputation changes here based on error type. debug!( target: "beefy::sync", "🥩 Failed to handle BEEFY justification request from {:?}: {}", peer, e, diff --git a/client/beefy/src/communication/request_response/outgoing_requests_engine.rs b/client/beefy/src/communication/request_response/outgoing_requests_engine.rs index 2958f7fab032e..c885e8a790c85 100644 --- a/client/beefy/src/communication/request_response/outgoing_requests_engine.rs +++ b/client/beefy/src/communication/request_response/outgoing_requests_engine.rs @@ -102,10 +102,12 @@ where } fn reset_peers_cache_for_block(&mut self, block: NumberFor) { + // TODO (issue #12296): replace peer selection with generic one that involves all protocols. self.peers_cache = self.live_peers.lock().at_least_at_block(block); } - pub fn try_next_peer(&mut self) -> Option { + fn try_next_peer(&mut self) -> Option { + // TODO (issue #12296): replace peer selection with generic one that involves all protocols. let live = self.live_peers.lock(); while let Some(peer) = self.peers_cache.pop_front() { if live.contains(&peer) { From b69471c5ce2e0157c2e5b4e68b786d5548e42c94 Mon Sep 17 00:00:00 2001 From: acatangiu Date: Tue, 20 Sep 2022 18:40:09 +0300 Subject: [PATCH 23/31] client/beefy: consolidate on-demand-justifications engine state machine Signed-off-by: acatangiu --- .../outgoing_requests_engine.rs | 72 ++++++------------- client/beefy/src/worker.rs | 14 ++-- 2 files changed, 29 insertions(+), 57 deletions(-) diff --git a/client/beefy/src/communication/request_response/outgoing_requests_engine.rs b/client/beefy/src/communication/request_response/outgoing_requests_engine.rs index c885e8a790c85..e0d8deb443fbd 100644 --- a/client/beefy/src/communication/request_response/outgoing_requests_engine.rs +++ b/client/beefy/src/communication/request_response/outgoing_requests_engine.rs @@ -22,8 +22,7 @@ use beefy_primitives::{crypto::AuthorityId, BeefyApi, ValidatorSet}; use codec::Encode; use futures::{ channel::{oneshot, oneshot::Canceled}, - future::Either, - stream::{FuturesUnordered, StreamExt}, + stream::{self, StreamExt}, }; use log::{debug, error, warn}; use parking_lot::Mutex; @@ -51,8 +50,8 @@ type Response = Result, RequestFailure>; type ResponseReceiver = oneshot::Receiver; enum State { - Idle, - AwaitingResponse(PeerId, NumberFor), + Idle(stream::Pending>), + AwaitingResponse(PeerId, NumberFor, stream::Once), } pub struct OnDemandJustificationsEngine { @@ -64,9 +63,6 @@ pub struct OnDemandJustificationsEngine { peers_cache: VecDeque, state: State, - engine: FuturesUnordered< - Either>, ResponseReceiver>, - >, } impl OnDemandJustificationsEngine @@ -82,25 +78,16 @@ where protocol_name: ProtocolName, live_peers: Arc>>, ) -> Self { - let engine = FuturesUnordered::new(); - engine.push(Either::Left(std::future::pending::<_>())); Self { network, runtime, protocol_name, live_peers, peers_cache: VecDeque::new(), - state: State::Idle, - engine, + state: State::Idle(stream::pending()), } } - fn reset_state(&mut self) { - self.state = State::Idle; - self.engine = FuturesUnordered::new(); - self.engine.push(Either::Left(std::future::pending::<_>())); - } - fn reset_peers_cache_for_block(&mut self, block: NumberFor) { // TODO (issue #12296): replace peer selection with generic one that involves all protocols. self.peers_cache = self.live_peers.lock().at_least_at_block(block); @@ -132,16 +119,15 @@ where IfDisconnected::ImmediateError, ); - self.engine.push(Either::Right(rx)); - self.state = State::AwaitingResponse(peer, block); + self.state = State::AwaitingResponse(peer, block, stream::once(rx)); } /// If no other request is in progress, start new justification request for `block`. pub fn request(&mut self, block: NumberFor) { // ignore new requests while there's already one pending match &self.state { - State::AwaitingResponse(_, _) => return, - State::Idle => (), + State::AwaitingResponse(_, _, _) => return, + State::Idle(_) => (), } self.reset_peers_cache_for_block(block); @@ -157,13 +143,13 @@ where /// Cancel any pending request for block numbers smaller or equal to `block`. pub fn cancel_requests_older_than(&mut self, block: NumberFor) { match &self.state { - State::AwaitingResponse(_, number) if *number <= block => { + State::AwaitingResponse(_, number, _) if *number <= block => { debug!( target: "beefy::sync", "🥩 cancel pending request for justification #{:?}", number ); - self.reset_state(); + self.state = State::Idle(stream::pending()); }, _ => (), } @@ -174,8 +160,8 @@ where peer: PeerId, block: NumberFor, validator_set: &ValidatorSet, - response: std::result::Result, - ) -> std::result::Result, Error> { + response: Result, + ) -> Result, Error> { response .map_err(|e| { debug!( @@ -208,34 +194,20 @@ where } pub async fn next(&mut self) -> Option> { - // The `engine` contains at the very least a `Pending` future (future that never finishes). - // This "blocks" until: - // - we have a [ResponseReceiver] added to the engine, and - // - we also get a [Response] on it. - let resp = self - .engine - .next() - .await - .expect("Engine will never end because of inner `Pending` future, qed."); - - let (peer, block) = match self.state { - State::AwaitingResponse(peer, block) => (peer, block), - // Programming error to have active [ResponseReceiver]s in the engine with no pending - // requests. Just log an error for now. - State::Idle => { - error!( - target: "beefy::sync", - "🥩 unexpected response received in 'Idle' state: {:?}", - resp - ); - self.reset_state(); + let (peer, block, resp) = match &mut self.state { + State::Idle(pending) => { + let _ = pending.next().await; + // This never happens since 'stream::pending' never generates any items. return None }, + State::AwaitingResponse(peer, block, receiver) => { + let resp = receiver.next().await?; + (*peer, *block, resp) + }, }; - // We received the awaited response. All response processing success and error cases - // ultimately move the engine in `Idle` state, so just do it now to make things simple - // and make sure we don't miss any error short-circuit. - self.reset_state(); + // We received the awaited response. Our 'stream::once()' receiver will never generate any + // other response, meaning we're done with current state. Move the engine to `State::Idle`. + self.state = State::Idle(stream::pending()); let block_id = BlockId::number(block); let validator_set = self diff --git a/client/beefy/src/worker.rs b/client/beefy/src/worker.rs index 4c7bb593631af..d8b0f7b51a4dc 100644 --- a/client/beefy/src/worker.rs +++ b/client/beefy/src/worker.rs @@ -846,23 +846,23 @@ where } }, // Process incoming justifications as these can make some in-flight votes obsolete. - justif = block_import_justif.next() => { + justif = self.on_demand_justifications.next().fuse() => { if let Some(justif) = justif { - // Block import justifications have already been verified to be valid - // by `BeefyBlockImport`. if let Err(err) = self.triage_incoming_justif(justif) { debug!(target: "beefy", "🥩 {}", err); } - } else { - error!(target: "beefy", "🥩 Block import stream terminated, closing worker."); - return; } }, - justif = self.on_demand_justifications.next().fuse() => { + justif = block_import_justif.next() => { if let Some(justif) = justif { + // Block import justifications have already been verified to be valid + // by `BeefyBlockImport`. if let Err(err) = self.triage_incoming_justif(justif) { debug!(target: "beefy", "🥩 {}", err); } + } else { + error!(target: "beefy", "🥩 Block import stream terminated, closing worker."); + return; } }, // Finally process incoming votes. From 68854a866da83e88caa8c939a3b69cc5ec2c470e Mon Sep 17 00:00:00 2001 From: Adrian Catangiu Date: Wed, 21 Sep 2022 15:18:34 +0300 Subject: [PATCH 24/31] client/beefy: fix for polkadot companion --- .../request_response/incoming_requests_handler.rs | 14 +++++++------- client/beefy/src/lib.rs | 2 +- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/client/beefy/src/communication/request_response/incoming_requests_handler.rs b/client/beefy/src/communication/request_response/incoming_requests_handler.rs index c5a485e89a532..8d51f356c385e 100644 --- a/client/beefy/src/communication/request_response/incoming_requests_handler.rs +++ b/client/beefy/src/communication/request_response/incoming_requests_handler.rs @@ -128,13 +128,13 @@ where Client: BlockBackend + Send + Sync, { /// Create a new [`BeefyJustifsRequestHandler`]. - pub fn new(fork_id: Option<&str>, client: Arc) -> (Self, RequestResponseConfig) { - let genesis = client - .block_hash(0u32.into()) - .ok() - .flatten() - .expect("Genesis block exists; qed"); - let (request_receiver, config) = on_demand_justifications_protocol_config(genesis, fork_id); + pub fn new>( + genesis_hash: Hash, + fork_id: Option<&str>, + client: Arc, + ) -> (Self, RequestResponseConfig) { + let (request_receiver, config) = + on_demand_justifications_protocol_config(genesis_hash, fork_id); let justif_protocol_name = config.name.clone(); (Self { request_receiver, justif_protocol_name, client, _block: PhantomData }, config) diff --git a/client/beefy/src/lib.rs b/client/beefy/src/lib.rs index 7f080dfb361ca..269e3bb0d8cfa 100644 --- a/client/beefy/src/lib.rs +++ b/client/beefy/src/lib.rs @@ -167,7 +167,7 @@ where /// [`beefy_protocol_name::justifications_protocol_name`]. pub justifications_protocol_name: ProtocolName, - _phantom: PhantomData, + pub _phantom: PhantomData, } /// BEEFY gadget initialization parameters. From 683ddc5b692ae13fadde7ab3c64f0be41384f119 Mon Sep 17 00:00:00 2001 From: acatangiu Date: Thu, 22 Sep 2022 13:52:27 +0300 Subject: [PATCH 25/31] client/beefy: implement review suggestions --- client/beefy/src/communication/peers.rs | 2 +- .../request_response/incoming_requests_handler.rs | 4 ++-- .../request_response/outgoing_requests_engine.rs | 9 ++++----- client/beefy/src/lib.rs | 8 ++++---- client/beefy/src/worker.rs | 4 ++-- 5 files changed, 13 insertions(+), 14 deletions(-) diff --git a/client/beefy/src/communication/peers.rs b/client/beefy/src/communication/peers.rs index 4027465480369..0e20a0f4e0ff6 100644 --- a/client/beefy/src/communication/peers.rs +++ b/client/beefy/src/communication/peers.rs @@ -66,7 +66,7 @@ impl KnownPeers { pub fn at_least_at_block(&self, block: NumberFor) -> VecDeque { self.live .iter() - .filter_map(|(k, v)| if v.last_voted_on >= block { Some(k) } else { None }) + .filter_map(|(k, v)| (v.last_voted_on >= block).then_some(k)) .cloned() .collect() } diff --git a/client/beefy/src/communication/request_response/incoming_requests_handler.rs b/client/beefy/src/communication/request_response/incoming_requests_handler.rs index 8d51f356c385e..716cb0e547130 100644 --- a/client/beefy/src/communication/request_response/incoming_requests_handler.rs +++ b/client/beefy/src/communication/request_response/incoming_requests_handler.rs @@ -146,7 +146,7 @@ where } // Sends back justification response if justification found in client backend. - async fn handle_request(&self, request: IncomingRequest) -> Result<(), Error> { + fn handle_request(&self, request: IncomingRequest) -> Result<(), Error> { // TODO (issue #12293): validate `request` and change peer reputation for invalid requests. let maybe_encoded_proof = self @@ -174,7 +174,7 @@ where while let Ok(request) = self.request_receiver.recv(|| vec![]).await { let peer = request.peer; - match self.handle_request(request).await { + match self.handle_request(request) { Ok(()) => { debug!( target: "beefy::sync", diff --git a/client/beefy/src/communication/request_response/outgoing_requests_engine.rs b/client/beefy/src/communication/request_response/outgoing_requests_engine.rs index e0d8deb443fbd..e22958e19cd2e 100644 --- a/client/beefy/src/communication/request_response/outgoing_requests_engine.rs +++ b/client/beefy/src/communication/request_response/outgoing_requests_engine.rs @@ -54,8 +54,8 @@ enum State { AwaitingResponse(PeerId, NumberFor, stream::Once), } -pub struct OnDemandJustificationsEngine { - network: N, +pub struct OnDemandJustificationsEngine { + network: Arc, runtime: Arc, protocol_name: ProtocolName, @@ -65,15 +65,14 @@ pub struct OnDemandJustificationsEngine { state: State, } -impl OnDemandJustificationsEngine +impl OnDemandJustificationsEngine where B: Block, - N: NetworkRequest, R: ProvideRuntimeApi, R::Api: BeefyApi, { pub fn new( - network: N, + network: Arc, runtime: Arc, protocol_name: ProtocolName, live_peers: Arc>>, diff --git a/client/beefy/src/lib.rs b/client/beefy/src/lib.rs index 269e3bb0d8cfa..22e77c693e134 100644 --- a/client/beefy/src/lib.rs +++ b/client/beefy/src/lib.rs @@ -156,10 +156,10 @@ where pub struct BeefyNetworkParams where B: Block, - N: GossipNetwork + NetworkRequest + Clone + SyncOracle + Send + Sync + 'static, + N: GossipNetwork + NetworkRequest + SyncOracle + Send + Sync + 'static, { /// Network implementing gossip, requests and sync-oracle. - pub network: N, + pub network: Arc, /// Chain specific BEEFY gossip protocol name. See /// [`beefy_protocol_name::gossip_protocol_name`]. pub gossip_protocol_name: ProtocolName, @@ -178,7 +178,7 @@ where C: Client, R: ProvideRuntimeApi, R::Api: BeefyApi + MmrApi, - N: GossipNetwork + NetworkRequest + Clone + SyncOracle + Send + Sync + 'static, + N: GossipNetwork + NetworkRequest + SyncOracle + Send + Sync + 'static, { /// BEEFY client pub client: Arc, @@ -210,7 +210,7 @@ where C: Client + BlockBackend, R: ProvideRuntimeApi, R::Api: BeefyApi + MmrApi, - N: GossipNetwork + NetworkRequest + Clone + SyncOracle + Send + Sync + 'static, + N: GossipNetwork + NetworkRequest + SyncOracle + Send + Sync + 'static, { let BeefyParams { client, diff --git a/client/beefy/src/worker.rs b/client/beefy/src/worker.rs index d8b0f7b51a4dc..832b43315515f 100644 --- a/client/beefy/src/worker.rs +++ b/client/beefy/src/worker.rs @@ -203,7 +203,7 @@ pub(crate) struct WorkerParams { pub known_peers: Arc>>, pub gossip_engine: GossipEngine, pub gossip_validator: Arc>, - pub on_demand_justifications: OnDemandJustificationsEngine, + pub on_demand_justifications: OnDemandJustificationsEngine, pub links: BeefyVoterLinks, pub metrics: Option, pub min_block_delta: u32, @@ -222,7 +222,7 @@ pub(crate) struct BeefyWorker { known_peers: Arc>>, gossip_engine: GossipEngine, gossip_validator: Arc>, - on_demand_justifications: OnDemandJustificationsEngine, + on_demand_justifications: OnDemandJustificationsEngine, // channels /// Links between the block importer, the background voter and the RPC layer. From 200cdf01b10fe3eba3be4a70a514d3f001e338e8 Mon Sep 17 00:00:00 2001 From: acatangiu Date: Fri, 23 Sep 2022 16:05:11 +0300 Subject: [PATCH 26/31] cargo fmt and clippy --- client/beefy/rpc/src/lib.rs | 4 +++- .../request_response/incoming_requests_handler.rs | 3 +-- client/network/test/src/lib.rs | 9 +++++---- 3 files changed, 9 insertions(+), 7 deletions(-) diff --git a/client/beefy/rpc/src/lib.rs b/client/beefy/rpc/src/lib.rs index 77a1c5d751be9..4c4585bca9b11 100644 --- a/client/beefy/rpc/src/lib.rs +++ b/client/beefy/rpc/src/lib.rs @@ -35,7 +35,9 @@ use jsonrpsee::{ }; use log::warn; -use beefy_gadget::communication::notification::{BeefyBestBlockStream, BeefyVersionedFinalityProofStream}; +use beefy_gadget::communication::notification::{ + BeefyBestBlockStream, BeefyVersionedFinalityProofStream, +}; mod notification; diff --git a/client/beefy/src/communication/request_response/incoming_requests_handler.rs b/client/beefy/src/communication/request_response/incoming_requests_handler.rs index 716cb0e547130..c0910a60fba3b 100644 --- a/client/beefy/src/communication/request_response/incoming_requests_handler.rs +++ b/client/beefy/src/communication/request_response/incoming_requests_handler.rs @@ -153,8 +153,7 @@ where .client .justifications(&BlockId::Number(request.payload.begin)) .map_err(Error::Client)? - .map(|justifs| justifs.get(BEEFY_ENGINE_ID).cloned()) - .flatten() + .and_then(|justifs| justifs.get(BEEFY_ENGINE_ID).cloned()) // No BEEFY justification present. .ok_or(()); diff --git a/client/network/test/src/lib.rs b/client/network/test/src/lib.rs index 126e1ad3bcdb6..6a2e73267e235 100644 --- a/client/network/test/src/lib.rs +++ b/client/network/test/src/lib.rs @@ -50,12 +50,11 @@ use sc_consensus::{ pub use sc_network::config::EmptyTransactionPool; use sc_network::{ config::{ - NetworkConfiguration, NonDefaultSetConfig, NonReservedPeerMode, Role, SyncMode, - TransportConfig, + NetworkConfiguration, NonDefaultSetConfig, NonReservedPeerMode, RequestResponseConfig, + Role, SyncMode, TransportConfig, }, Multiaddr, NetworkService, NetworkWorker, }; -use sc_network::config::RequestResponseConfig; use sc_network_common::{ config::{MultiaddrWithPeerId, ProtocolId}, protocol::ProtocolName, @@ -795,7 +794,9 @@ where network_config.transport = TransportConfig::MemoryOnly; network_config.listen_addresses = vec![listen_addr.clone()]; network_config.allow_non_globals_in_dht = true; - network_config.request_response_protocols.extend(config.request_response_protocols); + network_config + .request_response_protocols + .extend(config.request_response_protocols); network_config.extra_sets = config .notifications_protocols .into_iter() From 447dc2db743bceeee3fc94d8f36027c3739b081f Mon Sep 17 00:00:00 2001 From: Adrian Catangiu Date: Mon, 26 Sep 2022 14:10:10 +0300 Subject: [PATCH 27/31] fix merge damage --- client/beefy/rpc/src/lib.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/client/beefy/rpc/src/lib.rs b/client/beefy/rpc/src/lib.rs index 4c4585bca9b11..0af474116e6d0 100644 --- a/client/beefy/rpc/src/lib.rs +++ b/client/beefy/rpc/src/lib.rs @@ -166,6 +166,10 @@ where mod tests { use super::*; + use beefy_gadget::{ + communication::notification::BeefyVersionedFinalityProofSender, + justification::BeefyVersionedFinalityProof, + }; use beefy_primitives::{known_payload_ids, Payload, SignedCommitment}; use codec::{Decode, Encode}; use jsonrpsee::{types::EmptyParams, RpcModule}; From 65cc99ecd2e8f7ed4e26a6e71f4108f78aeaecfa Mon Sep 17 00:00:00 2001 From: Adrian Catangiu Date: Mon, 26 Sep 2022 15:40:34 +0300 Subject: [PATCH 28/31] fix rust-doc --- client/beefy/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/client/beefy/src/lib.rs b/client/beefy/src/lib.rs index 22e77c693e134..7407f101e99a5 100644 --- a/client/beefy/src/lib.rs +++ b/client/beefy/src/lib.rs @@ -161,10 +161,10 @@ where /// Network implementing gossip, requests and sync-oracle. pub network: Arc, /// Chain specific BEEFY gossip protocol name. See - /// [`beefy_protocol_name::gossip_protocol_name`]. + /// [`communication::beefy_protocol_name::gossip_protocol_name`]. pub gossip_protocol_name: ProtocolName, /// Chain specific BEEFY on-demand justifications protocol name. See - /// [`beefy_protocol_name::justifications_protocol_name`]. + /// [`communication::beefy_protocol_name::justifications_protocol_name`]. pub justifications_protocol_name: ProtocolName, pub _phantom: PhantomData, From c68cbef7cdbff5f076c422e5df6e4ed88e8916e6 Mon Sep 17 00:00:00 2001 From: Adrian Catangiu Date: Tue, 27 Sep 2022 15:15:10 +0300 Subject: [PATCH 29/31] fix merge damage --- client/beefy/src/communication/mod.rs | 7 ++++--- client/network/test/src/lib.rs | 5 +---- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/client/beefy/src/communication/mod.rs b/client/beefy/src/communication/mod.rs index bd6cd18d2cf25..0720f0fdc63ed 100644 --- a/client/beefy/src/communication/mod.rs +++ b/client/beefy/src/communication/mod.rs @@ -66,12 +66,13 @@ pub(crate) mod beefy_protocol_name { } /// Returns the configuration value to put in -/// [`sc_network::config::NetworkConfiguration::extra_sets`]. +/// [`sc_network_common::config::NetworkConfiguration::extra_sets`]. /// For standard protocol name see [`beefy_protocol_name::gossip_protocol_name`]. pub fn beefy_peers_set_config( gossip_protocol_name: sc_network::ProtocolName, -) -> sc_network::config::NonDefaultSetConfig { - let mut cfg = sc_network::config::NonDefaultSetConfig::new(gossip_protocol_name, 1024 * 1024); +) -> sc_network_common::config::NonDefaultSetConfig { + let mut cfg = + sc_network_common::config::NonDefaultSetConfig::new(gossip_protocol_name, 1024 * 1024); cfg.allow_non_reserved(25, 25); cfg.add_fallback_names(beefy_protocol_name::LEGACY_NAMES.iter().map(|&n| n.into()).collect()); diff --git a/client/network/test/src/lib.rs b/client/network/test/src/lib.rs index f7ae7d9dffd95..9d5abf98ceff0 100644 --- a/client/network/test/src/lib.rs +++ b/client/network/test/src/lib.rs @@ -48,10 +48,7 @@ use sc_consensus::{ Verifier, }; use sc_network::{ - config::{ - NetworkConfiguration, NonDefaultSetConfig, NonReservedPeerMode, RequestResponseConfig, - Role, SyncMode, TransportConfig, - }, + config::{NetworkConfiguration, RequestResponseConfig, Role, SyncMode}, Multiaddr, NetworkService, NetworkWorker, }; use sc_network_common::{ From 9a6bccdbd378444b523a16fe06a4b6d9e8ec3948 Mon Sep 17 00:00:00 2001 From: Adrian Catangiu Date: Tue, 27 Sep 2022 15:44:49 +0300 Subject: [PATCH 30/31] fix merge damage --- client/beefy/src/communication/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/beefy/src/communication/mod.rs b/client/beefy/src/communication/mod.rs index 0720f0fdc63ed..8f52b63ef9b73 100644 --- a/client/beefy/src/communication/mod.rs +++ b/client/beefy/src/communication/mod.rs @@ -66,7 +66,7 @@ pub(crate) mod beefy_protocol_name { } /// Returns the configuration value to put in -/// [`sc_network_common::config::NetworkConfiguration::extra_sets`]. +/// [`sc_network::config::NetworkConfiguration::extra_sets`]. /// For standard protocol name see [`beefy_protocol_name::gossip_protocol_name`]. pub fn beefy_peers_set_config( gossip_protocol_name: sc_network::ProtocolName, From b904741ee799b63d2e3fd78cc637670d37744d12 Mon Sep 17 00:00:00 2001 From: Adrian Catangiu Date: Mon, 3 Oct 2022 14:41:56 +0300 Subject: [PATCH 31/31] client/beefy: add test for justif proto name --- client/beefy/src/communication/mod.rs | 29 ++++++++++++++++++--------- 1 file changed, 20 insertions(+), 9 deletions(-) diff --git a/client/beefy/src/communication/mod.rs b/client/beefy/src/communication/mod.rs index 8f52b63ef9b73..93646677c0ecd 100644 --- a/client/beefy/src/communication/mod.rs +++ b/client/beefy/src/communication/mod.rs @@ -86,22 +86,33 @@ mod tests { use sp_core::H256; #[test] - fn beefy_gossip_protocol_name() { - use beefy_protocol_name::gossip_protocol_name; + fn beefy_protocols_names() { + use beefy_protocol_name::{gossip_protocol_name, justifications_protocol_name}; // Create protocol name using random genesis hash. let genesis_hash = H256::random(); - let expected = format!("/{}/beefy/1", array_bytes::bytes2hex("", genesis_hash.as_ref())); - let proto_name = gossip_protocol_name(&genesis_hash, None); - assert_eq!(proto_name.to_string(), expected); + let genesis_hex = array_bytes::bytes2hex("", genesis_hash.as_ref()); + + let expected_gossip_name = format!("/{}/beefy/1", genesis_hex); + let gossip_proto_name = gossip_protocol_name(&genesis_hash, None); + assert_eq!(gossip_proto_name.to_string(), expected_gossip_name); + + let expected_justif_name = format!("/{}/beefy/justifications/1", genesis_hex); + let justif_proto_name = justifications_protocol_name(&genesis_hash, None); + assert_eq!(justif_proto_name.to_string(), expected_justif_name); // Create protocol name using hardcoded genesis hash. Verify exact representation. let genesis_hash = [ 50, 4, 60, 123, 58, 106, 216, 246, 194, 188, 139, 193, 33, 212, 202, 171, 9, 55, 123, 94, 8, 43, 12, 251, 187, 57, 173, 19, 188, 74, 205, 147, ]; - let expected = - "/32043c7b3a6ad8f6c2bc8bc121d4caab09377b5e082b0cfbbb39ad13bc4acd93/beefy/1".to_string(); - let proto_name = gossip_protocol_name(&genesis_hash, None); - assert_eq!(proto_name.to_string(), expected); + let genesis_hex = "32043c7b3a6ad8f6c2bc8bc121d4caab09377b5e082b0cfbbb39ad13bc4acd93"; + + let expected_gossip_name = format!("/{}/beefy/1", genesis_hex); + let gossip_proto_name = gossip_protocol_name(&genesis_hash, None); + assert_eq!(gossip_proto_name.to_string(), expected_gossip_name); + + let expected_justif_name = format!("/{}/beefy/justifications/1", genesis_hex); + let justif_proto_name = justifications_protocol_name(&genesis_hash, None); + assert_eq!(justif_proto_name.to_string(), expected_justif_name); } }