From 4aec33182fb4e6ea54f3760ebdf176ec76288ad4 Mon Sep 17 00:00:00 2001 From: Svyatoslav Nikolsky Date: Tue, 28 Mar 2023 13:45:04 +0300 Subject: [PATCH] MaxRequests -> MaxFreeMandatoryHeadersPerBlock in pallet-bridge-grandpa (#1997) * MaxRequests -> MaxFreeMandatoryHeadersPerBlock in pallet-bridge-grandpa * fix comment * fix comment * fix comment --- bridges/bin/millau/runtime/src/lib.rs | 8 +- .../bin/rialto-parachain/runtime/src/lib.rs | 6 +- bridges/bin/rialto/runtime/src/lib.rs | 6 +- bridges/bin/runtime-common/src/integrity.rs | 6 +- bridges/bin/runtime-common/src/mock.rs | 2 +- bridges/modules/grandpa/src/lib.rs | 190 +++++++++++++----- bridges/modules/grandpa/src/mock.rs | 11 +- bridges/modules/parachains/src/mock.rs | 4 +- 8 files changed, 152 insertions(+), 81 deletions(-) diff --git a/bridges/bin/millau/runtime/src/lib.rs b/bridges/bin/millau/runtime/src/lib.rs index b0c6b2875fe94..deff859a81641 100644 --- a/bridges/bin/millau/runtime/src/lib.rs +++ b/bridges/bin/millau/runtime/src/lib.rs @@ -399,11 +399,7 @@ pub type RialtoGrandpaInstance = (); impl pallet_bridge_grandpa::Config for Runtime { type RuntimeEvent = RuntimeEvent; type BridgedChain = bp_rialto::Rialto; - // This is a pretty unscientific cap. - // - // Note that once this is hit the pallet will essentially throttle incoming requests down to one - // call per block. - type MaxRequests = ConstU32<50>; + type MaxFreeMandatoryHeadersPerBlock = ConstU32<4>; type HeadersToKeep = ConstU32<{ bp_rialto::DAYS }>; type WeightInfo = pallet_bridge_grandpa::weights::BridgeWeight; } @@ -412,7 +408,7 @@ pub type WestendGrandpaInstance = pallet_bridge_grandpa::Instance1; impl pallet_bridge_grandpa::Config for Runtime { type RuntimeEvent = RuntimeEvent; type BridgedChain = bp_westend::Westend; - type MaxRequests = ConstU32<50>; + type MaxFreeMandatoryHeadersPerBlock = ConstU32<4>; type HeadersToKeep = ConstU32<{ bp_westend::DAYS }>; type WeightInfo = pallet_bridge_grandpa::weights::BridgeWeight; } diff --git a/bridges/bin/rialto-parachain/runtime/src/lib.rs b/bridges/bin/rialto-parachain/runtime/src/lib.rs index b5419e2c25e88..cd4e256f4203e 100644 --- a/bridges/bin/rialto-parachain/runtime/src/lib.rs +++ b/bridges/bin/rialto-parachain/runtime/src/lib.rs @@ -540,11 +540,7 @@ pub type MillauGrandpaInstance = (); impl pallet_bridge_grandpa::Config for Runtime { type RuntimeEvent = RuntimeEvent; type BridgedChain = bp_millau::Millau; - /// This is a pretty unscientific cap. - /// - /// Note that once this is hit the pallet will essentially throttle incoming requests down to - /// one call per block. - type MaxRequests = ConstU32<50>; + type MaxFreeMandatoryHeadersPerBlock = ConstU32<4>; type HeadersToKeep = ConstU32<{ bp_millau::DAYS as u32 }>; type WeightInfo = pallet_bridge_grandpa::weights::BridgeWeight; } diff --git a/bridges/bin/rialto/runtime/src/lib.rs b/bridges/bin/rialto/runtime/src/lib.rs index 1a6f0f1e540c5..aeb0a3a7c5749 100644 --- a/bridges/bin/rialto/runtime/src/lib.rs +++ b/bridges/bin/rialto/runtime/src/lib.rs @@ -396,11 +396,7 @@ pub type MillauGrandpaInstance = (); impl pallet_bridge_grandpa::Config for Runtime { type RuntimeEvent = RuntimeEvent; type BridgedChain = bp_millau::Millau; - /// This is a pretty unscientific cap. - /// - /// Note that once this is hit the pallet will essentially throttle incoming requests down to - /// one call per block. - type MaxRequests = ConstU32<50>; + type MaxFreeMandatoryHeadersPerBlock = ConstU32<4>; type HeadersToKeep = ConstU32<{ bp_millau::DAYS as u32 }>; type WeightInfo = pallet_bridge_grandpa::weights::BridgeWeight; } diff --git a/bridges/bin/runtime-common/src/integrity.rs b/bridges/bin/runtime-common/src/integrity.rs index 96716fe851cb2..5820dd99b91ca 100644 --- a/bridges/bin/runtime-common/src/integrity.rs +++ b/bridges/bin/runtime-common/src/integrity.rs @@ -178,9 +178,9 @@ where GI: 'static, { assert!( - R::MaxRequests::get() > 0, - "MaxRequests ({}) must be larger than zero", - R::MaxRequests::get(), + R::HeadersToKeep::get() > 0, + "HeadersToKeep ({}) must be larger than zero", + R::HeadersToKeep::get(), ); } diff --git a/bridges/bin/runtime-common/src/mock.rs b/bridges/bin/runtime-common/src/mock.rs index ac6cf122b0f1d..c4c7c2fa8ac92 100644 --- a/bridges/bin/runtime-common/src/mock.rs +++ b/bridges/bin/runtime-common/src/mock.rs @@ -196,7 +196,7 @@ impl pallet_transaction_payment::Config for TestRuntime { impl pallet_bridge_grandpa::Config for TestRuntime { type RuntimeEvent = RuntimeEvent; type BridgedChain = BridgedUnderlyingChain; - type MaxRequests = ConstU32<50>; + type MaxFreeMandatoryHeadersPerBlock = ConstU32<4>; type HeadersToKeep = ConstU32<8>; type WeightInfo = pallet_bridge_grandpa::weights::BridgeWeight; } diff --git a/bridges/modules/grandpa/src/lib.rs b/bridges/modules/grandpa/src/lib.rs index 2d81f3106fc9d..df51aa1eb7ed6 100644 --- a/bridges/modules/grandpa/src/lib.rs +++ b/bridges/modules/grandpa/src/lib.rs @@ -103,14 +103,17 @@ pub mod pallet { /// The chain we are bridging to here. type BridgedChain: ChainWithGrandpa; - /// The upper bound on the number of requests allowed by the pallet. + /// Maximal number of "free" mandatory header transactions per block. /// - /// A request refers to an action which writes a header to storage. - /// - /// Once this bound is reached the pallet will not allow any dispatchables to be called - /// until the request count has decreased. + /// To be able to track the bridged chain, the pallet requires all headers that are + /// changing GRANDPA authorities set at the bridged chain (we call them mandatory). + /// So it is a common good deed to submit mandatory headers to the pallet. However, if the + /// bridged chain gets compromised, its validators may generate as many mandatory headers + /// as they want. And they may fill the whole block (at this chain) for free. This constants + /// limits number of calls that we may refund in a single block. All calls above this + /// limit are accepted, but are not refunded. #[pallet::constant] - type MaxRequests: Get; + type MaxFreeMandatoryHeadersPerBlock: Get; /// Maximal number of finalized headers to keep in the storage. /// @@ -131,10 +134,13 @@ pub mod pallet { #[pallet::hooks] impl, I: 'static> Hooks> for Pallet { - fn on_initialize(_n: T::BlockNumber) -> frame_support::weights::Weight { - >::mutate(|count| *count = count.saturating_sub(1)); + fn on_initialize(_n: BlockNumberFor) -> Weight { + FreeMandatoryHeadersRemaining::::put(T::MaxFreeMandatoryHeadersPerBlock::get()); + Weight::zero() + } - T::DbWeight::get().reads_writes(1, 1) + fn on_finalize(_n: BlockNumberFor) { + FreeMandatoryHeadersRemaining::::kill(); } } @@ -166,8 +172,6 @@ pub mod pallet { ) -> DispatchResultWithPostInfo { Self::ensure_not_halted().map_err(Error::::BridgeModule)?; - ensure!(Self::request_count() < T::MaxRequests::get(), >::TooManyRequests); - let (hash, number) = (finality_target.hash(), *finality_target.number()); log::trace!( target: LOG_TARGET, @@ -185,9 +189,16 @@ pub mod pallet { let is_authorities_change_enacted = try_enact_authority_change::(&finality_target, set_id)?; let may_refund_call_fee = is_authorities_change_enacted && + // if we have seen too many mandatory headers in this block, we don't want to refund + Self::free_mandatory_headers_remaining() > 0 && + // if arguments out of expected bounds, we don't want to refund submit_finality_proof_info_from_args::(&finality_target, &justification) .fits_limits(); - >::mutate(|count| *count += 1); + if may_refund_call_fee { + FreeMandatoryHeadersRemaining::::mutate(|count| { + *count = count.saturating_sub(1) + }); + } insert_header::(*finality_target, hash); log::info!( target: LOG_TARGET, @@ -273,16 +284,20 @@ pub mod pallet { } } - /// The current number of requests which have written to storage. + /// Number mandatory headers that we may accept in the current block for free (returning + /// `Pays::No`). /// - /// If the `RequestCount` hits `MaxRequests`, no more calls will be allowed to the pallet until - /// the request capacity is increased. + /// If the `FreeMandatoryHeadersRemaining` hits zero, all following mandatory headers in the + /// current block are accepted with fee (`Pays::Yes` is returned). /// - /// The `RequestCount` is decreased by one at the beginning of every block. This is to ensure - /// that the pallet can always make progress. + /// The `FreeMandatoryHeadersRemaining` is an ephemeral value that is set to + /// `MaxFreeMandatoryHeadersPerBlock` at each block initialization and is killed on block + /// finalization. So it never ends up in the storage trie. #[pallet::storage] - #[pallet::getter(fn request_count)] - pub(super) type RequestCount, I: 'static = ()> = StorageValue<_, u32, ValueQuery>; + #[pallet::whitelist_storage] + #[pallet::getter(fn free_mandatory_headers_remaining)] + pub(super) type FreeMandatoryHeadersRemaining, I: 'static = ()> = + StorageValue<_, u32, ValueQuery>; /// Hash of the header used to bootstrap the pallet. #[pallet::storage] @@ -392,8 +407,6 @@ pub mod pallet { InvalidJustification, /// The authority set from the underlying header chain is invalid. InvalidAuthoritySet, - /// There are too many requests for the current window to handle. - TooManyRequests, /// The header being imported is older than the best finalized header known to the pallet. OldHeader, /// The scheduled authority set change found in the header is unsupported by the pallet. @@ -662,7 +675,8 @@ mod tests { }; use codec::Encode; use frame_support::{ - assert_err, assert_noop, assert_ok, dispatch::PostDispatchInfo, + assert_err, assert_noop, assert_ok, + dispatch::{Pays, PostDispatchInfo}, storage::generator::StorageValue, }; use frame_system::{EventRecord, Phase}; @@ -705,6 +719,51 @@ mod tests { ) } + fn submit_finality_proof_with_set_id( + header: u8, + set_id: u64, + ) -> frame_support::dispatch::DispatchResultWithPostInfo { + let header = test_header(header.into()); + let justification = make_justification_for_header(JustificationGeneratorParams { + header: header.clone(), + set_id, + ..Default::default() + }); + + Pallet::::submit_finality_proof( + RuntimeOrigin::signed(1), + Box::new(header), + justification, + ) + } + + fn submit_mandatory_finality_proof( + number: u8, + set_id: u64, + ) -> frame_support::dispatch::DispatchResultWithPostInfo { + let mut header = test_header(number.into()); + // to ease tests that are using `submit_mandatory_finality_proof`, we'll be using the + // same set for all sessions + let consensus_log = + ConsensusLog::::ScheduledChange(sp_consensus_grandpa::ScheduledChange { + next_authorities: authority_list(), + delay: 0, + }); + header.digest = + Digest { logs: vec![DigestItem::Consensus(GRANDPA_ENGINE_ID, consensus_log.encode())] }; + let justification = make_justification_for_header(JustificationGeneratorParams { + header: header.clone(), + set_id, + ..Default::default() + }); + + Pallet::::submit_finality_proof( + RuntimeOrigin::signed(1), + Box::new(header), + justification, + ) + } + fn next_block() { use frame_support::traits::OnInitialize; @@ -1169,13 +1228,18 @@ mod tests { } #[test] - fn rate_limiter_disallows_imports_once_limit_is_hit_in_single_block() { + fn rate_limiter_disallows_free_imports_once_limit_is_hit_in_single_block() { run_test(|| { initialize_substrate_bridge(); - assert_ok!(submit_finality_proof(1)); - assert_ok!(submit_finality_proof(2)); - assert_err!(submit_finality_proof(3), >::TooManyRequests); + let result = submit_mandatory_finality_proof(1, 1); + assert_eq!(result.expect("call failed").pays_fee, Pays::No); + + let result = submit_mandatory_finality_proof(2, 2); + assert_eq!(result.expect("call failed").pays_fee, Pays::No); + + let result = submit_mandatory_finality_proof(3, 3); + assert_eq!(result.expect("call failed").pays_fee, Pays::Yes); }) } @@ -1183,7 +1247,8 @@ mod tests { fn rate_limiter_invalid_requests_do_not_count_towards_request_count() { run_test(|| { let submit_invalid_request = || { - let header = test_header(1); + let mut header = test_header(1); + header.digest = change_log(0); let mut invalid_justification = make_default_justification(&header); invalid_justification.round = 42; @@ -1196,15 +1261,19 @@ mod tests { initialize_substrate_bridge(); - for _ in 0..::MaxRequests::get() + 1 { - // Notice that the error here *isn't* `TooManyRequests` + for _ in 0..::MaxFreeMandatoryHeadersPerBlock::get() + 1 { assert_err!(submit_invalid_request(), >::InvalidJustification); } - // Can still submit `MaxRequests` requests afterwards - assert_ok!(submit_finality_proof(1)); - assert_ok!(submit_finality_proof(2)); - assert_err!(submit_finality_proof(3), >::TooManyRequests); + // Can still submit free mandatory headers afterwards + let result = submit_mandatory_finality_proof(1, 1); + assert_eq!(result.expect("call failed").pays_fee, Pays::No); + + let result = submit_mandatory_finality_proof(2, 2); + assert_eq!(result.expect("call failed").pays_fee, Pays::No); + + let result = submit_mandatory_finality_proof(3, 3); + assert_eq!(result.expect("call failed").pays_fee, Pays::Yes); }) } @@ -1212,40 +1281,51 @@ mod tests { fn rate_limiter_allows_request_after_new_block_has_started() { run_test(|| { initialize_substrate_bridge(); - assert_ok!(submit_finality_proof(1)); - assert_ok!(submit_finality_proof(2)); - next_block(); - assert_ok!(submit_finality_proof(3)); - }) - } + let result = submit_mandatory_finality_proof(1, 1); + assert_eq!(result.expect("call failed").pays_fee, Pays::No); - #[test] - fn rate_limiter_disallows_imports_once_limit_is_hit_across_different_blocks() { - run_test(|| { - initialize_substrate_bridge(); - assert_ok!(submit_finality_proof(1)); - assert_ok!(submit_finality_proof(2)); + let result = submit_mandatory_finality_proof(2, 2); + assert_eq!(result.expect("call failed").pays_fee, Pays::No); + + let result = submit_mandatory_finality_proof(3, 3); + assert_eq!(result.expect("call failed").pays_fee, Pays::Yes); next_block(); - assert_ok!(submit_finality_proof(3)); - assert_err!(submit_finality_proof(4), >::TooManyRequests); + + let result = submit_mandatory_finality_proof(4, 4); + assert_eq!(result.expect("call failed").pays_fee, Pays::No); + + let result = submit_mandatory_finality_proof(5, 5); + assert_eq!(result.expect("call failed").pays_fee, Pays::No); + + let result = submit_mandatory_finality_proof(6, 6); + assert_eq!(result.expect("call failed").pays_fee, Pays::Yes); }) } #[test] - fn rate_limiter_allows_max_requests_after_long_time_with_no_activity() { + fn rate_limiter_ignores_non_mandatory_headers() { run_test(|| { initialize_substrate_bridge(); - assert_ok!(submit_finality_proof(1)); - assert_ok!(submit_finality_proof(2)); - next_block(); - next_block(); + let result = submit_finality_proof(1); + assert_eq!(result.expect("call failed").pays_fee, Pays::Yes); - next_block(); - assert_ok!(submit_finality_proof(5)); - assert_ok!(submit_finality_proof(7)); + let result = submit_mandatory_finality_proof(2, 1); + assert_eq!(result.expect("call failed").pays_fee, Pays::No); + + let result = submit_finality_proof_with_set_id(3, 2); + assert_eq!(result.expect("call failed").pays_fee, Pays::Yes); + + let result = submit_mandatory_finality_proof(4, 2); + assert_eq!(result.expect("call failed").pays_fee, Pays::No); + + let result = submit_finality_proof_with_set_id(5, 3); + assert_eq!(result.expect("call failed").pays_fee, Pays::Yes); + + let result = submit_mandatory_finality_proof(6, 3); + assert_eq!(result.expect("call failed").pays_fee, Pays::Yes); }) } diff --git a/bridges/modules/grandpa/src/mock.rs b/bridges/modules/grandpa/src/mock.rs index 2220ba3f129f5..0ebbc0bccbb7b 100644 --- a/bridges/modules/grandpa/src/mock.rs +++ b/bridges/modules/grandpa/src/mock.rs @@ -21,7 +21,7 @@ use bp_header_chain::ChainWithGrandpa; use bp_runtime::Chain; use frame_support::{ construct_runtime, parameter_types, - traits::{ConstU32, ConstU64}, + traits::{ConstU32, ConstU64, Hooks}, weights::Weight, }; use sp_core::sr25519::Signature; @@ -87,7 +87,7 @@ impl frame_system::Config for TestRuntime { } parameter_types! { - pub const MaxRequests: u32 = 2; + pub const MaxFreeMandatoryHeadersPerBlock: u32 = 2; pub const HeadersToKeep: u32 = 5; pub const SessionLength: u64 = 5; pub const NumValidators: u32 = 5; @@ -96,7 +96,7 @@ parameter_types! { impl grandpa::Config for TestRuntime { type RuntimeEvent = RuntimeEvent; type BridgedChain = TestBridgedChain; - type MaxRequests = MaxRequests; + type MaxFreeMandatoryHeadersPerBlock = MaxFreeMandatoryHeadersPerBlock; type HeadersToKeep = HeadersToKeep; type WeightInfo = (); } @@ -138,7 +138,10 @@ pub fn new_test_ext() -> sp_io::TestExternalities { /// Return test within default test externalities context. pub fn run_test(test: impl FnOnce() -> T) -> T { - new_test_ext().execute_with(test) + new_test_ext().execute_with(|| { + let _ = Grandpa::on_initialize(0); + test() + }) } /// Return test header with given number. diff --git a/bridges/modules/parachains/src/mock.rs b/bridges/modules/parachains/src/mock.rs index 90e6d7c558e49..3086adc1cc2ea 100644 --- a/bridges/modules/parachains/src/mock.rs +++ b/bridges/modules/parachains/src/mock.rs @@ -199,7 +199,7 @@ parameter_types! { impl pallet_bridge_grandpa::Config for TestRuntime { type RuntimeEvent = RuntimeEvent; type BridgedChain = TestBridgedChain; - type MaxRequests = ConstU32<2>; + type MaxFreeMandatoryHeadersPerBlock = ConstU32<2>; type HeadersToKeep = HeadersToKeep; type WeightInfo = (); } @@ -207,7 +207,7 @@ impl pallet_bridge_grandpa::Config for TestRun impl pallet_bridge_grandpa::Config for TestRuntime { type RuntimeEvent = RuntimeEvent; type BridgedChain = TestBridgedChain; - type MaxRequests = ConstU32<2>; + type MaxFreeMandatoryHeadersPerBlock = ConstU32<2>; type HeadersToKeep = HeadersToKeep; type WeightInfo = (); }