From c25ae034828ee97a69919850e6db55cc3e792777 Mon Sep 17 00:00:00 2001 From: Svyatoslav Nikolsky Date: Thu, 9 Feb 2023 14:09:37 +0300 Subject: [PATCH] "refund" proof size in GRANDPa pallet (#1863) * "refund" proof size in GRANDPa pallet * clippy * extra_proof_size_bytes_works * use saturated_into * fix review comments --- bridges/modules/grandpa/src/lib.rs | 45 +++++++++++++-- bridges/modules/grandpa/src/mock.rs | 2 +- bridges/modules/grandpa/src/storage_types.rs | 58 +++++++++++++++++++- 3 files changed, 97 insertions(+), 8 deletions(-) diff --git a/bridges/modules/grandpa/src/lib.rs b/bridges/modules/grandpa/src/lib.rs index 6128030d0906..5063ea314383 100644 --- a/bridges/modules/grandpa/src/lib.rs +++ b/bridges/modules/grandpa/src/lib.rs @@ -44,9 +44,12 @@ use bp_header_chain::{ }; use bp_runtime::{BlockNumberOf, Chain, HashOf, HasherOf, HeaderId, HeaderOf, OwnedBridgeModule}; use finality_grandpa::voter_set::VoterSet; -use frame_support::{ensure, fail}; +use frame_support::{dispatch::PostDispatchInfo, ensure, fail}; use sp_finality_grandpa::{ConsensusLog, GRANDPA_ENGINE_ID}; -use sp_runtime::traits::{Header as HeaderT, Zero}; +use sp_runtime::{ + traits::{Header as HeaderT, Zero}, + SaturatedConversion, +}; use sp_std::{boxed::Box, convert::TryInto}; mod extension; @@ -152,8 +155,8 @@ pub mod pallet { /// pallet. #[pallet::call_index(0)] #[pallet::weight(T::WeightInfo::submit_finality_proof( - justification.commit.precommits.len().try_into().unwrap_or(u32::MAX), - justification.votes_ancestries.len().try_into().unwrap_or(u32::MAX), + justification.commit.precommits.len().saturated_into(), + justification.votes_ancestries.len().saturated_into(), ))] pub fn submit_finality_proof( _origin: OriginFor, @@ -189,6 +192,7 @@ pub mod pallet { ensure!(best_finalized_number < *number, >::OldHeader); let authority_set = >::get(); + let unused_proof_size = authority_set.unused_proof_size(); let set_id = authority_set.set_id; verify_justification::(&justification, hash, *number, authority_set.into())?; @@ -210,7 +214,18 @@ pub mod pallet { let is_mandatory_header = is_authorities_change_enacted; let pays_fee = if is_mandatory_header { Pays::No } else { Pays::Yes }; - Ok(pays_fee.into()) + // the proof size component of the call weight assumes that there are + // `MaxBridgedAuthorities` in the `CurrentAuthoritySet` (we use `MaxEncodedLen` + // estimation). But if their number is lower, then we may "refund" some `proof_size`, + // making proof smaller and leaving block space to other useful transactions + let pre_dispatch_weight = T::WeightInfo::submit_finality_proof( + justification.commit.precommits.len().saturated_into(), + justification.votes_ancestries.len().saturated_into(), + ); + let actual_weight = pre_dispatch_weight + .set_proof_size(pre_dispatch_weight.proof_size().saturating_sub(unused_proof_size)); + + Ok(PostDispatchInfo { actual_weight: Some(actual_weight), pays_fee }) } /// Bootstrap the bridge pallet with an initial header and authority set from which to sync. @@ -819,9 +834,27 @@ mod tests { fn succesfully_imports_header_with_valid_finality() { run_test(|| { initialize_substrate_bridge(); - let result = submit_finality_proof(1); + + let header_number = 1; + let header = test_header(header_number.into()); + let justification = make_default_justification(&header); + + let pre_dispatch_weight = ::WeightInfo::submit_finality_proof( + justification.commit.precommits.len().try_into().unwrap_or(u32::MAX), + justification.votes_ancestries.len().try_into().unwrap_or(u32::MAX), + ); + + let result = submit_finality_proof(header_number); assert_ok!(result); assert_eq!(result.unwrap().pays_fee, frame_support::dispatch::Pays::Yes); + // our test config assumes 2048 max authorities and we are just using couple + let pre_dispatch_proof_size = pre_dispatch_weight.proof_size(); + let actual_proof_size = result.unwrap().actual_weight.unwrap().proof_size(); + assert!(actual_proof_size > 0); + assert!( + actual_proof_size < pre_dispatch_proof_size, + "Actual proof size {actual_proof_size} must be less than the pre-dispatch {pre_dispatch_proof_size}", + ); let header = test_header(1); assert_eq!(>::get().unwrap().1, header.hash()); diff --git a/bridges/modules/grandpa/src/mock.rs b/bridges/modules/grandpa/src/mock.rs index ba6f176e8234..8757093ee682 100644 --- a/bridges/modules/grandpa/src/mock.rs +++ b/bridges/modules/grandpa/src/mock.rs @@ -33,7 +33,7 @@ pub type TestNumber = crate::BridgedBlockNumber; type Block = frame_system::mocking::MockBlock; type UncheckedExtrinsic = frame_system::mocking::MockUncheckedExtrinsic; -pub const MAX_BRIDGED_AUTHORITIES: u32 = 2048; +pub const MAX_BRIDGED_AUTHORITIES: u32 = 5; use crate as grandpa; diff --git a/bridges/modules/grandpa/src/storage_types.rs b/bridges/modules/grandpa/src/storage_types.rs index d930dbadbc63..732e72e2c0d6 100644 --- a/bridges/modules/grandpa/src/storage_types.rs +++ b/bridges/modules/grandpa/src/storage_types.rs @@ -20,7 +20,7 @@ use crate::Config; use bp_header_chain::AuthoritySet; use codec::{Decode, Encode, MaxEncodedLen}; -use frame_support::{BoundedVec, RuntimeDebugNoBound}; +use frame_support::{traits::Get, BoundedVec, RuntimeDebugNoBound}; use scale_info::TypeInfo; use sp_finality_grandpa::{AuthorityId, AuthorityList, AuthorityWeight, SetId}; @@ -45,6 +45,24 @@ impl, I: 'static> StoredAuthoritySet { pub fn try_new(authorities: AuthorityList, set_id: SetId) -> Result { Ok(Self { authorities: TryFrom::try_from(authorities).map_err(drop)?, set_id }) } + + /// Returns number of bytes that may be subtracted from the PoV component of + /// `submit_finality_proof` call, because the actual authorities set is smaller than the maximal + /// configured. + /// + /// Maximal authorities set size is configured by the `MaxBridgedAuthorities` constant from + /// the pallet configuration. The PoV of the call includes the size of maximal authorities + /// count. If the actual size is smaller, we may subtract extra bytes from this component. + pub fn unused_proof_size(&self) -> u64 { + // we can only safely estimate bytes that are occupied by the authority data itself. We have + // no means here to compute PoV bytes, occupied by extra trie nodes or extra bytes in the + // whole set encoding + let single_authority_max_encoded_len = + <(AuthorityId, AuthorityWeight)>::max_encoded_len() as u64; + let extra_authorities = + T::MaxBridgedAuthorities::get().saturating_sub(self.authorities.len() as _); + single_authority_max_encoded_len.saturating_mul(extra_authorities as u64) + } } impl, I: 'static> PartialEq for StoredAuthoritySet { @@ -64,3 +82,41 @@ impl, I: 'static> From> for AuthoritySet { AuthoritySet { authorities: t.authorities.into(), set_id: t.set_id } } } + +#[cfg(test)] +mod tests { + use crate::mock::{TestRuntime, MAX_BRIDGED_AUTHORITIES}; + use bp_test_utils::authority_list; + + type StoredAuthoritySet = super::StoredAuthoritySet; + + #[test] + fn unused_proof_size_works() { + let authority_entry = authority_list().pop().unwrap(); + + // when we have exactly `MaxBridgedAuthorities` authorities + assert_eq!( + StoredAuthoritySet::try_new( + vec![authority_entry.clone(); MAX_BRIDGED_AUTHORITIES as usize], + 0, + ) + .unwrap() + .unused_proof_size(), + 0, + ); + + // when we have less than `MaxBridgedAuthorities` authorities + assert_eq!( + StoredAuthoritySet::try_new( + vec![authority_entry; MAX_BRIDGED_AUTHORITIES as usize - 1], + 0, + ) + .unwrap() + .unused_proof_size(), + 40, + ); + + // and we can't have more than `MaxBridgedAuthorities` authorities in the bounded vec, so + // no test for this case + } +}