From 1d9df92ddaea492c263af8a48ca74a32a76c59bc Mon Sep 17 00:00:00 2001 From: librelois Date: Tue, 23 Nov 2021 18:09:50 +0100 Subject: [PATCH 01/13] Enable signature verification in background task --- frame/executive/Cargo.toml | 5 +- frame/executive/src/lib.rs | 13 +++- .../src/generic/unchecked_extrinsic.rs | 30 ++++++++- primitives/runtime/src/lib.rs | 22 +++++++ primitives/runtime/src/traits.rs | 63 +++++++++++++++++++ 5 files changed, 129 insertions(+), 4 deletions(-) diff --git a/frame/executive/Cargo.toml b/frame/executive/Cargo.toml index 20e31fe0a5720..54b409f51945d 100644 --- a/frame/executive/Cargo.toml +++ b/frame/executive/Cargo.toml @@ -36,7 +36,9 @@ sp-inherents = { version = "4.0.0-dev", path = "../../primitives/inherents" } [features] default = ["std"] -with-tracing = ["sp-tracing/with-tracing"] + +# Enable signature verification in background task +background-sig-check = [] std = [ "codec/std", "scale-info/std", @@ -48,3 +50,4 @@ std = [ "sp-std/std", ] try-runtime = ["frame-support/try-runtime"] +with-tracing = ["sp-tracing/with-tracing"] diff --git a/frame/executive/src/lib.rs b/frame/executive/src/lib.rs index dd0a9abf8687b..e3caff4b8f9b7 100644 --- a/frame/executive/src/lib.rs +++ b/frame/executive/src/lib.rs @@ -128,15 +128,19 @@ use frame_support::{ use sp_runtime::{ generic::Digest, traits::{ - self, Applyable, CheckEqual, Checkable, Dispatchable, Header, NumberFor, One, Saturating, + self, Applyable, CheckEqual, Dispatchable, Header, NumberFor, One, Saturating, ValidateUnsigned, Zero, }, transaction_validity::{TransactionSource, TransactionValidity}, ApplyExtrinsicResult, }; +#[cfg(feature = "background-sig-check")] +use sp_runtime::traits::{BackgroundCheckable as Checkable, Checkable as _}; +#[cfg(not(feature = "background-sig-check"))] +use sp_runtime::traits::Checkable; use sp_std::{marker::PhantomData, prelude::*}; -pub type CheckedOf = >::Checked; +pub type CheckedOf = >::Checked; pub type CallOf = as Applyable>::Call; pub type OriginOf = as Dispatchable>::Origin; @@ -366,12 +370,14 @@ where // any initial checks Self::initial_checks(&block); + #[cfg(feature = "background-sig-check")] let signature_batching = sp_runtime::SignatureBatching::start(); // execute extrinsics let (header, extrinsics) = block.deconstruct(); Self::execute_extrinsics_with_book_keeping(extrinsics, *header.number()); + #[cfg(feature = "background-sig-check")] if !signature_batching.verify() { panic!("Signature verification failed."); } @@ -461,6 +467,9 @@ where sp_tracing::enter_span!(sp_tracing::info_span!("apply_extrinsic", ext=?sp_core::hexdisplay::HexDisplay::from(&uxt.encode()))); // Verify that the signature is good. + #[cfg(feature = "background-sig-check")] + let xt = uxt.background_check(&Default::default())?; + #[cfg(not(feature = "background-sig-check"))] let xt = uxt.check(&Default::default())?; // We don't need to make sure to `note_extrinsic` only after we know it's going to be diff --git a/primitives/runtime/src/generic/unchecked_extrinsic.rs b/primitives/runtime/src/generic/unchecked_extrinsic.rs index 95f4f2f3584d9..76d0363606b3c 100644 --- a/primitives/runtime/src/generic/unchecked_extrinsic.rs +++ b/primitives/runtime/src/generic/unchecked_extrinsic.rs @@ -20,7 +20,7 @@ use crate::{ generic::CheckedExtrinsic, traits::{ - self, Checkable, Extrinsic, ExtrinsicMetadata, IdentifyAccount, MaybeDisplay, Member, + self, BackgroundCheckable, Checkable, Extrinsic, ExtrinsicMetadata, IdentifyAccount, MaybeDisplay, Member, SignedExtension, }, transaction_validity::{InvalidTransaction, TransactionValidityError}, @@ -159,6 +159,34 @@ where } } +impl BackgroundCheckable + for UncheckedExtrinsic +where + Address: Member + MaybeDisplay, + Call: Encode + Member, + Signature: Member + traits::BackgroundVerify, + ::Signer: IdentifyAccount, + Extra: SignedExtension, + AccountId: Member + MaybeDisplay, + Lookup: traits::Lookup, +{ + fn background_check(self, lookup: &Lookup) -> Result { + Ok(match self.signature { + Some((signed, signature, extra)) => { + let signed = lookup.lookup(signed)?; + let raw_payload = SignedPayload::new(self.function, extra)?; + if !raw_payload.using_encoded(|payload| signature.background_verify(payload, &signed)) { + return Err(InvalidTransaction::BadProof.into()) + } + + let (function, extra, _) = raw_payload.deconstruct(); + CheckedExtrinsic { signed: Some((signed, extra)), function } + }, + None => CheckedExtrinsic { signed: None, function: self.function }, + }) + } +} + impl ExtrinsicMetadata for UncheckedExtrinsic where diff --git a/primitives/runtime/src/lib.rs b/primitives/runtime/src/lib.rs index 80293fe734844..3dbaf811cfaf2 100644 --- a/primitives/runtime/src/lib.rs +++ b/primitives/runtime/src/lib.rs @@ -420,6 +420,28 @@ impl Verify for MultiSignature { } } +impl crate::traits::BackgroundVerify for MultiSignature { + fn background_verify>(&self, mut msg: L, signer: &AccountId32) -> bool { + match (self, signer) { + (Self::Ed25519(ref sig), who) => + sig.background_verify(msg, &ed25519::Public::from_slice(who.as_ref())), + (Self::Sr25519(ref sig), who) => + sig.background_verify(msg, &sr25519::Public::from_slice(who.as_ref())), + (Self::Ecdsa(ref sig), who) => { + // Unfortunatly, ecdsa signature can't be verified in a background task + // because we don't known the public key. + let m = sp_io::hashing::blake2_256(msg.get()); + match sp_io::crypto::secp256k1_ecdsa_recover_compressed(sig.as_ref(), &m) { + Ok(pubkey) => + &sp_io::hashing::blake2_256(pubkey.as_ref()) == + >::as_ref(who), + _ => false, + } + }, + } + } +} + /// Signature verify that can work with any known signature types.. #[derive(Eq, PartialEq, Clone, Default, Encode, Decode, RuntimeDebug)] #[cfg_attr(feature = "std", derive(Serialize, Deserialize))] diff --git a/primitives/runtime/src/traits.rs b/primitives/runtime/src/traits.rs index f61de70e35197..55d6806a92ecb 100644 --- a/primitives/runtime/src/traits.rs +++ b/primitives/runtime/src/traits.rs @@ -170,6 +170,57 @@ where } } +/// A signature that supports background verification. +pub trait BackgroundVerify: Verify { + /// Register a signature for background verification. + /// + /// This requires that background verification is enabled by doing XYZ. + /// + /// Returns `true` when the signature was successfully registered for background verification + /// or if background verification is not enabled the signature could be verified successfully + /// immediately. + /// + /// # Warning + /// + /// This requires that the background verification is finished by calling finalize_verify to + /// check the result of all submitted signature verifications. + fn background_verify>( + &self, + msg: L, + signer: &::AccountId, + ) -> bool; +} + +impl BackgroundVerify for sp_core::ed25519::Signature { + fn background_verify>( + &self, + mut msg: L, + signer: &::AccountId, + ) -> bool { + sp_io::crypto::ed25519_batch_verify(self, msg.get(), signer) + } +} + +impl BackgroundVerify for sp_core::sr25519::Signature { + fn background_verify>( + &self, + mut msg: L, + signer: &::AccountId, + ) -> bool { + sp_io::crypto::sr25519_batch_verify(self, msg.get(), signer) + } +} + +impl BackgroundVerify for sp_core::ecdsa::Signature { + fn background_verify>( + &self, + mut msg: L, + signer: &::AccountId, + ) -> bool { + sp_io::crypto::ecdsa_batch_verify(self, msg.get(), signer) + } +} + /// An error type that indicates that the origin is invalid. #[derive(Encode, Decode, RuntimeDebug)] pub struct BadOrigin; @@ -773,6 +824,18 @@ pub trait Checkable: Sized { fn check(self, c: &Context) -> Result; } +/// A piece of information "checkable" in a background task, used by the standard Substrate +/// Executive in order to check the validity of a piece of extrinsic information, usually by +/// verifying the signature. Implement for pieces of information that require some additional +/// context `Context` in order to be checked. +pub trait BackgroundCheckable: Checkable { + /// Check self in a background tas, given an instance of Context. + fn background_check( + self, + c: &Context, + ) -> Result<>::Checked, TransactionValidityError>; +} + /// A "checkable" piece of information, used by the standard Substrate Executive in order to /// check the validity of a piece of extrinsic information, usually by verifying the signature. /// Implement for pieces of information that don't require additional context in order to be From 9a73a446b72f4d2870f73654042d8578e88502b2 Mon Sep 17 00:00:00 2001 From: librelois Date: Mon, 29 Nov 2021 12:36:16 +0100 Subject: [PATCH 02/13] optimize : spawn all background checks before extrinsics execution --- frame/executive/src/lib.rs | 110 ++++++++++++++++-- .../src/generic/unchecked_extrinsic.rs | 8 +- 2 files changed, 104 insertions(+), 14 deletions(-) diff --git a/frame/executive/src/lib.rs b/frame/executive/src/lib.rs index e3caff4b8f9b7..739f80bca4990 100644 --- a/frame/executive/src/lib.rs +++ b/frame/executive/src/lib.rs @@ -125,6 +125,10 @@ use frame_support::{ }, weights::{DispatchClass, DispatchInfo, GetDispatchInfo}, }; +#[cfg(not(feature = "background-sig-check"))] +use sp_runtime::traits::Checkable; +#[cfg(feature = "background-sig-check")] +use sp_runtime::traits::{BackgroundCheckable as Checkable, Checkable as _}; use sp_runtime::{ generic::Digest, traits::{ @@ -134,10 +138,6 @@ use sp_runtime::{ transaction_validity::{TransactionSource, TransactionValidity}, ApplyExtrinsicResult, }; -#[cfg(feature = "background-sig-check")] -use sp_runtime::traits::{BackgroundCheckable as Checkable, Checkable as _}; -#[cfg(not(feature = "background-sig-check"))] -use sp_runtime::traits::Checkable; use sp_std::{marker::PhantomData, prelude::*}; pub type CheckedOf = >::Checked; @@ -359,6 +359,7 @@ where } } + #[cfg(feature = "background-sig-check")] /// Actually execute all transitions for `block`. pub fn execute_block(block: Block) { sp_io::init_tracing(); @@ -370,14 +371,28 @@ where // any initial checks Self::initial_checks(&block); - #[cfg(feature = "background-sig-check")] + + // check extrinsics in background + let (header, extrinsics) = block.deconstruct(); let signature_batching = sp_runtime::SignatureBatching::start(); + let checked_extrinsics = extrinsics + .into_iter() + .map(|extrinsic| { + let encoded = extrinsic.encode(); + match extrinsic.background_check(&Default::default()) { + Ok(checked_extrinsic) => (checked_extrinsic, encoded), + Err(e) => { + let err: &'static str = e.into(); + panic!("{}", err) + } + } + }) + .collect(); // execute extrinsics - let (header, extrinsics) = block.deconstruct(); - Self::execute_extrinsics_with_book_keeping(extrinsics, *header.number()); + Self::execute_checked_extrinsics_with_book_keeping(checked_extrinsics, *header.number()); - #[cfg(feature = "background-sig-check")] + // ensure that all background checks have been completed successfully. if !signature_batching.verify() { panic!("Signature verification failed."); } @@ -387,6 +402,27 @@ where } } + #[cfg(not(feature = "background-sig-check"))] + /// Actually execute all transitions for `block`. + pub fn execute_block(block: Block) { + sp_io::init_tracing(); + sp_tracing::within_span! { + sp_tracing::info_span!("execute_block", ?block); + + Self::initialize_block(block.header()); + + // any initial checks + Self::initial_checks(&block); + + // execute extrinsics + let (header, extrinsics) = block.deconstruct(); + Self::execute_extrinsics_with_book_keeping(extrinsics, *header.number()); + + // any final checks + Self::final_checks(&header); + } + } + /// Execute given extrinsics and take care of post-extrinsics book-keeping. fn execute_extrinsics_with_book_keeping( extrinsics: Vec, @@ -405,6 +441,29 @@ where Self::idle_and_finalize_hook(block_number); } + #[cfg(feature = "background-sig-check")] + /// Execute given checked extrinsics and take care of post-extrinsics book-keeping. + fn execute_checked_extrinsics_with_book_keeping( + checked_extrinsics: Vec<(CheckedOf, Vec)>, + block_number: NumberFor, + ) { + checked_extrinsics.into_iter().for_each( + |(checked_extrinsic, unchecked_extrinsic_encoded)| { + if let Err(e) = + Self::apply_checked_extrinsic(checked_extrinsic, unchecked_extrinsic_encoded) + { + let err: &'static str = e.into(); + panic!("{}", err) + } + }, + ); + + // post-extrinsics book-keeping + >::note_finished_extrinsics(); + + Self::idle_and_finalize_hook(block_number); + } + /// Finalize the block - it is up the caller to ensure that all header fields are valid /// except state-root. pub fn finalize_block() -> System::Header { @@ -467,9 +526,6 @@ where sp_tracing::enter_span!(sp_tracing::info_span!("apply_extrinsic", ext=?sp_core::hexdisplay::HexDisplay::from(&uxt.encode()))); // Verify that the signature is good. - #[cfg(feature = "background-sig-check")] - let xt = uxt.background_check(&Default::default())?; - #[cfg(not(feature = "background-sig-check"))] let xt = uxt.check(&Default::default())?; // We don't need to make sure to `note_extrinsic` only after we know it's going to be @@ -488,6 +544,38 @@ where Ok(r.map(|_| ()).map_err(|e| e.error)) } + #[cfg(feature = "background-sig-check")] + /// Apply checked extrinsic outside of the block execution function. + /// + /// This doesn't attempt to validate anything regarding the block, but it builds a list of uxt + /// hashes. + pub fn apply_checked_extrinsic( + checked_extrinsic: CheckedOf, + unchecked_extrinsic_encoded: Vec, + ) -> ApplyExtrinsicResult { + sp_io::init_tracing(); + sp_tracing::enter_span!(sp_tracing::info_span!("apply_checked_extrinsic", + ext=?sp_core::hexdisplay::HexDisplay::from(&unchecked_extrinsic_encoded))); + + let encoded_len = unchecked_extrinsic_encoded.len(); + + // We don't need to make sure to `note_extrinsic` only after we know it's going to be + // executed to prevent it from leaking in storage since at this point, it will either + // execute or panic (and revert storage changes). + >::note_extrinsic(unchecked_extrinsic_encoded); + + // AUDIT: Under no circumstances may this function panic from here onwards. + + // Decode parameters and dispatch + let dispatch_info = checked_extrinsic.get_dispatch_info(); + let r = + Applyable::apply::(checked_extrinsic, &dispatch_info, encoded_len)?; + + >::note_applied_extrinsic(&r, dispatch_info); + + Ok(r.map(|_| ()).map_err(|e| e.error)) + } + fn final_checks(header: &System::Header) { sp_tracing::enter_span!(sp_tracing::Level::TRACE, "final_checks"); // remove temporaries diff --git a/primitives/runtime/src/generic/unchecked_extrinsic.rs b/primitives/runtime/src/generic/unchecked_extrinsic.rs index 76d0363606b3c..2436167cd2912 100644 --- a/primitives/runtime/src/generic/unchecked_extrinsic.rs +++ b/primitives/runtime/src/generic/unchecked_extrinsic.rs @@ -20,8 +20,8 @@ use crate::{ generic::CheckedExtrinsic, traits::{ - self, BackgroundCheckable, Checkable, Extrinsic, ExtrinsicMetadata, IdentifyAccount, MaybeDisplay, Member, - SignedExtension, + self, BackgroundCheckable, Checkable, Extrinsic, ExtrinsicMetadata, IdentifyAccount, + MaybeDisplay, Member, SignedExtension, }, transaction_validity::{InvalidTransaction, TransactionValidityError}, OpaqueExtrinsic, @@ -175,7 +175,9 @@ where Some((signed, signature, extra)) => { let signed = lookup.lookup(signed)?; let raw_payload = SignedPayload::new(self.function, extra)?; - if !raw_payload.using_encoded(|payload| signature.background_verify(payload, &signed)) { + if !raw_payload + .using_encoded(|payload| signature.background_verify(payload, &signed)) + { return Err(InvalidTransaction::BadProof.into()) } From 9b974d80b19c861973d6481d5c25869e02c89134 Mon Sep 17 00:00:00 2001 From: librelois Date: Mon, 28 Feb 2022 20:18:26 +0100 Subject: [PATCH 03/13] fix warning: fn execute_extrinsics_with_book_keeping not used --- frame/executive/src/lib.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/frame/executive/src/lib.rs b/frame/executive/src/lib.rs index 698ad8622907b..e76b6a29ec8be 100644 --- a/frame/executive/src/lib.rs +++ b/frame/executive/src/lib.rs @@ -420,6 +420,7 @@ where } /// Execute given extrinsics and take care of post-extrinsics book-keeping. + #[cfg(not(feature = "background-sig-check"))] fn execute_extrinsics_with_book_keeping( extrinsics: Vec, block_number: NumberFor, From a222e60bd1822c9ed3a133bf364e33bf6bd2d5b7 Mon Sep 17 00:00:00 2001 From: librelois Date: Wed, 6 Apr 2022 23:21:01 +0200 Subject: [PATCH 04/13] fmt --- client/network/src/protocol/notifications/behaviour.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/client/network/src/protocol/notifications/behaviour.rs b/client/network/src/protocol/notifications/behaviour.rs index e89ad192dc66a..b47216473970e 100644 --- a/client/network/src/protocol/notifications/behaviour.rs +++ b/client/network/src/protocol/notifications/behaviour.rs @@ -695,7 +695,7 @@ impl Notifications { timer: delay_id, timer_deadline: *backoff, }; - } + }, // Disabled => Enabled PeerState::Disabled { mut connections, backoff_until } => { @@ -2077,7 +2077,7 @@ impl NetworkBehaviour for Notifications { .boxed(), ); } - } + }, // We intentionally never remove elements from `delays`, and it may // thus contain obsolete entries. This is a normal situation. From 80baeb1853f6b5d0dad07f684532decaca975258 Mon Sep 17 00:00:00 2001 From: librelois Date: Wed, 13 Apr 2022 21:33:43 +0200 Subject: [PATCH 05/13] refactor in a more generic way --- frame/executive/Cargo.toml | 2 +- frame/executive/src/lib.rs | 148 +++++++++++++------------------------ 2 files changed, 53 insertions(+), 97 deletions(-) diff --git a/frame/executive/Cargo.toml b/frame/executive/Cargo.toml index 01235404bb24d..cb8eed2c4e167 100644 --- a/frame/executive/Cargo.toml +++ b/frame/executive/Cargo.toml @@ -38,7 +38,7 @@ sp-inherents = { version = "4.0.0-dev", path = "../../primitives/inherents" } default = ["std"] # Enable signature verification in background task -background-sig-check = [] +background-signature-verification = [] std = [ "codec/std", "scale-info/std", diff --git a/frame/executive/src/lib.rs b/frame/executive/src/lib.rs index e76b6a29ec8be..c86d7bd41e5be 100644 --- a/frame/executive/src/lib.rs +++ b/frame/executive/src/lib.rs @@ -125,9 +125,9 @@ use frame_support::{ }, weights::{DispatchClass, DispatchInfo, GetDispatchInfo}, }; -#[cfg(not(feature = "background-sig-check"))] +#[cfg(not(feature = "background-signature-verification"))] use sp_runtime::traits::Checkable; -#[cfg(feature = "background-sig-check")] +#[cfg(feature = "background-signature-verification")] use sp_runtime::traits::{BackgroundCheckable as Checkable, Checkable as _}; use sp_runtime::{ generic::Digest, @@ -239,9 +239,19 @@ where Self::initialize_block(block.header()); Self::initial_checks(&block); + #[cfg(feature = "background-signature-verification")] + let signature_batching = sp_runtime::SignatureBatching::start(); + let (header, extrinsics) = block.deconstruct(); + let checked_extrinsics = check_extrinsics(extrinsics); + + Self::execute_checked_extrinsics_with_book_keeping(checked_extrinsics, *header.number()); - Self::execute_extrinsics_with_book_keeping(extrinsics, *header.number()); + #[cfg(feature = "background-signature-verification")] + // ensure that all background checks have been completed successfully. + if !signature_batching.verify() { + panic!("Signature verification failed."); + } // do some of the checks that would normally happen in `final_checks`, but definitely skip // the state root check. @@ -355,7 +365,33 @@ where } } - #[cfg(feature = "background-sig-check")] + fn check_extrinsics( + extrinsics: Vec, + ) -> Vec<(CheckedOf, Vec)> { + extrinsics + .into_iter() + .map(|extrinsic| { + let encoded = extrinsic.encode(); + #[cfg(feature = "background-signature-verification")] + match extrinsic.background_check(&Default::default()) { + Ok(checked_extrinsic) => (checked_extrinsic, encoded), + Err(e) => { + let err: &'static str = e.into(); + panic!("{}", err) + }, + } + #[cfg(not(feature = "background-signature-verification"))] + match extrinsic.check(&Default::default()) { + Ok(checked_extrinsic) => (checked_extrinsic, encoded), + Err(e) => { + let err: &'static str = e.into(); + panic!("{}", err) + }, + } + }) + .collect() + } + /// Actually execute all transitions for `block`. pub fn execute_block(block: Block) { sp_io::init_tracing(); @@ -367,27 +403,17 @@ where // any initial checks Self::initial_checks(&block); + #[cfg(feature = "background-signature-verification")] + let signature_batching = sp_runtime::SignatureBatching::start(); // check extrinsics in background let (header, extrinsics) = block.deconstruct(); - let signature_batching = sp_runtime::SignatureBatching::start(); - let checked_extrinsics = extrinsics - .into_iter() - .map(|extrinsic| { - let encoded = extrinsic.encode(); - match extrinsic.background_check(&Default::default()) { - Ok(checked_extrinsic) => (checked_extrinsic, encoded), - Err(e) => { - let err: &'static str = e.into(); - panic!("{}", err) - } - } - }) - .collect(); + let checked_extrinsics = Self::check_extrinsics(extrinsics); // execute extrinsics Self::execute_checked_extrinsics_with_book_keeping(checked_extrinsics, *header.number()); + #[cfg(feature = "background-signature-verification")] // ensure that all background checks have been completed successfully. if !signature_batching.verify() { panic!("Signature verification failed."); @@ -398,47 +424,6 @@ where } } - #[cfg(not(feature = "background-sig-check"))] - /// Actually execute all transitions for `block`. - pub fn execute_block(block: Block) { - sp_io::init_tracing(); - sp_tracing::within_span! { - sp_tracing::info_span!("execute_block", ?block); - - Self::initialize_block(block.header()); - - // any initial checks - Self::initial_checks(&block); - - // execute extrinsics - let (header, extrinsics) = block.deconstruct(); - Self::execute_extrinsics_with_book_keeping(extrinsics, *header.number()); - - // any final checks - Self::final_checks(&header); - } - } - - /// Execute given extrinsics and take care of post-extrinsics book-keeping. - #[cfg(not(feature = "background-sig-check"))] - fn execute_extrinsics_with_book_keeping( - extrinsics: Vec, - block_number: NumberFor, - ) { - extrinsics.into_iter().for_each(|e| { - if let Err(e) = Self::apply_extrinsic(e) { - let err: &'static str = e.into(); - panic!("{}", err) - } - }); - - // post-extrinsics book-keeping - >::note_finished_extrinsics(); - - Self::idle_and_finalize_hook(block_number); - } - - #[cfg(feature = "background-sig-check")] /// Execute given checked extrinsics and take care of post-extrinsics book-keeping. fn execute_checked_extrinsics_with_book_keeping( checked_extrinsics: Vec<(CheckedOf, Vec)>, @@ -447,7 +432,7 @@ where checked_extrinsics.into_iter().for_each( |(checked_extrinsic, unchecked_extrinsic_encoded)| { if let Err(e) = - Self::apply_checked_extrinsic(checked_extrinsic, unchecked_extrinsic_encoded) + Self::apply_extrinsic_with_len(checked_extrinsic, unchecked_extrinsic_encoded) { let err: &'static str = e.into(); panic!("{}", err) @@ -497,51 +482,22 @@ where /// /// This doesn't attempt to validate anything regarding the block, but it builds a list of uxt /// hashes. - pub fn apply_extrinsic(uxt: Block::Extrinsic) -> ApplyExtrinsicResult { + pub fn apply_extrinsic(unchecked_extrinsic: Block::Extrinsic) -> ApplyExtrinsicResult { sp_io::init_tracing(); - let encoded = uxt.encode(); - let encoded_len = encoded.len(); - Self::apply_extrinsic_with_len(uxt, encoded_len, encoded) - } + let unchecked_extrinsic_encoded = unchecked_extrinsic.encode(); - /// Actually apply an extrinsic given its `encoded_len`; this doesn't note its hash. - fn apply_extrinsic_with_len( - uxt: Block::Extrinsic, - encoded_len: usize, - to_note: Vec, - ) -> ApplyExtrinsicResult { - sp_tracing::enter_span!(sp_tracing::info_span!("apply_extrinsic", - ext=?sp_core::hexdisplay::HexDisplay::from(&uxt.encode()))); // Verify that the signature is good. - let xt = uxt.check(&Default::default())?; - - // We don't need to make sure to `note_extrinsic` only after we know it's going to be - // executed to prevent it from leaking in storage since at this point, it will either - // execute or panic (and revert storage changes). - >::note_extrinsic(to_note); - - // AUDIT: Under no circumstances may this function panic from here onwards. - - // Decode parameters and dispatch - let dispatch_info = xt.get_dispatch_info(); - let r = Applyable::apply::(xt, &dispatch_info, encoded_len)?; - - >::note_applied_extrinsic(&r, dispatch_info); + let checked_extrinsic = unchecked_extrinsic.check(&Default::default())?; - Ok(r.map(|_| ()).map_err(|e| e.error)) + Self::apply_extrinsic_with_len(checked_extrinsic, unchecked_extrinsic_encoded) } - #[cfg(feature = "background-sig-check")] - /// Apply checked extrinsic outside of the block execution function. - /// - /// This doesn't attempt to validate anything regarding the block, but it builds a list of uxt - /// hashes. - pub fn apply_checked_extrinsic( + /// Actually apply an extrinsic given its `encoded_len`; this doesn't note its hash. + fn apply_extrinsic_with_len( checked_extrinsic: CheckedOf, unchecked_extrinsic_encoded: Vec, ) -> ApplyExtrinsicResult { - sp_io::init_tracing(); - sp_tracing::enter_span!(sp_tracing::info_span!("apply_checked_extrinsic", + sp_tracing::enter_span!(sp_tracing::info_span!("apply_extrinsic", ext=?sp_core::hexdisplay::HexDisplay::from(&unchecked_extrinsic_encoded))); let encoded_len = unchecked_extrinsic_encoded.len(); From 4a027725d6068aad8a9871ca70759b059606e9af Mon Sep 17 00:00:00 2001 From: librelois Date: Wed, 13 Apr 2022 21:44:12 +0200 Subject: [PATCH 06/13] add ci job test-frame-executive-features-compile-to-wasm --- .gitlab-ci.yml | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index d196ade85a6fb..9297d0856d4fa 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -406,6 +406,22 @@ test-frame-examples-compile-to-wasm: - cargo +nightly build --target=wasm32-unknown-unknown --no-default-features - sccache -s +test-frame-executive-features-compile-to-wasm: + # into one job + stage: test + <<: *docker-env + <<: *test-refs + variables: + <<: *default-vars + # Enable debug assertions since we are running optimized builds for testing + # but still want to have debug assertions. + RUSTFLAGS: "-Cdebug-assertions=y" + RUST_BACKTRACE: 1 + script: + - cd ./frame/executive + - cargo +nightly build --target=wasm32-unknown-unknown --no-default-features --features background-signature-verification + - sccache -s + test-linux-stable-int: <<: *test-linux stage: test From 9409c4ca3c8077e6359d54e9aeb0ef66436d0bca Mon Sep 17 00:00:00 2001 From: librelois Date: Thu, 12 May 2022 13:47:33 +0200 Subject: [PATCH 07/13] apply review suggestions --- .gitlab-ci.yml | 2 +- frame/executive/src/lib.rs | 63 +++++++++++++++----------------- primitives/runtime/src/traits.rs | 18 ++++----- 3 files changed, 38 insertions(+), 45 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 9297d0856d4fa..71983e41c7407 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -419,7 +419,7 @@ test-frame-executive-features-compile-to-wasm: RUST_BACKTRACE: 1 script: - cd ./frame/executive - - cargo +nightly build --target=wasm32-unknown-unknown --no-default-features --features background-signature-verification + - cargo test --no-default-features --features background-signature-verification - sccache -s test-linux-stable-int: diff --git a/frame/executive/src/lib.rs b/frame/executive/src/lib.rs index c86d7bd41e5be..c52cbddec3aa9 100644 --- a/frame/executive/src/lib.rs +++ b/frame/executive/src/lib.rs @@ -367,29 +367,26 @@ where fn check_extrinsics( extrinsics: Vec, - ) -> Vec<(CheckedOf, Vec)> { - extrinsics - .into_iter() - .map(|extrinsic| { - let encoded = extrinsic.encode(); - #[cfg(feature = "background-signature-verification")] - match extrinsic.background_check(&Default::default()) { - Ok(checked_extrinsic) => (checked_extrinsic, encoded), - Err(e) => { - let err: &'static str = e.into(); - panic!("{}", err) - }, - } - #[cfg(not(feature = "background-signature-verification"))] - match extrinsic.check(&Default::default()) { - Ok(checked_extrinsic) => (checked_extrinsic, encoded), - Err(e) => { - let err: &'static str = e.into(); - panic!("{}", err) - }, - } - }) - .collect() + ) -> impl Iterator, Vec)> { + extrinsics.into_iter().map(|extrinsic| { + let encoded = extrinsic.encode(); + #[cfg(feature = "background-signature-verification")] + match extrinsic.background_check(&Default::default()) { + Ok(checked_extrinsic) => (checked_extrinsic, encoded), + Err(e) => { + let err: &'static str = e.into(); + panic!("{}", err) + }, + } + #[cfg(not(feature = "background-signature-verification"))] + match extrinsic.check(&Default::default()) { + Ok(checked_extrinsic) => (checked_extrinsic, encoded), + Err(e) => { + let err: &'static str = e.into(); + panic!("{}", err) + }, + } + }) } /// Actually execute all transitions for `block`. @@ -426,19 +423,17 @@ where /// Execute given checked extrinsics and take care of post-extrinsics book-keeping. fn execute_checked_extrinsics_with_book_keeping( - checked_extrinsics: Vec<(CheckedOf, Vec)>, + checked_extrinsics: impl Iterator, Vec)>, block_number: NumberFor, ) { - checked_extrinsics.into_iter().for_each( - |(checked_extrinsic, unchecked_extrinsic_encoded)| { - if let Err(e) = - Self::apply_extrinsic_with_len(checked_extrinsic, unchecked_extrinsic_encoded) - { - let err: &'static str = e.into(); - panic!("{}", err) - } - }, - ); + checked_extrinsics.for_each(|(checked_extrinsic, unchecked_extrinsic_encoded)| { + if let Err(e) = + Self::apply_extrinsic_with_len(checked_extrinsic, unchecked_extrinsic_encoded) + { + let err: &'static str = e.into(); + panic!("{}", err) + } + }); // post-extrinsics book-keeping >::note_finished_extrinsics(); diff --git a/primitives/runtime/src/traits.rs b/primitives/runtime/src/traits.rs index 9c28c00481648..6e3157e02eb7f 100644 --- a/primitives/runtime/src/traits.rs +++ b/primitives/runtime/src/traits.rs @@ -164,20 +164,18 @@ where } } -/// A signature that supports background verification. +/// Something that supports background verification. pub trait BackgroundVerify: Verify { /// Register a signature for background verification. /// - /// This requires that background verification is enabled by doing XYZ. - /// /// Returns `true` when the signature was successfully registered for background verification /// or if background verification is not enabled the signature could be verified successfully /// immediately. /// - /// # Warning + /// # Security /// - /// This requires that the background verification is finished by calling finalize_verify to - /// check the result of all submitted signature verifications. + /// It is required that this is called in a [`SignatureBatching`](crate::SignatureBatching) + /// context. fn background_verify>( &self, msg: L, @@ -820,10 +818,10 @@ pub trait Checkable: Sized { fn check(self, c: &Context) -> Result; } -/// A piece of information "checkable" in a background task, used by the standard Substrate -/// Executive in order to check the validity of a piece of extrinsic information, usually by -/// verifying the signature. Implement for pieces of information that require some additional -/// context `Context` in order to be checked. +/// A "checkable" piece of information, used by the standard Substrate Executive in order to +/// check the validity of a piece of extrinsic information, usually by verifying the signature. Any +/// signature verification should be done using [`BackgroundVerify`]. Implement for pieces of +/// information that require some additional `Context` in order to be checked. pub trait BackgroundCheckable: Checkable { /// Check self in a background tas, given an instance of Context. fn background_check( From a4765f4818dfef6d576bfbe0405309593ec6e957 Mon Sep 17 00:00:00 2001 From: librelois Date: Thu, 12 May 2022 13:47:33 +0200 Subject: [PATCH 08/13] apply review suggestions 2 --- frame/executive/src/lib.rs | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/frame/executive/src/lib.rs b/frame/executive/src/lib.rs index c52cbddec3aa9..1a45c33d2f46e 100644 --- a/frame/executive/src/lib.rs +++ b/frame/executive/src/lib.rs @@ -247,8 +247,8 @@ where Self::execute_checked_extrinsics_with_book_keeping(checked_extrinsics, *header.number()); - #[cfg(feature = "background-signature-verification")] // ensure that all background checks have been completed successfully. + #[cfg(feature = "background-signature-verification")] if !signature_batching.verify() { panic!("Signature verification failed."); } @@ -370,16 +370,13 @@ where ) -> impl Iterator, Vec)> { extrinsics.into_iter().map(|extrinsic| { let encoded = extrinsic.encode(); + #[cfg(feature = "background-signature-verification")] - match extrinsic.background_check(&Default::default()) { - Ok(checked_extrinsic) => (checked_extrinsic, encoded), - Err(e) => { - let err: &'static str = e.into(); - panic!("{}", err) - }, - } + let checked_extrinsic = extrinsic.background_check(&Default::default()); #[cfg(not(feature = "background-signature-verification"))] - match extrinsic.check(&Default::default()) { + let checked_extrinsic = extrinsic.check(&Default::default()); + + match checked_extrinsic { Ok(checked_extrinsic) => (checked_extrinsic, encoded), Err(e) => { let err: &'static str = e.into(); @@ -410,8 +407,8 @@ where // execute extrinsics Self::execute_checked_extrinsics_with_book_keeping(checked_extrinsics, *header.number()); - #[cfg(feature = "background-signature-verification")] // ensure that all background checks have been completed successfully. + #[cfg(feature = "background-signature-verification")] if !signature_batching.verify() { panic!("Signature verification failed."); } From 0c4b0027c0e9975991ba594bb47054df371b6150 Mon Sep 17 00:00:00 2001 From: librelois Date: Thu, 2 Jun 2022 13:59:05 +0200 Subject: [PATCH 09/13] rename SignatureBatching -> BackgroundVerifyContext --- frame/executive/src/lib.rs | 4 ++-- primitives/runtime/src/lib.rs | 22 ++++++++++++---------- primitives/runtime/src/traits.rs | 4 ++-- 3 files changed, 16 insertions(+), 14 deletions(-) diff --git a/frame/executive/src/lib.rs b/frame/executive/src/lib.rs index 0342f991ae6e7..1d98ed0616938 100644 --- a/frame/executive/src/lib.rs +++ b/frame/executive/src/lib.rs @@ -240,7 +240,7 @@ where Self::initial_checks(&block); #[cfg(feature = "background-signature-verification")] - let signature_batching = sp_runtime::SignatureBatching::start(); + let signature_batching = sp_runtime::BackgroundVerifyContext::start(); let (header, extrinsics) = block.deconstruct(); let checked_extrinsics = check_extrinsics(extrinsics); @@ -398,7 +398,7 @@ where Self::initial_checks(&block); #[cfg(feature = "background-signature-verification")] - let signature_batching = sp_runtime::SignatureBatching::start(); + let signature_batching = sp_runtime::BackgroundVerifyContext::start(); // check extrinsics in background let (header, extrinsics) = block.deconstruct(); diff --git a/primitives/runtime/src/lib.rs b/primitives/runtime/src/lib.rs index ec95cc8fbd259..e6c220c101aa2 100644 --- a/primitives/runtime/src/lib.rs +++ b/primitives/runtime/src/lib.rs @@ -918,18 +918,18 @@ pub fn print(print: impl traits::Printable) { print.print(); } -/// Batching session. +/// Background verification context. /// /// To be used in runtime only. Outside of runtime, just construct /// `BatchVerifier` directly. -#[must_use = "`verify()` needs to be called to finish batch signature verification!"] -pub struct SignatureBatching(bool); +#[must_use = "`verify()` needs to be called to finish background verifications!"] +pub struct BackgroundVerifyContext(bool); -impl SignatureBatching { - /// Start new batching session. +impl BackgroundVerifyContext { + /// Start new background verification context. pub fn start() -> Self { sp_io::crypto::start_batch_verify(); - SignatureBatching(false) + BackgroundVerifyContext(false) } /// Verify all signatures submitted during the batching session. @@ -940,14 +940,16 @@ impl SignatureBatching { } } -impl Drop for SignatureBatching { +impl Drop for BackgroundVerifyContext { fn drop(&mut self) { // Sanity check. If user forgets to actually call `verify()`. // // We should not panic if the current thread is already panicking, // because Rust otherwise aborts the process. if !self.0 && !sp_std::thread::panicking() { - panic!("Signature verification has not been called before `SignatureBatching::drop`") + panic!( + "Signature verification has not been called before `BackgroundVerifyContext::drop`" + ) } } } @@ -1068,7 +1070,7 @@ mod tests { )); ext.execute_with(|| { - let _batching = SignatureBatching::start(); + let _batching = BackgroundVerifyContext::start(); let dummy = UncheckedFrom::unchecked_from([1; 32]); let dummy_sig = UncheckedFrom::unchecked_from([1; 64]); sp_io::crypto::sr25519_verify(&dummy_sig, &Vec::new(), &dummy); @@ -1084,7 +1086,7 @@ mod tests { )); ext.execute_with(|| { - let _batching = SignatureBatching::start(); + let _batching = BackgroundVerifyContext::start(); panic!("Hey, I'm an error"); }); } diff --git a/primitives/runtime/src/traits.rs b/primitives/runtime/src/traits.rs index 00595b969b69a..4b0387925295d 100644 --- a/primitives/runtime/src/traits.rs +++ b/primitives/runtime/src/traits.rs @@ -174,8 +174,8 @@ pub trait BackgroundVerify: Verify { /// /// # Security /// - /// It is required that this is called in a [`SignatureBatching`](crate::SignatureBatching) - /// context. + /// It is required that this is called in a + /// [`BackgroundVerifyContext`](crate::BackgroundVerifyContext) context. fn background_verify>( &self, msg: L, From 1bde334d5f643e21bd3ff660f8efb28eb10dec25 Mon Sep 17 00:00:00 2001 From: librelois Date: Thu, 21 Jul 2022 13:33:33 +0200 Subject: [PATCH 10/13] fix try runtime build --- frame/executive/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frame/executive/src/lib.rs b/frame/executive/src/lib.rs index 804b4b62afc45..29cf4b0c07586 100644 --- a/frame/executive/src/lib.rs +++ b/frame/executive/src/lib.rs @@ -243,7 +243,7 @@ where let signature_batching = sp_runtime::BackgroundVerifyContext::start(); let (header, extrinsics) = block.deconstruct(); - let checked_extrinsics = check_extrinsics(extrinsics); + let checked_extrinsics = Self::check_extrinsics(extrinsics); Self::execute_checked_extrinsics_with_book_keeping(checked_extrinsics, *header.number()); @@ -251,7 +251,7 @@ where #[cfg(feature = "background-signature-verification")] if !signature_batching.verify() { panic!("Signature verification failed."); - } + }àabàabàab" // do some of the checks that would normally happen in `final_checks`, but definitely skip // the state root check. From 57275330f5618c9b09130d2751272cd4a89740fa Mon Sep 17 00:00:00 2001 From: librelois Date: Thu, 21 Jul 2022 15:42:45 +0200 Subject: [PATCH 11/13] fix tests --- primitives/runtime/src/testing.rs | 9 +++++++++ scripts/ci/gitlab/pipeline/test.yml | 2 +- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/primitives/runtime/src/testing.rs b/primitives/runtime/src/testing.rs index 003fa62c9e088..c7c69882a6b5d 100644 --- a/primitives/runtime/src/testing.rs +++ b/primitives/runtime/src/testing.rs @@ -334,6 +334,15 @@ impl Checkable for TestXt crate::traits::BackgroundCheckable for TestXt { + fn background_check( + self, + c: &Context, + ) -> Result<>::Checked, TransactionValidityError> { + Ok(self) + } +} + impl traits::Extrinsic for TestXt { type Call = Call; type SignaturePayload = (u64, Extra); diff --git a/scripts/ci/gitlab/pipeline/test.yml b/scripts/ci/gitlab/pipeline/test.yml index 644c573e22f28..0b8963333fe86 100644 --- a/scripts/ci/gitlab/pipeline/test.yml +++ b/scripts/ci/gitlab/pipeline/test.yml @@ -325,7 +325,7 @@ test-frame-executive-features-compile-to-wasm: RUST_BACKTRACE: 1 script: - cd ./frame/executive - - cargo test --no-default-features --features background-signature-verification + - cargo test --features background-signature-verification test-linux-stable-int: stage: test From 625ce0638e0f371cfb33b307d7b372f7b1a0bc72 Mon Sep 17 00:00:00 2001 From: librelois Date: Thu, 21 Jul 2022 16:20:45 +0200 Subject: [PATCH 12/13] fmt --- primitives/runtime/src/testing.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/primitives/runtime/src/testing.rs b/primitives/runtime/src/testing.rs index c7c69882a6b5d..f8769fe3b1169 100644 --- a/primitives/runtime/src/testing.rs +++ b/primitives/runtime/src/testing.rs @@ -334,7 +334,9 @@ impl Checkable for TestXt crate::traits::BackgroundCheckable for TestXt { +impl crate::traits::BackgroundCheckable + for TestXt +{ fn background_check( self, c: &Context, From df3636e939208671c296e0e1c1a1bca853df2647 Mon Sep 17 00:00:00 2001 From: librelois Date: Thu, 21 Jul 2022 16:36:00 +0200 Subject: [PATCH 13/13] fix warnings --- primitives/runtime/src/testing.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/primitives/runtime/src/testing.rs b/primitives/runtime/src/testing.rs index f8769fe3b1169..2effd748041c1 100644 --- a/primitives/runtime/src/testing.rs +++ b/primitives/runtime/src/testing.rs @@ -339,7 +339,7 @@ impl crate::traits::BackgroundCheckab { fn background_check( self, - c: &Context, + _: &Context, ) -> Result<>::Checked, TransactionValidityError> { Ok(self) }