From 31cb1ad5931fd6fb0cf8f95bd94434ad07c2e5f6 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Tue, 7 Jul 2020 11:03:09 +0200 Subject: [PATCH 01/45] feat bitfield distribution --- Cargo.lock | 11 ++ Cargo.toml | 1 + .../bitfield-distribution/Cargo.toml | 12 ++ .../bitfield-distribution/src/lib.rs | 167 ++++++++++++++++++ 4 files changed, 191 insertions(+) create mode 100644 node/availability/bitfield-distribution/Cargo.toml create mode 100644 node/availability/bitfield-distribution/src/lib.rs diff --git a/Cargo.lock b/Cargo.lock index 7f60ff2254f4..33e94238f30c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4310,6 +4310,17 @@ dependencies = [ "tempfile", ] +[[package]] +name = "polkadot-availability-bitfield-distribution" +version = "0.1.0" +dependencies = [ + "futures 0.3.5", + "log 0.4.8", + "polkadot-network-bridge", + "polkadot-node-subsystem", + "polkadot-primitives", +] + [[package]] name = "polkadot-availability-store" version = "0.8.14" diff --git a/Cargo.toml b/Cargo.toml index b6d1aa53eaa2..3b4886fd2b91 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -43,6 +43,7 @@ members = [ "service", "validation", + "node/availability/bitfield-distribution", "node/core/proposer", "node/network/bridge", "node/network/pov-distribution", diff --git a/node/availability/bitfield-distribution/Cargo.toml b/node/availability/bitfield-distribution/Cargo.toml new file mode 100644 index 000000000000..b2fb4ff1ad4f --- /dev/null +++ b/node/availability/bitfield-distribution/Cargo.toml @@ -0,0 +1,12 @@ +[package] +name = "polkadot-availability-bitfield-distribution" +version = "0.1.0" +authors = ["Parity Technologies "] +edition = "2018" + +[dependencies] +futures = "0.3.5" +log = "0.4.8" +polkadot-primitives = { path = "../../../primitives" } +polkadot-node-subsystem = { path = "../../subsystem" } +polkadot-network-bridge = { path = "../../network/bridge" } \ No newline at end of file diff --git a/node/availability/bitfield-distribution/src/lib.rs b/node/availability/bitfield-distribution/src/lib.rs new file mode 100644 index 000000000000..3d47df7c9cee --- /dev/null +++ b/node/availability/bitfield-distribution/src/lib.rs @@ -0,0 +1,167 @@ +// Copyright 2020 Parity Technologies (UK) Ltd. +// This file is part of Polkadot. + +// Polkadot 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. + +// Polkadot 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 Polkadot. If not, see . + +//! The bitfield distribution subsystem spreading @todo . + +use bridge::NetworkBridgeMessage; +use futures::{channel::oneshot, Future}; +use node_primitives::{ProtocolId, SignedFullStatement, View}; +use polkadot_node_subsystem::{ + messages::{AllMessages, BitfieldDistributionMessage}, + OverseerSignal, SubsystemResult, +}; +use polkadot_node_subsystem::{FromOverseer, SpawnedSubsystem, Subsystem, SubsystemContext}; +use polkadot_primitives::Hash; +use std::{collections::HashMap, pin::Pin}; + +// @todo split in multiple costs +const COST_UNEXPECTED: Rep = Rep::new(-100, "Unexpected"); + +#[derive(Default, Clone)] +struct Tracker { + // track all active peers and their views + // to determine what is relevant to them + peer_views: HashMap, + + // our current view + view: View, +} + + + +fn network_update_message(n: NetworkBridgeEvent) -> AllMessages { + AllMessages::BitfieldDistribution(BitfieldDistributionMessage::NetworkBridgeUpdate(n)) +} + +pub struct BitfieldDistribution; + +impl BitfieldDistribution { + const PROTOCOL_ID: ProtocolId = *b"bitd"; + + async fn run(mut ctx: Context) -> SubsystemResult<()> + where + Context: SubsystemContext, + { + // startup: register the network protocol with the bridge. + ctx.send_message(AllMessages::NetworkBridge(NetworkBridgeMessage::RegisterEventProducer( + Self::PROTOCOL_ID, + handle_network_msg, + ))).await?; + + let mut data = Tracker::default(); + loop { + { + let x = ctx.recv().await?; + match x { + FromOverseer::Communication { msg: _ } => { + unreachable!("BitfieldDistributionMessage does not exist; qed") + } + FromOverseer::Signal(OverseerSignal::StartWork(hash)) => { + // @todo cannot work + // tracker.active_heads.insert(hash.clone(), process(&mut data, hash)); + } + FromOverseer::Signal(OverseerSignal::StopWork(hash)) => { + // could work, but see above + tracker.active_heads.remove(&hash); + } + FromOverseer::Signal(OverseerSignal::Conclude) => break, + } + } + active_jobs.retain(|_, future| future.poll().is_pending()); + } + Ok(()) + } +} + +/// Handle an incoming message +async fn process_incoming( + tracker: &mut Tracker, + message: BitfieldDistributionMessage, +) -> SubsystemResult<()> { + match message { + /// Distribute a bitfield via gossip to other validators. + BitfieldDistributionMessage::DistributeBitfield(hash, signed_availability) => { + // @todo check signature, where to get the SingingContext from? + // signed_availability.check_signature(signing_ctx, validator_id)?; + + for (peerid, view) in tracker.peer_views.filter(|(_peerid,view)| { + view.contains(hash) + }) { + // @todo verify sequential execution is ok or if spawning tasks is better + + + } + } + BitfieldDistributionMessage::NetworkBridgeUpdate(event) => { + handle_network_msg( + &mut tracker, + &mut ctx, + event, + ) + .await? + } + } + Ok(()) +} + +/// Deal with network bridge updates and track what needs to be tracked +async fn handle_network_msg( + mut ctx: impl SubsystemContext, + tracker: &mut Tracker, + bridge_event: NetworkBridgeMessage, +) -> SubsystemResult<()> { + match bridge_message { + NetworkBridgeMessage::PeerConnected(peerid, _role) => { + // insert if none already present + tracker.peer_views.entry(peerid).or_insert(View::default()); + } + NetworkBridgeMessage::PeerDisconnected(peerid) => { + // get rid of superfluous data + tracker.peer_views.remove(peerid); + } + NetworkBridgeMessage::PeerViewChange(peerid, view) => { + tracker.peer_views.entry(peerid).modify(|val| { + *val = view + }); + + }, + NetworkBridgeEvent::OurViewChange(view) => { + let old_view = std::mem::replace(tracker.view, view); + tracker + .active_heads + .retain(|head, _| tracker.view.contains(head)); + + for new in tracker.view.difference(&old_view) { + if !tracker.active_heads.contains_key(&new) { + log::warn!("Active head running that's not active anymore, go catch it") //@todo rephrase + //@todo should we get rid of that right here + } + } + } + } + Ok(()) +} + +impl Subsystem for BitfieldDistribution +where + C: SubsystemContext, +{ + fn start(self, ctx: C) -> SpawnedSubsystem { + SpawnedSubsystem(Box::pin(async move { + Self::run(ctx).await; + })) + } +} From ddb62280ce46411bb5bd2d2f1d379ff21fe37773 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Tue, 7 Jul 2020 13:35:21 +0200 Subject: [PATCH 02/45] feat bitfield distribution part 2 --- .../bitfield-distribution/src/lib.rs | 192 +++++++++++------- .../availability/bitfield-distribution.md | 2 +- 2 files changed, 120 insertions(+), 74 deletions(-) diff --git a/node/availability/bitfield-distribution/src/lib.rs b/node/availability/bitfield-distribution/src/lib.rs index 3d47df7c9cee..bc15c0deced5 100644 --- a/node/availability/bitfield-distribution/src/lib.rs +++ b/node/availability/bitfield-distribution/src/lib.rs @@ -27,23 +27,30 @@ use polkadot_node_subsystem::{FromOverseer, SpawnedSubsystem, Subsystem, Subsyst use polkadot_primitives::Hash; use std::{collections::HashMap, pin::Pin}; -// @todo split in multiple costs -const COST_UNEXPECTED: Rep = Rep::new(-100, "Unexpected"); +const COST_SIGNATURE_INVALID: Rep = Rep::new(-10000, "Bitfield signature invalid"); +const COST_MULTIPLE_BITFIELDS_FROM_PEER: Rep = + Rep::new(-10000, "Received more than once bitfield from peer"); +const COST_NOT_INTERESTED: Rep = + Rep::new(-100, "Not intersted in that parent hash"); #[derive(Default, Clone)] struct Tracker { - // track all active peers and their views - // to determine what is relevant to them - peer_views: HashMap, + // track all active peers and their views + // to determine what is relevant to them + peer_views: HashMap, + + // set of active heads the overseer told us to work on + active_heads: HashSet, + + // set of validators which already sent a message + validator_bitset_received: HashSet, // our current view view: View, } - - fn network_update_message(n: NetworkBridgeEvent) -> AllMessages { - AllMessages::BitfieldDistribution(BitfieldDistributionMessage::NetworkBridgeUpdate(n)) + AllMessages::BitfieldDistribution(BitfieldDistributionMessage::NetworkBridgeUpdate(n)) } pub struct BitfieldDistribution; @@ -51,93 +58,131 @@ pub struct BitfieldDistribution; impl BitfieldDistribution { const PROTOCOL_ID: ProtocolId = *b"bitd"; - async fn run(mut ctx: Context) -> SubsystemResult<()> - where - Context: SubsystemContext, - { - // startup: register the network protocol with the bridge. - ctx.send_message(AllMessages::NetworkBridge(NetworkBridgeMessage::RegisterEventProducer( - Self::PROTOCOL_ID, - handle_network_msg, - ))).await?; - - let mut data = Tracker::default(); - loop { - { - let x = ctx.recv().await?; - match x { - FromOverseer::Communication { msg: _ } => { - unreachable!("BitfieldDistributionMessage does not exist; qed") - } - FromOverseer::Signal(OverseerSignal::StartWork(hash)) => { - // @todo cannot work - // tracker.active_heads.insert(hash.clone(), process(&mut data, hash)); - } - FromOverseer::Signal(OverseerSignal::StopWork(hash)) => { - // could work, but see above - tracker.active_heads.remove(&hash); - } - FromOverseer::Signal(OverseerSignal::Conclude) => break, - } - } - active_jobs.retain(|_, future| future.poll().is_pending()); - } - Ok(()) + async fn run( + mut ctx: impl SubsystemContext, + ) -> SubsystemResult<()> { + // startup: register the network protocol with the bridge. + ctx.send_message(AllMessages::NetworkBridge( + NetworkBridgeMessage::RegisterEventProducer(Self::PROTOCOL_ID, handle_network_msg), + )) + .await?; + + let mut data = Tracker::default(); + loop { + { + let message = ctx.recv().await?; + match message { + FromOverseer::Communication { msg: _ } => { + unreachable!("BitfieldDistributionMessage does not exist; qed") + } + FromOverseer::Signal(OverseerSignal::StartWork(relay_parent)) => { + let (validators, session_index) = { + let (validators_tx, validators_rx) = oneshot::channel(); + let (signing_tx, signing_rx) = oneshot::channel(); + + let query_validators = AllMessages::RuntimeApi( + RuntimeApiMessage::Request(relay_parent, RuntimeApiRequest::Validators(validators_tx)), + ); + + let query_signing_context = AllMessages::RuntimeApi( + RuntimeApiMessage::Request(relay_parent, RuntimeApiRequest::SigningContext(signing_tx)), + ); + + ctx.send_messages( + std::iter::once(query_validators).chain(std::iter::once(query_signing_context)) + ).await?; + + (validators_rx.await?, signing_rx.await?) + }; + + // @todo store validators and session_index + process_incoming(ctx, &mut data, relay_parent).await?; + } + FromOverseer::Signal(OverseerSignal::StopWork(relay_parent)) => { + // could work, but see above + tracker.active_heads.remove(&relay_parent); + } + FromOverseer::Signal(OverseerSignal::Conclude) => break, + } + } + active_jobs.retain(|_, future| future.poll().is_pending()); + } + Ok(()) } } /// Handle an incoming message async fn process_incoming( + mut ctx: impl SubsystemContext, tracker: &mut Tracker, message: BitfieldDistributionMessage, ) -> SubsystemResult<()> { match message { /// Distribute a bitfield via gossip to other validators. BitfieldDistributionMessage::DistributeBitfield(hash, signed_availability) => { - // @todo check signature, where to get the SingingContext from? - // signed_availability.check_signature(signing_ctx, validator_id)?; - - for (peerid, view) in tracker.peer_views.filter(|(_peerid,view)| { - view.contains(hash) - }) { - // @todo verify sequential execution is ok or if spawning tasks is better + // @todo should we only distribute availability messages to peer if they are relevant to us + // or is the only discriminator if the peer cares about it? + if !tracker.view.contains(hash) { + // we don't care about this one + // @todo should this be a penality? + return ctx + .send_message(AllMessages::NetworkBridge( + NetworkBridgeMessage::ReportPeer(peerid, COST_NOT_INTERESTED), + )) + .await; + } + // @todo check signature, where to get the SingingContext from? + if signed_availability.check_signature(signing_ctx, validator_id)? { + return ctx + .send_message(AllMessages::NetworkBridge( + NetworkBridgeMessage::ReportPeer(peerid, COST_SIGNATURE_INVALID), + )) + .await; + } - } - } + // @todo verify sequential execution is ok or if spawning tasks is better + // Send peers messages which are interesting to them + for (peerid, view) in tracker + .peer_views + .iter() + .filter(|(_peerid, view)| view.contains(hash)) + { + // @todo shall we assure these complete or just let them be? + ctx.spawn(Box::new(ctx.send_message( + AllMessages::BitfieldDistribution(DistributeBitfield::Bitfield( + hash, + signed_availability, + )), + ))) + .await?; + } + } BitfieldDistributionMessage::NetworkBridgeUpdate(event) => { - handle_network_msg( - &mut tracker, - &mut ctx, - event, - ) - .await? + handle_network_msg(ctx, &mut tracker, event).await?; } - } - Ok(()) + } + Ok(()) } /// Deal with network bridge updates and track what needs to be tracked async fn handle_network_msg( - mut ctx: impl SubsystemContext, + mut ctx: impl SubsystemContext, tracker: &mut Tracker, bridge_event: NetworkBridgeMessage, ) -> SubsystemResult<()> { match bridge_message { NetworkBridgeMessage::PeerConnected(peerid, _role) => { - // insert if none already present - tracker.peer_views.entry(peerid).or_insert(View::default()); + // insert if none already present + tracker.peer_views.entry(peerid).or_insert(View::default()); } NetworkBridgeMessage::PeerDisconnected(peerid) => { - // get rid of superfluous data - tracker.peer_views.remove(peerid); - } + // get rid of superfluous data + tracker.peer_views.remove(peerid); + } NetworkBridgeMessage::PeerViewChange(peerid, view) => { - tracker.peer_views.entry(peerid).modify(|val| { - *val = view - }); - - }, + tracker.peer_views.entry(peerid).modify(|val| *val = view); + } NetworkBridgeEvent::OurViewChange(view) => { let old_view = std::mem::replace(tracker.view, view); tracker @@ -146,13 +191,14 @@ async fn handle_network_msg( for new in tracker.view.difference(&old_view) { if !tracker.active_heads.contains_key(&new) { - log::warn!("Active head running that's not active anymore, go catch it") //@todo rephrase - //@todo should we get rid of that right here - } + log::warn!("Active head running that's not active anymore, go catch it") + //@todo rephrase + //@todo should we get rid of that right here + } } } - } - Ok(()) + } + Ok(()) } impl Subsystem for BitfieldDistribution diff --git a/roadmap/implementers-guide/src/node/availability/bitfield-distribution.md b/roadmap/implementers-guide/src/node/availability/bitfield-distribution.md index 97a5c14be3da..33c8eee7bf31 100644 --- a/roadmap/implementers-guide/src/node/availability/bitfield-distribution.md +++ b/roadmap/implementers-guide/src/node/availability/bitfield-distribution.md @@ -12,7 +12,7 @@ Output: - `NetworkBridge::RegisterEventProducer(ProtocolId)` - `NetworkBridge::SendMessage([PeerId], ProtocolId, Bytes)` - `NetworkBridge::ReportPeer(PeerId, cost_or_benefit)` -- `BlockAuthorshipProvisioning::Bitfield(relay_parent, SignedAvailabilityBitfield)` +- `DistributeBitfield::Bitfield(relay_parent, SignedAvailabilityBitfield)` ## Functionality From 6ac2c62410a785072b43e9b181ec21ac6aa609ae Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Tue, 7 Jul 2020 16:54:42 +0200 Subject: [PATCH 03/45] pair programming with rustc & cargo --- Cargo.lock | 5 + .../bitfield-distribution/Cargo.toml | 7 +- .../bitfield-distribution/src/lib.rs | 133 ++++++++++-------- 3 files changed, 87 insertions(+), 58 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 33e94238f30c..c87799489c47 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4315,10 +4315,15 @@ name = "polkadot-availability-bitfield-distribution" version = "0.1.0" dependencies = [ "futures 0.3.5", + "futures-timer 3.0.2", "log 0.4.8", + "parity-scale-codec", "polkadot-network-bridge", + "polkadot-node-primitives", "polkadot-node-subsystem", "polkadot-primitives", + "sc-network", + "streamunordered", ] [[package]] diff --git a/node/availability/bitfield-distribution/Cargo.toml b/node/availability/bitfield-distribution/Cargo.toml index b2fb4ff1ad4f..2a50b31d4738 100644 --- a/node/availability/bitfield-distribution/Cargo.toml +++ b/node/availability/bitfield-distribution/Cargo.toml @@ -6,7 +6,12 @@ edition = "2018" [dependencies] futures = "0.3.5" +futures-timer = "3.0.2" log = "0.4.8" +streamunordered = "0.5.1" +parity-scale-codec = "1.3.0" +node-primitives = { package = "polkadot-node-primitives", path = "../../primitives" } polkadot-primitives = { path = "../../../primitives" } polkadot-node-subsystem = { path = "../../subsystem" } -polkadot-network-bridge = { path = "../../network/bridge" } \ No newline at end of file +polkadot-network-bridge = { path = "../../network/bridge" } +sc-network = { git = "https://github.com/paritytech/substrate", branch = "master" } diff --git a/node/availability/bitfield-distribution/src/lib.rs b/node/availability/bitfield-distribution/src/lib.rs index bc15c0deced5..1f6e59b4650a 100644 --- a/node/availability/bitfield-distribution/src/lib.rs +++ b/node/availability/bitfield-distribution/src/lib.rs @@ -16,34 +16,43 @@ //! The bitfield distribution subsystem spreading @todo . -use bridge::NetworkBridgeMessage; -use futures::{channel::oneshot, Future}; +use futures::{ + channel::oneshot, + future::{abortable, AbortHandle, Abortable}, + Future, +}; use node_primitives::{ProtocolId, SignedFullStatement, View}; +use polkadot_network_bridge::NetworkBridgeMessage; +use polkadot_node_subsystem::messages::*; use polkadot_node_subsystem::{ - messages::{AllMessages, BitfieldDistributionMessage}, - OverseerSignal, SubsystemResult, + FromOverseer, OverseerSignal, SpawnedSubsystem, Subsystem, SubsystemContext, SubsystemResult, }; -use polkadot_node_subsystem::{FromOverseer, SpawnedSubsystem, Subsystem, SubsystemContext}; +use polkadot_primitives::parachain::ValidatorId; use polkadot_primitives::Hash; -use std::{collections::HashMap, pin::Pin}; +use sc_network::ReputationChange; +use std::{ + collections::{HashMap, HashSet}, + pin::Pin, +}; -const COST_SIGNATURE_INVALID: Rep = Rep::new(-10000, "Bitfield signature invalid"); -const COST_MULTIPLE_BITFIELDS_FROM_PEER: Rep = - Rep::new(-10000, "Received more than once bitfield from peer"); -const COST_NOT_INTERESTED: Rep = - Rep::new(-100, "Not intersted in that parent hash"); +const COST_SIGNATURE_INVALID: ReputationChange = + ReputationChange::new(-10000, "Bitfield signature invalid"); +const COST_MULTIPLE_BITFIELDS_FROM_PEER: ReputationChange = + ReputationChange::new(-10000, "Received more than once bitfield from peer"); +const COST_NOT_INTERESTED: ReputationChange = + ReputationChange::new(-100, "Not intersted in that parent hash"); #[derive(Default, Clone)] struct Tracker { // track all active peers and their views // to determine what is relevant to them - peer_views: HashMap, + peer_views: HashMap, - // set of active heads the overseer told us to work on - active_heads: HashSet, + // set of active heads the overseer told us to work on + active_jobs: HashMap, - // set of validators which already sent a message - validator_bitset_received: HashSet, + // set of validators which already sent a message + validator_bitset_received: HashSet, // our current view view: View, @@ -59,7 +68,7 @@ impl BitfieldDistribution { const PROTOCOL_ID: ProtocolId = *b"bitd"; async fn run( - mut ctx: impl SubsystemContext, + mut ctx: impl SubsystemContext, ) -> SubsystemResult<()> { // startup: register the network protocol with the bridge. ctx.send_message(AllMessages::NetworkBridge( @@ -67,7 +76,7 @@ impl BitfieldDistribution { )) .await?; - let mut data = Tracker::default(); + let mut tracker = Tracker::default(); loop { { let message = ctx.recv().await?; @@ -76,38 +85,49 @@ impl BitfieldDistribution { unreachable!("BitfieldDistributionMessage does not exist; qed") } FromOverseer::Signal(OverseerSignal::StartWork(relay_parent)) => { - let (validators, session_index) = { - let (validators_tx, validators_rx) = oneshot::channel(); - let (signing_tx, signing_rx) = oneshot::channel(); - - let query_validators = AllMessages::RuntimeApi( - RuntimeApiMessage::Request(relay_parent, RuntimeApiRequest::Validators(validators_tx)), - ); - - let query_signing_context = AllMessages::RuntimeApi( - RuntimeApiMessage::Request(relay_parent, RuntimeApiRequest::SigningContext(signing_tx)), - ); - - ctx.send_messages( - std::iter::once(query_validators).chain(std::iter::once(query_signing_context)) - ).await?; - - (validators_rx.await?, signing_rx.await?) - }; - - // @todo store validators and session_index - process_incoming(ctx, &mut data, relay_parent).await?; + let (validators, signing_context) = { + let (validators_tx, validators_rx) = oneshot::channel(); + let (signing_tx, signing_rx) = oneshot::channel(); + + let query_validators = + AllMessages::RuntimeApi(RuntimeApiMessage::Request( + relay_parent, + RuntimeApiRequest::Validators(validators_tx), + )); + + let query_signing_context = + AllMessages::RuntimeApi(RuntimeApiMessage::Request( + relay_parent, + RuntimeApiRequest::SigningContext(signing_tx), + )); + + ctx.send_messages( + std::iter::once(query_validators) + .chain(std::iter::once(query_signing_context)), + ) + .await?; + + (validators_rx.await?, signing_rx.await?) + }; + + let (future, abort_handle) = + abortable(process_incoming(ctx.clone(), relay_parent.clone())); + tracker + .active_jobs + .insert(relay_parent.clone(), abort_handle); } FromOverseer::Signal(OverseerSignal::StopWork(relay_parent)) => { // could work, but see above - tracker.active_heads.remove(&relay_parent); + tracker.active_jobs.remove(&relay_parent); + } + FromOverseer::Signal(OverseerSignal::Conclude) => { + // @todo add a timeout here? + return futures::future::join_all(tracker.active_jobs.into_iter()).await } - FromOverseer::Signal(OverseerSignal::Conclude) => break, } } - active_jobs.retain(|_, future| future.poll().is_pending()); + tracker.active_jobs.retain(|_, future| future.poll().is_pending()); } - Ok(()) } } @@ -123,8 +143,8 @@ async fn process_incoming( // @todo should we only distribute availability messages to peer if they are relevant to us // or is the only discriminator if the peer cares about it? if !tracker.view.contains(hash) { - // we don't care about this one - // @todo should this be a penality? + // we don't care about this one + // @todo should this be a penality? return ctx .send_message(AllMessages::NetworkBridge( NetworkBridgeMessage::ReportPeer(peerid, COST_NOT_INTERESTED), @@ -150,10 +170,9 @@ async fn process_incoming( { // @todo shall we assure these complete or just let them be? ctx.spawn(Box::new(ctx.send_message( - AllMessages::BitfieldDistribution(DistributeBitfield::Bitfield( - hash, - signed_availability, - )), + AllMessages::BitfieldDistribution( + BitfieldDistributionMessage::DistributeBitfield(hash, signed_availability), + ), ))) .await?; } @@ -169,28 +188,28 @@ async fn process_incoming( async fn handle_network_msg( mut ctx: impl SubsystemContext, tracker: &mut Tracker, - bridge_event: NetworkBridgeMessage, + bridge_message: NetworkBridgeMessage, ) -> SubsystemResult<()> { match bridge_message { - NetworkBridgeMessage::PeerConnected(peerid, _role) => { + NetworkBridgeEvent::PeerConnected(peerid, _role) => { // insert if none already present tracker.peer_views.entry(peerid).or_insert(View::default()); } - NetworkBridgeMessage::PeerDisconnected(peerid) => { + NetworkBridgeEvent::PeerDisconnected(peerid) => { // get rid of superfluous data tracker.peer_views.remove(peerid); } - NetworkBridgeMessage::PeerViewChange(peerid, view) => { - tracker.peer_views.entry(peerid).modify(|val| *val = view); + NetworkBridgeEvent::PeerViewChange(peerid, view) => { + tracker.peer_views.entry(peerid).and_modify(|val| *val = view); } NetworkBridgeEvent::OurViewChange(view) => { - let old_view = std::mem::replace(tracker.view, view); + let old_view = std::mem::replace(&mut tracker.view, view); tracker - .active_heads + .active_jobs .retain(|head, _| tracker.view.contains(head)); for new in tracker.view.difference(&old_view) { - if !tracker.active_heads.contains_key(&new) { + if !tracker.active_jobs.contains_key(&new) { log::warn!("Active head running that's not active anymore, go catch it") //@todo rephrase //@todo should we get rid of that right here From 4b100dfce41c9aef4230398db8d741bb826592d1 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Tue, 7 Jul 2020 18:46:18 +0200 Subject: [PATCH 04/45] lets go --- Cargo.lock | 1 + .../bitfield-distribution/Cargo.toml | 3 +- .../bitfield-distribution/src/lib.rs | 199 ++++++++++++------ 3 files changed, 143 insertions(+), 60 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index c87799489c47..1d6136fb4056 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4318,6 +4318,7 @@ dependencies = [ "futures-timer 3.0.2", "log 0.4.8", "parity-scale-codec", + "polkadot-network", "polkadot-network-bridge", "polkadot-node-primitives", "polkadot-node-subsystem", diff --git a/node/availability/bitfield-distribution/Cargo.toml b/node/availability/bitfield-distribution/Cargo.toml index 2a50b31d4738..751bb3f291e2 100644 --- a/node/availability/bitfield-distribution/Cargo.toml +++ b/node/availability/bitfield-distribution/Cargo.toml @@ -9,9 +9,10 @@ futures = "0.3.5" futures-timer = "3.0.2" log = "0.4.8" streamunordered = "0.5.1" -parity-scale-codec = "1.3.0" +codec = { package="parity-scale-codec", version = "1.3.0" } node-primitives = { package = "polkadot-node-primitives", path = "../../primitives" } polkadot-primitives = { path = "../../../primitives" } polkadot-node-subsystem = { path = "../../subsystem" } polkadot-network-bridge = { path = "../../network/bridge" } +polkadot-network = { path = "../../../network" } sc-network = { git = "https://github.com/paritytech/substrate", branch = "master" } diff --git a/node/availability/bitfield-distribution/src/lib.rs b/node/availability/bitfield-distribution/src/lib.rs index 1f6e59b4650a..b88ec29e0798 100644 --- a/node/availability/bitfield-distribution/src/lib.rs +++ b/node/availability/bitfield-distribution/src/lib.rs @@ -22,40 +22,51 @@ use futures::{ Future, }; use node_primitives::{ProtocolId, SignedFullStatement, View}; -use polkadot_network_bridge::NetworkBridgeMessage; use polkadot_node_subsystem::messages::*; use polkadot_node_subsystem::{ FromOverseer, OverseerSignal, SpawnedSubsystem, Subsystem, SubsystemContext, SubsystemResult, }; -use polkadot_primitives::parachain::ValidatorId; +use polkadot_primitives::parachain::{SigningContext, ValidatorId}; use polkadot_primitives::Hash; use sc_network::ReputationChange; use std::{ collections::{HashMap, HashSet}, pin::Pin, }; +use polkadot_network::protocol::Message; +use codec::{Encode, Decode, Codec}; const COST_SIGNATURE_INVALID: ReputationChange = ReputationChange::new(-10000, "Bitfield signature invalid"); +const COST_MISSING_PEER_SESSION_KEY: ReputationChange = + ReputationChange::new(-1337, "Missing peer session key"); const COST_MULTIPLE_BITFIELDS_FROM_PEER: ReputationChange = ReputationChange::new(-10000, "Received more than once bitfield from peer"); const COST_NOT_INTERESTED: ReputationChange = ReputationChange::new(-100, "Not intersted in that parent hash"); + #[derive(Default, Clone)] struct Tracker { // track all active peers and their views // to determine what is relevant to them - peer_views: HashMap, + peer_views: HashMap, + + // keys used for verifying signatures of that peer + peer_session_keys: HashMap, // set of active heads the overseer told us to work on - active_jobs: HashMap, + active_jobs: HashMap>)>, // set of validators which already sent a message validator_bitset_received: HashSet, // our current view - view: View, + view: View, + + // signing context + signing_context: SigningContext, + } fn network_update_message(n: NetworkBridgeEvent) -> AllMessages { @@ -67,9 +78,10 @@ pub struct BitfieldDistribution; impl BitfieldDistribution { const PROTOCOL_ID: ProtocolId = *b"bitd"; - async fn run( - mut ctx: impl SubsystemContext, - ) -> SubsystemResult<()> { + async fn run(mut ctx: Context) -> SubsystemResult<()> + where + Context: SubsystemContext + Clone, + { // startup: register the network protocol with the bridge. ctx.send_message(AllMessages::NetworkBridge( NetworkBridgeMessage::RegisterEventProducer(Self::PROTOCOL_ID, handle_network_msg), @@ -85,58 +97,93 @@ impl BitfieldDistribution { unreachable!("BitfieldDistributionMessage does not exist; qed") } FromOverseer::Signal(OverseerSignal::StartWork(relay_parent)) => { - let (validators, signing_context) = { - let (validators_tx, validators_rx) = oneshot::channel(); - let (signing_tx, signing_rx) = oneshot::channel(); - - let query_validators = - AllMessages::RuntimeApi(RuntimeApiMessage::Request( - relay_parent, - RuntimeApiRequest::Validators(validators_tx), - )); - - let query_signing_context = - AllMessages::RuntimeApi(RuntimeApiMessage::Request( - relay_parent, - RuntimeApiRequest::SigningContext(signing_tx), - )); - - ctx.send_messages( - std::iter::once(query_validators) - .chain(std::iter::once(query_signing_context)), - ) - .await?; - - (validators_rx.await?, signing_rx.await?) - }; + let (validators, signing_context) = query_basics(ctx.clone(), relay_parent).await?; let (future, abort_handle) = - abortable(process_incoming(ctx.clone(), relay_parent.clone())); + abortable(process_incoming_all_incoming( ctx.clone(), relay_parent.clone())); + + let spawn = ctx.spawn(Box::pin(future))?; + let future = Box::new(); tracker .active_jobs - .insert(relay_parent.clone(), abort_handle); + .insert(relay_parent.clone(), (abort_handle, future)); } FromOverseer::Signal(OverseerSignal::StopWork(relay_parent)) => { - // could work, but see above - tracker.active_jobs.remove(&relay_parent); + if let Some((future, abort_handle)) = tracker.active_jobs.take(&relay_parent) { + let _ = abort_handle.abort(); + } } FromOverseer::Signal(OverseerSignal::Conclude) => { - // @todo add a timeout here? - return futures::future::join_all(tracker.active_jobs.into_iter()).await + // @todo add a timeout here? + return futures::future::join_all( + tracker + .active_jobs + .drain() + .map(|(_relay_parent, (cancellation, future))| future) + ) + .await; } } } - tracker.active_jobs.retain(|_, future| future.poll().is_pending()); + tracker + .active_jobs + .retain(|_, future| future.poll().is_pending()); } } } -/// Handle an incoming message -async fn process_incoming( - mut ctx: impl SubsystemContext, - tracker: &mut Tracker, - message: BitfieldDistributionMessage, -) -> SubsystemResult<()> { + +/// query the validator set +async fn query_basics(mut ctx: Context, relay_parent: Hash) -> SubsystemResult<(Vec, SigningContext)> +where + Context: SubsystemContext + Clone, { + let (validators_tx, validators_rx) = oneshot::channel(); + let (signing_tx, signing_rx) = oneshot::channel(); + + let query_validators = + AllMessages::RuntimeApi(RuntimeApiMessage::Request( + relay_parent, + RuntimeApiRequest::Validators(validators_tx), + )); + + let query_signing = + AllMessages::RuntimeApi(RuntimeApiMessage::Request( + relay_parent, + RuntimeApiRequest::SigningContext(signing_tx), + )); + + ctx.send_messages( + std::iter::once(query_validators) + .chain(std::iter::once(query_signing)), + ) + .await?; + + Ok((validators_rx.await?, signing_rx.await?)) +} + + +async fn process_incoming_all_incoming(mut ctx: Context, relay_parent: Hash) -> SubsystemResult<()> +where +Context: SubsystemContext + Clone, +{ + loop { + // @todo shall these be spawned? scheduling + // @todo consume messages via a channel? + // @todo obtain all input args + process_incoming(&mut ctx, relay_parent, peerid, message).await?; + } +} + +/// Handle an incoming message from a peer +async fn process_incoming( + mut ctx: Context, + tracker: &mut Tracker, + peerid: PeerId, + message: BitfieldDistributionMessage, +) -> SubsystemResult<()> +where + Context: SubsystemContext + Clone, +{ match message { /// Distribute a bitfield via gossip to other validators. BitfieldDistributionMessage::DistributeBitfield(hash, signed_availability) => { @@ -152,8 +199,19 @@ async fn process_incoming( .await; } - // @todo check signature, where to get the SingingContext from? - if signed_availability.check_signature(signing_ctx, validator_id)? { + let signing_ctx = &tracker.signing_context.as_ref().unwrap(); + + let session_key = tracker.peer_session_keys.get(peerid); + if session_key.is_none() { + return ctx + .send_message(AllMessages::NetworkBridge( + NetworkBridgeMessage::ReportPeer(peerid, COST_MISSING_PEER_SESSION_KEY), + )) + .await; + } + let session_key = session_key.expect("Just proved it is not `None`; qed"); + + if signed_availability.check_signature(&signing_ctx, session_key)? { return ctx .send_message(AllMessages::NetworkBridge( NetworkBridgeMessage::ReportPeer(peerid, COST_SIGNATURE_INVALID), @@ -174,7 +232,7 @@ async fn process_incoming( BitfieldDistributionMessage::DistributeBitfield(hash, signed_availability), ), ))) - .await?; + .await?; } } BitfieldDistributionMessage::NetworkBridgeUpdate(event) => { @@ -185,11 +243,17 @@ async fn process_incoming( } /// Deal with network bridge updates and track what needs to be tracked -async fn handle_network_msg( - mut ctx: impl SubsystemContext, +async fn handle_network_msg( + mut ctx: Context, tracker: &mut Tracker, - bridge_message: NetworkBridgeMessage, -) -> SubsystemResult<()> { + bridge_message: NetworkBridgeEvent, +) -> SubsystemResult<()> +where + Context: SubsystemContext + Clone, +{ + let peer_views = &mut tracker.peer_views; + let active_jobs = &mut tracker.active_jobs; + let ego = &((*tracker).peer_views); match bridge_message { NetworkBridgeEvent::PeerConnected(peerid, _role) => { // insert if none already present @@ -197,16 +261,18 @@ async fn handle_network_msg( } NetworkBridgeEvent::PeerDisconnected(peerid) => { // get rid of superfluous data - tracker.peer_views.remove(peerid); + tracker.peer_views.remove(&peerid); } NetworkBridgeEvent::PeerViewChange(peerid, view) => { - tracker.peer_views.entry(peerid).and_modify(|val| *val = view); + tracker + .peer_views + .entry(peerid) + .and_modify(|val| *val = view); } NetworkBridgeEvent::OurViewChange(view) => { - let old_view = std::mem::replace(&mut tracker.view, view); - tracker - .active_jobs - .retain(|head, _| tracker.view.contains(head)); + let old_view = std::mem::replace(&mut tracker.view, view); + active_jobs + .retain(|head, _| ego.contains(head)); for new in tracker.view.difference(&old_view) { if !tracker.active_jobs.contains_key(&new) { @@ -216,13 +282,28 @@ async fn handle_network_msg( } } } + NetworkBridgeEvent::PeerMessage(remote, bytes) => { + // @todo what would we receive here? + match Message::decode(&mut bytes.as_ref()) { + Ok(message) => { + match message { + // a new session key + Message::ValidatorId(session_key) => { + tracker.peer_session_keys.insert(remote.clone(), session_key); + } + _ => {} + } + }, + Err(_) => unimplemented!("Invalid format shall be punished I guess"), + } + } } Ok(()) } impl Subsystem for BitfieldDistribution where - C: SubsystemContext, + C: SubsystemContext + Clone, { fn start(self, ctx: C) -> SpawnedSubsystem { SpawnedSubsystem(Box::pin(async move { From a3bd8c0f54369cda55488b9cd9b1ab611d7bd96b Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Wed, 8 Jul 2020 10:01:55 +0200 Subject: [PATCH 05/45] move bitfield-distribution to the node/network folder --- Cargo.toml | 2 +- .../bitfield-distribution/Cargo.toml | 0 .../bitfield-distribution/src/lib.rs | 4 ++-- 3 files changed, 3 insertions(+), 3 deletions(-) rename node/{availability => network}/bitfield-distribution/Cargo.toml (100%) rename node/{availability => network}/bitfield-distribution/src/lib.rs (98%) diff --git a/Cargo.toml b/Cargo.toml index 3b4886fd2b91..200ee0f98797 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -43,11 +43,11 @@ members = [ "service", "validation", - "node/availability/bitfield-distribution", "node/core/proposer", "node/network/bridge", "node/network/pov-distribution", "node/network/statement-distribution", + "node/network/bitfield-distribution", "node/overseer", "node/primitives", "node/service", diff --git a/node/availability/bitfield-distribution/Cargo.toml b/node/network/bitfield-distribution/Cargo.toml similarity index 100% rename from node/availability/bitfield-distribution/Cargo.toml rename to node/network/bitfield-distribution/Cargo.toml diff --git a/node/availability/bitfield-distribution/src/lib.rs b/node/network/bitfield-distribution/src/lib.rs similarity index 98% rename from node/availability/bitfield-distribution/src/lib.rs rename to node/network/bitfield-distribution/src/lib.rs index b88ec29e0798..a7956aac0a29 100644 --- a/node/availability/bitfield-distribution/src/lib.rs +++ b/node/network/bitfield-distribution/src/lib.rs @@ -272,7 +272,7 @@ where NetworkBridgeEvent::OurViewChange(view) => { let old_view = std::mem::replace(&mut tracker.view, view); active_jobs - .retain(|head, _| ego.contains(head)); + .retain(|head, _| ego.get(head).is_some()); for new in tracker.view.difference(&old_view) { if !tracker.active_jobs.contains_key(&new) { @@ -289,7 +289,7 @@ where match message { // a new session key Message::ValidatorId(session_key) => { - tracker.peer_session_keys.insert(remote.clone(), session_key); + let _ = tracker.peer_session_keys.insert(remote.clone(), session_key); } _ => {} } From 7909d4773175f8e2b5dc9917db47a0572237a5e3 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Wed, 8 Jul 2020 12:50:01 +0200 Subject: [PATCH 06/45] shape shifting --- node/network/bitfield-distribution/src/lib.rs | 225 +++++++++--------- 1 file changed, 119 insertions(+), 106 deletions(-) diff --git a/node/network/bitfield-distribution/src/lib.rs b/node/network/bitfield-distribution/src/lib.rs index a7956aac0a29..ea07943ba784 100644 --- a/node/network/bitfield-distribution/src/lib.rs +++ b/node/network/bitfield-distribution/src/lib.rs @@ -16,12 +16,14 @@ //! The bitfield distribution subsystem spreading @todo . +use codec::{Codec, Decode, Encode}; use futures::{ channel::oneshot, future::{abortable, AbortHandle, Abortable}, Future, }; use node_primitives::{ProtocolId, SignedFullStatement, View}; +use polkadot_network::protocol::Message; use polkadot_node_subsystem::messages::*; use polkadot_node_subsystem::{ FromOverseer, OverseerSignal, SpawnedSubsystem, Subsystem, SubsystemContext, SubsystemResult, @@ -33,8 +35,6 @@ use std::{ collections::{HashMap, HashSet}, pin::Pin, }; -use polkadot_network::protocol::Message; -use codec::{Encode, Decode, Codec}; const COST_SIGNATURE_INVALID: ReputationChange = ReputationChange::new(-10000, "Bitfield signature invalid"); @@ -44,29 +44,26 @@ const COST_MULTIPLE_BITFIELDS_FROM_PEER: ReputationChange = ReputationChange::new(-10000, "Received more than once bitfield from peer"); const COST_NOT_INTERESTED: ReputationChange = ReputationChange::new(-100, "Not intersted in that parent hash"); - +const COST_MESSAGE_NOT_DECODABLE: ReputationChange = + ReputationChange::new(-100, "Not intersted in that parent hash"); #[derive(Default, Clone)] struct Tracker { // track all active peers and their views // to determine what is relevant to them - peer_views: HashMap, - - // keys used for verifying signatures of that peer - peer_session_keys: HashMap, + peer_views: HashMap, - // set of active heads the overseer told us to work on - active_jobs: HashMap>)>, + // keys used for verifying signatures of that peer + peer_session_keys: HashMap, // set of validators which already sent a message validator_bitset_received: HashSet, // our current view - view: View, - - // signing context - signing_context: SigningContext, + view: View, + // signing context + signing_context: SigningContext, } fn network_update_message(n: NetworkBridgeEvent) -> AllMessages { @@ -84,42 +81,45 @@ impl BitfieldDistribution { { // startup: register the network protocol with the bridge. ctx.send_message(AllMessages::NetworkBridge( - NetworkBridgeMessage::RegisterEventProducer(Self::PROTOCOL_ID, handle_network_msg), + NetworkBridgeMessage::RegisterEventProducer(Self::PROTOCOL_ID, network_update_message), )) .await?; - let mut tracker = Tracker::default(); + // set of active heads the overseer told us to work on + let mut active_jobs = HashMap::>)>::new(); + loop { { let message = ctx.recv().await?; match message { FromOverseer::Communication { msg: _ } => { - unreachable!("BitfieldDistributionMessage does not exist; qed") + unreachable!("Overseer should not send us BitfieldDistributionMessages"); } FromOverseer::Signal(OverseerSignal::StartWork(relay_parent)) => { - let (validators, signing_context) = query_basics(ctx.clone(), relay_parent).await?; + let (validators, signing_context) = + query_basics(ctx.clone(), relay_parent).await?; let (future, abort_handle) = - abortable(process_incoming_all_incoming( ctx.clone(), relay_parent.clone())); + abortable(processor_per_relay_parent(ctx.clone(), relay_parent.clone(), )); - let spawn = ctx.spawn(Box::pin(future))?; - let future = Box::new(); - tracker - .active_jobs + let future = ctx.spawn(Box::pin(future))?; + let future = Box::pin(future); + active_jobs .insert(relay_parent.clone(), (abort_handle, future)); } FromOverseer::Signal(OverseerSignal::StopWork(relay_parent)) => { - if let Some((future, abort_handle)) = tracker.active_jobs.take(&relay_parent) { - let _ = abort_handle.abort(); - } + if let Some((_future, abort_handle)) = + active_jobs.take(&relay_parent) + { + let _ = abort_handle.abort(); + } } FromOverseer::Signal(OverseerSignal::Conclude) => { // @todo add a timeout here? return futures::future::join_all( - tracker - .active_jobs + active_jobs .drain() - .map(|(_relay_parent, (cancellation, future))| future) + .map(|(_relay_parent, (cancellation, future))| future), ) .await; } @@ -133,89 +133,64 @@ impl BitfieldDistribution { } -/// query the validator set -async fn query_basics(mut ctx: Context, relay_parent: Hash) -> SubsystemResult<(Vec, SigningContext)> -where - Context: SubsystemContext + Clone, { - let (validators_tx, validators_rx) = oneshot::channel(); - let (signing_tx, signing_rx) = oneshot::channel(); - - let query_validators = - AllMessages::RuntimeApi(RuntimeApiMessage::Request( - relay_parent, - RuntimeApiRequest::Validators(validators_tx), - )); - - let query_signing = - AllMessages::RuntimeApi(RuntimeApiMessage::Request( - relay_parent, - RuntimeApiRequest::SigningContext(signing_tx), - )); - - ctx.send_messages( - std::iter::once(query_validators) - .chain(std::iter::once(query_signing)), - ) - .await?; - - Ok((validators_rx.await?, signing_rx.await?)) +/// Process all requests related to one relay parent hash +async fn processor_per_relay_parent(mut ctx: Context, relay_parent: Hash) -> SubsystemResult<()> { + let mut tracker = Tracker::default(); + loop { + todo!("consume relay parents") + } } - -async fn process_incoming_all_incoming(mut ctx: Context, relay_parent: Hash) -> SubsystemResult<()> +/// modify the reputiation, good or bad +async fn modify_reputiation(mut ctx: Context, peerid: PeerId, rep: ReputationChange) -> SubsystemResult<()> where -Context: SubsystemContext + Clone, + Context: SubsystemContext + Clone, { - loop { - // @todo shall these be spawned? scheduling - // @todo consume messages via a channel? - // @todo obtain all input args - process_incoming(&mut ctx, relay_parent, peerid, message).await?; - } + ctx.send_message(AllMessages::NetworkBridge( + NetworkBridgeMessage::ReportPeer(peerid, rep), + )) + .await } /// Handle an incoming message from a peer async fn process_incoming( mut ctx: Context, - tracker: &mut Tracker, - peerid: PeerId, - message: BitfieldDistributionMessage, + tracker: &mut Tracker, + peerid: PeerId, + message: Vec, ) -> SubsystemResult<()> where Context: SubsystemContext + Clone, { + let message = if let Ok(message) = decode(message) { + message + } else { + return ctx + .send_message(AllMessages::NetworkBridge( + NetworkBridgeMessage::ReportPeer(peerid, COST_MESSAGE_NOT_DECODABLE), + )) + .await; + }; match message { /// Distribute a bitfield via gossip to other validators. BitfieldDistributionMessage::DistributeBitfield(hash, signed_availability) => { // @todo should we only distribute availability messages to peer if they are relevant to us // or is the only discriminator if the peer cares about it? if !tracker.view.contains(hash) { - // we don't care about this one - // @todo should this be a penality? - return ctx - .send_message(AllMessages::NetworkBridge( - NetworkBridgeMessage::ReportPeer(peerid, COST_NOT_INTERESTED), - )) - .await; + // we don't care about this, the other side should have known better + return modify_reputiation(ctx, peerid, COST_NOT_INTERESTED).await; } - let signing_ctx = &tracker.signing_context.as_ref().unwrap(); + let signing_ctx = &tracker.signing_context.as_ref().unwrap(); - let session_key = tracker.peer_session_keys.get(peerid); - if session_key.is_none() { - return ctx - .send_message(AllMessages::NetworkBridge( - NetworkBridgeMessage::ReportPeer(peerid, COST_MISSING_PEER_SESSION_KEY), - )) - .await; - } - let session_key = session_key.expect("Just proved it is not `None`; qed"); + let session_key = tracker.peer_session_keys.get(peerid); + if session_key.is_none() { + return modify_reputiation(ctx.clone(), peerid, COST_MISSING_PEER_SESSION_KEY) + } + let session_key = session_key.expect("Just proved it is not `None`; qed"); if signed_availability.check_signature(&signing_ctx, session_key)? { - return ctx - .send_message(AllMessages::NetworkBridge( - NetworkBridgeMessage::ReportPeer(peerid, COST_SIGNATURE_INVALID), - )) + return modify_reputiation(ctx.clone(), peerid, COST_SIGNATURE_INVALID) .await; } @@ -232,7 +207,7 @@ where BitfieldDistributionMessage::DistributeBitfield(hash, signed_availability), ), ))) - .await?; + .await?; } } BitfieldDistributionMessage::NetworkBridgeUpdate(event) => { @@ -251,9 +226,9 @@ async fn handle_network_msg( where Context: SubsystemContext + Clone, { - let peer_views = &mut tracker.peer_views; - let active_jobs = &mut tracker.active_jobs; - let ego = &((*tracker).peer_views); + let peer_views = &mut tracker.peer_views; + let active_jobs = &mut tracker.active_jobs; + let ego = &((*tracker).view); match bridge_message { NetworkBridgeEvent::PeerConnected(peerid, _role) => { // insert if none already present @@ -270,9 +245,8 @@ where .and_modify(|val| *val = view); } NetworkBridgeEvent::OurViewChange(view) => { - let old_view = std::mem::replace(&mut tracker.view, view); - active_jobs - .retain(|head, _| ego.get(head).is_some()); + let old_view = std::mem::replace(&mut tracker.view, view); + active_jobs.retain(|head, _| ego.get(head).is_some()); for new in tracker.view.difference(&old_view) { if !tracker.active_jobs.contains_key(&new) { @@ -284,18 +258,20 @@ where } NetworkBridgeEvent::PeerMessage(remote, bytes) => { // @todo what would we receive here? - match Message::decode(&mut bytes.as_ref()) { - Ok(message) => { - match message { - // a new session key - Message::ValidatorId(session_key) => { - let _ = tracker.peer_session_keys.insert(remote.clone(), session_key); - } - _ => {} - } - }, - Err(_) => unimplemented!("Invalid format shall be punished I guess"), - } + match Message::decode(&mut bytes.as_ref()) { + Ok(message) => { + match message { + // a new session key + Message::ValidatorId(session_key) => { + let _ = tracker + .peer_session_keys + .insert(remote.clone(), session_key); + } + _ => {} + } + } + Err(_) => unimplemented!("Invalid format shall be punished I guess"), + } } } Ok(()) @@ -311,3 +287,40 @@ where })) } } + +/// query the validator set +async fn query_basics( + mut ctx: Context, + relay_parent: Hash, +) -> SubsystemResult<(Vec, SigningContext)> +where + Context: SubsystemContext + Clone, +{ + let (validators_tx, validators_rx) = oneshot::channel(); + let (signing_tx, signing_rx) = oneshot::channel(); + + let query_validators = AllMessages::RuntimeApi(RuntimeApiMessage::Request( + relay_parent, + RuntimeApiRequest::Validators(validators_tx), + )); + + let query_signing = AllMessages::RuntimeApi(RuntimeApiMessage::Request( + relay_parent, + RuntimeApiRequest::SigningContext(signing_tx), + )); + + ctx.send_messages(std::iter::once(query_validators).chain(std::iter::once(query_signing))) + .await?; + + Ok((validators_rx.await?, signing_rx.await?)) +} + +#[cfg(test)] +mod test { + use super::*; + + #[test] + fn x() { + // @todo + } +} From e3c8248f2847da100f215c29f2352f5b2a002b0a Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Wed, 8 Jul 2020 13:02:20 +0200 Subject: [PATCH 07/45] lunchtime --- node/network/bitfield-distribution/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/node/network/bitfield-distribution/src/lib.rs b/node/network/bitfield-distribution/src/lib.rs index ea07943ba784..68f78ad5f44f 100644 --- a/node/network/bitfield-distribution/src/lib.rs +++ b/node/network/bitfield-distribution/src/lib.rs @@ -162,7 +162,7 @@ async fn process_incoming( where Context: SubsystemContext + Clone, { - let message = if let Ok(message) = decode(message) { + let message = if let Ok(message) = BitfieldDistributionMessage::decode(message) { message } else { return ctx From 86f4fdbfbe44c26b68e32b8b6a4a8be1629dadc4 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Wed, 8 Jul 2020 19:33:07 +0200 Subject: [PATCH 08/45] ignore the two fn recursion for now --- node/network/bitfield-distribution/src/lib.rs | 30 ++++++++++--------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/node/network/bitfield-distribution/src/lib.rs b/node/network/bitfield-distribution/src/lib.rs index 68f78ad5f44f..6e131a27ace5 100644 --- a/node/network/bitfield-distribution/src/lib.rs +++ b/node/network/bitfield-distribution/src/lib.rs @@ -92,15 +92,16 @@ impl BitfieldDistribution { { let message = ctx.recv().await?; match message { - FromOverseer::Communication { msg: _ } => { - unreachable!("Overseer should not send us BitfieldDistributionMessages"); + FromOverseer::Communication { msg } => { + let peerid = PeerId::default(); + process_incoming(ctx.clone(), &mut tracker, peerid, msg).await?; } FromOverseer::Signal(OverseerSignal::StartWork(relay_parent)) => { let (validators, signing_context) = query_basics(ctx.clone(), relay_parent).await?; let (future, abort_handle) = - abortable(processor_per_relay_parent(ctx.clone(), relay_parent.clone(), )); + abortable(processor_per_relay_parent(ctx.clone(), relay_parent.clone())); let future = ctx.spawn(Box::pin(future))?; let future = Box::pin(future); @@ -157,20 +158,21 @@ async fn process_incoming( mut ctx: Context, tracker: &mut Tracker, peerid: PeerId, - message: Vec, + // message: Vec, + message: BitfieldDistributionMessage, ) -> SubsystemResult<()> where Context: SubsystemContext + Clone, { - let message = if let Ok(message) = BitfieldDistributionMessage::decode(message) { - message - } else { - return ctx - .send_message(AllMessages::NetworkBridge( - NetworkBridgeMessage::ReportPeer(peerid, COST_MESSAGE_NOT_DECODABLE), - )) - .await; - }; + // let message = if let Ok(message) = BitfieldDistributionMessage::decode(message) { + // message + // } else { + // return ctx + // .send_message(AllMessages::NetworkBridge( + // NetworkBridgeMessage::ReportPeer(peerid, COST_MESSAGE_NOT_DECODABLE), + // )) + // .await; + // }; match message { /// Distribute a bitfield via gossip to other validators. BitfieldDistributionMessage::DistributeBitfield(hash, signed_availability) => { @@ -246,7 +248,7 @@ where } NetworkBridgeEvent::OurViewChange(view) => { let old_view = std::mem::replace(&mut tracker.view, view); - active_jobs.retain(|head, _| ego.get(head).is_some()); + active_jobs.retain(|head, _| ego.0.get(head).is_some()); for new in tracker.view.difference(&old_view) { if !tracker.active_jobs.contains_key(&new) { From 3cf0878cd051c54912ff4075eb1a7a7e5b2a58b9 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Thu, 9 Jul 2020 17:01:59 +0200 Subject: [PATCH 09/45] step by step --- node/network/bitfield-distribution/src/lib.rs | 163 ++++++++++-------- 1 file changed, 91 insertions(+), 72 deletions(-) diff --git a/node/network/bitfield-distribution/src/lib.rs b/node/network/bitfield-distribution/src/lib.rs index 6e131a27ace5..6c8b4afa82ee 100644 --- a/node/network/bitfield-distribution/src/lib.rs +++ b/node/network/bitfield-distribution/src/lib.rs @@ -20,7 +20,7 @@ use codec::{Codec, Decode, Encode}; use futures::{ channel::oneshot, future::{abortable, AbortHandle, Abortable}, - Future, + Future, FutureExt, }; use node_primitives::{ProtocolId, SignedFullStatement, View}; use polkadot_network::protocol::Message; @@ -53,19 +53,32 @@ struct Tracker { // to determine what is relevant to them peer_views: HashMap, - // keys used for verifying signatures of that peer - peer_session_keys: HashMap, + // our current view + view: View, + + // signing context for a particular relay_parent + jobs: HashMap, + + // set of validators for a particular relay_parent + per_job: HashMap, +} + +/// Data for each relay parent +#[derive(Debug, Clone, Default)] +struct JobData { // set of validators which already sent a message validator_bitset_received: HashSet, - // our current view - view: View, - - // signing context + // signing context for a particular relay_parent signing_context: SigningContext, + + // set of validators for a particular relay_parent + validator_set: Vec, } + + fn network_update_message(n: NetworkBridgeEvent) -> AllMessages { AllMessages::BitfieldDistribution(BitfieldDistributionMessage::NetworkBridgeUpdate(n)) } @@ -85,50 +98,59 @@ impl BitfieldDistribution { )) .await?; - // set of active heads the overseer told us to work on - let mut active_jobs = HashMap::>)>::new(); - + // set of active heads the overseer told us to work on with the connected + // tasks abort handles + // @todo do we need Box>) for anything? + let mut active_jobs = HashMap::::new(); + let mut tracker = Tracker::default(); loop { { let message = ctx.recv().await?; match message { FromOverseer::Communication { msg } => { - let peerid = PeerId::default(); + let peerid = PeerId::random(); // @todo process_incoming(ctx.clone(), &mut tracker, peerid, msg).await?; } FromOverseer::Signal(OverseerSignal::StartWork(relay_parent)) => { let (validators, signing_context) = query_basics(ctx.clone(), relay_parent).await?; - let (future, abort_handle) = - abortable(processor_per_relay_parent(ctx.clone(), relay_parent.clone())); - - let future = ctx.spawn(Box::pin(future))?; - let future = Box::pin(future); - active_jobs - .insert(relay_parent.clone(), (abort_handle, future)); + let _ = tracker.per_job.insert(relay_parent, JobData { + validator_bitset_received: HashSet::new(), + signing_context: HashMap::new(), + validator_set: Vec::new(), + }); + + let future = processor_per_relay_parent(ctx.clone(), relay_parent.clone()); + // let (future, abort_handle) = + // abortable(future); + + let future = ctx.spawn(Box::pin( + future.then(|_| {futures::future::ok(())}) + )); + // active_jobs + // .insert(relay_parent.clone(), abort_handle); } FromOverseer::Signal(OverseerSignal::StopWork(relay_parent)) => { - if let Some((_future, abort_handle)) = - active_jobs.take(&relay_parent) + if let Some(abort_handle) = + active_jobs.remove(&relay_parent) { let _ = abort_handle.abort(); } } FromOverseer::Signal(OverseerSignal::Conclude) => { - // @todo add a timeout here? - return futures::future::join_all( - active_jobs - .drain() - .map(|(_relay_parent, (cancellation, future))| future), - ) - .await; + // @todo cannot store the future + // return futures::future::join_all( + // active_jobs + // .drain() + // .map(|(_relay_parent, (cancellation, future))| future), + // ) + // .await; } } } - tracker - .active_jobs - .retain(|_, future| future.poll().is_pending()); + // active_jobs + // .retain(|_, future| future.poll().is_pending()); } } } @@ -138,8 +160,9 @@ impl BitfieldDistribution { async fn processor_per_relay_parent(mut ctx: Context, relay_parent: Hash) -> SubsystemResult<()> { let mut tracker = Tracker::default(); loop { - todo!("consume relay parents") + // todo!("consume relay parents") } + Ok(()) } /// modify the reputiation, good or bad @@ -158,59 +181,53 @@ async fn process_incoming( mut ctx: Context, tracker: &mut Tracker, peerid: PeerId, - // message: Vec, message: BitfieldDistributionMessage, ) -> SubsystemResult<()> where Context: SubsystemContext + Clone, { - // let message = if let Ok(message) = BitfieldDistributionMessage::decode(message) { - // message - // } else { - // return ctx - // .send_message(AllMessages::NetworkBridge( - // NetworkBridgeMessage::ReportPeer(peerid, COST_MESSAGE_NOT_DECODABLE), - // )) - // .await; - // }; + let peer_view = tracker.peer_views.get(&peerid).expect("TODO"); match message { - /// Distribute a bitfield via gossip to other validators. + // Distribute a bitfield via gossip to other validators. BitfieldDistributionMessage::DistributeBitfield(hash, signed_availability) => { + let job_data = if let Some(job_data) = tracker.per_job.get(&hash) { + job_data + } else { + return modify_reputiation(ctx, peerid, COST_NOT_INTERESTED).await; + }; + // @todo should we only distribute availability messages to peer if they are relevant to us // or is the only discriminator if the peer cares about it? - if !tracker.view.contains(hash) { + if !peer_view.contains(&hash) { // we don't care about this, the other side should have known better return modify_reputiation(ctx, peerid, COST_NOT_INTERESTED).await; } - let signing_ctx = &tracker.signing_context.as_ref().unwrap(); - - let session_key = tracker.peer_session_keys.get(peerid); - if session_key.is_none() { - return modify_reputiation(ctx.clone(), peerid, COST_MISSING_PEER_SESSION_KEY) + let validator_set = &job_data.validator_set; + if validator_set.len() == 0 { + return modify_reputiation(ctx.clone(), peerid, COST_MISSING_PEER_SESSION_KEY).await } - let session_key = session_key.expect("Just proved it is not `None`; qed"); - if signed_availability.check_signature(&signing_ctx, session_key)? { + // check all validators that could have signed this message + if let Some(_) = validator_set.iter().find(|validator| { signed_availability.check_signature(&job_data.signing_context, validator).is_ok() }) { return modify_reputiation(ctx.clone(), peerid, COST_SIGNATURE_INVALID) - .await; + .await } // @todo verify sequential execution is ok or if spawning tasks is better // Send peers messages which are interesting to them - for (peerid, view) in tracker - .peer_views - .iter() - .filter(|(_peerid, view)| view.contains(hash)) - { - // @todo shall we assure these complete or just let them be? - ctx.spawn(Box::new(ctx.send_message( - AllMessages::BitfieldDistribution( - BitfieldDistributionMessage::DistributeBitfield(hash, signed_availability), - ), - ))) - .await?; - } + let _ = futures::future::join_all( + tracker.peer_views.iter() + .filter(|(_peerid, view)| view.contains(&hash)) + .map(|(peerid, view) | { + // @todo shall we assure these complete or just let them be? + ctx.spawn(Box::pin(ctx.send_message( + AllMessages::BitfieldDistribution( + BitfieldDistributionMessage::DistributeBitfield(hash, signed_availability), + ) + ).then(|_| {futures::future::ok(())}))) + }) + ).await; } BitfieldDistributionMessage::NetworkBridgeUpdate(event) => { handle_network_msg(ctx, &mut tracker, event).await?; @@ -229,7 +246,7 @@ where Context: SubsystemContext + Clone, { let peer_views = &mut tracker.peer_views; - let active_jobs = &mut tracker.active_jobs; + let per_job = &mut tracker.per_job; let ego = &((*tracker).view); match bridge_message { NetworkBridgeEvent::PeerConnected(peerid, _role) => { @@ -248,10 +265,10 @@ where } NetworkBridgeEvent::OurViewChange(view) => { let old_view = std::mem::replace(&mut tracker.view, view); - active_jobs.retain(|head, _| ego.0.get(head).is_some()); + tracker.per_job.retain(|hash, _job_data| ego.0.get(hash).is_some()); for new in tracker.view.difference(&old_view) { - if !tracker.active_jobs.contains_key(&new) { + if !tracker.per_job.contains_key(&new) { log::warn!("Active head running that's not active anymore, go catch it") //@todo rephrase //@todo should we get rid of that right here @@ -259,15 +276,17 @@ where } } NetworkBridgeEvent::PeerMessage(remote, bytes) => { + log::info!("Got a peer message from {:?}", &remote); // @todo what would we receive here? match Message::decode(&mut bytes.as_ref()) { Ok(message) => { match message { // a new session key Message::ValidatorId(session_key) => { - let _ = tracker - .peer_session_keys - .insert(remote.clone(), session_key); + // @todo update all Validator ids I guess? + // let _ = tracker + // .per_job. + // .insert(remote.clone(), session_key); } _ => {} } @@ -302,12 +321,12 @@ where let (signing_tx, signing_rx) = oneshot::channel(); let query_validators = AllMessages::RuntimeApi(RuntimeApiMessage::Request( - relay_parent, + relay_parent.clone(), RuntimeApiRequest::Validators(validators_tx), )); let query_signing = AllMessages::RuntimeApi(RuntimeApiMessage::Request( - relay_parent, + relay_parent.clone(), RuntimeApiRequest::SigningContext(signing_tx), )); From 6a6d7bc8f43145997d28462ba5c582a18779d6b6 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Thu, 9 Jul 2020 17:14:06 +0200 Subject: [PATCH 10/45] triplesteps --- node/network/bitfield-distribution/src/lib.rs | 31 +++++++++---------- 1 file changed, 14 insertions(+), 17 deletions(-) diff --git a/node/network/bitfield-distribution/src/lib.rs b/node/network/bitfield-distribution/src/lib.rs index 6c8b4afa82ee..8175afb4ed93 100644 --- a/node/network/bitfield-distribution/src/lib.rs +++ b/node/network/bitfield-distribution/src/lib.rs @@ -117,7 +117,7 @@ impl BitfieldDistribution { let _ = tracker.per_job.insert(relay_parent, JobData { validator_bitset_received: HashSet::new(), - signing_context: HashMap::new(), + signing_context, validator_set: Vec::new(), }); @@ -126,7 +126,7 @@ impl BitfieldDistribution { // abortable(future); let future = ctx.spawn(Box::pin( - future.then(|_| {futures::future::ok(())}) + future.map(|_| { () }) )); // active_jobs // .insert(relay_parent.clone(), abort_handle); @@ -216,21 +216,18 @@ where // @todo verify sequential execution is ok or if spawning tasks is better // Send peers messages which are interesting to them - let _ = futures::future::join_all( - tracker.peer_views.iter() - .filter(|(_peerid, view)| view.contains(&hash)) - .map(|(peerid, view) | { - // @todo shall we assure these complete or just let them be? - ctx.spawn(Box::pin(ctx.send_message( - AllMessages::BitfieldDistribution( - BitfieldDistributionMessage::DistributeBitfield(hash, signed_availability), - ) - ).then(|_| {futures::future::ok(())}))) - }) - ).await; + for (peerid, view) in tracker.peer_views.iter() + .filter(|(_peerid, view)| view.contains(&hash)) + { + // @todo shall we assure these complete or just let them be? + let _ = ctx.send_message( + AllMessages::BitfieldDistribution( + BitfieldDistributionMessage::DistributeBitfield(hash.clone(), signed_availability.clone()), + )).await; + } } BitfieldDistributionMessage::NetworkBridgeUpdate(event) => { - handle_network_msg(ctx, &mut tracker, event).await?; + handle_network_msg(ctx, tracker, event).await?; } } Ok(()) @@ -264,8 +261,8 @@ where .and_modify(|val| *val = view); } NetworkBridgeEvent::OurViewChange(view) => { - let old_view = std::mem::replace(&mut tracker.view, view); - tracker.per_job.retain(|hash, _job_data| ego.0.get(hash).is_some()); + let old_view = std::mem::replace(&mut (tracker.view), view); + tracker.per_job.retain(|hash, _job_data| ego.0.contains(hash)); for new in tracker.view.difference(&old_view) { if !tracker.per_job.contains_key(&new) { From 1aa1842eb58e620e849028a6982ffc086edf7bd0 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Fri, 10 Jul 2020 11:39:24 +0200 Subject: [PATCH 11/45] bandaid commit --- node/network/bitfield-distribution/src/lib.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/node/network/bitfield-distribution/src/lib.rs b/node/network/bitfield-distribution/src/lib.rs index 8175afb4ed93..37b46bad11fe 100644 --- a/node/network/bitfield-distribution/src/lib.rs +++ b/node/network/bitfield-distribution/src/lib.rs @@ -261,8 +261,9 @@ where .and_modify(|val| *val = view); } NetworkBridgeEvent::OurViewChange(view) => { + let ego = ego.clone(); let old_view = std::mem::replace(&mut (tracker.view), view); - tracker.per_job.retain(|hash, _job_data| ego.0.contains(hash)); + tracker.per_job.retain(move |hash, _job_data| ego.0.contains(hash)); for new in tracker.view.difference(&old_view) { if !tracker.per_job.contains_key(&new) { @@ -297,7 +298,7 @@ where impl Subsystem for BitfieldDistribution where - C: SubsystemContext + Clone, + C: SubsystemContext + Clone + Sync, { fn start(self, ctx: C) -> SpawnedSubsystem { SpawnedSubsystem(Box::pin(async move { @@ -310,7 +311,7 @@ where async fn query_basics( mut ctx: Context, relay_parent: Hash, -) -> SubsystemResult<(Vec, SigningContext)> +) -> SubsystemResult<(Vec, SigningContext)> where Context: SubsystemContext + Clone, { From ad0555ba02f728f732456014df525b76476f638d Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Fri, 10 Jul 2020 12:57:09 +0200 Subject: [PATCH 12/45] unordered futures magic --- node/network/bitfield-distribution/src/lib.rs | 66 ++++++++++++------- 1 file changed, 44 insertions(+), 22 deletions(-) diff --git a/node/network/bitfield-distribution/src/lib.rs b/node/network/bitfield-distribution/src/lib.rs index 37b46bad11fe..09c3921511f9 100644 --- a/node/network/bitfield-distribution/src/lib.rs +++ b/node/network/bitfield-distribution/src/lib.rs @@ -21,7 +21,10 @@ use futures::{ channel::oneshot, future::{abortable, AbortHandle, Abortable}, Future, FutureExt, + stream::FuturesUnordered, }; +use futures::stream::StreamExt; + use node_primitives::{ProtocolId, SignedFullStatement, View}; use polkadot_network::protocol::Message; use polkadot_node_subsystem::messages::*; @@ -101,35 +104,41 @@ impl BitfieldDistribution { // set of active heads the overseer told us to work on with the connected // tasks abort handles // @todo do we need Box>) for anything? + // let mut active_jobs = HashMap::>>)>::new(); let mut active_jobs = HashMap::::new(); let mut tracker = Tracker::default(); loop { { - let message = ctx.recv().await?; + let message = { + let mut ctx = ctx.clone(); + ctx.recv().await? + }; match message { FromOverseer::Communication { msg } => { let peerid = PeerId::random(); // @todo process_incoming(ctx.clone(), &mut tracker, peerid, msg).await?; } FromOverseer::Signal(OverseerSignal::StartWork(relay_parent)) => { - let (validators, signing_context) = + let (validator_set, signing_context) = query_basics(ctx.clone(), relay_parent).await?; let _ = tracker.per_job.insert(relay_parent, JobData { validator_bitset_received: HashSet::new(), signing_context, - validator_set: Vec::new(), + validator_set: validator_set, }); let future = processor_per_relay_parent(ctx.clone(), relay_parent.clone()); - // let (future, abort_handle) = - // abortable(future); - - let future = ctx.spawn(Box::pin( - future.map(|_| { () }) - )); - // active_jobs - // .insert(relay_parent.clone(), abort_handle); + let (future, abort_handle) = abortable(future); + + + let _future = + ctx.spawn(Box::pin( + future.map(|_| { () }) + )); + + active_jobs + .insert(relay_parent.clone(), abort_handle); } FromOverseer::Signal(OverseerSignal::StopWork(relay_parent)) => { if let Some(abort_handle) = @@ -140,12 +149,16 @@ impl BitfieldDistribution { } FromOverseer::Signal(OverseerSignal::Conclude) => { // @todo cannot store the future - // return futures::future::join_all( + // let _ = futures::future::join_all( // active_jobs // .drain() // .map(|(_relay_parent, (cancellation, future))| future), // ) // .await; + for (_relay_parent, abort_handle) in active_jobs.drain() { + let _ = abort_handle.abort(); + } + return Ok(()) } } } @@ -215,16 +228,25 @@ where } // @todo verify sequential execution is ok or if spawning tasks is better - // Send peers messages which are interesting to them - for (peerid, view) in tracker.peer_views.iter() + // pass on the + + let future = move || { + tracker.peer_views.clone().into_iter() .filter(|(_peerid, view)| view.contains(&hash)) - { - // @todo shall we assure these complete or just let them be? - let _ = ctx.send_message( - AllMessages::BitfieldDistribution( - BitfieldDistributionMessage::DistributeBitfield(hash.clone(), signed_availability.clone()), - )).await; - } + .map(|(peerid, view)| { + let mut ctx = ctx.clone(); + let hash = hash.clone(); + let signed_availability = signed_availability.clone(); + async move { + ctx.send_message( + AllMessages::BitfieldDistribution( + BitfieldDistributionMessage::DistributeBitfield(hash, signed_availability), + )).await + } + }) + .collect::>().into_future() + }; + future().await; } BitfieldDistributionMessage::NetworkBridgeUpdate(event) => { handle_network_msg(ctx, tracker, event).await?; @@ -307,7 +329,7 @@ where } } -/// query the validator set +/// query the validator set and signing context async fn query_basics( mut ctx: Context, relay_parent: Hash, From 79667699f530642c2c5f54be990f104aa7deab73 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Fri, 10 Jul 2020 12:58:13 +0200 Subject: [PATCH 13/45] chore --- node/network/bitfield-distribution/src/lib.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/node/network/bitfield-distribution/src/lib.rs b/node/network/bitfield-distribution/src/lib.rs index 09c3921511f9..ded5f7161133 100644 --- a/node/network/bitfield-distribution/src/lib.rs +++ b/node/network/bitfield-distribution/src/lib.rs @@ -227,9 +227,7 @@ where .await } - // @todo verify sequential execution is ok or if spawning tasks is better - // pass on the - + // concurrently pass on the bitfield distribution to all peers let future = move || { tracker.peer_views.clone().into_iter() .filter(|(_peerid, view)| view.contains(&hash)) From e8c901a28ccbaff937ef3003d4c5d508b8191298 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Fri, 10 Jul 2020 15:49:20 +0200 Subject: [PATCH 14/45] reword markdown --- .../node/availability/bitfield-distribution.md | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/roadmap/implementers-guide/src/node/availability/bitfield-distribution.md b/roadmap/implementers-guide/src/node/availability/bitfield-distribution.md index 33c8eee7bf31..7864fb3e25bb 100644 --- a/roadmap/implementers-guide/src/node/availability/bitfield-distribution.md +++ b/roadmap/implementers-guide/src/node/availability/bitfield-distribution.md @@ -6,16 +6,24 @@ Validators vote on the availability of a backed candidate by issuing signed bitf `ProtocolId`: `b"bitd"` -Input: [`BitfieldDistributionMessage`](../../types/overseer-protocol.md#bitfield-distribution-message) +Input: +[`BitfieldDistributionMessage`](../../types/overseer-protocol.md#bitfield-distribution-message) which are gossiped to all peers, no matter if validator or not. + Output: -- `NetworkBridge::RegisterEventProducer(ProtocolId)` +- `NetworkBridge::RegisterEventProducer(ProtocolId)` in order to register ourselfs as an event provide for the protocol. - `NetworkBridge::SendMessage([PeerId], ProtocolId, Bytes)` -- `NetworkBridge::ReportPeer(PeerId, cost_or_benefit)` -- `DistributeBitfield::Bitfield(relay_parent, SignedAvailabilityBitfield)` +- `NetworkBridge::ReportPeer(PeerId, cost_or_benefit)` improve or penalize the reputiation of peer based on the message we received relative to our state. +- `BitfieldDistributionMessage::DistributeBitfield(relay_parent, SignedAvailabilityBitfield)` gossip a verified incoming bitfield on to interested subsystems within this validator node. ## Functionality -This is implemented as a gossip system. Register a [network bridge](../utility/network-bridge.md) event producer on startup and track peer connection, view change, and disconnection events. Only accept bitfields relevant to our current view and only distribute bitfields to other peers when relevant to their most recent view. Check bitfield signatures in this subsystem and accept and distribute only one bitfield per validator. +This is implemented as a gossip system. Register a [network bridge](../utility/network-bridge.md) event producer on startup. + +It is necessary to track peer connection, view change, and disconnection events, in order to maintain an index of which peers are interested in which relay parent bitfields. +Before gossiping incoming bitfields on, they must be checked to be signed by one of the validators +of the validator set relevant to the current relay parent. +Only accept bitfields relevant to our current view and only distribute bitfields to other peers when relevant to their most recent view. +Accept and distribute only one bitfield per validator. When receiving a bitfield either from the network or from a `DistributeBitfield` message, forward it along to the block authorship (provisioning) subsystem for potential inclusion in a block. From e33dafe7257676ae4a7b98daf6866978b59f8257 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Fri, 10 Jul 2020 18:08:05 +0200 Subject: [PATCH 15/45] clarify --- node/network/bitfield-distribution/src/lib.rs | 213 ++++++++++-------- 1 file changed, 124 insertions(+), 89 deletions(-) diff --git a/node/network/bitfield-distribution/src/lib.rs b/node/network/bitfield-distribution/src/lib.rs index ded5f7161133..cc0d5b721667 100644 --- a/node/network/bitfield-distribution/src/lib.rs +++ b/node/network/bitfield-distribution/src/lib.rs @@ -17,13 +17,13 @@ //! The bitfield distribution subsystem spreading @todo . use codec::{Codec, Decode, Encode}; +use futures::stream::StreamExt; use futures::{ channel::oneshot, future::{abortable, AbortHandle, Abortable}, - Future, FutureExt, stream::FuturesUnordered, + Future, FutureExt, }; -use futures::stream::StreamExt; use node_primitives::{ProtocolId, SignedFullStatement, View}; use polkadot_network::protocol::Message; @@ -31,6 +31,7 @@ use polkadot_node_subsystem::messages::*; use polkadot_node_subsystem::{ FromOverseer, OverseerSignal, SpawnedSubsystem, Subsystem, SubsystemContext, SubsystemResult, }; +use polkadot_primitives::parachain::SignedAvailabilityBitfield; use polkadot_primitives::parachain::{SigningContext, ValidatorId}; use polkadot_primitives::Hash; use sc_network::ReputationChange; @@ -66,7 +67,6 @@ struct Tracker { per_job: HashMap, } - /// Data for each relay parent #[derive(Debug, Clone, Default)] struct JobData { @@ -80,8 +80,6 @@ struct JobData { validator_set: Vec, } - - fn network_update_message(n: NetworkBridgeEvent) -> AllMessages { AllMessages::BitfieldDistribution(BitfieldDistributionMessage::NetworkBridgeUpdate(n)) } @@ -110,40 +108,51 @@ impl BitfieldDistribution { loop { { let message = { - let mut ctx = ctx.clone(); - ctx.recv().await? + let mut ctx = ctx.clone(); + ctx.recv().await? }; match message { FromOverseer::Communication { msg } => { let peerid = PeerId::random(); // @todo - process_incoming(ctx.clone(), &mut tracker, peerid, msg).await?; + match msg { + // Distribute a bitfield via gossip to other validators. + BitfieldDistributionMessage::DistributeBitfield( + hash, + signed_availability, + ) => { + let msg = BitfieldGossip { + relay_parent: hash, + signed_availability, + }; + process_incoming(ctx.clone(), &mut tracker, peerid, msg).await?; + } + BitfieldDistributionMessage::NetworkBridgeUpdate(event) => { + handle_network_msg(ctx.clone(), &mut tracker, event).await?; + } + } } FromOverseer::Signal(OverseerSignal::StartWork(relay_parent)) => { let (validator_set, signing_context) = query_basics(ctx.clone(), relay_parent).await?; - let _ = tracker.per_job.insert(relay_parent, JobData { + let _ = tracker.per_job.insert( + relay_parent, + JobData { validator_bitset_received: HashSet::new(), signing_context, validator_set: validator_set, - }); + }, + ); let future = processor_per_relay_parent(ctx.clone(), relay_parent.clone()); let (future, abort_handle) = abortable(future); + let _future = ctx.spawn(Box::pin(future.map(|_| ()))); - let _future = - ctx.spawn(Box::pin( - future.map(|_| { () }) - )); - - active_jobs - .insert(relay_parent.clone(), abort_handle); + active_jobs.insert(relay_parent.clone(), abort_handle); } FromOverseer::Signal(OverseerSignal::StopWork(relay_parent)) => { - if let Some(abort_handle) = - active_jobs.remove(&relay_parent) - { + if let Some(abort_handle) = active_jobs.remove(&relay_parent) { let _ = abort_handle.abort(); } } @@ -155,10 +164,33 @@ impl BitfieldDistribution { // .map(|(_relay_parent, (cancellation, future))| future), // ) // .await; + + // better: + // let future = move || { + // tracker.peer_views.clone().into_iter() + // .filter(|(_peerid, view)| view.contains(&hash)) + // .map(|(peerid, view)| { + // let mut ctx = ctx.clone(); + // let hash = hash.clone(); + // let signed_availability = signed_availability.clone(); + // async move { + // let bytes = BitfieldGossip { + // relay_parent: hash, + // signed_availability, + // }.encode(); + // ctx.send_message( + // AllMessages::NetworkBridge( + // NetworkBridgeMessage::SendMessage(vec![], BitfieldDistribution::PROTOCOL_ID, bytes), + // )).await + // } + // }) + // .collect::>().into_future() + // }; + // future().await; for (_relay_parent, abort_handle) in active_jobs.drain() { let _ = abort_handle.abort(); } - return Ok(()) + return Ok(()); } } } @@ -168,9 +200,11 @@ impl BitfieldDistribution { } } - /// Process all requests related to one relay parent hash -async fn processor_per_relay_parent(mut ctx: Context, relay_parent: Hash) -> SubsystemResult<()> { +async fn processor_per_relay_parent( + mut ctx: Context, + relay_parent: Hash, +) -> SubsystemResult<()> { let mut tracker = Tracker::default(); loop { // todo!("consume relay parents") @@ -179,7 +213,11 @@ async fn processor_per_relay_parent(mut ctx: Context, relay_parent: Has } /// modify the reputiation, good or bad -async fn modify_reputiation(mut ctx: Context, peerid: PeerId, rep: ReputationChange) -> SubsystemResult<()> +async fn modify_reputiation( + mut ctx: Context, + peerid: PeerId, + rep: ReputationChange, +) -> SubsystemResult<()> where Context: SubsystemContext + Clone, { @@ -194,65 +232,71 @@ async fn process_incoming( mut ctx: Context, tracker: &mut Tracker, peerid: PeerId, - message: BitfieldDistributionMessage, + message: BitfieldGossip, ) -> SubsystemResult<()> where Context: SubsystemContext + Clone, { let peer_view = tracker.peer_views.get(&peerid).expect("TODO"); - match message { - // Distribute a bitfield via gossip to other validators. - BitfieldDistributionMessage::DistributeBitfield(hash, signed_availability) => { - let job_data = if let Some(job_data) = tracker.per_job.get(&hash) { - job_data - } else { - return modify_reputiation(ctx, peerid, COST_NOT_INTERESTED).await; - }; - - // @todo should we only distribute availability messages to peer if they are relevant to us - // or is the only discriminator if the peer cares about it? - if !peer_view.contains(&hash) { - // we don't care about this, the other side should have known better - return modify_reputiation(ctx, peerid, COST_NOT_INTERESTED).await; - } - - let validator_set = &job_data.validator_set; - if validator_set.len() == 0 { - return modify_reputiation(ctx.clone(), peerid, COST_MISSING_PEER_SESSION_KEY).await - } + let BitfieldGossip { relay_parent, signed_availability} = message; + + let job_data = if let Some(job_data) = tracker.per_job.get(&relay_parent) { + job_data + } else { + return modify_reputiation(ctx, peerid, COST_NOT_INTERESTED).await; + }; + + // @todo should we only distribute availability messages to peer if they are relevant to us + // or is the only discriminator if the peer cares about it? + if !peer_view.contains(&relay_parent) { + // we don't care about this, the other side should have known better + return modify_reputiation(ctx, peerid, COST_NOT_INTERESTED).await; + } - // check all validators that could have signed this message - if let Some(_) = validator_set.iter().find(|validator| { signed_availability.check_signature(&job_data.signing_context, validator).is_ok() }) { - return modify_reputiation(ctx.clone(), peerid, COST_SIGNATURE_INVALID) - .await - } + let validator_set = &job_data.validator_set; + if validator_set.len() == 0 { + return modify_reputiation(ctx.clone(), peerid, COST_MISSING_PEER_SESSION_KEY).await; + } - // concurrently pass on the bitfield distribution to all peers - let future = move || { - tracker.peer_views.clone().into_iter() - .filter(|(_peerid, view)| view.contains(&hash)) - .map(|(peerid, view)| { - let mut ctx = ctx.clone(); - let hash = hash.clone(); - let signed_availability = signed_availability.clone(); - async move { - ctx.send_message( - AllMessages::BitfieldDistribution( - BitfieldDistributionMessage::DistributeBitfield(hash, signed_availability), - )).await - } - }) - .collect::>().into_future() - }; - future().await; - } - BitfieldDistributionMessage::NetworkBridgeUpdate(event) => { - handle_network_msg(ctx, tracker, event).await?; - } + // check all validators that could have signed this message + if let Some(_) = validator_set.iter().find(|validator| { + signed_availability + .check_signature(&job_data.signing_context, validator) + .is_ok() + }) { + return modify_reputiation(ctx.clone(), peerid, COST_SIGNATURE_INVALID).await; } + + // concurrently pass on the bitfield distribution to all interested peers + let interested_peers = tracker + .peer_views + .iter() + .filter(|(_peerid, view)| view.contains(&relay_parent)) + .map(|(peerid, _)| peerid.clone()) + .collect::>(); + let message = BitfieldGossip { + relay_parent: relay_parent, + signed_availability, + }; + let bytes = message.encode(); + ctx.send_message(AllMessages::NetworkBridge( + NetworkBridgeMessage::SendMessage( + interested_peers, + BitfieldDistribution::PROTOCOL_ID, + bytes, + ), + )) + .await?; Ok(()) } +/// A Gossiped signed availability bitfield for a particular relay hash +#[derive(Debug, Clone, Encode, Decode, PartialEq, Eq)] +pub struct BitfieldGossip { + relay_parent: Hash, + signed_availability: SignedAvailabilityBitfield, +} + /// Deal with network bridge updates and track what needs to be tracked async fn handle_network_msg( mut ctx: Context, @@ -283,7 +327,9 @@ where NetworkBridgeEvent::OurViewChange(view) => { let ego = ego.clone(); let old_view = std::mem::replace(&mut (tracker.view), view); - tracker.per_job.retain(move |hash, _job_data| ego.0.contains(hash)); + tracker + .per_job + .retain(move |hash, _job_data| ego.0.contains(hash)); for new in tracker.view.difference(&old_view) { if !tracker.per_job.contains_key(&new) { @@ -293,23 +339,12 @@ where } } } - NetworkBridgeEvent::PeerMessage(remote, bytes) => { + NetworkBridgeEvent::PeerMessage(remote, mut bytes) => { log::info!("Got a peer message from {:?}", &remote); - // @todo what would we receive here? - match Message::decode(&mut bytes.as_ref()) { - Ok(message) => { - match message { - // a new session key - Message::ValidatorId(session_key) => { - // @todo update all Validator ids I guess? - // let _ = tracker - // .per_job. - // .insert(remote.clone(), session_key); - } - _ => {} - } - } - Err(_) => unimplemented!("Invalid format shall be punished I guess"), + if let Ok(gossiped_bitfield) = BitfieldGossip::decode(&mut (bytes.as_slice())) { + process_incoming(ctx, tracker, remote, gossiped_bitfield).await?; + } else { + return modify_reputiation(ctx.clone(), remote, COST_MESSAGE_NOT_DECODABLE).await; } } } From 6594deec486129c668896a8e563d57937d5cf1ec Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Tue, 14 Jul 2020 13:58:41 +0200 Subject: [PATCH 16/45] lacks abortable processing impl details --- node/network/bitfield-distribution/src/lib.rs | 216 +++++++++--------- 1 file changed, 108 insertions(+), 108 deletions(-) diff --git a/node/network/bitfield-distribution/src/lib.rs b/node/network/bitfield-distribution/src/lib.rs index cc0d5b721667..0d7b3daca5c8 100644 --- a/node/network/bitfield-distribution/src/lib.rs +++ b/node/network/bitfield-distribution/src/lib.rs @@ -17,11 +17,10 @@ //! The bitfield distribution subsystem spreading @todo . use codec::{Codec, Decode, Encode}; -use futures::stream::StreamExt; use futures::{ channel::oneshot, future::{abortable, AbortHandle, Abortable}, - stream::FuturesUnordered, + stream::{FuturesUnordered, Stream, StreamExt}, Future, FutureExt, }; @@ -41,13 +40,13 @@ use std::{ }; const COST_SIGNATURE_INVALID: ReputationChange = - ReputationChange::new(-10000, "Bitfield signature invalid"); + ReputationChange::new(-100, "Bitfield signature invalid"); const COST_MISSING_PEER_SESSION_KEY: ReputationChange = - ReputationChange::new(-1337, "Missing peer session key"); + ReputationChange::new(-133, "Missing peer session key"); const COST_MULTIPLE_BITFIELDS_FROM_PEER: ReputationChange = - ReputationChange::new(-10000, "Received more than once bitfield from peer"); + ReputationChange::new(-22, "Received more than once bitfield from peer"); const COST_NOT_INTERESTED: ReputationChange = - ReputationChange::new(-100, "Not intersted in that parent hash"); + ReputationChange::new(-51, "Not intersted in that parent hash"); const COST_MESSAGE_NOT_DECODABLE: ReputationChange = ReputationChange::new(-100, "Not intersted in that parent hash"); @@ -78,6 +77,10 @@ struct JobData { // set of validators for a particular relay_parent validator_set: Vec, + + // set of validators for a particular relay_parent and the number of messages + // received authored by them + received_bitsets_per_validator: HashSet, } fn network_update_message(n: NetworkBridgeEvent) -> AllMessages { @@ -101,9 +104,8 @@ impl BitfieldDistribution { // set of active heads the overseer told us to work on with the connected // tasks abort handles - // @todo do we need Box>) for anything? - // let mut active_jobs = HashMap::>>)>::new(); let mut active_jobs = HashMap::::new(); + let mut futurama = HashMap::>>>::new(); let mut tracker = Tracker::default(); loop { { @@ -113,21 +115,21 @@ impl BitfieldDistribution { }; match message { FromOverseer::Communication { msg } => { - let peerid = PeerId::random(); // @todo + // we signed this bitfield match msg { // Distribute a bitfield via gossip to other validators. BitfieldDistributionMessage::DistributeBitfield( hash, signed_availability, ) => { - let msg = BitfieldGossip { + let msg = BitfieldGossipMessage { relay_parent: hash, signed_availability, }; - process_incoming(ctx.clone(), &mut tracker, peerid, msg).await?; + distribute(ctx.clone(), &mut tracker, msg).await?; } BitfieldDistributionMessage::NetworkBridgeUpdate(event) => { - handle_network_msg(ctx.clone(), &mut tracker, event).await?; + handle_network_msg(ctx.clone(), &mut tracker, event).await; } } } @@ -138,58 +140,39 @@ impl BitfieldDistribution { let _ = tracker.per_job.insert( relay_parent, JobData { - validator_bitset_received: HashSet::new(), signing_context, validator_set: validator_set, + ..Default::default() }, ); - - let future = processor_per_relay_parent(ctx.clone(), relay_parent.clone()); - let (future, abort_handle) = abortable(future); - - let _future = ctx.spawn(Box::pin(future.map(|_| ()))); - - active_jobs.insert(relay_parent.clone(), abort_handle); + // futurama.insert(relay_parent.clone(), Box::pin(future.map(|_| ()))); + // active_jobs.insert(relay_parent.clone(), abort_handle); } FromOverseer::Signal(OverseerSignal::StopWork(relay_parent)) => { - if let Some(abort_handle) = active_jobs.remove(&relay_parent) { - let _ = abort_handle.abort(); - } + // if let Some(abort_handle) = active_jobs.remove(&relay_parent) { + // let _ = abort_handle.abort(); + // } + // let _ = futurama.remove(&relay_parent); + let _ = tracker.per_job.remove(&relay_parent); } FromOverseer::Signal(OverseerSignal::Conclude) => { - // @todo cannot store the future - // let _ = futures::future::join_all( - // active_jobs - // .drain() - // .map(|(_relay_parent, (cancellation, future))| future), - // ) - // .await; - - // better: - // let future = move || { - // tracker.peer_views.clone().into_iter() - // .filter(|(_peerid, view)| view.contains(&hash)) - // .map(|(peerid, view)| { - // let mut ctx = ctx.clone(); - // let hash = hash.clone(); - // let signed_availability = signed_availability.clone(); - // async move { - // let bytes = BitfieldGossip { - // relay_parent: hash, - // signed_availability, - // }.encode(); - // ctx.send_message( - // AllMessages::NetworkBridge( - // NetworkBridgeMessage::SendMessage(vec![], BitfieldDistribution::PROTOCOL_ID, bytes), - // )).await - // } + tracker.per_job.clear(); + // let unordered = futurama + // .drain() + // .map(|(_relay_parent, future)| future) + // .zip( + // active_jobs + // .drain() + // .map(|(_relay_parent, abort_handle)| abort_handle), + // ) + // .map(|(future, abort_handle)| { + // abort_handle.abort(); + // // TODO pipe to cleanup state + // future // }) - // .collect::>().into_future() - // }; - // future().await; - for (_relay_parent, abort_handle) in active_jobs.drain() { - let _ = abort_handle.abort(); - } + // .collect::>(); + + // let _ = async move { unordered.into_future().await }.await; return Ok(()); } } @@ -200,18 +183,6 @@ impl BitfieldDistribution { } } -/// Process all requests related to one relay parent hash -async fn processor_per_relay_parent( - mut ctx: Context, - relay_parent: Hash, -) -> SubsystemResult<()> { - let mut tracker = Tracker::default(); - loop { - // todo!("consume relay parents") - } - Ok(()) -} - /// modify the reputiation, good or bad async fn modify_reputiation( mut ctx: Context, @@ -227,20 +198,59 @@ where .await } +/// Distribute a checked message, either originated by us or gossiped on from other peers. +async fn distribute( + mut ctx: Context, + tracker: &mut Tracker, + message: BitfieldGossipMessage, +) -> SubsystemResult<()> +where + Context: SubsystemContext + Clone, +{ + let BitfieldGossipMessage { + relay_parent, + signed_availability, + } = message; + // concurrently pass on the bitfield distribution to all interested peers + let interested_peers = tracker + .peer_views + .iter() + .filter(|(_peerid, view)| view.contains(&relay_parent)) + .map(|(peerid, _)| peerid.clone()) + .collect::>(); + let message = BitfieldGossipMessage { + relay_parent: relay_parent, + signed_availability, + }; + let bytes = message.encode(); + ctx.send_message(AllMessages::NetworkBridge( + NetworkBridgeMessage::SendMessage( + interested_peers, + BitfieldDistribution::PROTOCOL_ID, + bytes, + ), + )) + .await?; + Ok(()) +} + /// Handle an incoming message from a peer -async fn process_incoming( +async fn process_incoming_peer_message( mut ctx: Context, tracker: &mut Tracker, peerid: PeerId, - message: BitfieldGossip, + message: BitfieldGossipMessage, ) -> SubsystemResult<()> where Context: SubsystemContext + Clone, { - let peer_view = tracker.peer_views.get(&peerid).expect("TODO"); - let BitfieldGossip { relay_parent, signed_availability} = message; + let peer_view = if let Some(peer_view) = tracker.peer_views.get(&peerid) { + peer_view + } else { + return modify_reputiation(ctx, peerid, COST_NOT_INTERESTED).await; + }; - let job_data = if let Some(job_data) = tracker.per_job.get(&relay_parent) { + let job_data = if let Some(job_data) = tracker.per_job.get(&message.relay_parent) { job_data } else { return modify_reputiation(ctx, peerid, COST_NOT_INTERESTED).await; @@ -248,51 +258,34 @@ where // @todo should we only distribute availability messages to peer if they are relevant to us // or is the only discriminator if the peer cares about it? - if !peer_view.contains(&relay_parent) { + if !peer_view.contains(&message.relay_parent) { // we don't care about this, the other side should have known better return modify_reputiation(ctx, peerid, COST_NOT_INTERESTED).await; } let validator_set = &job_data.validator_set; if validator_set.len() == 0 { - return modify_reputiation(ctx.clone(), peerid, COST_MISSING_PEER_SESSION_KEY).await; + return modify_reputiation(ctx, peerid, COST_MISSING_PEER_SESSION_KEY).await; } // check all validators that could have signed this message - if let Some(_) = validator_set.iter().find(|validator| { - signed_availability + if validator_set.iter().find(|validator| { + message + .signed_availability .check_signature(&job_data.signing_context, validator) .is_ok() - }) { - return modify_reputiation(ctx.clone(), peerid, COST_SIGNATURE_INVALID).await; + }).is_none() { + return modify_reputiation(ctx, peerid, COST_SIGNATURE_INVALID).await; } - // concurrently pass on the bitfield distribution to all interested peers - let interested_peers = tracker - .peer_views - .iter() - .filter(|(_peerid, view)| view.contains(&relay_parent)) - .map(|(peerid, _)| peerid.clone()) - .collect::>(); - let message = BitfieldGossip { - relay_parent: relay_parent, - signed_availability, - }; - let bytes = message.encode(); - ctx.send_message(AllMessages::NetworkBridge( - NetworkBridgeMessage::SendMessage( - interested_peers, - BitfieldDistribution::PROTOCOL_ID, - bytes, - ), - )) - .await?; + distribute(ctx, tracker, message).await?; + Ok(()) } -/// A Gossiped signed availability bitfield for a particular relay hash +/// A gossiped or gossipable signed availability bitfield for a particular relay hash #[derive(Debug, Clone, Encode, Decode, PartialEq, Eq)] -pub struct BitfieldGossip { +pub struct BitfieldGossipMessage { relay_parent: Hash, signed_availability: SignedAvailabilityBitfield, } @@ -341,10 +334,15 @@ where } NetworkBridgeEvent::PeerMessage(remote, mut bytes) => { log::info!("Got a peer message from {:?}", &remote); - if let Ok(gossiped_bitfield) = BitfieldGossip::decode(&mut (bytes.as_slice())) { - process_incoming(ctx, tracker, remote, gossiped_bitfield).await?; + if let Ok(gossiped_bitfield) = BitfieldGossipMessage::decode(&mut (bytes.as_slice())) { + let (future, _abort_handle) = abortable(process_incoming_peer_message(ctx, tracker, remote, gossiped_bitfield)); + future.await; + // tracker.active_jobs.insert(&gossiped_bitfield.relay_parent, abort_handle); + // let _future = ctx.spawn(Box::pin(async move { + // future.map(|_| ()).await + // })); } else { - return modify_reputiation(ctx.clone(), remote, COST_MESSAGE_NOT_DECODABLE).await; + return modify_reputiation(ctx, remote, COST_MESSAGE_NOT_DECODABLE).await; } } } @@ -353,12 +351,10 @@ where impl Subsystem for BitfieldDistribution where - C: SubsystemContext + Clone + Sync, + C: SubsystemContext + Clone + Sync + Send, { fn start(self, ctx: C) -> SpawnedSubsystem { - SpawnedSubsystem(Box::pin(async move { - Self::run(ctx).await; - })) + SpawnedSubsystem(Box::pin(async move { Self::run(ctx) }.map(|_| ()))) } } @@ -393,8 +389,12 @@ where mod test { use super::*; + fn generate_valid_message() -> AllMessages {} + + fn generate_invalid_message() -> AllMessages {} + #[test] - fn x() { + fn game_changer() { // @todo } } From 4c6f9b458c7649587446bdf6cc6e94fcf1ac0404 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Tue, 14 Jul 2020 16:46:43 +0200 Subject: [PATCH 17/45] slimify --- node/network/bitfield-distribution/src/lib.rs | 119 +++++++----------- 1 file changed, 46 insertions(+), 73 deletions(-) diff --git a/node/network/bitfield-distribution/src/lib.rs b/node/network/bitfield-distribution/src/lib.rs index 0d7b3daca5c8..5f5a3957b725 100644 --- a/node/network/bitfield-distribution/src/lib.rs +++ b/node/network/bitfield-distribution/src/lib.rs @@ -16,7 +16,7 @@ //! The bitfield distribution subsystem spreading @todo . -use codec::{Codec, Decode, Encode}; +use codec::{Decode, Encode}; use futures::{ channel::oneshot, future::{abortable, AbortHandle, Abortable}, @@ -24,8 +24,8 @@ use futures::{ Future, FutureExt, }; -use node_primitives::{ProtocolId, SignedFullStatement, View}; -use polkadot_network::protocol::Message; +use node_primitives::{ProtocolId, View}; + use polkadot_node_subsystem::messages::*; use polkadot_node_subsystem::{ FromOverseer, OverseerSignal, SpawnedSubsystem, Subsystem, SubsystemContext, SubsystemResult, @@ -43,13 +43,12 @@ const COST_SIGNATURE_INVALID: ReputationChange = ReputationChange::new(-100, "Bitfield signature invalid"); const COST_MISSING_PEER_SESSION_KEY: ReputationChange = ReputationChange::new(-133, "Missing peer session key"); -const COST_MULTIPLE_BITFIELDS_FROM_PEER: ReputationChange = - ReputationChange::new(-22, "Received more than once bitfield from peer"); const COST_NOT_INTERESTED: ReputationChange = ReputationChange::new(-51, "Not intersted in that parent hash"); const COST_MESSAGE_NOT_DECODABLE: ReputationChange = ReputationChange::new(-100, "Not intersted in that parent hash"); + #[derive(Default, Clone)] struct Tracker { // track all active peers and their views @@ -59,16 +58,13 @@ struct Tracker { // our current view view: View, - // signing context for a particular relay_parent - jobs: HashMap, - // set of validators for a particular relay_parent - per_job: HashMap, + per_relay_parent: HashMap, } /// Data for each relay parent #[derive(Debug, Clone, Default)] -struct JobData { +struct PerRelayParentData { // set of validators which already sent a message validator_bitset_received: HashSet, @@ -80,7 +76,7 @@ struct JobData { // set of validators for a particular relay_parent and the number of messages // received authored by them - received_bitsets_per_validator: HashSet, + one_per_validator: HashSet, } fn network_update_message(n: NetworkBridgeEvent) -> AllMessages { @@ -104,8 +100,6 @@ impl BitfieldDistribution { // set of active heads the overseer told us to work on with the connected // tasks abort handles - let mut active_jobs = HashMap::::new(); - let mut futurama = HashMap::>>>::new(); let mut tracker = Tracker::default(); loop { { @@ -126,6 +120,8 @@ impl BitfieldDistribution { relay_parent: hash, signed_availability, }; + // @todo do we also make sure to not send it twice if we are the source + // @todo this also apply to ourself? distribute(ctx.clone(), &mut tracker, msg).await?; } BitfieldDistributionMessage::NetworkBridgeUpdate(event) => { @@ -137,9 +133,9 @@ impl BitfieldDistribution { let (validator_set, signing_context) = query_basics(ctx.clone(), relay_parent).await?; - let _ = tracker.per_job.insert( + let _ = tracker.per_relay_parent.insert( relay_parent, - JobData { + PerRelayParentData { signing_context, validator_set: validator_set, ..Default::default() @@ -149,30 +145,10 @@ impl BitfieldDistribution { // active_jobs.insert(relay_parent.clone(), abort_handle); } FromOverseer::Signal(OverseerSignal::StopWork(relay_parent)) => { - // if let Some(abort_handle) = active_jobs.remove(&relay_parent) { - // let _ = abort_handle.abort(); - // } - // let _ = futurama.remove(&relay_parent); - let _ = tracker.per_job.remove(&relay_parent); + let _ = tracker.per_relay_parent.remove(&relay_parent); } FromOverseer::Signal(OverseerSignal::Conclude) => { - tracker.per_job.clear(); - // let unordered = futurama - // .drain() - // .map(|(_relay_parent, future)| future) - // .zip( - // active_jobs - // .drain() - // .map(|(_relay_parent, abort_handle)| abort_handle), - // ) - // .map(|(future, abort_handle)| { - // abort_handle.abort(); - // // TODO pipe to cleanup state - // future - // }) - // .collect::>(); - - // let _ = async move { unordered.into_future().await }.await; + tracker.per_relay_parent.clear(); return Ok(()); } } @@ -207,6 +183,7 @@ async fn distribute( where Context: SubsystemContext + Clone, { + let BitfieldGossipMessage { relay_parent, signed_availability, @@ -236,7 +213,7 @@ where /// Handle an incoming message from a peer async fn process_incoming_peer_message( - mut ctx: Context, + ctx: Context, tracker: &mut Tracker, peerid: PeerId, message: BitfieldGossipMessage, @@ -244,40 +221,44 @@ async fn process_incoming_peer_message( where Context: SubsystemContext + Clone, { - let peer_view = if let Some(peer_view) = tracker.peer_views.get(&peerid) { - peer_view - } else { + // we don't care about this, not part of our view + if !tracker.view.contains(&message.relay_parent) { return modify_reputiation(ctx, peerid, COST_NOT_INTERESTED).await; - }; + } - let job_data = if let Some(job_data) = tracker.per_job.get(&message.relay_parent) { + // Ignore anything the overseer did not tell this subsystem to work on + let mut job_data = tracker.per_relay_parent.get_mut(&message.relay_parent); + let job_data: &mut _ = if let Some(ref mut job_data) = job_data { job_data } else { return modify_reputiation(ctx, peerid, COST_NOT_INTERESTED).await; }; - // @todo should we only distribute availability messages to peer if they are relevant to us - // or is the only discriminator if the peer cares about it? - if !peer_view.contains(&message.relay_parent) { - // we don't care about this, the other side should have known better - return modify_reputiation(ctx, peerid, COST_NOT_INTERESTED).await; - } - let validator_set = &job_data.validator_set; if validator_set.len() == 0 { return modify_reputiation(ctx, peerid, COST_MISSING_PEER_SESSION_KEY).await; } // check all validators that could have signed this message - if validator_set.iter().find(|validator| { + // @todo there must be a better way figuring this out cheaply + let signing_context = job_data.signing_context.clone(); + if let Some(validator) = validator_set.iter().find(|validator| { message .signed_availability - .check_signature(&job_data.signing_context, validator) + .check_signature(&signing_context, validator) .is_ok() - }).is_none() { + }) { + let one_per_validator = &mut (job_data.one_per_validator); + // only distribute a message of a validator once + if one_per_validator.contains(validator) { + return Ok(()) + } + one_per_validator.insert(validator.clone()); + } else { return modify_reputiation(ctx, peerid, COST_SIGNATURE_INVALID).await; } + // passed all conditions, distribute! distribute(ctx, tracker, message).await?; Ok(()) @@ -292,15 +273,13 @@ pub struct BitfieldGossipMessage { /// Deal with network bridge updates and track what needs to be tracked async fn handle_network_msg( - mut ctx: Context, + ctx: Context, tracker: &mut Tracker, bridge_message: NetworkBridgeEvent, ) -> SubsystemResult<()> where Context: SubsystemContext + Clone, { - let peer_views = &mut tracker.peer_views; - let per_job = &mut tracker.per_job; let ego = &((*tracker).view); match bridge_message { NetworkBridgeEvent::PeerConnected(peerid, _role) => { @@ -318,29 +297,18 @@ where .and_modify(|val| *val = view); } NetworkBridgeEvent::OurViewChange(view) => { - let ego = ego.clone(); let old_view = std::mem::replace(&mut (tracker.view), view); - tracker - .per_job - .retain(move |hash, _job_data| ego.0.contains(hash)); for new in tracker.view.difference(&old_view) { - if !tracker.per_job.contains_key(&new) { - log::warn!("Active head running that's not active anymore, go catch it") - //@todo rephrase - //@todo should we get rid of that right here + if !tracker.per_relay_parent.contains_key(&new) { + log::warn!("Our view contains {} but the overseer never told use we should work on this", &new); } } } - NetworkBridgeEvent::PeerMessage(remote, mut bytes) => { + NetworkBridgeEvent::PeerMessage(remote, bytes) => { log::info!("Got a peer message from {:?}", &remote); if let Ok(gossiped_bitfield) = BitfieldGossipMessage::decode(&mut (bytes.as_slice())) { - let (future, _abort_handle) = abortable(process_incoming_peer_message(ctx, tracker, remote, gossiped_bitfield)); - future.await; - // tracker.active_jobs.insert(&gossiped_bitfield.relay_parent, abort_handle); - // let _future = ctx.spawn(Box::pin(async move { - // future.map(|_| ()).await - // })); + process_incoming_peer_message(ctx, tracker, remote, gossiped_bitfield).await; } else { return modify_reputiation(ctx, remote, COST_MESSAGE_NOT_DECODABLE).await; } @@ -389,9 +357,14 @@ where mod test { use super::*; - fn generate_valid_message() -> AllMessages {} + fn generate_valid_message() -> AllMessages { + // AllMessages::BitfieldDistribution(BitfieldDistributionMessage::DistributeBitfield()) + unimplemented!() + } - fn generate_invalid_message() -> AllMessages {} + fn generate_invalid_message() -> AllMessages { + unimplemented!() + } #[test] fn game_changer() { From af2849b247473a5d7ef3f347a54c7a3451b8b1c9 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Wed, 15 Jul 2020 17:31:40 +0200 Subject: [PATCH 18/45] fix: warnings and avoid ctx.clone() improve comments --- node/network/bitfield-distribution/src/lib.rs | 122 +++++++++--------- 1 file changed, 64 insertions(+), 58 deletions(-) diff --git a/node/network/bitfield-distribution/src/lib.rs b/node/network/bitfield-distribution/src/lib.rs index 5f5a3957b725..fbd3d91117a8 100644 --- a/node/network/bitfield-distribution/src/lib.rs +++ b/node/network/bitfield-distribution/src/lib.rs @@ -14,14 +14,16 @@ // You should have received a copy of the GNU General Public License // along with Polkadot. If not, see . -//! The bitfield distribution subsystem spreading @todo . +//! The bitfield distribution +//! +//! In case this node is a validator, gossips its own signed availability bitfield +//! for a particular relay parent. +//! Independently of that, gossips on received messages from peers to other interested peers. use codec::{Decode, Encode}; use futures::{ channel::oneshot, - future::{abortable, AbortHandle, Abortable}, - stream::{FuturesUnordered, Stream, StreamExt}, - Future, FutureExt, + FutureExt, }; use node_primitives::{ProtocolId, View}; @@ -30,14 +32,10 @@ use polkadot_node_subsystem::messages::*; use polkadot_node_subsystem::{ FromOverseer, OverseerSignal, SpawnedSubsystem, Subsystem, SubsystemContext, SubsystemResult, }; -use polkadot_primitives::parachain::SignedAvailabilityBitfield; -use polkadot_primitives::parachain::{SigningContext, ValidatorId}; -use polkadot_primitives::Hash; +use polkadot_primitives::v1::{Hash, SignedAvailabilityBitfield}; +use polkadot_primitives::v0::{SigningContext, ValidatorId}; use sc_network::ReputationChange; -use std::{ - collections::{HashMap, HashSet}, - pin::Pin, -}; +use std::collections::{HashMap, HashSet}; const COST_SIGNATURE_INVALID: ReputationChange = ReputationChange::new(-100, "Bitfield signature invalid"); @@ -49,33 +47,33 @@ const COST_MESSAGE_NOT_DECODABLE: ReputationChange = ReputationChange::new(-100, "Not intersted in that parent hash"); +/// Data used to track information of peers and relay parents the +/// overseer ordered us to work on. #[derive(Default, Clone)] struct Tracker { - // track all active peers and their views - // to determine what is relevant to them + /// track all active peers and their views + /// to determine what is relevant to them. peer_views: HashMap, - // our current view + /// Our current view. view: View, - // set of validators for a particular relay_parent + /// Additional data particular to a relay parent. per_relay_parent: HashMap, } -/// Data for each relay parent +/// Data for a particular relay parent. #[derive(Debug, Clone, Default)] struct PerRelayParentData { - // set of validators which already sent a message - validator_bitset_received: HashSet, - - // signing context for a particular relay_parent + /// Signing context for a particular relay parent. signing_context: SigningContext, - // set of validators for a particular relay_parent + /// Set of validators for a particular relay parent. validator_set: Vec, - // set of validators for a particular relay_parent and the number of messages - // received authored by them + /// Set of validators for a particular relay parent for which we + /// received a valid `BitfieldGossipMessage` and gossiped it to + /// interested peers. one_per_validator: HashSet, } @@ -83,11 +81,14 @@ fn network_update_message(n: NetworkBridgeEvent) -> AllMessages { AllMessages::BitfieldDistribution(BitfieldDistributionMessage::NetworkBridgeUpdate(n)) } +/// The bitfield distribution subsystem. pub struct BitfieldDistribution; impl BitfieldDistribution { + /// The protocol identifier for bitfield distribution. const PROTOCOL_ID: ProtocolId = *b"bitd"; + /// Start processing work as passed on from the Overseer. async fn run(mut ctx: Context) -> SubsystemResult<()> where Context: SubsystemContext + Clone, @@ -98,20 +99,16 @@ impl BitfieldDistribution { )) .await?; - // set of active heads the overseer told us to work on with the connected - // tasks abort handles + // work: process incoming messages from the overseer and process accordingly. let mut tracker = Tracker::default(); loop { { - let message = { - let mut ctx = ctx.clone(); - ctx.recv().await? - }; + let message = ctx.recv().await?; match message { FromOverseer::Communication { msg } => { - // we signed this bitfield + // another subsystem created this signed availability bitfield messages match msg { - // Distribute a bitfield via gossip to other validators. + // distribute a bitfield via gossip to other validators. BitfieldDistributionMessage::DistributeBitfield( hash, signed_availability, @@ -120,31 +117,36 @@ impl BitfieldDistribution { relay_parent: hash, signed_availability, }; - // @todo do we also make sure to not send it twice if we are the source + // @todo are we subject to sending something multiple times? // @todo this also apply to ourself? - distribute(ctx.clone(), &mut tracker, msg).await?; + distribute(&mut ctx, &mut tracker, msg).await?; } BitfieldDistributionMessage::NetworkBridgeUpdate(event) => { - handle_network_msg(ctx.clone(), &mut tracker, event).await; + // a network message was received + if let Err(e) = handle_network_msg(&mut ctx, &mut tracker, event).await { + log::warn!("Failed to handle incomming network messages: {:?}", e); + } } } } FromOverseer::Signal(OverseerSignal::StartWork(relay_parent)) => { + // query basic system parameters once + // @todo assumption: these cannot change within a session let (validator_set, signing_context) = - query_basics(ctx.clone(), relay_parent).await?; + query_basics(&mut ctx, relay_parent).await?; let _ = tracker.per_relay_parent.insert( relay_parent, PerRelayParentData { signing_context, - validator_set: validator_set, + validator_set, ..Default::default() }, ); - // futurama.insert(relay_parent.clone(), Box::pin(future.map(|_| ()))); - // active_jobs.insert(relay_parent.clone(), abort_handle); } FromOverseer::Signal(OverseerSignal::StopWork(relay_parent)) => { + // @todo assumption: it is good enough to prevent addition work from being + // scheduled, the individual futures are supposedly completed quickly let _ = tracker.per_relay_parent.remove(&relay_parent); } FromOverseer::Signal(OverseerSignal::Conclude) => { @@ -153,15 +155,13 @@ impl BitfieldDistribution { } } } - // active_jobs - // .retain(|_, future| future.poll().is_pending()); } } } -/// modify the reputiation, good or bad +/// Modify the reputiation of peer based on their behaviour. async fn modify_reputiation( - mut ctx: Context, + ctx: &mut Context, peerid: PeerId, rep: ReputationChange, ) -> SubsystemResult<()> @@ -174,16 +174,17 @@ where .await } -/// Distribute a checked message, either originated by us or gossiped on from other peers. +/// Distribute a given valid bitfield message. +/// +/// Can be originated by another subsystem or received via network from another peer. async fn distribute( - mut ctx: Context, + ctx: &mut Context, tracker: &mut Tracker, message: BitfieldGossipMessage, ) -> SubsystemResult<()> where Context: SubsystemContext + Clone, { - let BitfieldGossipMessage { relay_parent, signed_availability, @@ -192,11 +193,17 @@ where let interested_peers = tracker .peer_views .iter() - .filter(|(_peerid, view)| view.contains(&relay_parent)) - .map(|(peerid, _)| peerid.clone()) + .filter_map(|(peerid, view)| { + if view.contains(&relay_parent) { + Some(peerid.clone()) + } else { + None + } + }) .collect::>(); + let message = BitfieldGossipMessage { - relay_parent: relay_parent, + relay_parent, signed_availability, }; let bytes = message.encode(); @@ -211,9 +218,9 @@ where Ok(()) } -/// Handle an incoming message from a peer +/// Handle an incoming message from a peer. async fn process_incoming_peer_message( - ctx: Context, + ctx: &mut Context, tracker: &mut Tracker, peerid: PeerId, message: BitfieldGossipMessage, @@ -228,7 +235,7 @@ where // Ignore anything the overseer did not tell this subsystem to work on let mut job_data = tracker.per_relay_parent.get_mut(&message.relay_parent); - let job_data: &mut _ = if let Some(ref mut job_data) = job_data { + let job_data: &mut _ = if let Some(ref mut job_data) = job_data { job_data } else { return modify_reputiation(ctx, peerid, COST_NOT_INTERESTED).await; @@ -251,10 +258,10 @@ where let one_per_validator = &mut (job_data.one_per_validator); // only distribute a message of a validator once if one_per_validator.contains(validator) { - return Ok(()) + return Ok(()); } one_per_validator.insert(validator.clone()); - } else { + } else { return modify_reputiation(ctx, peerid, COST_SIGNATURE_INVALID).await; } @@ -264,7 +271,7 @@ where Ok(()) } -/// A gossiped or gossipable signed availability bitfield for a particular relay hash +/// A gossiped or gossipable signed availability bitfield for a particular relay parent. #[derive(Debug, Clone, Encode, Decode, PartialEq, Eq)] pub struct BitfieldGossipMessage { relay_parent: Hash, @@ -273,14 +280,13 @@ pub struct BitfieldGossipMessage { /// Deal with network bridge updates and track what needs to be tracked async fn handle_network_msg( - ctx: Context, + ctx: &mut Context, tracker: &mut Tracker, bridge_message: NetworkBridgeEvent, ) -> SubsystemResult<()> where Context: SubsystemContext + Clone, { - let ego = &((*tracker).view); match bridge_message { NetworkBridgeEvent::PeerConnected(peerid, _role) => { // insert if none already present @@ -308,7 +314,7 @@ where NetworkBridgeEvent::PeerMessage(remote, bytes) => { log::info!("Got a peer message from {:?}", &remote); if let Ok(gossiped_bitfield) = BitfieldGossipMessage::decode(&mut (bytes.as_slice())) { - process_incoming_peer_message(ctx, tracker, remote, gossiped_bitfield).await; + process_incoming_peer_message(ctx, tracker, remote, gossiped_bitfield).await?; } else { return modify_reputiation(ctx, remote, COST_MESSAGE_NOT_DECODABLE).await; } @@ -328,7 +334,7 @@ where /// query the validator set and signing context async fn query_basics( - mut ctx: Context, + ctx: &mut Context, relay_parent: Hash, ) -> SubsystemResult<(Vec, SigningContext)> where From 977102b1630c06c59e270a6e48c18404621c4c20 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Mon, 20 Jul 2020 13:54:53 +0200 Subject: [PATCH 19/45] review comments --- node/network/bitfield-distribution/src/lib.rs | 152 ++++++++++++------ node/subsystem/src/messages.rs | 3 +- .../availability/bitfield-distribution.md | 14 +- 3 files changed, 118 insertions(+), 51 deletions(-) diff --git a/node/network/bitfield-distribution/src/lib.rs b/node/network/bitfield-distribution/src/lib.rs index fbd3d91117a8..8898894e7c81 100644 --- a/node/network/bitfield-distribution/src/lib.rs +++ b/node/network/bitfield-distribution/src/lib.rs @@ -1,12 +1,12 @@ // Copyright 2020 Parity Technologies (UK) Ltd. // This file is part of Polkadot. -// Polkadot is free software: you can redistribute it and/or modify +// Polkadot is free software: you can rerelay_message 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. -// Polkadot is distributed in the hope that it will be useful, +// Polkadot is relay_messaged 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. @@ -47,6 +47,17 @@ const COST_MESSAGE_NOT_DECODABLE: ReputationChange = ReputationChange::new(-100, "Not intersted in that parent hash"); +/// Checked signed availability bitfield that is distributed +/// to other peers. +#[derive(Encode, Decode, Debug, Clone)] +pub struct BitfieldGossipMessage { + /// The relay parent this message is relative to. + pub relay_parent: Hash, + /// The actual signed availability bitfield. + pub signed_availability: SignedAvailabilityBitfield, +} + + /// Data used to track information of peers and relay parents the /// overseer ordered us to work on. #[derive(Default, Clone)] @@ -72,9 +83,23 @@ struct PerRelayParentData { validator_set: Vec, /// Set of validators for a particular relay parent for which we - /// received a valid `BitfieldGossipMessage` and gossiped it to - /// interested peers. - one_per_validator: HashSet, + /// received a valid `BitfieldGossipMessage`. + /// Also serves as the list of known messages for peers connecting + /// after bitfield gossips were already received. + one_per_validator: HashMap, + + /// which messages of which validators were already sent + message_sent_to_peer: HashMap>, +} + +impl PerRelayParentData { + fn peer_already_was_sent_message_signed_by_validator(&self, peer: &PeerId, validator: &ValidatorId) -> bool { + if let Some(set) = self.message_sent_to_peer.get(peer) { + !set.contains(validator) + } else { + false + } + } } fn network_update_message(n: NetworkBridgeEvent) -> AllMessages { @@ -108,7 +133,7 @@ impl BitfieldDistribution { FromOverseer::Communication { msg } => { // another subsystem created this signed availability bitfield messages match msg { - // distribute a bitfield via gossip to other validators. + // relay_message a bitfield via gossip to other validators BitfieldDistributionMessage::DistributeBitfield( hash, signed_availability, @@ -119,12 +144,12 @@ impl BitfieldDistribution { }; // @todo are we subject to sending something multiple times? // @todo this also apply to ourself? - distribute(&mut ctx, &mut tracker, msg).await?; + relay_message(&mut ctx, &mut tracker, msg).await?; } BitfieldDistributionMessage::NetworkBridgeUpdate(event) => { // a network message was received if let Err(e) = handle_network_msg(&mut ctx, &mut tracker, event).await { - log::warn!("Failed to handle incomming network messages: {:?}", e); + log::warn!(target: "bitd", "Failed to handle incomming network messages: {:?}", e); } } } @@ -145,7 +170,7 @@ impl BitfieldDistribution { ); } FromOverseer::Signal(OverseerSignal::StopWork(relay_parent)) => { - // @todo assumption: it is good enough to prevent addition work from being + // @todo assumption: it is good enough to prevent additional work from being // scheduled, the individual futures are supposedly completed quickly let _ = tracker.per_relay_parent.remove(&relay_parent); } @@ -159,8 +184,8 @@ impl BitfieldDistribution { } } -/// Modify the reputiation of peer based on their behaviour. -async fn modify_reputiation( +/// Modify the reputation of peer based on their behaviour. +async fn modify_reputation( ctx: &mut Context, peerid: PeerId, rep: ReputationChange, @@ -177,7 +202,7 @@ where /// Distribute a given valid bitfield message. /// /// Can be originated by another subsystem or received via network from another peer. -async fn distribute( +async fn relay_message( ctx: &mut Context, tracker: &mut Tracker, message: BitfieldGossipMessage, @@ -185,16 +210,16 @@ async fn distribute( where Context: SubsystemContext + Clone, { - let BitfieldGossipMessage { - relay_parent, - signed_availability, - } = message; // concurrently pass on the bitfield distribution to all interested peers let interested_peers = tracker .peer_views .iter() + .filter(|(peerid, view)| { + // @todo + true + }) .filter_map(|(peerid, view)| { - if view.contains(&relay_parent) { + if view.contains(&message.relay_parent) { Some(peerid.clone()) } else { None @@ -202,11 +227,7 @@ where }) .collect::>(); - let message = BitfieldGossipMessage { - relay_parent, - signed_availability, - }; - let bytes = message.encode(); + let bytes = Encode::encode(&message); ctx.send_message(AllMessages::NetworkBridge( NetworkBridgeMessage::SendMessage( interested_peers, @@ -218,11 +239,12 @@ where Ok(()) } + /// Handle an incoming message from a peer. async fn process_incoming_peer_message( ctx: &mut Context, tracker: &mut Tracker, - peerid: PeerId, + origin: PeerId, message: BitfieldGossipMessage, ) -> SubsystemResult<()> where @@ -230,7 +252,7 @@ where { // we don't care about this, not part of our view if !tracker.view.contains(&message.relay_parent) { - return modify_reputiation(ctx, peerid, COST_NOT_INTERESTED).await; + return modify_reputation(ctx, origin, COST_NOT_INTERESTED).await; } // Ignore anything the overseer did not tell this subsystem to work on @@ -238,12 +260,12 @@ where let job_data: &mut _ = if let Some(ref mut job_data) = job_data { job_data } else { - return modify_reputiation(ctx, peerid, COST_NOT_INTERESTED).await; + return modify_reputation(ctx, origin, COST_NOT_INTERESTED).await; }; let validator_set = &job_data.validator_set; if validator_set.len() == 0 { - return modify_reputiation(ctx, peerid, COST_MISSING_PEER_SESSION_KEY).await; + return modify_reputation(ctx, origin, COST_MISSING_PEER_SESSION_KEY).await; } // check all validators that could have signed this message @@ -256,28 +278,32 @@ where .is_ok() }) { let one_per_validator = &mut (job_data.one_per_validator); - // only distribute a message of a validator once - if one_per_validator.contains(validator) { + // only relay_message a message of a validator once + if one_per_validator.get(validator).is_some() { return Ok(()); } - one_per_validator.insert(validator.clone()); + one_per_validator.insert(validator.clone(), message.clone()); + + + // track which messages that peer already received + let message_sent_to_peer = &mut (job_data.message_sent_to_peer); + message_sent_to_peer + .entry(origin) + .or_insert_with(|| { + HashSet::with_capacity(16) + }) + .insert(validator.clone()); + } else { - return modify_reputiation(ctx, peerid, COST_SIGNATURE_INVALID).await; + return modify_reputation(ctx, origin, COST_SIGNATURE_INVALID).await; } - // passed all conditions, distribute! - distribute(ctx, tracker, message).await?; + // passed all conditions, distribute to peers! + relay_message(ctx, tracker, message).await?; Ok(()) } -/// A gossiped or gossipable signed availability bitfield for a particular relay parent. -#[derive(Debug, Clone, Encode, Decode, PartialEq, Eq)] -pub struct BitfieldGossipMessage { - relay_parent: Hash, - signed_availability: SignedAvailabilityBitfield, -} - /// Deal with network bridge updates and track what needs to be tracked async fn handle_network_msg( ctx: &mut Context, @@ -297,32 +323,66 @@ where tracker.peer_views.remove(&peerid); } NetworkBridgeEvent::PeerViewChange(peerid, view) => { - tracker - .peer_views - .entry(peerid) - .and_modify(|val| *val = view); + catch_up_messages(ctx, tracker, peerid, view).await?; } NetworkBridgeEvent::OurViewChange(view) => { let old_view = std::mem::replace(&mut (tracker.view), view); for new in tracker.view.difference(&old_view) { if !tracker.per_relay_parent.contains_key(&new) { - log::warn!("Our view contains {} but the overseer never told use we should work on this", &new); + log::warn!(target: "bitd", "Our view contains {} but the overseer never told use we should work on this", &new); } } } NetworkBridgeEvent::PeerMessage(remote, bytes) => { - log::info!("Got a peer message from {:?}", &remote); if let Ok(gossiped_bitfield) = BitfieldGossipMessage::decode(&mut (bytes.as_slice())) { + log::trace!(target: "bitd", "Received bitfield gossip from peer {:?}", &remote); process_incoming_peer_message(ctx, tracker, remote, gossiped_bitfield).await?; } else { - return modify_reputiation(ctx, remote, COST_MESSAGE_NOT_DECODABLE).await; + return modify_reputation(ctx, remote, COST_MESSAGE_NOT_DECODABLE).await; } } } Ok(()) } +// Send the difference between two views which were not sent +// to that particular peer. +async fn catch_up_messages( + ctx: &mut Context, + tracker: &mut Tracker, + origin: PeerId, + view: View, +) -> SubsystemResult<()> +where + Context: SubsystemContext + Clone, +{ + use std::collections::hash_map::Entry; + + match tracker + .peer_views + .entry(origin) { + Entry::Occupied(ref mut occupied) => { + let current = occupied.get_mut(); + + // send all messages we've seen before for this peer + for new_relay_parent_interest in (*current).difference(&view) { + if let Some(data) = tracker.per_relay_parent.get(new_relay_parent_interest) { + // if !data.peer_ { + // } @todo + todo!("XXX"); + } + } + + *current = view; + }, + Entry::Vacant(vacant) => { let _ = vacant.insert(view.clone()); } , + } + + Ok(()) +} + + impl Subsystem for BitfieldDistribution where C: SubsystemContext + Clone + Sync + Send, diff --git a/node/subsystem/src/messages.rs b/node/subsystem/src/messages.rs index d3c630cb56f0..686139732de8 100644 --- a/node/subsystem/src/messages.rs +++ b/node/subsystem/src/messages.rs @@ -34,7 +34,6 @@ use polkadot_primitives::v1::{ use polkadot_node_primitives::{ MisbehaviorReport, SignedFullStatement, View, ProtocolId, ValidationResult, }; - use std::sync::Arc; pub use sc_network::{ObservedRole, ReputationChange, PeerId}; @@ -398,6 +397,8 @@ pub enum AllMessages { AvailabilityDistribution(AvailabilityDistributionMessage), /// Message for the bitfield distribution subsystem. BitfieldDistribution(BitfieldDistributionMessage), + /// Message for the block authorship provisioning subsystem. + BlockAuthorshipProvisioning(BlockAuthorshipProvisioningMessage), /// Message for the Provisioner subsystem. Provisioner(ProvisionerMessage), /// Message for the PoV Distribution subsystem. diff --git a/roadmap/implementers-guide/src/node/availability/bitfield-distribution.md b/roadmap/implementers-guide/src/node/availability/bitfield-distribution.md index 7864fb3e25bb..a21807de402d 100644 --- a/roadmap/implementers-guide/src/node/availability/bitfield-distribution.md +++ b/roadmap/implementers-guide/src/node/availability/bitfield-distribution.md @@ -12,18 +12,24 @@ Input: Output: - `NetworkBridge::RegisterEventProducer(ProtocolId)` in order to register ourselfs as an event provide for the protocol. -- `NetworkBridge::SendMessage([PeerId], ProtocolId, Bytes)` -- `NetworkBridge::ReportPeer(PeerId, cost_or_benefit)` improve or penalize the reputiation of peer based on the message we received relative to our state. -- `BitfieldDistributionMessage::DistributeBitfield(relay_parent, SignedAvailabilityBitfield)` gossip a verified incoming bitfield on to interested subsystems within this validator node. +- `NetworkBridge::SendMessage([PeerId], ProtocolId, Bytes)` gossip a verified incoming bitfield on to interested subsystems within this validator node. +- `NetworkBridge::ReportPeer(PeerId, cost_or_benefit)` improve or penalize the reputation of peers based on the messages that are received relative to the current view. +- `BlockAuthorshipProvisioning::Bitfield(relay_parent, SignedAvailabilityBitfield)` pass + on the bitfield to the other submodules via the overseer. ## Functionality This is implemented as a gossip system. Register a [network bridge](../utility/network-bridge.md) event producer on startup. It is necessary to track peer connection, view change, and disconnection events, in order to maintain an index of which peers are interested in which relay parent bitfields. -Before gossiping incoming bitfields on, they must be checked to be signed by one of the validators + + +Before gossiping incoming bitfields, they must be checked to be signed by one of the validators of the validator set relevant to the current relay parent. Only accept bitfields relevant to our current view and only distribute bitfields to other peers when relevant to their most recent view. Accept and distribute only one bitfield per validator. + When receiving a bitfield either from the network or from a `DistributeBitfield` message, forward it along to the block authorship (provisioning) subsystem for potential inclusion in a block. + +Peers connecting after a set of valid bitfield gossip messages was received, those messages must be cached and sent upon connection of new peers or re-connecting peers. From 92431f304cb777dd4e783afc606ca84bd859c02d Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Mon, 20 Jul 2020 15:14:20 +0200 Subject: [PATCH 20/45] fix details --- node/network/bitfield-distribution/src/lib.rs | 58 ++++++++++++------- node/subsystem/src/messages.rs | 2 - .../availability/bitfield-distribution.md | 2 +- 3 files changed, 38 insertions(+), 24 deletions(-) diff --git a/node/network/bitfield-distribution/src/lib.rs b/node/network/bitfield-distribution/src/lib.rs index 8898894e7c81..8c000a67dac7 100644 --- a/node/network/bitfield-distribution/src/lib.rs +++ b/node/network/bitfield-distribution/src/lib.rs @@ -93,7 +93,8 @@ struct PerRelayParentData { } impl PerRelayParentData { - fn peer_already_was_sent_message_signed_by_validator(&self, peer: &PeerId, validator: &ValidatorId) -> bool { + /// Determines if that particular message signed by a validator is needed by the given peer. + fn message_from_validator_needed_by_peer(&self, peer: &PeerId, validator: &ValidatorId) -> bool { if let Some(set) = self.message_sent_to_peer.get(peer) { !set.contains(validator) } else { @@ -290,10 +291,9 @@ where message_sent_to_peer .entry(origin) .or_insert_with(|| { - HashSet::with_capacity(16) + HashSet::default() }) .insert(validator.clone()); - } else { return modify_reputation(ctx, origin, COST_SIGNATURE_INVALID).await; } @@ -358,26 +358,42 @@ where Context: SubsystemContext + Clone, { use std::collections::hash_map::Entry; - - match tracker + let current = tracker .peer_views - .entry(origin) { - Entry::Occupied(ref mut occupied) => { - let current = occupied.get_mut(); - - // send all messages we've seen before for this peer - for new_relay_parent_interest in (*current).difference(&view) { - if let Some(data) = tracker.per_relay_parent.get(new_relay_parent_interest) { - // if !data.peer_ { - // } @todo - todo!("XXX"); - } - } + .entry(origin.clone()) + .or_default(); - *current = view; - }, - Entry::Vacant(vacant) => { let _ = vacant.insert(view.clone()); } , - } + + let delta_vec: Vec = (*current).difference(&view).cloned().collect(); + + *current = view; + + // Send all messages we've seen before and the peer is now interested + // in to that peer. + + let delta_set: HashMap = delta_vec.into_iter() + .filter_map(|new_relay_parent_interest| { + if let Some(per_job) = (&*tracker).per_relay_parent.get(&new_relay_parent_interest) { + // send all messages + let one_per_validator = per_job.one_per_validator.clone(); + let origin = origin.clone(); + Some(one_per_validator.into_iter().filter(move |(validator, _message)| { + // except for the ones the peer already has + // let validator = validator.clone(); + per_job.message_from_validator_needed_by_peer(&origin, validator) + })) + } else { + // A relay parent is in the peers view, which is not in ours, ignore those. + None + } + }) + .flatten() + .collect(); + + for (validator, message) in delta_set.into_iter() { + // @todo track the send messages + relay_message(ctx, tracker, message).await?; + } Ok(()) } diff --git a/node/subsystem/src/messages.rs b/node/subsystem/src/messages.rs index 686139732de8..e4d9987fb280 100644 --- a/node/subsystem/src/messages.rs +++ b/node/subsystem/src/messages.rs @@ -397,8 +397,6 @@ pub enum AllMessages { AvailabilityDistribution(AvailabilityDistributionMessage), /// Message for the bitfield distribution subsystem. BitfieldDistribution(BitfieldDistributionMessage), - /// Message for the block authorship provisioning subsystem. - BlockAuthorshipProvisioning(BlockAuthorshipProvisioningMessage), /// Message for the Provisioner subsystem. Provisioner(ProvisionerMessage), /// Message for the PoV Distribution subsystem. diff --git a/roadmap/implementers-guide/src/node/availability/bitfield-distribution.md b/roadmap/implementers-guide/src/node/availability/bitfield-distribution.md index a21807de402d..5b6a6f9b4dda 100644 --- a/roadmap/implementers-guide/src/node/availability/bitfield-distribution.md +++ b/roadmap/implementers-guide/src/node/availability/bitfield-distribution.md @@ -14,7 +14,7 @@ Output: - `NetworkBridge::RegisterEventProducer(ProtocolId)` in order to register ourselfs as an event provide for the protocol. - `NetworkBridge::SendMessage([PeerId], ProtocolId, Bytes)` gossip a verified incoming bitfield on to interested subsystems within this validator node. - `NetworkBridge::ReportPeer(PeerId, cost_or_benefit)` improve or penalize the reputation of peers based on the messages that are received relative to the current view. -- `BlockAuthorshipProvisioning::Bitfield(relay_parent, SignedAvailabilityBitfield)` pass +- `ProvisionerMessage::ProvisionableData(ProvisionableData::Bitfield(relay_parent, SignedAvailabilityBitfield))` pass on the bitfield to the other submodules via the overseer. ## Functionality From 23c050e2f0daa0539656c6e9092898de36d2d7c5 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Mon, 20 Jul 2020 15:36:04 +0200 Subject: [PATCH 21/45] make sure outgoing messages are tracked --- node/network/bitfield-distribution/src/lib.rs | 44 ++++++++++++++++--- 1 file changed, 38 insertions(+), 6 deletions(-) diff --git a/node/network/bitfield-distribution/src/lib.rs b/node/network/bitfield-distribution/src/lib.rs index 8c000a67dac7..0a802b46e93f 100644 --- a/node/network/bitfield-distribution/src/lib.rs +++ b/node/network/bitfield-distribution/src/lib.rs @@ -215,10 +215,6 @@ where let interested_peers = tracker .peer_views .iter() - .filter(|(peerid, view)| { - // @todo - true - }) .filter_map(|(peerid, view)| { if view.contains(&message.relay_parent) { Some(peerid.clone()) @@ -391,14 +387,50 @@ where .collect(); for (validator, message) in delta_set.into_iter() { - // @todo track the send messages - relay_message(ctx, tracker, message).await?; + send_tracked_gossip_message(ctx, tracker, origin.clone(), validator, message).await?; } Ok(()) } +/// Send messages which were exchanged in the past +async fn send_tracked_gossip_message( + ctx: &mut Context, + tracker: &mut Tracker, + dest: PeerId, + validator: ValidatorId, + message: BitfieldGossipMessage, +) -> SubsystemResult<()> +where + Context: SubsystemContext + Clone, +{ + let per_job = if let Some(per_job) = tracker.per_relay_parent.get_mut(&message.relay_parent) { + per_job + } else { + // TODO punishing here seems unreasonable + return Ok(()); + }; + + let message_sent_to_peer = &mut (per_job.message_sent_to_peer); + message_sent_to_peer + .entry(dest.clone()) + .or_default() + .insert(validator.clone()); + + + let bytes = Encode::encode(&message); + ctx.send_message(AllMessages::NetworkBridge( + NetworkBridgeMessage::SendMessage( + vec![dest], + BitfieldDistribution::PROTOCOL_ID, + bytes, + ), + )) + .await?; + Ok(()) +} + impl Subsystem for BitfieldDistribution where C: SubsystemContext + Clone + Sync + Send, From a4316010f0159c3ce3c9d08066f90fa702d11151 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Mon, 20 Jul 2020 15:38:10 +0200 Subject: [PATCH 22/45] fix name --- node/network/bitfield-distribution/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/node/network/bitfield-distribution/src/lib.rs b/node/network/bitfield-distribution/src/lib.rs index 0a802b46e93f..fcea51083372 100644 --- a/node/network/bitfield-distribution/src/lib.rs +++ b/node/network/bitfield-distribution/src/lib.rs @@ -394,7 +394,7 @@ where } -/// Send messages which were exchanged in the past +/// Send a gossip message and track it in the per relay parent data. async fn send_tracked_gossip_message( ctx: &mut Context, tracker: &mut Tracker, From be2533ed94da09270309b8d506ad6d4c6490eb4f Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Mon, 20 Jul 2020 15:54:21 +0200 Subject: [PATCH 23/45] fix subsystem --- node/network/bitfield-distribution/src/lib.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/node/network/bitfield-distribution/src/lib.rs b/node/network/bitfield-distribution/src/lib.rs index fcea51083372..267c14a32222 100644 --- a/node/network/bitfield-distribution/src/lib.rs +++ b/node/network/bitfield-distribution/src/lib.rs @@ -436,7 +436,10 @@ where C: SubsystemContext + Clone + Sync + Send, { fn start(self, ctx: C) -> SpawnedSubsystem { - SpawnedSubsystem(Box::pin(async move { Self::run(ctx) }.map(|_| ()))) + SpawnedSubsystem { + name: "bitfield-distribution", + future: Box::pin(async move { Self::run(ctx) }.map(|_| ())), + } } } From 64b6ec17ffeead3fcc9a97e96f86c6d4c41785e3 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Mon, 20 Jul 2020 17:09:05 +0200 Subject: [PATCH 24/45] partial test impl --- Cargo.lock | 4 ++ node/network/bitfield-distribution/Cargo.toml | 6 ++ node/network/bitfield-distribution/src/lib.rs | 59 +++++++++++++++++-- 3 files changed, 63 insertions(+), 6 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 68018dee0d80..08a14876715f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4318,16 +4318,20 @@ dependencies = [ name = "polkadot-availability-bitfield-distribution" version = "0.1.0" dependencies = [ + "bitvec", "futures 0.3.5", "futures-timer 3.0.2", "log 0.4.8", "parity-scale-codec", + "parking_lot 0.10.2", "polkadot-network", "polkadot-network-bridge", "polkadot-node-primitives", "polkadot-node-subsystem", "polkadot-primitives", + "polkadot-subsystem-test-helpers", "sc-network", + "sp-core", "streamunordered", ] diff --git a/node/network/bitfield-distribution/Cargo.toml b/node/network/bitfield-distribution/Cargo.toml index 751bb3f291e2..293126c52862 100644 --- a/node/network/bitfield-distribution/Cargo.toml +++ b/node/network/bitfield-distribution/Cargo.toml @@ -16,3 +16,9 @@ polkadot-node-subsystem = { path = "../../subsystem" } polkadot-network-bridge = { path = "../../network/bridge" } polkadot-network = { path = "../../../network" } sc-network = { git = "https://github.com/paritytech/substrate", branch = "master" } + +[dev-dependencies] +bitvec = { version = "0.17.4", default-features = false, features = ["alloc"] } +sp-core = { git = "https://github.com/paritytech/substrate", branch = "master" } +subsystem-test = { package = "polkadot-subsystem-test-helpers", path = "../../test-helpers/subsystem" } +parking_lot = "0.10.0" diff --git a/node/network/bitfield-distribution/src/lib.rs b/node/network/bitfield-distribution/src/lib.rs index 267c14a32222..7ad1b4fb665b 100644 --- a/node/network/bitfield-distribution/src/lib.rs +++ b/node/network/bitfield-distribution/src/lib.rs @@ -1,12 +1,12 @@ // Copyright 2020 Parity Technologies (UK) Ltd. // This file is part of Polkadot. -// Polkadot is free software: you can rerelay_message it and/or modify +// Polkadot 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. -// Polkadot is relay_messaged in the hope that it will be useful, +// Polkadot 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. @@ -143,8 +143,6 @@ impl BitfieldDistribution { relay_parent: hash, signed_availability, }; - // @todo are we subject to sending something multiple times? - // @todo this also apply to ourself? relay_message(&mut ctx, &mut tracker, msg).await?; } BitfieldDistributionMessage::NetworkBridgeUpdate(event) => { @@ -473,6 +471,11 @@ where #[cfg(test)] mod test { use super::*; + use bitvec::{bitvec, vec::BitVec}; + use polkadot_primitives::v1::AvailabilityBitfield; + use polkadot_primitives::v0::{Signed, ValidatorPair}; + use sp_core::crypto::Pair; + use futures::executor; fn generate_valid_message() -> AllMessages { // AllMessages::BitfieldDistribution(BitfieldDistributionMessage::DistributeBitfield()) @@ -483,8 +486,52 @@ mod test { unimplemented!() } + macro_rules! msg_sequence { + ($( $input:expr ),+ $(,)? ) => [ + vec![ $( AllMessages::BitfieldDistribution($input) ),+ ] + ]; + } + + macro_rules! view { + ( $( $hash:expr ),+ $(,)? ) => [ + View(vec![ $( $hash.clone() ),+ ]) + ]; + } + #[test] - fn game_changer() { - // @todo + fn relay_must_work() { + let hash_a: Hash = [0; 32].into(); // us + let hash_b: Hash = [1; 32].into(); // other + + let peer_a = PeerId::random(); + let peer_b = PeerId::random(); + + + let context = SigningContext { + session_index: 1, + parent_hash: hash_a.clone(), + }; + + // validator 0 key pair + let (validator_pair, _seed) = ValidatorPair::generate(); + let validator = validator_pair.public(); + + let payload = AvailabilityBitfield(bitvec![bitvec::order::Lsb0, u8; 1u8; 32]); + let signed = Signed::::sign( + payload, &context, 0, &validator_pair); + + let input = msg_sequence![ + BitfieldDistributionMessage::NetworkBridgeUpdate(NetworkBridgeEvent::OurViewChange(view![hash_a, hash_b])), + BitfieldDistributionMessage::NetworkBridgeUpdate(NetworkBridgeEvent::PeerConnected(peer_b.clone(), ObservedRole::Full)), + BitfieldDistributionMessage::NetworkBridgeUpdate(NetworkBridgeEvent::PeerViewChange(peer_b.clone(), view![hash_a, hash_b])), + BitfieldDistributionMessage::DistributeBitfield(hash_b, signed), + ]; + + let pool = sp_core::testing::SpawnBlockingExecutor::new(); + let (mut ctx, mut handle) = subsystem_test::make_subsystem_context::(pool); + + executor::block_on(async move { + + }); } } From 52d9800319630bafac4d531eb3d0cca0ec99a0e5 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Tue, 21 Jul 2020 12:32:57 +0200 Subject: [PATCH 25/45] relax context bounds --- node/network/bitfield-distribution/src/lib.rs | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/node/network/bitfield-distribution/src/lib.rs b/node/network/bitfield-distribution/src/lib.rs index 7ad1b4fb665b..29664c97efca 100644 --- a/node/network/bitfield-distribution/src/lib.rs +++ b/node/network/bitfield-distribution/src/lib.rs @@ -117,7 +117,7 @@ impl BitfieldDistribution { /// Start processing work as passed on from the Overseer. async fn run(mut ctx: Context) -> SubsystemResult<()> where - Context: SubsystemContext + Clone, + Context: SubsystemContext, { // startup: register the network protocol with the bridge. ctx.send_message(AllMessages::NetworkBridge( @@ -190,7 +190,7 @@ async fn modify_reputation( rep: ReputationChange, ) -> SubsystemResult<()> where - Context: SubsystemContext + Clone, + Context: SubsystemContext, { ctx.send_message(AllMessages::NetworkBridge( NetworkBridgeMessage::ReportPeer(peerid, rep), @@ -207,7 +207,7 @@ async fn relay_message( message: BitfieldGossipMessage, ) -> SubsystemResult<()> where - Context: SubsystemContext + Clone, + Context: SubsystemContext, { // concurrently pass on the bitfield distribution to all interested peers let interested_peers = tracker @@ -243,7 +243,7 @@ async fn process_incoming_peer_message( message: BitfieldGossipMessage, ) -> SubsystemResult<()> where - Context: SubsystemContext + Clone, + Context: SubsystemContext, { // we don't care about this, not part of our view if !tracker.view.contains(&message.relay_parent) { @@ -305,7 +305,7 @@ async fn handle_network_msg( bridge_message: NetworkBridgeEvent, ) -> SubsystemResult<()> where - Context: SubsystemContext + Clone, + Context: SubsystemContext, { match bridge_message { NetworkBridgeEvent::PeerConnected(peerid, _role) => { @@ -349,7 +349,7 @@ async fn catch_up_messages( view: View, ) -> SubsystemResult<()> where - Context: SubsystemContext + Clone, + Context: SubsystemContext, { use std::collections::hash_map::Entry; let current = tracker @@ -401,7 +401,7 @@ async fn send_tracked_gossip_message( message: BitfieldGossipMessage, ) -> SubsystemResult<()> where - Context: SubsystemContext + Clone, + Context: SubsystemContext, { let per_job = if let Some(per_job) = tracker.per_relay_parent.get_mut(&message.relay_parent) { per_job @@ -431,7 +431,7 @@ where impl Subsystem for BitfieldDistribution where - C: SubsystemContext + Clone + Sync + Send, + C: SubsystemContext + Sync + Send, { fn start(self, ctx: C) -> SpawnedSubsystem { SpawnedSubsystem { @@ -447,7 +447,7 @@ async fn query_basics( relay_parent: Hash, ) -> SubsystemResult<(Vec, SigningContext)> where - Context: SubsystemContext + Clone, + Context: SubsystemContext, { let (validators_tx, validators_rx) = oneshot::channel(); let (signing_tx, signing_rx) = oneshot::channel(); @@ -488,7 +488,7 @@ mod test { macro_rules! msg_sequence { ($( $input:expr ),+ $(,)? ) => [ - vec![ $( AllMessages::BitfieldDistribution($input) ),+ ] + vec![ $( FromOverseer::Communication { msg: $input } ),+ ] ]; } From df8fe1d7e597825a5763f9c071016ff48a4e2aac Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Tue, 21 Jul 2020 12:33:11 +0200 Subject: [PATCH 26/45] test --- node/network/bitfield-distribution/src/lib.rs | 27 +++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/node/network/bitfield-distribution/src/lib.rs b/node/network/bitfield-distribution/src/lib.rs index 29664c97efca..4b0425476c94 100644 --- a/node/network/bitfield-distribution/src/lib.rs +++ b/node/network/bitfield-distribution/src/lib.rs @@ -524,14 +524,37 @@ mod test { BitfieldDistributionMessage::NetworkBridgeUpdate(NetworkBridgeEvent::OurViewChange(view![hash_a, hash_b])), BitfieldDistributionMessage::NetworkBridgeUpdate(NetworkBridgeEvent::PeerConnected(peer_b.clone(), ObservedRole::Full)), BitfieldDistributionMessage::NetworkBridgeUpdate(NetworkBridgeEvent::PeerViewChange(peer_b.clone(), view![hash_a, hash_b])), - BitfieldDistributionMessage::DistributeBitfield(hash_b, signed), + BitfieldDistributionMessage::DistributeBitfield(hash_b.clone(), signed.clone()), ]; + let mut tracker = Tracker::default(); + let pool = sp_core::testing::SpawnBlockingExecutor::new(); - let (mut ctx, mut handle) = subsystem_test::make_subsystem_context::(pool); + let (mut ctx, mut handle) = subsystem_test::make_subsystem_context::(pool); + // executor::block_on(async move { + println!("11111111111"); + for input in input { + println!("msg"); + handle.send(input.into()); + } + let _ = BitfieldDistribution::start(BitfieldDistribution, ctx).future.await; + + let msg = BitfieldGossipMessage { + relay_parent: hash_a.clone(), + signed_availability: signed.clone(), + }; + println!("tx"); + let x = dbg!(send_tracked_gossip_message(&mut ctx, &mut tracker, peer_b, validator, msg).await); + + println!("wait for rx"); + + while let Ok(rxd) = ctx.recv().await { + dbg!(rxd); + } }); + } } From 38661f7e6a53988635223d56f28e0f780675da72 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Tue, 21 Jul 2020 14:11:40 +0200 Subject: [PATCH 27/45] X --- Cargo.lock | 19 ++++ node/network/bitfield-distribution/Cargo.toml | 3 + node/network/bitfield-distribution/src/lib.rs | 98 +++++++++++++++---- node/test-helpers/subsystem/src/lib.rs | 1 + 4 files changed, 102 insertions(+), 19 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 08a14876715f..dba082b8bcc3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3021,6 +3021,12 @@ dependencies = [ "libc", ] +[[package]] +name = "maplit" +version = "1.0.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3e2e65a1a2e43cfcb47a895c4c8b10d1f4a61097f9f254f183aee60cad9c651d" + [[package]] name = "matches" version = "0.1.8" @@ -4322,6 +4328,7 @@ dependencies = [ "futures 0.3.5", "futures-timer 3.0.2", "log 0.4.8", + "maplit", "parity-scale-codec", "parking_lot 0.10.2", "polkadot-network", @@ -4331,6 +4338,8 @@ dependencies = [ "polkadot-primitives", "polkadot-subsystem-test-helpers", "sc-network", + "smol", + "smol-timeout", "sp-core", "streamunordered", ] @@ -7213,6 +7222,16 @@ dependencies = [ "winapi 0.3.9", ] +[[package]] +name = "smol-timeout" +version = "0.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "024818c1f00b80e8171ddcfcee33860134293aa3aced60c9cbd7a5a2d41db392" +dependencies = [ + "pin-project", + "smol", +] + [[package]] name = "snow" version = "0.7.1" diff --git a/node/network/bitfield-distribution/Cargo.toml b/node/network/bitfield-distribution/Cargo.toml index 293126c52862..7497fae8d1f2 100644 --- a/node/network/bitfield-distribution/Cargo.toml +++ b/node/network/bitfield-distribution/Cargo.toml @@ -22,3 +22,6 @@ bitvec = { version = "0.17.4", default-features = false, features = ["alloc"] } sp-core = { git = "https://github.com/paritytech/substrate", branch = "master" } subsystem-test = { package = "polkadot-subsystem-test-helpers", path = "../../test-helpers/subsystem" } parking_lot = "0.10.0" +maplit = "1.0.2" +smol-timeout = "0.1" +smol = "0.1" \ No newline at end of file diff --git a/node/network/bitfield-distribution/src/lib.rs b/node/network/bitfield-distribution/src/lib.rs index 4b0425476c94..34b933cbd5bc 100644 --- a/node/network/bitfield-distribution/src/lib.rs +++ b/node/network/bitfield-distribution/src/lib.rs @@ -499,7 +499,7 @@ mod test { } #[test] - fn relay_must_work() { + fn boundary_to_boundary() { let hash_a: Hash = [0; 32].into(); // us let hash_b: Hash = [1; 32].into(); // other @@ -507,7 +507,7 @@ mod test { let peer_b = PeerId::random(); - let context = SigningContext { + let signing_context = SigningContext { session_index: 1, parent_hash: hash_a.clone(), }; @@ -518,7 +518,7 @@ mod test { let payload = AvailabilityBitfield(bitvec![bitvec::order::Lsb0, u8; 1u8; 32]); let signed = Signed::::sign( - payload, &context, 0, &validator_pair); + payload, &signing_context, 0, &validator_pair); let input = msg_sequence![ BitfieldDistributionMessage::NetworkBridgeUpdate(NetworkBridgeEvent::OurViewChange(view![hash_a, hash_b])), @@ -527,34 +527,94 @@ mod test { BitfieldDistributionMessage::DistributeBitfield(hash_b.clone(), signed.clone()), ]; + // empty initial state let mut tracker = Tracker::default(); let pool = sp_core::testing::SpawnBlockingExecutor::new(); let (mut ctx, mut handle) = subsystem_test::make_subsystem_context::(pool); - // - executor::block_on(async move { - - println!("11111111111"); - for input in input { - println!("msg"); + executor::block_on(async move { + for input in input.into_iter() { handle.send(input.into()); } - let _ = BitfieldDistribution::start(BitfieldDistribution, ctx).future.await; - - let msg = BitfieldGossipMessage { - relay_parent: hash_a.clone(), - signed_availability: signed.clone(), - }; - println!("tx"); - let x = dbg!(send_tracked_gossip_message(&mut ctx, &mut tracker, peer_b, validator, msg).await); - - println!("wait for rx"); + let completion = ctx.spawn("test-system", BitfieldDistribution::start(BitfieldDistribution, ctx.clone()).future); while let Ok(rxd) = ctx.recv().await { + // @todo impl expectation checks against a hashmap dbg!(rxd); } }); } + + use maplit::hashmap; + use maplit::hashset; + + fn prewarmed_tracker(validator: ValidatorId, signing_context: SigningContext, message: BitfieldGossipMessage, peers: Vec) -> Tracker { + let mut tracker = Tracker::default(); + tracker.per_relay_parent.insert(message.relay_parent.clone(), + PerRelayParentData { + signing_context, + validator_set: vec![validator.clone()], + one_per_validator: hashmap!{ + validator.clone() => message.clone(), + }, + message_sent_to_peer: hashmap!{}, + }); + tracker + } + + + use std::time::Duration; + use smol::Timer; + use smol_timeout::TimeoutExt; + + #[test] + fn relay_must_work() { + let hash_a: Hash = [0; 32].into(); // us + let hash_b: Hash = [1; 32].into(); // other + + let peer_a = PeerId::random(); + let peer_b = PeerId::random(); + assert_ne!(peer_a, peer_b); + + + let signing_context = SigningContext { + session_index: 1, + parent_hash: hash_a.clone(), + }; + + // validator 0 key pair + let (validator_pair, _seed) = ValidatorPair::generate(); + let validator = validator_pair.public(); + + let payload = AvailabilityBitfield(bitvec![bitvec::order::Lsb0, u8; 1u8; 32]); + let signed = Signed::::sign( + payload, &signing_context, 0, &validator_pair); + + let msg = BitfieldGossipMessage { + relay_parent: hash_a.clone(), + signed_availability: signed.clone(), + }; + + + let pool = sp_core::testing::SpawnBlockingExecutor::new(); + let (mut ctx, mut handle) = subsystem_test::make_subsystem_context::(pool); + + let mut tracker = prewarmed_tracker(validator.clone(), signing_context.clone(), msg.clone(), vec![peer_b.clone()]); + + executor::block_on(async move { + let x = async move { send_tracked_gossip_message(&mut ctx, &mut tracker, peer_b, validator, msg).await }; + + let res = dbg!(x.timeout(Duration::from_millis(500)).await); + + println!("complete?"); + // while let Ok(rxd) = async move { + // ctx.recv() + // }.await { + // dbg!(rxd); + // } + }); + + } } diff --git a/node/test-helpers/subsystem/src/lib.rs b/node/test-helpers/subsystem/src/lib.rs index 5fa7f0b9eca1..c0821584e913 100644 --- a/node/test-helpers/subsystem/src/lib.rs +++ b/node/test-helpers/subsystem/src/lib.rs @@ -45,6 +45,7 @@ enum SinkState { pub struct SingleItemSink(Arc>>); /// The stream half of a single-item sink. +#[derive(Clone)] pub struct SingleItemStream(Arc>>); impl Sink for SingleItemSink { From f1f7cab788250e35443bb53aad262565b00b1a3e Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Tue, 21 Jul 2020 14:13:55 +0200 Subject: [PATCH 28/45] X --- node/test-helpers/subsystem/src/lib.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/node/test-helpers/subsystem/src/lib.rs b/node/test-helpers/subsystem/src/lib.rs index c0821584e913..5fa7f0b9eca1 100644 --- a/node/test-helpers/subsystem/src/lib.rs +++ b/node/test-helpers/subsystem/src/lib.rs @@ -45,7 +45,6 @@ enum SinkState { pub struct SingleItemSink(Arc>>); /// The stream half of a single-item sink. -#[derive(Clone)] pub struct SingleItemStream(Arc>>); impl Sink for SingleItemSink { From f94b8c5cc1abf2699eaf01e2ef71fa9dadb7edfb Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Tue, 21 Jul 2020 14:34:51 +0200 Subject: [PATCH 29/45] initial test --- node/network/bitfield-distribution/src/lib.rs | 948 +++++++++--------- 1 file changed, 480 insertions(+), 468 deletions(-) diff --git a/node/network/bitfield-distribution/src/lib.rs b/node/network/bitfield-distribution/src/lib.rs index 34b933cbd5bc..28d9aeaa899f 100644 --- a/node/network/bitfield-distribution/src/lib.rs +++ b/node/network/bitfield-distribution/src/lib.rs @@ -21,31 +21,27 @@ //! Independently of that, gossips on received messages from peers to other interested peers. use codec::{Decode, Encode}; -use futures::{ - channel::oneshot, - FutureExt, -}; +use futures::{channel::oneshot, FutureExt}; use node_primitives::{ProtocolId, View}; use polkadot_node_subsystem::messages::*; use polkadot_node_subsystem::{ - FromOverseer, OverseerSignal, SpawnedSubsystem, Subsystem, SubsystemContext, SubsystemResult, + FromOverseer, OverseerSignal, SpawnedSubsystem, Subsystem, SubsystemContext, SubsystemResult, }; -use polkadot_primitives::v1::{Hash, SignedAvailabilityBitfield}; use polkadot_primitives::v0::{SigningContext, ValidatorId}; +use polkadot_primitives::v1::{Hash, SignedAvailabilityBitfield}; use sc_network::ReputationChange; use std::collections::{HashMap, HashSet}; const COST_SIGNATURE_INVALID: ReputationChange = - ReputationChange::new(-100, "Bitfield signature invalid"); + ReputationChange::new(-100, "Bitfield signature invalid"); const COST_MISSING_PEER_SESSION_KEY: ReputationChange = - ReputationChange::new(-133, "Missing peer session key"); + ReputationChange::new(-133, "Missing peer session key"); const COST_NOT_INTERESTED: ReputationChange = - ReputationChange::new(-51, "Not intersted in that parent hash"); + ReputationChange::new(-51, "Not intersted in that parent hash"); const COST_MESSAGE_NOT_DECODABLE: ReputationChange = - ReputationChange::new(-100, "Not intersted in that parent hash"); - + ReputationChange::new(-100, "Not intersted in that parent hash"); /// Checked signed availability bitfield that is distributed /// to other peers. @@ -54,567 +50,583 @@ pub struct BitfieldGossipMessage { /// The relay parent this message is relative to. pub relay_parent: Hash, /// The actual signed availability bitfield. - pub signed_availability: SignedAvailabilityBitfield, + pub signed_availability: SignedAvailabilityBitfield, } - /// Data used to track information of peers and relay parents the /// overseer ordered us to work on. #[derive(Default, Clone)] struct Tracker { - /// track all active peers and their views - /// to determine what is relevant to them. - peer_views: HashMap, + /// track all active peers and their views + /// to determine what is relevant to them. + peer_views: HashMap, - /// Our current view. - view: View, + /// Our current view. + view: View, - /// Additional data particular to a relay parent. - per_relay_parent: HashMap, + /// Additional data particular to a relay parent. + per_relay_parent: HashMap, } /// Data for a particular relay parent. #[derive(Debug, Clone, Default)] struct PerRelayParentData { - /// Signing context for a particular relay parent. - signing_context: SigningContext, + /// Signing context for a particular relay parent. + signing_context: SigningContext, - /// Set of validators for a particular relay parent. - validator_set: Vec, + /// Set of validators for a particular relay parent. + validator_set: Vec, - /// Set of validators for a particular relay parent for which we - /// received a valid `BitfieldGossipMessage`. - /// Also serves as the list of known messages for peers connecting - /// after bitfield gossips were already received. - one_per_validator: HashMap, + /// Set of validators for a particular relay parent for which we + /// received a valid `BitfieldGossipMessage`. + /// Also serves as the list of known messages for peers connecting + /// after bitfield gossips were already received. + one_per_validator: HashMap, - /// which messages of which validators were already sent - message_sent_to_peer: HashMap>, + /// which messages of which validators were already sent + message_sent_to_peer: HashMap>, } impl PerRelayParentData { - /// Determines if that particular message signed by a validator is needed by the given peer. - fn message_from_validator_needed_by_peer(&self, peer: &PeerId, validator: &ValidatorId) -> bool { - if let Some(set) = self.message_sent_to_peer.get(peer) { - !set.contains(validator) - } else { - false - } - } + /// Determines if that particular message signed by a validator is needed by the given peer. + fn message_from_validator_needed_by_peer( + &self, + peer: &PeerId, + validator: &ValidatorId, + ) -> bool { + if let Some(set) = self.message_sent_to_peer.get(peer) { + !set.contains(validator) + } else { + false + } + } } fn network_update_message(n: NetworkBridgeEvent) -> AllMessages { - AllMessages::BitfieldDistribution(BitfieldDistributionMessage::NetworkBridgeUpdate(n)) + AllMessages::BitfieldDistribution(BitfieldDistributionMessage::NetworkBridgeUpdate(n)) } /// The bitfield distribution subsystem. pub struct BitfieldDistribution; impl BitfieldDistribution { - /// The protocol identifier for bitfield distribution. - const PROTOCOL_ID: ProtocolId = *b"bitd"; - - /// Start processing work as passed on from the Overseer. - async fn run(mut ctx: Context) -> SubsystemResult<()> - where - Context: SubsystemContext, - { - // startup: register the network protocol with the bridge. - ctx.send_message(AllMessages::NetworkBridge( - NetworkBridgeMessage::RegisterEventProducer(Self::PROTOCOL_ID, network_update_message), - )) - .await?; - - // work: process incoming messages from the overseer and process accordingly. - let mut tracker = Tracker::default(); - loop { - { - let message = ctx.recv().await?; - match message { - FromOverseer::Communication { msg } => { - // another subsystem created this signed availability bitfield messages - match msg { - // relay_message a bitfield via gossip to other validators - BitfieldDistributionMessage::DistributeBitfield( - hash, - signed_availability, - ) => { - let msg = BitfieldGossipMessage { - relay_parent: hash, - signed_availability, - }; - relay_message(&mut ctx, &mut tracker, msg).await?; - } - BitfieldDistributionMessage::NetworkBridgeUpdate(event) => { - // a network message was received - if let Err(e) = handle_network_msg(&mut ctx, &mut tracker, event).await { - log::warn!(target: "bitd", "Failed to handle incomming network messages: {:?}", e); - } - } - } - } - FromOverseer::Signal(OverseerSignal::StartWork(relay_parent)) => { - // query basic system parameters once - // @todo assumption: these cannot change within a session - let (validator_set, signing_context) = - query_basics(&mut ctx, relay_parent).await?; - - let _ = tracker.per_relay_parent.insert( - relay_parent, - PerRelayParentData { - signing_context, - validator_set, - ..Default::default() - }, - ); - } - FromOverseer::Signal(OverseerSignal::StopWork(relay_parent)) => { - // @todo assumption: it is good enough to prevent additional work from being - // scheduled, the individual futures are supposedly completed quickly - let _ = tracker.per_relay_parent.remove(&relay_parent); - } - FromOverseer::Signal(OverseerSignal::Conclude) => { - tracker.per_relay_parent.clear(); - return Ok(()); - } - } - } - } - } + /// The protocol identifier for bitfield distribution. + const PROTOCOL_ID: ProtocolId = *b"bitd"; + + /// Start processing work as passed on from the Overseer. + async fn run(mut ctx: Context) -> SubsystemResult<()> + where + Context: SubsystemContext, + { + // startup: register the network protocol with the bridge. + ctx.send_message(AllMessages::NetworkBridge( + NetworkBridgeMessage::RegisterEventProducer(Self::PROTOCOL_ID, network_update_message), + )) + .await?; + + // work: process incoming messages from the overseer and process accordingly. + let mut tracker = Tracker::default(); + loop { + { + let message = ctx.recv().await?; + match message { + FromOverseer::Communication { msg } => { + // another subsystem created this signed availability bitfield messages + match msg { + // relay_message a bitfield via gossip to other validators + BitfieldDistributionMessage::DistributeBitfield( + hash, + signed_availability, + ) => { + let msg = BitfieldGossipMessage { + relay_parent: hash, + signed_availability, + }; + relay_message(&mut ctx, &mut tracker, msg).await?; + } + BitfieldDistributionMessage::NetworkBridgeUpdate(event) => { + // a network message was received + if let Err(e) = + handle_network_msg(&mut ctx, &mut tracker, event).await + { + log::warn!(target: "bitd", "Failed to handle incomming network messages: {:?}", e); + } + } + } + } + FromOverseer::Signal(OverseerSignal::StartWork(relay_parent)) => { + // query basic system parameters once + // @todo assumption: these cannot change within a session + let (validator_set, signing_context) = + query_basics(&mut ctx, relay_parent).await?; + + let _ = tracker.per_relay_parent.insert( + relay_parent, + PerRelayParentData { + signing_context, + validator_set, + ..Default::default() + }, + ); + } + FromOverseer::Signal(OverseerSignal::StopWork(relay_parent)) => { + // @todo assumption: it is good enough to prevent additional work from being + // scheduled, the individual futures are supposedly completed quickly + let _ = tracker.per_relay_parent.remove(&relay_parent); + } + FromOverseer::Signal(OverseerSignal::Conclude) => { + tracker.per_relay_parent.clear(); + return Ok(()); + } + } + } + } + } } /// Modify the reputation of peer based on their behaviour. async fn modify_reputation( - ctx: &mut Context, - peerid: PeerId, - rep: ReputationChange, + ctx: &mut Context, + peerid: PeerId, + rep: ReputationChange, ) -> SubsystemResult<()> where - Context: SubsystemContext, + Context: SubsystemContext, { - ctx.send_message(AllMessages::NetworkBridge( - NetworkBridgeMessage::ReportPeer(peerid, rep), - )) - .await + ctx.send_message(AllMessages::NetworkBridge( + NetworkBridgeMessage::ReportPeer(peerid, rep), + )) + .await } /// Distribute a given valid bitfield message. /// /// Can be originated by another subsystem or received via network from another peer. async fn relay_message( - ctx: &mut Context, - tracker: &mut Tracker, - message: BitfieldGossipMessage, + ctx: &mut Context, + tracker: &mut Tracker, + message: BitfieldGossipMessage, ) -> SubsystemResult<()> where - Context: SubsystemContext, + Context: SubsystemContext, { - // concurrently pass on the bitfield distribution to all interested peers - let interested_peers = tracker - .peer_views - .iter() - .filter_map(|(peerid, view)| { - if view.contains(&message.relay_parent) { - Some(peerid.clone()) - } else { - None - } - }) - .collect::>(); - - let bytes = Encode::encode(&message); - ctx.send_message(AllMessages::NetworkBridge( - NetworkBridgeMessage::SendMessage( - interested_peers, - BitfieldDistribution::PROTOCOL_ID, - bytes, - ), - )) - .await?; - Ok(()) + // concurrently pass on the bitfield distribution to all interested peers + let interested_peers = tracker + .peer_views + .iter() + .filter_map(|(peerid, view)| { + if view.contains(&message.relay_parent) { + Some(peerid.clone()) + } else { + None + } + }) + .collect::>(); + + let bytes = Encode::encode(&message); + ctx.send_message(AllMessages::NetworkBridge( + NetworkBridgeMessage::SendMessage( + interested_peers, + BitfieldDistribution::PROTOCOL_ID, + bytes, + ), + )) + .await?; + Ok(()) } - /// Handle an incoming message from a peer. async fn process_incoming_peer_message( - ctx: &mut Context, - tracker: &mut Tracker, - origin: PeerId, - message: BitfieldGossipMessage, + ctx: &mut Context, + tracker: &mut Tracker, + origin: PeerId, + message: BitfieldGossipMessage, ) -> SubsystemResult<()> where - Context: SubsystemContext, + Context: SubsystemContext, { - // we don't care about this, not part of our view - if !tracker.view.contains(&message.relay_parent) { - return modify_reputation(ctx, origin, COST_NOT_INTERESTED).await; - } - - // Ignore anything the overseer did not tell this subsystem to work on - let mut job_data = tracker.per_relay_parent.get_mut(&message.relay_parent); - let job_data: &mut _ = if let Some(ref mut job_data) = job_data { - job_data - } else { - return modify_reputation(ctx, origin, COST_NOT_INTERESTED).await; - }; - - let validator_set = &job_data.validator_set; - if validator_set.len() == 0 { - return modify_reputation(ctx, origin, COST_MISSING_PEER_SESSION_KEY).await; - } - - // check all validators that could have signed this message - // @todo there must be a better way figuring this out cheaply - let signing_context = job_data.signing_context.clone(); - if let Some(validator) = validator_set.iter().find(|validator| { - message - .signed_availability - .check_signature(&signing_context, validator) - .is_ok() - }) { - let one_per_validator = &mut (job_data.one_per_validator); - // only relay_message a message of a validator once - if one_per_validator.get(validator).is_some() { - return Ok(()); - } - one_per_validator.insert(validator.clone(), message.clone()); - - - // track which messages that peer already received - let message_sent_to_peer = &mut (job_data.message_sent_to_peer); - message_sent_to_peer - .entry(origin) - .or_insert_with(|| { - HashSet::default() - }) - .insert(validator.clone()); - } else { - return modify_reputation(ctx, origin, COST_SIGNATURE_INVALID).await; - } - - // passed all conditions, distribute to peers! - relay_message(ctx, tracker, message).await?; - - Ok(()) + // we don't care about this, not part of our view + if !tracker.view.contains(&message.relay_parent) { + return modify_reputation(ctx, origin, COST_NOT_INTERESTED).await; + } + + // Ignore anything the overseer did not tell this subsystem to work on + let mut job_data = tracker.per_relay_parent.get_mut(&message.relay_parent); + let job_data: &mut _ = if let Some(ref mut job_data) = job_data { + job_data + } else { + return modify_reputation(ctx, origin, COST_NOT_INTERESTED).await; + }; + + let validator_set = &job_data.validator_set; + if validator_set.len() == 0 { + return modify_reputation(ctx, origin, COST_MISSING_PEER_SESSION_KEY).await; + } + + // check all validators that could have signed this message + // @todo there must be a better way figuring this out cheaply + let signing_context = job_data.signing_context.clone(); + if let Some(validator) = validator_set.iter().find(|validator| { + message + .signed_availability + .check_signature(&signing_context, validator) + .is_ok() + }) { + let one_per_validator = &mut (job_data.one_per_validator); + // only relay_message a message of a validator once + if one_per_validator.get(validator).is_some() { + return Ok(()); + } + one_per_validator.insert(validator.clone(), message.clone()); + + // track which messages that peer already received + let message_sent_to_peer = &mut (job_data.message_sent_to_peer); + message_sent_to_peer + .entry(origin) + .or_insert_with(|| HashSet::default()) + .insert(validator.clone()); + } else { + return modify_reputation(ctx, origin, COST_SIGNATURE_INVALID).await; + } + + // passed all conditions, distribute to peers! + relay_message(ctx, tracker, message).await?; + + Ok(()) } /// Deal with network bridge updates and track what needs to be tracked async fn handle_network_msg( - ctx: &mut Context, - tracker: &mut Tracker, - bridge_message: NetworkBridgeEvent, + ctx: &mut Context, + tracker: &mut Tracker, + bridge_message: NetworkBridgeEvent, ) -> SubsystemResult<()> where - Context: SubsystemContext, + Context: SubsystemContext, { - match bridge_message { - NetworkBridgeEvent::PeerConnected(peerid, _role) => { - // insert if none already present - tracker.peer_views.entry(peerid).or_insert(View::default()); - } - NetworkBridgeEvent::PeerDisconnected(peerid) => { - // get rid of superfluous data - tracker.peer_views.remove(&peerid); - } - NetworkBridgeEvent::PeerViewChange(peerid, view) => { - catch_up_messages(ctx, tracker, peerid, view).await?; - } - NetworkBridgeEvent::OurViewChange(view) => { - let old_view = std::mem::replace(&mut (tracker.view), view); - - for new in tracker.view.difference(&old_view) { - if !tracker.per_relay_parent.contains_key(&new) { - log::warn!(target: "bitd", "Our view contains {} but the overseer never told use we should work on this", &new); - } - } - } - NetworkBridgeEvent::PeerMessage(remote, bytes) => { - if let Ok(gossiped_bitfield) = BitfieldGossipMessage::decode(&mut (bytes.as_slice())) { - log::trace!(target: "bitd", "Received bitfield gossip from peer {:?}", &remote); - process_incoming_peer_message(ctx, tracker, remote, gossiped_bitfield).await?; - } else { - return modify_reputation(ctx, remote, COST_MESSAGE_NOT_DECODABLE).await; - } - } - } - Ok(()) + match bridge_message { + NetworkBridgeEvent::PeerConnected(peerid, _role) => { + // insert if none already present + tracker.peer_views.entry(peerid).or_insert(View::default()); + } + NetworkBridgeEvent::PeerDisconnected(peerid) => { + // get rid of superfluous data + tracker.peer_views.remove(&peerid); + } + NetworkBridgeEvent::PeerViewChange(peerid, view) => { + catch_up_messages(ctx, tracker, peerid, view).await?; + } + NetworkBridgeEvent::OurViewChange(view) => { + let old_view = std::mem::replace(&mut (tracker.view), view); + + for new in tracker.view.difference(&old_view) { + if !tracker.per_relay_parent.contains_key(&new) { + log::warn!(target: "bitd", "Our view contains {} but the overseer never told use we should work on this", &new); + } + } + } + NetworkBridgeEvent::PeerMessage(remote, bytes) => { + if let Ok(gossiped_bitfield) = BitfieldGossipMessage::decode(&mut (bytes.as_slice())) { + log::trace!(target: "bitd", "Received bitfield gossip from peer {:?}", &remote); + process_incoming_peer_message(ctx, tracker, remote, gossiped_bitfield).await?; + } else { + return modify_reputation(ctx, remote, COST_MESSAGE_NOT_DECODABLE).await; + } + } + } + Ok(()) } // Send the difference between two views which were not sent // to that particular peer. async fn catch_up_messages( - ctx: &mut Context, - tracker: &mut Tracker, - origin: PeerId, - view: View, + ctx: &mut Context, + tracker: &mut Tracker, + origin: PeerId, + view: View, ) -> SubsystemResult<()> where - Context: SubsystemContext, + Context: SubsystemContext, { - use std::collections::hash_map::Entry; - let current = tracker - .peer_views - .entry(origin.clone()) - .or_default(); - - - let delta_vec: Vec = (*current).difference(&view).cloned().collect(); - - *current = view; - - // Send all messages we've seen before and the peer is now interested - // in to that peer. - - let delta_set: HashMap = delta_vec.into_iter() - .filter_map(|new_relay_parent_interest| { - if let Some(per_job) = (&*tracker).per_relay_parent.get(&new_relay_parent_interest) { - // send all messages - let one_per_validator = per_job.one_per_validator.clone(); - let origin = origin.clone(); - Some(one_per_validator.into_iter().filter(move |(validator, _message)| { - // except for the ones the peer already has - // let validator = validator.clone(); - per_job.message_from_validator_needed_by_peer(&origin, validator) - })) - } else { - // A relay parent is in the peers view, which is not in ours, ignore those. - None - } - }) - .flatten() - .collect(); - - for (validator, message) in delta_set.into_iter() { - send_tracked_gossip_message(ctx, tracker, origin.clone(), validator, message).await?; - } - - Ok(()) + use std::collections::hash_map::Entry; + let current = tracker.peer_views.entry(origin.clone()).or_default(); + + let delta_vec: Vec = (*current).difference(&view).cloned().collect(); + + *current = view; + + // Send all messages we've seen before and the peer is now interested + // in to that peer. + + let delta_set: HashMap = delta_vec + .into_iter() + .filter_map(|new_relay_parent_interest| { + if let Some(per_job) = (&*tracker).per_relay_parent.get(&new_relay_parent_interest) { + // send all messages + let one_per_validator = per_job.one_per_validator.clone(); + let origin = origin.clone(); + Some( + one_per_validator + .into_iter() + .filter(move |(validator, _message)| { + // except for the ones the peer already has + // let validator = validator.clone(); + per_job.message_from_validator_needed_by_peer(&origin, validator) + }), + ) + } else { + // A relay parent is in the peers view, which is not in ours, ignore those. + None + } + }) + .flatten() + .collect(); + + for (validator, message) in delta_set.into_iter() { + send_tracked_gossip_message(ctx, tracker, origin.clone(), validator, message).await?; + } + + Ok(()) } - /// Send a gossip message and track it in the per relay parent data. async fn send_tracked_gossip_message( - ctx: &mut Context, - tracker: &mut Tracker, - dest: PeerId, - validator: ValidatorId, - message: BitfieldGossipMessage, + ctx: &mut Context, + tracker: &mut Tracker, + dest: PeerId, + validator: ValidatorId, + message: BitfieldGossipMessage, ) -> SubsystemResult<()> where - Context: SubsystemContext, + Context: SubsystemContext, { - let per_job = if let Some(per_job) = tracker.per_relay_parent.get_mut(&message.relay_parent) { - per_job - } else { - // TODO punishing here seems unreasonable - return Ok(()); - }; - - let message_sent_to_peer = &mut (per_job.message_sent_to_peer); - message_sent_to_peer - .entry(dest.clone()) - .or_default() - .insert(validator.clone()); - - - let bytes = Encode::encode(&message); - ctx.send_message(AllMessages::NetworkBridge( - NetworkBridgeMessage::SendMessage( - vec![dest], - BitfieldDistribution::PROTOCOL_ID, - bytes, - ), - )) - .await?; - Ok(()) + let per_job = if let Some(per_job) = tracker.per_relay_parent.get_mut(&message.relay_parent) { + per_job + } else { + // TODO punishing here seems unreasonable + return Ok(()); + }; + + let message_sent_to_peer = &mut (per_job.message_sent_to_peer); + message_sent_to_peer + .entry(dest.clone()) + .or_default() + .insert(validator.clone()); + + let bytes = Encode::encode(&message); + ctx.send_message(AllMessages::NetworkBridge( + NetworkBridgeMessage::SendMessage(vec![dest], BitfieldDistribution::PROTOCOL_ID, bytes), + )) + .await?; + Ok(()) } impl Subsystem for BitfieldDistribution where - C: SubsystemContext + Sync + Send, + C: SubsystemContext + Sync + Send, { - fn start(self, ctx: C) -> SpawnedSubsystem { - SpawnedSubsystem { - name: "bitfield-distribution", - future: Box::pin(async move { Self::run(ctx) }.map(|_| ())), - } - } + fn start(self, ctx: C) -> SpawnedSubsystem { + SpawnedSubsystem { + name: "bitfield-distribution", + future: Box::pin(async move { Self::run(ctx) }.map(|_| ())), + } + } } /// query the validator set and signing context async fn query_basics( - ctx: &mut Context, - relay_parent: Hash, + ctx: &mut Context, + relay_parent: Hash, ) -> SubsystemResult<(Vec, SigningContext)> where - Context: SubsystemContext, + Context: SubsystemContext, { - let (validators_tx, validators_rx) = oneshot::channel(); - let (signing_tx, signing_rx) = oneshot::channel(); + let (validators_tx, validators_rx) = oneshot::channel(); + let (signing_tx, signing_rx) = oneshot::channel(); - let query_validators = AllMessages::RuntimeApi(RuntimeApiMessage::Request( - relay_parent.clone(), - RuntimeApiRequest::Validators(validators_tx), - )); + let query_validators = AllMessages::RuntimeApi(RuntimeApiMessage::Request( + relay_parent.clone(), + RuntimeApiRequest::Validators(validators_tx), + )); - let query_signing = AllMessages::RuntimeApi(RuntimeApiMessage::Request( - relay_parent.clone(), - RuntimeApiRequest::SigningContext(signing_tx), - )); + let query_signing = AllMessages::RuntimeApi(RuntimeApiMessage::Request( + relay_parent.clone(), + RuntimeApiRequest::SigningContext(signing_tx), + )); - ctx.send_messages(std::iter::once(query_validators).chain(std::iter::once(query_signing))) - .await?; + ctx.send_messages(std::iter::once(query_validators).chain(std::iter::once(query_signing))) + .await?; - Ok((validators_rx.await?, signing_rx.await?)) + Ok((validators_rx.await?, signing_rx.await?)) } #[cfg(test)] mod test { - use super::*; - use bitvec::{bitvec, vec::BitVec}; - use polkadot_primitives::v1::AvailabilityBitfield; - use polkadot_primitives::v0::{Signed, ValidatorPair}; - use sp_core::crypto::Pair; + use super::*; + use bitvec::{bitvec, vec::BitVec}; use futures::executor; - - fn generate_valid_message() -> AllMessages { - // AllMessages::BitfieldDistribution(BitfieldDistributionMessage::DistributeBitfield()) - unimplemented!() - } - - fn generate_invalid_message() -> AllMessages { - unimplemented!() - } - - macro_rules! msg_sequence { - ($( $input:expr ),+ $(,)? ) => [ - vec![ $( FromOverseer::Communication { msg: $input } ),+ ] - ]; - } - - macro_rules! view { - ( $( $hash:expr ),+ $(,)? ) => [ - View(vec![ $( $hash.clone() ),+ ]) - ]; - } - - #[test] - fn boundary_to_boundary() { + use polkadot_primitives::v0::{Signed, ValidatorPair}; + use polkadot_primitives::v1::AvailabilityBitfield; + use sp_core::crypto::Pair; + + fn generate_valid_message() -> AllMessages { + // AllMessages::BitfieldDistribution(BitfieldDistributionMessage::DistributeBitfield()) + unimplemented!() + } + + fn generate_invalid_message() -> AllMessages { + unimplemented!() + } + + macro_rules! msg_sequence { + ($( $input:expr ),+ $(,)? ) => [ + vec![ $( FromOverseer::Communication { msg: $input } ),+ ] + ]; + } + + macro_rules! view { + ( $( $hash:expr ),+ $(,)? ) => [ + View(vec![ $( $hash.clone() ),+ ]) + ]; + } + + #[test] + #[ignore] + fn boundary_to_boundary() { let hash_a: Hash = [0; 32].into(); // us let hash_b: Hash = [1; 32].into(); // other let peer_a = PeerId::random(); - let peer_b = PeerId::random(); - - - let signing_context = SigningContext { - session_index: 1, - parent_hash: hash_a.clone(), - }; - - // validator 0 key pair - let (validator_pair, _seed) = ValidatorPair::generate(); - let validator = validator_pair.public(); - - let payload = AvailabilityBitfield(bitvec![bitvec::order::Lsb0, u8; 1u8; 32]); - let signed = Signed::::sign( - payload, &signing_context, 0, &validator_pair); - - let input = msg_sequence![ - BitfieldDistributionMessage::NetworkBridgeUpdate(NetworkBridgeEvent::OurViewChange(view![hash_a, hash_b])), - BitfieldDistributionMessage::NetworkBridgeUpdate(NetworkBridgeEvent::PeerConnected(peer_b.clone(), ObservedRole::Full)), - BitfieldDistributionMessage::NetworkBridgeUpdate(NetworkBridgeEvent::PeerViewChange(peer_b.clone(), view![hash_a, hash_b])), - BitfieldDistributionMessage::DistributeBitfield(hash_b.clone(), signed.clone()), - ]; - - // empty initial state - let mut tracker = Tracker::default(); - - let pool = sp_core::testing::SpawnBlockingExecutor::new(); - let (mut ctx, mut handle) = subsystem_test::make_subsystem_context::(pool); - - executor::block_on(async move { - for input in input.into_iter() { - handle.send(input.into()); - } - let completion = ctx.spawn("test-system", BitfieldDistribution::start(BitfieldDistribution, ctx.clone()).future); - - while let Ok(rxd) = ctx.recv().await { - // @todo impl expectation checks against a hashmap - dbg!(rxd); - } - }); - - } - - use maplit::hashmap; - use maplit::hashset; - - fn prewarmed_tracker(validator: ValidatorId, signing_context: SigningContext, message: BitfieldGossipMessage, peers: Vec) -> Tracker { - let mut tracker = Tracker::default(); - tracker.per_relay_parent.insert(message.relay_parent.clone(), - PerRelayParentData { - signing_context, - validator_set: vec![validator.clone()], - one_per_validator: hashmap!{ - validator.clone() => message.clone(), - }, - message_sent_to_peer: hashmap!{}, - }); - tracker - } - - - use std::time::Duration; - use smol::Timer; - use smol_timeout::TimeoutExt; - - #[test] - fn relay_must_work() { + let peer_b = PeerId::random(); + + let signing_context = SigningContext { + session_index: 1, + parent_hash: hash_a.clone(), + }; + + // validator 0 key pair + let (validator_pair, _seed) = ValidatorPair::generate(); + let validator = validator_pair.public(); + + let payload = AvailabilityBitfield(bitvec![bitvec::order::Lsb0, u8; 1u8; 32]); + let signed = + Signed::::sign(payload, &signing_context, 0, &validator_pair); + + let input = + msg_sequence![ + BitfieldDistributionMessage::NetworkBridgeUpdate( + NetworkBridgeEvent::OurViewChange(view![hash_a, hash_b]) + ), + BitfieldDistributionMessage::NetworkBridgeUpdate( + NetworkBridgeEvent::PeerConnected(peer_b.clone(), ObservedRole::Full) + ), + BitfieldDistributionMessage::NetworkBridgeUpdate( + NetworkBridgeEvent::PeerViewChange(peer_b.clone(), view![hash_a, hash_b]) + ), + BitfieldDistributionMessage::DistributeBitfield(hash_b.clone(), signed.clone()), + ]; + + // empty initial state + let mut tracker = Tracker::default(); + + let pool = sp_core::testing::SpawnBlockingExecutor::new(); + let (mut ctx, mut handle) = + subsystem_test::make_subsystem_context::(pool); + + executor::block_on(async move { + for input in input.into_iter() { + handle.send(input.into()); + } + + // @todo cannot clone or move `ctx` + // + // let completion = BitfieldDistribution::start(BitfieldDistribution, ctx) + // .future + // .timeout(Duration::from_millis(1000)) + // .await; + + // while let Ok(rxd) = ctx.recv().await { + // // @todo impl expectation checks against a hashmap + // dbg!(rxd); + // } + }); + } + + use maplit::hashmap; + use maplit::hashset; + + fn prewarmed_tracker( + validator: ValidatorId, + signing_context: SigningContext, + message: BitfieldGossipMessage, + peers: Vec, + ) -> Tracker { + let mut tracker = Tracker::default(); + tracker.per_relay_parent.insert( + message.relay_parent.clone(), + PerRelayParentData { + signing_context, + validator_set: vec![validator.clone()], + one_per_validator: hashmap! { + validator.clone() => message.clone(), + }, + message_sent_to_peer: hashmap! {}, + }, + ); + tracker + } + + use smol::Timer; + use smol_timeout::TimeoutExt; + use std::time::Duration; + + #[test] + fn relay_must_work() { let hash_a: Hash = [0; 32].into(); // us let hash_b: Hash = [1; 32].into(); // other let peer_a = PeerId::random(); - let peer_b = PeerId::random(); - assert_ne!(peer_a, peer_b); - - - let signing_context = SigningContext { - session_index: 1, - parent_hash: hash_a.clone(), - }; - - // validator 0 key pair - let (validator_pair, _seed) = ValidatorPair::generate(); - let validator = validator_pair.public(); - - let payload = AvailabilityBitfield(bitvec![bitvec::order::Lsb0, u8; 1u8; 32]); - let signed = Signed::::sign( - payload, &signing_context, 0, &validator_pair); - - let msg = BitfieldGossipMessage { - relay_parent: hash_a.clone(), - signed_availability: signed.clone(), - }; - - - let pool = sp_core::testing::SpawnBlockingExecutor::new(); - let (mut ctx, mut handle) = subsystem_test::make_subsystem_context::(pool); - - let mut tracker = prewarmed_tracker(validator.clone(), signing_context.clone(), msg.clone(), vec![peer_b.clone()]); + let peer_b = PeerId::random(); + assert_ne!(peer_a, peer_b); + + let signing_context = SigningContext { + session_index: 1, + parent_hash: hash_a.clone(), + }; + + // validator 0 key pair + let (validator_pair, _seed) = ValidatorPair::generate(); + let validator = validator_pair.public(); + + let payload = AvailabilityBitfield(bitvec![bitvec::order::Lsb0, u8; 1u8; 32]); + let signed = + Signed::::sign(payload, &signing_context, 0, &validator_pair); + + let msg = BitfieldGossipMessage { + relay_parent: hash_a.clone(), + signed_availability: signed.clone(), + }; + + let pool = sp_core::testing::SpawnBlockingExecutor::new(); + let (mut ctx, mut handle) = + subsystem_test::make_subsystem_context::(pool); + + let mut tracker = prewarmed_tracker( + validator.clone(), + signing_context.clone(), + msg.clone(), + vec![peer_b.clone()], + ); executor::block_on(async move { - let x = async move { send_tracked_gossip_message(&mut ctx, &mut tracker, peer_b, validator, msg).await }; - - let res = dbg!(x.timeout(Duration::from_millis(500)).await); + let res = send_tracked_gossip_message(&mut ctx, &mut tracker, peer_b, validator, msg) + .timeout(Duration::from_millis(100)).await.expect("Ran into timeout for sending"); + assert!(dbg!(res).is_ok()); - println!("complete?"); - // while let Ok(rxd) = async move { - // ctx.recv() - // }.await { - // dbg!(rxd); - // } - }); + while let Some(Ok(rxd)) = ctx.recv().timeout(Duration::from_millis(100)).await { + dbg!(rxd); + } - } + }); + } } From 7a4bd5efadf891b7cb3c0e2f6e3a8fbe245efd06 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Tue, 21 Jul 2020 15:23:14 +0200 Subject: [PATCH 30/45] fix relay_message not tracked when origin is self --- Cargo.lock | 104 ++++++++++- node/network/bitfield-distribution/Cargo.toml | 3 +- node/network/bitfield-distribution/src/lib.rs | 174 +++++++++++------- 3 files changed, 204 insertions(+), 77 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index dba082b8bcc3..fad622fabea0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -219,6 +219,35 @@ version = "1.3.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7deb0a829ca7bcfaf5da70b073a8d128619259a7be8216a355e23f00763059e5" +[[package]] +name = "async-channel" +version = "1.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ee81ba99bee79f3c8ae114ae4baa7eaa326f63447cf2ec65e4393618b63f8770" +dependencies = [ + "concurrent-queue", + "event-listener", + "futures-core", +] + +[[package]] +name = "async-io" +version = "0.1.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4a0fc2017a5cca12763bb5636092a7786b52789c23c5838a392db2eb99963fd3" +dependencies = [ + "cfg-if", + "concurrent-queue", + "futures-lite", + "libc", + "once_cell", + "parking", + "socket2", + "vec-arena", + "wepoll-sys-stjepang", + "winapi 0.3.9", +] + [[package]] name = "async-std" version = "1.6.2" @@ -239,7 +268,7 @@ dependencies = [ "pin-project-lite", "pin-utils", "slab", - "smol", + "smol 0.1.18", "wasm-bindgen-futures", ] @@ -272,6 +301,12 @@ dependencies = [ "syn 1.0.33", ] +[[package]] +name = "atomic-waker" +version = "1.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "065374052e7df7ee4047b1160cca5e1467a12351a40b3da123c870ba0b8eda2a" + [[package]] name = "atty" version = "0.2.14" @@ -469,12 +504,13 @@ dependencies = [ [[package]] name = "blocking" -version = "0.4.6" +version = "0.4.7" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "9d17efb70ce4421e351d61aafd90c16a20fb5bfe339fcdc32a86816280e62ce0" +checksum = "d2468ff7bf85066b4a3678fede6fe66db31846d753ff0adfbfab2c6a6e81612b" dependencies = [ - "futures-channel", - "futures-util", + "async-channel", + "atomic-waker", + "futures-lite", "once_cell", "parking", "waker-fn", @@ -1161,6 +1197,12 @@ dependencies = [ "serde_json", ] +[[package]] +name = "event-listener" +version = "2.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "699d84875f1b72b4da017e6b0f77dfa88c0137f089958a88974d15938cbc2976" + [[package]] name = "exit-future" version = "0.2.0" @@ -1614,6 +1656,19 @@ version = "0.3.5" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "de27142b013a8e869c14957e6d2edeef89e97c289e69d042ee3a49acd8b51789" +[[package]] +name = "futures-lite" +version = "0.1.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "af0bbcb0ec905ef6ee23fab499119b5da2362b8697d66e08d1ef01a8c0d438e2" +dependencies = [ + "fastrand", + "futures-core", + "futures-io", + "memchr", + "pin-project-lite", +] + [[package]] name = "futures-macro" version = "0.3.5" @@ -3237,6 +3292,17 @@ dependencies = [ "unsigned-varint 0.4.0", ] +[[package]] +name = "multitask" +version = "0.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c09c35271e7dcdb5f709779111f2c8e8ab8e06c1b587c1c6a9e179d865aaa5b4" +dependencies = [ + "async-task", + "concurrent-queue", + "fastrand", +] + [[package]] name = "nalgebra" version = "0.18.1" @@ -4139,9 +4205,9 @@ checksum = "ddfc878dac00da22f8f61e7af3157988424567ab01d9920b962ef7dcbd7cd865" [[package]] name = "parking" -version = "1.0.4" +version = "1.0.5" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1efcee3c6d23b94012e240525f131c6abaa9e5eeb8f211002d93beec3b7be350" +checksum = "50d4a6da31f8144a32532fe38fe8fb439a6842e0ec633f0037f0144c14e7f907" [[package]] name = "parking_lot" @@ -4325,6 +4391,7 @@ name = "polkadot-availability-bitfield-distribution" version = "0.1.0" dependencies = [ "bitvec", + "env_logger", "futures 0.3.5", "futures-timer 3.0.2", "log 0.4.8", @@ -4338,7 +4405,7 @@ dependencies = [ "polkadot-primitives", "polkadot-subsystem-test-helpers", "sc-network", - "smol", + "smol 0.2.0", "smol-timeout", "sp-core", "streamunordered", @@ -7222,6 +7289,19 @@ dependencies = [ "winapi 0.3.9", ] +[[package]] +name = "smol" +version = "0.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "346a94824d48ed7c5fc7247f3cbbf0317bdfe15fc39d08f9262609cccce61254" +dependencies = [ + "async-io", + "blocking", + "multitask", + "num_cpus", + "once_cell", +] + [[package]] name = "smol-timeout" version = "0.1.0" @@ -7229,7 +7309,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "024818c1f00b80e8171ddcfcee33860134293aa3aced60c9cbd7a5a2d41db392" dependencies = [ "pin-project", - "smol", + "smol 0.1.18", ] [[package]] @@ -9089,6 +9169,12 @@ version = "0.2.10" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "6454029bf181f092ad1b853286f23e2c507d8e8194d01d92da4a55c274a5508c" +[[package]] +name = "vec-arena" +version = "0.5.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "17dfb54bf57c9043f4616cb03dab30eff012cc26631b797d8354b916708db919" + [[package]] name = "vec_map" version = "0.8.2" diff --git a/node/network/bitfield-distribution/Cargo.toml b/node/network/bitfield-distribution/Cargo.toml index 7497fae8d1f2..064f9da4538d 100644 --- a/node/network/bitfield-distribution/Cargo.toml +++ b/node/network/bitfield-distribution/Cargo.toml @@ -23,5 +23,6 @@ sp-core = { git = "https://github.com/paritytech/substrate", branch = "master" } subsystem-test = { package = "polkadot-subsystem-test-helpers", path = "../../test-helpers/subsystem" } parking_lot = "0.10.0" maplit = "1.0.2" +smol = "0.2" smol-timeout = "0.1" -smol = "0.1" \ No newline at end of file +env_logger = "0.7" \ No newline at end of file diff --git a/node/network/bitfield-distribution/src/lib.rs b/node/network/bitfield-distribution/src/lib.rs index 28d9aeaa899f..5a7b0c14f794 100644 --- a/node/network/bitfield-distribution/src/lib.rs +++ b/node/network/bitfield-distribution/src/lib.rs @@ -25,6 +25,7 @@ use futures::{channel::oneshot, FutureExt}; use node_primitives::{ProtocolId, View}; +use log::{debug, info, trace, warn}; use polkadot_node_subsystem::messages::*; use polkadot_node_subsystem::{ FromOverseer, OverseerSignal, SpawnedSubsystem, Subsystem, SubsystemContext, SubsystemResult, @@ -138,23 +139,39 @@ impl BitfieldDistribution { hash, signed_availability, ) => { + trace!(target: "bitd", "Processing DistributeBitfield"); + let per_job = &mut tracker.per_relay_parent.get_mut(&hash).expect("Overseer does not send work items related to relay parents that are not part of our workset. qed"); + + let validator = { + per_job + .validator_set + .get(signed_availability.validator_index() as usize) + .expect("Our own validation index exists. qed") + } + .clone(); + + let peer_views = &mut tracker.peer_views; let msg = BitfieldGossipMessage { relay_parent: hash, signed_availability, }; - relay_message(&mut ctx, &mut tracker, msg).await?; + + relay_message(&mut ctx, per_job, peer_views, validator, msg) + .await?; } BitfieldDistributionMessage::NetworkBridgeUpdate(event) => { + trace!(target: "bitd", "Processing NetworkMessage"); // a network message was received if let Err(e) = handle_network_msg(&mut ctx, &mut tracker, event).await { - log::warn!(target: "bitd", "Failed to handle incomming network messages: {:?}", e); + warn!(target: "bitd", "Failed to handle incomming network messages: {:?}", e); } } } } FromOverseer::Signal(OverseerSignal::StartWork(relay_parent)) => { + trace!(target: "bitd", "Start {:?}", relay_parent); // query basic system parameters once // @todo assumption: these cannot change within a session let (validator_set, signing_context) = @@ -170,11 +187,13 @@ impl BitfieldDistribution { ); } FromOverseer::Signal(OverseerSignal::StopWork(relay_parent)) => { + trace!(target: "bitd", "Stop {:?}", relay_parent); // @todo assumption: it is good enough to prevent additional work from being // scheduled, the individual futures are supposedly completed quickly let _ = tracker.per_relay_parent.remove(&relay_parent); } FromOverseer::Signal(OverseerSignal::Conclude) => { + trace!(target: "bitd", "Conclude"); tracker.per_relay_parent.clear(); return Ok(()); } @@ -187,14 +206,15 @@ impl BitfieldDistribution { /// Modify the reputation of peer based on their behaviour. async fn modify_reputation( ctx: &mut Context, - peerid: PeerId, + peer: PeerId, rep: ReputationChange, ) -> SubsystemResult<()> where Context: SubsystemContext, { + trace!(target: "bitd", "Reputation change of {:?} for peer {:?}", rep, peer); ctx.send_message(AllMessages::NetworkBridge( - NetworkBridgeMessage::ReportPeer(peerid, rep), + NetworkBridgeMessage::ReportPeer(peer, rep), )) .await } @@ -204,19 +224,29 @@ where /// Can be originated by another subsystem or received via network from another peer. async fn relay_message( ctx: &mut Context, - tracker: &mut Tracker, + per_job: &mut PerRelayParentData, + peer_views: &mut HashMap, + validator: ValidatorId, message: BitfieldGossipMessage, ) -> SubsystemResult<()> where Context: SubsystemContext, { + let message_sent_to_peer = &mut (per_job.message_sent_to_peer); + // concurrently pass on the bitfield distribution to all interested peers - let interested_peers = tracker - .peer_views + let interested_peers = peer_views .iter() - .filter_map(|(peerid, view)| { + .filter_map(|(peer, view)| { + // check interest in the peer in this message's relay parent if view.contains(&message.relay_parent) { - Some(peerid.clone()) + // track the message as sent for this peer + message_sent_to_peer + .entry(peer.clone()) + .or_default() + .insert(validator.clone()); + + Some(peer.clone()) } else { None } @@ -263,36 +293,39 @@ where return modify_reputation(ctx, origin, COST_MISSING_PEER_SESSION_KEY).await; } - // check all validators that could have signed this message - // @todo there must be a better way figuring this out cheaply + // use the (untrusted) validator index provided by the signed payload + // and see if that one actually signed the availability bitset let signing_context = job_data.signing_context.clone(); - if let Some(validator) = validator_set.iter().find(|validator| { - message - .signed_availability - .check_signature(&signing_context, validator) - .is_ok() - }) { + let validator_index = message.signed_availability.validator_index() as usize; + let validator = if let Some(validator) = validator_set.get(validator_index) { + validator.clone() + } else { + return modify_reputation(ctx, origin, COST_SIGNATURE_INVALID).await; + }; + + if message + .signed_availability + .check_signature(&signing_context, &validator) + .is_ok() + { let one_per_validator = &mut (job_data.one_per_validator); // only relay_message a message of a validator once - if one_per_validator.get(validator).is_some() { + if one_per_validator.get(&validator).is_some() { + trace!(target: "bitd", "Alrady received a message for validator at index {}", validator_index); return Ok(()); } one_per_validator.insert(validator.clone(), message.clone()); // track which messages that peer already received - let message_sent_to_peer = &mut (job_data.message_sent_to_peer); - message_sent_to_peer - .entry(origin) - .or_insert_with(|| HashSet::default()) - .insert(validator.clone()); + // let message_sent_to_peer = &mut (job_data.message_sent_to_peer); + // message_sent_to_peer + // .entry(origin) + // .or_insert_with(|| HashSet::default()) + // .insert(validator.clone()); + relay_message(ctx, job_data, &mut tracker.peer_views, validator, message).await } else { return modify_reputation(ctx, origin, COST_SIGNATURE_INVALID).await; } - - // passed all conditions, distribute to peers! - relay_message(ctx, tracker, message).await?; - - Ok(()) } /// Deal with network bridge updates and track what needs to be tracked @@ -321,13 +354,13 @@ where for new in tracker.view.difference(&old_view) { if !tracker.per_relay_parent.contains_key(&new) { - log::warn!(target: "bitd", "Our view contains {} but the overseer never told use we should work on this", &new); + warn!(target: "bitd", "Our view contains {} but the overseer never told use we should work on this", &new); } } } NetworkBridgeEvent::PeerMessage(remote, bytes) => { if let Ok(gossiped_bitfield) = BitfieldGossipMessage::decode(&mut (bytes.as_slice())) { - log::trace!(target: "bitd", "Received bitfield gossip from peer {:?}", &remote); + trace!(target: "bitd", "Received bitfield gossip from peer {:?}", &remote); process_incoming_peer_message(ctx, tracker, remote, gossiped_bitfield).await?; } else { return modify_reputation(ctx, remote, COST_MESSAGE_NOT_DECODABLE).await; @@ -403,7 +436,7 @@ where let per_job = if let Some(per_job) = tracker.per_relay_parent.get_mut(&message.relay_parent) { per_job } else { - // TODO punishing here seems unreasonable + // @todo punishing here seems unreasonable return Ok(()); }; @@ -467,16 +500,10 @@ mod test { use futures::executor; use polkadot_primitives::v0::{Signed, ValidatorPair}; use polkadot_primitives::v1::AvailabilityBitfield; + use smol_timeout::TimeoutExt; use sp_core::crypto::Pair; - - fn generate_valid_message() -> AllMessages { - // AllMessages::BitfieldDistribution(BitfieldDistributionMessage::DistributeBitfield()) - unimplemented!() - } - - fn generate_invalid_message() -> AllMessages { - unimplemented!() - } + use std::time::Duration; + use maplit::{hashmap, hashset}; macro_rules! msg_sequence { ($( $input:expr ),+ $(,)? ) => [ @@ -552,37 +579,44 @@ mod test { }); } - use maplit::hashmap; - use maplit::hashset; - + /// A very limited tracker, only interested in the relay parent of the + /// given message, which must be signed by `validator` and a set of peers + /// which are also only interested in that relay parent. fn prewarmed_tracker( validator: ValidatorId, signing_context: SigningContext, - message: BitfieldGossipMessage, + known_message: BitfieldGossipMessage, peers: Vec, ) -> Tracker { - let mut tracker = Tracker::default(); - tracker.per_relay_parent.insert( - message.relay_parent.clone(), - PerRelayParentData { - signing_context, - validator_set: vec![validator.clone()], - one_per_validator: hashmap! { - validator.clone() => message.clone(), - }, - message_sent_to_peer: hashmap! {}, + let relay_parent = known_message.relay_parent.clone(); + Tracker { + per_relay_parent: hashmap! { + relay_parent.clone() => + PerRelayParentData { + signing_context, + validator_set: vec![validator.clone()], + one_per_validator: hashmap! { + validator.clone() => known_message.clone(), + }, + message_sent_to_peer: hashmap! {}, + }, }, - ); - tracker + peer_views: peers + .into_iter() + .map(|peer| (peer, view!(relay_parent))) + .collect(), + view: view!(relay_parent), + } } - use smol::Timer; - use smol_timeout::TimeoutExt; - use std::time::Duration; - #[test] - fn relay_must_work() { - let hash_a: Hash = [0; 32].into(); // us + fn receive_invalid_signature() { + let _ = env_logger::builder() + .filter(None, log::LevelFilter::Trace) + .is_test(true) + .try_init(); + + let hash_a: Hash = [0; 32].into(); let hash_b: Hash = [1; 32].into(); // other let peer_a = PeerId::random(); @@ -598,9 +632,12 @@ mod test { let (validator_pair, _seed) = ValidatorPair::generate(); let validator = validator_pair.public(); + // another validator not part of the validatorset + let (mallicious, _seed) = ValidatorPair::generate(); + let payload = AvailabilityBitfield(bitvec![bitvec::order::Lsb0, u8; 1u8; 32]); let signed = - Signed::::sign(payload, &signing_context, 0, &validator_pair); + Signed::::sign(payload, &signing_context, 0, &mallicious); let msg = BitfieldGossipMessage { relay_parent: hash_a.clone(), @@ -619,14 +656,17 @@ mod test { ); executor::block_on(async move { - let res = send_tracked_gossip_message(&mut ctx, &mut tracker, peer_b, validator, msg) - .timeout(Duration::from_millis(100)).await.expect("Ran into timeout for sending"); + let res = handle_network_msg(&mut ctx, &mut tracker, NetworkBridgeEvent::PeerMessage(peer_b.clone(), msg.encode())) + .timeout(Duration::from_millis(10)) + .await + .expect("10ms is more than enough for sending messages. qed"); + assert!(dbg!(res).is_ok()); - while let Some(Ok(rxd)) = ctx.recv().timeout(Duration::from_millis(100)).await { + // we should have a reputiation change due to invalid signature + while let Some(Ok(Some(rxd))) = ctx.try_recv().timeout(Duration::from_millis(100)).await { dbg!(rxd); } - }); } } From b299ad308028f9c1e0554985d356492ca72a43fe Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Tue, 21 Jul 2020 16:47:51 +0200 Subject: [PATCH 31/45] fix/guide: grammar Co-authored-by: Robert Habermeier --- .../src/node/availability/bitfield-distribution.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/roadmap/implementers-guide/src/node/availability/bitfield-distribution.md b/roadmap/implementers-guide/src/node/availability/bitfield-distribution.md index 5b6a6f9b4dda..528b5f9d1d74 100644 --- a/roadmap/implementers-guide/src/node/availability/bitfield-distribution.md +++ b/roadmap/implementers-guide/src/node/availability/bitfield-distribution.md @@ -11,7 +11,7 @@ Input: Output: -- `NetworkBridge::RegisterEventProducer(ProtocolId)` in order to register ourselfs as an event provide for the protocol. +- `NetworkBridge::RegisterEventProducer(ProtocolId)` in order to register ourself as an event provider for the protocol. - `NetworkBridge::SendMessage([PeerId], ProtocolId, Bytes)` gossip a verified incoming bitfield on to interested subsystems within this validator node. - `NetworkBridge::ReportPeer(PeerId, cost_or_benefit)` improve or penalize the reputation of peers based on the messages that are received relative to the current view. - `ProvisionerMessage::ProvisionableData(ProvisionableData::Bitfield(relay_parent, SignedAvailabilityBitfield))` pass From 09766c154acd00f475b54e633cb4ce3e38ed71ea Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Tue, 21 Jul 2020 17:13:12 +0200 Subject: [PATCH 32/45] work around missing Eq+PartialEq --- node/network/bitfield-distribution/src/lib.rs | 30 +++++++++---------- 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/node/network/bitfield-distribution/src/lib.rs b/node/network/bitfield-distribution/src/lib.rs index 5a7b0c14f794..fc1dbd66a651 100644 --- a/node/network/bitfield-distribution/src/lib.rs +++ b/node/network/bitfield-distribution/src/lib.rs @@ -37,6 +37,8 @@ use std::collections::{HashMap, HashSet}; const COST_SIGNATURE_INVALID: ReputationChange = ReputationChange::new(-100, "Bitfield signature invalid"); +const COST_VALIDATOR_INDEX_INVALID: ReputationChange = + ReputationChange::new(-100, "Bitfield validator index invalid"); const COST_MISSING_PEER_SESSION_KEY: ReputationChange = ReputationChange::new(-133, "Missing peer session key"); const COST_NOT_INTERESTED: ReputationChange = @@ -46,7 +48,7 @@ const COST_MESSAGE_NOT_DECODABLE: ReputationChange = /// Checked signed availability bitfield that is distributed /// to other peers. -#[derive(Encode, Decode, Debug, Clone)] +#[derive(Encode, Decode, Debug, Clone, PartialEq, Eq)] pub struct BitfieldGossipMessage { /// The relay parent this message is relative to. pub relay_parent: Hash, @@ -253,12 +255,11 @@ where }) .collect::>(); - let bytes = Encode::encode(&message); ctx.send_message(AllMessages::NetworkBridge( NetworkBridgeMessage::SendMessage( interested_peers, BitfieldDistribution::PROTOCOL_ID, - bytes, + Encode::encode(&message), ), )) .await?; @@ -300,7 +301,7 @@ where let validator = if let Some(validator) = validator_set.get(validator_index) { validator.clone() } else { - return modify_reputation(ctx, origin, COST_SIGNATURE_INVALID).await; + return modify_reputation(ctx, origin, COST_VALIDATOR_INDEX_INVALID).await; }; if message @@ -316,12 +317,6 @@ where } one_per_validator.insert(validator.clone(), message.clone()); - // track which messages that peer already received - // let message_sent_to_peer = &mut (job_data.message_sent_to_peer); - // message_sent_to_peer - // .entry(origin) - // .or_insert_with(|| HashSet::default()) - // .insert(validator.clone()); relay_message(ctx, job_data, &mut tracker.peer_views, validator, message).await } else { return modify_reputation(ctx, origin, COST_SIGNATURE_INVALID).await; @@ -656,16 +651,19 @@ mod test { ); executor::block_on(async move { - let res = handle_network_msg(&mut ctx, &mut tracker, NetworkBridgeEvent::PeerMessage(peer_b.clone(), msg.encode())) + let _res = handle_network_msg(&mut ctx, &mut tracker, NetworkBridgeEvent::PeerMessage(peer_b.clone(), msg.encode())) .timeout(Duration::from_millis(10)) .await - .expect("10ms is more than enough for sending messages. qed"); - - assert!(dbg!(res).is_ok()); + .expect("10ms is more than enough for sending messages. qed") + .expect("There are no error values. qed"); // we should have a reputiation change due to invalid signature - while let Some(Ok(Some(rxd))) = ctx.try_recv().timeout(Duration::from_millis(100)).await { - dbg!(rxd); + // @todo assess if Eq+PartialEq are viable to simplify this kind of code + if let AllMessages::NetworkBridge(NetworkBridgeMessage::ReportPeer(peer, rep)) = handle.recv().await { + assert_eq!(peer, peer_b); + assert_eq!(rep, COST_SIGNATURE_INVALID); + } else { + panic!("Received unexpected message type."); } }); } From df7a6f004b8644fdb04742cc79077f1b3d3a36d0 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Tue, 21 Jul 2020 17:23:27 +0200 Subject: [PATCH 33/45] fix: add missing message to provisioner --- node/network/bitfield-distribution/src/lib.rs | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/node/network/bitfield-distribution/src/lib.rs b/node/network/bitfield-distribution/src/lib.rs index fc1dbd66a651..cb2d76eea59a 100644 --- a/node/network/bitfield-distribution/src/lib.rs +++ b/node/network/bitfield-distribution/src/lib.rs @@ -221,7 +221,7 @@ where .await } -/// Distribute a given valid bitfield message. +/// Distribute a given valid and signature checked bitfield message. /// /// Can be originated by another subsystem or received via network from another peer. async fn relay_message( @@ -234,6 +234,19 @@ async fn relay_message( where Context: SubsystemContext, { + + // notify the overseer about a new and valid signed bitfield + ctx.send_message( + AllMessages::Provisioner( + ProvisionerMessage::ProvisionableData( + ProvisionableData::Bitfield( + message.relay_parent.clone(), + message.signed_availability.clone(), + ) + ) + ) + ).await; + let message_sent_to_peer = &mut (per_job.message_sent_to_peer); // concurrently pass on the bitfield distribution to all interested peers @@ -324,6 +337,7 @@ where } /// Deal with network bridge updates and track what needs to be tracked +/// which depends on the message type received. async fn handle_network_msg( ctx: &mut Context, tracker: &mut Tracker, @@ -461,7 +475,7 @@ where } } -/// query the validator set and signing context +/// Query our validator set and signing context for a particular relay parent. async fn query_basics( ctx: &mut Context, relay_parent: Hash, From 2398681a99e460206bc5f9034955f3b9a89be7dc Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Tue, 21 Jul 2020 17:24:47 +0200 Subject: [PATCH 34/45] unify per_job to job_data --- node/network/bitfield-distribution/src/lib.rs | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/node/network/bitfield-distribution/src/lib.rs b/node/network/bitfield-distribution/src/lib.rs index cb2d76eea59a..588c20b3c28e 100644 --- a/node/network/bitfield-distribution/src/lib.rs +++ b/node/network/bitfield-distribution/src/lib.rs @@ -142,10 +142,10 @@ impl BitfieldDistribution { signed_availability, ) => { trace!(target: "bitd", "Processing DistributeBitfield"); - let per_job = &mut tracker.per_relay_parent.get_mut(&hash).expect("Overseer does not send work items related to relay parents that are not part of our workset. qed"); + let job_data = &mut tracker.per_relay_parent.get_mut(&hash).expect("Overseer does not send work items related to relay parents that are not part of our workset. qed"); let validator = { - per_job + job_data .validator_set .get(signed_availability.validator_index() as usize) .expect("Our own validation index exists. qed") @@ -158,7 +158,7 @@ impl BitfieldDistribution { signed_availability, }; - relay_message(&mut ctx, per_job, peer_views, validator, msg) + relay_message(&mut ctx, job_data, peer_views, validator, msg) .await?; } BitfieldDistributionMessage::NetworkBridgeUpdate(event) => { @@ -226,7 +226,7 @@ where /// Can be originated by another subsystem or received via network from another peer. async fn relay_message( ctx: &mut Context, - per_job: &mut PerRelayParentData, + job_data: &mut PerRelayParentData, peer_views: &mut HashMap, validator: ValidatorId, message: BitfieldGossipMessage, @@ -247,7 +247,7 @@ where ) ).await; - let message_sent_to_peer = &mut (per_job.message_sent_to_peer); + let message_sent_to_peer = &mut (job_data.message_sent_to_peer); // concurrently pass on the bitfield distribution to all interested peers let interested_peers = peer_views @@ -403,9 +403,9 @@ where let delta_set: HashMap = delta_vec .into_iter() .filter_map(|new_relay_parent_interest| { - if let Some(per_job) = (&*tracker).per_relay_parent.get(&new_relay_parent_interest) { + if let Some(job_data) = (&*tracker).per_relay_parent.get(&new_relay_parent_interest) { // send all messages - let one_per_validator = per_job.one_per_validator.clone(); + let one_per_validator = job_data.one_per_validator.clone(); let origin = origin.clone(); Some( one_per_validator @@ -413,7 +413,7 @@ where .filter(move |(validator, _message)| { // except for the ones the peer already has // let validator = validator.clone(); - per_job.message_from_validator_needed_by_peer(&origin, validator) + job_data.message_from_validator_needed_by_peer(&origin, validator) }), ) } else { @@ -442,14 +442,14 @@ async fn send_tracked_gossip_message( where Context: SubsystemContext, { - let per_job = if let Some(per_job) = tracker.per_relay_parent.get_mut(&message.relay_parent) { - per_job + let job_data = if let Some(job_data) = tracker.per_relay_parent.get_mut(&message.relay_parent) { + job_data } else { // @todo punishing here seems unreasonable return Ok(()); }; - let message_sent_to_peer = &mut (per_job.message_sent_to_peer); + let message_sent_to_peer = &mut (job_data.message_sent_to_peer); message_sent_to_peer .entry(dest.clone()) .or_default() From 8ed8aba91651d701c243d7e6bae8445eb0a97a04 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Wed, 22 Jul 2020 10:18:50 +0200 Subject: [PATCH 35/45] fix/review: part one --- node/network/bitfield-distribution/src/lib.rs | 284 +++++++++++++----- 1 file changed, 213 insertions(+), 71 deletions(-) diff --git a/node/network/bitfield-distribution/src/lib.rs b/node/network/bitfield-distribution/src/lib.rs index 588c20b3c28e..deb00b499a2e 100644 --- a/node/network/bitfield-distribution/src/lib.rs +++ b/node/network/bitfield-distribution/src/lib.rs @@ -30,8 +30,7 @@ use polkadot_node_subsystem::messages::*; use polkadot_node_subsystem::{ FromOverseer, OverseerSignal, SpawnedSubsystem, Subsystem, SubsystemContext, SubsystemResult, }; -use polkadot_primitives::v0::{SigningContext, ValidatorId}; -use polkadot_primitives::v1::{Hash, SignedAvailabilityBitfield}; +use polkadot_primitives::v1::{SigningContext, ValidatorId, Hash, SignedAvailabilityBitfield}; use sc_network::ReputationChange; use std::collections::{HashMap, HashSet}; @@ -45,6 +44,10 @@ const COST_NOT_INTERESTED: ReputationChange = ReputationChange::new(-51, "Not intersted in that parent hash"); const COST_MESSAGE_NOT_DECODABLE: ReputationChange = ReputationChange::new(-100, "Not intersted in that parent hash"); +const GAIN_VALID_MESSAGE_FIRST: ReputationChange = + ReputationChange::new(15, "Valid message with new information"); +const GAIN_VALID_MESSAGE: ReputationChange = + ReputationChange::new(5, "Valid message"); /// Checked signed availability bitfield that is distributed /// to other peers. @@ -75,7 +78,7 @@ struct Tracker { #[derive(Debug, Clone, Default)] struct PerRelayParentData { /// Signing context for a particular relay parent. - signing_context: SigningContext, + signing_context: SigningContext, /// Set of validators for a particular relay parent. validator_set: Vec, @@ -130,75 +133,73 @@ impl BitfieldDistribution { // work: process incoming messages from the overseer and process accordingly. let mut tracker = Tracker::default(); loop { - { - let message = ctx.recv().await?; - match message { - FromOverseer::Communication { msg } => { - // another subsystem created this signed availability bitfield messages - match msg { - // relay_message a bitfield via gossip to other validators - BitfieldDistributionMessage::DistributeBitfield( - hash, - signed_availability, - ) => { - trace!(target: "bitd", "Processing DistributeBitfield"); - let job_data = &mut tracker.per_relay_parent.get_mut(&hash).expect("Overseer does not send work items related to relay parents that are not part of our workset. qed"); - - let validator = { - job_data - .validator_set - .get(signed_availability.validator_index() as usize) - .expect("Our own validation index exists. qed") - } - .clone(); - - let peer_views = &mut tracker.peer_views; - let msg = BitfieldGossipMessage { - relay_parent: hash, - signed_availability, - }; - - relay_message(&mut ctx, job_data, peer_views, validator, msg) - .await?; + let message = ctx.recv().await?; + match message { + FromOverseer::Communication { msg } => { + // another subsystem created this signed availability bitfield messages + match msg { + // relay_message a bitfield via gossip to other validators + BitfieldDistributionMessage::DistributeBitfield( + hash, + signed_availability, + ) => { + trace!(target: "bitd", "Processing DistributeBitfield"); + let job_data = &mut tracker.per_relay_parent.get_mut(&hash).expect("Overseer does not send work items related to relay parents that are not part of our workset. qed"); + + let validator = { + job_data + .validator_set + .get(signed_availability.validator_index() as usize) + .expect("Our own validation index exists. qed") } - BitfieldDistributionMessage::NetworkBridgeUpdate(event) => { - trace!(target: "bitd", "Processing NetworkMessage"); - // a network message was received - if let Err(e) = - handle_network_msg(&mut ctx, &mut tracker, event).await - { - warn!(target: "bitd", "Failed to handle incomming network messages: {:?}", e); - } + .clone(); + + let peer_views = &mut tracker.peer_views; + let msg = BitfieldGossipMessage { + relay_parent: hash, + signed_availability, + }; + + relay_message(&mut ctx, job_data, peer_views, validator, msg) + .await?; + } + BitfieldDistributionMessage::NetworkBridgeUpdate(event) => { + trace!(target: "bitd", "Processing NetworkMessage"); + // a network message was received + if let Err(e) = + handle_network_msg(&mut ctx, &mut tracker, event).await + { + warn!(target: "bitd", "Failed to handle incomming network messages: {:?}", e); } } } - FromOverseer::Signal(OverseerSignal::StartWork(relay_parent)) => { - trace!(target: "bitd", "Start {:?}", relay_parent); - // query basic system parameters once - // @todo assumption: these cannot change within a session - let (validator_set, signing_context) = - query_basics(&mut ctx, relay_parent).await?; - - let _ = tracker.per_relay_parent.insert( - relay_parent, - PerRelayParentData { - signing_context, - validator_set, - ..Default::default() - }, - ); - } - FromOverseer::Signal(OverseerSignal::StopWork(relay_parent)) => { - trace!(target: "bitd", "Stop {:?}", relay_parent); - // @todo assumption: it is good enough to prevent additional work from being - // scheduled, the individual futures are supposedly completed quickly - let _ = tracker.per_relay_parent.remove(&relay_parent); - } - FromOverseer::Signal(OverseerSignal::Conclude) => { - trace!(target: "bitd", "Conclude"); - tracker.per_relay_parent.clear(); - return Ok(()); - } + } + FromOverseer::Signal(OverseerSignal::StartWork(relay_parent)) => { + trace!(target: "bitd", "Start {:?}", relay_parent); + // query basic system parameters once + // @todo assumption: these cannot change within a session + let (validator_set, signing_context) = + query_basics(&mut ctx, relay_parent).await?; + + let _ = tracker.per_relay_parent.insert( + relay_parent, + PerRelayParentData { + signing_context, + validator_set, + ..Default::default() + }, + ); + } + FromOverseer::Signal(OverseerSignal::StopWork(relay_parent)) => { + trace!(target: "bitd", "Stop {:?}", relay_parent); + // @todo assumption: it is good enough to prevent additional work from being + // scheduled, the individual futures are supposedly completed quickly + let _ = tracker.per_relay_parent.remove(&relay_parent); + } + FromOverseer::Signal(OverseerSignal::Conclude) => { + trace!(target: "bitd", "Conclude"); + tracker.per_relay_parent.clear(); + return Ok(()); } } } @@ -330,9 +331,11 @@ where } one_per_validator.insert(validator.clone(), message.clone()); - relay_message(ctx, job_data, &mut tracker.peer_views, validator, message).await + relay_message(ctx, job_data, &mut tracker.peer_views, validator, message).await; + + modify_reputation(ctx, origin, GAIN_VALID_MESSAGE).await } else { - return modify_reputation(ctx, origin, COST_SIGNATURE_INVALID).await; + modify_reputation(ctx, origin, COST_SIGNATURE_INVALID).await } } @@ -412,7 +415,6 @@ where .into_iter() .filter(move |(validator, _message)| { // except for the ones the peer already has - // let validator = validator.clone(); job_data.message_from_validator_needed_by_peer(&origin, validator) }), ) @@ -681,4 +683,144 @@ mod test { } }); } + + + #[test] + fn receive_invalid_validator_index() { + let _ = env_logger::builder() + .filter(None, log::LevelFilter::Trace) + .is_test(true) + .try_init(); + + let hash_a: Hash = [0; 32].into(); + let hash_b: Hash = [1; 32].into(); // other + + let peer_a = PeerId::random(); + let peer_b = PeerId::random(); + assert_ne!(peer_a, peer_b); + + let signing_context = SigningContext { + session_index: 1, + parent_hash: hash_a.clone(), + }; + + // validator 0 key pair + let (validator_pair, _seed) = ValidatorPair::generate(); + let validator = validator_pair.public(); + + let payload = AvailabilityBitfield(bitvec![bitvec::order::Lsb0, u8; 1u8; 32]); + let signed = + Signed::::sign(payload, &signing_context, 42, &validator_pair); + + let msg = BitfieldGossipMessage { + relay_parent: hash_a.clone(), + signed_availability: signed.clone(), + }; + + let pool = sp_core::testing::SpawnBlockingExecutor::new(); + let (mut ctx, mut handle) = + subsystem_test::make_subsystem_context::(pool); + + let mut tracker = prewarmed_tracker( + validator.clone(), + signing_context.clone(), + msg.clone(), + vec![peer_b.clone()], + ); + + executor::block_on(async move { + let _res = handle_network_msg(&mut ctx, &mut tracker, NetworkBridgeEvent::PeerMessage(peer_b.clone(), msg.encode())) + .timeout(Duration::from_millis(10)) + .await + .expect("10ms is more than enough for sending messages. qed") + .expect("There are no error values. qed"); + + // we should have a reputiation change due to invalid signature + // @todo assess if Eq+PartialEq are viable to simplify this kind of code + if let AllMessages::NetworkBridge(NetworkBridgeMessage::ReportPeer(peer, rep)) = handle.recv().await { + assert_eq!(peer, peer_b); + assert_eq!(rep, COST_VALIDATOR_INDEX_INVALID); + } else { + panic!("Received unexpected message type."); + } + }); + } + + + + + + #[test] + fn duplicate_message() { + let _ = env_logger::builder() + .filter(None, log::LevelFilter::Trace) + .is_test(true) + .try_init(); + + let hash_a: Hash = [0; 32].into(); + let hash_b: Hash = [1; 32].into(); // other + + let peer_a = PeerId::random(); + let peer_b = PeerId::random(); + assert_ne!(peer_a, peer_b); + + let signing_context = SigningContext { + session_index: 1, + parent_hash: hash_a.clone(), + }; + + // validator 0 key pair + let (validator_pair, _seed) = ValidatorPair::generate(); + let validator = validator_pair.public(); + + let payload = AvailabilityBitfield(bitvec![bitvec::order::Lsb0, u8; 1u8; 32]); + let signed = + Signed::::sign(payload, &signing_context, 42, &validator_pair); + + let msg = BitfieldGossipMessage { + relay_parent: hash_a.clone(), + signed_availability: signed.clone(), + }; + + let pool = sp_core::testing::SpawnBlockingExecutor::new(); + let (mut ctx, mut handle) = + subsystem_test::make_subsystem_context::(pool); + + let mut tracker = prewarmed_tracker( + validator.clone(), + signing_context.clone(), + msg.clone(), + vec![peer_b.clone()], + ); + + executor::block_on(async move { + let _res = handle_network_msg(&mut ctx, &mut tracker, NetworkBridgeEvent::PeerMessage(peer_b.clone(), msg.encode())) + .timeout(Duration::from_millis(10)) + .await + .expect("10ms is more than enough for sending messages. qed") + .expect("There are no error values. qed"); + + if let AllMessages::NetworkBridge(NetworkBridgeMessage::ReportPeer(peer, rep)) = handle.recv().await { + assert_eq!(peer, peer_b); + assert_eq!(rep, COST_VALIDATOR_INDEX_INVALID); + } else { + panic!("Received unexpected message type."); + } + + let _res = handle_network_msg(&mut ctx, &mut tracker, NetworkBridgeEvent::PeerMessage(peer_b.clone(), msg.encode())) + .timeout(Duration::from_millis(10)) + .await + .expect("10ms is more than enough for sending messages. qed") + .expect("There are no error values. qed"); + + // we should have a reputiation change due to invalid signature + // @todo assess if Eq+PartialEq are viable to simplify this kind of code + if let AllMessages::NetworkBridge(NetworkBridgeMessage::ReportPeer(peer, rep)) = handle.recv().await { + assert_eq!(peer, peer_b); + assert_eq!(rep, COST_VALIDATOR_INDEX_INVALID); + } else { + panic!("Received unexpected message type."); + } + }); + } } From 65e8edbae55f1117bc8c21a57698c51234351c78 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Wed, 22 Jul 2020 11:01:57 +0200 Subject: [PATCH 36/45] fix/review: more grumbles --- node/network/bitfield-distribution/src/lib.rs | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/node/network/bitfield-distribution/src/lib.rs b/node/network/bitfield-distribution/src/lib.rs index deb00b499a2e..20d870f8754b 100644 --- a/node/network/bitfield-distribution/src/lib.rs +++ b/node/network/bitfield-distribution/src/lib.rs @@ -198,7 +198,6 @@ impl BitfieldDistribution { } FromOverseer::Signal(OverseerSignal::Conclude) => { trace!(target: "bitd", "Conclude"); - tracker.per_relay_parent.clear(); return Ok(()); } } @@ -250,7 +249,7 @@ where let message_sent_to_peer = &mut (job_data.message_sent_to_peer); - // concurrently pass on the bitfield distribution to all interested peers + // pass on the bitfield distribution to all interested peers let interested_peers = peer_views .iter() .filter_map(|(peer, view)| { @@ -326,7 +325,7 @@ where let one_per_validator = &mut (job_data.one_per_validator); // only relay_message a message of a validator once if one_per_validator.get(&validator).is_some() { - trace!(target: "bitd", "Alrady received a message for validator at index {}", validator_index); + trace!(target: "bitd", "Already received a message for validator at index {}", validator_index); return Ok(()); } one_per_validator.insert(validator.clone(), message.clone()); @@ -457,11 +456,14 @@ where .or_default() .insert(validator.clone()); - let bytes = Encode::encode(&message); ctx.send_message(AllMessages::NetworkBridge( - NetworkBridgeMessage::SendMessage(vec![dest], BitfieldDistribution::PROTOCOL_ID, bytes), - )) - .await?; + NetworkBridgeMessage::SendMessage( + vec![dest], + BitfieldDistribution::PROTOCOL_ID, + message.encode() + ), + )).await?; + Ok(()) } From 087784a43ec61e20338073d9f130b0d5e58cf5c6 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Wed, 22 Jul 2020 12:20:39 +0200 Subject: [PATCH 37/45] fix/review: track incoming messages per peer --- node/network/bitfield-distribution/src/lib.rs | 74 ++++++++++++------- 1 file changed, 49 insertions(+), 25 deletions(-) diff --git a/node/network/bitfield-distribution/src/lib.rs b/node/network/bitfield-distribution/src/lib.rs index 20d870f8754b..91ed5f87f26d 100644 --- a/node/network/bitfield-distribution/src/lib.rs +++ b/node/network/bitfield-distribution/src/lib.rs @@ -89,8 +89,13 @@ struct PerRelayParentData { /// after bitfield gossips were already received. one_per_validator: HashMap, - /// which messages of which validators were already sent + /// Avoid duplicate message transmission to our peers. message_sent_to_peer: HashMap>, + + + /// Track messages that were already received by a peer + /// to prevent flooding. + message_received_from_peer: HashMap>, } impl PerRelayParentData { @@ -177,7 +182,6 @@ impl BitfieldDistribution { FromOverseer::Signal(OverseerSignal::StartWork(relay_parent)) => { trace!(target: "bitd", "Start {:?}", relay_parent); // query basic system parameters once - // @todo assumption: these cannot change within a session let (validator_set, signing_context) = query_basics(&mut ctx, relay_parent).await?; @@ -192,9 +196,7 @@ impl BitfieldDistribution { } FromOverseer::Signal(OverseerSignal::StopWork(relay_parent)) => { trace!(target: "bitd", "Stop {:?}", relay_parent); - // @todo assumption: it is good enough to prevent additional work from being - // scheduled, the individual futures are supposedly completed quickly - let _ = tracker.per_relay_parent.remove(&relay_parent); + // defer the cleanup to the view change } FromOverseer::Signal(OverseerSignal::Conclude) => { trace!(target: "bitd", "Conclude"); @@ -205,7 +207,7 @@ impl BitfieldDistribution { } } -/// Modify the reputation of peer based on their behaviour. +/// Modify the reputation of a peer based on its behaviour. async fn modify_reputation( ctx: &mut Context, peer: PeerId, @@ -304,11 +306,16 @@ where let validator_set = &job_data.validator_set; if validator_set.len() == 0 { + trace!( + target: "bitd", + "Validator set for relay parent {:?} is empty", + &message.relay_parent + ); return modify_reputation(ctx, origin, COST_MISSING_PEER_SESSION_KEY).await; } - // use the (untrusted) validator index provided by the signed payload - // and see if that one actually signed the availability bitset + // Use the (untrusted) validator index provided by the signed payload + // and see if that one actually signed the availability bitset. let signing_context = job_data.signing_context.clone(); let validator_index = message.signed_availability.validator_index() as usize; let validator = if let Some(validator) = validator_set.get(validator_index) { @@ -317,22 +324,36 @@ where return modify_reputation(ctx, origin, COST_VALIDATOR_INDEX_INVALID).await; }; + // Check if the peer already sent us a message for this validator earlier. + let received_set = job_data.message_received_from_peer.entry(origin.clone()).or_default(); + if !received_set.contains(&validator) { + received_set.insert(validator.clone()); + } else { + return modify_reputation(ctx, origin, COST_VALIDATOR_INDEX_INVALID).await; + }; + if message .signed_availability .check_signature(&signing_context, &validator) .is_ok() { let one_per_validator = &mut (job_data.one_per_validator); + // only relay_message a message of a validator once if one_per_validator.get(&validator).is_some() { - trace!(target: "bitd", "Already received a message for validator at index {}", validator_index); + trace!( + target: "bitd", + "Already received a message for validator at index {}", + validator_index + ); + modify_reputation(ctx, origin, GAIN_VALID_MESSAGE).await; return Ok(()); } one_per_validator.insert(validator.clone(), message.clone()); relay_message(ctx, job_data, &mut tracker.peer_views, validator, message).await; - modify_reputation(ctx, origin, GAIN_VALID_MESSAGE).await + modify_reputation(ctx, origin, GAIN_VALID_MESSAGE_FIRST).await } else { modify_reputation(ctx, origin, COST_SIGNATURE_INVALID).await } @@ -351,30 +372,38 @@ where match bridge_message { NetworkBridgeEvent::PeerConnected(peerid, _role) => { // insert if none already present - tracker.peer_views.entry(peerid).or_insert(View::default()); + tracker.peer_views.entry(peerid).or_default(); } NetworkBridgeEvent::PeerDisconnected(peerid) => { // get rid of superfluous data tracker.peer_views.remove(&peerid); } NetworkBridgeEvent::PeerViewChange(peerid, view) => { - catch_up_messages(ctx, tracker, peerid, view).await?; + handle_peer_view_change(ctx, tracker, peerid, view).await?; } NetworkBridgeEvent::OurViewChange(view) => { let old_view = std::mem::replace(&mut (tracker.view), view); - for new in tracker.view.difference(&old_view) { - if !tracker.per_relay_parent.contains_key(&new) { - warn!(target: "bitd", "Our view contains {} but the overseer never told use we should work on this", &new); + for added in tracker.view.difference(&old_view) { + if !tracker.per_relay_parent.contains_key(&added) { + warn!( + target: "bitd", + "Our view contains {} but the overseer never told use we should work on this", + &added + ); } } + for removed in old_view.difference(&tracker.view) { + // cleanup relay parents we are not interested in any more + let _ = tracker.per_relay_parent.remove(&removed); + } } NetworkBridgeEvent::PeerMessage(remote, bytes) => { if let Ok(gossiped_bitfield) = BitfieldGossipMessage::decode(&mut (bytes.as_slice())) { trace!(target: "bitd", "Received bitfield gossip from peer {:?}", &remote); process_incoming_peer_message(ctx, tracker, remote, gossiped_bitfield).await?; } else { - return modify_reputation(ctx, remote, COST_MESSAGE_NOT_DECODABLE).await; + modify_reputation(ctx, remote, COST_MESSAGE_NOT_DECODABLE).await?; } } } @@ -383,7 +412,7 @@ where // Send the difference between two views which were not sent // to that particular peer. -async fn catch_up_messages( +async fn handle_peer_view_change( ctx: &mut Context, tracker: &mut Tracker, origin: PeerId, @@ -402,7 +431,7 @@ where // Send all messages we've seen before and the peer is now interested // in to that peer. - let delta_set: HashMap = delta_vec + let delta_set: Vec<(ValidatorId, BitfieldGossipMessage)> = delta_vec .into_iter() .filter_map(|new_relay_parent_interest| { if let Some(job_data) = (&*tracker).per_relay_parent.get(&new_relay_parent_interest) { @@ -446,7 +475,6 @@ where let job_data = if let Some(job_data) = tracker.per_relay_parent.get_mut(&message.relay_parent) { job_data } else { - // @todo punishing here seems unreasonable return Ok(()); }; @@ -578,17 +606,12 @@ mod test { handle.send(input.into()); } - // @todo cannot clone or move `ctx` + // launch a complete subsystem instance and check responses on stimuli // // let completion = BitfieldDistribution::start(BitfieldDistribution, ctx) // .future // .timeout(Duration::from_millis(1000)) // .await; - - // while let Ok(rxd) = ctx.recv().await { - // // @todo impl expectation checks against a hashmap - // dbg!(rxd); - // } }); } @@ -611,6 +634,7 @@ mod test { one_per_validator: hashmap! { validator.clone() => known_message.clone(), }, + message_received_from_peer: hashmap! {}, message_sent_to_peer: hashmap! {}, }, }, From d91bcbda8ef29335e0723f8d98e556eeef060fb2 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Wed, 22 Jul 2020 12:39:04 +0200 Subject: [PATCH 38/45] fix/review: extract fn, avoid nested matches --- node/network/bitfield-distribution/src/lib.rs | 223 +++++++++++------- 1 file changed, 132 insertions(+), 91 deletions(-) diff --git a/node/network/bitfield-distribution/src/lib.rs b/node/network/bitfield-distribution/src/lib.rs index 91ed5f87f26d..3882784afe10 100644 --- a/node/network/bitfield-distribution/src/lib.rs +++ b/node/network/bitfield-distribution/src/lib.rs @@ -30,7 +30,7 @@ use polkadot_node_subsystem::messages::*; use polkadot_node_subsystem::{ FromOverseer, OverseerSignal, SpawnedSubsystem, Subsystem, SubsystemContext, SubsystemResult, }; -use polkadot_primitives::v1::{SigningContext, ValidatorId, Hash, SignedAvailabilityBitfield}; +use polkadot_primitives::v1::{Hash, SignedAvailabilityBitfield, SigningContext, ValidatorId}; use sc_network::ReputationChange; use std::collections::{HashMap, HashSet}; @@ -46,8 +46,7 @@ const COST_MESSAGE_NOT_DECODABLE: ReputationChange = ReputationChange::new(-100, "Not intersted in that parent hash"); const GAIN_VALID_MESSAGE_FIRST: ReputationChange = ReputationChange::new(15, "Valid message with new information"); -const GAIN_VALID_MESSAGE: ReputationChange = - ReputationChange::new(5, "Valid message"); +const GAIN_VALID_MESSAGE: ReputationChange = ReputationChange::new(5, "Valid message"); /// Checked signed availability bitfield that is distributed /// to other peers. @@ -92,7 +91,6 @@ struct PerRelayParentData { /// Avoid duplicate message transmission to our peers. message_sent_to_peer: HashMap>, - /// Track messages that were already received by a peer /// to prevent flooding. message_received_from_peer: HashMap>, @@ -140,43 +138,20 @@ impl BitfieldDistribution { loop { let message = ctx.recv().await?; match message { - FromOverseer::Communication { msg } => { - // another subsystem created this signed availability bitfield messages - match msg { - // relay_message a bitfield via gossip to other validators - BitfieldDistributionMessage::DistributeBitfield( - hash, - signed_availability, - ) => { - trace!(target: "bitd", "Processing DistributeBitfield"); - let job_data = &mut tracker.per_relay_parent.get_mut(&hash).expect("Overseer does not send work items related to relay parents that are not part of our workset. qed"); - - let validator = { - job_data - .validator_set - .get(signed_availability.validator_index() as usize) - .expect("Our own validation index exists. qed") - } - .clone(); - - let peer_views = &mut tracker.peer_views; - let msg = BitfieldGossipMessage { - relay_parent: hash, - signed_availability, - }; - - relay_message(&mut ctx, job_data, peer_views, validator, msg) - .await?; - } - BitfieldDistributionMessage::NetworkBridgeUpdate(event) => { - trace!(target: "bitd", "Processing NetworkMessage"); - // a network message was received - if let Err(e) = - handle_network_msg(&mut ctx, &mut tracker, event).await - { - warn!(target: "bitd", "Failed to handle incomming network messages: {:?}", e); - } - } + FromOverseer::Communication { + msg: BitfieldDistributionMessage::DistributeBitfield(hash, signed_availability), + } => { + trace!(target: "bitd", "Processing DistributeBitfield"); + handle_bitfield_distribution(&mut ctx, &mut tracker, hash, signed_availability) + .await?; + } + FromOverseer::Communication { + msg: BitfieldDistributionMessage::NetworkBridgeUpdate(event), + } => { + trace!(target: "bitd", "Processing NetworkMessage"); + // a network message was received + if let Err(e) = handle_network_msg(&mut ctx, &mut tracker, event).await { + warn!(target: "bitd", "Failed to handle incomming network messages: {:?}", e); } } FromOverseer::Signal(OverseerSignal::StartWork(relay_parent)) => { @@ -223,6 +198,52 @@ where .await } +/// Distribute a given valid and signature checked bitfield message. +/// +/// For this variant the source is this node. +async fn handle_bitfield_distribution( + ctx: &mut Context, + tracker: &mut Tracker, + relay_parent: Hash, + signed_availability: SignedAvailabilityBitfield, +) -> SubsystemResult<()> +where + Context: SubsystemContext, +{ + // Ignore anything the overseer did not tell this subsystem to work on + let mut job_data = tracker.per_relay_parent.get_mut(&relay_parent); + let job_data: &mut _ = if let Some(ref mut job_data) = job_data { + job_data + } else { + trace!( + target: "bitd", + "Not supposed to work on relay parent {} related data", + relay_parent + ); + + return Ok(()); + }; + + let validator_index = signed_availability.validator_index() as usize; + let validator_set = &job_data.validator_set; + let validator = if let Some(validator) = validator_set.get(validator_index) { + validator.clone() + } else { + trace!(target: "bitd", "Could not find a validator matching index {}", validator_index); + return Ok(()); + }; + + let peer_views = &mut tracker.peer_views; + let msg = BitfieldGossipMessage { + relay_parent, + signed_availability, + }; + + relay_message(ctx, job_data, peer_views, validator, msg).await?; + + Ok(()) +} + /// Distribute a given valid and signature checked bitfield message. /// /// Can be originated by another subsystem or received via network from another peer. @@ -236,18 +257,13 @@ async fn relay_message( where Context: SubsystemContext, { - // notify the overseer about a new and valid signed bitfield - ctx.send_message( - AllMessages::Provisioner( - ProvisionerMessage::ProvisionableData( - ProvisionableData::Bitfield( - message.relay_parent.clone(), - message.signed_availability.clone(), - ) - ) - ) - ).await; + ctx.send_message(AllMessages::Provisioner( + ProvisionerMessage::ProvisionableData(ProvisionableData::Bitfield( + message.relay_parent.clone(), + message.signed_availability.clone(), + )), + )).await; let message_sent_to_peer = &mut (job_data.message_sent_to_peer); @@ -324,8 +340,13 @@ where return modify_reputation(ctx, origin, COST_VALIDATOR_INDEX_INVALID).await; }; - // Check if the peer already sent us a message for this validator earlier. - let received_set = job_data.message_received_from_peer.entry(origin.clone()).or_default(); + // Check if the peer already sent us a message for the validator denoted in the message earlier. + // Must be done after validator index verification, in order to avoid storing an unbounded + // number of set entries. + let received_set = job_data + .message_received_from_peer + .entry(origin.clone()) + .or_default(); if !received_set.contains(&validator) { received_set.insert(validator.clone()); } else { @@ -485,12 +506,13 @@ where .insert(validator.clone()); ctx.send_message(AllMessages::NetworkBridge( - NetworkBridgeMessage::SendMessage( - vec![dest], - BitfieldDistribution::PROTOCOL_ID, - message.encode() - ), - )).await?; + NetworkBridgeMessage::SendMessage( + vec![dest], + BitfieldDistribution::PROTOCOL_ID, + message.encode(), + ), + )) + .await?; Ok(()) } @@ -539,12 +561,12 @@ mod test { use super::*; use bitvec::{bitvec, vec::BitVec}; use futures::executor; + use maplit::{hashmap, hashset}; use polkadot_primitives::v0::{Signed, ValidatorPair}; use polkadot_primitives::v1::AvailabilityBitfield; use smol_timeout::TimeoutExt; use sp_core::crypto::Pair; use std::time::Duration; - use maplit::{hashmap, hashset}; macro_rules! msg_sequence { ($( $input:expr ),+ $(,)? ) => [ @@ -693,15 +715,21 @@ mod test { ); executor::block_on(async move { - let _res = handle_network_msg(&mut ctx, &mut tracker, NetworkBridgeEvent::PeerMessage(peer_b.clone(), msg.encode())) - .timeout(Duration::from_millis(10)) - .await - .expect("10ms is more than enough for sending messages. qed") - .expect("There are no error values. qed"); + let _res = handle_network_msg( + &mut ctx, + &mut tracker, + NetworkBridgeEvent::PeerMessage(peer_b.clone(), msg.encode()), + ) + .timeout(Duration::from_millis(10)) + .await + .expect("10ms is more than enough for sending messages. qed") + .expect("There are no error values. qed"); // we should have a reputiation change due to invalid signature // @todo assess if Eq+PartialEq are viable to simplify this kind of code - if let AllMessages::NetworkBridge(NetworkBridgeMessage::ReportPeer(peer, rep)) = handle.recv().await { + if let AllMessages::NetworkBridge(NetworkBridgeMessage::ReportPeer(peer, rep)) = + handle.recv().await + { assert_eq!(peer, peer_b); assert_eq!(rep, COST_SIGNATURE_INVALID); } else { @@ -710,7 +738,6 @@ mod test { }); } - #[test] fn receive_invalid_validator_index() { let _ = env_logger::builder() @@ -755,15 +782,21 @@ mod test { ); executor::block_on(async move { - let _res = handle_network_msg(&mut ctx, &mut tracker, NetworkBridgeEvent::PeerMessage(peer_b.clone(), msg.encode())) - .timeout(Duration::from_millis(10)) - .await - .expect("10ms is more than enough for sending messages. qed") - .expect("There are no error values. qed"); + let _res = handle_network_msg( + &mut ctx, + &mut tracker, + NetworkBridgeEvent::PeerMessage(peer_b.clone(), msg.encode()), + ) + .timeout(Duration::from_millis(10)) + .await + .expect("10ms is more than enough for sending messages. qed") + .expect("There are no error values. qed"); // we should have a reputiation change due to invalid signature // @todo assess if Eq+PartialEq are viable to simplify this kind of code - if let AllMessages::NetworkBridge(NetworkBridgeMessage::ReportPeer(peer, rep)) = handle.recv().await { + if let AllMessages::NetworkBridge(NetworkBridgeMessage::ReportPeer(peer, rep)) = + handle.recv().await + { assert_eq!(peer, peer_b); assert_eq!(rep, COST_VALIDATOR_INDEX_INVALID); } else { @@ -772,10 +805,6 @@ mod test { }); } - - - - #[test] fn duplicate_message() { let _ = env_logger::builder() @@ -820,28 +849,40 @@ mod test { ); executor::block_on(async move { - let _res = handle_network_msg(&mut ctx, &mut tracker, NetworkBridgeEvent::PeerMessage(peer_b.clone(), msg.encode())) - .timeout(Duration::from_millis(10)) - .await - .expect("10ms is more than enough for sending messages. qed") - .expect("There are no error values. qed"); - - if let AllMessages::NetworkBridge(NetworkBridgeMessage::ReportPeer(peer, rep)) = handle.recv().await { + let _res = handle_network_msg( + &mut ctx, + &mut tracker, + NetworkBridgeEvent::PeerMessage(peer_b.clone(), msg.encode()), + ) + .timeout(Duration::from_millis(10)) + .await + .expect("10ms is more than enough for sending messages. qed") + .expect("There are no error values. qed"); + + if let AllMessages::NetworkBridge(NetworkBridgeMessage::ReportPeer(peer, rep)) = + handle.recv().await + { assert_eq!(peer, peer_b); assert_eq!(rep, COST_VALIDATOR_INDEX_INVALID); } else { panic!("Received unexpected message type."); } - let _res = handle_network_msg(&mut ctx, &mut tracker, NetworkBridgeEvent::PeerMessage(peer_b.clone(), msg.encode())) - .timeout(Duration::from_millis(10)) - .await - .expect("10ms is more than enough for sending messages. qed") - .expect("There are no error values. qed"); + let _res = handle_network_msg( + &mut ctx, + &mut tracker, + NetworkBridgeEvent::PeerMessage(peer_b.clone(), msg.encode()), + ) + .timeout(Duration::from_millis(10)) + .await + .expect("10ms is more than enough for sending messages. qed") + .expect("There are no error values. qed"); // we should have a reputiation change due to invalid signature // @todo assess if Eq+PartialEq are viable to simplify this kind of code - if let AllMessages::NetworkBridge(NetworkBridgeMessage::ReportPeer(peer, rep)) = handle.recv().await { + if let AllMessages::NetworkBridge(NetworkBridgeMessage::ReportPeer(peer, rep)) = + handle.recv().await + { assert_eq!(peer, peer_b); assert_eq!(rep, COST_VALIDATOR_INDEX_INVALID); } else { From c957bf97862e1071215d3b22611008cd6e4b1b5a Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Wed, 22 Jul 2020 15:16:57 +0200 Subject: [PATCH 39/45] fix/review: more tests, simplify test --- Cargo.lock | 1 + node/network/bitfield-distribution/Cargo.toml | 7 +- node/network/bitfield-distribution/src/lib.rs | 252 +++++++++++------- 3 files changed, 160 insertions(+), 100 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index fad622fabea0..3e03d0d068e7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4390,6 +4390,7 @@ dependencies = [ name = "polkadot-availability-bitfield-distribution" version = "0.1.0" dependencies = [ + "assert_matches", "bitvec", "env_logger", "futures 0.3.5", diff --git a/node/network/bitfield-distribution/Cargo.toml b/node/network/bitfield-distribution/Cargo.toml index 064f9da4538d..57aba77e4e29 100644 --- a/node/network/bitfield-distribution/Cargo.toml +++ b/node/network/bitfield-distribution/Cargo.toml @@ -23,6 +23,7 @@ sp-core = { git = "https://github.com/paritytech/substrate", branch = "master" } subsystem-test = { package = "polkadot-subsystem-test-helpers", path = "../../test-helpers/subsystem" } parking_lot = "0.10.0" maplit = "1.0.2" -smol = "0.2" -smol-timeout = "0.1" -env_logger = "0.7" \ No newline at end of file +smol = "0.2.0" +smol-timeout = "0.1.0" +env_logger = "0.7.1" +assert_matches = "1.3.0" \ No newline at end of file diff --git a/node/network/bitfield-distribution/src/lib.rs b/node/network/bitfield-distribution/src/lib.rs index 3882784afe10..4ae42f7f4a10 100644 --- a/node/network/bitfield-distribution/src/lib.rs +++ b/node/network/bitfield-distribution/src/lib.rs @@ -41,12 +41,15 @@ const COST_VALIDATOR_INDEX_INVALID: ReputationChange = const COST_MISSING_PEER_SESSION_KEY: ReputationChange = ReputationChange::new(-133, "Missing peer session key"); const COST_NOT_INTERESTED: ReputationChange = - ReputationChange::new(-51, "Not intersted in that parent hash"); + ReputationChange::new(-51, "Not interested in that parent hash"); const COST_MESSAGE_NOT_DECODABLE: ReputationChange = - ReputationChange::new(-100, "Not intersted in that parent hash"); + ReputationChange::new(-100, "Not interested in that parent hash"); +const COST_PEER_DUPLICATE_MESSAGE: ReputationChange = + ReputationChange::new(-500, "Peer sent the same message multiple times"); const GAIN_VALID_MESSAGE_FIRST: ReputationChange = ReputationChange::new(15, "Valid message with new information"); -const GAIN_VALID_MESSAGE: ReputationChange = ReputationChange::new(5, "Valid message"); +const GAIN_VALID_MESSAGE: ReputationChange = + ReputationChange::new(10, "Valid message"); /// Checked signed availability bitfield that is distributed /// to other peers. @@ -259,11 +262,12 @@ where { // notify the overseer about a new and valid signed bitfield ctx.send_message(AllMessages::Provisioner( - ProvisionerMessage::ProvisionableData(ProvisionableData::Bitfield( - message.relay_parent.clone(), - message.signed_availability.clone(), - )), - )).await; + ProvisionerMessage::ProvisionableData(ProvisionableData::Bitfield( + message.relay_parent.clone(), + message.signed_availability.clone(), + )), + )) + .await; let message_sent_to_peer = &mut (job_data.message_sent_to_peer); @@ -286,14 +290,22 @@ where }) .collect::>(); - ctx.send_message(AllMessages::NetworkBridge( - NetworkBridgeMessage::SendMessage( - interested_peers, - BitfieldDistribution::PROTOCOL_ID, - Encode::encode(&message), - ), - )) - .await?; + if interested_peers.is_empty() { + trace!( + target: "bitd", + "No peers are interested in gossip for relay parent {:?}", + message.relay_parent + ); + } else { + ctx.send_message(AllMessages::NetworkBridge( + NetworkBridgeMessage::SendMessage( + interested_peers, + BitfieldDistribution::PROTOCOL_ID, + Encode::encode(&message), + ), + )) + .await?; + } Ok(()) } @@ -321,7 +333,7 @@ where }; let validator_set = &job_data.validator_set; - if validator_set.len() == 0 { + if validator_set.is_empty() { trace!( target: "bitd", "Validator set for relay parent {:?} is empty", @@ -347,10 +359,11 @@ where .message_received_from_peer .entry(origin.clone()) .or_default(); + if !received_set.contains(&validator) { received_set.insert(validator.clone()); } else { - return modify_reputation(ctx, origin, COST_VALIDATOR_INDEX_INVALID).await; + return modify_reputation(ctx, origin, COST_PEER_DUPLICATE_MESSAGE).await; }; if message @@ -567,6 +580,7 @@ mod test { use smol_timeout::TimeoutExt; use sp_core::crypto::Pair; use std::time::Duration; + use assert_matches::assert_matches; macro_rules! msg_sequence { ($( $input:expr ),+ $(,)? ) => [ @@ -637,6 +651,17 @@ mod test { }); } + + macro_rules! launch { + ($fut:expr) => { + $fut + .timeout(Duration::from_millis(10)) + .await + .expect("10ms is more than enough for sending messages.") + .expect("Error values should really never occur.") + }; + } + /// A very limited tracker, only interested in the relay parent of the /// given message, which must be signed by `validator` and a set of peers /// which are also only interested in that relay parent. @@ -668,6 +693,31 @@ mod test { } } + fn tracker_with_view(view: View) -> (Tracker, ValidatorPair) { + let mut tracker = Tracker::default(); + + let (validator_pair, _seed) = ValidatorPair::generate(); + let validator = validator_pair.public(); + + tracker.per_relay_parent = view.0.iter().map(|relay_parent| {( + relay_parent.clone(), + PerRelayParentData { + signing_context: SigningContext { + session_index: 1, + parent_hash: relay_parent.clone(), + }, + validator_set: vec![validator.clone()], + one_per_validator: hashmap! {}, + message_received_from_peer: hashmap! {}, + message_sent_to_peer: hashmap!{}, + }) + }).collect(); + + tracker.view = view; + + (tracker, validator_pair) + } + #[test] fn receive_invalid_signature() { let _ = env_logger::builder() @@ -715,26 +765,22 @@ mod test { ); executor::block_on(async move { - let _res = handle_network_msg( + launch!(handle_network_msg( &mut ctx, &mut tracker, NetworkBridgeEvent::PeerMessage(peer_b.clone(), msg.encode()), - ) - .timeout(Duration::from_millis(10)) - .await - .expect("10ms is more than enough for sending messages. qed") - .expect("There are no error values. qed"); - - // we should have a reputiation change due to invalid signature - // @todo assess if Eq+PartialEq are viable to simplify this kind of code - if let AllMessages::NetworkBridge(NetworkBridgeMessage::ReportPeer(peer, rep)) = - handle.recv().await - { - assert_eq!(peer, peer_b); - assert_eq!(rep, COST_SIGNATURE_INVALID); - } else { - panic!("Received unexpected message type."); - } + )); + + // we should have a reputation change due to invalid validator index + assert_matches!( + handle.recv().await, + AllMessages::NetworkBridge( + NetworkBridgeMessage::ReportPeer(peer, rep) + ) => { + assert_eq!(peer, peer_b); + assert_eq!(rep, COST_SIGNATURE_INVALID) + } + ); }); } @@ -782,31 +828,27 @@ mod test { ); executor::block_on(async move { - let _res = handle_network_msg( + launch!(handle_network_msg( &mut ctx, &mut tracker, NetworkBridgeEvent::PeerMessage(peer_b.clone(), msg.encode()), - ) - .timeout(Duration::from_millis(10)) - .await - .expect("10ms is more than enough for sending messages. qed") - .expect("There are no error values. qed"); - - // we should have a reputiation change due to invalid signature - // @todo assess if Eq+PartialEq are viable to simplify this kind of code - if let AllMessages::NetworkBridge(NetworkBridgeMessage::ReportPeer(peer, rep)) = - handle.recv().await - { - assert_eq!(peer, peer_b); - assert_eq!(rep, COST_VALIDATOR_INDEX_INVALID); - } else { - panic!("Received unexpected message type."); - } + )); + + // we should have a reputation change due to invalid validator index + assert_matches!( + handle.recv().await, + AllMessages::NetworkBridge( + NetworkBridgeMessage::ReportPeer(peer, rep) + ) => { + assert_eq!(peer, peer_b); + assert_eq!(rep, COST_VALIDATOR_INDEX_INVALID) + } + ); }); } #[test] - fn duplicate_message() { + fn receive_duplicate_messages() { let _ = env_logger::builder() .filter(None, log::LevelFilter::Trace) .is_test(true) @@ -825,69 +867,85 @@ mod test { }; // validator 0 key pair - let (validator_pair, _seed) = ValidatorPair::generate(); - let validator = validator_pair.public(); + let (mut tracker, validator_pair) = tracker_with_view(view![hash_a, hash_b]); let payload = AvailabilityBitfield(bitvec![bitvec::order::Lsb0, u8; 1u8; 32]); - let signed = - Signed::::sign(payload, &signing_context, 42, &validator_pair); + let signed_bitfield = + Signed::::sign(payload, &signing_context, 0, &validator_pair); let msg = BitfieldGossipMessage { relay_parent: hash_a.clone(), - signed_availability: signed.clone(), + signed_availability: signed_bitfield.clone(), }; let pool = sp_core::testing::SpawnBlockingExecutor::new(); let (mut ctx, mut handle) = subsystem_test::make_subsystem_context::(pool); - let mut tracker = prewarmed_tracker( - validator.clone(), - signing_context.clone(), - msg.clone(), - vec![peer_b.clone()], - ); - executor::block_on(async move { - let _res = handle_network_msg( + // send a first message + launch!(handle_network_msg( &mut ctx, &mut tracker, NetworkBridgeEvent::PeerMessage(peer_b.clone(), msg.encode()), - ) - .timeout(Duration::from_millis(10)) - .await - .expect("10ms is more than enough for sending messages. qed") - .expect("There are no error values. qed"); - - if let AllMessages::NetworkBridge(NetworkBridgeMessage::ReportPeer(peer, rep)) = - handle.recv().await - { - assert_eq!(peer, peer_b); - assert_eq!(rep, COST_VALIDATOR_INDEX_INVALID); - } else { - panic!("Received unexpected message type."); - } + )); + + // none of our peers has any interest in any messages + // so we do not receive a network send type message here + assert_matches!( + handle.recv().await, + AllMessages::Provisioner(ProvisionerMessage::ProvisionableData( + ProvisionableData::Bitfield(hash, signed) + )) => { + assert_eq!(hash, hash_a); + assert_eq!(signed, signed_bitfield) + } + ); + + assert_matches!( + handle.recv().await, + AllMessages::NetworkBridge( + NetworkBridgeMessage::ReportPeer(peer, rep) + ) => { + assert_eq!(peer, peer_b); + assert_eq!(rep, GAIN_VALID_MESSAGE_FIRST) + } + ); - let _res = handle_network_msg( + // let peer A send the same message again + launch!(handle_network_msg( + &mut ctx, + &mut tracker, + NetworkBridgeEvent::PeerMessage(peer_a.clone(), msg.encode()), + )); + + assert_matches!( + handle.recv().await, + AllMessages::NetworkBridge( + NetworkBridgeMessage::ReportPeer(peer, rep) + ) => { + assert_eq!(peer, peer_a); + assert_eq!(rep, GAIN_VALID_MESSAGE) + } + ); + + // let peer B send the initial message again + launch!(handle_network_msg( &mut ctx, &mut tracker, NetworkBridgeEvent::PeerMessage(peer_b.clone(), msg.encode()), - ) - .timeout(Duration::from_millis(10)) - .await - .expect("10ms is more than enough for sending messages. qed") - .expect("There are no error values. qed"); - - // we should have a reputiation change due to invalid signature - // @todo assess if Eq+PartialEq are viable to simplify this kind of code - if let AllMessages::NetworkBridge(NetworkBridgeMessage::ReportPeer(peer, rep)) = - handle.recv().await - { - assert_eq!(peer, peer_b); - assert_eq!(rep, COST_VALIDATOR_INDEX_INVALID); - } else { - panic!("Received unexpected message type."); - } + )); + + assert_matches!( + handle.recv().await, + AllMessages::NetworkBridge( + NetworkBridgeMessage::ReportPeer(peer, rep) + ) => { + assert_eq!(peer, peer_b); + assert_eq!(rep, COST_PEER_DUPLICATE_MESSAGE) + } + ); + }); } } From 36a667286ba38c8674193c6b3a610ea8fba4e9c3 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Wed, 22 Jul 2020 16:12:16 +0200 Subject: [PATCH 40/45] fix/review: extend tests to cover more cases --- node/network/bitfield-distribution/src/lib.rs | 249 ++++++++++++++---- 1 file changed, 198 insertions(+), 51 deletions(-) diff --git a/node/network/bitfield-distribution/src/lib.rs b/node/network/bitfield-distribution/src/lib.rs index 4ae42f7f4a10..dc9bb0d4b66d 100644 --- a/node/network/bitfield-distribution/src/lib.rs +++ b/node/network/bitfield-distribution/src/lib.rs @@ -40,7 +40,7 @@ const COST_VALIDATOR_INDEX_INVALID: ReputationChange = ReputationChange::new(-100, "Bitfield validator index invalid"); const COST_MISSING_PEER_SESSION_KEY: ReputationChange = ReputationChange::new(-133, "Missing peer session key"); -const COST_NOT_INTERESTED: ReputationChange = +const COST_NOT_IN_VIEW: ReputationChange = ReputationChange::new(-51, "Not interested in that parent hash"); const COST_MESSAGE_NOT_DECODABLE: ReputationChange = ReputationChange::new(-100, "Not interested in that parent hash"); @@ -301,7 +301,7 @@ where NetworkBridgeMessage::SendMessage( interested_peers, BitfieldDistribution::PROTOCOL_ID, - Encode::encode(&message), + message.encode(), ), )) .await?; @@ -321,7 +321,7 @@ where { // we don't care about this, not part of our view if !tracker.view.contains(&message.relay_parent) { - return modify_reputation(ctx, origin, COST_NOT_INTERESTED).await; + return modify_reputation(ctx, origin, COST_NOT_IN_VIEW).await; } // Ignore anything the overseer did not tell this subsystem to work on @@ -329,7 +329,7 @@ where let job_data: &mut _ = if let Some(ref mut job_data) = job_data { job_data } else { - return modify_reputation(ctx, origin, COST_NOT_INTERESTED).await; + return modify_reputation(ctx, origin, COST_NOT_IN_VIEW).await; }; let validator_set = &job_data.validator_set; @@ -392,7 +392,6 @@ where modify_reputation(ctx, origin, COST_SIGNATURE_INVALID).await } } - /// Deal with network bridge updates and track what needs to be tracked /// which depends on the message type received. async fn handle_network_msg( @@ -416,21 +415,7 @@ where handle_peer_view_change(ctx, tracker, peerid, view).await?; } NetworkBridgeEvent::OurViewChange(view) => { - let old_view = std::mem::replace(&mut (tracker.view), view); - - for added in tracker.view.difference(&old_view) { - if !tracker.per_relay_parent.contains_key(&added) { - warn!( - target: "bitd", - "Our view contains {} but the overseer never told use we should work on this", - &added - ); - } - } - for removed in old_view.difference(&tracker.view) { - // cleanup relay parents we are not interested in any more - let _ = tracker.per_relay_parent.remove(&removed); - } + handle_our_view_change(tracker, view)?; } NetworkBridgeEvent::PeerMessage(remote, bytes) => { if let Ok(gossiped_bitfield) = BitfieldGossipMessage::decode(&mut (bytes.as_slice())) { @@ -444,6 +429,27 @@ where Ok(()) } +/// Handle the changes necassary when our view changes. +fn handle_our_view_change(tracker: &mut Tracker, view: View) -> SubsystemResult<()> { + let old_view = std::mem::replace(&mut (tracker.view), view); + + for added in tracker.view.difference(&old_view) { + if !tracker.per_relay_parent.contains_key(&added) { + warn!( + target: "bitd", + "Our view contains {} but the overseer never told use we should work on this", + &added + ); + } + } + for removed in old_view.difference(&tracker.view) { + // cleanup relay parents we are not interested in any more + let _ = tracker.per_relay_parent.remove(&removed); + } + Ok(()) +} + + // Send the difference between two views which were not sent // to that particular peer. async fn handle_peer_view_change( @@ -583,14 +589,20 @@ mod test { use assert_matches::assert_matches; macro_rules! msg_sequence { - ($( $input:expr ),+ $(,)? ) => [ - vec![ $( FromOverseer::Communication { msg: $input } ),+ ] + ($( $input:expr ),* $(,)? ) => [ + vec![ $( FromOverseer::Communication { msg: $input } ),* ] ]; } macro_rules! view { - ( $( $hash:expr ),+ $(,)? ) => [ - View(vec![ $( $hash.clone() ),+ ]) + ( $( $hash:expr ),* $(,)? ) => [ + View(vec![ $( $hash.clone() ),* ]) + ]; + } + + macro_rules! peers { + ( $( $peer:expr ),* $(,)? ) => [ + vec![ $( $peer.clone() ),* ] ]; } @@ -693,19 +705,21 @@ mod test { } } - fn tracker_with_view(view: View) -> (Tracker, ValidatorPair) { + fn tracker_with_view(view: View, relay_parent: Hash) -> (Tracker, SigningContext, ValidatorPair) { let mut tracker = Tracker::default(); let (validator_pair, _seed) = ValidatorPair::generate(); let validator = validator_pair.public(); + let signing_context = SigningContext { + session_index: 1, + parent_hash: relay_parent.clone(), + }; + tracker.per_relay_parent = view.0.iter().map(|relay_parent| {( relay_parent.clone(), PerRelayParentData { - signing_context: SigningContext { - session_index: 1, - parent_hash: relay_parent.clone(), - }, + signing_context: signing_context.clone(), validator_set: vec![validator.clone()], one_per_validator: hashmap! {}, message_received_from_peer: hashmap! {}, @@ -715,7 +729,7 @@ mod test { tracker.view = view; - (tracker, validator_pair) + (tracker, signing_context, validator_pair) } #[test] @@ -798,14 +812,9 @@ mod test { let peer_b = PeerId::random(); assert_ne!(peer_a, peer_b); - let signing_context = SigningContext { - session_index: 1, - parent_hash: hash_a.clone(), - }; - // validator 0 key pair - let (validator_pair, _seed) = ValidatorPair::generate(); - let validator = validator_pair.public(); + let (mut tracker, signing_context, validator_pair) = tracker_with_view(view![hash_a, hash_b], hash_a.clone()); + tracker.peer_views.insert(peer_b.clone(), view![hash_a]); let payload = AvailabilityBitfield(bitvec![bitvec::order::Lsb0, u8; 1u8; 32]); let signed = @@ -820,13 +829,6 @@ mod test { let (mut ctx, mut handle) = subsystem_test::make_subsystem_context::(pool); - let mut tracker = prewarmed_tracker( - validator.clone(), - signing_context.clone(), - msg.clone(), - vec![peer_b.clone()], - ); - executor::block_on(async move { launch!(handle_network_msg( &mut ctx, @@ -855,20 +857,16 @@ mod test { .try_init(); let hash_a: Hash = [0; 32].into(); - let hash_b: Hash = [1; 32].into(); // other + let hash_b: Hash = [1; 32].into(); let peer_a = PeerId::random(); let peer_b = PeerId::random(); assert_ne!(peer_a, peer_b); - let signing_context = SigningContext { - session_index: 1, - parent_hash: hash_a.clone(), - }; - // validator 0 key pair - let (mut tracker, validator_pair) = tracker_with_view(view![hash_a, hash_b]); + let (mut tracker, signing_context, validator_pair) = tracker_with_view(view![hash_a, hash_b], hash_a.clone()); + // create a signed message by validator 0 let payload = AvailabilityBitfield(bitvec![bitvec::order::Lsb0, u8; 1u8; 32]); let signed_bitfield = Signed::::sign(payload, &signing_context, 0, &validator_pair); @@ -892,6 +890,7 @@ mod test { // none of our peers has any interest in any messages // so we do not receive a network send type message here + // but only the one for the next subsystem assert_matches!( handle.recv().await, AllMessages::Provisioner(ProvisionerMessage::ProvisionableData( @@ -945,6 +944,154 @@ mod test { assert_eq!(rep, COST_PEER_DUPLICATE_MESSAGE) } ); + }); + } + #[test] + fn change_view_and_then_recv() { + let _ = env_logger::builder() + .filter(None, log::LevelFilter::Trace) + .is_test(true) + .try_init(); + + let hash_a: Hash = [0; 32].into(); + let hash_b: Hash = [1; 32].into(); + + let peer_a = PeerId::random(); + let peer_b = PeerId::random(); + assert_ne!(peer_a, peer_b); + + // validator 0 key pair + let (mut tracker, signing_context, validator_pair) = tracker_with_view(view![hash_a, hash_b], hash_a.clone()); + + // create a signed message by validator 0 + let payload = AvailabilityBitfield(bitvec![bitvec::order::Lsb0, u8; 1u8; 32]); + let signed_bitfield = + Signed::::sign(payload, &signing_context, 0, &validator_pair); + + let msg = BitfieldGossipMessage { + relay_parent: hash_a.clone(), + signed_availability: signed_bitfield.clone(), + }; + + let pool = sp_core::testing::SpawnBlockingExecutor::new(); + let (mut ctx, mut handle) = + subsystem_test::make_subsystem_context::(pool); + + executor::block_on(async move { + launch!(handle_network_msg( + &mut ctx, + &mut tracker, + NetworkBridgeEvent::PeerConnected(peer_b.clone(), ObservedRole::Full), + )); + + // make peer b interested + launch!(handle_network_msg( + &mut ctx, + &mut tracker, + NetworkBridgeEvent::PeerViewChange(peer_b.clone(), view![hash_a, hash_b]), + )); + + assert!(tracker.peer_views.contains_key(&peer_b)); + + // recv a first message from the network + launch!(handle_network_msg( + &mut ctx, + &mut tracker, + NetworkBridgeEvent::PeerMessage(peer_b.clone(), msg.encode()), + )); + + // gossip to the overseer + assert_matches!( + handle.recv().await, + AllMessages::Provisioner(ProvisionerMessage::ProvisionableData( + ProvisionableData::Bitfield(hash, signed) + )) => { + assert_eq!(hash, hash_a); + assert_eq!(signed, signed_bitfield) + } + ); + + // gossip to the network + assert_matches!( + handle.recv().await, + AllMessages::NetworkBridge(NetworkBridgeMessage::SendMessage ( + peers, proto, bytes + )) => { + assert_eq!(peers, peers![peer_b]); + assert_eq!(proto, BitfieldDistribution::PROTOCOL_ID); + assert_eq!(bytes, msg.encode()); + } + ); + + // reputation change for peer B + assert_matches!( + handle.recv().await, + AllMessages::NetworkBridge( + NetworkBridgeMessage::ReportPeer(peer, rep) + ) => { + assert_eq!(peer, peer_b); + assert_eq!(rep, GAIN_VALID_MESSAGE_FIRST) + } + ); + + launch!(handle_network_msg( + &mut ctx, + &mut tracker, + NetworkBridgeEvent::PeerViewChange(peer_b.clone(), view![]), + )); + + assert!(tracker.peer_views.contains_key(&peer_b)); + assert_eq!( + tracker.peer_views.get(&peer_b).expect("Must contain value for peer B"), + &view![] + ); + + // on rx of the same message, since we are not interested, + // should give penalty + launch!(handle_network_msg( + &mut ctx, + &mut tracker, + NetworkBridgeEvent::PeerMessage(peer_b.clone(), msg.encode()), + )); + + // reputation change for peer B + assert_matches!( + handle.recv().await, + AllMessages::NetworkBridge( + NetworkBridgeMessage::ReportPeer(peer, rep) + ) => { + assert_eq!(peer, peer_b); + assert_eq!(rep, COST_PEER_DUPLICATE_MESSAGE) + } + ); + + launch!(handle_network_msg( + &mut ctx, + &mut tracker, + NetworkBridgeEvent::PeerDisconnected(peer_b.clone()), + )); + + // we are not interested in any peers at all anymore + tracker.view = view![]; + + // on rx of the same message, since we are not interested, + // should give penalty + launch!(handle_network_msg( + &mut ctx, + &mut tracker, + NetworkBridgeEvent::PeerMessage(peer_a.clone(), msg.encode()), + )); + + // reputation change for peer B + assert_matches!( + handle.recv().await, + AllMessages::NetworkBridge( + NetworkBridgeMessage::ReportPeer(peer, rep) + ) => { + assert_eq!(peer, peer_a); + assert_eq!(rep, COST_NOT_IN_VIEW) + } + ); }); } From 5b7c963398c8ab7fde9d209c23b9e1ff492f762f Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Wed, 22 Jul 2020 16:14:39 +0200 Subject: [PATCH 41/45] chore/rename: Tracker -> ProtocolState --- node/network/bitfield-distribution/src/lib.rs | 122 +++++++++--------- 1 file changed, 61 insertions(+), 61 deletions(-) diff --git a/node/network/bitfield-distribution/src/lib.rs b/node/network/bitfield-distribution/src/lib.rs index dc9bb0d4b66d..1791d46eaa6d 100644 --- a/node/network/bitfield-distribution/src/lib.rs +++ b/node/network/bitfield-distribution/src/lib.rs @@ -64,7 +64,7 @@ pub struct BitfieldGossipMessage { /// Data used to track information of peers and relay parents the /// overseer ordered us to work on. #[derive(Default, Clone)] -struct Tracker { +struct ProtocolState { /// track all active peers and their views /// to determine what is relevant to them. peer_views: HashMap, @@ -137,7 +137,7 @@ impl BitfieldDistribution { .await?; // work: process incoming messages from the overseer and process accordingly. - let mut tracker = Tracker::default(); + let mut state = ProtocolState::default(); loop { let message = ctx.recv().await?; match message { @@ -145,7 +145,7 @@ impl BitfieldDistribution { msg: BitfieldDistributionMessage::DistributeBitfield(hash, signed_availability), } => { trace!(target: "bitd", "Processing DistributeBitfield"); - handle_bitfield_distribution(&mut ctx, &mut tracker, hash, signed_availability) + handle_bitfield_distribution(&mut ctx, &mut state, hash, signed_availability) .await?; } FromOverseer::Communication { @@ -153,7 +153,7 @@ impl BitfieldDistribution { } => { trace!(target: "bitd", "Processing NetworkMessage"); // a network message was received - if let Err(e) = handle_network_msg(&mut ctx, &mut tracker, event).await { + if let Err(e) = handle_network_msg(&mut ctx, &mut state, event).await { warn!(target: "bitd", "Failed to handle incomming network messages: {:?}", e); } } @@ -163,7 +163,7 @@ impl BitfieldDistribution { let (validator_set, signing_context) = query_basics(&mut ctx, relay_parent).await?; - let _ = tracker.per_relay_parent.insert( + let _ = state.per_relay_parent.insert( relay_parent, PerRelayParentData { signing_context, @@ -206,7 +206,7 @@ where /// For this variant the source is this node. async fn handle_bitfield_distribution( ctx: &mut Context, - tracker: &mut Tracker, + state: &mut ProtocolState, relay_parent: Hash, signed_availability: SignedAvailabilityBitfield, ) -> SubsystemResult<()> @@ -214,7 +214,7 @@ where Context: SubsystemContext, { // Ignore anything the overseer did not tell this subsystem to work on - let mut job_data = tracker.per_relay_parent.get_mut(&relay_parent); + let mut job_data = state.per_relay_parent.get_mut(&relay_parent); let job_data: &mut _ = if let Some(ref mut job_data) = job_data { job_data } else { @@ -236,7 +236,7 @@ where return Ok(()); }; - let peer_views = &mut tracker.peer_views; + let peer_views = &mut state.peer_views; let msg = BitfieldGossipMessage { relay_parent, signed_availability, @@ -312,7 +312,7 @@ where /// Handle an incoming message from a peer. async fn process_incoming_peer_message( ctx: &mut Context, - tracker: &mut Tracker, + state: &mut ProtocolState, origin: PeerId, message: BitfieldGossipMessage, ) -> SubsystemResult<()> @@ -320,12 +320,12 @@ where Context: SubsystemContext, { // we don't care about this, not part of our view - if !tracker.view.contains(&message.relay_parent) { + if !state.view.contains(&message.relay_parent) { return modify_reputation(ctx, origin, COST_NOT_IN_VIEW).await; } // Ignore anything the overseer did not tell this subsystem to work on - let mut job_data = tracker.per_relay_parent.get_mut(&message.relay_parent); + let mut job_data = state.per_relay_parent.get_mut(&message.relay_parent); let job_data: &mut _ = if let Some(ref mut job_data) = job_data { job_data } else { @@ -385,7 +385,7 @@ where } one_per_validator.insert(validator.clone(), message.clone()); - relay_message(ctx, job_data, &mut tracker.peer_views, validator, message).await; + relay_message(ctx, job_data, &mut state.peer_views, validator, message).await; modify_reputation(ctx, origin, GAIN_VALID_MESSAGE_FIRST).await } else { @@ -396,7 +396,7 @@ where /// which depends on the message type received. async fn handle_network_msg( ctx: &mut Context, - tracker: &mut Tracker, + state: &mut ProtocolState, bridge_message: NetworkBridgeEvent, ) -> SubsystemResult<()> where @@ -405,22 +405,22 @@ where match bridge_message { NetworkBridgeEvent::PeerConnected(peerid, _role) => { // insert if none already present - tracker.peer_views.entry(peerid).or_default(); + state.peer_views.entry(peerid).or_default(); } NetworkBridgeEvent::PeerDisconnected(peerid) => { // get rid of superfluous data - tracker.peer_views.remove(&peerid); + state.peer_views.remove(&peerid); } NetworkBridgeEvent::PeerViewChange(peerid, view) => { - handle_peer_view_change(ctx, tracker, peerid, view).await?; + handle_peer_view_change(ctx, state, peerid, view).await?; } NetworkBridgeEvent::OurViewChange(view) => { - handle_our_view_change(tracker, view)?; + handle_our_view_change(state, view)?; } NetworkBridgeEvent::PeerMessage(remote, bytes) => { if let Ok(gossiped_bitfield) = BitfieldGossipMessage::decode(&mut (bytes.as_slice())) { trace!(target: "bitd", "Received bitfield gossip from peer {:?}", &remote); - process_incoming_peer_message(ctx, tracker, remote, gossiped_bitfield).await?; + process_incoming_peer_message(ctx, state, remote, gossiped_bitfield).await?; } else { modify_reputation(ctx, remote, COST_MESSAGE_NOT_DECODABLE).await?; } @@ -430,11 +430,11 @@ where } /// Handle the changes necassary when our view changes. -fn handle_our_view_change(tracker: &mut Tracker, view: View) -> SubsystemResult<()> { - let old_view = std::mem::replace(&mut (tracker.view), view); +fn handle_our_view_change(state: &mut ProtocolState, view: View) -> SubsystemResult<()> { + let old_view = std::mem::replace(&mut (state.view), view); - for added in tracker.view.difference(&old_view) { - if !tracker.per_relay_parent.contains_key(&added) { + for added in state.view.difference(&old_view) { + if !state.per_relay_parent.contains_key(&added) { warn!( target: "bitd", "Our view contains {} but the overseer never told use we should work on this", @@ -442,9 +442,9 @@ fn handle_our_view_change(tracker: &mut Tracker, view: View) -> SubsystemResult< ); } } - for removed in old_view.difference(&tracker.view) { + for removed in old_view.difference(&state.view) { // cleanup relay parents we are not interested in any more - let _ = tracker.per_relay_parent.remove(&removed); + let _ = state.per_relay_parent.remove(&removed); } Ok(()) } @@ -454,7 +454,7 @@ fn handle_our_view_change(tracker: &mut Tracker, view: View) -> SubsystemResult< // to that particular peer. async fn handle_peer_view_change( ctx: &mut Context, - tracker: &mut Tracker, + state: &mut ProtocolState, origin: PeerId, view: View, ) -> SubsystemResult<()> @@ -462,7 +462,7 @@ where Context: SubsystemContext, { use std::collections::hash_map::Entry; - let current = tracker.peer_views.entry(origin.clone()).or_default(); + let current = state.peer_views.entry(origin.clone()).or_default(); let delta_vec: Vec = (*current).difference(&view).cloned().collect(); @@ -474,7 +474,7 @@ where let delta_set: Vec<(ValidatorId, BitfieldGossipMessage)> = delta_vec .into_iter() .filter_map(|new_relay_parent_interest| { - if let Some(job_data) = (&*tracker).per_relay_parent.get(&new_relay_parent_interest) { + if let Some(job_data) = (&*state).per_relay_parent.get(&new_relay_parent_interest) { // send all messages let one_per_validator = job_data.one_per_validator.clone(); let origin = origin.clone(); @@ -495,7 +495,7 @@ where .collect(); for (validator, message) in delta_set.into_iter() { - send_tracked_gossip_message(ctx, tracker, origin.clone(), validator, message).await?; + send_tracked_gossip_message(ctx, state, origin.clone(), validator, message).await?; } Ok(()) @@ -504,7 +504,7 @@ where /// Send a gossip message and track it in the per relay parent data. async fn send_tracked_gossip_message( ctx: &mut Context, - tracker: &mut Tracker, + state: &mut ProtocolState, dest: PeerId, validator: ValidatorId, message: BitfieldGossipMessage, @@ -512,7 +512,7 @@ async fn send_tracked_gossip_message( where Context: SubsystemContext, { - let job_data = if let Some(job_data) = tracker.per_relay_parent.get_mut(&message.relay_parent) { + let job_data = if let Some(job_data) = state.per_relay_parent.get_mut(&message.relay_parent) { job_data } else { return Ok(()); @@ -643,7 +643,7 @@ mod test { ]; // empty initial state - let mut tracker = Tracker::default(); + let mut state = ProtocolState::default(); let pool = sp_core::testing::SpawnBlockingExecutor::new(); let (mut ctx, mut handle) = @@ -674,17 +674,17 @@ mod test { }; } - /// A very limited tracker, only interested in the relay parent of the + /// A very limited state, only interested in the relay parent of the /// given message, which must be signed by `validator` and a set of peers /// which are also only interested in that relay parent. - fn prewarmed_tracker( + fn prewarmed_state( validator: ValidatorId, signing_context: SigningContext, known_message: BitfieldGossipMessage, peers: Vec, - ) -> Tracker { + ) -> ProtocolState { let relay_parent = known_message.relay_parent.clone(); - Tracker { + ProtocolState { per_relay_parent: hashmap! { relay_parent.clone() => PerRelayParentData { @@ -705,8 +705,8 @@ mod test { } } - fn tracker_with_view(view: View, relay_parent: Hash) -> (Tracker, SigningContext, ValidatorPair) { - let mut tracker = Tracker::default(); + fn state_with_view(view: View, relay_parent: Hash) -> (ProtocolState, SigningContext, ValidatorPair) { + let mut state = ProtocolState::default(); let (validator_pair, _seed) = ValidatorPair::generate(); let validator = validator_pair.public(); @@ -716,7 +716,7 @@ mod test { parent_hash: relay_parent.clone(), }; - tracker.per_relay_parent = view.0.iter().map(|relay_parent| {( + state.per_relay_parent = view.0.iter().map(|relay_parent| {( relay_parent.clone(), PerRelayParentData { signing_context: signing_context.clone(), @@ -727,9 +727,9 @@ mod test { }) }).collect(); - tracker.view = view; + state.view = view; - (tracker, signing_context, validator_pair) + (state, signing_context, validator_pair) } #[test] @@ -771,7 +771,7 @@ mod test { let (mut ctx, mut handle) = subsystem_test::make_subsystem_context::(pool); - let mut tracker = prewarmed_tracker( + let mut state = prewarmed_state( validator.clone(), signing_context.clone(), msg.clone(), @@ -781,7 +781,7 @@ mod test { executor::block_on(async move { launch!(handle_network_msg( &mut ctx, - &mut tracker, + &mut state, NetworkBridgeEvent::PeerMessage(peer_b.clone(), msg.encode()), )); @@ -813,8 +813,8 @@ mod test { assert_ne!(peer_a, peer_b); // validator 0 key pair - let (mut tracker, signing_context, validator_pair) = tracker_with_view(view![hash_a, hash_b], hash_a.clone()); - tracker.peer_views.insert(peer_b.clone(), view![hash_a]); + let (mut state, signing_context, validator_pair) = state_with_view(view![hash_a, hash_b], hash_a.clone()); + state.peer_views.insert(peer_b.clone(), view![hash_a]); let payload = AvailabilityBitfield(bitvec![bitvec::order::Lsb0, u8; 1u8; 32]); let signed = @@ -832,7 +832,7 @@ mod test { executor::block_on(async move { launch!(handle_network_msg( &mut ctx, - &mut tracker, + &mut state, NetworkBridgeEvent::PeerMessage(peer_b.clone(), msg.encode()), )); @@ -864,7 +864,7 @@ mod test { assert_ne!(peer_a, peer_b); // validator 0 key pair - let (mut tracker, signing_context, validator_pair) = tracker_with_view(view![hash_a, hash_b], hash_a.clone()); + let (mut state, signing_context, validator_pair) = state_with_view(view![hash_a, hash_b], hash_a.clone()); // create a signed message by validator 0 let payload = AvailabilityBitfield(bitvec![bitvec::order::Lsb0, u8; 1u8; 32]); @@ -884,7 +884,7 @@ mod test { // send a first message launch!(handle_network_msg( &mut ctx, - &mut tracker, + &mut state, NetworkBridgeEvent::PeerMessage(peer_b.clone(), msg.encode()), )); @@ -914,7 +914,7 @@ mod test { // let peer A send the same message again launch!(handle_network_msg( &mut ctx, - &mut tracker, + &mut state, NetworkBridgeEvent::PeerMessage(peer_a.clone(), msg.encode()), )); @@ -931,7 +931,7 @@ mod test { // let peer B send the initial message again launch!(handle_network_msg( &mut ctx, - &mut tracker, + &mut state, NetworkBridgeEvent::PeerMessage(peer_b.clone(), msg.encode()), )); @@ -961,7 +961,7 @@ mod test { assert_ne!(peer_a, peer_b); // validator 0 key pair - let (mut tracker, signing_context, validator_pair) = tracker_with_view(view![hash_a, hash_b], hash_a.clone()); + let (mut state, signing_context, validator_pair) = state_with_view(view![hash_a, hash_b], hash_a.clone()); // create a signed message by validator 0 let payload = AvailabilityBitfield(bitvec![bitvec::order::Lsb0, u8; 1u8; 32]); @@ -980,23 +980,23 @@ mod test { executor::block_on(async move { launch!(handle_network_msg( &mut ctx, - &mut tracker, + &mut state, NetworkBridgeEvent::PeerConnected(peer_b.clone(), ObservedRole::Full), )); // make peer b interested launch!(handle_network_msg( &mut ctx, - &mut tracker, + &mut state, NetworkBridgeEvent::PeerViewChange(peer_b.clone(), view![hash_a, hash_b]), )); - assert!(tracker.peer_views.contains_key(&peer_b)); + assert!(state.peer_views.contains_key(&peer_b)); // recv a first message from the network launch!(handle_network_msg( &mut ctx, - &mut tracker, + &mut state, NetworkBridgeEvent::PeerMessage(peer_b.clone(), msg.encode()), )); @@ -1036,13 +1036,13 @@ mod test { launch!(handle_network_msg( &mut ctx, - &mut tracker, + &mut state, NetworkBridgeEvent::PeerViewChange(peer_b.clone(), view![]), )); - assert!(tracker.peer_views.contains_key(&peer_b)); + assert!(state.peer_views.contains_key(&peer_b)); assert_eq!( - tracker.peer_views.get(&peer_b).expect("Must contain value for peer B"), + state.peer_views.get(&peer_b).expect("Must contain value for peer B"), &view![] ); @@ -1050,7 +1050,7 @@ mod test { // should give penalty launch!(handle_network_msg( &mut ctx, - &mut tracker, + &mut state, NetworkBridgeEvent::PeerMessage(peer_b.clone(), msg.encode()), )); @@ -1067,18 +1067,18 @@ mod test { launch!(handle_network_msg( &mut ctx, - &mut tracker, + &mut state, NetworkBridgeEvent::PeerDisconnected(peer_b.clone()), )); // we are not interested in any peers at all anymore - tracker.view = view![]; + state.view = view![]; // on rx of the same message, since we are not interested, // should give penalty launch!(handle_network_msg( &mut ctx, - &mut tracker, + &mut state, NetworkBridgeEvent::PeerMessage(peer_a.clone(), msg.encode()), )); From 794c6c3260c303490f35ad6a06ec00928546928d Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Wed, 22 Jul 2020 16:21:29 +0200 Subject: [PATCH 42/45] chore check and comment rewording --- node/network/bitfield-distribution/src/lib.rs | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/node/network/bitfield-distribution/src/lib.rs b/node/network/bitfield-distribution/src/lib.rs index 1791d46eaa6d..d4ecc83c8d52 100644 --- a/node/network/bitfield-distribution/src/lib.rs +++ b/node/network/bitfield-distribution/src/lib.rs @@ -226,13 +226,17 @@ where return Ok(()); }; + let validator_set = &job_data.validator_set; + if validator_set.is_empty() { + trace!(target: "bitd", "Validator set for {:?} is empty", relay_parent); + return Ok(()); + } let validator_index = signed_availability.validator_index() as usize; - let validator_set = &job_data.validator_set; let validator = if let Some(validator) = validator_set.get(validator_index) { validator.clone() } else { - trace!(target: "bitd", "Could not find a validator matching index {}", validator_index); + trace!(target: "bitd", "Could not find a validator for index {}", validator_index); return Ok(()); }; @@ -475,14 +479,15 @@ where .into_iter() .filter_map(|new_relay_parent_interest| { if let Some(job_data) = (&*state).per_relay_parent.get(&new_relay_parent_interest) { - // send all messages + // Send all jointly known messages for a validator (given the current relay parent) + // to the peer `origin`... let one_per_validator = job_data.one_per_validator.clone(); let origin = origin.clone(); Some( one_per_validator .into_iter() .filter(move |(validator, _message)| { - // except for the ones the peer already has + // ..except for the ones the peer already has job_data.message_from_validator_needed_by_peer(&origin, validator) }), ) From f3a718ed0b271388dce73545a23ea4797498d33d Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Wed, 22 Jul 2020 16:37:08 +0200 Subject: [PATCH 43/45] feat test: invalid peer message --- node/network/bitfield-distribution/src/lib.rs | 57 ++++++++++++++++++- 1 file changed, 56 insertions(+), 1 deletion(-) diff --git a/node/network/bitfield-distribution/src/lib.rs b/node/network/bitfield-distribution/src/lib.rs index d4ecc83c8d52..6d5def0e7c82 100644 --- a/node/network/bitfield-distribution/src/lib.rs +++ b/node/network/bitfield-distribution/src/lib.rs @@ -952,7 +952,7 @@ mod test { }); } #[test] - fn change_view_and_then_recv() { + fn changing_view() { let _ = env_logger::builder() .filter(None, log::LevelFilter::Trace) .is_test(true) @@ -1100,4 +1100,59 @@ mod test { }); } + + + #[test] + fn invalid_peer_message() { + let _ = env_logger::builder() + .filter(None, log::LevelFilter::Trace) + .is_test(true) + .try_init(); + + let hash_a: Hash = [0; 32].into(); + let peer_a = PeerId::random(); + + // validator 0 key pair + let (mut state, _signing_context, _validator_pair) = state_with_view(view![], hash_a.clone()); + + let pool = sp_core::testing::SpawnBlockingExecutor::new(); + let (mut ctx, mut handle) = + subsystem_test::make_subsystem_context::(pool); + + executor::block_on(async move { + launch!(handle_network_msg( + &mut ctx, + &mut state, + NetworkBridgeEvent::PeerConnected(peer_a.clone(), ObservedRole::Full), + )); + + // make peer b interested + launch!(handle_network_msg( + &mut ctx, + &mut state, + NetworkBridgeEvent::PeerViewChange(peer_a.clone(), view![hash_a]), + )); + + assert!(state.peer_views.contains_key(&peer_a)); + + // recv a first message from the network + launch!(handle_network_msg( + &mut ctx, + &mut state, + NetworkBridgeEvent::PeerMessage(peer_a.clone(), b"00AaBbCcDdEeFf".to_vec()), + )); + + // reputation change for peer A + assert_matches!( + handle.recv().await, + AllMessages::NetworkBridge( + NetworkBridgeMessage::ReportPeer(peer, rep) + ) => { + assert_eq!(peer, peer_a); + assert_eq!(rep, COST_MESSAGE_NOT_DECODABLE); + } + ); + + }); + } } From e33e9436b8cd75788b72c3182baef1b5c28c2a49 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Wed, 22 Jul 2020 16:38:19 +0200 Subject: [PATCH 44/45] remove ignored test cases and unused macros --- node/network/bitfield-distribution/src/lib.rs | 68 +------------------ 1 file changed, 2 insertions(+), 66 deletions(-) diff --git a/node/network/bitfield-distribution/src/lib.rs b/node/network/bitfield-distribution/src/lib.rs index 6d5def0e7c82..604f6368ffba 100644 --- a/node/network/bitfield-distribution/src/lib.rs +++ b/node/network/bitfield-distribution/src/lib.rs @@ -593,12 +593,6 @@ mod test { use std::time::Duration; use assert_matches::assert_matches; - macro_rules! msg_sequence { - ($( $input:expr ),* $(,)? ) => [ - vec![ $( FromOverseer::Communication { msg: $input } ),* ] - ]; - } - macro_rules! view { ( $( $hash:expr ),* $(,)? ) => [ View(vec![ $( $hash.clone() ),* ]) @@ -611,64 +605,6 @@ mod test { ]; } - #[test] - #[ignore] - fn boundary_to_boundary() { - let hash_a: Hash = [0; 32].into(); // us - let hash_b: Hash = [1; 32].into(); // other - - let peer_a = PeerId::random(); - let peer_b = PeerId::random(); - - let signing_context = SigningContext { - session_index: 1, - parent_hash: hash_a.clone(), - }; - - // validator 0 key pair - let (validator_pair, _seed) = ValidatorPair::generate(); - let validator = validator_pair.public(); - - let payload = AvailabilityBitfield(bitvec![bitvec::order::Lsb0, u8; 1u8; 32]); - let signed = - Signed::::sign(payload, &signing_context, 0, &validator_pair); - - let input = - msg_sequence![ - BitfieldDistributionMessage::NetworkBridgeUpdate( - NetworkBridgeEvent::OurViewChange(view![hash_a, hash_b]) - ), - BitfieldDistributionMessage::NetworkBridgeUpdate( - NetworkBridgeEvent::PeerConnected(peer_b.clone(), ObservedRole::Full) - ), - BitfieldDistributionMessage::NetworkBridgeUpdate( - NetworkBridgeEvent::PeerViewChange(peer_b.clone(), view![hash_a, hash_b]) - ), - BitfieldDistributionMessage::DistributeBitfield(hash_b.clone(), signed.clone()), - ]; - - // empty initial state - let mut state = ProtocolState::default(); - - let pool = sp_core::testing::SpawnBlockingExecutor::new(); - let (mut ctx, mut handle) = - subsystem_test::make_subsystem_context::(pool); - - executor::block_on(async move { - for input in input.into_iter() { - handle.send(input.into()); - } - - // launch a complete subsystem instance and check responses on stimuli - // - // let completion = BitfieldDistribution::start(BitfieldDistribution, ctx) - // .future - // .timeout(Duration::from_millis(1000)) - // .await; - }); - } - - macro_rules! launch { ($fut:expr) => { $fut @@ -790,7 +726,7 @@ mod test { NetworkBridgeEvent::PeerMessage(peer_b.clone(), msg.encode()), )); - // we should have a reputation change due to invalid validator index + // reputation change due to invalid validator index assert_matches!( handle.recv().await, AllMessages::NetworkBridge( @@ -841,7 +777,7 @@ mod test { NetworkBridgeEvent::PeerMessage(peer_b.clone(), msg.encode()), )); - // we should have a reputation change due to invalid validator index + // reputation change due to invalid validator index assert_matches!( handle.recv().await, AllMessages::NetworkBridge( From 99f574ebad30f242c7ae8b469de986bae6ca2b50 Mon Sep 17 00:00:00 2001 From: Bernhard Schuster Date: Thu, 23 Jul 2020 10:09:09 +0200 Subject: [PATCH 45/45] fix master merge fallout + warnings --- Cargo.lock | 1 - node/network/bitfield-distribution/Cargo.toml | 4 +- node/network/bitfield-distribution/src/lib.rs | 47 ++++++++++--------- 3 files changed, 26 insertions(+), 26 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index fed80a144309..4968b0131c85 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4416,7 +4416,6 @@ dependencies = [ "polkadot-node-primitives", "polkadot-node-subsystem", "polkadot-primitives", - "polkadot-subsystem-test-helpers", "sc-network", "smol 0.2.0", "smol-timeout", diff --git a/node/network/bitfield-distribution/Cargo.toml b/node/network/bitfield-distribution/Cargo.toml index 57aba77e4e29..a6dab0307127 100644 --- a/node/network/bitfield-distribution/Cargo.toml +++ b/node/network/bitfield-distribution/Cargo.toml @@ -12,15 +12,15 @@ streamunordered = "0.5.1" codec = { package="parity-scale-codec", version = "1.3.0" } node-primitives = { package = "polkadot-node-primitives", path = "../../primitives" } polkadot-primitives = { path = "../../../primitives" } -polkadot-node-subsystem = { path = "../../subsystem" } +polkadot-subsystem = { package = "polkadot-node-subsystem", path = "../../subsystem" } polkadot-network-bridge = { path = "../../network/bridge" } polkadot-network = { path = "../../../network" } sc-network = { git = "https://github.com/paritytech/substrate", branch = "master" } [dev-dependencies] +polkadot-subsystem = { package = "polkadot-node-subsystem", path = "../../subsystem", features = [ "test-helpers" ] } bitvec = { version = "0.17.4", default-features = false, features = ["alloc"] } sp-core = { git = "https://github.com/paritytech/substrate", branch = "master" } -subsystem-test = { package = "polkadot-subsystem-test-helpers", path = "../../test-helpers/subsystem" } parking_lot = "0.10.0" maplit = "1.0.2" smol = "0.2.0" diff --git a/node/network/bitfield-distribution/src/lib.rs b/node/network/bitfield-distribution/src/lib.rs index 604f6368ffba..bc7c6690c614 100644 --- a/node/network/bitfield-distribution/src/lib.rs +++ b/node/network/bitfield-distribution/src/lib.rs @@ -25,9 +25,9 @@ use futures::{channel::oneshot, FutureExt}; use node_primitives::{ProtocolId, View}; -use log::{debug, info, trace, warn}; -use polkadot_node_subsystem::messages::*; -use polkadot_node_subsystem::{ +use log::{trace, warn}; +use polkadot_subsystem::messages::*; +use polkadot_subsystem::{ FromOverseer, OverseerSignal, SpawnedSubsystem, Subsystem, SubsystemContext, SubsystemResult, }; use polkadot_primitives::v1::{Hash, SignedAvailabilityBitfield, SigningContext, ValidatorId}; @@ -271,7 +271,7 @@ where message.signed_availability.clone(), )), )) - .await; + .await?; let message_sent_to_peer = &mut (job_data.message_sent_to_peer); @@ -384,12 +384,12 @@ where "Already received a message for validator at index {}", validator_index ); - modify_reputation(ctx, origin, GAIN_VALID_MESSAGE).await; + modify_reputation(ctx, origin, GAIN_VALID_MESSAGE).await?; return Ok(()); } one_per_validator.insert(validator.clone(), message.clone()); - relay_message(ctx, job_data, &mut state.peer_views, validator, message).await; + relay_message(ctx, job_data, &mut state.peer_views, validator, message).await?; modify_reputation(ctx, origin, GAIN_VALID_MESSAGE_FIRST).await } else { @@ -465,7 +465,6 @@ async fn handle_peer_view_change( where Context: SubsystemContext, { - use std::collections::hash_map::Entry; let current = state.peer_views.entry(origin.clone()).or_default(); let delta_vec: Vec = (*current).difference(&view).cloned().collect(); @@ -583,11 +582,11 @@ where #[cfg(test)] mod test { use super::*; - use bitvec::{bitvec, vec::BitVec}; + use bitvec::bitvec; use futures::executor; - use maplit::{hashmap, hashset}; - use polkadot_primitives::v0::{Signed, ValidatorPair}; - use polkadot_primitives::v1::AvailabilityBitfield; + use maplit::hashmap; + use polkadot_primitives::v1::{Signed, ValidatorPair, AvailabilityBitfield}; + use polkadot_subsystem::test_helpers::make_subsystem_context; use smol_timeout::TimeoutExt; use sp_core::crypto::Pair; use std::time::Duration; @@ -634,8 +633,8 @@ mod test { one_per_validator: hashmap! { validator.clone() => known_message.clone(), }, - message_received_from_peer: hashmap! {}, - message_sent_to_peer: hashmap! {}, + message_received_from_peer: hashmap!{}, + message_sent_to_peer: hashmap!{}, }, }, peer_views: peers @@ -662,8 +661,8 @@ mod test { PerRelayParentData { signing_context: signing_context.clone(), validator_set: vec![validator.clone()], - one_per_validator: hashmap! {}, - message_received_from_peer: hashmap! {}, + one_per_validator: hashmap!{}, + message_received_from_peer: hashmap!{}, message_sent_to_peer: hashmap!{}, }) }).collect(); @@ -681,7 +680,6 @@ mod test { .try_init(); let hash_a: Hash = [0; 32].into(); - let hash_b: Hash = [1; 32].into(); // other let peer_a = PeerId::random(); let peer_b = PeerId::random(); @@ -710,7 +708,7 @@ mod test { let pool = sp_core::testing::SpawnBlockingExecutor::new(); let (mut ctx, mut handle) = - subsystem_test::make_subsystem_context::(pool); + make_subsystem_context::(pool); let mut state = prewarmed_state( validator.clone(), @@ -754,7 +752,9 @@ mod test { assert_ne!(peer_a, peer_b); // validator 0 key pair - let (mut state, signing_context, validator_pair) = state_with_view(view![hash_a, hash_b], hash_a.clone()); + let (mut state, signing_context, validator_pair) = + state_with_view(view![hash_a, hash_b], hash_a.clone()); + state.peer_views.insert(peer_b.clone(), view![hash_a]); let payload = AvailabilityBitfield(bitvec![bitvec::order::Lsb0, u8; 1u8; 32]); @@ -768,7 +768,7 @@ mod test { let pool = sp_core::testing::SpawnBlockingExecutor::new(); let (mut ctx, mut handle) = - subsystem_test::make_subsystem_context::(pool); + make_subsystem_context::(pool); executor::block_on(async move { launch!(handle_network_msg( @@ -805,7 +805,8 @@ mod test { assert_ne!(peer_a, peer_b); // validator 0 key pair - let (mut state, signing_context, validator_pair) = state_with_view(view![hash_a, hash_b], hash_a.clone()); + let (mut state, signing_context, validator_pair) = + state_with_view(view![hash_a, hash_b], hash_a.clone()); // create a signed message by validator 0 let payload = AvailabilityBitfield(bitvec![bitvec::order::Lsb0, u8; 1u8; 32]); @@ -819,7 +820,7 @@ mod test { let pool = sp_core::testing::SpawnBlockingExecutor::new(); let (mut ctx, mut handle) = - subsystem_test::make_subsystem_context::(pool); + make_subsystem_context::(pool); executor::block_on(async move { // send a first message @@ -916,7 +917,7 @@ mod test { let pool = sp_core::testing::SpawnBlockingExecutor::new(); let (mut ctx, mut handle) = - subsystem_test::make_subsystem_context::(pool); + make_subsystem_context::(pool); executor::block_on(async move { launch!(handle_network_msg( @@ -1053,7 +1054,7 @@ mod test { let pool = sp_core::testing::SpawnBlockingExecutor::new(); let (mut ctx, mut handle) = - subsystem_test::make_subsystem_context::(pool); + make_subsystem_context::(pool); executor::block_on(async move { launch!(handle_network_msg(