From 170efd3cff8355b2439423e4b52a78687c1b6163 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Mon, 6 Jul 2020 15:33:20 +0200 Subject: [PATCH 01/16] update guide to reduce confusion and TODOs --- .../src/node/availability/bitfield-signing.md | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/roadmap/implementers-guide/src/node/availability/bitfield-signing.md b/roadmap/implementers-guide/src/node/availability/bitfield-signing.md index 613736901d2f..24c9d8dbedc8 100644 --- a/roadmap/implementers-guide/src/node/availability/bitfield-signing.md +++ b/roadmap/implementers-guide/src/node/availability/bitfield-signing.md @@ -4,6 +4,10 @@ Validators vote on the availability of a backed candidate by issuing signed bitf ## Protocol +Input: + +There is no dedicated input mechanism for bitfield signing. Instead, Bitfield Signing produces a bitfield representing the current state of availability on `StartWork`. + Output: - BitfieldDistribution::DistributeBitfield: distribute a locally signed bitfield @@ -19,7 +23,8 @@ Localized to a specific relay-parent `r` If not running as a validator, do nothing. - Determine our validator index `i`, the set of backed candidates pending availability in `r`, and which bit of the bitfield each corresponds to. -- > TODO: wait T time for availability distribution? - Start with an empty bitfield. For each bit in the bitfield, if there is a candidate pending availability, query the [Availability Store](../utility/availability-store.md) for whether we have the availability chunk for our validator index. - For all chunks we have, set the corresponding bit in the bitfield. - Sign the bitfield and dispatch a `BitfieldDistribution::DistributeBitfield` message. + +Note that because the bitfield signing job is launched once per block and executes immediately, the signed availability statement for block `N` is best considered as a report of the candidates available at the end of block `N-1`. From ed778a45648219ddbaf6a4aca97c27da87ad1186 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Mon, 20 Jul 2020 10:24:46 +0200 Subject: [PATCH 02/16] work from previous bitfield signing effort There were large merge issues with the old bitfield signing PR, so we're just copying all the work from that onto this and restarting. Much of the existing work will be discarded because we now have better tools available, but that's fine. --- Cargo.lock | 11 ++ Cargo.toml | 1 + node/bitfield-signing/Cargo.toml | 12 ++ node/bitfield-signing/src/lib.rs | 136 ++++++++++++++++++ node/subsystem/src/messages.rs | 15 ++ .../src/node/availability/bitfield-signing.md | 3 +- 6 files changed, 176 insertions(+), 2 deletions(-) create mode 100644 node/bitfield-signing/Cargo.toml create mode 100644 node/bitfield-signing/src/lib.rs diff --git a/Cargo.lock b/Cargo.lock index c4e95771fe29..8dd267cd027a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4496,6 +4496,17 @@ dependencies = [ "sp-runtime", ] +[[package]] +name = "polkadot-node-bitfield-signing" +version = "0.1.0" +dependencies = [ + "futures 0.3.5", + "log 0.4.8", + "polkadot-node-subsystem", + "polkadot-primitives", + "wasm-timer", +] + [[package]] name = "polkadot-node-core-backing" version = "0.1.0" diff --git a/Cargo.toml b/Cargo.toml index b7f45d5d2614..89cd9e4a4875 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -43,6 +43,7 @@ members = [ "service", "validation", + "node/bitfield-signing", "node/core/proposer", "node/network/bridge", "node/network/pov-distribution", diff --git a/node/bitfield-signing/Cargo.toml b/node/bitfield-signing/Cargo.toml new file mode 100644 index 000000000000..52ef2b8a284e --- /dev/null +++ b/node/bitfield-signing/Cargo.toml @@ -0,0 +1,12 @@ +[package] +name = "polkadot-node-bitfield-signing" +version = "0.1.0" +authors = ["Peter Goodspeed-Niklaus "] +edition = "2018" + +[dependencies] +futures = "0.3.5" +log = "0.4.8" +polkadot-primitives = { path = "../../primitives" } +polkadot-node-subsystem = { path = "../subsystem" } +wasm-timer = "0.2.4" diff --git a/node/bitfield-signing/src/lib.rs b/node/bitfield-signing/src/lib.rs new file mode 100644 index 000000000000..ba8e9a905c24 --- /dev/null +++ b/node/bitfield-signing/src/lib.rs @@ -0,0 +1,136 @@ +// 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 signing subsystem produces `SignedAvailabilityBitfield`s once per block. + +use futures::{ + channel::{mpsc, oneshot}, + future::{abortable, AbortHandle}, + prelude::*, + Future, +}; +use polkadot_node_subsystem::{ + messages::{AllMessages, BitfieldSigningMessage}, + OverseerSignal, SubsystemResult, +}; +use polkadot_node_subsystem::{FromOverseer, SpawnedSubsystem, Subsystem, SubsystemContext}; +use polkadot_primitives::Hash; +use std::{ + collections::HashMap, + pin::Pin, + time::{Duration, Instant}, +}; + +/// Delay between starting a bitfield signing job and its attempting to create a bitfield. +const JOB_DELAY: Duration = Duration::from_millis(1500); + +/// JobCanceler aborts all abort handles on drop. +#[derive(Debug, Default)] +struct JobCanceler(HashMap); + +// AbortHandle doesn't impl Drop on its own, so we wrap it +// in this struct to get free cancellation on drop. +impl Drop for JobCanceler { + fn drop(&mut self) { + for abort_handle in self.0.values() { + abort_handle.abort(); + } + } +} + +/// Bitfield signing subsystem. +struct BitfieldSigning; + +impl BitfieldSigning { + async fn run(mut ctx: Context) -> SubsystemResult<()> + where + Context: SubsystemContext + Clone, + { + let mut active_jobs = JobCanceler::default(); + + loop { + use FromOverseer::*; + use OverseerSignal::*; + match ctx.recv().await { + Ok(Communication { msg: _ }) => { + unreachable!("BitfieldSigningMessage is uninstantiable; qed") + } + Ok(Signal(StartWork(hash))) => { + let (future, abort_handle) = + abortable(bitfield_signing_job(hash.clone(), ctx.clone())); + // future currently returns a Result based on whether or not it was aborted; + // let's ignore all that and return () unconditionally, to fit the interface. + let future = async move { + let _ = future.await; + }; + active_jobs.0.insert(hash.clone(), abort_handle); + ctx.spawn(Box::pin(future)).await?; + } + Ok(Signal(StopWork(hash))) => { + if let Some(abort_handle) = active_jobs.0.remove(&hash) { + abort_handle.abort(); + } + } + Ok(Signal(Conclude)) => break, + Err(err) => { + return Err(err); + } + } + } + + Ok(()) + } +} + +impl Subsystem for BitfieldSigning +where + Context: SubsystemContext + Clone, +{ + fn start(self, ctx: Context) -> SpawnedSubsystem { + SpawnedSubsystem(Box::pin(async move { + if let Err(err) = Self::run(ctx).await { + log::error!("{:?}", err); + }; + })) + } +} + +async fn bitfield_signing_job(hash: Hash, ctx: Context) +where + Context: SubsystemContext, +{ + // first up, figure out when we need to wait until + let delay = wasm_timer::Delay::new_at(Instant::now() + JOB_DELAY); + // next, do some prerequisite work + todo!(); + // now, wait for the delay to be complete + if let Err(_) = delay.await { + return; + } + // let (tx, _) = oneshot::channel(); + + // ctx.send_message(AllMessages::CandidateValidation( + // CandidateValidationMessage::Validate( + // Default::default(), + // Default::default(), + // PoVBlock { + // block_data: BlockData(Vec::new()), + // }, + // tx, + // ) + // )).await.unwrap(); + unimplemented!() +} diff --git a/node/subsystem/src/messages.rs b/node/subsystem/src/messages.rs index 2040b413488d..eeff74ca341d 100644 --- a/node/subsystem/src/messages.rs +++ b/node/subsystem/src/messages.rs @@ -220,6 +220,19 @@ impl BitfieldDistributionMessage { } } +/// Bitfield signing message. +/// +/// Currently non-instantiable. +#[derive(Debug)] +pub enum BitfieldSigningMessage {} + +impl BitfieldSigningMessage { + /// If the current variant contains the relay parent hash, return it. + pub fn relay_parent(&self) -> Option { + None + } +} + /// Availability store subsystem message. #[derive(Debug)] pub enum AvailabilityStoreMessage { @@ -398,6 +411,8 @@ pub enum AllMessages { AvailabilityDistribution(AvailabilityDistributionMessage), /// Message for the bitfield distribution subsystem. BitfieldDistribution(BitfieldDistributionMessage), + /// Message for the bitfield signing subsystem. + BitfieldSigning(BitfieldSigningMessage), /// Message for the Provisioner subsystem. Provisioner(ProvisionerMessage), /// Message for the PoV Distribution subsystem. diff --git a/roadmap/implementers-guide/src/node/availability/bitfield-signing.md b/roadmap/implementers-guide/src/node/availability/bitfield-signing.md index 24c9d8dbedc8..856139589636 100644 --- a/roadmap/implementers-guide/src/node/availability/bitfield-signing.md +++ b/roadmap/implementers-guide/src/node/availability/bitfield-signing.md @@ -22,9 +22,8 @@ Upon onset of a new relay-chain head with `StartWork`, launch bitfield signing j Localized to a specific relay-parent `r` If not running as a validator, do nothing. +- Begin by waiting a fixed period of time so availability distribution has the chance to make candidates available. - Determine our validator index `i`, the set of backed candidates pending availability in `r`, and which bit of the bitfield each corresponds to. - Start with an empty bitfield. For each bit in the bitfield, if there is a candidate pending availability, query the [Availability Store](../utility/availability-store.md) for whether we have the availability chunk for our validator index. - For all chunks we have, set the corresponding bit in the bitfield. - Sign the bitfield and dispatch a `BitfieldDistribution::DistributeBitfield` message. - -Note that because the bitfield signing job is launched once per block and executes immediately, the signed availability statement for block `N` is best considered as a report of the candidates available at the end of block `N-1`. From a8d312cf6fe7663e6afe2d1af0e9fb9c7a97985d Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Mon, 20 Jul 2020 11:38:08 +0200 Subject: [PATCH 03/16] start rewriting bitfield signing in terms of the util module --- Cargo.lock | 3 + node/bitfield-signing/Cargo.toml | 3 + node/bitfield-signing/src/lib.rs | 220 ++++++++++++++++++------------- 3 files changed, 137 insertions(+), 89 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 8dd267cd027a..75a100993029 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4500,10 +4500,13 @@ dependencies = [ name = "polkadot-node-bitfield-signing" version = "0.1.0" dependencies = [ + "bitvec", + "derive_more 0.99.9", "futures 0.3.5", "log 0.4.8", "polkadot-node-subsystem", "polkadot-primitives", + "sc-keystore", "wasm-timer", ] diff --git a/node/bitfield-signing/Cargo.toml b/node/bitfield-signing/Cargo.toml index 52ef2b8a284e..c3a2934aaddd 100644 --- a/node/bitfield-signing/Cargo.toml +++ b/node/bitfield-signing/Cargo.toml @@ -5,8 +5,11 @@ authors = ["Peter Goodspeed-Niklaus "] edition = "2018" [dependencies] +bitvec = "0.17.4" +derive_more = "0.99.9" futures = "0.3.5" log = "0.4.8" polkadot-primitives = { path = "../../primitives" } polkadot-node-subsystem = { path = "../subsystem" } +keystore = { package = "sc-keystore", git = "https://github.com/paritytech/substrate", branch = "master" } wasm-timer = "0.2.4" diff --git a/node/bitfield-signing/src/lib.rs b/node/bitfield-signing/src/lib.rs index ba8e9a905c24..b0917b665cc8 100644 --- a/node/bitfield-signing/src/lib.rs +++ b/node/bitfield-signing/src/lib.rs @@ -16,121 +16,163 @@ //! The bitfield signing subsystem produces `SignedAvailabilityBitfield`s once per block. +use bitvec::bitvec; use futures::{ channel::{mpsc, oneshot}, - future::{abortable, AbortHandle}, prelude::*, Future, }; +use keystore::KeyStorePtr; use polkadot_node_subsystem::{ - messages::{AllMessages, BitfieldSigningMessage}, - OverseerSignal, SubsystemResult, + messages::{self, AllMessages, AvailabilityStoreMessage, BitfieldDistributionMessage, BitfieldSigningMessage, CandidateBackingMessage}, + util::{self, JobManager, JobTrait, ToJobTrait, Validator}, +}; +use polkadot_primitives::v1::{ + BackedCandidate, Hash, }; -use polkadot_node_subsystem::{FromOverseer, SpawnedSubsystem, Subsystem, SubsystemContext}; -use polkadot_primitives::Hash; use std::{ - collections::HashMap, + convert::TryFrom, pin::Pin, - time::{Duration, Instant}, + time::Duration, }; +use wasm_timer::{Delay, Instant}; /// Delay between starting a bitfield signing job and its attempting to create a bitfield. const JOB_DELAY: Duration = Duration::from_millis(1500); -/// JobCanceler aborts all abort handles on drop. -#[derive(Debug, Default)] -struct JobCanceler(HashMap); +/// Each `BitfieldSigningJob` prepares a signed bitfield for a single relay parent. +pub struct BitfieldSigningJob; + +/// Messages which a `BitfieldSigningJob` is prepared to receive. +pub enum ToJob { + BitfieldSigning(BitfieldSigningMessage), + Stop, +} + +impl ToJobTrait for ToJob { + const STOP: Self = ToJob::Stop; -// AbortHandle doesn't impl Drop on its own, so we wrap it -// in this struct to get free cancellation on drop. -impl Drop for JobCanceler { - fn drop(&mut self) { - for abort_handle in self.0.values() { - abort_handle.abort(); + fn relay_parent(&self) -> Option { + match self { + Self::BitfieldSigning(bsm) => bsm.relay_parent(), + Self::Stop => None, } } } -/// Bitfield signing subsystem. -struct BitfieldSigning; - -impl BitfieldSigning { - async fn run(mut ctx: Context) -> SubsystemResult<()> - where - Context: SubsystemContext + Clone, - { - let mut active_jobs = JobCanceler::default(); - - loop { - use FromOverseer::*; - use OverseerSignal::*; - match ctx.recv().await { - Ok(Communication { msg: _ }) => { - unreachable!("BitfieldSigningMessage is uninstantiable; qed") - } - Ok(Signal(StartWork(hash))) => { - let (future, abort_handle) = - abortable(bitfield_signing_job(hash.clone(), ctx.clone())); - // future currently returns a Result based on whether or not it was aborted; - // let's ignore all that and return () unconditionally, to fit the interface. - let future = async move { - let _ = future.await; - }; - active_jobs.0.insert(hash.clone(), abort_handle); - ctx.spawn(Box::pin(future)).await?; - } - Ok(Signal(StopWork(hash))) => { - if let Some(abort_handle) = active_jobs.0.remove(&hash) { - abort_handle.abort(); - } - } - Ok(Signal(Conclude)) => break, - Err(err) => { - return Err(err); - } - } +impl TryFrom for ToJob { + type Error = (); + + fn try_from(msg: AllMessages) -> Result { + match msg { + AllMessages::BitfieldSigning(bsm) => Ok(ToJob::BitfieldSigning(bsm)), + _ => Err(()) } + } +} - Ok(()) +impl From for ToJob { + fn from(bsm: BitfieldSigningMessage) -> ToJob { + ToJob::BitfieldSigning(bsm) } } -impl Subsystem for BitfieldSigning -where - Context: SubsystemContext + Clone, -{ - fn start(self, ctx: Context) -> SpawnedSubsystem { - SpawnedSubsystem(Box::pin(async move { - if let Err(err) = Self::run(ctx).await { - log::error!("{:?}", err); - }; - })) +/// Messages which may be sent from a `BitfieldSigningJob`. +pub enum FromJob { + AvailabilityStore(AvailabilityStoreMessage), + BitfieldDistribution(BitfieldDistributionMessage), + CandidateBacking(CandidateBackingMessage), +} + +impl From for AllMessages { + fn from(from_job: FromJob) -> AllMessages { + match from_job { + FromJob::AvailabilityStore(asm) => AllMessages::AvailabilityStore(asm), + FromJob::BitfieldDistribution(bdm) => AllMessages::BitfieldDistribution(bdm), + FromJob::CandidateBacking(cbm) => AllMessages::CandidateBacking(cbm), + } } } -async fn bitfield_signing_job(hash: Hash, ctx: Context) -where - Context: SubsystemContext, -{ - // first up, figure out when we need to wait until - let delay = wasm_timer::Delay::new_at(Instant::now() + JOB_DELAY); - // next, do some prerequisite work - todo!(); - // now, wait for the delay to be complete - if let Err(_) = delay.await { - return; +impl TryFrom for FromJob { + type Error = (); + + fn try_from(msg: AllMessages) -> Result { + match msg { + AllMessages::AvailabilityStore(asm) => Ok(Self::AvailabilityStore(asm)), + AllMessages::BitfieldDistribution(bdm) => Ok(Self::BitfieldDistribution(bdm)), + AllMessages::CandidateBacking(cbm) => Ok(Self::CandidateBacking(cbm)), + _ => Err(()), + } } - // let (tx, _) = oneshot::channel(); - - // ctx.send_message(AllMessages::CandidateValidation( - // CandidateValidationMessage::Validate( - // Default::default(), - // Default::default(), - // PoVBlock { - // block_data: BlockData(Vec::new()), - // }, - // tx, - // ) - // )).await.unwrap(); - unimplemented!() } + +/// Errors we may encounter in the course of executing the `BitfieldSigningSubsystem`. +#[derive(Debug, derive_more::From)] +pub enum Error { + /// error propagated from the utility subsystem + #[from] + Util(util::Error), + /// io error + #[from] + Io(std::io::Error), + /// a one shot channel was canceled + #[from] + Oneshot(oneshot::Canceled), + /// a mspc channel failed to send + #[from] + MpscSend(mpsc::SendError), +} + +async fn get_backed_candidates_pending_availability(relay_parent: Hash, sender: &mut mpsc::Sender) -> Result, Error> { + use FromJob::CandidateBacking; + use CandidateBackingMessage::GetBackedCandidates; + use messages::NewBackedCandidate; + + let (tx, rx) = oneshot::channel(); + // REVIEW: is this where we should be getting the set of backed candidates from? + sender.send(CandidateBacking(GetBackedCandidates(relay_parent, tx))).await?; + Ok(rx.await?.into_iter().map(|NewBackedCandidate(backed_candidate)| backed_candidate).collect()) +} + +impl JobTrait for BitfieldSigningJob { + type ToJob = ToJob; + type FromJob = FromJob; + type Error = Error; + type RunArgs = KeyStorePtr; + + const NAME: &'static str = "BitfieldSigningJob"; + + /// Run a job for the parent block indicated + fn run( + parent: Hash, + keystore: Self::RunArgs, + _receiver: mpsc::Receiver, + mut sender: mpsc::Sender, + ) -> Pin> + Send>> { + async move { + // figure out when to wait to + let wait_until = Instant::now() + JOB_DELAY; + + // now do all the work we can before we need to wait for the availability store + // if we're not a validator, we can just succeed effortlessly + let validator = match Validator::new(parent, keystore, sender.clone()).await { + Ok(validator) => validator, + Err(util::Error::NotAValidator) => return Ok(()), + Err(err) => return Err(Error::Util(err)), + }; + + // wait a bit before doing anything else + Delay::new_at(wait_until).await?; + + let backed_candidates = get_backed_candidates_pending_availability(parent, &mut sender).await?; + let bitvec_size: usize = todo!(); + let mut out = bitvec![0; bitvec_size]; + + unimplemented!() + }.boxed() + } +} + +/// BitfieldSigningSubsystem manages a number of bitfield signing jobs. +pub type BitfieldSigningSubsystem = JobManager; From 6d3081a1bfede4a11464dc696f2115a1706a78cf Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Tue, 21 Jul 2020 17:05:41 +0200 Subject: [PATCH 04/16] implement construct_availability_bitvec It's not an ideal implementation--we can make it much more concurrent-- but at least it compiles. --- node/bitfield-signing/src/lib.rs | 67 +++++++++++++++++++++++++------- node/subsystem/src/messages.rs | 12 +++++- parachain/src/primitives.rs | 12 ++++++ 3 files changed, 77 insertions(+), 14 deletions(-) diff --git a/node/bitfield-signing/src/lib.rs b/node/bitfield-signing/src/lib.rs index b0917b665cc8..2f6e092f399c 100644 --- a/node/bitfield-signing/src/lib.rs +++ b/node/bitfield-signing/src/lib.rs @@ -24,11 +24,19 @@ use futures::{ }; use keystore::KeyStorePtr; use polkadot_node_subsystem::{ - messages::{self, AllMessages, AvailabilityStoreMessage, BitfieldDistributionMessage, BitfieldSigningMessage, CandidateBackingMessage}, + messages::{ + self, + AllMessages, + AvailabilityStoreMessage, + BitfieldDistributionMessage, + BitfieldSigningMessage, + CandidateBackingMessage, + RuntimeApiMessage, + }, util::{self, JobManager, JobTrait, ToJobTrait, Validator}, }; use polkadot_primitives::v1::{ - BackedCandidate, Hash, + CoreOccupied, Hash, }; use std::{ convert::TryFrom, @@ -40,6 +48,9 @@ use wasm_timer::{Delay, Instant}; /// Delay between starting a bitfield signing job and its attempting to create a bitfield. const JOB_DELAY: Duration = Duration::from_millis(1500); +/// Unsigned bitfield type +pub type Bitfield = bitvec::vec::BitVec; + /// Each `BitfieldSigningJob` prepares a signed bitfield for a single relay parent. pub struct BitfieldSigningJob; @@ -82,6 +93,7 @@ pub enum FromJob { AvailabilityStore(AvailabilityStoreMessage), BitfieldDistribution(BitfieldDistributionMessage), CandidateBacking(CandidateBackingMessage), + RuntimeApi(RuntimeApiMessage), } impl From for AllMessages { @@ -90,6 +102,7 @@ impl From for AllMessages { FromJob::AvailabilityStore(asm) => AllMessages::AvailabilityStore(asm), FromJob::BitfieldDistribution(bdm) => AllMessages::BitfieldDistribution(bdm), FromJob::CandidateBacking(cbm) => AllMessages::CandidateBacking(cbm), + FromJob::RuntimeApi(ram) => AllMessages::RuntimeApi(ram), } } } @@ -102,6 +115,7 @@ impl TryFrom for FromJob { AllMessages::AvailabilityStore(asm) => Ok(Self::AvailabilityStore(asm)), AllMessages::BitfieldDistribution(bdm) => Ok(Self::BitfieldDistribution(bdm)), AllMessages::CandidateBacking(cbm) => Ok(Self::CandidateBacking(cbm)), + AllMessages::RuntimeApi(ram) => Ok(Self::RuntimeApi(ram)), _ => Err(()), } } @@ -124,15 +138,44 @@ pub enum Error { MpscSend(mpsc::SendError), } -async fn get_backed_candidates_pending_availability(relay_parent: Hash, sender: &mut mpsc::Sender) -> Result, Error> { - use FromJob::CandidateBacking; - use CandidateBackingMessage::GetBackedCandidates; - use messages::NewBackedCandidate; +// the way this function works is not intuitive: +// +// - get the scheduler roster so we have a list of cores, in order. +// - for each occupied core, fetch `candidate_pending_availability` from runtime +// - from there, we can get the `CandidateDescriptor` +// - from there, we can send a `AvailabilityStore::QueryPoV` and set the indexed bit to 1 if it returns Some(_) +async fn construct_availability_bitvec(relay_parent: Hash, sender: &mut mpsc::Sender) -> Result { + use messages::{ + AvailabilityStoreMessage::QueryPoVAvailable, + RuntimeApiRequest::{CandidatePendingAvailability, ValidatorGroups}, + }; + use RuntimeApiMessage::Request; + use FromJob::{AvailabilityStore, RuntimeApi}; let (tx, rx) = oneshot::channel(); - // REVIEW: is this where we should be getting the set of backed candidates from? - sender.send(CandidateBacking(GetBackedCandidates(relay_parent, tx))).await?; - Ok(rx.await?.into_iter().map(|NewBackedCandidate(backed_candidate)| backed_candidate).collect()) + + sender.send(RuntimeApi(Request(relay_parent, ValidatorGroups(tx)))).await?; + let scheduler_roster = rx.await?; + + let mut out = bitvec!(bitvec::order::Lsb0, u8; 0; scheduler_roster.availability_cores.len()); + + // TODO: make this concurrent by spawning a new task for each iteration of this loop + for (idx, core) in scheduler_roster.availability_cores.iter().enumerate() { + // REVIEW: is it safe to ignore parathreads here, or do they also figure in the availability mapping? + if let Some(CoreOccupied::Parachain) = core { + let (tx, rx) = oneshot::channel(); + sender.send(RuntimeApi(Request(relay_parent, CandidatePendingAvailability(idx.into(), tx)))).await?; + let committed_candidate_receipt = match rx.await? { + Some(ccr) => ccr, + None => continue, + }; + let (tx, rx) = oneshot::channel(); + sender.send(AvailabilityStore(QueryPoVAvailable(committed_candidate_receipt.descriptor.pov_hash, tx))).await?; + out.set(idx, rx.await?); + } + } + + Ok(out) } impl JobTrait for BitfieldSigningJob { @@ -165,11 +208,9 @@ impl JobTrait for BitfieldSigningJob { // wait a bit before doing anything else Delay::new_at(wait_until).await?; - let backed_candidates = get_backed_candidates_pending_availability(parent, &mut sender).await?; - let bitvec_size: usize = todo!(); - let mut out = bitvec![0; bitvec_size]; - + let bitvec = construct_availability_bitvec(parent, &mut sender).await?; unimplemented!() + }.boxed() } } diff --git a/node/subsystem/src/messages.rs b/node/subsystem/src/messages.rs index eeff74ca341d..7f1c7f48fffc 100644 --- a/node/subsystem/src/messages.rs +++ b/node/subsystem/src/messages.rs @@ -26,7 +26,7 @@ use futures::channel::{mpsc, oneshot}; use polkadot_primitives::v1::{ BlockNumber, Hash, - CandidateReceipt, PoV, ErasureChunk, BackedCandidate, Id as ParaId, + CandidateReceipt, CommittedCandidateReceipt, PoV, ErasureChunk, BackedCandidate, Id as ParaId, SignedAvailabilityBitfield, SigningContext, ValidatorId, ValidationCode, ValidatorIndex, CoreAssignment, CoreOccupied, HeadData, CandidateDescriptor, ValidatorSignature, OmittedValidationData, @@ -239,6 +239,13 @@ pub enum AvailabilityStoreMessage { /// Query a `PoV` from the AV store. QueryPoV(Hash, oneshot::Sender>), + /// Query whether a `PoV` exists within the AV Store. + /// + /// This is useful in cases like bitfield signing, when existence + /// matters, but we don't want to necessarily pass around multiple + /// megabytes of data to get a single bit of information. + QueryPoVAvailable(Hash, oneshot::Sender), + /// Query an `ErasureChunk` from the AV store. QueryChunk(Hash, ValidatorIndex, oneshot::Sender), @@ -251,6 +258,7 @@ impl AvailabilityStoreMessage { pub fn relay_parent(&self) -> Option { match self { Self::QueryPoV(hash, _) => Some(*hash), + Self::QueryPoVAvailable(hash, _) => Some(*hash), Self::QueryChunk(hash, _, _) => Some(*hash), Self::StoreChunk(hash, _, _) => Some(*hash), } @@ -285,6 +293,8 @@ pub enum RuntimeApiRequest { ValidationCode(ParaId, BlockNumber, Option, oneshot::Sender), /// Get head data for a specific para. HeadData(ParaId, oneshot::Sender), + /// Get a the candidate pending availability for a particular parachain by parachain / core index + CandidatePendingAvailability(ParaId, oneshot::Sender>), } /// A message to the Runtime API subsystem. diff --git a/parachain/src/primitives.rs b/parachain/src/primitives.rs index f6b69cd89441..1b3226e656d6 100644 --- a/parachain/src/primitives.rs +++ b/parachain/src/primitives.rs @@ -80,6 +80,18 @@ impl From for Id { fn from(x: u32) -> Self { Id(x) } } +impl From for Id { + fn from(x: usize) -> Self { + use std::convert::TryInto; + // can't panic, so need to truncate + let x: u32 = match x.try_into() { + Ok(x) => x, + Err(_) => u32::MAX, + }; + Id(x) + } +} + const USER_INDEX_START: u32 = 1000; /// The ID of the first user (non-system) parachain. From 29cc1c33a4692b4c327161caa863345314c5f69a Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Wed, 22 Jul 2020 10:35:51 +0200 Subject: [PATCH 05/16] implement the unimplemented portions of bitfield signing --- node/bitfield-signing/src/lib.rs | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/node/bitfield-signing/src/lib.rs b/node/bitfield-signing/src/lib.rs index 2f6e092f399c..3e5b9dcc0cba 100644 --- a/node/bitfield-signing/src/lib.rs +++ b/node/bitfield-signing/src/lib.rs @@ -36,7 +36,7 @@ use polkadot_node_subsystem::{ util::{self, JobManager, JobTrait, ToJobTrait, Validator}, }; use polkadot_primitives::v1::{ - CoreOccupied, Hash, + AvailabilityBitfield, CoreOccupied, Hash, }; use std::{ convert::TryFrom, @@ -48,9 +48,6 @@ use wasm_timer::{Delay, Instant}; /// Delay between starting a bitfield signing job and its attempting to create a bitfield. const JOB_DELAY: Duration = Duration::from_millis(1500); -/// Unsigned bitfield type -pub type Bitfield = bitvec::vec::BitVec; - /// Each `BitfieldSigningJob` prepares a signed bitfield for a single relay parent. pub struct BitfieldSigningJob; @@ -144,7 +141,7 @@ pub enum Error { // - for each occupied core, fetch `candidate_pending_availability` from runtime // - from there, we can get the `CandidateDescriptor` // - from there, we can send a `AvailabilityStore::QueryPoV` and set the indexed bit to 1 if it returns Some(_) -async fn construct_availability_bitvec(relay_parent: Hash, sender: &mut mpsc::Sender) -> Result { +async fn construct_availability_bitfield(relay_parent: Hash, sender: &mut mpsc::Sender) -> Result { use messages::{ AvailabilityStoreMessage::QueryPoVAvailable, RuntimeApiRequest::{CandidatePendingAvailability, ValidatorGroups}, @@ -175,7 +172,7 @@ async fn construct_availability_bitvec(relay_parent: Hash, sender: &mut mpsc::Se } } - Ok(out) + Ok(out.into()) } impl JobTrait for BitfieldSigningJob { @@ -188,7 +185,7 @@ impl JobTrait for BitfieldSigningJob { /// Run a job for the parent block indicated fn run( - parent: Hash, + relay_parent: Hash, keystore: Self::RunArgs, _receiver: mpsc::Receiver, mut sender: mpsc::Sender, @@ -199,7 +196,7 @@ impl JobTrait for BitfieldSigningJob { // now do all the work we can before we need to wait for the availability store // if we're not a validator, we can just succeed effortlessly - let validator = match Validator::new(parent, keystore, sender.clone()).await { + let validator = match Validator::new(relay_parent, keystore, sender.clone()).await { Ok(validator) => validator, Err(util::Error::NotAValidator) => return Ok(()), Err(err) => return Err(Error::Util(err)), @@ -208,9 +205,16 @@ impl JobTrait for BitfieldSigningJob { // wait a bit before doing anything else Delay::new_at(wait_until).await?; - let bitvec = construct_availability_bitvec(parent, &mut sender).await?; - unimplemented!() + let bitfield = construct_availability_bitfield(relay_parent, &mut sender).await?; + let signed_bitfield = validator.sign(bitfield); + + // make an anonymous scope to contain some use statements to simplify creating the outbound message + { + use BitfieldDistributionMessage::DistributeBitfield; + use FromJob::BitfieldDistribution; + sender.send(BitfieldDistribution(DistributeBitfield(relay_parent, signed_bitfield))).await.map_err(Into::into) + } }.boxed() } } From 5807799670340506621a4b56d9d7265ba3193818 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Wed, 22 Jul 2020 12:31:08 +0200 Subject: [PATCH 06/16] get core availability concurrently, not sequentially --- node/bitfield-signing/src/lib.rs | 155 ++++++++++++++++++++++--------- 1 file changed, 111 insertions(+), 44 deletions(-) diff --git a/node/bitfield-signing/src/lib.rs b/node/bitfield-signing/src/lib.rs index 3e5b9dcc0cba..bd7e4f7dc753 100644 --- a/node/bitfield-signing/src/lib.rs +++ b/node/bitfield-signing/src/lib.rs @@ -20,29 +20,18 @@ use bitvec::bitvec; use futures::{ channel::{mpsc, oneshot}, prelude::*, - Future, + stream, Future, }; use keystore::KeyStorePtr; use polkadot_node_subsystem::{ messages::{ - self, - AllMessages, - AvailabilityStoreMessage, - BitfieldDistributionMessage, - BitfieldSigningMessage, - CandidateBackingMessage, - RuntimeApiMessage, + self, AllMessages, AvailabilityStoreMessage, BitfieldDistributionMessage, + BitfieldSigningMessage, CandidateBackingMessage, RuntimeApiMessage, }, util::{self, JobManager, JobTrait, ToJobTrait, Validator}, }; -use polkadot_primitives::v1::{ - AvailabilityBitfield, CoreOccupied, Hash, -}; -use std::{ - convert::TryFrom, - pin::Pin, - time::Duration, -}; +use polkadot_primitives::v1::{AvailabilityBitfield, CoreOccupied, Hash}; +use std::{convert::TryFrom, pin::Pin, time::Duration}; use wasm_timer::{Delay, Instant}; /// Delay between starting a bitfield signing job and its attempting to create a bitfield. @@ -74,7 +63,7 @@ impl TryFrom for ToJob { fn try_from(msg: AllMessages) -> Result { match msg { AllMessages::BitfieldSigning(bsm) => Ok(ToJob::BitfieldSigning(bsm)), - _ => Err(()) + _ => Err(()), } } } @@ -133,6 +122,51 @@ pub enum Error { /// a mspc channel failed to send #[from] MpscSend(mpsc::SendError), + /// several errors collected into one + #[from] + Multiple(Vec), +} + +// this function exists mainly to collect a bunch of potential error points into one. +async fn get_core_availability( + relay_parent: Hash, + idx: usize, + core: Option, + sender: &mpsc::Sender, +) -> Result { + use messages::{ + AvailabilityStoreMessage::QueryPoVAvailable, + RuntimeApiRequest::CandidatePendingAvailability, + }; + use FromJob::{AvailabilityStore, RuntimeApi}; + use RuntimeApiMessage::Request; + + // we have to (cheaply) clone this sender so we can mutate it to actually send anything + let mut sender = sender.clone(); + + // REVIEW: is it safe to ignore parathreads here, or do they also figure in the availability mapping? + if let Some(CoreOccupied::Parachain) = core { + let (tx, rx) = oneshot::channel(); + sender + .send(RuntimeApi(Request( + relay_parent, + CandidatePendingAvailability(idx.into(), tx), + ))) + .await?; + let committed_candidate_receipt = match rx.await? { + Some(ccr) => ccr, + None => return Ok(false), + }; + let (tx, rx) = oneshot::channel(); + sender + .send(AvailabilityStore(QueryPoVAvailable( + committed_candidate_receipt.descriptor.pov_hash, + tx, + ))) + .await?; + return Ok(rx.await?); + } + Ok(false) } // the way this function works is not intuitive: @@ -141,38 +175,63 @@ pub enum Error { // - for each occupied core, fetch `candidate_pending_availability` from runtime // - from there, we can get the `CandidateDescriptor` // - from there, we can send a `AvailabilityStore::QueryPoV` and set the indexed bit to 1 if it returns Some(_) -async fn construct_availability_bitfield(relay_parent: Hash, sender: &mut mpsc::Sender) -> Result { - use messages::{ - AvailabilityStoreMessage::QueryPoVAvailable, - RuntimeApiRequest::{CandidatePendingAvailability, ValidatorGroups}, - }; +async fn construct_availability_bitfield( + relay_parent: Hash, + sender: &mut mpsc::Sender, +) -> Result { + use futures::lock::Mutex; + + use messages::RuntimeApiRequest::ValidatorGroups; + use FromJob::RuntimeApi; use RuntimeApiMessage::Request; - use FromJob::{AvailabilityStore, RuntimeApi}; + // request the validator groups so we can get the scheduler roster let (tx, rx) = oneshot::channel(); + sender + .send(RuntimeApi(Request(relay_parent, ValidatorGroups(tx)))) + .await?; + + // we now need sender to be immutable so we can copy the reference to multiple concurrent closures + let sender = &*sender; - sender.send(RuntimeApi(Request(relay_parent, ValidatorGroups(tx)))).await?; + // wait for the scheduler roster let scheduler_roster = rx.await?; - let mut out = bitvec!(bitvec::order::Lsb0, u8; 0; scheduler_roster.availability_cores.len()); - - // TODO: make this concurrent by spawning a new task for each iteration of this loop - for (idx, core) in scheduler_roster.availability_cores.iter().enumerate() { - // REVIEW: is it safe to ignore parathreads here, or do they also figure in the availability mapping? - if let Some(CoreOccupied::Parachain) = core { - let (tx, rx) = oneshot::channel(); - sender.send(RuntimeApi(Request(relay_parent, CandidatePendingAvailability(idx.into(), tx)))).await?; - let committed_candidate_receipt = match rx.await? { - Some(ccr) => ccr, - None => continue, + // prepare outputs + let out = + Mutex::new(bitvec!(bitvec::order::Lsb0, u8; 0; scheduler_roster.availability_cores.len())); + // in principle, we know that we never want concurrent access to the _same_ bit within the vec; + // we could `let out_ref = out.as_mut_ptr();` here instead, and manually assign bits, avoiding + // any need to ever wait to lock this mutex. + // in practice, it's safer to just use the mutex, and speed optimizations should wait until + // benchmarking proves that they are necessary. + let out_ref = &out; + let errs = Mutex::new(Vec::new()); + let errs_ref = &errs; + + // Handle each (idx, core) pair concurrently + // + // In principle, this work is all concurrent, not parallel. In practice, we can't guarantee it, which is why + // we need the mutexes and explicit references above. + stream::iter(scheduler_roster.availability_cores.into_iter().enumerate()) + .for_each_concurrent(None, |(idx, core)| async move { + let availability = match get_core_availability(relay_parent, idx, core, sender).await { + Ok(availability) => availability, + Err(err) => { + errs_ref.lock().await.push(err); + return; + } }; - let (tx, rx) = oneshot::channel(); - sender.send(AvailabilityStore(QueryPoVAvailable(committed_candidate_receipt.descriptor.pov_hash, tx))).await?; - out.set(idx, rx.await?); - } + out_ref.lock().await.set(idx, availability); + }) + .await; + + let errs = errs.into_inner(); + if errs.is_empty() { + Ok(out.into_inner().into()) + } else { + Err(errs.into()) } - - Ok(out.into()) } impl JobTrait for BitfieldSigningJob { @@ -213,11 +272,19 @@ impl JobTrait for BitfieldSigningJob { use BitfieldDistributionMessage::DistributeBitfield; use FromJob::BitfieldDistribution; - sender.send(BitfieldDistribution(DistributeBitfield(relay_parent, signed_bitfield))).await.map_err(Into::into) + sender + .send(BitfieldDistribution(DistributeBitfield( + relay_parent, + signed_bitfield, + ))) + .await + .map_err(Into::into) } - }.boxed() + } + .boxed() } } /// BitfieldSigningSubsystem manages a number of bitfield signing jobs. -pub type BitfieldSigningSubsystem = JobManager; +pub type BitfieldSigningSubsystem = + JobManager; From 00ef6116f8f02ae01f6d8c882d89326ef1926e64 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Wed, 22 Jul 2020 14:35:43 +0200 Subject: [PATCH 07/16] use sp-std instead of std for a parachain item --- parachain/src/primitives.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/parachain/src/primitives.rs b/parachain/src/primitives.rs index 1b3226e656d6..7e155e04fd2c 100644 --- a/parachain/src/primitives.rs +++ b/parachain/src/primitives.rs @@ -82,7 +82,7 @@ impl From for Id { impl From for Id { fn from(x: usize) -> Self { - use std::convert::TryInto; + use sp_std::convert::TryInto; // can't panic, so need to truncate let x: u32 = match x.try_into() { Ok(x) => x, From 7fc77805de5e5074a1b01037f8d4e3919e03e0e1 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Wed, 22 Jul 2020 15:03:28 +0200 Subject: [PATCH 08/16] resolve type inference failure caused by multiple From impls --- availability-store/src/store.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/availability-store/src/store.rs b/availability-store/src/store.rs index cd76de5d44ad..ca1009bf2357 100644 --- a/availability-store/src/store.rs +++ b/availability-store/src/store.rs @@ -412,8 +412,8 @@ mod tests { fn finalization_removes_unneeded() { let relay_parent = [1; 32].into(); - let para_id_1 = 5.into(); - let para_id_2 = 6.into(); + let para_id_1 = 5_u32.into(); + let para_id_2 = 6_u32.into(); let mut candidate_1 = AbridgedCandidateReceipt::default(); let mut candidate_2 = AbridgedCandidateReceipt::default(); @@ -482,7 +482,7 @@ mod tests { #[test] fn erasure_coding() { let relay_parent: Hash = [1; 32].into(); - let para_id: ParaId = 5.into(); + let para_id: ParaId = 5_u32.into(); let available_data = available_data(&[42; 8]); let n_validators = 5; @@ -555,11 +555,11 @@ mod tests { let mut receipt_2 = AbridgedCandidateReceipt::default(); - receipt_1.parachain_index = 1.into(); + receipt_1.parachain_index = 1_u32.into(); receipt_1.commitments.erasure_root = erasure_root_1; receipt_1.relay_parent = relay_parent; - receipt_2.parachain_index = 2.into(); + receipt_2.parachain_index = 2_u32.into(); receipt_2.commitments.erasure_root = erasure_root_2; receipt_2.relay_parent = relay_parent; From 684038fd504bbd993e03e8b63309cb078fa9201b Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Wed, 22 Jul 2020 15:36:30 +0200 Subject: [PATCH 09/16] handle bitfield signing subsystem & Allmessages variant in overseer --- node/overseer/examples/minimal-example.rs | 1 + node/overseer/src/lib.rs | 32 ++++++++++++++++++++--- 2 files changed, 29 insertions(+), 4 deletions(-) diff --git a/node/overseer/examples/minimal-example.rs b/node/overseer/examples/minimal-example.rs index 35ebf8f3367d..36440275cb5b 100644 --- a/node/overseer/examples/minimal-example.rs +++ b/node/overseer/examples/minimal-example.rs @@ -147,6 +147,7 @@ fn main() { candidate_selection: DummySubsystem, statement_distribution: DummySubsystem, availability_distribution: DummySubsystem, + bitfield_signing: DummySubsystem, bitfield_distribution: DummySubsystem, provisioner: DummySubsystem, pov_distribution: DummySubsystem, diff --git a/node/overseer/src/lib.rs b/node/overseer/src/lib.rs index c6ac6444c29e..cb158d09c129 100644 --- a/node/overseer/src/lib.rs +++ b/node/overseer/src/lib.rs @@ -77,7 +77,7 @@ use client::{BlockImportNotification, BlockchainEvents, FinalityNotification}; use polkadot_subsystem::messages::{ CandidateValidationMessage, CandidateBackingMessage, CandidateSelectionMessage, StatementDistributionMessage, - AvailabilityDistributionMessage, BitfieldDistributionMessage, + AvailabilityDistributionMessage, BitfieldSigningMessage, BitfieldDistributionMessage, ProvisionerMessage, PoVDistributionMessage, RuntimeApiMessage, AvailabilityStoreMessage, NetworkBridgeMessage, AllMessages, }; @@ -339,6 +339,9 @@ pub struct Overseer { /// An availability distribution subsystem. availability_distribution_subsystem: OverseenSubsystem, + /// A bitfield signing subsystem. + bitfield_signing_subsystem: OverseenSubsystem, + /// A bitfield distribution subsystem. bitfield_distribution_subsystem: OverseenSubsystem, @@ -390,7 +393,7 @@ pub struct Overseer { /// /// [`Subsystem`]: trait.Subsystem.html /// [`DummySubsystem`]: struct.DummySubsystem.html -pub struct AllSubsystems { +pub struct AllSubsystems { /// A candidate validation subsystem. pub candidate_validation: CV, /// A candidate backing subsystem. @@ -401,6 +404,8 @@ pub struct AllSubsystems { pub statement_distribution: SD, /// An availability distribution subsystem. pub availability_distribution: AD, + /// A bitfield signing subsystem. + pub bitfield_signing: BS, /// A bitfield distribution subsystem. pub bitfield_distribution: BD, /// A provisioner subsystem. @@ -487,6 +492,7 @@ where /// candidate_selection: DummySubsystem, /// statement_distribution: DummySubsystem, /// availability_distribution: DummySubsystem, + /// bitfield_signing: DummySubsystem, /// bitfield_distribution: DummySubsystem, /// provisioner: DummySubsystem, /// pov_distribution: DummySubsystem, @@ -513,9 +519,9 @@ where /// # /// # }); } /// ``` - pub fn new( + pub fn new( leaves: impl IntoIterator, - all_subsystems: AllSubsystems, + all_subsystems: AllSubsystems, mut s: S, ) -> SubsystemResult<(Self, OverseerHandler)> where @@ -524,6 +530,7 @@ where CS: Subsystem> + Send, SD: Subsystem> + Send, AD: Subsystem> + Send, + BS: Subsystem> + Send, BD: Subsystem> + Send, P: Subsystem> + Send, PoVD: Subsystem> + Send, @@ -575,6 +582,13 @@ where all_subsystems.availability_distribution, )?; + let bitfield_signing_subsystem = spawn( + &mut s, + &mut running_subsystems, + &mut running_subsystems_rx, + all_subsystems.bitfield_signing, + )?; + let bitfield_distribution_subsystem = spawn( &mut s, &mut running_subsystems, @@ -630,6 +644,7 @@ where candidate_selection_subsystem, statement_distribution_subsystem, availability_distribution_subsystem, + bitfield_signing_subsystem, bitfield_distribution_subsystem, provisioner_subsystem, pov_distribution_subsystem, @@ -871,6 +886,11 @@ where let _ = s.tx.send(FromOverseer::Communication { msg }).await; } } + AllMessages::BitfieldSigning(msg) => { + if let Some(ref mut s) = self.bitfield_signing_subsystem.instance { + let _ = s.tx.send(FromOverseer::Communication{ msg }).await; + } + } AllMessages::Provisioner(msg) => { if let Some(ref mut s) = self.provisioner_subsystem.instance { let _ = s.tx.send(FromOverseer::Communication { msg }).await; @@ -1050,6 +1070,7 @@ mod tests { candidate_selection: DummySubsystem, statement_distribution: DummySubsystem, availability_distribution: DummySubsystem, + bitfield_signing: DummySubsystem, bitfield_distribution: DummySubsystem, provisioner: DummySubsystem, pov_distribution: DummySubsystem, @@ -1112,6 +1133,7 @@ mod tests { candidate_selection: DummySubsystem, statement_distribution: DummySubsystem, availability_distribution: DummySubsystem, + bitfield_signing: DummySubsystem, bitfield_distribution: DummySubsystem, provisioner: DummySubsystem, pov_distribution: DummySubsystem, @@ -1227,6 +1249,7 @@ mod tests { candidate_selection: DummySubsystem, statement_distribution: DummySubsystem, availability_distribution: DummySubsystem, + bitfield_signing: DummySubsystem, bitfield_distribution: DummySubsystem, provisioner: DummySubsystem, pov_distribution: DummySubsystem, @@ -1323,6 +1346,7 @@ mod tests { candidate_selection: DummySubsystem, statement_distribution: DummySubsystem, availability_distribution: DummySubsystem, + bitfield_signing: DummySubsystem, bitfield_distribution: DummySubsystem, provisioner: DummySubsystem, pov_distribution: DummySubsystem, From f14ffe589e20d664d8a900ed62f68b6fb844a514 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Wed, 22 Jul 2020 16:00:36 +0200 Subject: [PATCH 10/16] fix more multi-From inference issues --- node/core/backing/src/lib.rs | 6 +++--- node/network/statement-distribution/src/lib.rs | 10 +++++----- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/node/core/backing/src/lib.rs b/node/core/backing/src/lib.rs index 3dd31e5e4837..4d7c0a1902b0 100644 --- a/node/core/backing/src/lib.rs +++ b/node/core/backing/src/lib.rs @@ -798,9 +798,9 @@ mod tests { impl Default for TestState { fn default() -> Self { - let chain_a = ParaId::from(1); - let chain_b = ParaId::from(2); - let thread_a = ParaId::from(3); + let chain_a = ParaId::from(1_u32); + let chain_b = ParaId::from(2_u32); + let thread_a = ParaId::from(3_u32); let chain_ids = vec![chain_a, chain_b, thread_a]; diff --git a/node/network/statement-distribution/src/lib.rs b/node/network/statement-distribution/src/lib.rs index ae13b62791f8..720e9782fa45 100644 --- a/node/network/statement-distribution/src/lib.rs +++ b/node/network/statement-distribution/src/lib.rs @@ -915,21 +915,21 @@ mod tests { let candidate_a = { let mut c = CommittedCandidateReceipt::default(); c.descriptor.relay_parent = parent_hash; - c.descriptor.para_id = 1.into(); + c.descriptor.para_id = 1_u32.into(); c }; let candidate_b = { let mut c = CommittedCandidateReceipt::default(); c.descriptor.relay_parent = parent_hash; - c.descriptor.para_id = 2.into(); + c.descriptor.para_id = 2_u32.into(); c }; let candidate_c = { let mut c = CommittedCandidateReceipt::default(); c.descriptor.relay_parent = parent_hash; - c.descriptor.para_id = 3.into(); + c.descriptor.para_id = 3_u32.into(); c }; @@ -1144,7 +1144,7 @@ mod tests { let candidate = { let mut c = CommittedCandidateReceipt::default(); c.descriptor.relay_parent = hash_c; - c.descriptor.para_id = 1.into(); + c.descriptor.para_id = 1_u32.into(); c }; let candidate_hash = candidate.hash(); @@ -1279,7 +1279,7 @@ mod tests { let candidate = { let mut c = CommittedCandidateReceipt::default(); c.descriptor.relay_parent = hash_b; - c.descriptor.para_id = 1.into(); + c.descriptor.para_id = 1_u32.into(); c }; From 89ff07baa8126cc0a412d0d89d0656728d9ac8dc Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Wed, 22 Jul 2020 16:07:20 +0200 Subject: [PATCH 11/16] more concisely handle overflow Co-authored-by: Andronik Ordian --- parachain/src/primitives.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/parachain/src/primitives.rs b/parachain/src/primitives.rs index 7e155e04fd2c..671793fc0aeb 100644 --- a/parachain/src/primitives.rs +++ b/parachain/src/primitives.rs @@ -84,10 +84,7 @@ impl From for Id { fn from(x: usize) -> Self { use sp_std::convert::TryInto; // can't panic, so need to truncate - let x: u32 = match x.try_into() { - Ok(x) => x, - Err(_) => u32::MAX, - }; + let x = x.try_into().unwrap_or(u32::MAX); Id(x) } } From c1b8b54f997bc6fb0d732b458b063ae2c7aee2fb Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Thu, 23 Jul 2020 12:54:56 +0200 Subject: [PATCH 12/16] Revert "resolve type inference failure caused by multiple From impls" This reverts commit 7fc77805de5e5074a1b01037f8d4e3919e03e0e1. --- availability-store/src/store.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/availability-store/src/store.rs b/availability-store/src/store.rs index ca1009bf2357..cd76de5d44ad 100644 --- a/availability-store/src/store.rs +++ b/availability-store/src/store.rs @@ -412,8 +412,8 @@ mod tests { fn finalization_removes_unneeded() { let relay_parent = [1; 32].into(); - let para_id_1 = 5_u32.into(); - let para_id_2 = 6_u32.into(); + let para_id_1 = 5.into(); + let para_id_2 = 6.into(); let mut candidate_1 = AbridgedCandidateReceipt::default(); let mut candidate_2 = AbridgedCandidateReceipt::default(); @@ -482,7 +482,7 @@ mod tests { #[test] fn erasure_coding() { let relay_parent: Hash = [1; 32].into(); - let para_id: ParaId = 5_u32.into(); + let para_id: ParaId = 5.into(); let available_data = available_data(&[42; 8]); let n_validators = 5; @@ -555,11 +555,11 @@ mod tests { let mut receipt_2 = AbridgedCandidateReceipt::default(); - receipt_1.parachain_index = 1_u32.into(); + receipt_1.parachain_index = 1.into(); receipt_1.commitments.erasure_root = erasure_root_1; receipt_1.relay_parent = relay_parent; - receipt_2.parachain_index = 2_u32.into(); + receipt_2.parachain_index = 2.into(); receipt_2.commitments.erasure_root = erasure_root_2; receipt_2.relay_parent = relay_parent; From 4410c87ceefd65e3e99e0ff142cd7263d5d50df8 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Thu, 23 Jul 2020 12:55:01 +0200 Subject: [PATCH 13/16] Revert "fix more multi-From inference issues" This reverts commit f14ffe589e20d664d8a900ed62f68b6fb844a514. --- node/core/backing/src/lib.rs | 6 +++--- node/network/statement-distribution/src/lib.rs | 10 +++++----- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/node/core/backing/src/lib.rs b/node/core/backing/src/lib.rs index fa5769ab12e0..3d5440968db9 100644 --- a/node/core/backing/src/lib.rs +++ b/node/core/backing/src/lib.rs @@ -802,9 +802,9 @@ mod tests { impl Default for TestState { fn default() -> Self { - let chain_a = ParaId::from(1_u32); - let chain_b = ParaId::from(2_u32); - let thread_a = ParaId::from(3_u32); + let chain_a = ParaId::from(1); + let chain_b = ParaId::from(2); + let thread_a = ParaId::from(3); let chain_ids = vec![chain_a, chain_b, thread_a]; diff --git a/node/network/statement-distribution/src/lib.rs b/node/network/statement-distribution/src/lib.rs index 720e9782fa45..ae13b62791f8 100644 --- a/node/network/statement-distribution/src/lib.rs +++ b/node/network/statement-distribution/src/lib.rs @@ -915,21 +915,21 @@ mod tests { let candidate_a = { let mut c = CommittedCandidateReceipt::default(); c.descriptor.relay_parent = parent_hash; - c.descriptor.para_id = 1_u32.into(); + c.descriptor.para_id = 1.into(); c }; let candidate_b = { let mut c = CommittedCandidateReceipt::default(); c.descriptor.relay_parent = parent_hash; - c.descriptor.para_id = 2_u32.into(); + c.descriptor.para_id = 2.into(); c }; let candidate_c = { let mut c = CommittedCandidateReceipt::default(); c.descriptor.relay_parent = parent_hash; - c.descriptor.para_id = 3_u32.into(); + c.descriptor.para_id = 3.into(); c }; @@ -1144,7 +1144,7 @@ mod tests { let candidate = { let mut c = CommittedCandidateReceipt::default(); c.descriptor.relay_parent = hash_c; - c.descriptor.para_id = 1_u32.into(); + c.descriptor.para_id = 1.into(); c }; let candidate_hash = candidate.hash(); @@ -1279,7 +1279,7 @@ mod tests { let candidate = { let mut c = CommittedCandidateReceipt::default(); c.descriptor.relay_parent = hash_b; - c.descriptor.para_id = 1_u32.into(); + c.descriptor.para_id = 1.into(); c }; From 528cf9bf2d26d94e5e0fc66c98f0ddc1d1cce990 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Thu, 23 Jul 2020 13:10:32 +0200 Subject: [PATCH 14/16] impl From for ParaId --- parachain/src/primitives.rs | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/parachain/src/primitives.rs b/parachain/src/primitives.rs index 671793fc0aeb..65d890e26dcf 100644 --- a/parachain/src/primitives.rs +++ b/parachain/src/primitives.rs @@ -89,6 +89,27 @@ impl From for Id { } } +// When we added a second From impl for Id, type inference could no longer +// determine which impl should apply for things like `5.into()`. It therefore +// raised a bunch of errors in our test code, scattered throughout the +// various modules' tests, that there is no impl of `From` (`i32` being +// the default numeric type). +// +// We can't use `cfg(test)` here, because that configuration directive does not +// propagate between crates, which would fail to fix tests in crates other than +// this one. +// +// Instead, let's take advantage of the observation that what really matters for a +// ParaId within a test context is that it is unique and constant. I believe that +// there is no case where someone does `(-1).into()` anyway, but if they do, it +// never matters whether the actual contained ID is `-1` or `4294967295`. Nobody +// does arithmetic on a `ParaId`; doing so would be a bug. +impl From for Id { + fn from(x: i32) -> Self { + Id(x as u32) + } +} + const USER_INDEX_START: u32 = 1000; /// The ID of the first user (non-system) parachain. From 77794059890dd97e3d90e3821b8c5ee41ea47823 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Thu, 23 Jul 2020 13:31:01 +0200 Subject: [PATCH 15/16] handle another instance of AllSubsystems --- node/service/src/lib.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/node/service/src/lib.rs b/node/service/src/lib.rs index 6d5cdbda427e..46ba4d0e03fa 100644 --- a/node/service/src/lib.rs +++ b/node/service/src/lib.rs @@ -280,6 +280,7 @@ fn real_overseer( candidate_selection: DummySubsystem, statement_distribution: DummySubsystem, availability_distribution: DummySubsystem, + bitfield_signing: DummySubsystem, bitfield_distribution: DummySubsystem, provisioner: DummySubsystem, pov_distribution: DummySubsystem, From 333d0764d3f9635c606494fea514ef0b5a3fe8d7 Mon Sep 17 00:00:00 2001 From: Peter Goodspeed-Niklaus Date: Thu, 23 Jul 2020 15:45:40 +0200 Subject: [PATCH 16/16] improve consistency when returning existing options --- node/bitfield-signing/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/node/bitfield-signing/src/lib.rs b/node/bitfield-signing/src/lib.rs index bd7e4f7dc753..b973f4b3ae09 100644 --- a/node/bitfield-signing/src/lib.rs +++ b/node/bitfield-signing/src/lib.rs @@ -164,7 +164,7 @@ async fn get_core_availability( tx, ))) .await?; - return Ok(rx.await?); + return rx.await.map_err(Into::into); } Ok(false) }