From 2b8a17365612c1d783f022596726fd6da37f42b0 Mon Sep 17 00:00:00 2001 From: Svyatoslav Nikolsky Date: Mon, 16 May 2022 14:07:17 +0300 Subject: [PATCH] CheckBridgedBlockNumber signed extension to reject duplicate header-submit transactions (#1352) * CheckBridgedBlockNumber signed extension to reject duplicate header submit transactions * fix depends_on --- bridges/bin/millau/runtime/src/lib.rs | 7 + bridges/modules/grandpa/src/extension.rs | 171 +++++++++++++++++++++++ bridges/modules/grandpa/src/lib.rs | 5 +- bridges/modules/grandpa/src/mock.rs | 2 +- bridges/relays/client-millau/Cargo.toml | 1 + bridges/relays/client-millau/src/lib.rs | 2 + 6 files changed, 185 insertions(+), 3 deletions(-) create mode 100644 bridges/modules/grandpa/src/extension.rs diff --git a/bridges/bin/millau/runtime/src/lib.rs b/bridges/bin/millau/runtime/src/lib.rs index 178ec2e18e567..9d59d85d88e2a 100644 --- a/bridges/bin/millau/runtime/src/lib.rs +++ b/bridges/bin/millau/runtime/src/lib.rs @@ -500,6 +500,12 @@ construct_runtime!( } ); +pallet_bridge_grandpa::declare_check_bridged_block_number_ext! { + Runtime, + Call::BridgeRialtoGrandpa => RialtoGrandpaInstance, + Call::BridgeWestendGrandpa => WestendGrandpaInstance +} + /// The address format for describing accounts. pub type Address = AccountId; /// Block header type as expected by this runtime. @@ -520,6 +526,7 @@ pub type SignedExtra = ( frame_system::CheckNonce, frame_system::CheckWeight, pallet_transaction_payment::ChargeTransactionPayment, + CheckBridgedBlockNumber, ); /// The payload being signed in transactions. pub type SignedPayload = generic::SignedPayload; diff --git a/bridges/modules/grandpa/src/extension.rs b/bridges/modules/grandpa/src/extension.rs new file mode 100644 index 0000000000000..bddb722c92b83 --- /dev/null +++ b/bridges/modules/grandpa/src/extension.rs @@ -0,0 +1,171 @@ +// Copyright 2021 Parity Technologies (UK) Ltd. +// This file is part of Parity Bridges Common. + +// Parity Bridges Common 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. + +// Parity Bridges Common 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 Parity Bridges Common. If not, see . + +/// Declares a runtime-specific `CheckBridgedBlockNumber` signed extension. +/// +/// ## Example +/// +/// ```nocompile +/// pallet_bridge_grandpa::declare_check_bridged_block_number_ext!{ +/// Runtime, +/// Call::BridgeRialtoGrandpa => RialtoGrandpaInstance, +/// Call::BridgeWestendGrandpa => WestendGrandpaInstance, +/// } +/// ``` +#[macro_export] +macro_rules! declare_check_bridged_block_number_ext { + ($runtime:ident, $($call:path => $instance:ty),*) => { + /// Transaction-with-obsolete-bridged-header check that will reject transaction if + /// it submits obsolete bridged header. + #[derive(Clone, codec::Decode, codec::Encode, Eq, PartialEq, frame_support::RuntimeDebug, scale_info::TypeInfo)] + pub struct CheckBridgedBlockNumber; + + impl sp_runtime::traits::SignedExtension for CheckBridgedBlockNumber { + const IDENTIFIER: &'static str = "CheckBridgedBlockNumber"; + type AccountId = <$runtime as frame_system::Config>::AccountId; + type Call = <$runtime as frame_system::Config>::Call; + type AdditionalSigned = (); + type Pre = (); + + fn additional_signed(&self) -> sp_std::result::Result< + (), + sp_runtime::transaction_validity::TransactionValidityError, + > { + Ok(()) + } + + fn validate( + &self, + _who: &Self::AccountId, + call: &Self::Call, + _info: &sp_runtime::traits::DispatchInfoOf, + _len: usize, + ) -> sp_runtime::transaction_validity::TransactionValidity { + match *call { + $( + $call($crate::Call::<$runtime, $instance>::submit_finality_proof { ref finality_target, ..}) => { + use sp_runtime::traits::Header as HeaderT; + + let bundled_block_number = *finality_target.number(); + + let best_finalized_hash = $crate::BestFinalized::<$runtime, $instance>::get(); + let best_finalized_number = match $crate::ImportedHeaders::< + $runtime, + $instance, + >::get(best_finalized_hash) { + Some(best_finalized_header) => *best_finalized_header.number(), + None => return sp_runtime::transaction_validity::InvalidTransaction::Call.into(), + }; + + if best_finalized_number < bundled_block_number { + Ok(sp_runtime::transaction_validity::ValidTransaction::default()) + } else { + sp_runtime::transaction_validity::InvalidTransaction::Stale.into() + } + }, + )* + _ => Ok(sp_runtime::transaction_validity::ValidTransaction::default()), + } + } + + fn pre_dispatch( + self, + who: &Self::AccountId, + call: &Self::Call, + info: &sp_runtime::traits::DispatchInfoOf, + len: usize, + ) -> Result { + self.validate(who, call, info, len).map(drop) + } + + fn post_dispatch( + _maybe_pre: Option, + _info: &sp_runtime::traits::DispatchInfoOf, + _post_info: &sp_runtime::traits::PostDispatchInfoOf, + _len: usize, + _result: &sp_runtime::DispatchResult, + ) -> Result<(), sp_runtime::transaction_validity::TransactionValidityError> { + Ok(()) + } + } + }; +} + +#[cfg(test)] +mod tests { + use crate::{ + mock::{run_test, test_header, Call, TestNumber, TestRuntime}, + BestFinalized, ImportedHeaders, + }; + use bp_test_utils::make_default_justification; + use frame_support::weights::{DispatchClass, DispatchInfo, Pays}; + use sp_runtime::traits::SignedExtension; + + declare_check_bridged_block_number_ext! { + TestRuntime, + Call::Grandpa => () + } + + fn validate_block_submit(num: TestNumber) -> bool { + CheckBridgedBlockNumber + .validate( + &42, + &Call::Grandpa(crate::Call::::submit_finality_proof { + finality_target: Box::new(test_header(num)), + justification: make_default_justification(&test_header(num)), + }), + &DispatchInfo { weight: 0, class: DispatchClass::Operational, pays_fee: Pays::Yes }, + 0, + ) + .is_ok() + } + + fn sync_to_header_10() { + let header10_hash = sp_core::H256::default(); + BestFinalized::::put(header10_hash); + ImportedHeaders::::insert(header10_hash, test_header(10)); + } + + #[test] + fn check_bridged_block_number_rejects_obsolete_header() { + run_test(|| { + // when current best finalized is #10 and we're trying to import header#5 => tx is + // rejected + sync_to_header_10(); + assert!(!validate_block_submit(5)); + }); + } + + #[test] + fn check_bridged_block_number_rejects_same_header() { + run_test(|| { + // when current best finalized is #10 and we're trying to import header#10 => tx is + // rejected + sync_to_header_10(); + assert!(!validate_block_submit(10)); + }); + } + + #[test] + fn check_bridged_block_number_accepts_new_header() { + run_test(|| { + // when current best finalized is #10 and we're trying to import header#15 => tx is + // accepted + sync_to_header_10(); + assert!(validate_block_submit(15)); + }); + } +} diff --git a/bridges/modules/grandpa/src/lib.rs b/bridges/modules/grandpa/src/lib.rs index 105dfb15ac703..4184464231286 100644 --- a/bridges/modules/grandpa/src/lib.rs +++ b/bridges/modules/grandpa/src/lib.rs @@ -45,10 +45,11 @@ use sp_finality_grandpa::{ConsensusLog, GRANDPA_ENGINE_ID}; use sp_runtime::traits::{BadOrigin, Header as HeaderT, Zero}; use sp_std::{boxed::Box, convert::TryInto}; +mod extension; #[cfg(test)] mod mock; -/// Pallet containing weights for this pallet. +/// Module, containing weights for this pallet. pub mod weights; #[cfg(feature = "runtime-benchmarks")] @@ -269,7 +270,7 @@ pub mod pallet { /// Hash of the best finalized header. #[pallet::storage] - pub(super) type BestFinalized, I: 'static = ()> = + pub type BestFinalized, I: 'static = ()> = StorageValue<_, BridgedBlockHash, ValueQuery>; /// A ring buffer of imported hashes. Ordered by the insertion time. diff --git a/bridges/modules/grandpa/src/mock.rs b/bridges/modules/grandpa/src/mock.rs index bfc749d5230c6..a0327761fc850 100644 --- a/bridges/modules/grandpa/src/mock.rs +++ b/bridges/modules/grandpa/src/mock.rs @@ -42,7 +42,7 @@ construct_runtime! { UncheckedExtrinsic = UncheckedExtrinsic, { System: frame_system::{Pallet, Call, Config, Storage, Event}, - Grandpa: grandpa::{Pallet}, + Grandpa: grandpa::{Pallet, Call}, } } diff --git a/bridges/relays/client-millau/Cargo.toml b/bridges/relays/client-millau/Cargo.toml index 9893243345518..ab342f63ad9a4 100644 --- a/bridges/relays/client-millau/Cargo.toml +++ b/bridges/relays/client-millau/Cargo.toml @@ -15,6 +15,7 @@ relay-utils = { path = "../utils" } bp-messages = { path = "../../primitives/messages" } bp-millau = { path = "../../primitives/chain-millau" } millau-runtime = { path = "../../bin/millau/runtime" } +pallet-bridge-grandpa = { path = "../../modules/grandpa" } # Substrate Dependencies diff --git a/bridges/relays/client-millau/src/lib.rs b/bridges/relays/client-millau/src/lib.rs index eae9d9b4586a5..cefc722168e46 100644 --- a/bridges/relays/client-millau/src/lib.rs +++ b/bridges/relays/client-millau/src/lib.rs @@ -113,6 +113,7 @@ impl TransactionSignScheme for Millau { frame_system::CheckNonce::::from(param.unsigned.nonce), frame_system::CheckWeight::::new(), pallet_transaction_payment::ChargeTransactionPayment::::from(param.unsigned.tip), + millau_runtime::CheckBridgedBlockNumber, // TODO ), ( (), @@ -123,6 +124,7 @@ impl TransactionSignScheme for Millau { (), (), (), + (), ), ); let signature = raw_payload.using_encoded(|payload| param.signer.sign(payload));