diff --git a/cumulus/pallets/parachain-system/src/lib.rs b/cumulus/pallets/parachain-system/src/lib.rs index a8e9a0bf9ae4..d774089ef91f 100644 --- a/cumulus/pallets/parachain-system/src/lib.rs +++ b/cumulus/pallets/parachain-system/src/lib.rs @@ -494,8 +494,12 @@ pub mod pallet { "ValidationData must be updated only once in a block", ); + let data_len = data.encoded_size() as u64; + // TODO: This is more than zero, but will need benchmarking to figure out what. - let mut total_weight = Weight::zero(); + // MOONBEAM TODO: custom weight to account for validation data size is the PoV + // until https://github.com/paritytech/substrate/issues/13810 it's properly fix. + let mut total_weight = Weight::from_parts(0, data_len); // NOTE: the inherent data is expected to be unique, even if this block is built // in the context of the same relay parent as the previous one. In particular, diff --git a/cumulus/test/client/src/block_builder.rs b/cumulus/test/client/src/block_builder.rs index 2d930d1be597..a04d237cd2a8 100644 --- a/cumulus/test/client/src/block_builder.rs +++ b/cumulus/test/client/src/block_builder.rs @@ -108,7 +108,7 @@ fn init_block_builder( inherents .into_iter() - .for_each(|ext| block_builder.push(ext).expect("Pushes inherent")); + .for_each(|ext| block_builder.push(ext, None).expect("Pushes inherent")); block_builder } diff --git a/cumulus/test/service/benches/block_import.rs b/cumulus/test/service/benches/block_import.rs index b79598b15302..78d10c70bb9a 100644 --- a/cumulus/test/service/benches/block_import.rs +++ b/cumulus/test/service/benches/block_import.rs @@ -55,7 +55,7 @@ fn benchmark_block_import(c: &mut Criterion) { let mut block_builder = client.new_block_at(parent_hash, Default::default(), RecordProof::No).unwrap(); for extrinsic in extrinsics { - block_builder.push(extrinsic).unwrap(); + block_builder.push(extrinsic, None).unwrap(); } let benchmark_block = block_builder.build().unwrap(); diff --git a/cumulus/test/service/benches/block_import_glutton.rs b/cumulus/test/service/benches/block_import_glutton.rs index b49db9f449e9..60624b1d5727 100644 --- a/cumulus/test/service/benches/block_import_glutton.rs +++ b/cumulus/test/service/benches/block_import_glutton.rs @@ -68,7 +68,7 @@ fn benchmark_block_import(c: &mut Criterion) { block_builder .push(utils::extrinsic_set_validation_data(parent_header.clone()).clone()) .unwrap(); - block_builder.push(utils::extrinsic_set_time(&client)).unwrap(); + block_builder.push(utils::extrinsic_set_time(&client), None).unwrap(); let benchmark_block = block_builder.build().unwrap(); group.bench_function( diff --git a/cumulus/test/service/benches/block_production.rs b/cumulus/test/service/benches/block_production.rs index 1b868d736302..e1acbb68be94 100644 --- a/cumulus/test/service/benches/block_production.rs +++ b/cumulus/test/service/benches/block_production.rs @@ -51,8 +51,8 @@ fn benchmark_block_production(c: &mut Criterion) { let set_validation_data_extrinsic = utils::extrinsic_set_validation_data(parent_header); let mut block_builder = client.new_block(Default::default()).unwrap(); - block_builder.push(utils::extrinsic_set_time(&client)).unwrap(); - block_builder.push(set_validation_data_extrinsic).unwrap(); + block_builder.push(utils::extrinsic_set_time(&client), None).unwrap(); + block_builder.push(set_validation_data_extrinsic, None).unwrap(); let built_block = block_builder.build().unwrap(); runtime.block_on(utils::import_block(&client, &built_block.block, false)); @@ -78,7 +78,7 @@ fn benchmark_block_production(c: &mut Criterion) { .new_block_at(best_hash, Default::default(), RecordProof::Yes) .unwrap(); for extrinsic in extrinsics { - block_builder.push(extrinsic).unwrap(); + block_builder.push(extrinsic, None).unwrap(); } block_builder.build().unwrap() }, @@ -97,7 +97,7 @@ fn benchmark_block_production(c: &mut Criterion) { .new_block_at(best_hash, Default::default(), RecordProof::No) .unwrap(); for extrinsic in extrinsics { - block_builder.push(extrinsic).unwrap(); + block_builder.push(extrinsic, None).unwrap(); } block_builder.build().unwrap() }, diff --git a/cumulus/test/service/benches/block_production_glutton.rs b/cumulus/test/service/benches/block_production_glutton.rs index 92a368c88c8d..4b40d1af9f3e 100644 --- a/cumulus/test/service/benches/block_production_glutton.rs +++ b/cumulus/test/service/benches/block_production_glutton.rs @@ -79,8 +79,8 @@ fn benchmark_block_production_compute(c: &mut Criterion) { let mut block_builder = client .new_block_at(best_hash, Default::default(), RecordProof::Yes) .unwrap(); - block_builder.push(validation_data).unwrap(); - block_builder.push(time).unwrap(); + block_builder.push(validation_data, None).unwrap(); + block_builder.push(time, None).unwrap(); block_builder.build().unwrap() }, BatchSize::SmallInput, @@ -101,8 +101,8 @@ fn benchmark_block_production_compute(c: &mut Criterion) { let mut block_builder = client .new_block_at(best_hash, Default::default(), RecordProof::No) .unwrap(); - block_builder.push(validation_data).unwrap(); - block_builder.push(time).unwrap(); + block_builder.push(validation_data, None).unwrap(); + block_builder.push(time, None).unwrap(); block_builder.build().unwrap() }, BatchSize::SmallInput, diff --git a/cumulus/test/service/benches/validate_block.rs b/cumulus/test/service/benches/validate_block.rs index f3b4d0b12144..89d36e3a0352 100644 --- a/cumulus/test/service/benches/validate_block.rs +++ b/cumulus/test/service/benches/validate_block.rs @@ -58,7 +58,7 @@ fn create_extrinsics( None, ); - match block_builder.push(extrinsic.clone()) { + match block_builder.push(extrinsic.clone(), None) { Ok(_) => {}, Err(ApplyExtrinsicFailed(Validity(TransactionValidityError::Invalid( InvalidTransaction::ExhaustsResources, @@ -96,7 +96,7 @@ fn benchmark_block_validation(c: &mut Criterion) { let mut block_builder = client.init_block_builder(Some(validation_data), Default::default()); for extrinsic in extrinsics { - block_builder.push(extrinsic).unwrap(); + block_builder.push(extrinsic, None).unwrap(); } let parachain_block = block_builder.build_parachain_block(*parent_header.state_root()); diff --git a/cumulus/test/service/benches/validate_block_glutton.rs b/cumulus/test/service/benches/validate_block_glutton.rs index 0e049d8665dc..8b2467e2491c 100644 --- a/cumulus/test/service/benches/validate_block_glutton.rs +++ b/cumulus/test/service/benches/validate_block_glutton.rs @@ -200,7 +200,7 @@ fn set_glutton_parameters( let mut block_builder = client.init_block_builder(Some(validation_data), Default::default()); for extrinsic in extrinsics { - block_builder.push(extrinsic).unwrap(); + block_builder.push(extrinsic, None).unwrap(); } block_builder.build_parachain_block(*parent_header.state_root()) diff --git a/cumulus/test/service/src/bench_utils.rs b/cumulus/test/service/src/bench_utils.rs index 172c9e504196..0a8bb7a8d62a 100644 --- a/cumulus/test/service/src/bench_utils.rs +++ b/cumulus/test/service/src/bench_utils.rs @@ -148,7 +148,7 @@ pub fn create_benchmarking_transfer_extrinsics( Some(0), ); - match block_builder.push(extrinsic.clone().into()) { + match block_builder.push(extrinsic.clone().into(), None) { Ok(_) => {}, Err(ApplyExtrinsicFailed(Validity(TransactionValidityError::Invalid( InvalidTransaction::ExhaustsResources, @@ -250,10 +250,10 @@ pub fn set_glutton_parameters( extrinsics.push(set_storage); let mut block_builder = client.new_block(Default::default()).unwrap(); - block_builder.push(extrinsic_set_time(client)).unwrap(); - block_builder.push(extrinsic_set_validation_data(parent_header)).unwrap(); + block_builder.push(extrinsic_set_time(client), None).unwrap(); + block_builder.push(extrinsic_set_validation_data(parent_header), None).unwrap(); for extrinsic in extrinsics { - block_builder.push(extrinsic.into()).unwrap(); + block_builder.push(extrinsic.into(), None).unwrap(); } let built_block = block_builder.build().unwrap(); diff --git a/polkadot/node/test/client/src/block_builder.rs b/polkadot/node/test/client/src/block_builder.rs index b4ff050ff152..346c3ab68882 100644 --- a/polkadot/node/test/client/src/block_builder.rs +++ b/polkadot/node/test/client/src/block_builder.rs @@ -123,7 +123,7 @@ impl InitPolkadotBlockBuilder for Client { inherents .into_iter() - .for_each(|ext| block_builder.push(ext).expect("Pushes inherent")); + .for_each(|ext| block_builder.push(ext, None).expect("Pushes inherent")); block_builder } @@ -154,6 +154,7 @@ impl BlockBuilderExt for BlockBuilder<'_, Block, Client, FullBackend> { Decode::decode(&mut &encoded[..]).expect( "The runtime specific extrinsic always decodes to an opaque extrinsic; qed", ), + None, ) } } diff --git a/polkadot/xcm/src/v3/mod.rs b/polkadot/xcm/src/v3/mod.rs index a428f1277a5f..4fe42d406883 100644 --- a/polkadot/xcm/src/v3/mod.rs +++ b/polkadot/xcm/src/v3/mod.rs @@ -354,7 +354,8 @@ impl TryFrom for WeightLimit { fn try_from(x: OldWeightLimit) -> result::Result { use OldWeightLimit::*; match x { - Limited(w) => Ok(Self::Limited(Weight::from_parts(w, DEFAULT_PROOF_SIZE))), + // To support xcm v2 messages with limited weight, we need to buy a lot of PoV + Limited(w) => Ok(Self::Limited(Weight::from_parts(w, 5 * DEFAULT_PROOF_SIZE))), Unlimited => Ok(Self::Unlimited), } } diff --git a/polkadot/xcm/xcm-builder/src/lib.rs b/polkadot/xcm/xcm-builder/src/lib.rs index e3f910409638..ca8212a11f2f 100644 --- a/polkadot/xcm/xcm-builder/src/lib.rs +++ b/polkadot/xcm/xcm-builder/src/lib.rs @@ -34,8 +34,8 @@ pub use location_conversion::{ ChildParachainConvertsVia, DescribeAccountId32Terminal, DescribeAccountIdTerminal, DescribeAccountKey20Terminal, DescribeAllTerminal, DescribeFamily, DescribeLocation, DescribePalletTerminal, DescribeTerminus, GlobalConsensusConvertsFor, - GlobalConsensusParachainConvertsFor, HashedDescription, ParentIsPreset, - SiblingParachainConvertsVia, + GlobalConsensusParachainConvertsFor, HashedDescription, + HashedDescriptionDescribeFamilyAllTerminal, ParentIsPreset, SiblingParachainConvertsVia, }; mod origin_conversion; diff --git a/polkadot/xcm/xcm-builder/src/location_conversion.rs b/polkadot/xcm/xcm-builder/src/location_conversion.rs index 26b48fc88adc..2acbc4cce4c7 100644 --- a/polkadot/xcm/xcm-builder/src/location_conversion.rs +++ b/polkadot/xcm/xcm-builder/src/location_conversion.rs @@ -166,6 +166,48 @@ impl DescribeLocation for LegacyDescribeForeignChainAccount { } } +/// temporary struct that mimin the behavior of the upstream type: +/// HashedDescription> +pub struct HashedDescriptionDescribeFamilyAllTerminal(PhantomData); +impl + Clone> HashedDescriptionDescribeFamilyAllTerminal { + fn describe_location_suffix(l: &MultiLocation) -> Option> { + match (l.parents, &l.interior) { + (0, Here) => Some(Vec::new()), + (0, X1(PalletInstance(i))) => + Some((b"Pallet", Compact::::from(*i as u32)).encode()), + (0, X1(AccountId32 { id, .. })) => Some((b"AccountId32", id).encode()), + (0, X1(AccountKey20 { key, .. })) => Some((b"AccountKey20", key).encode()), + _ => return None, + } + } +} + +impl + Clone> ConvertLocation + for HashedDescriptionDescribeFamilyAllTerminal +{ + fn convert_location(location: &MultiLocation) -> Option { + let to_hash = match (location.parents, location.interior.first()) { + (0, Some(Parachain(index))) => { + let tail = location.interior.split_first().0; + let interior = Self::describe_location_suffix(&tail.into())?; + (b"ChildChain", Compact::::from(*index), interior).encode() + }, + (1, Some(Parachain(index))) => { + let tail = location.interior.split_first().0; + let interior = Self::describe_location_suffix(&tail.into())?; + (b"SiblingChain", Compact::::from(*index), interior).encode() + }, + (1, _) => { + let tail = location.interior.into(); + let interior = Self::describe_location_suffix(&tail)?; + (b"ParentChain", interior).encode() + }, + _ => return None, + }; + Some(blake2_256(&to_hash).into()) + } +} + /// Prefix for generating alias account for accounts coming /// from chains that use 32 byte long representations. pub const FOREIGN_CHAIN_PREFIX_PARA_32: [u8; 37] = *b"ForeignChainAliasAccountPrefix_Para32"; @@ -925,4 +967,73 @@ mod tests { }; assert!(ForeignChainAliasAccount::<[u8; 32]>::convert_location(&mul).is_none()); } + + #[test] + fn test_hashed_family_all_terminal_converter() { + type Converter = HashedDescriptionDescribeFamilyAllTerminal; + + assert_eq!( + [ + 129, 211, 14, 6, 146, 54, 225, 200, 135, 103, 248, 244, 125, 112, 53, 133, 91, 42, + 215, 236, 154, 199, 191, 208, 110, 148, 223, 55, 92, 216, 250, 34 + ], + Converter::<[u8; 32]>::convert_location(&MultiLocation { + parents: 0, + interior: X2(Parachain(1), AccountId32 { network: None, id: [0u8; 32] }), + }) + .unwrap() + ); + assert_eq!( + [ + 17, 142, 105, 253, 199, 34, 43, 136, 155, 48, 12, 137, 155, 219, 155, 110, 93, 181, + 93, 252, 124, 60, 250, 195, 229, 86, 31, 220, 121, 111, 254, 252 + ], + Converter::<[u8; 32]>::convert_location(&MultiLocation { + parents: 1, + interior: X2(Parachain(1), AccountId32 { network: None, id: [0u8; 32] }), + }) + .unwrap() + ); + assert_eq!( + [ + 237, 65, 190, 49, 53, 182, 196, 183, 151, 24, 214, 23, 72, 244, 235, 87, 187, 67, + 52, 122, 195, 192, 10, 58, 253, 49, 0, 112, 175, 224, 125, 66 + ], + Converter::<[u8; 32]>::convert_location(&MultiLocation { + parents: 0, + interior: X2(Parachain(1), AccountKey20 { network: None, key: [0u8; 20] }), + }) + .unwrap() + ); + assert_eq!( + [ + 226, 225, 225, 162, 254, 156, 113, 95, 68, 155, 160, 118, 126, 18, 166, 132, 144, + 19, 8, 204, 228, 112, 164, 189, 179, 124, 249, 1, 168, 110, 151, 50 + ], + Converter::<[u8; 32]>::convert_location(&MultiLocation { + parents: 1, + interior: X2(Parachain(1), AccountKey20 { network: None, key: [0u8; 20] }), + }) + .unwrap() + ); + assert_eq!( + [ + 254, 186, 179, 229, 13, 24, 84, 36, 84, 35, 64, 95, 114, 136, 62, 69, 247, 74, 215, + 104, 121, 114, 53, 6, 124, 46, 42, 245, 121, 197, 12, 208 + ], + Converter::<[u8; 32]>::convert_location(&MultiLocation { + parents: 1, + interior: X2(Parachain(2), PalletInstance(3)), + }) + .unwrap() + ); + assert_eq!( + [ + 217, 56, 0, 36, 228, 154, 250, 26, 200, 156, 1, 39, 254, 162, 16, 187, 107, 67, 27, + 16, 218, 254, 250, 184, 6, 27, 216, 138, 194, 93, 23, 165 + ], + Converter::<[u8; 32]>::convert_location(&MultiLocation { parents: 1, interior: Here }) + .unwrap() + ); + } } diff --git a/substrate/bin/node/cli/benches/block_production.rs b/substrate/bin/node/cli/benches/block_production.rs index b877aa735022..24d9f928e430 100644 --- a/substrate/bin/node/cli/benches/block_production.rs +++ b/substrate/bin/node/cli/benches/block_production.rs @@ -130,7 +130,7 @@ fn prepare_benchmark(client: &FullClient) -> (usize, Vec) { // Every block needs one timestamp extrinsic. let extrinsic_set_time = extrinsic_set_time(1 + MINIMUM_PERIOD_FOR_BLOCKS); - block_builder.push(extrinsic_set_time.clone()).unwrap(); + block_builder.push(extrinsic_set_time.clone(), None).unwrap(); extrinsics.push(extrinsic_set_time); // Creating those is surprisingly costly, so let's only do it once and later just `clone` them. @@ -147,7 +147,7 @@ fn prepare_benchmark(client: &FullClient) -> (usize, Vec) { ) .into(); - match block_builder.push(extrinsic.clone()) { + match block_builder.push(extrinsic.clone(), None) { Ok(_) => {}, Err(ApplyExtrinsicFailed(Validity(TransactionValidityError::Invalid( InvalidTransaction::ExhaustsResources, @@ -174,7 +174,7 @@ fn block_production(c: &mut Criterion) { // Buliding the very first block is around ~30x slower than any subsequent one, // so let's make sure it's built and imported before we benchmark anything. let mut block_builder = client.new_block(Default::default()).unwrap(); - block_builder.push(extrinsic_set_time(1)).unwrap(); + block_builder.push(extrinsic_set_time(1), None).unwrap(); import_block(client, block_builder.build().unwrap()); let (max_transfer_count, extrinsics) = prepare_benchmark(&client); @@ -194,7 +194,7 @@ fn block_production(c: &mut Criterion) { let mut block_builder = client.new_block_at(best_hash, Default::default(), RecordProof::No).unwrap(); for extrinsic in extrinsics { - block_builder.push(extrinsic).unwrap(); + block_builder.push(extrinsic, None).unwrap(); } block_builder.build().unwrap() }, @@ -209,7 +209,7 @@ fn block_production(c: &mut Criterion) { let mut block_builder = client.new_block_at(best_hash, Default::default(), RecordProof::Yes).unwrap(); for extrinsic in extrinsics { - block_builder.push(extrinsic).unwrap(); + block_builder.push(extrinsic, None).unwrap(); } block_builder.build().unwrap() }, diff --git a/substrate/bin/node/testing/src/bench.rs b/substrate/bin/node/testing/src/bench.rs index f1ab2212239b..ebf506658569 100644 --- a/substrate/bin/node/testing/src/bench.rs +++ b/substrate/bin/node/testing/src/bench.rs @@ -459,12 +459,12 @@ impl BenchDb { let mut block = client.new_block(Default::default()).expect("Block creation failed"); for extrinsic in self.generate_inherents(&client) { - block.push(extrinsic).expect("Push inherent failed"); + block.push(extrinsic, None).expect("Push inherent failed"); } let start = std::time::Instant::now(); for opaque in self.block_content(content, &client) { - match block.push(opaque) { + match block.push(opaque, None) { Err(sp_blockchain::Error::ApplyExtrinsicFailed( sp_blockchain::ApplyExtrinsicFailed::Validity(e), )) if e.exhausted_resources() => break, diff --git a/substrate/client/basic-authorship/src/basic_authorship.rs b/substrate/client/basic-authorship/src/basic_authorship.rs index b3a8f0d8970b..22b73047a9ae 100644 --- a/substrate/client/basic-authorship/src/basic_authorship.rs +++ b/substrate/client/basic-authorship/src/basic_authorship.rs @@ -33,7 +33,11 @@ use sc_client_api::backend; use sc_telemetry::{telemetry, TelemetryHandle, CONSENSUS_INFO}; use sc_transaction_pool_api::{InPoolTransaction, TransactionPool}; use sp_api::{ApiExt, ProvideRuntimeApi}; -use sp_blockchain::{ApplyExtrinsicFailed::Validity, Error::ApplyExtrinsicFailed, HeaderBackend}; +use sp_blockchain::{ + ApplyExtrinsicFailed::{TooBigStorageProof, Validity}, + Error::ApplyExtrinsicFailed, + HeaderBackend, +}; use sp_consensus::{DisableProofRecording, EnableProofRecording, ProofRecording, Proposal}; use sp_core::traits::SpawnNamed; use sp_inherents::InherentData; @@ -84,6 +88,11 @@ pub struct ProposerFactory { telemetry: Option, /// When estimating the block size, should the proof be included? include_proof_in_block_size_estimation: bool, + /// Ensure that an extrinsic execution cannot increase the size of the proof above the limit. + /// This check is **not** done by default because it represents additional costs for the node + /// who produces the block (requires to be able to rollback the recording of the proof if + /// needed). + ensure_proof_size_limit_after_each_extrinsic: bool, /// phantom member to pin the `Backend`/`ProofRecording` type. _phantom: PhantomData<(B, PR)>, } @@ -109,6 +118,7 @@ impl ProposerFactory { telemetry, client, include_proof_in_block_size_estimation: false, + ensure_proof_size_limit_after_each_extrinsic: false, _phantom: PhantomData, } } @@ -137,6 +147,7 @@ impl ProposerFactory { soft_deadline_percent: DEFAULT_SOFT_DEADLINE_PERCENT, telemetry, include_proof_in_block_size_estimation: true, + ensure_proof_size_limit_after_each_extrinsic: false, _phantom: PhantomData, } } @@ -145,6 +156,11 @@ impl ProposerFactory { pub fn disable_proof_in_block_size_estimation(&mut self) { self.include_proof_in_block_size_estimation = false; } + + /// Enable an additional check after each extrinsic that ensure the proof size limit. + pub fn enable_ensure_proof_size_limit_after_each_extrinsic(&mut self) { + self.ensure_proof_size_limit_after_each_extrinsic = true; + } } impl ProposerFactory { @@ -211,6 +227,8 @@ where telemetry: self.telemetry.clone(), _phantom: PhantomData, include_proof_in_block_size_estimation: self.include_proof_in_block_size_estimation, + ensure_proof_size_limit_after_each_extrinsic: self + .ensure_proof_size_limit_after_each_extrinsic, }; proposer @@ -251,6 +269,7 @@ pub struct Proposer { metrics: PrometheusMetrics, default_block_size_limit: usize, include_proof_in_block_size_estimation: bool, + ensure_proof_size_limit_after_each_extrinsic: bool, soft_deadline_percent: Percent, telemetry: Option, _phantom: PhantomData<(B, PR)>, @@ -375,7 +394,7 @@ where }); for inherent in inherents { - match block_builder.push(inherent) { + match block_builder.push(inherent, None) { Err(ApplyExtrinsicFailed(Validity(e))) if e.exhausted_resources() => { warn!( target: LOG_TARGET, @@ -416,6 +435,7 @@ where now + time::Duration::from_micros(self.soft_deadline_percent.mul_floor(left_micros)); let mut skipped = 0; let mut unqueue_invalid = Vec::new(); + let mut suspicious_txs = Vec::new(); let mut t1 = self.transaction_pool.ready_at(self.parent_number).fuse(); let mut t2 = @@ -457,11 +477,66 @@ where } let pending_tx_data = pending_tx.data().clone(); + let pending_tx_encoded_size = pending_tx_data.encoded_size(); let pending_tx_hash = pending_tx.hash().clone(); let block_size = block_builder.estimate_block_size(self.include_proof_in_block_size_estimation); - if block_size + pending_tx_data.encoded_size() > block_size_limit { + if let Some(remaining_size) = + block_size_limit.checked_sub(block_size + pending_tx_encoded_size) + { + // There is enough space left in the block, we push the transaction + trace!("[{:?}] Pushing to the block.", pending_tx_hash); + match sc_block_builder::BlockBuilder::push( + block_builder, + pending_tx_data, + self.ensure_proof_size_limit_after_each_extrinsic.then(|| remaining_size), + ) { + Ok(()) => { + transaction_pushed = true; + debug!("[{:?}] Pushed to the block.", pending_tx_hash); + }, + Err(ApplyExtrinsicFailed(TooBigStorageProof(proof_diff, _))) => { + pending_iterator.report_invalid(&pending_tx); + if pending_tx_encoded_size + proof_diff > block_size_limit { + // The transaction and its storage proof are too big to be included + // in a future block, we must ban the transaction right away + debug!( + "[{:?}] Invalid transaction: too big storage proof", + pending_tx_hash + ); + unqueue_invalid.push(pending_tx_hash); + } else { + // The transaction is suspicious, but it could be a false positive + suspicious_txs.push(pending_tx_hash); + } + }, + Err(ApplyExtrinsicFailed(Validity(e))) if e.exhausted_resources() => { + pending_iterator.report_invalid(&pending_tx); + if skipped < MAX_SKIPPED_TRANSACTIONS { + skipped += 1; + debug!( + "Block seems full, but will try {} more transactions before quitting.", + MAX_SKIPPED_TRANSACTIONS - skipped, + ); + } else if (self.now)() < soft_deadline { + debug!( + "Block seems full, but we still have time before the soft deadline, \ + so we will try a bit more before quitting." + ); + } else { + debug!("Reached block weight limit, proceeding with proposing."); + break EndProposingReason::HitBlockWeightLimit + } + }, + Err(e) => { + pending_iterator.report_invalid(&pending_tx); + debug!("[{:?}] Invalid transaction: {}", pending_tx_hash, e); + unqueue_invalid.push(pending_tx_hash); + }, + } + } else { + // There is not enough space left in the block for this transaction pending_iterator.report_invalid(&pending_tx); if skipped < MAX_SKIPPED_TRANSACTIONS { skipped += 1; @@ -488,45 +563,14 @@ where break EndProposingReason::HitBlockSizeLimit } } - - trace!(target: LOG_TARGET, "[{:?}] Pushing to the block.", pending_tx_hash); - match sc_block_builder::BlockBuilder::push(block_builder, pending_tx_data) { - Ok(()) => { - transaction_pushed = true; - debug!(target: LOG_TARGET, "[{:?}] Pushed to the block.", pending_tx_hash); - }, - Err(ApplyExtrinsicFailed(Validity(e))) if e.exhausted_resources() => { - pending_iterator.report_invalid(&pending_tx); - if skipped < MAX_SKIPPED_TRANSACTIONS { - skipped += 1; - debug!(target: LOG_TARGET, - "Block seems full, but will try {} more transactions before quitting.", - MAX_SKIPPED_TRANSACTIONS - skipped, - ); - } else if (self.now)() < soft_deadline { - debug!(target: LOG_TARGET, - "Block seems full, but we still have time before the soft deadline, \ - so we will try a bit more before quitting." - ); - } else { - debug!( - target: LOG_TARGET, - "Reached block weight limit, proceeding with proposing." - ); - break EndProposingReason::HitBlockWeightLimit - } - }, - Err(e) => { - pending_iterator.report_invalid(&pending_tx); - debug!( - target: LOG_TARGET, - "[{:?}] Invalid transaction: {}", pending_tx_hash, e - ); - unqueue_invalid.push(pending_tx_hash); - }, - } }; + if matches!(end_reason, EndProposingReason::HitDeadline) && !transaction_pushed { + warn!("Hit deadline `{}` without including any transaction!", block_size_limit,); + // If we hits the hard deadline but the block still empty, we ban suspicious txs + self.transaction_pool.remove_invalid(&suspicious_txs); + } + if matches!(end_reason, EndProposingReason::HitBlockSizeLimit) && !transaction_pushed { warn!( target: LOG_TARGET, diff --git a/substrate/client/block-builder/src/lib.rs b/substrate/client/block-builder/src/lib.rs index 1878e7627480..c70c89a5af36 100644 --- a/substrate/client/block-builder/src/lib.rs +++ b/substrate/client/block-builder/src/lib.rs @@ -196,10 +196,18 @@ where /// Push onto the block's list of extrinsics. /// /// This will ensure the extrinsic can be validly executed (by executing it). - pub fn push(&mut self, xt: ::Extrinsic) -> Result<(), Error> { + /// If `remaining_size_for_proof` is provided, this function will ensure that the execution + /// of this extrinsic will not accrue the proof size more than the provided remaining size. + pub fn push( + &mut self, + xt: ::Extrinsic, + remaining_size_for_proof: Option, + ) -> Result<(), Error> { let parent_hash = self.parent_hash; let extrinsics = &mut self.extrinsics; let version = self.version; + let estimate_proof_size_before = + self.api.proof_recorder().map(|pr| pr.estimate_encoded_size()).unwrap_or(0); self.api.execute_in_transaction(|api| { let res = if version < 6 { @@ -212,6 +220,28 @@ where match res { Ok(Ok(_)) => { + // Verify that the transaction execution was not exhaust the proof size limit + if let Some(remaining_size_for_proof) = remaining_size_for_proof { + let estimate_proof_size_after = + api.proof_recorder().map(|pr| pr.estimate_encoded_size()).unwrap_or(0); + let estimate_proof_inflation = + estimate_proof_size_after.saturating_sub(estimate_proof_size_before); + + if estimate_proof_inflation > remaining_size_for_proof { + // The execution of the transaction results in exceeding the limits, + // we should rollback and return the error + // `InvalidTransaction::ExhaustsResources`. + return TransactionOutcome::Rollback(Err( + ApplyExtrinsicFailed::TooBigStorageProof( + estimate_proof_inflation, + remaining_size_for_proof, + ) + .into(), + )) + } + } + // The transaction is effectively committed only if the result of its execution + // does not exceed the limits extrinsics.push(xt); TransactionOutcome::Commit(Ok(())) }, @@ -346,7 +376,9 @@ mod tests { ) .unwrap(); - block_builder.push(ExtrinsicBuilder::new_read_and_panic(8).build()).unwrap_err(); + block_builder + .push(ExtrinsicBuilder::new_read_and_panic(8).build(), None) + .unwrap_err(); let block = block_builder.build().unwrap(); @@ -362,7 +394,7 @@ mod tests { ) .unwrap(); - block_builder.push(ExtrinsicBuilder::new_read(8).build()).unwrap(); + block_builder.push(ExtrinsicBuilder::new_read(8).build(), None).unwrap(); let block = block_builder.build().unwrap(); diff --git a/substrate/client/network/bitswap/src/lib.rs b/substrate/client/network/bitswap/src/lib.rs index 7bb8b0030653..0cd0756e5258 100644 --- a/substrate/client/network/bitswap/src/lib.rs +++ b/substrate/client/network/bitswap/src/lib.rs @@ -474,7 +474,7 @@ mod tests { let ext = ExtrinsicBuilder::new_indexed_call(vec![0x13, 0x37, 0x13, 0x38]).build(); let pattern_index = ext.encoded_size() - 4; - block_builder.push(ext.clone()).unwrap(); + block_builder.push(ext.clone(), None).unwrap(); let block = block_builder.build().unwrap().block; client.import(BlockOrigin::File, block).await.unwrap(); diff --git a/substrate/client/network/test/src/lib.rs b/substrate/client/network/test/src/lib.rs index d350b0e54ae1..d4272ad9a191 100644 --- a/substrate/client/network/test/src/lib.rs +++ b/substrate/client/network/test/src/lib.rs @@ -471,7 +471,7 @@ where amount: 1, nonce, }; - builder.push(transfer.into_unchecked_extrinsic()).unwrap(); + builder.push(transfer.into_unchecked_extrinsic(), None).unwrap(); nonce += 1; builder.build().unwrap().block }, diff --git a/substrate/client/offchain/src/lib.rs b/substrate/client/offchain/src/lib.rs index a11ac7d86ecb..e6946abef362 100644 --- a/substrate/client/offchain/src/lib.rs +++ b/substrate/client/offchain/src/lib.rs @@ -472,7 +472,7 @@ mod tests { let value = &b"world"[..]; let mut block_builder = client.new_block(Default::default()).unwrap(); let ext = ExtrinsicBuilder::new_offchain_index_set(key.to_vec(), value.to_vec()).build(); - block_builder.push(ext).unwrap(); + block_builder.push(ext, None).unwrap(); let block = block_builder.build().unwrap().block; block_on(client.import(BlockOrigin::Own, block)).unwrap(); @@ -481,7 +481,7 @@ mod tests { let mut block_builder = client.new_block(Default::default()).unwrap(); let ext = ExtrinsicBuilder::new_offchain_index_clear(key.to_vec()).nonce(1).build(); - block_builder.push(ext).unwrap(); + block_builder.push(ext, None).unwrap(); let block = block_builder.build().unwrap().block; block_on(client.import(BlockOrigin::Own, block)).unwrap(); diff --git a/substrate/client/transaction-pool/tests/pool.rs b/substrate/client/transaction-pool/tests/pool.rs index 4adf811b4252..aad92e637b39 100644 --- a/substrate/client/transaction-pool/tests/pool.rs +++ b/substrate/client/transaction-pool/tests/pool.rs @@ -955,7 +955,7 @@ fn import_notification_to_pool_maintain_works() { // Build the block with the transaction included let mut block_builder = client.new_block(Default::default()).unwrap(); - block_builder.push(xt).unwrap(); + block_builder.push(xt, None).unwrap(); let block = block_builder.build().unwrap().block; block_on(client.import(BlockOrigin::Own, block)).unwrap(); diff --git a/substrate/frame/balances/src/benchmarking.rs b/substrate/frame/balances/src/benchmarking.rs index 5641c68516c2..6207add3d978 100644 --- a/substrate/frame/balances/src/benchmarking.rs +++ b/substrate/frame/balances/src/benchmarking.rs @@ -31,6 +31,8 @@ const SEED: u32 = 0; // existential deposit multiplier const ED_MULTIPLIER: u32 = 10; +const MINIMUM_BALANCE: u32 = 100; + #[instance_benchmarks] mod benchmarks { use super::*; @@ -40,7 +42,7 @@ mod benchmarks { // * Transfer will create the recipient account. #[benchmark] fn transfer_allow_death() { - let existential_deposit = T::ExistentialDeposit::get(); + let existential_deposit: T::Balance = MINIMUM_BALANCE.into(); let caller = whitelisted_caller(); // Give some multiple of the existential deposit @@ -51,8 +53,7 @@ mod benchmarks { // and reap this user. let recipient: T::AccountId = account("recipient", 0, SEED); let recipient_lookup = T::Lookup::unlookup(recipient.clone()); - let transfer_amount = - existential_deposit.saturating_mul((ED_MULTIPLIER - 1).into()) + 1u32.into(); + let transfer_amount = balance; #[extrinsic_call] _(RawOrigin::Signed(caller.clone()), recipient_lookup, transfer_amount); @@ -75,7 +76,7 @@ mod benchmarks { as Currency<_>>::make_free_balance_be(&caller, T::Balance::max_value()); // Give the recipient account existential deposit (thus their account already exists). - let existential_deposit = T::ExistentialDeposit::get(); + let existential_deposit: T::Balance = MINIMUM_BALANCE.into(); let _ = as Currency<_>>::make_free_balance_be(&recipient, existential_deposit); let transfer_amount = existential_deposit.saturating_mul(ED_MULTIPLIER.into()); @@ -98,7 +99,7 @@ mod benchmarks { // Give the sender account max funds, thus a transfer will not kill account. let _ = as Currency<_>>::make_free_balance_be(&caller, T::Balance::max_value()); - let existential_deposit = T::ExistentialDeposit::get(); + let existential_deposit: T::Balance = MINIMUM_BALANCE.into(); let transfer_amount = existential_deposit.saturating_mul(ED_MULTIPLIER.into()); #[extrinsic_call] @@ -115,7 +116,7 @@ mod benchmarks { let user_lookup = T::Lookup::unlookup(user.clone()); // Give the user some initial balance. - let existential_deposit = T::ExistentialDeposit::get(); + let existential_deposit: T::Balance = MINIMUM_BALANCE.into(); let balance_amount = existential_deposit.saturating_mul(ED_MULTIPLIER.into()); let _ = as Currency<_>>::make_free_balance_be(&user, balance_amount); @@ -132,7 +133,7 @@ mod benchmarks { let user_lookup = T::Lookup::unlookup(user.clone()); // Give the user some initial balance. - let existential_deposit = T::ExistentialDeposit::get(); + let existential_deposit: T::Balance = MINIMUM_BALANCE.into(); let balance_amount = existential_deposit.saturating_mul(ED_MULTIPLIER.into()); let _ = as Currency<_>>::make_free_balance_be(&user, balance_amount); @@ -147,7 +148,7 @@ mod benchmarks { // * Transfer will create the recipient account. #[benchmark] fn force_transfer() { - let existential_deposit = T::ExistentialDeposit::get(); + let existential_deposit: T::Balance = MINIMUM_BALANCE.into(); let source: T::AccountId = account("source", 0, SEED); let source_lookup = T::Lookup::unlookup(source.clone()); @@ -159,8 +160,7 @@ mod benchmarks { // and reap this user. let recipient: T::AccountId = account("recipient", 0, SEED); let recipient_lookup = T::Lookup::unlookup(recipient.clone()); - let transfer_amount = - existential_deposit.saturating_mul((ED_MULTIPLIER - 1).into()) + 1u32.into(); + let transfer_amount = balance; #[extrinsic_call] _(RawOrigin::Root, source_lookup, recipient_lookup, transfer_amount); @@ -175,7 +175,7 @@ mod benchmarks { #[benchmark(extra)] fn transfer_increasing_users(u: Linear<0, 1_000>) { // 1_000 is not very much, but this upper bound can be controlled by the CLI. - let existential_deposit = T::ExistentialDeposit::get(); + let existential_deposit: T::Balance = MINIMUM_BALANCE.into(); let caller = whitelisted_caller(); // Give some multiple of the existential deposit @@ -214,7 +214,7 @@ mod benchmarks { let recipient_lookup = T::Lookup::unlookup(recipient.clone()); // Give some multiple of the existential deposit - let existential_deposit = T::ExistentialDeposit::get(); + let existential_deposit: T::Balance = MINIMUM_BALANCE.into(); let balance = existential_deposit.saturating_mul(ED_MULTIPLIER.into()); let _ = as Currency<_>>::make_free_balance_be(&caller, balance); @@ -231,7 +231,7 @@ mod benchmarks { let user_lookup = T::Lookup::unlookup(user.clone()); // Give some multiple of the existential deposit - let ed = T::ExistentialDeposit::get(); + let ed = MINIMUM_BALANCE.into(); let balance = ed + ed; let _ = as Currency<_>>::make_free_balance_be(&user, balance); @@ -257,8 +257,8 @@ mod benchmarks { .map(|i| -> T::AccountId { let user = account("old_user", i, SEED); let account = AccountData { - free: T::ExistentialDeposit::get(), - reserved: T::ExistentialDeposit::get(), + free: MINIMUM_BALANCE.into(), + reserved: MINIMUM_BALANCE.into(), frozen: Zero::zero(), flags: ExtraFlags::old_logic(), }; diff --git a/substrate/frame/identity/src/benchmarking.rs b/substrate/frame/identity/src/benchmarking.rs index 4b51d23f6b34..82d56d8c2d92 100644 --- a/substrate/frame/identity/src/benchmarking.rs +++ b/substrate/frame/identity/src/benchmarking.rs @@ -32,6 +32,8 @@ use sp_runtime::traits::Bounded; const SEED: u32 = 0; +const MINIMUM_BALANCE: u32 = 100; + fn assert_last_event(generic_event: ::RuntimeEvent) { frame_system::Pallet::::assert_last_event(generic_event.into()); } @@ -148,7 +150,7 @@ benchmarks! { for i in 0..r { let registrar: T::AccountId = account("registrar", i, SEED); let registrar_lookup = T::Lookup::unlookup(registrar.clone()); - let balance_to_use = T::Currency::minimum_balance() * 10u32.into(); + let balance_to_use = (MINIMUM_BALANCE * 10u32).into(); let _ = T::Currency::make_free_balance_be(®istrar, balance_to_use); Identity::::request_judgement(caller_origin.clone(), i, 10u32.into())?; @@ -221,7 +223,7 @@ benchmarks! { // User requests judgement from all the registrars, and they approve for i in 0..r { let registrar: T::AccountId = account("registrar", i, SEED); - let balance_to_use = T::Currency::minimum_balance() * 10u32.into(); + let balance_to_use = (MINIMUM_BALANCE * 10u32).into(); let _ = T::Currency::make_free_balance_be(®istrar, balance_to_use); Identity::::request_judgement(caller_origin.clone(), i, 10u32.into())?; @@ -378,7 +380,7 @@ benchmarks! { // User requests judgement from all the registrars, and they approve for i in 0..r { let registrar: T::AccountId = account("registrar", i, SEED); - let balance_to_use = T::Currency::minimum_balance() * 10u32.into(); + let balance_to_use = (MINIMUM_BALANCE * 10u32).into(); let _ = T::Currency::make_free_balance_be(®istrar, balance_to_use); Identity::::request_judgement(target_origin.clone(), i, 10u32.into())?; diff --git a/substrate/primitives/blockchain/src/error.rs b/substrate/primitives/blockchain/src/error.rs index 74a2ed3fba50..0d761e46560d 100644 --- a/substrate/primitives/blockchain/src/error.rs +++ b/substrate/primitives/blockchain/src/error.rs @@ -40,6 +40,9 @@ pub enum ApplyExtrinsicFailed { #[error("Application specific error")] Application(#[source] Box), + + #[error("Extrinsic execution generated a too big storage proof: {0}/{1}")] + TooBigStorageProof(usize, usize), } /// Substrate Client error diff --git a/substrate/test-utils/runtime/client/src/block_builder_ext.rs b/substrate/test-utils/runtime/client/src/block_builder_ext.rs index 78863209e33e..91ae728dc4fa 100644 --- a/substrate/test-utils/runtime/client/src/block_builder_ext.rs +++ b/substrate/test-utils/runtime/client/src/block_builder_ext.rs @@ -56,7 +56,7 @@ where &mut self, transfer: substrate_test_runtime::Transfer, ) -> Result<(), sp_blockchain::Error> { - self.push(transfer.into_unchecked_extrinsic()) + self.push(transfer.into_unchecked_extrinsic(), None) } fn push_storage_change( @@ -64,13 +64,13 @@ where key: Vec, value: Option>, ) -> Result<(), sp_blockchain::Error> { - self.push(ExtrinsicBuilder::new_storage_change(key, value).build()) + self.push(ExtrinsicBuilder::new_storage_change(key, value).build(), None) } fn push_deposit_log_digest_item( &mut self, log: sp_runtime::generic::DigestItem, ) -> Result<(), sp_blockchain::Error> { - self.push(ExtrinsicBuilder::new_deposit_log_digest_item(log).build()) + self.push(ExtrinsicBuilder::new_deposit_log_digest_item(log).build(), None) } } diff --git a/substrate/utils/frame/benchmarking-cli/src/extrinsic/bench.rs b/substrate/utils/frame/benchmarking-cli/src/extrinsic/bench.rs index 693b9f99f08e..48c08e021651 100644 --- a/substrate/utils/frame/benchmarking-cli/src/extrinsic/bench.rs +++ b/substrate/utils/frame/benchmarking-cli/src/extrinsic/bench.rs @@ -133,7 +133,7 @@ where // Create and insert the inherents. let inherents = builder.create_inherents(self.inherent_data.clone())?; for inherent in inherents { - builder.push(inherent)?; + builder.push(inherent, None)?; } // Return early if `ext_builder` is `None`. @@ -148,7 +148,7 @@ where let mut num_ext = 0; for nonce in 0..self.max_ext_per_block() { let ext = ext_builder.build(nonce)?; - match builder.push(ext.clone()) { + match builder.push(ext.clone(), None) { Ok(()) => {}, Err(ApplyExtrinsicFailed(Validity(TransactionValidityError::Invalid( InvalidTransaction::ExhaustsResources,