"refund" proof size in GRANDPa pallet (paritytech#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

@@ -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<T>,
@@ -189,6 +192,7 @@ pub mod pallet {
 			ensure!(best_finalized_number < *number, <Error<T, I>>::OldHeader);
 
 			let authority_set = <CurrentAuthoritySet<T, I>>::get();
+			let unused_proof_size = authority_set.unused_proof_size();
 			let set_id = authority_set.set_id;
 			verify_justification::<T, I>(&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 = <TestRuntime as Config>::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!(<BestFinalized<TestRuntime>>::get().unwrap().1, header.hash());

bridges/modules/grandpa/src/mock.rs

@@ -33,7 +33,7 @@ pub type TestNumber = crate::BridgedBlockNumber<TestRuntime, ()>;
 type Block = frame_system::mocking::MockBlock<TestRuntime>;
 type UncheckedExtrinsic = frame_system::mocking::MockUncheckedExtrinsic<TestRuntime>;
 
-pub const MAX_BRIDGED_AUTHORITIES: u32 = 2048;
+pub const MAX_BRIDGED_AUTHORITIES: u32 = 5;
 
 use crate as grandpa;
 

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<T: Config<I>, I: 'static> StoredAuthoritySet<T, I> {
 	pub fn try_new(authorities: AuthorityList, set_id: SetId) -> Result<Self, ()> {
 		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<T: Config<I>, I: 'static> PartialEq for StoredAuthoritySet<T, I> {
@@ -64,3 +82,41 @@ impl<T: Config<I>, I: 'static> From<StoredAuthoritySet<T, I>> 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<TestRuntime, ()>;
+
+	#[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
+	}
+}