From fba901e1628b2b81f55437c5b4f4aa617e73ae0c Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Wed, 28 Apr 2021 18:14:57 +0200 Subject: [PATCH 01/26] Remove signature verification in backing. `SignedFullStatement` now signals that the signature has already been checked. --- node/core/backing/src/lib.rs | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/node/core/backing/src/lib.rs b/node/core/backing/src/lib.rs index b269ab6762f6..111cc10b3f5d 100644 --- a/node/core/backing/src/lib.rs +++ b/node/core/backing/src/lib.rs @@ -870,7 +870,6 @@ impl CandidateBackingJob { .with_candidate(statement.payload().candidate_hash()) .with_relay_parent(_relay_parent); - self.check_statement_signature(&statement)?; match self.maybe_validate_and_import(&root_span, sender, statement).await { Err(Error::ValidationFailed(_)) => return Ok(()), Err(e) => return Err(e), @@ -1028,22 +1027,6 @@ impl CandidateBackingJob { Some(signed) } - #[tracing::instrument(level = "trace", skip(self), fields(subsystem = LOG_TARGET))] - fn check_statement_signature(&self, statement: &SignedFullStatement) -> Result<(), Error> { - let idx = statement.validator_index().0 as usize; - - if self.table_context.validators.len() > idx { - statement.check_signature( - &self.table_context.signing_context, - &self.table_context.validators[idx], - ).map_err(|_| Error::InvalidSignature)?; - } else { - return Err(Error::InvalidSignature); - } - - Ok(()) - } - /// Insert or get the unbacked-span for the given candidate hash. fn insert_or_get_unbacked_span( &mut self, From a72822f1c71b0290ee1c8feecaa5ec65c34bd9ae Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Wed, 28 Apr 2021 18:15:57 +0200 Subject: [PATCH 02/26] Remove unused check_payload function. --- node/subsystem-util/src/lib.rs | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/node/subsystem-util/src/lib.rs b/node/subsystem-util/src/lib.rs index 98f3dda1a345..287b530440bf 100644 --- a/node/subsystem-util/src/lib.rs +++ b/node/subsystem-util/src/lib.rs @@ -322,21 +322,6 @@ impl Validator { ) -> Result>, KeystoreError> { Signed::sign(&keystore, payload, &self.signing_context, self.index, &self.key).await } - - /// Validate the payload with this validator - /// - /// Validation can only succeed if `signed.validator_index() == self.index()`. - /// Normally, this will always be the case for a properly operating program, - /// but it's double-checked here anyway. - pub fn check_payload, RealPayload: Encode>( - &self, - signed: Signed, - ) -> Result<(), ()> { - if signed.validator_index() != self.index { - return Err(()); - } - signed.check_signature(&self.signing_context, &self.id()) - } } struct AbortOnDrop(future::AbortHandle); From 759ba54bd916fb5c72208287bded060304c45f52 Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Wed, 28 Apr 2021 18:17:06 +0200 Subject: [PATCH 03/26] Introduced unchecked signed variants. --- primitives/src/v0.rs | 285 ++++++++++++++++++++++++++++++++++++------- primitives/src/v1.rs | 8 +- 2 files changed, 247 insertions(+), 46 deletions(-) diff --git a/primitives/src/v0.rs b/primitives/src/v0.rs index 9a7112bccd0d..54d316bb298d 100644 --- a/primitives/src/v0.rs +++ b/primitives/src/v0.rs @@ -892,12 +892,26 @@ impl EncodeAs for T { } } +/// Signed data with signature already verified. +/// +/// NOTE: This type does have an Encode/Decode instance because we are encoding/decoding accross +/// trusted bundaries for runtime communication (inherent data), so care must be taken to never use +/// those instances on untrusted boundaries as they render our valid signatures guarantees invalid. +/// Instead use `UncheckedSigned` on such boundaries, `Signed` can easily be converted into +/// `UncheckedSigned` and conversion back via `into_signed` enforces a valid signature again. +#[derive(Clone, PartialEq, Eq, RuntimeDebug)] +pub struct Signed(SignedImpl); + +/// Unsigned data, can be converted to `Signed` by checking the signature. +#[derive(Clone, PartialEq, Eq, RuntimeDebug)] +pub struct UncheckedSigned(SignedImpl); + /// A signed type which encapsulates the common desire to sign some data and validate a signature. /// /// Note that the internal fields are not public; they are all accessable by immutable getters. /// This reduces the chance that they are accidentally mutated, invalidating the signature. -#[derive(Clone, PartialEq, Eq, Encode, Decode, RuntimeDebug)] -pub struct Signed { +#[derive(Clone, PartialEq, Eq, RuntimeDebug, Encode, Decode)] +struct SignedImpl { /// The payload is part of the signed data. The rest is the signing context, /// which is known both at signing and at validation. payload: Payload, @@ -909,19 +923,10 @@ pub struct Signed { real_payload: sp_std::marker::PhantomData, } -// We can't bound this on `Payload: Into` beacuse that conversion consumes -// the payload, and we don't want that. We can't bound it on `Payload: AsRef` -// because there's no blanket impl of `AsRef for T`. In the end, we just invent our -// own trait which does what we need: EncodeAs. impl, RealPayload: Encode> Signed { - fn payload_data(payload: &Payload, context: &SigningContext) -> Vec { - // equivalent to (real_payload, context).encode() - let mut out = payload.encode_as(); - out.extend(context.encode()); - out - } - /// Used to create a `Signed` from already existing parts. + /// + /// The signature is checked as part of the process. #[cfg(feature = "std")] pub fn new( payload: Payload, @@ -930,7 +935,7 @@ impl, RealPayload: Encode> Signed, key: &ValidatorId, ) -> Option { - let s = Self { + let s = SignedImpl { payload, validator_index, signature, @@ -939,10 +944,10 @@ impl, RealPayload: Encode> Signed( keystore: &SyncCryptoStorePtr, @@ -950,6 +955,139 @@ impl, RealPayload: Encode> Signed, validator_index: ValidatorIndex, key: &ValidatorId, + ) -> Result, KeystoreError> { + let r = SignedImpl::sign(keystore, payload, context, validator_index, key).await?; + Ok(r.map(Self)) + } + + /// Try to convert from `UncheckedSigned` by checking the signature. + pub fn from_unchecked( + unchecked: UncheckedSigned, + context: &SigningContext, + key: &ValidatorId + ) -> Option { + if unchecked.0.check_signature(context, key).is_ok() { + Some(Self(unchecked.0)) + } else { + None + } + } + + /// Immutably access the payload. + #[inline] + pub fn payload(&self) -> &Payload { + &self.0.payload + } + + /// Immutably access the validator index. + #[inline] + pub fn validator_index(&self) -> ValidatorIndex { + self.0.validator_index + } + + /// Immutably access the signature. + #[inline] + pub fn signature(&self) -> &ValidatorSignature { + &self.0.signature + } + + /// Discard signing data, get the payload + #[inline] + pub fn into_payload(self) -> Payload { + self.0.payload + } + + /// Convert `Payload` into `RealPayload`. + pub fn convert_payload(&self) -> Signed where for<'a> &'a Payload: Into { + Signed(self.0.convert_payload()) + } +} + +impl, RealPayload: Encode> UncheckedSigned { + /// Used to create a `UncheckedSigned` from already existing parts. + /// + /// Signature is not checked here, hence `UncheckedSigned`. + #[cfg(feature = "std")] + pub fn new( + payload: Payload, + validator_index: ValidatorIndex, + signature: ValidatorSignature, + ) -> Self { + let s = SignedImpl { + payload, + validator_index, + signature, + real_payload: std::marker::PhantomData, + }; + + Self(s) + } + + /// Check signature and convert to `Signed` if successful. + pub fn into_checked( + self, + context: &SigningContext, + key: &ValidatorId + ) -> Option> { + Signed::from_unchecked(self, context, key) + } + + /// Immutably access the payload. + #[inline] + pub fn unchecked_payload(&self) -> &Payload { + &self.0.payload + } + + /// Immutably access the validator index. + #[inline] + pub fn unchecked_validator_index(&self) -> ValidatorIndex { + self.0.validator_index + } + + /// Immutably access the signature. + #[inline] + pub fn unchecked_signature(&self) -> &ValidatorSignature { + &self.0.signature + } + + /// Discard signing data, get the payload + #[inline] + pub fn into_unchecked_payload(self) -> Payload { + self.0.payload + } + + /// Convert `Payload` into `RealPayload`. + pub fn convert_unchecked_payload(&self) -> UncheckedSigned where for<'a> &'a Payload: Into { + UncheckedSigned(self.0.convert_payload()) + } +} + +impl From> for UncheckedSigned { + fn from(signed: Signed) -> Self { + Self(signed.0) + } +} + +// We can't bound this on `Payload: Into` beacuse that conversion consumes +// the payload, and we don't want that. We can't bound it on `Payload: AsRef` +// because there's no blanket impl of `AsRef for T`. In the end, we just invent our +// own trait which does what we need: EncodeAs. +impl, RealPayload: Encode> SignedImpl { + fn payload_data(payload: &Payload, context: &SigningContext) -> Vec { + // equivalent to (real_payload, context).encode() + let mut out = payload.encode_as(); + out.extend(context.encode()); + out + } + + /// Sign this payload with the given context and key, storing the validator index. + #[cfg(feature = "std")] + async fn sign( + keystore: &SyncCryptoStorePtr, + payload: Payload, + context: &SigningContext, + validator_index: ValidatorIndex, + key: &ValidatorId, ) -> Result, KeystoreError> { let data = Self::payload_data(&payload, context); let signature = CryptoStore::sign_with( @@ -973,46 +1111,105 @@ impl, RealPayload: Encode> Signed(&self, context: &SigningContext, key: &ValidatorId) -> Result<(), ()> { + fn check_signature(&self, context: &SigningContext, key: &ValidatorId) -> Result<(), ()> { let data = Self::payload_data(&self.payload, context); if self.signature.verify(data.as_slice(), key) { Ok(()) } else { Err(()) } } - /// Immutably access the payload. - #[inline] - pub fn payload(&self) -> &Payload { - &self.payload + /// Convert `Payload` into `RealPayload`. + fn convert_payload(&self) -> SignedImpl where for<'a> &'a Payload: Into { + SignedImpl { + signature: self.signature.clone(), + validator_index: self.validator_index, + payload: (&self.payload).into(), + real_payload: sp_std::marker::PhantomData, + } } +} - /// Immutably access the validator index. - #[inline] - pub fn validator_index(&self) -> ValidatorIndex { - self.validator_index +/// Dummy implementation (derive does not do the right thing): +impl Encode for Signed + where + Payload: Encode +{ + fn size_hint(&self) -> usize { + self.0.size_hint() } - /// Immutably access the signature. - #[inline] - pub fn signature(&self) -> &ValidatorSignature { - &self.signature + fn encode_to(&self, dest: &mut T) { + self.0.encode_to(dest) } - /// Discard signing data, get the payload - // Note: can't `impl From> for P` because the orphan rule exception doesn't - // handle this case yet. Likewise can't `impl Into

for Signed` because it might - // potentially conflict with the global blanket impl, even though it currently doesn't. - #[inline] - pub fn into_payload(self) -> Payload { - self.payload + fn encode(&self) -> Vec { + self.0.encode() } - /// Convert `Payload` into `RealPayload`. - pub fn convert_payload(&self) -> Signed where for<'a> &'a Payload: Into { - Signed { - signature: self.signature.clone(), - validator_index: self.validator_index, - payload: self.payload().into(), - real_payload: sp_std::marker::PhantomData, - } + fn using_encoded R>(&self, f: F) -> R { + self.0.using_encoded(f) + } + + fn encoded_size(&self) -> usize { + self.0.encoded_size() + } +} + +/// Dummy implementation (derive does not do the right thing): +impl Encode for UncheckedSigned + where + Payload: Encode, +{ + fn size_hint(&self) -> usize { + self.0.size_hint() + } + + fn encode_to(&self, dest: &mut T) { + self.0.encode_to(dest) + } + + fn encode(&self) -> Vec { + self.0.encode() + } + + fn using_encoded R>(&self, f: F) -> R { + self.0.using_encoded(f) + } + + fn encoded_size(&self) -> usize { + self.0.encoded_size() + } +} + +impl Decode for Signed + where + Payload: Decode, +{ + fn decode(input: &mut I) -> Result { + as Decode>::decode(input).map(Self) + } + + fn skip(input: &mut I) -> Result<(), parity_scale_codec::Error> { + as Decode>::skip(input) + } + + fn encoded_fixed_size() -> Option { + as Decode>::encoded_fixed_size() + } +} + +impl Decode for UncheckedSigned + where + Payload: Decode, +{ + fn decode(input: &mut I) -> Result { + as Decode>::decode(input).map(Self) + } + + fn skip(input: &mut I) -> Result<(), parity_scale_codec::Error> { + as Decode>::skip(input) + } + + fn encoded_fixed_size() -> Option { + as Decode>::encoded_fixed_size() } } diff --git a/primitives/src/v1.rs b/primitives/src/v1.rs index 4fdab1f3e1ad..e5106bf306c7 100644 --- a/primitives/src/v1.rs +++ b/primitives/src/v1.rs @@ -44,7 +44,7 @@ pub use polkadot_parachain::primitives::{ // Export some basic parachain primitives from v0. pub use crate::v0::{ CollatorId, CollatorSignature, PARACHAIN_KEY_TYPE_ID, ValidatorId, ValidatorIndex, - ValidatorSignature, SigningContext, Signed, ValidityAttestation, + ValidatorSignature, SigningContext, Signed, UncheckedSigned, ValidityAttestation, CompactStatement, SignedStatement, EncodeAs, }; @@ -463,9 +463,13 @@ impl From> for AvailabilityBitfield { /// A bitfield signed by a particular validator about the availability of pending candidates. pub type SignedAvailabilityBitfield = Signed; +/// A signed bitfield with signature not yet checked. +pub type UncheckedSignedAvailabilityBitfield = UncheckedSigned; /// A set of signed availability bitfields. Should be sorted by validator index, ascending. pub type SignedAvailabilityBitfields = Vec; +/// A set of unchecked signed availability bitfields. Should be sorted by validator index, ascending. +pub type UncheckedSignedAvailabilityBitfields = Vec; /// A backed (or backable, depending on context) candidate. #[derive(Encode, Decode, Clone, PartialEq, Eq, RuntimeDebug)] @@ -1120,7 +1124,7 @@ pub struct DisputeState { #[derive(Encode, Decode, Clone, PartialEq, RuntimeDebug)] pub struct InherentData { /// Signed bitfields by validators about availability. - pub bitfields: SignedAvailabilityBitfields, + pub bitfields: UncheckedSignedAvailabilityBitfields, /// Backed candidates for inclusion in the block. pub backed_candidates: Vec>, /// Sets of dispute votes for inclusion, From 0573178bef78711a7941ea348f29a5ec48f63593 Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Wed, 28 Apr 2021 18:17:40 +0200 Subject: [PATCH 04/26] Fix inclusion to use unchecked variant. --- runtime/parachains/src/inclusion.rs | 71 ++++++++++++++++------------- 1 file changed, 39 insertions(+), 32 deletions(-) diff --git a/runtime/parachains/src/inclusion.rs b/runtime/parachains/src/inclusion.rs index 733e3bc138ea..f8b6ed16681e 100644 --- a/runtime/parachains/src/inclusion.rs +++ b/runtime/parachains/src/inclusion.rs @@ -23,7 +23,7 @@ use sp_std::prelude::*; use primitives::v1::{ CandidateCommitments, CandidateDescriptor, ValidatorIndex, Id as ParaId, - AvailabilityBitfield as AvailabilityBitfield, SignedAvailabilityBitfields, SigningContext, + AvailabilityBitfield as AvailabilityBitfield, UncheckedSignedAvailabilityBitfields, SigningContext, BackedCandidate, CoreIndex, GroupIndex, CommittedCandidateReceipt, CandidateReceipt, HeadData, CandidateHash, }; @@ -236,7 +236,7 @@ impl Module { /// becoming available. pub(crate) fn process_bitfields( expected_bits: usize, - signed_bitfields: SignedAvailabilityBitfields, + unchecked_bitfields: UncheckedSignedAvailabilityBitfields, core_lookup: impl Fn(CoreIndex) -> Option, ) -> Result, DispatchError> { let validators = shared::Module::::active_validator_keys(); @@ -247,12 +247,13 @@ impl Module { .map(|core_para| core_para.map(|p| (p, PendingAvailability::::get(&p)))) .collect(); + // do sanity checks on the bitfields: // 1. no more than one bitfield per validator // 2. bitfields are ascending by validator index. // 3. each bitfield has exactly `expected_bits` // 4. signature is valid. - { + let signed_bitfields = { let occupied_bitmask: BitVec = assigned_paras_record.iter() .map(|p| p.as_ref() .map_or(false, |(_id, pending_availability)| pending_availability.is_some()) @@ -266,37 +267,42 @@ impl Module { session_index, }; - for signed_bitfield in &signed_bitfields { + let mut signed_bitfields = Vec::with_capacity(unchecked_bitfields.len()); + + for unchecked_bitfield in unchecked_bitfields { ensure!( - signed_bitfield.payload().0.len() == expected_bits, + unchecked_bitfield.unchecked_payload().0.len() == expected_bits, Error::::WrongBitfieldSize, ); ensure!( - last_index.map_or(true, |last| last < signed_bitfield.validator_index()), + last_index.map_or(true, |last| last < unchecked_bitfield.unchecked_validator_index()), Error::::BitfieldDuplicateOrUnordered, ); ensure!( - (signed_bitfield.validator_index().0 as usize) < validators.len(), + (unchecked_bitfield.unchecked_validator_index().0 as usize) < validators.len(), Error::::ValidatorIndexOutOfBounds, ); ensure!( - occupied_bitmask.clone() & signed_bitfield.payload().0.clone() == signed_bitfield.payload().0, + occupied_bitmask.clone() & unchecked_bitfield.unchecked_payload().0.clone() == unchecked_bitfield.unchecked_payload().0, Error::::UnoccupiedBitInBitfield, ); - let validator_public = &validators[signed_bitfield.validator_index().0 as usize]; + let validator_public = &validators[unchecked_bitfield.unchecked_validator_index().0 as usize]; - signed_bitfield.check_signature( - &signing_context, - validator_public, - ).map_err(|_| Error::::InvalidBitfieldSignature)?; + last_index = Some(unchecked_bitfield.unchecked_validator_index()); - last_index = Some(signed_bitfield.validator_index()); + signed_bitfields.push( + unchecked_bitfield.into_checked( + &signing_context, + validator_public, + ).ok_or(Error::::InvalidBitfieldSignature)? + ); } - } + signed_bitfields + }; let now = >::block_number(); for signed_bitfield in signed_bitfields { @@ -902,7 +908,7 @@ mod tests { use std::sync::Arc; use futures::executor::block_on; - use primitives::v0::PARACHAIN_KEY_TYPE_ID; + use primitives::{v0::PARACHAIN_KEY_TYPE_ID, v1::UncheckedSignedAvailabilityBitfield}; use primitives::v1::{BlockNumber, Hash}; use primitives::v1::{ SignedAvailabilityBitfield, CompactStatement as Statement, ValidityAttestation, CollatorId, @@ -1257,7 +1263,7 @@ mod tests { assert!(Inclusion::process_bitfields( expected_bits(), - vec![signed], + vec![signed.into()], &core_lookup, ).is_err()); } @@ -1275,7 +1281,7 @@ mod tests { assert!(Inclusion::process_bitfields( expected_bits() + 1, - vec![signed], + vec![signed.into()], &core_lookup, ).is_err()); } @@ -1283,13 +1289,14 @@ mod tests { // duplicate. { let bare_bitfield = default_bitfield(); - let signed = block_on(sign_bitfield( - &keystore, - &validators[0], - ValidatorIndex(0), - bare_bitfield, - &signing_context, - )); + let signed: UncheckedSignedAvailabilityBitfield = + block_on(sign_bitfield( + &keystore, + &validators[0], + ValidatorIndex(0), + bare_bitfield, + &signing_context, + )).into(); assert!(Inclusion::process_bitfields( expected_bits(), @@ -1307,7 +1314,7 @@ mod tests { ValidatorIndex(0), bare_bitfield.clone(), &signing_context, - )); + )).into(); let signed_1 = block_on(sign_bitfield( &keystore, @@ -1315,7 +1322,7 @@ mod tests { ValidatorIndex(1), bare_bitfield, &signing_context, - )); + )).into(); assert!(Inclusion::process_bitfields( expected_bits(), @@ -1338,7 +1345,7 @@ mod tests { assert!(Inclusion::process_bitfields( expected_bits(), - vec![signed], + vec![signed.into()], &core_lookup, ).is_err()); } @@ -1356,7 +1363,7 @@ mod tests { assert!(Inclusion::process_bitfields( expected_bits(), - vec![signed], + vec![signed.into()], &core_lookup, ).is_ok()); } @@ -1391,7 +1398,7 @@ mod tests { assert!(Inclusion::process_bitfields( expected_bits(), - vec![signed], + vec![signed.into()], &core_lookup, ).is_ok()); @@ -1430,7 +1437,7 @@ mod tests { assert_eq!( Inclusion::process_bitfields( expected_bits(), - vec![signed], + vec![signed.into()], &core_lookup, ), Ok(vec![]), @@ -1549,7 +1556,7 @@ mod tests { ValidatorIndex(i as _), to_sign, &signing_context, - ))) + )).into()) }).collect(); assert!(Inclusion::process_bitfields( From 91fade486e74a51210db27e446d01144df3fbd67 Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Wed, 28 Apr 2021 18:17:55 +0200 Subject: [PATCH 05/26] More unchecked variants. --- node/primitives/src/lib.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/node/primitives/src/lib.rs b/node/primitives/src/lib.rs index 8c45e843556d..a59f6cd1e05d 100644 --- a/node/primitives/src/lib.rs +++ b/node/primitives/src/lib.rs @@ -33,7 +33,7 @@ pub use sp_consensus_babe::{ Epoch as BabeEpoch, BabeEpochConfiguration, AllowedSlots as BabeAllowedSlots, }; -use polkadot_primitives::v1::{CandidateCommitments, CandidateHash, CollatorPair, CommittedCandidateReceipt, CompactStatement, EncodeAs, Hash, HeadData, Id as ParaId, OutboundHrmpMessage, PersistedValidationData, Signed, UpwardMessage, ValidationCode, BlakeTwo256, HashT, ValidatorIndex}; +use polkadot_primitives::v1::{BlakeTwo256, CandidateCommitments, CandidateHash, CollatorPair, CommittedCandidateReceipt, CompactStatement, EncodeAs, Hash, HashT, HeadData, Id as ParaId, OutboundHrmpMessage, PersistedValidationData, Signed, UncheckedSigned, UpwardMessage, ValidationCode, ValidatorIndex}; pub use polkadot_parachain::primitives::BlockData; pub mod approval; @@ -114,6 +114,9 @@ impl EncodeAs for Statement { /// Only the compact `SignedStatement` is suitable for submission to the chain. pub type SignedFullStatement = Signed; +/// Variant of `SignedFullStatement` where the signature has not yet been verified. +pub type UncheckedSignedFullStatement = UncheckedSigned; + /// Candidate invalidity details #[derive(Debug)] pub enum InvalidCandidate { From 26f09e231f46f7fc77fe611bd7b829bda57c4d50 Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Wed, 28 Apr 2021 18:18:36 +0200 Subject: [PATCH 06/26] Use unchecked variants in protocols. --- node/network/protocol/src/lib.rs | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/node/network/protocol/src/lib.rs b/node/network/protocol/src/lib.rs index 33cb2034adfd..5fbcb5c04c8b 100644 --- a/node/network/protocol/src/lib.rs +++ b/node/network/protocol/src/lib.rs @@ -291,18 +291,23 @@ pub mod v1 { use parity_scale_codec::{Encode, Decode}; use std::convert::TryFrom; - use polkadot_primitives::v1::{CandidateHash, CandidateIndex, CollatorId, CollatorSignature, CompactStatement, Hash, Id as ParaId, SignedAvailabilityBitfield, ValidatorIndex, ValidatorSignature}; + use polkadot_primitives::v1::{ + CandidateHash, CandidateIndex, CollatorId, CollatorSignature, + CompactStatement, Hash, Id as ParaId, UncheckedSignedAvailabilityBitfield, + ValidatorIndex, ValidatorSignature + }; use polkadot_node_primitives::{ approval::{IndirectAssignmentCert, IndirectSignedApprovalVote}, - SignedFullStatement, + UncheckedSignedFullStatement, }; + /// Network messages used by the bitfield distribution subsystem. #[derive(Debug, Clone, Encode, Decode, PartialEq, Eq)] pub enum BitfieldDistributionMessage { /// A signed availability bitfield for a given relay-parent hash. #[codec(index = 0)] - Bitfield(Hash, SignedAvailabilityBitfield), + Bitfield(Hash, UncheckedSignedAvailabilityBitfield), } /// Network messages used by the statement distribution subsystem. @@ -310,7 +315,7 @@ pub mod v1 { pub enum StatementDistributionMessage { /// A signed full statement under a given relay-parent. #[codec(index = 0)] - Statement(Hash, SignedFullStatement), + Statement(Hash, UncheckedSignedFullStatement), /// Seconded statement with large payload (e.g. containing a runtime upgrade). /// /// We only gossip the hash in that case, actual payloads can be fetched from sending node @@ -338,9 +343,9 @@ pub mod v1 { match self { Self::Statement(relay_parent, statement) => StatementMetadata { relay_parent: *relay_parent, - candidate_hash: statement.payload().candidate_hash(), - signed_by: statement.validator_index(), - signature: statement.signature().clone(), + candidate_hash: statement.unchecked_payload().candidate_hash(), + signed_by: statement.unchecked_validator_index(), + signature: statement.unchecked_signature().clone(), }, Self::LargeStatement(metadata) => metadata.clone(), } @@ -350,7 +355,7 @@ pub mod v1 { pub fn get_fingerprint(&self) -> (CompactStatement, ValidatorIndex) { match self { Self::Statement(_, statement) => - (statement.payload().to_compact(), statement.validator_index()), + (statement.unchecked_payload().to_compact(), statement.unchecked_validator_index()), Self::LargeStatement(meta) => (CompactStatement::Seconded(meta.candidate_hash), meta.signed_by), } @@ -400,7 +405,7 @@ pub mod v1 { AdvertiseCollation(Hash), /// A collation sent to a validator was seconded. #[codec(index = 4)] - CollationSeconded(SignedFullStatement), + CollationSeconded(UncheckedSignedFullStatement), } /// All network messages on the validation peer-set. From 56f71b2c5a3ef62150cd39372f6f50a9dc3c964f Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Wed, 28 Apr 2021 18:19:02 +0200 Subject: [PATCH 07/26] Start fixing statement-distribution. --- .../network/statement-distribution/src/lib.rs | 89 +++++++------------ 1 file changed, 32 insertions(+), 57 deletions(-) diff --git a/node/network/statement-distribution/src/lib.rs b/node/network/statement-distribution/src/lib.rs index b7ddb74dff37..77b513a5486c 100644 --- a/node/network/statement-distribution/src/lib.rs +++ b/node/network/statement-distribution/src/lib.rs @@ -37,7 +37,7 @@ use polkadot_node_subsystem_util::{ metrics::{self, prometheus}, self as util, MIN_GOSSIP_PEERS, }; -use polkadot_node_primitives::{SignedFullStatement, Statement}; +use polkadot_node_primitives::{SignedFullStatement, UncheckedSignedFullStatement, Statement}; use polkadot_primitives::v1::{ CandidateHash, CommittedCandidateReceipt, CompactStatement, Hash, SigningContext, ValidatorId, ValidatorIndex, ValidatorSignature, AuthorityDiscoveryId, @@ -720,15 +720,15 @@ impl ActiveHeadData { /// Returns an error if the statement is already known or not useful /// without modifying the internal state. - fn check_useful_or_unknown(&self, statement: SignedFullStatement) + fn check_useful_or_unknown(&self, statement: UncheckedSignedFullStatement) -> std::result::Result<(), DeniedStatement> { - let validator_index = statement.validator_index(); - let compact = statement.payload().to_compact(); + let validator_index = statement.unchecked_validator_index(); + let compact = statement.unchecked_payload().to_compact(); let comparator = StoredStatementComparator { compact: compact.clone(), validator_index, - signature: statement.signature().clone(), + signature: statement.unchecked_signature().clone(), }; let stored = StoredStatement { @@ -801,16 +801,16 @@ impl ActiveHeadData { fn check_statement_signature( head: &ActiveHeadData, relay_parent: Hash, - statement: &SignedFullStatement, -) -> std::result::Result<(), ()> { + statement: &UncheckedSignedFullStatement, +) -> std::result::Result { let signing_context = SigningContext { session_index: head.session_index, parent_hash: relay_parent, }; - head.validators.get(statement.validator_index().0 as usize) + head.validators.get(statement.unchecked_validator_index().0 as usize) .ok_or(()) - .and_then(|v| statement.check_signature(&signing_context, v)) + .and_then(|v| statement.into_checked(&signing_context, v)) } /// Places the statement in storage if it is new, and then @@ -887,7 +887,7 @@ fn statement_message(relay_parent: Hash, statement: SignedFullStatement) } ) } else { - protocol_v1::StatementDistributionMessage::Statement(relay_parent, statement) + protocol_v1::StatementDistributionMessage::Statement(relay_parent, statement.into()) }; protocol_v1::ValidationProtocol::StatementDistribution(msg) @@ -1092,7 +1092,7 @@ async fn retrieve_statement_from_message<'a>( ctx: &mut impl SubsystemContext, req_sender: &mpsc::Sender, metrics: &Metrics, -) -> Option { +) -> Option { let fingerprint = message.get_fingerprint(); let candidate_hash = *fingerprint.0.candidate_hash(); @@ -1100,7 +1100,7 @@ async fn retrieve_statement_from_message<'a>( // Immediately return any Seconded statement: let message = if let protocol_v1::StatementDistributionMessage::Statement(h, s) = message { - if let Statement::Seconded(_) = s.payload() { + if let Statement::Seconded(_) = s.unchecked_payload() { return Some(s) } protocol_v1::StatementDistributionMessage::Statement(h, s) @@ -1148,40 +1148,12 @@ async fn retrieve_statement_from_message<'a>( return Some(s) } protocol_v1::StatementDistributionMessage::LargeStatement(metadata) => { - - let validator_id = active_head.validators.get(metadata.signed_by.0 as usize); - - if let Some(validator_id) = validator_id { - let signing_context = SigningContext { - session_index: active_head.session_index, - parent_hash: metadata.relay_parent, - }; - - let statement = SignedFullStatement::new( - Statement::Seconded(committed.clone()), + return Some(UncheckedSignedFullStatement::new( + Statement::Seconded( + committed.clone()), metadata.signed_by, metadata.signature.clone(), - &signing_context, - validator_id, - ); - - if let Some(statement) = statement { - return Some(statement) - } else { - tracing::debug!( - target: LOG_TARGET, - validator_index = ?metadata.signed_by, - "Building statement failed - invalid signature!" - ); - report_peer(ctx, peer, COST_INVALID_SIGNATURE).await; - } - } else { - tracing::debug!( - target: LOG_TARGET, - validator_index = ?metadata.signed_by, - "Error loading statement, could not find key for validator." - ); - } + )) } } } @@ -1368,16 +1340,19 @@ async fn handle_incoming_message<'a>( } // check the signature on the statement. - if let Err(()) = check_statement_signature(&active_head, relay_parent, &statement) { - tracing::debug!( - target: LOG_TARGET, - ?peer, - ?statement, - "Invalid statement signature" - ); - report_peer(ctx, peer, COST_INVALID_SIGNATURE).await; - return None; - } + let statement = match check_statement_signature(&active_head, relay_parent, &statement) { + Err(()) => { + tracing::debug!( + target: LOG_TARGET, + ?peer, + ?statement, + "Invalid statement signature" + ); + report_peer(ctx, peer, COST_INVALID_SIGNATURE).await; + return None + } + Ok(statement) => statement, + }; // Ensure the statement is stored in the peer data. // @@ -2746,7 +2721,7 @@ mod tests { msg: StatementDistributionMessage::NetworkBridgeUpdateV1( NetworkBridgeEvent::PeerMessage( peer_a.clone(), - protocol_v1::StatementDistributionMessage::Statement(hash_a, statement.clone()), + protocol_v1::StatementDistributionMessage::Statement(hash_a, statement.clone().into()), ) ) }).await; @@ -2777,7 +2752,7 @@ mod tests { ) => { assert_eq!(recipients, vec![peer_b.clone()]); assert_eq!(r, hash_a); - assert_eq!(s, statement); + assert_eq!(s, statement.into()); } ); handle.send(FromOverseer::Signal(OverseerSignal::Conclude)).await; @@ -2951,7 +2926,7 @@ mod tests { }; let metadata = - protocol_v1::StatementDistributionMessage::Statement(hash_a, statement.clone()).get_metadata(); + protocol_v1::StatementDistributionMessage::Statement(hash_a, statement.clone().into()).get_metadata(); handle.send(FromOverseer::Communication { msg: StatementDistributionMessage::NetworkBridgeUpdateV1( From e615e8a6aeae48c109d252583b098bc61147cf9e Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Wed, 28 Apr 2021 19:55:43 +0200 Subject: [PATCH 08/26] Fixup statement distribution. --- .../network/statement-distribution/src/lib.rs | 220 ++++++++---------- primitives/src/v0.rs | 8 +- 2 files changed, 104 insertions(+), 124 deletions(-) diff --git a/node/network/statement-distribution/src/lib.rs b/node/network/statement-distribution/src/lib.rs index 77b513a5486c..96cf7e0fd467 100644 --- a/node/network/statement-distribution/src/lib.rs +++ b/node/network/statement-distribution/src/lib.rs @@ -54,7 +54,7 @@ use polkadot_node_network_protocol::{ use futures::{channel::mpsc, future::RemoteHandle, prelude::*}; use futures::channel::oneshot; -use indexmap::{IndexSet, IndexMap, map::Entry as IEntry}; +use indexmap::{IndexMap, map::Entry as IEntry}; use sp_keystore::SyncCryptoStorePtr; use util::{Fault, runtime::RuntimeInfo}; @@ -461,10 +461,10 @@ impl PeerData { } // A statement stored while a relay chain head is active. -#[derive(Debug)] -struct StoredStatement { - comparator: StoredStatementComparator, - statement: SignedFullStatement, +#[derive(Debug, Copy, Clone)] +struct StoredStatement<'a> { + comparator: &'a StoredStatementComparator, + statement: &'a SignedFullStatement, } // A value used for comparison of stored statements to each other. @@ -480,40 +480,26 @@ struct StoredStatementComparator { signature: ValidatorSignature, } -impl StoredStatement { - fn compact(&self) -> &CompactStatement { - &self.comparator.compact - } - - fn fingerprint(&self) -> (CompactStatement, ValidatorIndex) { - (self.comparator.compact.clone(), self.statement.validator_index()) +impl<'a> From<(&'a StoredStatementComparator, &'a SignedFullStatement)> for StoredStatement<'a> { + fn from((comparator, statement): (&'a StoredStatementComparator, &'a SignedFullStatement)) -> Self { + Self { comparator, statement } } } -impl std::borrow::Borrow for StoredStatement { - fn borrow(&self) -> &StoredStatementComparator { - &self.comparator - } -} - -impl std::hash::Hash for StoredStatement { - fn hash(&self, state: &mut H) { - self.comparator.hash(state) +impl<'a> StoredStatement<'a> { + fn compact(&self) -> &'a CompactStatement { + &self.comparator.compact } -} -impl std::cmp::PartialEq for StoredStatement { - fn eq(&self, other: &Self) -> bool { - &self.comparator == &other.comparator + fn fingerprint(&self) -> (CompactStatement, ValidatorIndex) { + (self.comparator.compact.clone(), self.statement.validator_index()) } } -impl std::cmp::Eq for StoredStatement {} - #[derive(Debug)] enum NotedStatement<'a> { NotUseful, - Fresh(&'a StoredStatement), + Fresh(StoredStatement<'a>), UsefulButKnown } @@ -588,7 +574,7 @@ struct ActiveHeadData { /// /// These are iterable in insertion order, and `Seconded` statements are always /// accepted before dependent statements. - statements: IndexSet, + statements: IndexMap, /// Large statements we are waiting for with associated meta data. waiting_large_statements: HashMap, /// The validators at this head. @@ -641,11 +627,6 @@ impl ActiveHeadData { signature: statement.signature().clone(), }; - let stored = StoredStatement { - comparator: comparator.clone(), - statement, - }; - match comparator.compact { CompactStatement::Seconded(h) => { let seconded_so_far = self.seconded_counts.entry(validator_index).or_insert(0); @@ -653,34 +634,36 @@ impl ActiveHeadData { tracing::trace!( target: LOG_TARGET, ?validator_index, - statement = ?stored.statement, + ?statement, "Extra statement is ignored" ); return NotedStatement::NotUseful; } self.candidates.insert(h); - if self.statements.insert(stored) { - *seconded_so_far += 1; - + if let Some(old) = self.statements.insert(comparator.clone(), statement) { tracing::trace!( target: LOG_TARGET, ?validator_index, - statement = ?self.statements.last().expect("Just inserted").statement, - "Noted new statement" + statement = ?old, + "Known statement" ); - // This will always return `Some` because it was just inserted. - NotedStatement::Fresh(self.statements.get(&comparator) - .expect("Statement was just inserted; qed")) + NotedStatement::UsefulButKnown } else { + *seconded_so_far += 1; + tracing::trace!( target: LOG_TARGET, ?validator_index, - statement = ?self.statements.get(&comparator) - .expect("Existence was just checked; qed").statement, - "Known statement" + statement = ?self.statements.last().expect("Just inserted").1, + "Noted new statement" ); - NotedStatement::UsefulButKnown + // This will always return `Some` because it was just inserted. + let key_value = self.statements + .get_key_value(&comparator) + .expect("Statement was just inserted; qed"); + + NotedStatement::Fresh(key_value.into()) } } CompactStatement::Valid(h) => { @@ -688,31 +671,34 @@ impl ActiveHeadData { tracing::trace!( target: LOG_TARGET, ?validator_index, - statement = ?stored.statement, + ?statement, "Statement for unknown candidate" ); return NotedStatement::NotUseful; } - if self.statements.insert(stored) { + if let Some(old) = self.statements.insert(comparator.clone(), statement) { tracing::trace!( target: LOG_TARGET, ?validator_index, - statement = ?self.statements.last().expect("Just inserted").statement, - "Noted new statement" + statement = ?old, + "Known statement" ); - // This will always return `Some` because it was just inserted. - NotedStatement::Fresh(self.statements.get(&comparator) - .expect("Statement was just inserted; qed")) + NotedStatement::UsefulButKnown } else { tracing::trace!( target: LOG_TARGET, ?validator_index, - statement = ?self.statements.get(&comparator) - .expect("Existence was just checked; qed").statement, - "Known statement" + statement = ?self.statements.last().expect("Just inserted").1, + "Noted new statement" ); - NotedStatement::UsefulButKnown + // This will always return `Some` because it was just inserted. + NotedStatement::Fresh( + self.statements + .get_key_value(&comparator) + .expect("Statement was just inserted; qed") + .into() + ) } } } @@ -720,7 +706,7 @@ impl ActiveHeadData { /// Returns an error if the statement is already known or not useful /// without modifying the internal state. - fn check_useful_or_unknown(&self, statement: UncheckedSignedFullStatement) + fn check_useful_or_unknown(&self, statement: &UncheckedSignedFullStatement) -> std::result::Result<(), DeniedStatement> { let validator_index = statement.unchecked_validator_index(); @@ -731,11 +717,6 @@ impl ActiveHeadData { signature: statement.unchecked_signature().clone(), }; - let stored = StoredStatement { - comparator, - statement, - }; - match compact { CompactStatement::Seconded(_) => { let seconded_so_far = self.seconded_counts.get(&validator_index).unwrap_or(&0); @@ -743,17 +724,17 @@ impl ActiveHeadData { tracing::trace!( target: LOG_TARGET, ?validator_index, - statement = ?stored.statement, + ?statement, "Extra statement is ignored", ); return Err(DeniedStatement::NotUseful); } - if self.statements.contains(&stored) { + if self.statements.contains_key(&comparator) { tracing::trace!( target: LOG_TARGET, ?validator_index, - statement = ?stored.statement, + ?statement, "Known statement", ); return Err(DeniedStatement::UsefulButKnown); @@ -764,17 +745,17 @@ impl ActiveHeadData { tracing::trace!( target: LOG_TARGET, ?validator_index, - statement = ?stored.statement, + ?statement, "Statement for unknown candidate", ); return Err(DeniedStatement::NotUseful); } - if self.statements.contains(&stored) { + if self.statements.contains_key(&comparator) { tracing::trace!( target: LOG_TARGET, ?validator_index, - statement = ?stored.statement, + ?statement, "Known statement", ); return Err(DeniedStatement::UsefulButKnown); @@ -785,14 +766,13 @@ impl ActiveHeadData { } /// Get an iterator over all statements for the active head. Seconded statements come first. - fn statements(&self) -> impl Iterator + '_ { - self.statements.iter() + fn statements(&self) -> impl Iterator> + '_ { + self.statements.iter().map(Into::into) } /// Get an iterator over all statements for the active head that are for a particular candidate. fn statements_about(&self, candidate_hash: CandidateHash) - -> impl Iterator + '_ - { + -> impl Iterator> + '_ { self.statements().filter(move |s| s.compact().candidate_hash() == &candidate_hash) } } @@ -801,15 +781,16 @@ impl ActiveHeadData { fn check_statement_signature( head: &ActiveHeadData, relay_parent: Hash, - statement: &UncheckedSignedFullStatement, -) -> std::result::Result { + statement: UncheckedSignedFullStatement, +) -> std::result::Result { let signing_context = SigningContext { session_index: head.session_index, parent_hash: relay_parent, }; - head.validators.get(statement.unchecked_validator_index().0 as usize) - .ok_or(()) + head.validators + .get(statement.unchecked_validator_index().0 as usize) + .ok_or_else(|| statement.clone()) .and_then(|v| statement.into_checked(&signing_context, v)) } @@ -919,11 +900,11 @@ fn is_statement_large(statement: &SignedFullStatement) -> bool { /// Circulates a statement to all peers who have not seen it yet, and returns /// an iterator over peers who need to have dependent statements sent. #[tracing::instrument(level = "trace", skip(peers, ctx), fields(subsystem = LOG_TARGET))] -async fn circulate_statement( +async fn circulate_statement<'a>( peers: &mut HashMap, ctx: &mut impl SubsystemContext, relay_parent: Hash, - stored: &StoredStatement, + stored: StoredStatement<'a>, mut priority_peers: Vec, ) -> Vec { let fingerprint = stored.fingerprint(); @@ -1281,7 +1262,7 @@ async fn handle_incoming_message<'a>( message: protocol_v1::StatementDistributionMessage, req_sender: &mpsc::Sender, metrics: &Metrics, -) -> Option<(Hash, &'a StoredStatement)> { +) -> Option<(Hash, StoredStatement<'a>)> { let relay_parent = message.get_relay_parent(); let active_head = match active_heads.get_mut(&relay_parent) { @@ -1328,7 +1309,7 @@ async fn handle_incoming_message<'a>( metrics, ).await?; - match active_head.check_useful_or_unknown(statement.clone()) { + match active_head.check_useful_or_unknown(&statement) { Ok(()) => {}, Err(DeniedStatement::NotUseful) => { return None; @@ -1340,8 +1321,8 @@ async fn handle_incoming_message<'a>( } // check the signature on the statement. - let statement = match check_statement_signature(&active_head, relay_parent, &statement) { - Err(()) => { + let statement = match check_statement_signature(&active_head, relay_parent, statement) { + Err(statement) => { tracing::debug!( target: LOG_TARGET, ?peer, @@ -2091,14 +2072,14 @@ mod tests { ValidatorIndex(0), &alice_public.into(), )).ok().flatten().expect("should be signed"); - assert!(head_data.check_useful_or_unknown(a_seconded_val_0.clone()).is_ok()); + assert!(head_data.check_useful_or_unknown(&a_seconded_val_0.clone().into()).is_ok()); let noted = head_data.note_statement(a_seconded_val_0.clone()); assert_matches!(noted, NotedStatement::Fresh(_)); // note A (duplicate) assert_eq!( - head_data.check_useful_or_unknown(a_seconded_val_0.clone()), + head_data.check_useful_or_unknown(&a_seconded_val_0.clone().into()), Err(DeniedStatement::UsefulButKnown), ); let noted = head_data.note_statement(a_seconded_val_0); @@ -2113,7 +2094,7 @@ mod tests { ValidatorIndex(0), &alice_public.into(), )).ok().flatten().expect("should be signed"); - assert!(head_data.check_useful_or_unknown(statement.clone()).is_ok()); + assert!(head_data.check_useful_or_unknown(&statement.clone().into()).is_ok()); let noted = head_data.note_statement(statement); assert_matches!(noted, NotedStatement::Fresh(_)); @@ -2126,7 +2107,7 @@ mod tests { &alice_public.into(), )).ok().flatten().expect("should be signed"); assert_eq!( - head_data.check_useful_or_unknown(statement.clone()), + head_data.check_useful_or_unknown(&statement.clone().into()), Err(DeniedStatement::NotUseful), ); let noted = head_data.note_statement(statement); @@ -2140,7 +2121,7 @@ mod tests { ValidatorIndex(1), &bob_public.into(), )).ok().flatten().expect("should be signed"); - assert!(head_data.check_useful_or_unknown(statement.clone()).is_ok()); + assert!(head_data.check_useful_or_unknown(&statement.clone().into()).is_ok()); let noted = head_data.note_statement(statement); assert_matches!(noted, NotedStatement::Fresh(_)); @@ -2152,7 +2133,7 @@ mod tests { ValidatorIndex(1), &bob_public.into(), )).ok().flatten().expect("should be signed"); - assert!(head_data.check_useful_or_unknown(statement.clone()).is_ok()); + assert!(head_data.check_useful_or_unknown(&statement.clone().into()).is_ok()); let noted = head_data.note_statement(statement); assert_matches!(noted, NotedStatement::Fresh(_)); } @@ -2381,7 +2362,7 @@ mod tests { ValidatorIndex(0), &alice_public.into(), )).ok().flatten().expect("should be signed"); - assert!(data.check_useful_or_unknown(statement.clone()).is_ok()); + assert!(data.check_useful_or_unknown(&statement.clone().into()).is_ok()); let noted = data.note_statement(statement); assert_matches!(noted, NotedStatement::Fresh(_)); @@ -2393,7 +2374,7 @@ mod tests { ValidatorIndex(1), &bob_public.into(), )).ok().flatten().expect("should be signed"); - assert!(data.check_useful_or_unknown(statement.clone()).is_ok()); + assert!(data.check_useful_or_unknown(&statement.clone().into()).is_ok()); let noted = data.note_statement(statement); assert_matches!(noted, NotedStatement::Fresh(_)); @@ -2405,7 +2386,7 @@ mod tests { ValidatorIndex(2), &charlie_public.into(), )).ok().flatten().expect("should be signed"); - assert!(data.check_useful_or_unknown(statement.clone()).is_ok()); + assert!(data.check_useful_or_unknown(&statement.clone().into()).is_ok()); let noted = data.note_statement(statement); assert_matches!(noted, NotedStatement::Fresh(_)); @@ -2528,40 +2509,39 @@ mod tests { ::(pool); executor::block_on(async move { - let statement = { - let signing_context = SigningContext { - parent_hash: hash_b, - session_index, - }; + let signing_context = SigningContext { + parent_hash: hash_b, + session_index, + }; - let keystore: SyncCryptoStorePtr = Arc::new(LocalKeystore::in_memory()); - let alice_public = CryptoStore::sr25519_generate_new( - &*keystore, ValidatorId::ID, Some(&Sr25519Keyring::Alice.to_seed()) - ).await.unwrap(); + let keystore: SyncCryptoStorePtr = Arc::new(LocalKeystore::in_memory()); + let alice_public = CryptoStore::sr25519_generate_new( + &*keystore, ValidatorId::ID, Some(&Sr25519Keyring::Alice.to_seed()) + ).await.unwrap(); - let statement = SignedFullStatement::sign( - &keystore, - Statement::Seconded(candidate), - &signing_context, - ValidatorIndex(0), - &alice_public.into(), - ).await.ok().flatten().expect("should be signed"); - - StoredStatement { - comparator: StoredStatementComparator { - compact: statement.payload().to_compact(), - validator_index: ValidatorIndex(0), - signature: statement.signature().clone() - }, - statement, - } + let statement = SignedFullStatement::sign( + &keystore, + Statement::Seconded(candidate), + &signing_context, + ValidatorIndex(0), + &alice_public.into(), + ).await.ok().flatten().expect("should be signed"); + + let comparator = StoredStatementComparator { + compact: statement.payload().to_compact(), + validator_index: ValidatorIndex(0), + signature: statement.signature().clone() + }; + let statement = StoredStatement { + comparator: &comparator, + statement: &statement, }; let needs_dependents = circulate_statement( &mut peer_data, &mut ctx, hash_b, - &statement, + statement, Vec::new(), ).await; @@ -3429,7 +3409,7 @@ mod tests { }; let metadata = - protocol_v1::StatementDistributionMessage::Statement(hash_a, statement.clone()).get_metadata(); + protocol_v1::StatementDistributionMessage::Statement(hash_a, statement.clone().into()).get_metadata(); handle.send(FromOverseer::Communication { msg: StatementDistributionMessage::Share(hash_a, statement.clone()) diff --git a/primitives/src/v0.rs b/primitives/src/v0.rs index 54d316bb298d..4402ca705f42 100644 --- a/primitives/src/v0.rs +++ b/primitives/src/v0.rs @@ -965,11 +965,11 @@ impl, RealPayload: Encode> Signed, context: &SigningContext, key: &ValidatorId - ) -> Option { + ) -> Result> { if unchecked.0.check_signature(context, key).is_ok() { - Some(Self(unchecked.0)) + Ok(Self(unchecked.0)) } else { - None + Err(unchecked) } } @@ -1028,7 +1028,7 @@ impl, RealPayload: Encode> UncheckedSigned, key: &ValidatorId - ) -> Option> { + ) -> Result, Self> { Signed::from_unchecked(self, context, key) } From 6739a36e975bfd09e587b07404ce9aca2195f3bc Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Wed, 28 Apr 2021 21:24:52 +0200 Subject: [PATCH 09/26] Fix inclusion. --- runtime/parachains/src/inclusion.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/runtime/parachains/src/inclusion.rs b/runtime/parachains/src/inclusion.rs index f8b6ed16681e..18751d155c09 100644 --- a/runtime/parachains/src/inclusion.rs +++ b/runtime/parachains/src/inclusion.rs @@ -298,7 +298,7 @@ impl Module { unchecked_bitfield.into_checked( &signing_context, validator_public, - ).ok_or(Error::::InvalidBitfieldSignature)? + ).map_err(|_| Error::::InvalidBitfieldSignature)? ); } signed_bitfields From 0c8ee1dbe53f31902830fe1268443c2f1672826a Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Wed, 28 Apr 2021 21:25:00 +0200 Subject: [PATCH 10/26] Fix warning. --- node/core/backing/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/node/core/backing/src/lib.rs b/node/core/backing/src/lib.rs index 111cc10b3f5d..ac7708198468 100644 --- a/node/core/backing/src/lib.rs +++ b/node/core/backing/src/lib.rs @@ -29,7 +29,7 @@ use sp_keystore::SyncCryptoStorePtr; use polkadot_primitives::v1::{ BackedCandidate, CandidateCommitments, CandidateDescriptor, CandidateHash, CandidateReceipt, CollatorId, CommittedCandidateReceipt, CoreIndex, CoreState, Hash, Id as ParaId, - SigningContext, ValidatorId, ValidatorIndex, ValidatorSignature, ValidityAttestation, + ValidatorId, ValidatorIndex, ValidatorSignature, ValidityAttestation, }; use polkadot_node_primitives::{ Statement, SignedFullStatement, ValidationResult, PoV, AvailableData, From ef67d582c60709ad0a6ffe9c492a816ad9f180bb Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Wed, 28 Apr 2021 21:36:36 +0200 Subject: [PATCH 11/26] Fix backing properly. --- node/core/backing/src/lib.rs | 35 ++++------------------------------- 1 file changed, 4 insertions(+), 31 deletions(-) diff --git a/node/core/backing/src/lib.rs b/node/core/backing/src/lib.rs index ac7708198468..49d739ff2c3f 100644 --- a/node/core/backing/src/lib.rs +++ b/node/core/backing/src/lib.rs @@ -29,7 +29,7 @@ use sp_keystore::SyncCryptoStorePtr; use polkadot_primitives::v1::{ BackedCandidate, CandidateCommitments, CandidateDescriptor, CandidateHash, CandidateReceipt, CollatorId, CommittedCandidateReceipt, CoreIndex, CoreState, Hash, Id as ParaId, - ValidatorId, ValidatorIndex, ValidatorSignature, ValidityAttestation, + SigningContext, ValidatorId, ValidatorIndex, ValidatorSignature, ValidityAttestation, }; use polkadot_node_primitives::{ Statement, SignedFullStatement, ValidationResult, PoV, AvailableData, @@ -195,7 +195,6 @@ const fn group_quorum(n_validators: usize) -> usize { #[derive(Default)] struct TableContext { - signing_context: SigningContext, validator: Option, groups: HashMap>, validators: Vec, @@ -1187,7 +1186,6 @@ impl util::JobTrait for CandidateBackingJob { let table_context = TableContext { groups, validators, - signing_context, validator, }; @@ -1641,14 +1639,9 @@ mod tests { AllMessages::StatementDistribution( StatementDistributionMessage::Share( parent_hash, - signed_statement, + _signed_statement, ) - ) if parent_hash == test_state.relay_parent => { - signed_statement.check_signature( - &test_state.signing_context, - &test_state.validator_public[0], - ).unwrap(); - } + ) if parent_hash == test_state.relay_parent => {} ); assert_matches!( @@ -1691,11 +1684,6 @@ mod tests { }.build(); let candidate_a_hash = candidate_a.hash(); - let public0 = CryptoStore::sr25519_generate_new( - &*test_state.keystore, - ValidatorId::ID, - Some(&test_state.validators[0].to_seed()), - ).await.expect("Insert key into keystore"); let public1 = CryptoStore::sr25519_generate_new( &*test_state.keystore, ValidatorId::ID, @@ -1778,10 +1766,9 @@ mod tests { assert_matches!( virtual_overseer.recv().await, AllMessages::StatementDistribution( - StatementDistributionMessage::Share(hash, stmt) + StatementDistributionMessage::Share(hash, _stmt) ) => { assert_eq!(test_state.relay_parent, hash); - stmt.check_signature(&test_state.signing_context, &public0.into()).expect("Is signed correctly"); } ); @@ -2075,11 +2062,6 @@ mod tests { signed_statement, ) ) if relay_parent == test_state.relay_parent => { - signed_statement.check_signature( - &test_state.signing_context, - &test_state.validator_public[0], - ).unwrap(); - assert_eq!(*signed_statement.payload(), Statement::Valid(candidate_a_hash)); } ); @@ -2240,11 +2222,6 @@ mod tests { signed_statement, ) ) if parent_hash == test_state.relay_parent => { - signed_statement.check_signature( - &test_state.signing_context, - &test_state.validator_public[0], - ).unwrap(); - assert_eq!(*signed_statement.payload(), Statement::Seconded(candidate_b)); } ); @@ -2576,10 +2553,7 @@ mod tests { use sp_core::Encode; use std::convert::TryFrom; - let relay_parent = [1; 32].into(); let para_id = ParaId::from(10); - let session_index = 5; - let signing_context = SigningContext { parent_hash: relay_parent, session_index }; let validators = vec![ Sr25519Keyring::Alice, Sr25519Keyring::Bob, @@ -2597,7 +2571,6 @@ mod tests { }; let table_context = TableContext { - signing_context, validator: None, groups: validator_groups, validators: validator_public.clone(), From 999e161a5681c0c189b025dbd054c360bb4c4973 Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Wed, 28 Apr 2021 22:07:58 +0200 Subject: [PATCH 12/26] Fix bitfield distribution. --- node/network/bitfield-distribution/src/lib.rs | 69 +++++++++---------- primitives/src/v0.rs | 9 +++ 2 files changed, 42 insertions(+), 36 deletions(-) diff --git a/node/network/bitfield-distribution/src/lib.rs b/node/network/bitfield-distribution/src/lib.rs index dbe9a61cf564..b009d55cf1cc 100644 --- a/node/network/bitfield-distribution/src/lib.rs +++ b/node/network/bitfield-distribution/src/lib.rs @@ -69,7 +69,7 @@ impl BitfieldGossipMessage { { protocol_v1::BitfieldDistributionMessage::Bitfield( self.relay_parent, - self.signed_availability, + self.signed_availability.into(), ) } } @@ -392,19 +392,26 @@ async fn process_incoming_peer_message( state: &mut ProtocolState, metrics: &Metrics, origin: PeerId, - message: BitfieldGossipMessage, + message: protocol_v1::BitfieldDistributionMessage, ) where Context: SubsystemContext, { + let protocol_v1::BitfieldDistributionMessage::Bitfield(relay_parent, bitfield) = message; + tracing::trace!( + target: LOG_TARGET, + peer_id = %origin, + ?relay_parent, + "received bitfield gossip from peer" + ); // we don't care about this, not part of our view. - if !state.view.contains(&message.relay_parent) { + if !state.view.contains(&relay_parent) { modify_reputation(ctx, origin, COST_NOT_IN_VIEW).await; return; } // Ignore anything the overseer did not tell this subsystem to work on. - let mut job_data = state.per_relay_parent.get_mut(&message.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 { @@ -412,17 +419,19 @@ where return; }; + let validator_index = bitfield.unchecked_validator_index(); + let mut _span = job_data.span .child("msg-received") .with_peer_id(&origin) - .with_claimed_validator_index(message.signed_availability.validator_index()) + .with_claimed_validator_index(validator_index) .with_stage(jaeger::Stage::BitfieldDistribution); let validator_set = &job_data.validator_set; if validator_set.is_empty() { tracing::trace!( target: LOG_TARGET, - relay_parent = %message.relay_parent, + relay_parent = %relay_parent, ?origin, "Validator set is empty", ); @@ -433,8 +442,7 @@ where // 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().0 as usize; - let validator = if let Some(validator) = validator_set.get(validator_index) { + let validator = if let Some(validator) = validator_set.get(validator_index.0 as usize) { validator.clone() } else { modify_reputation(ctx, origin, COST_VALIDATOR_INDEX_INVALID).await; @@ -454,7 +462,7 @@ where } else { tracing::trace!( target: LOG_TARGET, - validator_index, + ?validator_index, ?origin, "Duplicate message", ); @@ -468,22 +476,26 @@ where if let Some(old_message) = one_per_validator.get(&validator) { tracing::trace!( target: LOG_TARGET, - validator_index, + ?validator_index, "already received a message for validator", ); - if old_message.signed_availability == message.signed_availability { + if old_message.signed_availability.equals_unchecked(&bitfield) { modify_reputation(ctx, origin, BENEFIT_VALID_MESSAGE).await; } return; } - if message - .signed_availability - .check_signature(&signing_context, &validator) - .is_err() - { - modify_reputation(ctx, origin, COST_SIGNATURE_INVALID).await; - return; - } + let signed_availability = match bitfield.into_checked(&signing_context, &validator) { + Err(_) => { + modify_reputation(ctx, origin, COST_SIGNATURE_INVALID).await; + return; + }, + Ok(bitfield) => bitfield, + }; + + let message = BitfieldGossipMessage { + relay_parent, + signed_availability, + }; metrics.on_bitfield_received(); one_per_validator.insert(validator.clone(), message.clone()); @@ -544,23 +556,8 @@ where ); handle_our_view_change(state, view); } - NetworkBridgeEvent::PeerMessage(remote, message) => { - match message { - protocol_v1::BitfieldDistributionMessage::Bitfield(relay_parent, bitfield) => { - tracing::trace!( - target: LOG_TARGET, - peer_id = %remote, - ?relay_parent, - "received bitfield gossip from peer" - ); - let gossiped_bitfield = BitfieldGossipMessage { - relay_parent, - signed_availability: bitfield, - }; - process_incoming_peer_message(ctx, state, metrics, remote, gossiped_bitfield).await; - } - } - } + NetworkBridgeEvent::PeerMessage(remote, message) => + process_incoming_peer_message(ctx, state, metrics, remote, message).await, } } diff --git a/primitives/src/v0.rs b/primitives/src/v0.rs index 4402ca705f42..d1f6faebf3d2 100644 --- a/primitives/src/v0.rs +++ b/primitives/src/v0.rs @@ -973,6 +973,15 @@ impl, RealPayload: Encode> Signed) -> bool + where + Payload: PartialEq + Eq, + RealPayload: PartialEq + Eq, + { + self.0 == unchecked.0 + } + /// Immutably access the payload. #[inline] pub fn payload(&self) -> &Payload { From 8e6976144435ddcaca8430402e4f82ba1ca0ca69 Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Wed, 28 Apr 2021 22:34:37 +0200 Subject: [PATCH 13/26] Make crypto store optional for `RuntimeInfo`. --- node/network/availability-distribution/src/lib.rs | 2 +- .../availability-distribution/src/pov_requester/mod.rs | 2 +- node/network/statement-distribution/src/lib.rs | 2 +- node/subsystem-util/src/runtime/mod.rs | 7 ++++--- 4 files changed, 7 insertions(+), 6 deletions(-) diff --git a/node/network/availability-distribution/src/lib.rs b/node/network/availability-distribution/src/lib.rs index f0c80eb2b2d9..9ebc0af130b9 100644 --- a/node/network/availability-distribution/src/lib.rs +++ b/node/network/availability-distribution/src/lib.rs @@ -85,7 +85,7 @@ impl AvailabilityDistributionSubsystem { /// Create a new instance of the availability distribution. pub fn new(keystore: SyncCryptoStorePtr, metrics: Metrics) -> Self { - let runtime = RuntimeInfo::new(keystore.clone()); + let runtime = RuntimeInfo::new(Some(keystore.clone())); Self { keystore, runtime, metrics } } diff --git a/node/network/availability-distribution/src/pov_requester/mod.rs b/node/network/availability-distribution/src/pov_requester/mod.rs index e53fdd4b241f..a7d856fefbca 100644 --- a/node/network/availability-distribution/src/pov_requester/mod.rs +++ b/node/network/availability-distribution/src/pov_requester/mod.rs @@ -280,7 +280,7 @@ mod tests { let (mut context, mut virtual_overseer) = test_helpers::make_subsystem_context::(pool.clone()); let keystore = make_ferdie_keystore(); - let mut runtime = polkadot_node_subsystem_util::runtime::RuntimeInfo::new(keystore); + let mut runtime = polkadot_node_subsystem_util::runtime::RuntimeInfo::new(Some(keystore)); let (tx, rx) = oneshot::channel(); let testee = async { diff --git a/node/network/statement-distribution/src/lib.rs b/node/network/statement-distribution/src/lib.rs index 96cf7e0fd467..729ae9bfe2ab 100644 --- a/node/network/statement-distribution/src/lib.rs +++ b/node/network/statement-distribution/src/lib.rs @@ -1509,7 +1509,7 @@ impl StatementDistribution { let mut authorities: HashMap = HashMap::new(); let mut active_heads: HashMap = HashMap::new(); - let mut runtime = RuntimeInfo::new(self.keystore.clone()); + let mut runtime = RuntimeInfo::new(Some(self.keystore.clone())); // Sender/Receiver for getting news from our statement fetching tasks. let (req_sender, mut req_receiver) = mpsc::channel(1); diff --git a/node/subsystem-util/src/runtime/mod.rs b/node/subsystem-util/src/runtime/mod.rs index 0012c9f6b441..f8f9c7cc58a1 100644 --- a/node/subsystem-util/src/runtime/mod.rs +++ b/node/subsystem-util/src/runtime/mod.rs @@ -49,7 +49,7 @@ pub struct RuntimeInfo { session_info_cache: LruCache, /// Key store for determining whether we are a validator and what `ValidatorIndex` we have. - keystore: SyncCryptoStorePtr, + keystore: Option, } /// SessionInfo with additional useful data for validator nodes. @@ -72,7 +72,7 @@ pub struct ValidatorInfo { impl RuntimeInfo { /// Create a new `RuntimeInfo` for convenient runtime fetches. - pub fn new(keystore: SyncCryptoStorePtr) -> Self { + pub fn new(keystore: Option) -> Self { Self { // Adjust, depending on how many forks we want to support. session_index_cache: LruCache::new(10), @@ -187,8 +187,9 @@ impl RuntimeInfo { /// /// Returns: None if we are not a validator. async fn get_our_index(&self, validators: &[ValidatorId]) -> Option { + let keystore = self.keystore.as_ref()?; for (i, v) in validators.iter().enumerate() { - if CryptoStore::has_keys(&*self.keystore, &[(v.to_raw_vec(), ValidatorId::ID)]) + if CryptoStore::has_keys(&**keystore, &[(v.to_raw_vec(), ValidatorId::ID)]) .await { return Some(ValidatorIndex(i as u32)); From 1fe106e3e68b800e51d125439163d3471f5a4402 Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Wed, 28 Apr 2021 23:34:35 +0200 Subject: [PATCH 14/26] Factor out utility functions. --- .../src/requester/mod.rs | 32 +++-------------- node/subsystem-util/src/runtime/mod.rs | 35 ++++++++++++++++++- 2 files changed, 39 insertions(+), 28 deletions(-) diff --git a/node/network/availability-distribution/src/requester/mod.rs b/node/network/availability-distribution/src/requester/mod.rs index eaef4e5f3cc6..052b7cded431 100644 --- a/node/network/availability-distribution/src/requester/mod.rs +++ b/node/network/availability-distribution/src/requester/mod.rs @@ -32,14 +32,14 @@ use futures::{ use sp_keystore::SyncCryptoStorePtr; -use polkadot_node_subsystem_util::request_availability_cores; -use polkadot_primitives::v1::{CandidateHash, CoreState, Hash, OccupiedCore}; +use polkadot_node_subsystem_util::runtime::get_occupied_cores; +use polkadot_primitives::v1::{CandidateHash, Hash, OccupiedCore}; use polkadot_subsystem::{ messages::AllMessages, ActiveLeavesUpdate, SubsystemContext, ActivatedLeaf, }; -use super::{error::recv_runtime, session_cache::SessionCache, LOG_TARGET, Metrics}; -use crate::error::Error; +use super::{session_cache::SessionCache, LOG_TARGET, Metrics}; + /// A task fetching a particular chunk. mod fetch_task; @@ -125,7 +125,7 @@ impl Requester { Context: SubsystemContext, { for ActivatedLeaf { hash: leaf, .. } in new_heads { - let cores = query_occupied_cores(ctx, leaf).await?; + let cores = get_occupied_cores(ctx, leaf).await?; tracing::trace!( target: LOG_TARGET, occupied_cores = ?cores, @@ -226,25 +226,3 @@ impl Stream for Requester { } } -/// Query all hashes and descriptors of candidates pending availability at a particular block. -#[tracing::instrument(level = "trace", skip(ctx), fields(subsystem = LOG_TARGET))] -async fn query_occupied_cores( - ctx: &mut Context, - relay_parent: Hash, -) -> Result, Error> -where - Context: SubsystemContext, -{ - let cores = recv_runtime(request_availability_cores(relay_parent, ctx.sender()).await).await?; - - Ok(cores - .into_iter() - .filter_map(|core_state| { - if let CoreState::Occupied(occupied) = core_state { - Some(occupied) - } else { - None - } - }) - .collect()) -} diff --git a/node/subsystem-util/src/runtime/mod.rs b/node/subsystem-util/src/runtime/mod.rs index f8f9c7cc58a1..bbaf6e84f0d9 100644 --- a/node/subsystem-util/src/runtime/mod.rs +++ b/node/subsystem-util/src/runtime/mod.rs @@ -22,11 +22,12 @@ use sp_application_crypto::AppKey; use sp_core::crypto::Public; use sp_keystore::{CryptoStore, SyncCryptoStorePtr}; -use polkadot_primitives::v1::{GroupIndex, Hash, SessionIndex, SessionInfo, ValidatorId, ValidatorIndex}; +use polkadot_primitives::v1::{CoreState, GroupIndex, Hash, OccupiedCore, SessionIndex, SessionInfo, ValidatorId, ValidatorIndex}; use polkadot_node_subsystem::SubsystemContext; use crate::{ request_session_index_for_child, request_session_info, + request_availability_cores, }; /// Errors that can happen on runtime fetches. @@ -198,3 +199,35 @@ impl RuntimeInfo { None } } + +/// Request availability cores from the runtime. +pub async fn get_availability_cores(ctx: &mut Context, relay_parent: Hash) + -> Result> + where + Context: SubsystemContext, +{ + recv_runtime(request_availability_cores(relay_parent, ctx.sender()).await).await +} + +/// Variant of `request_availability_cores` that only returns occupied ones. +pub async fn get_occupied_cores( + ctx: &mut Context, + relay_parent: Hash, +) -> Result> +where + Context: SubsystemContext, +{ + let cores = get_availability_cores(ctx, relay_parent).await?; + + Ok(cores + .into_iter() + .filter_map(|core_state| { + if let CoreState::Occupied(occupied) = core_state { + Some(occupied) + } else { + None + } + }) + .collect() + ) +} From ad91cfee7cd57580279e0417a245dec4cb351fcb Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Thu, 29 Apr 2021 14:12:02 +0200 Subject: [PATCH 15/26] get_group_rotation_info --- node/subsystem-util/src/runtime/mod.rs | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/node/subsystem-util/src/runtime/mod.rs b/node/subsystem-util/src/runtime/mod.rs index bbaf6e84f0d9..37a7c225fd0c 100644 --- a/node/subsystem-util/src/runtime/mod.rs +++ b/node/subsystem-util/src/runtime/mod.rs @@ -22,12 +22,13 @@ use sp_application_crypto::AppKey; use sp_core::crypto::Public; use sp_keystore::{CryptoStore, SyncCryptoStorePtr}; -use polkadot_primitives::v1::{CoreState, GroupIndex, Hash, OccupiedCore, SessionIndex, SessionInfo, ValidatorId, ValidatorIndex}; +use polkadot_primitives::v1::{CoreState, GroupIndex, GroupRotationInfo, Hash, OccupiedCore, SessionIndex, SessionInfo, ValidatorId, ValidatorIndex}; use polkadot_node_subsystem::SubsystemContext; use crate::{ request_session_index_for_child, request_session_info, request_availability_cores, + request_validator_groups, }; /// Errors that can happen on runtime fetches. @@ -231,3 +232,15 @@ where .collect() ) } + +/// Get group rotation info based on the given relay_parent. +pub async fn get_group_rotation_info(ctx: &mut Context, relay_parent: Hash) + -> Result + where + Context: SubsystemContext +{ + // We drop `groups` here as we don't need them, because of `RuntimeInfo`. Ideally we would not + // fetch them in the first place. + let (_, info) = recv_runtime(request_validator_groups(relay_parent, ctx.sender()).await).await?; + Ok(info) +} From 114dea1e85780ad2463732de48f90f94a0f241b8 Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Thu, 29 Apr 2021 14:12:23 +0200 Subject: [PATCH 16/26] WIP: Collator cleanup + check signatures. --- .../collator-protocol/src/collator_side.rs | 208 ++++++++---------- node/network/collator-protocol/src/error.rs | 98 +++++++++ node/network/collator-protocol/src/lib.rs | 22 +- .../collator-protocol/src/validator_side.rs | 2 +- 4 files changed, 197 insertions(+), 133 deletions(-) create mode 100644 node/network/collator-protocol/src/error.rs diff --git a/node/network/collator-protocol/src/collator_side.rs b/node/network/collator-protocol/src/collator_side.rs index 99f6344aa98b..3e4055a5182c 100644 --- a/node/network/collator-protocol/src/collator_side.rs +++ b/node/network/collator-protocol/src/collator_side.rs @@ -18,29 +18,22 @@ use std::collections::{HashMap, HashSet}; use super::{LOG_TARGET, Result}; -use futures::{select, FutureExt, channel::oneshot}; +use futures::{select, FutureExt, channel::oneshot, channel::mpsc}; use sp_core::Pair; use polkadot_primitives::v1::{ CandidateHash, CandidateReceipt, CollatorPair, CoreIndex, CoreState, Hash, - Id as ParaId, ValidatorId -}; -use polkadot_subsystem::{ - jaeger, PerLeafSpan, - FromOverseer, OverseerSignal, SubsystemContext, - messages::{AllMessages, CollatorProtocolMessage, NetworkBridgeMessage, NetworkBridgeEvent}, + Id as ParaId, AuthorityDiscoveryId, }; +use polkadot_subsystem::{FromOverseer, OverseerSignal, PerLeafSpan, SubsystemContext, jaeger, messages::{AllMessages, CollatorProtocolMessage, NetworkBridgeEvent, NetworkBridgeMessage}}; use polkadot_node_network_protocol::{ OurView, PeerId, View, peer_set::PeerSet, request_response::{IncomingRequest, v1::{CollationFetchingRequest, CollationFetchingResponse}}, v1 as protocol_v1, UnifiedReputationChange as Rep, }; use polkadot_node_subsystem_util::{ - validator_discovery, - request_validators, - request_validator_groups, - request_availability_cores, metrics::{self, prometheus}, + runtime::{RuntimeInfo, get_availability_cores, get_group_rotation_info}, }; use polkadot_node_primitives::{SignedFullStatement, Statement, PoV}; @@ -130,49 +123,34 @@ impl metrics::Metrics for Metrics { /// stores a mapping from [`PeerId`] to [`ValidatorId`] as we learn about it over the lifetime of this object. Besides /// that it also keeps track to which validators we advertised our collation. struct ValidatorGroup { - /// All [`ValidatorId`]'s that are assigned to us in this group. - validator_ids: HashSet, - /// The mapping from [`PeerId`] to [`ValidatorId`]. This is filled over time as we learn the [`PeerId`]'s from the - /// authority discovery. It is not ensured that this will contain *all* validators of this group. - peer_ids: HashMap, + /// All [`AuthorityDiscoveryId`]'s that are assigned to us in this group. + discovery_ids: HashSet, /// All [`ValidatorId`]'s of the current group to that we advertised our collation. - advertised_to: HashSet, + advertised_to: HashSet, } impl ValidatorGroup { /// Returns `true` if we should advertise our collation to the given peer. - fn should_advertise_to(&self, peer: &PeerId) -> bool { - match self.peer_ids.get(peer) { - Some(validator_id) => !self.advertised_to.contains(validator_id), + fn should_advertise_to(&self, peer_ids: &HashMap, peer: &PeerId) + -> bool { + match peer_ids.get(peer) { + Some(discovery_id) => !self.advertised_to.contains(discovery_id), None => false, } } /// Should be called after we advertised our collation to the given `peer` to keep track of it. - fn advertised_to_peer(&mut self, peer: &PeerId) { - if let Some(validator_id) = self.peer_ids.get(peer) { + fn advertised_to_peer(&mut self, peer_ids: &HashMap, peer: &PeerId) { + if let Some(validator_id) = peer_ids.get(peer) { self.advertised_to.insert(validator_id.clone()); } } - - /// Add a [`PeerId`] that belongs to the given [`ValidatorId`]. - /// - /// This returns `true` if the given validator belongs to this group and we could insert its [`PeerId`]. - fn add_peer_id_for_validator(&mut self, peer_id: &PeerId, validator_id: &ValidatorId) -> bool { - if !self.validator_ids.contains(validator_id) { - false - } else { - self.peer_ids.insert(peer_id.clone(), validator_id.clone()); - true - } - } } -impl From> for ValidatorGroup { - fn from(validator_ids: HashSet) -> Self { +impl From> for ValidatorGroup { + fn from(discovery_ids: HashSet) -> Self { Self { - validator_ids, - peer_ids: HashMap::new(), + discovery_ids, advertised_to: HashSet::new(), } } @@ -243,8 +221,11 @@ struct State { /// Our validator groups per active leaf. our_validators_groups: HashMap, - /// The connection requests to validators per relay parent. - connection_requests: validator_discovery::ConnectionRequests, + /// The mapping from [`PeerId`] to [`ValidatorId`]. This is filled over time as we learn the [`PeerId`]'s by `PeerConnected` events. + peer_ids: HashMap, + + /// The connection handles to validators per relay parent. + connection_handles: HashMap>>, /// Metrics. metrics: Metrics, @@ -265,7 +246,7 @@ impl State { collations: Default::default(), collation_result_senders: Default::default(), our_validators_groups: Default::default(), - connection_requests: Default::default(), + connection_handles: Default::default(), } } @@ -273,6 +254,15 @@ impl State { fn peer_interested_in_leaf(&self, peer: &PeerId, relay_parent: &Hash) -> bool { self.peer_views.get(peer).map(|v| v.contains(relay_parent)).unwrap_or(false) } + + /// Get all peers which have the given relay parent in their view. + fn peers_interested_in_leaf(&self, relay_parent: &Hash) -> Vec { + self.peer_views + .iter() + .filter(|(peer, v)| v.contains(relay_parent)) + .map(|(peer, _)| *peer) + .collect() + } } /// Distribute a collation. @@ -283,9 +273,10 @@ impl State { /// or the relay-parent isn't in the active-leaves set, we ignore the message /// as it must be invalid in that case - although this indicates a logic error /// elsewhere in the node. -#[tracing::instrument(level = "trace", skip(ctx, state, pov), fields(subsystem = LOG_TARGET))] +#[tracing::instrument(level = "trace", skip(ctx, runtime, state, pov), fields(subsystem = LOG_TARGET))] async fn distribute_collation( - ctx: &mut impl SubsystemContext, + ctx: &mut impl SubsystemContext, + runtime: &mut RuntimeInfo, state: &mut State, id: ParaId, receipt: CandidateReceipt, @@ -327,7 +318,7 @@ async fn distribute_collation( }; // Determine the group on that core and the next group on that core. - let (current_validators, next_validators) = determine_our_validators(ctx, our_core, num_cores, relay_parent).await?; + let (current_validators, next_validators) = determine_our_validators(ctx, runtime, our_core, num_cores, relay_parent).await?; if current_validators.is_empty() && next_validators.is_empty() { tracing::warn!( @@ -357,7 +348,7 @@ async fn distribute_collation( id, state, current_validators.union(&next_validators).cloned().collect(), - ).await?; + ).await; state.our_validators_groups.insert(relay_parent, current_validators.into()); @@ -367,6 +358,11 @@ async fn distribute_collation( state.collations.insert(relay_parent, Collation { receipt, pov, status: CollationStatus::Created }); + // Make sure already connected peers get collations: + for peer_id in state.peers_interested_in_leaf(&relay_parent) { + advertise_collation(ctx, state, relay_parent, peer_id).await; + } + Ok(()) } @@ -374,11 +370,11 @@ async fn distribute_collation( /// and the total number of cores. #[tracing::instrument(level = "trace", skip(ctx), fields(subsystem = LOG_TARGET))] async fn determine_core( - ctx: &mut impl SubsystemContext, + ctx: &mut impl SubsystemContext, para_id: ParaId, relay_parent: Hash, ) -> Result> { - let cores = request_availability_cores(relay_parent, ctx.sender()).await.await??; + let cores = get_availability_cores(ctx, relay_parent).await?; for (idx, core) in cores.iter().enumerate() { if let CoreState::Scheduled(occupied) = core { @@ -387,31 +383,31 @@ async fn determine_core( } } } - Ok(None) } /// Figure out current and next group of validators assigned to the para being collated on. /// /// Returns [`ValidatorId`]'s of current and next group as determined based on the `relay_parent`. -#[tracing::instrument(level = "trace", skip(ctx), fields(subsystem = LOG_TARGET))] +#[tracing::instrument(level = "trace", skip(ctx, runtime), fields(subsystem = LOG_TARGET))] async fn determine_our_validators( - ctx: &mut impl SubsystemContext, + ctx: &mut impl SubsystemContext, + runtime: &mut RuntimeInfo, core_index: CoreIndex, cores: usize, relay_parent: Hash, -) -> Result<(HashSet, HashSet)> { - let groups = request_validator_groups(relay_parent, ctx.sender()).await; - - let groups = groups.await??; +) -> Result<(HashSet, HashSet)> { + let info = &runtime.get_session_info(ctx, relay_parent).await?.session_info; + let groups = &info.validator_groups; + let rotation_info = get_group_rotation_info(ctx, relay_parent).await?; - let current_group_index = groups.1.group_for_core(core_index, cores); - let current_validators = groups.0.get(current_group_index.0 as usize).map(|v| v.as_slice()).unwrap_or_default(); + let current_group_index = rotation_info.group_for_core(core_index, cores); + let current_validators = groups.get(current_group_index.0 as usize).map(|v| v.as_slice()).unwrap_or_default(); - let next_group_idx = (current_group_index.0 as usize + 1) % groups.0.len(); - let next_validators = groups.0.get(next_group_idx).map(|v| v.as_slice()).unwrap_or_default(); + let next_group_idx = (current_group_index.0 as usize + 1) % groups.len(); + let next_validators = groups.get(next_group_idx).map(|v| v.as_slice()).unwrap_or_default(); - let validators = request_validators(relay_parent, ctx.sender()).await.await??; + let validators = &info.discovery_keys; let current_validators = current_validators.iter().map(|i| validators[i.0 as usize].clone()).collect(); let next_validators = next_validators.iter().map(|i| validators[i.0 as usize].clone()).collect(); @@ -448,22 +444,27 @@ async fn declare( /// revoke the previous connection request. #[tracing::instrument(level = "trace", skip(ctx, state), fields(subsystem = LOG_TARGET))] async fn connect_to_validators( - ctx: &mut impl SubsystemContext, + ctx: &mut impl SubsystemContext, relay_parent: Hash, para_id: ParaId, state: &mut State, - validators: Vec, -) -> Result<()> { - let request = validator_discovery::connect_to_validators( - ctx, - relay_parent, - validators, - PeerSet::Collation, - ).await?; - - state.connection_requests.put(relay_parent, para_id, request); - - Ok(()) + validator_ids: Vec, +) { + let (tx, rx) = mpsc::channel(0); + ctx.send_message(AllMessages::NetworkBridge(NetworkBridgeMessage::ConnectToValidators { + validator_ids, peer_set: PeerSet::Collation, connected: tx + })).await; + + if let Some(old) = state.connection_handles + .entry(relay_parent) + .or_insert(Default::default()) + .insert(para_id, rx) { + tracing::debug!( + target: LOG_TARGET, + ?old, + "Connection handle for para was already present" + ); + } } /// Advertise collation to the given `peer`. @@ -472,14 +473,14 @@ async fn connect_to_validators( /// set as validator for our para at the given `relay_parent`. #[tracing::instrument(level = "trace", skip(ctx, state), fields(subsystem = LOG_TARGET))] async fn advertise_collation( - ctx: &mut impl SubsystemContext, + ctx: &mut impl SubsystemContext, state: &mut State, relay_parent: Hash, peer: PeerId, ) { let should_advertise = state.our_validators_groups .get(&relay_parent) - .map(|g| g.should_advertise_to(&peer)) + .map(|g| g.should_advertise_to(&state.peer_ids, &peer)) .unwrap_or(false); match (state.collations.get_mut(&relay_parent), should_advertise) { @@ -524,16 +525,17 @@ async fn advertise_collation( )).await; if let Some(validators) = state.our_validators_groups.get_mut(&relay_parent) { - validators.advertised_to_peer(&peer); + validators.advertised_to_peer(&state.peer_ids, &peer); } state.metrics.on_advertisment_made(); } /// The main incoming message dispatching switch. -#[tracing::instrument(level = "trace", skip(ctx, state), fields(subsystem = LOG_TARGET))] +#[tracing::instrument(level = "trace", skip(ctx, runtime, state), fields(subsystem = LOG_TARGET))] async fn process_msg( ctx: &mut impl SubsystemContext, + runtime: &mut RuntimeInfo, state: &mut State, msg: CollatorProtocolMessage, ) -> Result<()> { @@ -561,7 +563,7 @@ async fn process_msg( ); } Some(id) => { - distribute_collation(ctx, state, id, receipt, pov, result_sender).await?; + distribute_collation(ctx, runtime, state, id, receipt, pov, result_sender).await?; } None => { tracing::warn!( @@ -750,37 +752,6 @@ async fn handle_peer_view_change( } } -/// A validator is connected. -/// -/// `Declare` that we are a collator with a given `CollatorId`. -#[tracing::instrument(level = "trace", skip(ctx, state), fields(subsystem = LOG_TARGET))] -async fn handle_validator_connected( - ctx: &mut impl SubsystemContext, - state: &mut State, - peer_id: PeerId, - validator_id: ValidatorId, - relay_parent: Hash, -) { - tracing::trace!( - target: LOG_TARGET, - ?validator_id, - "Connected to requested validator" - ); - - // Store the PeerId and find out if we should advertise to this peer. - // - // If this peer does not belong to the para validators, we also don't need to try to advertise our collation. - let advertise = if let Some(validators) = state.our_validators_groups.get_mut(&relay_parent) { - validators.add_peer_id_for_validator(&peer_id, &validator_id) - } else { - false - }; - - if advertise && state.peer_interested_in_leaf(&peer_id, &relay_parent) { - advertise_collation(ctx, state, relay_parent, peer_id).await; - } -} - /// Bridge messages switch. #[tracing::instrument(level = "trace", skip(ctx, state), fields(subsystem = LOG_TARGET))] async fn handle_network_msg( @@ -791,7 +762,7 @@ async fn handle_network_msg( use NetworkBridgeEvent::*; match bridge_message { - PeerConnected(peer_id, observed_role, _) => { + PeerConnected(peer_id, observed_role, maybe_authority) => { // If it is possible that a disconnected validator would attempt a reconnect // it should be handled here. tracing::trace!( @@ -800,9 +771,17 @@ async fn handle_network_msg( ?observed_role, "Peer connected", ); + if let Some(authority) = maybe_authority { + tracing::trace!( + target: LOG_TARGET, + ?authority, + ?peer_id, + "Connected to requested validator" + ); + state.peer_ids.insert(peer_id, authority); - // Always declare to every peer. We should be connecting only to validators. - declare(ctx, state, peer_id.clone()).await; + declare(ctx, state, peer_id).await; + } } PeerViewChange(peer_id, view) => { tracing::trace!( @@ -820,6 +799,7 @@ async fn handle_network_msg( "Peer disconnected", ); state.peer_views.remove(&peer_id); + state.peer_ids.remove(&peer_id); } OurViewChange(view) => { tracing::trace!( @@ -871,7 +851,8 @@ async fn handle_our_view_change( } } state.our_validators_groups.remove(removed); - state.connection_requests.remove_all(removed); + // TODO: Make sure they stay alive until we connected to new ones. + state.connection_handles.remove(removed); state.span_per_relay_parent.remove(removed); } @@ -892,6 +873,7 @@ pub(crate) async fn run( use OverseerSignal::*; let mut state = State::new(local_peer_id, collator_pair, metrics); + let mut runtime = RuntimeInfo::new(None); loop { select! { @@ -908,7 +890,7 @@ pub(crate) async fn run( }, msg = ctx.recv().fuse() => match msg? { Communication { msg } => { - if let Err(e) = process_msg(&mut ctx, &mut state, msg).await { + if let Err(e) = process_msg(&mut ctx, &mut runtime, &mut state, msg).await { tracing::warn!(target: LOG_TARGET, err = ?e, "Failed to process message"); } }, diff --git a/node/network/collator-protocol/src/error.rs b/node/network/collator-protocol/src/error.rs new file mode 100644 index 000000000000..10b5cb45f14c --- /dev/null +++ b/node/network/collator-protocol/src/error.rs @@ -0,0 +1,98 @@ +// Copyright 2021 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 . +// + +//! Error handling related code and Error/Result definitions. + +use polkadot_node_network_protocol::PeerId; +use polkadot_primitives::v1::{CandidateHash, Hash}; +use polkadot_subsystem::SubsystemError; +use thiserror::Error; + +use polkadot_node_subsystem_util::{Fault, runtime, unwrap_non_fatal}; + +use crate::LOG_TARGET; + +/// General result. +pub type Result = std::result::Result; +/// Result for non fatal only failures. +pub type NonFatalResult = std::result::Result; +/// Result for fatal only failures. +pub type FatalResult = std::result::Result; + +/// Errors for statement distribution. +#[derive(Debug, Error)] +#[error(transparent)] +pub struct Error(pub Fault); + +impl From for Error { + fn from(e: NonFatal) -> Self { + Self(Fault::from_non_fatal(e)) + } +} + +impl From for Error { + fn from(f: Fatal) -> Self { + Self(Fault::from_fatal(f)) + } +} + +impl From for Error { + fn from(o: runtime::Error) -> Self { + Self(Fault::from_other(o)) + } +} + +/// Fatal runtime errors. +#[derive(Debug, Error)] +pub enum Fatal { + /// Spawning a running task failed. + #[error("Spawning subsystem task failed")] + SpawnTask(#[source] SubsystemError), + + /// Receiving subsystem message from overseer failed. + #[error("Receiving message from overseer failed")] + SubsystemReceive(#[source] SubsystemError), + + /// Errors coming from runtime::Runtime. + #[error("Error while accessing runtime information")] + Runtime(#[from] #[source] runtime::Fatal), +} + +/// Errors for fetching of runtime information. +#[derive(Debug, Error)] +pub enum NonFatal { + /// Relay parent was not present in active heads. + #[error("Relay parent could not be found in active heads")] + NoSuchHead(Hash), + + /// Errors coming from runtime::Runtime. + #[error("Error while accessing runtime information")] + Runtime(#[from] #[source] runtime::NonFatal), +} + +/// Utility for eating top level errors and log them. +/// +/// We basically always want to try and continue on error. This utility function is meant to +/// consume top-level errors by simply logging them. +pub fn log_error(result: Result<()>, ctx: &'static str) + -> FatalResult<()> +{ + if let Some(error) = unwrap_non_fatal(result.map_err(|e| e.0))? { + tracing::debug!(target: LOG_TARGET, error = ?error, ctx) + } + Ok(()) +} diff --git a/node/network/collator-protocol/src/lib.rs b/node/network/collator-protocol/src/lib.rs index 2f85f4c0c9e3..6e3979e3f600 100644 --- a/node/network/collator-protocol/src/lib.rs +++ b/node/network/collator-protocol/src/lib.rs @@ -36,27 +36,13 @@ use polkadot_subsystem::{ SpawnedSubsystem, Subsystem, SubsystemContext, SubsystemError, }; +mod error; +use error::{Result, Error}; mod collator_side; mod validator_side; const LOG_TARGET: &'static str = "parachain::collator-protocol"; -#[derive(Debug, Error)] -enum Error { - #[error(transparent)] - Subsystem(#[from] SubsystemError), - #[error(transparent)] - Oneshot(#[from] oneshot::Canceled), - #[error(transparent)] - RuntimeApi(#[from] RuntimeApiError), - #[error(transparent)] - UtilError(#[from] util::Error), - #[error(transparent)] - Prometheus(#[from] prometheus::PrometheusError), -} - -type Result = std::result::Result; - /// A collator eviction policy - how fast to evict collators which are inactive. #[derive(Debug, Clone, Copy)] pub struct CollatorEvictionPolicy { @@ -124,9 +110,7 @@ impl CollatorProtocolSubsystem { collator_pair, metrics, ).await, - }.map_err(|e| { - SubsystemError::with_origin("collator-protocol", e).into() - }) + } } } diff --git a/node/network/collator-protocol/src/validator_side.rs b/node/network/collator-protocol/src/validator_side.rs index ae5ee6fd12c2..44f54c0e0855 100644 --- a/node/network/collator-protocol/src/validator_side.rs +++ b/node/network/collator-protocol/src/validator_side.rs @@ -552,7 +552,7 @@ async fn notify_collation_seconded( } if let Some(peer_id) = collator_peer_id(peer_data, &id) { - let wire_message = protocol_v1::CollatorProtocolMessage::CollationSeconded(statement); + let wire_message = protocol_v1::CollatorProtocolMessage::CollationSeconded(statement.into()); ctx.send_message(AllMessages::NetworkBridge( NetworkBridgeMessage::SendCollationMessage( From 95c5e075f08a707801e1c860ccaca09365bd8a1a Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Thu, 29 Apr 2021 21:59:03 +0200 Subject: [PATCH 17/26] Convenience signature checking functions. --- node/subsystem-util/src/runtime/mod.rs | 42 +++++++++++++++++++++++++- 1 file changed, 41 insertions(+), 1 deletion(-) diff --git a/node/subsystem-util/src/runtime/mod.rs b/node/subsystem-util/src/runtime/mod.rs index 37a7c225fd0c..165eeb00228e 100644 --- a/node/subsystem-util/src/runtime/mod.rs +++ b/node/subsystem-util/src/runtime/mod.rs @@ -18,11 +18,12 @@ use lru::LruCache; +use parity_scale_codec::Encode; use sp_application_crypto::AppKey; use sp_core::crypto::Public; use sp_keystore::{CryptoStore, SyncCryptoStorePtr}; -use polkadot_primitives::v1::{CoreState, GroupIndex, GroupRotationInfo, Hash, OccupiedCore, SessionIndex, SessionInfo, ValidatorId, ValidatorIndex}; +use polkadot_primitives::v1::{CoreState, EncodeAs, GroupIndex, GroupRotationInfo, Hash, OccupiedCore, SessionIndex, SessionInfo, Signed, SigningContext, UncheckedSigned, ValidatorId, ValidatorIndex}; use polkadot_node_subsystem::SubsystemContext; use crate::{ @@ -152,6 +153,23 @@ impl RuntimeInfo { ) } + /// Convenience function for checking the signature of something signed. + pub async fn check_signature( + &mut self, + ctx: &mut Context, + parent: Hash, + signed: UncheckedSigned, + ) -> Result, UncheckedSigned>> + where + Context: SubsystemContext, + Payload: EncodeAs + Clone, + RealPayload: Encode + Clone, + { + let session_index = self.get_session_index(ctx, parent).await?; + let info = self.get_session_info_by_index(ctx, parent, session_index).await?; + Ok(check_signature(session_index, &info.session_info, parent, signed)) + } + /// Build `ValidatorInfo` for the current session. /// /// @@ -201,6 +219,28 @@ impl RuntimeInfo { } } +/// Convenience function for quickly checking the signature on signed data. +pub fn check_signature( + session_index: SessionIndex, + session_info: &SessionInfo, + relay_parent: Hash, + signed: UncheckedSigned, +) -> std::result::Result, UncheckedSigned> +where + Payload: EncodeAs + Clone, + RealPayload: Encode + Clone, +{ + let signing_context = SigningContext { + session_index, + parent_hash: relay_parent, + }; + + session_info.validators + .get(signed.unchecked_validator_index().0 as usize) + .ok_or_else(|| signed.clone()) + .and_then(|v| signed.into_checked(&signing_context, v)) +} + /// Request availability cores from the runtime. pub async fn get_availability_cores(ctx: &mut Context, relay_parent: Hash) -> Result> From 2268809136646900edf611a29b61d67a43739189 Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Thu, 29 Apr 2021 22:01:20 +0200 Subject: [PATCH 18/26] Check signature on collator-side. --- node/core/candidate-selection/src/lib.rs | 13 +- .../collator-protocol/src/collator_side.rs | 185 +++++++++--------- node/network/collator-protocol/src/error.rs | 9 +- .../collator-protocol/src/validator_side.rs | 2 +- node/network/protocol/src/lib.rs | 2 +- node/subsystem/src/messages.rs | 2 +- .../src/types/overseer-protocol.md | 8 +- 7 files changed, 113 insertions(+), 108 deletions(-) diff --git a/node/core/candidate-selection/src/lib.rs b/node/core/candidate-selection/src/lib.rs index 25180f5004b0..37433ec6345c 100644 --- a/node/core/candidate-selection/src/lib.rs +++ b/node/core/candidate-selection/src/lib.rs @@ -249,12 +249,12 @@ impl CandidateSelectionJob { .with_relay_parent(_relay_parent); self.handle_invalid(sender, candidate_receipt).await; } - Some(CandidateSelectionMessage::Seconded(_relay_parent, statement)) => { + Some(CandidateSelectionMessage::Seconded(relay_parent, statement)) => { let _span = span.child("handle-seconded") .with_stage(jaeger::Stage::CandidateSelection) .with_candidate(statement.payload().candidate_hash()) - .with_relay_parent(_relay_parent); - self.handle_seconded(sender, statement).await; + .with_relay_parent(relay_parent); + self.handle_seconded(sender, relay_parent, statement).await; } None => break, } @@ -345,6 +345,7 @@ impl CandidateSelectionJob { async fn handle_seconded( &mut self, sender: &mut impl SubsystemSender, + relay_parent: Hash, statement: SignedFullStatement, ) { let received_from = match &self.seconded_candidate { @@ -368,7 +369,11 @@ impl CandidateSelectionJob { .await; sender.send_message( - CollatorProtocolMessage::NotifyCollationSeconded(received_from.clone(), statement).into() + CollatorProtocolMessage::NotifyCollationSeconded( + received_from.clone(), + relay_parent, + statement + ).into() ).await; } } diff --git a/node/network/collator-protocol/src/collator_side.rs b/node/network/collator-protocol/src/collator_side.rs index 3e4055a5182c..32c0996d546c 100644 --- a/node/network/collator-protocol/src/collator_side.rs +++ b/node/network/collator-protocol/src/collator_side.rs @@ -14,29 +14,24 @@ // You should have received a copy of the GNU General Public License // along with Polkadot. If not, see . -use std::collections::{HashMap, HashSet}; - -use super::{LOG_TARGET, Result}; +use std::collections::{HashMap, HashSet, hash_map::Entry}; use futures::{select, FutureExt, channel::oneshot, channel::mpsc}; use sp_core::Pair; -use polkadot_primitives::v1::{ - CandidateHash, CandidateReceipt, CollatorPair, CoreIndex, CoreState, Hash, - Id as ParaId, AuthorityDiscoveryId, -}; +use polkadot_primitives::v1::{AuthorityDiscoveryId, CandidateHash, CandidateReceipt, CollatorPair, CoreIndex, CoreState, GroupIndex, Hash, Id as ParaId}; use polkadot_subsystem::{FromOverseer, OverseerSignal, PerLeafSpan, SubsystemContext, jaeger, messages::{AllMessages, CollatorProtocolMessage, NetworkBridgeEvent, NetworkBridgeMessage}}; use polkadot_node_network_protocol::{ OurView, PeerId, View, peer_set::PeerSet, request_response::{IncomingRequest, v1::{CollationFetchingRequest, CollationFetchingResponse}}, v1 as protocol_v1, UnifiedReputationChange as Rep, }; -use polkadot_node_subsystem_util::{ - metrics::{self, prometheus}, - runtime::{RuntimeInfo, get_availability_cores, get_group_rotation_info}, -}; +use polkadot_node_subsystem_util::{metrics::{self, prometheus}, runtime::{RuntimeInfo, get_availability_cores, get_group_rotation_info}}; use polkadot_node_primitives::{SignedFullStatement, Statement, PoV}; +use crate::error::{Fatal, NonFatal, log_error}; +use super::{LOG_TARGET, Result}; + const COST_UNEXPECTED_MESSAGE: Rep = Rep::CostMinor("An unexpected message"); #[derive(Clone, Default)] @@ -55,11 +50,6 @@ impl Metrics { } } - /// Provide a timer for handling `ConnectionRequest` which observes on drop. - fn time_handle_connection_request(&self) -> Option { - self.0.as_ref().map(|metrics| metrics.handle_connection_request.start_timer()) - } - /// Provide a timer for `process_msg` which observes on drop. fn time_process_msg(&self) -> Option { self.0.as_ref().map(|metrics| metrics.process_msg.start_timer()) @@ -70,7 +60,6 @@ impl Metrics { struct MetricsInner { advertisements_made: prometheus::Counter, collations_sent: prometheus::Counter, - handle_connection_request: prometheus::Histogram, process_msg: prometheus::Histogram, } @@ -93,15 +82,6 @@ impl metrics::Metrics for Metrics { )?, registry, )?, - handle_connection_request: prometheus::register( - prometheus::Histogram::with_opts( - prometheus::HistogramOpts::new( - "parachain_collator_protocol_collator_handle_connection_request", - "Time spent within `collator_protocol_collator::handle_connection_request`", - ) - )?, - registry, - )?, process_msg: prometheus::register( prometheus::Histogram::with_opts( prometheus::HistogramOpts::new( @@ -122,6 +102,7 @@ impl metrics::Metrics for Metrics { /// This structure is responsible for keeping track of which validators belong to a certain group for a para. It also /// stores a mapping from [`PeerId`] to [`ValidatorId`] as we learn about it over the lifetime of this object. Besides /// that it also keeps track to which validators we advertised our collation. +#[derive(Debug)] struct ValidatorGroup { /// All [`AuthorityDiscoveryId`]'s that are assigned to us in this group. discovery_ids: HashSet, @@ -224,8 +205,8 @@ struct State { /// The mapping from [`PeerId`] to [`ValidatorId`]. This is filled over time as we learn the [`PeerId`]'s by `PeerConnected` events. peer_ids: HashMap, - /// The connection handles to validators per relay parent. - connection_handles: HashMap>>, + /// The connection handles to validators per group we are interested in. + connection_handles: HashMap>, /// Metrics. metrics: Metrics, @@ -246,6 +227,7 @@ impl State { collations: Default::default(), collation_result_senders: Default::default(), our_validators_groups: Default::default(), + peer_ids: Default::default(), connection_handles: Default::default(), } } @@ -320,7 +302,7 @@ async fn distribute_collation( // Determine the group on that core and the next group on that core. let (current_validators, next_validators) = determine_our_validators(ctx, runtime, our_core, num_cores, relay_parent).await?; - if current_validators.is_empty() && next_validators.is_empty() { + if current_validators.validators.is_empty() && next_validators.validators.is_empty() { tracing::warn!( target: LOG_TARGET, core = ?our_core, @@ -341,16 +323,26 @@ async fn distribute_collation( ?next_validators, "Accepted collation, connecting to validators." ); - // Issue a discovery request for the validators of the current group and the next group. + + // Drop obsolete connections: + let new_groups: HashSet<_> = vec![current_validators.group, next_validators.group].into_iter().collect(); + state.connection_handles.retain(|k, _| new_groups.contains(k)); + + let validator_group: HashSet<_> = current_validators.validators.iter().map(Clone::clone).collect(); + + // Issue a discovery request for the validators of the current group and the next group: connect_to_validators( ctx, - relay_parent, - id, state, - current_validators.union(&next_validators).cloned().collect(), + current_validators, + ).await; + connect_to_validators( + ctx, + state, + next_validators, ).await; - state.our_validators_groups.insert(relay_parent, current_validators.into()); + state.our_validators_groups.insert(relay_parent, validator_group.into()); if let Some(result_sender) = result_sender { state.collation_result_senders.insert(receipt.hash(), result_sender); @@ -386,6 +378,15 @@ async fn determine_core( Ok(None) } +/// Validators of a particular group index. +#[derive(Debug)] +struct GroupValidators { + /// The group those validators belong to. + group: GroupIndex, + /// The validators of above group (their discovery keys). + validators: Vec, +} + /// Figure out current and next group of validators assigned to the para being collated on. /// /// Returns [`ValidatorId`]'s of current and next group as determined based on the `relay_parent`. @@ -396,7 +397,7 @@ async fn determine_our_validators( core_index: CoreIndex, cores: usize, relay_parent: Hash, -) -> Result<(HashSet, HashSet)> { +) -> Result<(GroupValidators, GroupValidators)> { let info = &runtime.get_session_info(ctx, relay_parent).await?.session_info; let groups = &info.validator_groups; let rotation_info = get_group_rotation_info(ctx, relay_parent).await?; @@ -412,6 +413,15 @@ async fn determine_our_validators( let current_validators = current_validators.iter().map(|i| validators[i.0 as usize].clone()).collect(); let next_validators = next_validators.iter().map(|i| validators[i.0 as usize].clone()).collect(); + let current_validators = GroupValidators { + group: current_group_index, + validators: current_validators, + }; + let next_validators = GroupValidators { + group: GroupIndex(next_group_idx as u32), + validators: next_validators, + }; + Ok((current_validators, next_validators)) } @@ -445,25 +455,18 @@ async fn declare( #[tracing::instrument(level = "trace", skip(ctx, state), fields(subsystem = LOG_TARGET))] async fn connect_to_validators( ctx: &mut impl SubsystemContext, - relay_parent: Hash, - para_id: ParaId, state: &mut State, - validator_ids: Vec, + group: GroupValidators, ) { - let (tx, rx) = mpsc::channel(0); - ctx.send_message(AllMessages::NetworkBridge(NetworkBridgeMessage::ConnectToValidators { - validator_ids, peer_set: PeerSet::Collation, connected: tx - })).await; - - if let Some(old) = state.connection_handles - .entry(relay_parent) - .or_insert(Default::default()) - .insert(para_id, rx) { - tracing::debug!( - target: LOG_TARGET, - ?old, - "Connection handle for para was already present" - ); + match state.connection_handles.entry(group.group) { + Entry::Occupied(occupied) => {} + Entry::Vacant(vacant) => { + let (tx, rx) = mpsc::channel(0); + ctx.send_message(AllMessages::NetworkBridge(NetworkBridgeMessage::ConnectToValidators { + validator_ids: group.validators, peer_set: PeerSet::Collation, connected: tx + })).await; + vacant.insert(rx); + } } } @@ -592,7 +595,7 @@ async fn process_msg( "NoteGoodCollation message is not expected on the collator side of the protocol", ); } - NotifyCollationSeconded(_, _) => { + NotifyCollationSeconded(_, _, _) => { tracing::warn!( target: LOG_TARGET, "NotifyCollationSeconded message is not expected on the collator side of the protocol", @@ -601,6 +604,7 @@ async fn process_msg( NetworkBridgeUpdateV1(event) => { if let Err(e) = handle_network_msg( ctx, + runtime, state, event, ).await { @@ -672,9 +676,10 @@ async fn send_collation( } /// A networking messages switch. -#[tracing::instrument(level = "trace", skip(ctx, state), fields(subsystem = LOG_TARGET))] +#[tracing::instrument(level = "trace", skip(ctx, runtime, state), fields(subsystem = LOG_TARGET))] async fn handle_incoming_peer_message( ctx: &mut impl SubsystemContext, + runtime: &mut RuntimeInfo, state: &mut State, origin: PeerId, msg: protocol_v1::CollatorProtocolMessage, @@ -710,22 +715,31 @@ async fn handle_incoming_peer_message( NetworkBridgeMessage::DisconnectPeer(origin, PeerSet::Collation).into() ).await; } - CollationSeconded(statement) => { - if !matches!(statement.payload(), Statement::Seconded(_)) { + CollationSeconded(relay_parent, statement) => { + if !matches!(statement.unchecked_payload(), Statement::Seconded(_)) { tracing::warn!( target: LOG_TARGET, ?statement, ?origin, "Collation seconded message received with none-seconded statement.", ); - } else if let Some(sender) = state.collation_result_senders.remove(&statement.payload().candidate_hash()) { - tracing::trace!( - target: LOG_TARGET, - ?statement, - ?origin, - "received a `CollationSeconded`", - ); - let _ = sender.send(statement); + } else { + let statement = runtime.check_signature(ctx, relay_parent, statement) + .await? + .map_err(NonFatal::InvalidStatementSignature)?; + + let removed = state.collation_result_senders + .remove(&statement.payload().candidate_hash()); + + if let Some(sender) = removed { + tracing::trace!( + target: LOG_TARGET, + ?statement, + ?origin, + "received a `CollationSeconded`", + ); + let _ = sender.send(statement); + } } } } @@ -753,9 +767,10 @@ async fn handle_peer_view_change( } /// Bridge messages switch. -#[tracing::instrument(level = "trace", skip(ctx, state), fields(subsystem = LOG_TARGET))] +#[tracing::instrument(level = "trace", skip(ctx, runtime, state), fields(subsystem = LOG_TARGET))] async fn handle_network_msg( ctx: &mut impl SubsystemContext, + runtime: &mut RuntimeInfo, state: &mut State, bridge_message: NetworkBridgeEvent, ) -> Result<()> { @@ -810,7 +825,7 @@ async fn handle_network_msg( handle_our_view_change(state, view).await?; } PeerMessage(remote, msg) => { - handle_incoming_peer_message(ctx, state, remote, msg).await?; + handle_incoming_peer_message(ctx, runtime, state, remote, msg).await?; } } @@ -851,8 +866,6 @@ async fn handle_our_view_change( } } state.our_validators_groups.remove(removed); - // TODO: Make sure they stay alive until we connected to new ones. - state.connection_handles.remove(removed); state.span_per_relay_parent.remove(removed); } @@ -876,28 +889,17 @@ pub(crate) async fn run( let mut runtime = RuntimeInfo::new(None); loop { - select! { - res = state.connection_requests.next().fuse() => { - let _timer = state.metrics.time_handle_connection_request(); - - handle_validator_connected( - &mut ctx, - &mut state, - res.peer_id, - res.validator_id, - res.relay_parent, - ).await; + let msg = ctx.recv().fuse().await.map_err(Fatal::SubsystemReceive)?; + match msg { + Communication { msg } => { + log_error( + process_msg(&mut ctx, &mut runtime, &mut state, msg).await, + "Failed to process message" + ); }, - msg = ctx.recv().fuse() => match msg? { - Communication { msg } => { - if let Err(e) = process_msg(&mut ctx, &mut runtime, &mut state, msg).await { - tracing::warn!(target: LOG_TARGET, err = ?e, "Failed to process message"); - } - }, - Signal(ActiveLeaves(_update)) => {} - Signal(BlockFinalized(..)) => {} - Signal(Conclude) => return Ok(()), - } + Signal(ActiveLeaves(_update)) => {} + Signal(BlockFinalized(..)) => {} + Signal(Conclude) => return Ok(()), } } } @@ -921,10 +923,7 @@ mod tests { request_response::request::IncomingRequest, }; use polkadot_node_subsystem_util::TimeoutExt; - use polkadot_primitives::v1::{ - AuthorityDiscoveryId, CandidateDescriptor, CollatorPair, GroupRotationInfo, - ScheduledCore, SessionIndex, SessionInfo, ValidatorIndex, - }; + use polkadot_primitives::v1::{AuthorityDiscoveryId, CandidateDescriptor, CollatorPair, GroupRotationInfo, ScheduledCore, SessionIndex, SessionInfo, ValidatorId, ValidatorIndex}; use polkadot_node_primitives::BlockData; use polkadot_subsystem::{ jaeger, diff --git a/node/network/collator-protocol/src/error.rs b/node/network/collator-protocol/src/error.rs index 10b5cb45f14c..a1af09d19e83 100644 --- a/node/network/collator-protocol/src/error.rs +++ b/node/network/collator-protocol/src/error.rs @@ -18,6 +18,7 @@ //! Error handling related code and Error/Result definitions. use polkadot_node_network_protocol::PeerId; +use polkadot_node_primitives::UncheckedSignedFullStatement; use polkadot_primitives::v1::{CandidateHash, Hash}; use polkadot_subsystem::SubsystemError; use thiserror::Error; @@ -75,9 +76,9 @@ pub enum Fatal { /// Errors for fetching of runtime information. #[derive(Debug, Error)] pub enum NonFatal { - /// Relay parent was not present in active heads. - #[error("Relay parent could not be found in active heads")] - NoSuchHead(Hash), + /// Signature was invalid on received statement. + #[error("CollationSeconded contained statement with invalid signature.")] + InvalidStatementSignature(UncheckedSignedFullStatement), /// Errors coming from runtime::Runtime. #[error("Error while accessing runtime information")] @@ -92,7 +93,7 @@ pub fn log_error(result: Result<()>, ctx: &'static str) -> FatalResult<()> { if let Some(error) = unwrap_non_fatal(result.map_err(|e| e.0))? { - tracing::debug!(target: LOG_TARGET, error = ?error, ctx) + tracing::warn!(target: LOG_TARGET, error = ?error, ctx) } Ok(()) } diff --git a/node/network/collator-protocol/src/validator_side.rs b/node/network/collator-protocol/src/validator_side.rs index 44f54c0e0855..127e098a18de 100644 --- a/node/network/collator-protocol/src/validator_side.rs +++ b/node/network/collator-protocol/src/validator_side.rs @@ -934,7 +934,7 @@ where NoteGoodCollation(id) => { note_good_collation(ctx, &state.peer_data, id).await; } - NotifyCollationSeconded(id, statement) => { + NotifyCollationSeconded(id, relay_parent, statement) => { notify_collation_seconded(ctx, &state.peer_data, id, statement).await; } NetworkBridgeUpdateV1(event) => { diff --git a/node/network/protocol/src/lib.rs b/node/network/protocol/src/lib.rs index 5fbcb5c04c8b..5f19a4af8992 100644 --- a/node/network/protocol/src/lib.rs +++ b/node/network/protocol/src/lib.rs @@ -405,7 +405,7 @@ pub mod v1 { AdvertiseCollation(Hash), /// A collation sent to a validator was seconded. #[codec(index = 4)] - CollationSeconded(UncheckedSignedFullStatement), + CollationSeconded(Hash, UncheckedSignedFullStatement), } /// All network messages on the validation peer-set. diff --git a/node/subsystem/src/messages.rs b/node/subsystem/src/messages.rs index 7d6d39938ec0..55b733864f01 100644 --- a/node/subsystem/src/messages.rs +++ b/node/subsystem/src/messages.rs @@ -200,7 +200,7 @@ pub enum CollatorProtocolMessage { /// Note a collator as having provided a good collation. NoteGoodCollation(CollatorId), /// Notify a collator that its collation was seconded. - NotifyCollationSeconded(CollatorId, SignedFullStatement), + NotifyCollationSeconded(CollatorId, Hash, SignedFullStatement), /// Get a network bridge update. #[from] NetworkBridgeUpdateV1(NetworkBridgeEvent), diff --git a/roadmap/implementers-guide/src/types/overseer-protocol.md b/roadmap/implementers-guide/src/types/overseer-protocol.md index c4d51a611042..c8b8736e2cd1 100644 --- a/roadmap/implementers-guide/src/types/overseer-protocol.md +++ b/roadmap/implementers-guide/src/types/overseer-protocol.md @@ -83,7 +83,7 @@ enum ApprovalVotingMessage { /// /// The base number is typically the number of the last finalized block, but in GRANDPA it is /// possible for the base to be slightly higher than the last finalized block. - /// + /// /// The `BlockNumber` provided is the number of the block's ancestor which is the /// earliest possible vote. /// @@ -91,7 +91,7 @@ enum ApprovalVotingMessage { /// Return `None` if the input hash is unrecognized. ApprovedAncestor { target_hash: Hash, - base_number: BlockNumber, + base_number: BlockNumber, rx: ResponseChannel)>)>> }, } @@ -334,7 +334,7 @@ enum CollatorProtocolMessage { /// Note a collator as having provided a good collation. NoteGoodCollation(CollatorId, SignedFullStatement), /// Notify a collator that its collation was seconded. - NotifyCollationSeconded(CollatorId, SignedFullStatement), + NotifyCollationSeconded(CollatorId, Hash, SignedFullStatement), } ``` @@ -378,7 +378,7 @@ enum DisputeCoordinatorMessage { /// Sign and issue local dispute votes. A value of `true` indicates validity, and `false` invalidity. IssueLocalStatement(SessionIndex, CandidateHash, CandidateReceipt, bool), /// Determine the highest undisputed block within the given chain, based on where candidates - /// were included. If even the base block should not be finalized due to a dispute, + /// were included. If even the base block should not be finalized due to a dispute, /// then `None` should be returned on the channel. /// /// The block descriptions begin counting upwards from the block after the given `base_number`. The `base_number` From 3ec469f7f69058f94933727b6a4c1afcf2bf3640 Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Thu, 29 Apr 2021 23:41:07 +0200 Subject: [PATCH 19/26] Fix warnings. --- .../collator-protocol/src/collator_side.rs | 18 ++++++++---------- node/network/collator-protocol/src/error.rs | 7 +------ node/network/collator-protocol/src/lib.rs | 8 +++----- .../collator-protocol/src/validator_side.rs | 11 +++++++---- 4 files changed, 19 insertions(+), 25 deletions(-) diff --git a/node/network/collator-protocol/src/collator_side.rs b/node/network/collator-protocol/src/collator_side.rs index 32c0996d546c..a3d57f20d02f 100644 --- a/node/network/collator-protocol/src/collator_side.rs +++ b/node/network/collator-protocol/src/collator_side.rs @@ -16,7 +16,7 @@ use std::collections::{HashMap, HashSet, hash_map::Entry}; -use futures::{select, FutureExt, channel::oneshot, channel::mpsc}; +use futures::{FutureExt, channel::oneshot, channel::mpsc}; use sp_core::Pair; use polkadot_primitives::v1::{AuthorityDiscoveryId, CandidateHash, CandidateReceipt, CollatorPair, CoreIndex, CoreState, GroupIndex, Hash, Id as ParaId}; @@ -26,7 +26,10 @@ use polkadot_node_network_protocol::{ request_response::{IncomingRequest, v1::{CollationFetchingRequest, CollationFetchingResponse}}, v1 as protocol_v1, UnifiedReputationChange as Rep, }; -use polkadot_node_subsystem_util::{metrics::{self, prometheus}, runtime::{RuntimeInfo, get_availability_cores, get_group_rotation_info}}; +use polkadot_node_subsystem_util::{ + metrics::{self, prometheus}, + runtime::{RuntimeInfo, get_availability_cores, get_group_rotation_info} +}; use polkadot_node_primitives::{SignedFullStatement, Statement, PoV}; use crate::error::{Fatal, NonFatal, log_error}; @@ -232,16 +235,11 @@ impl State { } } - /// Returns `true` if the given `peer` is interested in the leaf that is represented by `relay_parent`. - fn peer_interested_in_leaf(&self, peer: &PeerId, relay_parent: &Hash) -> bool { - self.peer_views.get(peer).map(|v| v.contains(relay_parent)).unwrap_or(false) - } - /// Get all peers which have the given relay parent in their view. fn peers_interested_in_leaf(&self, relay_parent: &Hash) -> Vec { self.peer_views .iter() - .filter(|(peer, v)| v.contains(relay_parent)) + .filter(|(_, v)| v.contains(relay_parent)) .map(|(peer, _)| *peer) .collect() } @@ -459,7 +457,7 @@ async fn connect_to_validators( group: GroupValidators, ) { match state.connection_handles.entry(group.group) { - Entry::Occupied(occupied) => {} + Entry::Occupied(_) => {} Entry::Vacant(vacant) => { let (tx, rx) = mpsc::channel(0); ctx.send_message(AllMessages::NetworkBridge(NetworkBridgeMessage::ConnectToValidators { @@ -895,7 +893,7 @@ pub(crate) async fn run( log_error( process_msg(&mut ctx, &mut runtime, &mut state, msg).await, "Failed to process message" - ); + )?; }, Signal(ActiveLeaves(_update)) => {} Signal(BlockFinalized(..)) => {} diff --git a/node/network/collator-protocol/src/error.rs b/node/network/collator-protocol/src/error.rs index a1af09d19e83..ed9e39159cdd 100644 --- a/node/network/collator-protocol/src/error.rs +++ b/node/network/collator-protocol/src/error.rs @@ -17,9 +17,7 @@ //! Error handling related code and Error/Result definitions. -use polkadot_node_network_protocol::PeerId; use polkadot_node_primitives::UncheckedSignedFullStatement; -use polkadot_primitives::v1::{CandidateHash, Hash}; use polkadot_subsystem::SubsystemError; use thiserror::Error; @@ -30,6 +28,7 @@ use crate::LOG_TARGET; /// General result. pub type Result = std::result::Result; /// Result for non fatal only failures. +#[allow(dead_code)] pub type NonFatalResult = std::result::Result; /// Result for fatal only failures. pub type FatalResult = std::result::Result; @@ -60,10 +59,6 @@ impl From for Error { /// Fatal runtime errors. #[derive(Debug, Error)] pub enum Fatal { - /// Spawning a running task failed. - #[error("Spawning subsystem task failed")] - SpawnTask(#[source] SubsystemError), - /// Receiving subsystem message from overseer failed. #[error("Receiving message from overseer failed")] SubsystemReceive(#[source] SubsystemError), diff --git a/node/network/collator-protocol/src/lib.rs b/node/network/collator-protocol/src/lib.rs index 6e3979e3f600..24ae2407d12c 100644 --- a/node/network/collator-protocol/src/lib.rs +++ b/node/network/collator-protocol/src/lib.rs @@ -22,22 +22,20 @@ use std::time::Duration; -use futures::{channel::oneshot, FutureExt, TryFutureExt}; -use thiserror::Error; +use futures::{FutureExt, TryFutureExt}; use sp_keystore::SyncCryptoStorePtr; use polkadot_node_network_protocol::{PeerId, UnifiedReputationChange as Rep}; -use polkadot_node_subsystem_util::{self as util, metrics::prometheus}; use polkadot_primitives::v1::CollatorPair; use polkadot_subsystem::{ - errors::RuntimeApiError, messages::{AllMessages, CollatorProtocolMessage, NetworkBridgeMessage}, SpawnedSubsystem, Subsystem, SubsystemContext, SubsystemError, }; mod error; -use error::{Result, Error}; +use error::Result; + mod collator_side; mod validator_side; diff --git a/node/network/collator-protocol/src/validator_side.rs b/node/network/collator-protocol/src/validator_side.rs index 127e098a18de..2a161c65d832 100644 --- a/node/network/collator-protocol/src/validator_side.rs +++ b/node/network/collator-protocol/src/validator_side.rs @@ -47,6 +47,8 @@ use polkadot_subsystem::{ FromOverseer, OverseerSignal, PerLeafSpan, SubsystemContext, SubsystemSender, }; +use crate::error::Fatal; + use super::{modify_reputation, Result, LOG_TARGET}; const COST_UNEXPECTED_MESSAGE: Rep = Rep::CostMinor("An unexpected message"); @@ -540,6 +542,7 @@ async fn notify_collation_seconded( ctx: &mut impl SubsystemContext, peer_data: &HashMap, id: CollatorId, + relay_parent: Hash, statement: SignedFullStatement, ) { if !matches!(statement.payload(), Statement::Seconded(_)) { @@ -552,7 +555,7 @@ async fn notify_collation_seconded( } if let Some(peer_id) = collator_peer_id(peer_data, &id) { - let wire_message = protocol_v1::CollatorProtocolMessage::CollationSeconded(statement.into()); + let wire_message = protocol_v1::CollatorProtocolMessage::CollationSeconded(relay_parent, statement.into()); ctx.send_message(AllMessages::NetworkBridge( NetworkBridgeMessage::SendCollationMessage( @@ -782,7 +785,7 @@ where } } } - CollationSeconded(_) => { + CollationSeconded(_, _) => { tracing::warn!( target: LOG_TARGET, peer_id = ?origin, @@ -935,7 +938,7 @@ where note_good_collation(ctx, &state.peer_data, id).await; } NotifyCollationSeconded(id, relay_parent, statement) => { - notify_collation_seconded(ctx, &state.peer_data, id, statement).await; + notify_collation_seconded(ctx, &state.peer_data, id, relay_parent, statement).await; } NetworkBridgeUpdateV1(event) => { if let Err(e) = handle_network_msg( @@ -1003,7 +1006,7 @@ pub(crate) async fn run( if let Poll::Ready(res) = futures::poll!(s) { Some(match res { - Either::Left((msg, _)) => Either::Left(msg?), + Either::Left((msg, _)) => Either::Left(msg.map_err(Fatal::SubsystemReceive)?), Either::Right((_, _)) => Either::Right(()), }) } else { From 1e9a8e7b1e6740c2e51ca18ef17888ead10acdc4 Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Fri, 30 Apr 2021 10:58:29 +0200 Subject: [PATCH 20/26] Fix collator side tests. --- .../collator-protocol/src/collator_side.rs | 245 +++++++++--------- 1 file changed, 122 insertions(+), 123 deletions(-) diff --git a/node/network/collator-protocol/src/collator_side.rs b/node/network/collator-protocol/src/collator_side.rs index a3d57f20d02f..47cb9e71c889 100644 --- a/node/network/collator-protocol/src/collator_side.rs +++ b/node/network/collator-protocol/src/collator_side.rs @@ -19,12 +19,21 @@ use std::collections::{HashMap, HashSet, hash_map::Entry}; use futures::{FutureExt, channel::oneshot, channel::mpsc}; use sp_core::Pair; -use polkadot_primitives::v1::{AuthorityDiscoveryId, CandidateHash, CandidateReceipt, CollatorPair, CoreIndex, CoreState, GroupIndex, Hash, Id as ParaId}; -use polkadot_subsystem::{FromOverseer, OverseerSignal, PerLeafSpan, SubsystemContext, jaeger, messages::{AllMessages, CollatorProtocolMessage, NetworkBridgeEvent, NetworkBridgeMessage}}; +use polkadot_primitives::v1::{AuthorityDiscoveryId, CandidateHash, CandidateReceipt, CollatorPair, CoreIndex, CoreState, GroupIndex, GroupRotationInfo, Hash, Id as ParaId}; +use polkadot_subsystem::{ + FromOverseer, OverseerSignal, PerLeafSpan, SubsystemContext, jaeger, + messages::{ + AllMessages, CollatorProtocolMessage, NetworkBridgeEvent, NetworkBridgeMessage, + }, +}; use polkadot_node_network_protocol::{ OurView, PeerId, View, peer_set::PeerSet, - request_response::{IncomingRequest, v1::{CollationFetchingRequest, CollationFetchingResponse}}, - v1 as protocol_v1, UnifiedReputationChange as Rep, + request_response::{ + IncomingRequest, + v1::{CollationFetchingRequest, CollationFetchingResponse}, + }, + v1 as protocol_v1, + UnifiedReputationChange as Rep, }; use polkadot_node_subsystem_util::{ metrics::{self, prometheus}, @@ -298,7 +307,8 @@ async fn distribute_collation( }; // Determine the group on that core and the next group on that core. - let (current_validators, next_validators) = determine_our_validators(ctx, runtime, our_core, num_cores, relay_parent).await?; + let (current_validators, next_validators) = + determine_our_validators(ctx, runtime, our_core, num_cores, relay_parent,).await?; if current_validators.validators.is_empty() && next_validators.validators.is_empty() { tracing::warn!( @@ -397,6 +407,7 @@ async fn determine_our_validators( relay_parent: Hash, ) -> Result<(GroupValidators, GroupValidators)> { let info = &runtime.get_session_info(ctx, relay_parent).await?.session_info; + tracing::debug!(target: LOG_TARGET, ?info, "Received info"); let groups = &info.validator_groups; let rotation_info = get_group_rotation_info(ctx, relay_parent).await?; @@ -956,10 +967,9 @@ mod tests { struct TestState { para_id: ParaId, validators: Vec, - validator_public: Vec, - validator_authority_id: Vec, + session_info: SessionInfo, + group_rotation_info: GroupRotationInfo, validator_peer_id: Vec, - validator_groups: (Vec>, GroupRotationInfo), relay_parent: Hash, availability_core: CoreState, local_peer_id: PeerId, @@ -988,10 +998,10 @@ mod tests { ]; let validator_public = validator_pubkeys(&validators); - let validator_authority_id = validator_authority_id(&validators); + let discovery_keys = validator_authority_id(&validators); let validator_peer_id = std::iter::repeat_with(|| PeerId::random()) - .take(validator_public.len()) + .take(discovery_keys.len()) .collect(); let validator_groups = vec![vec![2, 0, 4], vec![3, 2, 4]] @@ -1001,7 +1011,6 @@ mod tests { group_rotation_frequency: 100, now: 1, }; - let validator_groups = (validator_groups, group_rotation_info); let availability_core = CoreState::Scheduled(ScheduledCore { para_id, @@ -1016,10 +1025,14 @@ mod tests { Self { para_id, validators, - validator_public, - validator_authority_id, + session_info: SessionInfo { + validators: validator_public, + discovery_keys, + validator_groups, + ..Default::default() + }, + group_rotation_info, validator_peer_id, - validator_groups, relay_parent, availability_core, local_peer_id, @@ -1031,7 +1044,7 @@ mod tests { impl TestState { fn current_group_validator_indices(&self) -> &[ValidatorIndex] { - &self.validator_groups.0[0] + &self.session_info.validator_groups[0] } fn current_session_index(&self) -> SessionIndex { @@ -1045,25 +1058,25 @@ mod tests { fn current_group_validator_authority_ids(&self) -> Vec { self.current_group_validator_indices() .iter() - .map(|i| self.validator_authority_id[i.0 as usize].clone()) + .map(|i| self.session_info.discovery_keys[i.0 as usize].clone()) .collect() } fn current_group_validator_ids(&self) -> Vec { self.current_group_validator_indices() .iter() - .map(|i| self.validator_public[i.0 as usize].clone()) + .map(|i| self.session_info.validators[i.0 as usize].clone()) .collect() } fn next_group_validator_indices(&self) -> &[ValidatorIndex] { - &self.validator_groups.0[1] + &self.session_info.validator_groups[1] } fn next_group_validator_authority_ids(&self) -> Vec { self.next_group_validator_indices() .iter() - .map(|i| self.validator_authority_id[i.0 as usize].clone()) + .map(|i| self.session_info.discovery_keys[i.0 as usize].clone()) .collect() } @@ -1197,7 +1210,7 @@ mod tests { /// Result of [`distribute_collation`] struct DistributeCollation { /// Should be used to inform the subsystem about connected validators. - connected: mpsc::Sender<(AuthorityDiscoveryId, PeerId)>, + connected: Vec>, candidate: CandidateReceipt, pov_block: PoV, } @@ -1206,6 +1219,8 @@ mod tests { async fn distribute_collation( virtual_overseer: &mut VirtualOverseer, test_state: &TestState, + // whether or not the currently active validators are already connected or not. + already_connected: bool, ) -> DistributeCollation { // Now we want to distribute a PoVBlock let pov_block = PoV { @@ -1238,91 +1253,88 @@ mod tests { } ); - // Obtain the validator groups - assert_matches!( - overseer_recv(virtual_overseer).await, - AllMessages::RuntimeApi(RuntimeApiMessage::Request( - relay_parent, - RuntimeApiRequest::ValidatorGroups(tx) - )) => { - assert_eq!(relay_parent, test_state.relay_parent); - tx.send(Ok(test_state.validator_groups.clone())).unwrap(); - } - ); - - // obtain the validators per relay parent - assert_matches!( - overseer_recv(virtual_overseer).await, - AllMessages::RuntimeApi(RuntimeApiMessage::Request( - relay_parent, - RuntimeApiRequest::Validators(tx), - )) => { - assert_eq!(relay_parent, test_state.relay_parent); - tx.send(Ok(test_state.validator_public.clone())).unwrap(); - } - ); - - // obtain the validator_id to authority_id mapping - assert_matches!( - overseer_recv(virtual_overseer).await, - AllMessages::RuntimeApi(RuntimeApiMessage::Request( - relay_parent, - RuntimeApiRequest::SessionIndexForChild(tx), - )) => { - assert_eq!(relay_parent, test_state.relay_parent); - tx.send(Ok(test_state.current_session_index())).unwrap(); - } - ); - - assert_matches!( - overseer_recv(virtual_overseer).await, - AllMessages::RuntimeApi(RuntimeApiMessage::Request( - relay_parent, - RuntimeApiRequest::SessionInfo(index, tx), - )) => { - assert_eq!(relay_parent, test_state.relay_parent); - assert_eq!(index, test_state.current_session_index()); - - let validators = test_state.current_group_validator_ids(); - let current_discovery_keys = test_state.current_group_validator_authority_ids(); - let next_discovery_keys = test_state.next_group_validator_authority_ids(); + // We don't know precisely what is going to come as session info might be cached: + loop { + match overseer_recv(virtual_overseer).await { + AllMessages::RuntimeApi(RuntimeApiMessage::Request( + relay_parent, + RuntimeApiRequest::SessionIndexForChild(tx), + )) => { + assert_eq!(relay_parent, test_state.relay_parent); + tx.send(Ok(test_state.current_session_index())).unwrap(); + } - let discovery_keys = [¤t_discovery_keys[..], &next_discovery_keys[..]].concat(); + AllMessages::RuntimeApi(RuntimeApiMessage::Request( + relay_parent, + RuntimeApiRequest::SessionInfo(index, tx), + )) => { + assert_eq!(relay_parent, test_state.relay_parent); + assert_eq!(index, test_state.current_session_index()); - tx.send(Ok(Some(SessionInfo { - validators, - discovery_keys, - ..Default::default() - }))).unwrap(); - } - ); - - assert_matches!( - overseer_recv(virtual_overseer).await, - AllMessages::NetworkBridge( - NetworkBridgeMessage::ConnectToValidators { - connected, - .. + tx.send(Ok(Some(test_state.session_info.clone()))).unwrap(); } - ) => { - DistributeCollation { - connected, - candidate, - pov_block, + + AllMessages::RuntimeApi(RuntimeApiMessage::Request( + relay_parent, + RuntimeApiRequest::ValidatorGroups(tx) + )) => { + assert_eq!(relay_parent, test_state.relay_parent); + tx.send(Ok(( + test_state.session_info.validator_groups.clone(), + test_state.group_rotation_info.clone(), + ))).unwrap(); + // This call is mandatory - we are done: + break; } + other => + panic!("Unexpected message received: {:?}", other), } - ) + } + + let connected = if already_connected { + Vec::new() + } else { + let connected_current = assert_matches!( + overseer_recv(virtual_overseer).await, + AllMessages::NetworkBridge( + NetworkBridgeMessage::ConnectToValidators { + connected, + .. + } + ) => { connected } + ); + let connected_next = assert_matches!( + overseer_recv(virtual_overseer).await, + AllMessages::NetworkBridge( + NetworkBridgeMessage::ConnectToValidators { + connected, + .. + } + ) => { connected } + ); + vec![connected_current, connected_next] + }; + + DistributeCollation { + connected, + candidate, + pov_block, + } } /// Connect a peer - async fn connect_peer(virtual_overseer: &mut VirtualOverseer, peer: PeerId) { + async fn connect_peer( + virtual_overseer: &mut VirtualOverseer, + peer: PeerId, + authority_id: Option + ) { overseer_send( virtual_overseer, CollatorProtocolMessage::NetworkBridgeUpdateV1( NetworkBridgeEvent::PeerConnected( peer.clone(), polkadot_node_network_protocol::ObservedRole::Authority, - None, + authority_id, ), ), ).await; @@ -1425,15 +1437,14 @@ mod tests { setup_system(&mut virtual_overseer, &test_state).await; - let DistributeCollation { mut connected, candidate, pov_block } = - distribute_collation(&mut virtual_overseer, &test_state).await; + let DistributeCollation { connected: _connected, candidate, pov_block } = + distribute_collation(&mut virtual_overseer, &test_state, false).await; for (val, peer) in test_state.current_group_validator_authority_ids() .into_iter() .zip(test_state.current_group_validator_peer_ids()) { - connect_peer(&mut virtual_overseer, peer.clone()).await; - connected.try_send((val, peer)).unwrap(); + connect_peer(&mut virtual_overseer, peer.clone(), Some(val.clone())).await; } // We declare to the connected validators that we are a collator. @@ -1510,15 +1521,8 @@ mod tests { assert!(overseer_recv_with_timeout(&mut virtual_overseer, TIMEOUT).await.is_none()); - let DistributeCollation { mut connected, .. } = - distribute_collation(&mut virtual_overseer, &test_state).await; - - for (val, peer) in test_state.current_group_validator_authority_ids() - .into_iter() - .zip(test_state.current_group_validator_peer_ids()) - { - connected.try_send((val, peer)).unwrap(); - } + let DistributeCollation { connected: _connected, .. } = + distribute_collation(&mut virtual_overseer, &test_state, true).await; // Send info about peer's view. overseer_send( @@ -1546,11 +1550,12 @@ mod tests { let mut virtual_overseer = test_harness.virtual_overseer; let peer = test_state.validator_peer_id[0].clone(); + let validator_id = test_state.current_group_validator_authority_ids()[0].clone(); setup_system(&mut virtual_overseer, &test_state).await; // A validator connected to us - connect_peer(&mut virtual_overseer, peer.clone()).await; + connect_peer(&mut virtual_overseer, peer.clone(), Some(validator_id)).await; expect_declare_msg(&mut virtual_overseer, &test_state, &peer).await; virtual_overseer }) @@ -1574,10 +1579,10 @@ mod tests { setup_system(&mut virtual_overseer, &test_state).await; // A validator connected to us - connect_peer(&mut virtual_overseer, peer.clone()).await; + connect_peer(&mut virtual_overseer, peer.clone(), Some(validator_id)).await; // Connect the second validator - connect_peer(&mut virtual_overseer, peer2.clone()).await; + connect_peer(&mut virtual_overseer, peer2.clone(), Some(validator_id2)).await; expect_declare_msg(&mut virtual_overseer, &test_state, &peer).await; expect_declare_msg(&mut virtual_overseer, &test_state, &peer2).await; @@ -1585,9 +1590,7 @@ mod tests { // And let it tell us that it is has the same view. send_peer_view_change(&mut virtual_overseer, &peer2, vec![test_state.relay_parent]).await; - let mut connected = distribute_collation(&mut virtual_overseer, &test_state).await.connected; - connected.try_send((validator_id, peer.clone())).unwrap(); - connected.try_send((validator_id2, peer2.clone())).unwrap(); + let _connected = distribute_collation(&mut virtual_overseer, &test_state, false).await.connected; expect_advertise_collation_msg(&mut virtual_overseer, &peer2, test_state.relay_parent).await; @@ -1618,26 +1621,22 @@ mod tests { setup_system(&mut virtual_overseer, &test_state).await; // A validator connected to us - connect_peer(&mut virtual_overseer, peer.clone()).await; + connect_peer(&mut virtual_overseer, peer.clone(), Some(validator_id)).await; // Connect the second validator - connect_peer(&mut virtual_overseer, peer2.clone()).await; + connect_peer(&mut virtual_overseer, peer2.clone(), Some(validator_id2)).await; expect_declare_msg(&mut virtual_overseer, &test_state, &peer).await; expect_declare_msg(&mut virtual_overseer, &test_state, &peer2).await; - let mut connected = distribute_collation(&mut virtual_overseer, &test_state).await.connected; - connected.try_send((validator_id.clone(), peer.clone())).unwrap(); - connected.try_send((validator_id2.clone(), peer2.clone())).unwrap(); + let _connected = distribute_collation(&mut virtual_overseer, &test_state, false).await.connected; let old_relay_parent = test_state.relay_parent; // Advance to a new round, while informing the subsystem that the old and the new relay parent are active. test_state.advance_to_new_round(&mut virtual_overseer, true).await; - let mut connected = distribute_collation(&mut virtual_overseer, &test_state).await.connected; - connected.try_send((validator_id, peer.clone())).unwrap(); - connected.try_send((validator_id2, peer2.clone())).unwrap(); + let _connected = distribute_collation(&mut virtual_overseer, &test_state, true).await.connected; send_peer_view_change(&mut virtual_overseer, &peer, vec![old_relay_parent]).await; expect_advertise_collation_msg(&mut virtual_overseer, &peer, old_relay_parent).await; @@ -1664,18 +1663,17 @@ mod tests { setup_system(&mut virtual_overseer, &test_state).await; // A validator connected to us - connect_peer(&mut virtual_overseer, peer.clone()).await; + connect_peer(&mut virtual_overseer, peer.clone(), Some(validator_id.clone())).await; expect_declare_msg(&mut virtual_overseer, &test_state, &peer).await; - let mut connected = distribute_collation(&mut virtual_overseer, &test_state).await.connected; - connected.try_send((validator_id.clone(), peer.clone())).unwrap(); + let _ = distribute_collation(&mut virtual_overseer, &test_state, false).await.connected; send_peer_view_change(&mut virtual_overseer, &peer, vec![test_state.relay_parent]).await; expect_advertise_collation_msg(&mut virtual_overseer, &peer, test_state.relay_parent).await; // Disconnect and reconnect directly disconnect_peer(&mut virtual_overseer, peer.clone()).await; - connect_peer(&mut virtual_overseer, peer.clone()).await; + connect_peer(&mut virtual_overseer, peer.clone(), Some(validator_id)).await; expect_declare_msg(&mut virtual_overseer, &test_state, &peer).await; send_peer_view_change(&mut virtual_overseer, &peer, vec![test_state.relay_parent]).await; @@ -1696,11 +1694,12 @@ mod tests { let mut virtual_overseer = test_harness.virtual_overseer; let peer = test_state.current_group_validator_peer_ids()[0].clone(); + let validator_id = test_state.current_group_validator_authority_ids()[0].clone(); setup_system(&mut virtual_overseer, &test_state).await; // A validator connected to us - connect_peer(&mut virtual_overseer, peer.clone()).await; + connect_peer(&mut virtual_overseer, peer.clone(), Some(validator_id)).await; expect_declare_msg(&mut virtual_overseer, &test_state, &peer).await; overseer_send( From ce9c9d793ae485b0278cb7baa7ce52b2a70deef2 Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Fri, 30 Apr 2021 11:01:16 +0200 Subject: [PATCH 21/26] Get rid of warnings. --- .../collator-protocol/src/collator_side.rs | 20 +------------------ 1 file changed, 1 insertion(+), 19 deletions(-) diff --git a/node/network/collator-protocol/src/collator_side.rs b/node/network/collator-protocol/src/collator_side.rs index 47cb9e71c889..a873f623cce4 100644 --- a/node/network/collator-protocol/src/collator_side.rs +++ b/node/network/collator-protocol/src/collator_side.rs @@ -19,7 +19,7 @@ use std::collections::{HashMap, HashSet, hash_map::Entry}; use futures::{FutureExt, channel::oneshot, channel::mpsc}; use sp_core::Pair; -use polkadot_primitives::v1::{AuthorityDiscoveryId, CandidateHash, CandidateReceipt, CollatorPair, CoreIndex, CoreState, GroupIndex, GroupRotationInfo, Hash, Id as ParaId}; +use polkadot_primitives::v1::{AuthorityDiscoveryId, CandidateHash, CandidateReceipt, CollatorPair, CoreIndex, CoreState, GroupIndex, Hash, Id as ParaId}; use polkadot_subsystem::{ FromOverseer, OverseerSignal, PerLeafSpan, SubsystemContext, jaeger, messages::{ @@ -1062,24 +1062,6 @@ mod tests { .collect() } - fn current_group_validator_ids(&self) -> Vec { - self.current_group_validator_indices() - .iter() - .map(|i| self.session_info.validators[i.0 as usize].clone()) - .collect() - } - - fn next_group_validator_indices(&self) -> &[ValidatorIndex] { - &self.session_info.validator_groups[1] - } - - fn next_group_validator_authority_ids(&self) -> Vec { - self.next_group_validator_indices() - .iter() - .map(|i| self.session_info.discovery_keys[i.0 as usize].clone()) - .collect() - } - /// Generate a new relay parent and inform the subsystem about the new view. /// /// If `merge_views == true` it means the subsystem will be informed that we working on the old `relay_parent` From 07579d0d73b99d28afa5dd444bda6b64a4961433 Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Fri, 30 Apr 2021 11:53:20 +0200 Subject: [PATCH 22/26] Better Signed/UncheckedSigned implementation. Also get rid of Encode/Decode for Signed! *party* --- Cargo.lock | 1 - node/core/proposer/src/lib.rs | 2 +- node/network/bitfield-distribution/Cargo.toml | 1 - node/network/bitfield-distribution/src/lib.rs | 5 +- .../network/statement-distribution/src/lib.rs | 2 +- primitives/src/v0.rs | 178 ++++-------------- 6 files changed, 41 insertions(+), 148 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 536f131b6bbf..6e76d8417641 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5579,7 +5579,6 @@ dependencies = [ "futures 0.3.14", "log", "maplit", - "parity-scale-codec", "polkadot-node-network-protocol", "polkadot-node-subsystem", "polkadot-node-subsystem-test-helpers", diff --git a/node/core/proposer/src/lib.rs b/node/core/proposer/src/lib.rs index 64ad751f561c..035f76d83c14 100644 --- a/node/core/proposer/src/lib.rs +++ b/node/core/proposer/src/lib.rs @@ -213,7 +213,7 @@ where let parachains_inherent_data = match self.get_provisioner_data().await { Ok(pd) => ParachainsInherentData { - bitfields: pd.bitfields, + bitfields: pd.bitfields.into_iter().map(Into::into).collect(), backed_candidates: pd.backed_candidates, disputes: pd.disputes, parent_header: self.parent_header, diff --git a/node/network/bitfield-distribution/Cargo.toml b/node/network/bitfield-distribution/Cargo.toml index f19807a91bea..712c95c5dff5 100644 --- a/node/network/bitfield-distribution/Cargo.toml +++ b/node/network/bitfield-distribution/Cargo.toml @@ -7,7 +7,6 @@ edition = "2018" [dependencies] futures = "0.3.12" tracing = "0.1.25" -parity-scale-codec = { version = "2.0.0", default-features = false, features = ["derive"] } polkadot-primitives = { path = "../../../primitives" } polkadot-subsystem = { package = "polkadot-node-subsystem", path = "../../subsystem" } polkadot-node-subsystem-util = { path = "../../subsystem-util" } diff --git a/node/network/bitfield-distribution/src/lib.rs b/node/network/bitfield-distribution/src/lib.rs index b009d55cf1cc..76820bedfb6b 100644 --- a/node/network/bitfield-distribution/src/lib.rs +++ b/node/network/bitfield-distribution/src/lib.rs @@ -22,7 +22,6 @@ #![deny(unused_crate_dependencies)] -use parity_scale_codec::{Decode, Encode}; use futures::{channel::oneshot, FutureExt}; use polkadot_subsystem::messages::*; @@ -49,7 +48,7 @@ const BENEFIT_VALID_MESSAGE: Rep = Rep::BenefitMinor("Valid message"); /// Checked signed availability bitfield that is distributed /// to other peers. -#[derive(Encode, Decode, Debug, Clone, PartialEq, Eq)] +#[derive(Debug, Clone, PartialEq, Eq)] struct BitfieldGossipMessage { /// The relay parent this message is relative to. relay_parent: Hash, @@ -479,7 +478,7 @@ where ?validator_index, "already received a message for validator", ); - if old_message.signed_availability.equals_unchecked(&bitfield) { + if old_message.signed_availability.as_unchecked() == &bitfield { modify_reputation(ctx, origin, BENEFIT_VALID_MESSAGE).await; } return; diff --git a/node/network/statement-distribution/src/lib.rs b/node/network/statement-distribution/src/lib.rs index 729ae9bfe2ab..db5e7aab7faf 100644 --- a/node/network/statement-distribution/src/lib.rs +++ b/node/network/statement-distribution/src/lib.rs @@ -883,7 +883,7 @@ fn is_statement_large(statement: &SignedFullStatement) -> bool { return true } // No runtime upgrade, now we need to be more nuanced: - let size = statement.encoded_size(); + let size = statement.as_unchecked().encoded_size(); // Half max size seems to be a good threshold to start not using notifications: let threshold = diff --git a/primitives/src/v0.rs b/primitives/src/v0.rs index d1f6faebf3d2..77ed37828751 100644 --- a/primitives/src/v0.rs +++ b/primitives/src/v0.rs @@ -894,24 +894,18 @@ impl EncodeAs for T { /// Signed data with signature already verified. /// -/// NOTE: This type does have an Encode/Decode instance because we are encoding/decoding accross -/// trusted bundaries for runtime communication (inherent data), so care must be taken to never use -/// those instances on untrusted boundaries as they render our valid signatures guarantees invalid. -/// Instead use `UncheckedSigned` on such boundaries, `Signed` can easily be converted into -/// `UncheckedSigned` and conversion back via `into_signed` enforces a valid signature again. +/// NOTE: This type does not have an Encode/Decode instance, as this would cancel out our +/// valid signature guarantees. If you need to encode/decode you have to convert into an +/// `UncheckedSigned` first. +/// +/// `Signed` can easily be converted into `UncheckedSigned` and conversion back via `into_signed` +/// enforces a valid signature again. #[derive(Clone, PartialEq, Eq, RuntimeDebug)] -pub struct Signed(SignedImpl); +pub struct Signed(UncheckedSigned); /// Unsigned data, can be converted to `Signed` by checking the signature. -#[derive(Clone, PartialEq, Eq, RuntimeDebug)] -pub struct UncheckedSigned(SignedImpl); - -/// A signed type which encapsulates the common desire to sign some data and validate a signature. -/// -/// Note that the internal fields are not public; they are all accessable by immutable getters. -/// This reduces the chance that they are accidentally mutated, invalidating the signature. #[derive(Clone, PartialEq, Eq, RuntimeDebug, Encode, Decode)] -struct SignedImpl { +pub struct UncheckedSigned { /// The payload is part of the signed data. The rest is the signing context, /// which is known both at signing and at validation. payload: Payload, @@ -935,7 +929,7 @@ impl, RealPayload: Encode> Signed, key: &ValidatorId, ) -> Option { - let s = SignedImpl { + let s = UncheckedSigned { payload, validator_index, signature, @@ -956,7 +950,7 @@ impl, RealPayload: Encode> Signed Result, KeystoreError> { - let r = SignedImpl::sign(keystore, payload, context, validator_index, key).await?; + let r = UncheckedSigned::sign(keystore, payload, context, validator_index, key).await?; Ok(r.map(Self)) } @@ -966,20 +960,16 @@ impl, RealPayload: Encode> Signed, key: &ValidatorId ) -> Result> { - if unchecked.0.check_signature(context, key).is_ok() { - Ok(Self(unchecked.0)) + if unchecked.check_signature(context, key).is_ok() { + Ok(Self(unchecked)) } else { Err(unchecked) } } - /// Check whether this checked signed is equal to some unchecked one. - pub fn equals_unchecked(&self, unchecked: &UncheckedSigned) -> bool - where - Payload: PartialEq + Eq, - RealPayload: PartialEq + Eq, - { - self.0 == unchecked.0 + /// Get a reference to data as unchecked. + pub fn as_unchecked(&self) -> &UncheckedSigned { + &self.0 } /// Immutably access the payload. @@ -1008,10 +998,14 @@ impl, RealPayload: Encode> Signed Signed where for<'a> &'a Payload: Into { - Signed(self.0.convert_payload()) + Signed(self.0.unchecked_convert_payload()) } } +// We can't bound this on `Payload: Into` beacuse that conversion consumes +// the payload, and we don't want that. We can't bound it on `Payload: AsRef` +// because there's no blanket impl of `AsRef for T`. In the end, we just invent our +// own trait which does what we need: EncodeAs. impl, RealPayload: Encode> UncheckedSigned { /// Used to create a `UncheckedSigned` from already existing parts. /// @@ -1022,14 +1016,12 @@ impl, RealPayload: Encode> UncheckedSigned Self { - let s = SignedImpl { + Self { payload, validator_index, signature, real_payload: std::marker::PhantomData, - }; - - Self(s) + } } /// Check signature and convert to `Signed` if successful. @@ -1044,44 +1036,37 @@ impl, RealPayload: Encode> UncheckedSigned &Payload { - &self.0.payload + &self.payload } /// Immutably access the validator index. #[inline] pub fn unchecked_validator_index(&self) -> ValidatorIndex { - self.0.validator_index + self.validator_index } /// Immutably access the signature. #[inline] pub fn unchecked_signature(&self) -> &ValidatorSignature { - &self.0.signature + &self.signature } /// Discard signing data, get the payload #[inline] - pub fn into_unchecked_payload(self) -> Payload { - self.0.payload + pub fn unchecked_into_payload(self) -> Payload { + self.payload } /// Convert `Payload` into `RealPayload`. - pub fn convert_unchecked_payload(&self) -> UncheckedSigned where for<'a> &'a Payload: Into { - UncheckedSigned(self.0.convert_payload()) - } -} - -impl From> for UncheckedSigned { - fn from(signed: Signed) -> Self { - Self(signed.0) + pub fn unchecked_convert_payload(&self) -> UncheckedSigned where for<'a> &'a Payload: Into { + UncheckedSigned { + signature: self.signature.clone(), + validator_index: self.validator_index, + payload: (&self.payload).into(), + real_payload: sp_std::marker::PhantomData, + } } -} -// We can't bound this on `Payload: Into` beacuse that conversion consumes -// the payload, and we don't want that. We can't bound it on `Payload: AsRef` -// because there's no blanket impl of `AsRef for T`. In the end, we just invent our -// own trait which does what we need: EncodeAs. -impl, RealPayload: Encode> SignedImpl { fn payload_data(payload: &Payload, context: &SigningContext) -> Vec { // equivalent to (real_payload, context).encode() let mut out = payload.encode_as(); @@ -1125,100 +1110,11 @@ impl, RealPayload: Encode> SignedImpl SignedImpl where for<'a> &'a Payload: Into { - SignedImpl { - signature: self.signature.clone(), - validator_index: self.validator_index, - payload: (&self.payload).into(), - real_payload: sp_std::marker::PhantomData, - } - } -} - -/// Dummy implementation (derive does not do the right thing): -impl Encode for Signed - where - Payload: Encode -{ - fn size_hint(&self) -> usize { - self.0.size_hint() - } - - fn encode_to(&self, dest: &mut T) { - self.0.encode_to(dest) - } - - fn encode(&self) -> Vec { - self.0.encode() - } - - fn using_encoded R>(&self, f: F) -> R { - self.0.using_encoded(f) - } - - fn encoded_size(&self) -> usize { - self.0.encoded_size() - } } -/// Dummy implementation (derive does not do the right thing): -impl Encode for UncheckedSigned - where - Payload: Encode, -{ - fn size_hint(&self) -> usize { - self.0.size_hint() - } - - fn encode_to(&self, dest: &mut T) { - self.0.encode_to(dest) - } - - fn encode(&self) -> Vec { - self.0.encode() - } - - fn using_encoded R>(&self, f: F) -> R { - self.0.using_encoded(f) - } - - fn encoded_size(&self) -> usize { - self.0.encoded_size() - } -} - -impl Decode for Signed - where - Payload: Decode, -{ - fn decode(input: &mut I) -> Result { - as Decode>::decode(input).map(Self) - } - - fn skip(input: &mut I) -> Result<(), parity_scale_codec::Error> { - as Decode>::skip(input) - } - - fn encoded_fixed_size() -> Option { - as Decode>::encoded_fixed_size() - } -} - -impl Decode for UncheckedSigned - where - Payload: Decode, -{ - fn decode(input: &mut I) -> Result { - as Decode>::decode(input).map(Self) - } - - fn skip(input: &mut I) -> Result<(), parity_scale_codec::Error> { - as Decode>::skip(input) - } - - fn encoded_fixed_size() -> Option { - as Decode>::encoded_fixed_size() +impl From> for UncheckedSigned { + fn from(signed: Signed) -> Self { + signed.0 } } From 1332ef17ceaf5b24216365736fc9b993c52560dd Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Fri, 30 Apr 2021 18:10:44 +0200 Subject: [PATCH 23/26] Get rid of dead code. --- node/network/collator-protocol/src/error.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/node/network/collator-protocol/src/error.rs b/node/network/collator-protocol/src/error.rs index ed9e39159cdd..37f8df0731b2 100644 --- a/node/network/collator-protocol/src/error.rs +++ b/node/network/collator-protocol/src/error.rs @@ -27,9 +27,7 @@ use crate::LOG_TARGET; /// General result. pub type Result = std::result::Result; -/// Result for non fatal only failures. -#[allow(dead_code)] -pub type NonFatalResult = std::result::Result; + /// Result for fatal only failures. pub type FatalResult = std::result::Result; From fe03aba9cb8dd3b39fb745c7ce9f81f96dcecb32 Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Fri, 30 Apr 2021 18:33:27 +0200 Subject: [PATCH 24/26] Move Signed in its own module. --- primitives/src/v0.rs | 261 ------------------------- primitives/src/{v1.rs => v1/mod.rs} | 13 +- primitives/src/v1/signed.rs | 282 ++++++++++++++++++++++++++++ 3 files changed, 293 insertions(+), 263 deletions(-) rename primitives/src/{v1.rs => v1/mod.rs} (99%) create mode 100644 primitives/src/v1/signed.rs diff --git a/primitives/src/v0.rs b/primitives/src/v0.rs index 77ed37828751..dc3c8dceb7d2 100644 --- a/primitives/src/v0.rs +++ b/primitives/src/v0.rs @@ -18,8 +18,6 @@ //! perspective. use sp_std::prelude::*; -#[cfg(feature = "std")] -use sp_std::convert::TryInto; use sp_std::cmp::Ordering; use parity_scale_codec::{Encode, Decode}; @@ -29,13 +27,9 @@ use serde::{Serialize, Deserialize}; #[cfg(feature = "std")] use parity_util_mem::{MallocSizeOf, MallocSizeOfOps}; -#[cfg(feature = "std")] -use sp_keystore::{CryptoStore, SyncCryptoStorePtr, Error as KeystoreError}; use primitives::RuntimeDebug; use runtime_primitives::traits::{AppVerify, Block as BlockT}; use inherents::InherentIdentifier; -#[cfg(feature = "std")] -use application_crypto::AppKey; use application_crypto::KeyTypeId; pub use runtime_primitives::traits::{BlakeTwo256, Hash as HashT, Verify, IdentifyAccount}; @@ -731,9 +725,6 @@ impl CompactStatement { } } -/// A signed compact statement, suitable to be sent to the chain. -pub type SignedStatement = Signed; - /// An either implicit or explicit attestation to the validity of a parachain /// candidate. #[derive(Clone, Eq, PartialEq, Decode, Encode, RuntimeDebug)] @@ -866,258 +857,6 @@ pub mod id { pub const PARACHAIN_HOST: ApiId = *b"parahost"; } -/// This helper trait ensures that we can encode Statement as CompactStatement, -/// and anything as itself. -/// -/// This resembles `parity_scale_codec::EncodeLike`, but it's distinct: -/// EncodeLike is a marker trait which asserts at the typesystem level that -/// one type's encoding is a valid encoding for another type. It doesn't -/// perform any type conversion when encoding. -/// -/// This trait, on the other hand, provides a method which can be used to -/// simultaneously convert and encode one type as another. -pub trait EncodeAs { - /// Convert Self into T, then encode T. - /// - /// This is useful when T is a subset of Self, reducing encoding costs; - /// its signature also means that we do not need to clone Self in order - /// to retain ownership, as we would if we were to do - /// `self.clone().into().encode()`. - fn encode_as(&self) -> Vec; -} - -impl EncodeAs for T { - fn encode_as(&self) -> Vec { - self.encode() - } -} - -/// Signed data with signature already verified. -/// -/// NOTE: This type does not have an Encode/Decode instance, as this would cancel out our -/// valid signature guarantees. If you need to encode/decode you have to convert into an -/// `UncheckedSigned` first. -/// -/// `Signed` can easily be converted into `UncheckedSigned` and conversion back via `into_signed` -/// enforces a valid signature again. -#[derive(Clone, PartialEq, Eq, RuntimeDebug)] -pub struct Signed(UncheckedSigned); - -/// Unsigned data, can be converted to `Signed` by checking the signature. -#[derive(Clone, PartialEq, Eq, RuntimeDebug, Encode, Decode)] -pub struct UncheckedSigned { - /// The payload is part of the signed data. The rest is the signing context, - /// which is known both at signing and at validation. - payload: Payload, - /// The index of the validator signing this statement. - validator_index: ValidatorIndex, - /// The signature by the validator of the signed payload. - signature: ValidatorSignature, - /// This ensures the real payload is tracked at the typesystem level. - real_payload: sp_std::marker::PhantomData, -} - -impl, RealPayload: Encode> Signed { - /// Used to create a `Signed` from already existing parts. - /// - /// The signature is checked as part of the process. - #[cfg(feature = "std")] - pub fn new( - payload: Payload, - validator_index: ValidatorIndex, - signature: ValidatorSignature, - context: &SigningContext, - key: &ValidatorId, - ) -> Option { - let s = UncheckedSigned { - payload, - validator_index, - signature, - real_payload: std::marker::PhantomData, - }; - - s.check_signature(context, key).ok()?; - - Some(Self(s)) - } - - /// Create a new `Signed` by signing data. - #[cfg(feature = "std")] - pub async fn sign( - keystore: &SyncCryptoStorePtr, - payload: Payload, - context: &SigningContext, - validator_index: ValidatorIndex, - key: &ValidatorId, - ) -> Result, KeystoreError> { - let r = UncheckedSigned::sign(keystore, payload, context, validator_index, key).await?; - Ok(r.map(Self)) - } - - /// Try to convert from `UncheckedSigned` by checking the signature. - pub fn from_unchecked( - unchecked: UncheckedSigned, - context: &SigningContext, - key: &ValidatorId - ) -> Result> { - if unchecked.check_signature(context, key).is_ok() { - Ok(Self(unchecked)) - } else { - Err(unchecked) - } - } - - /// Get a reference to data as unchecked. - pub fn as_unchecked(&self) -> &UncheckedSigned { - &self.0 - } - - /// Immutably access the payload. - #[inline] - pub fn payload(&self) -> &Payload { - &self.0.payload - } - - /// Immutably access the validator index. - #[inline] - pub fn validator_index(&self) -> ValidatorIndex { - self.0.validator_index - } - - /// Immutably access the signature. - #[inline] - pub fn signature(&self) -> &ValidatorSignature { - &self.0.signature - } - - /// Discard signing data, get the payload - #[inline] - pub fn into_payload(self) -> Payload { - self.0.payload - } - - /// Convert `Payload` into `RealPayload`. - pub fn convert_payload(&self) -> Signed where for<'a> &'a Payload: Into { - Signed(self.0.unchecked_convert_payload()) - } -} - -// We can't bound this on `Payload: Into` beacuse that conversion consumes -// the payload, and we don't want that. We can't bound it on `Payload: AsRef` -// because there's no blanket impl of `AsRef for T`. In the end, we just invent our -// own trait which does what we need: EncodeAs. -impl, RealPayload: Encode> UncheckedSigned { - /// Used to create a `UncheckedSigned` from already existing parts. - /// - /// Signature is not checked here, hence `UncheckedSigned`. - #[cfg(feature = "std")] - pub fn new( - payload: Payload, - validator_index: ValidatorIndex, - signature: ValidatorSignature, - ) -> Self { - Self { - payload, - validator_index, - signature, - real_payload: std::marker::PhantomData, - } - } - - /// Check signature and convert to `Signed` if successful. - pub fn into_checked( - self, - context: &SigningContext, - key: &ValidatorId - ) -> Result, Self> { - Signed::from_unchecked(self, context, key) - } - - /// Immutably access the payload. - #[inline] - pub fn unchecked_payload(&self) -> &Payload { - &self.payload - } - - /// Immutably access the validator index. - #[inline] - pub fn unchecked_validator_index(&self) -> ValidatorIndex { - self.validator_index - } - - /// Immutably access the signature. - #[inline] - pub fn unchecked_signature(&self) -> &ValidatorSignature { - &self.signature - } - - /// Discard signing data, get the payload - #[inline] - pub fn unchecked_into_payload(self) -> Payload { - self.payload - } - - /// Convert `Payload` into `RealPayload`. - pub fn unchecked_convert_payload(&self) -> UncheckedSigned where for<'a> &'a Payload: Into { - UncheckedSigned { - signature: self.signature.clone(), - validator_index: self.validator_index, - payload: (&self.payload).into(), - real_payload: sp_std::marker::PhantomData, - } - } - - fn payload_data(payload: &Payload, context: &SigningContext) -> Vec { - // equivalent to (real_payload, context).encode() - let mut out = payload.encode_as(); - out.extend(context.encode()); - out - } - - /// Sign this payload with the given context and key, storing the validator index. - #[cfg(feature = "std")] - async fn sign( - keystore: &SyncCryptoStorePtr, - payload: Payload, - context: &SigningContext, - validator_index: ValidatorIndex, - key: &ValidatorId, - ) -> Result, KeystoreError> { - let data = Self::payload_data(&payload, context); - let signature = CryptoStore::sign_with( - &**keystore, - ValidatorId::ID, - &key.into(), - &data, - ).await?; - - let signature = match signature { - Some(sig) => sig.try_into().map_err(|_| KeystoreError::KeyNotSupported(ValidatorId::ID))?, - None => return Ok(None), - }; - - Ok(Some(Self { - payload, - validator_index, - signature, - real_payload: std::marker::PhantomData, - })) - } - - /// Validate the payload given the context and public key. - fn check_signature(&self, context: &SigningContext, key: &ValidatorId) -> Result<(), ()> { - let data = Self::payload_data(&self.payload, context); - if self.signature.verify(data.as_slice(), key) { Ok(()) } else { Err(()) } - } - -} - -impl From> for UncheckedSigned { - fn from(signed: Signed) -> Self { - signed.0 - } -} - /// Custom validity errors used in Polkadot while validating transactions. #[repr(u8)] pub enum ValidityError { diff --git a/primitives/src/v1.rs b/primitives/src/v1/mod.rs similarity index 99% rename from primitives/src/v1.rs rename to primitives/src/v1/mod.rs index e5106bf306c7..f0d44905b1c4 100644 --- a/primitives/src/v1.rs +++ b/primitives/src/v1/mod.rs @@ -44,8 +44,8 @@ pub use polkadot_parachain::primitives::{ // Export some basic parachain primitives from v0. pub use crate::v0::{ CollatorId, CollatorSignature, PARACHAIN_KEY_TYPE_ID, ValidatorId, ValidatorIndex, - ValidatorSignature, SigningContext, Signed, UncheckedSigned, ValidityAttestation, - CompactStatement, SignedStatement, EncodeAs, + ValidatorSignature, SigningContext, ValidityAttestation, + CompactStatement, }; #[cfg(feature = "std")] @@ -58,6 +58,10 @@ pub use crate::v0::{ValidatorPair, CollatorPair}; pub use sp_staking::SessionIndex; pub use sp_authority_discovery::AuthorityId as AuthorityDiscoveryId; +/// Signed data. +mod signed; +pub use signed::{Signed, UncheckedSigned, EncodeAs}; + /// A declarations of storage keys where an external observer can find some interesting data. pub mod well_known_keys { use super::{Id, HrmpChannelId}; @@ -169,6 +173,7 @@ pub mod well_known_keys { } } + /// Unique identifier for the Parachains Inherent pub const PARACHAINS_INHERENT_IDENTIFIER: InherentIdentifier = *b"parachn0"; @@ -461,6 +466,10 @@ impl From> for AvailabilityBitfield { } } + +/// A signed compact statement, suitable to be sent to the chain. +pub type SignedStatement = Signed; + /// A bitfield signed by a particular validator about the availability of pending candidates. pub type SignedAvailabilityBitfield = Signed; /// A signed bitfield with signature not yet checked. diff --git a/primitives/src/v1/signed.rs b/primitives/src/v1/signed.rs new file mode 100644 index 000000000000..e601858fe3c3 --- /dev/null +++ b/primitives/src/v1/signed.rs @@ -0,0 +1,282 @@ +// Copyright 2021 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 . + +use parity_scale_codec::{Encode, Decode}; + +use sp_std::prelude::Vec; +#[cfg(feature = "std")] +use sp_std::convert::TryInto; +#[cfg(feature = "std")] +use application_crypto::AppKey; +#[cfg(feature = "std")] +use sp_keystore::{CryptoStore, SyncCryptoStorePtr, Error as KeystoreError}; + +use primitives::RuntimeDebug; +use runtime_primitives::traits::AppVerify; + +use crate::v0::{SigningContext, ValidatorId, ValidatorSignature, ValidatorIndex}; + +/// Signed data with signature already verified. +/// +/// NOTE: This type does not have an Encode/Decode instance, as this would cancel out our +/// valid signature guarantees. If you need to encode/decode you have to convert into an +/// `UncheckedSigned` first. +/// +/// `Signed` can easily be converted into `UncheckedSigned` and conversion back via `into_signed` +/// enforces a valid signature again. +#[derive(Clone, PartialEq, Eq, RuntimeDebug)] +pub struct Signed(UncheckedSigned); + +/// Unchecked signed data, can be converted to `Signed` by checking the signature. +#[derive(Clone, PartialEq, Eq, RuntimeDebug, Encode, Decode)] +pub struct UncheckedSigned { + /// The payload is part of the signed data. The rest is the signing context, + /// which is known both at signing and at validation. + payload: Payload, + /// The index of the validator signing this statement. + validator_index: ValidatorIndex, + /// The signature by the validator of the signed payload. + signature: ValidatorSignature, + /// This ensures the real payload is tracked at the typesystem level. + real_payload: sp_std::marker::PhantomData, +} + +impl, RealPayload: Encode> Signed { + /// Used to create a `Signed` from already existing parts. + /// + /// The signature is checked as part of the process. + #[cfg(feature = "std")] + pub fn new( + payload: Payload, + validator_index: ValidatorIndex, + signature: ValidatorSignature, + context: &SigningContext, + key: &ValidatorId, + ) -> Option { + let s = UncheckedSigned { + payload, + validator_index, + signature, + real_payload: std::marker::PhantomData, + }; + + s.check_signature(context, key).ok()?; + + Some(Self(s)) + } + + /// Create a new `Signed` by signing data. + #[cfg(feature = "std")] + pub async fn sign( + keystore: &SyncCryptoStorePtr, + payload: Payload, + context: &SigningContext, + validator_index: ValidatorIndex, + key: &ValidatorId, + ) -> Result, KeystoreError> { + let r = UncheckedSigned::sign(keystore, payload, context, validator_index, key).await?; + Ok(r.map(Self)) + } + + /// Try to convert from `UncheckedSigned` by checking the signature. + pub fn try_from_unchecked( + unchecked: UncheckedSigned, + context: &SigningContext, + key: &ValidatorId + ) -> Result> { + if unchecked.check_signature(context, key).is_ok() { + Ok(Self(unchecked)) + } else { + Err(unchecked) + } + } + + /// Get a reference to data as unchecked. + pub fn as_unchecked(&self) -> &UncheckedSigned { + &self.0 + } + + /// Immutably access the payload. + #[inline] + pub fn payload(&self) -> &Payload { + &self.0.payload + } + + /// Immutably access the validator index. + #[inline] + pub fn validator_index(&self) -> ValidatorIndex { + self.0.validator_index + } + + /// Immutably access the signature. + #[inline] + pub fn signature(&self) -> &ValidatorSignature { + &self.0.signature + } + + /// Discard signing data, get the payload + #[inline] + pub fn into_payload(self) -> Payload { + self.0.payload + } + + /// Convert `Payload` into `RealPayload`. + pub fn convert_payload(&self) -> Signed where for<'a> &'a Payload: Into { + Signed(self.0.unchecked_convert_payload()) + } +} + +// We can't bound this on `Payload: Into` beacuse that conversion consumes +// the payload, and we don't want that. We can't bound it on `Payload: AsRef` +// because there's no blanket impl of `AsRef for T`. In the end, we just invent our +// own trait which does what we need: EncodeAs. +impl, RealPayload: Encode> UncheckedSigned { + /// Used to create a `UncheckedSigned` from already existing parts. + /// + /// Signature is not checked here, hence `UncheckedSigned`. + #[cfg(feature = "std")] + pub fn new( + payload: Payload, + validator_index: ValidatorIndex, + signature: ValidatorSignature, + ) -> Self { + Self { + payload, + validator_index, + signature, + real_payload: std::marker::PhantomData, + } + } + + /// Check signature and convert to `Signed` if successful. + pub fn into_checked( + self, + context: &SigningContext, + key: &ValidatorId + ) -> Result, Self> { + Signed::try_from_unchecked(self, context, key) + } + + /// Immutably access the payload. + #[inline] + pub fn unchecked_payload(&self) -> &Payload { + &self.payload + } + + /// Immutably access the validator index. + #[inline] + pub fn unchecked_validator_index(&self) -> ValidatorIndex { + self.validator_index + } + + /// Immutably access the signature. + #[inline] + pub fn unchecked_signature(&self) -> &ValidatorSignature { + &self.signature + } + + /// Discard signing data, get the payload + #[inline] + pub fn unchecked_into_payload(self) -> Payload { + self.payload + } + + /// Convert `Payload` into `RealPayload`. + pub fn unchecked_convert_payload(&self) -> UncheckedSigned where for<'a> &'a Payload: Into { + UncheckedSigned { + signature: self.signature.clone(), + validator_index: self.validator_index, + payload: (&self.payload).into(), + real_payload: sp_std::marker::PhantomData, + } + } + + fn payload_data(payload: &Payload, context: &SigningContext) -> Vec { + // equivalent to (real_payload, context).encode() + let mut out = payload.encode_as(); + out.extend(context.encode()); + out + } + + /// Sign this payload with the given context and key, storing the validator index. + #[cfg(feature = "std")] + async fn sign( + keystore: &SyncCryptoStorePtr, + payload: Payload, + context: &SigningContext, + validator_index: ValidatorIndex, + key: &ValidatorId, + ) -> Result, KeystoreError> { + let data = Self::payload_data(&payload, context); + let signature = CryptoStore::sign_with( + &**keystore, + ValidatorId::ID, + &key.into(), + &data, + ).await?; + + let signature = match signature { + Some(sig) => sig.try_into().map_err(|_| KeystoreError::KeyNotSupported(ValidatorId::ID))?, + None => return Ok(None), + }; + + Ok(Some(Self { + payload, + validator_index, + signature, + real_payload: std::marker::PhantomData, + })) + } + + /// Validate the payload given the context and public key. + fn check_signature(&self, context: &SigningContext, key: &ValidatorId) -> Result<(), ()> { + let data = Self::payload_data(&self.payload, context); + if self.signature.verify(data.as_slice(), key) { Ok(()) } else { Err(()) } + } + +} + +impl From> for UncheckedSigned { + fn from(signed: Signed) -> Self { + signed.0 + } +} + +/// This helper trait ensures that we can encode Statement as CompactStatement, +/// and anything as itself. +/// +/// This resembles `parity_scale_codec::EncodeLike`, but it's distinct: +/// EncodeLike is a marker trait which asserts at the typesystem level that +/// one type's encoding is a valid encoding for another type. It doesn't +/// perform any type conversion when encoding. +/// +/// This trait, on the other hand, provides a method which can be used to +/// simultaneously convert and encode one type as another. +pub trait EncodeAs { + /// Convert Self into T, then encode T. + /// + /// This is useful when T is a subset of Self, reducing encoding costs; + /// its signature also means that we do not need to clone Self in order + /// to retain ownership, as we would if we were to do + /// `self.clone().into().encode()`. + fn encode_as(&self) -> Vec; +} + +impl EncodeAs for T { + fn encode_as(&self) -> Vec { + self.encode() + } +} From 68c6690d79cbd1bf1925576e57dd8a3c684023b5 Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Fri, 30 Apr 2021 18:39:44 +0200 Subject: [PATCH 25/26] into_checked -> try_into_checked --- node/network/bitfield-distribution/src/lib.rs | 2 +- node/network/statement-distribution/src/lib.rs | 2 +- node/subsystem-util/src/runtime/mod.rs | 2 +- primitives/src/v1/signed.rs | 2 +- runtime/parachains/src/inclusion.rs | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/node/network/bitfield-distribution/src/lib.rs b/node/network/bitfield-distribution/src/lib.rs index 76820bedfb6b..92c160cfc235 100644 --- a/node/network/bitfield-distribution/src/lib.rs +++ b/node/network/bitfield-distribution/src/lib.rs @@ -483,7 +483,7 @@ where } return; } - let signed_availability = match bitfield.into_checked(&signing_context, &validator) { + let signed_availability = match bitfield.try_into_checked(&signing_context, &validator) { Err(_) => { modify_reputation(ctx, origin, COST_SIGNATURE_INVALID).await; return; diff --git a/node/network/statement-distribution/src/lib.rs b/node/network/statement-distribution/src/lib.rs index db5e7aab7faf..3af3d16c001a 100644 --- a/node/network/statement-distribution/src/lib.rs +++ b/node/network/statement-distribution/src/lib.rs @@ -791,7 +791,7 @@ fn check_statement_signature( head.validators .get(statement.unchecked_validator_index().0 as usize) .ok_or_else(|| statement.clone()) - .and_then(|v| statement.into_checked(&signing_context, v)) + .and_then(|v| statement.try_into_checked(&signing_context, v)) } /// Places the statement in storage if it is new, and then diff --git a/node/subsystem-util/src/runtime/mod.rs b/node/subsystem-util/src/runtime/mod.rs index 165eeb00228e..f07db0763027 100644 --- a/node/subsystem-util/src/runtime/mod.rs +++ b/node/subsystem-util/src/runtime/mod.rs @@ -238,7 +238,7 @@ where session_info.validators .get(signed.unchecked_validator_index().0 as usize) .ok_or_else(|| signed.clone()) - .and_then(|v| signed.into_checked(&signing_context, v)) + .and_then(|v| signed.try_into_checked(&signing_context, v)) } /// Request availability cores from the runtime. diff --git a/primitives/src/v1/signed.rs b/primitives/src/v1/signed.rs index e601858fe3c3..3aa6d964342c 100644 --- a/primitives/src/v1/signed.rs +++ b/primitives/src/v1/signed.rs @@ -162,7 +162,7 @@ impl, RealPayload: Encode> UncheckedSigned( + pub fn try_into_checked( self, context: &SigningContext, key: &ValidatorId diff --git a/runtime/parachains/src/inclusion.rs b/runtime/parachains/src/inclusion.rs index 18751d155c09..a884656f7ad8 100644 --- a/runtime/parachains/src/inclusion.rs +++ b/runtime/parachains/src/inclusion.rs @@ -295,7 +295,7 @@ impl Module { last_index = Some(unchecked_bitfield.unchecked_validator_index()); signed_bitfields.push( - unchecked_bitfield.into_checked( + unchecked_bitfield.try_into_checked( &signing_context, validator_public, ).map_err(|_| Error::::InvalidBitfieldSignature)? From ab3fc3da4bfa0a2df83fdefa22638306d08376db Mon Sep 17 00:00:00 2001 From: Robert Klotzner Date: Mon, 3 May 2021 18:15:20 +0200 Subject: [PATCH 26/26] Fix merge. --- node/core/parachains-inherent/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/node/core/parachains-inherent/src/lib.rs b/node/core/parachains-inherent/src/lib.rs index 9c6b8ed0dbb5..e8ffb573658c 100644 --- a/node/core/parachains-inherent/src/lib.rs +++ b/node/core/parachains-inherent/src/lib.rs @@ -79,7 +79,7 @@ impl ParachainsInherentDataProvider { let inherent_data = match res { Ok(pd) => ParachainsInherentData { - bitfields: pd.bitfields, + bitfields: pd.bitfields.into_iter().map(Into::into).collect(), backed_candidates: pd.backed_candidates, disputes: pd.disputes, parent_header,