diff --git a/prdoc/pr_10593.prdoc b/prdoc/pr_10593.prdoc new file mode 100644 index 000000000000..e3627d4792d2 --- /dev/null +++ b/prdoc/pr_10593.prdoc @@ -0,0 +1,7 @@ +title: Align Common Functions between Bulletin and SDK +doc: +- audience: Runtime Dev + description: The PR aligns common functions between Bulletin and SDK. +crates: +- name: pallet-transaction-storage + bump: major diff --git a/substrate/frame/transaction-storage/src/lib.rs b/substrate/frame/transaction-storage/src/lib.rs index a0206631da67..62040c9d8966 100644 --- a/substrate/frame/transaction-storage/src/lib.rs +++ b/substrate/frame/transaction-storage/src/lib.rs @@ -32,7 +32,7 @@ extern crate alloc; use alloc::vec::Vec; use codec::{Decode, Encode, MaxEncodedLen}; -use core::{fmt::Debug, result}; +use core::fmt::Debug; use frame_support::{ dispatch::GetDispatchInfo, pallet_prelude::InvalidTransaction, @@ -57,7 +57,7 @@ pub type CreditOf = Credit<::AccountId, ; /// Maximum data set in a single transaction in bytes. + #[pallet::constant] type MaxTransactionSize: Get; } @@ -185,8 +187,6 @@ pub mod pallet { NotConfigured, /// Renewed extrinsic is not found. RenewedNotFound, - /// Attempting to store an empty transaction - EmptyTransaction, /// Proof was not expected in this block. UnexpectedProof, /// Proof failed verification. @@ -199,8 +199,6 @@ pub mod pallet { DoubleCheck, /// Storage proof was not checked in the block. ProofNotChecked, - /// Transaction is too large. - TransactionTooLarge, /// Authorization was not found. AuthorizationNotFound, /// Authorization has not expired. @@ -248,6 +246,7 @@ pub mod pallet { }, "Storage proof must be checked once in the block" ); + // Insert new transactions, iff they have chunks. let transactions = BlockTransactions::::take(); let total_chunks = TransactionInfo::total_chunks(&transactions); @@ -255,29 +254,50 @@ pub mod pallet { Transactions::::insert(n, transactions); } } + + fn integrity_test() { + assert!( + !T::MaxBlockTransactions::get().is_zero(), + "MaxTransactionSize must be greater than zero" + ); + assert!( + !T::MaxTransactionSize::get().is_zero(), + "MaxTransactionSize must be greater than zero" + ); + let default_period = sp_transaction_storage_proof::DEFAULT_STORAGE_PERIOD.into(); + let storage_period = GenesisConfig::::default().storage_period; + assert_eq!( + storage_period, default_period, + "GenesisConfig.storage_period must match DEFAULT_STORAGE_PERIOD" + ); + } } #[pallet::call] impl Pallet { - /// Index and store data off chain. Minimum data size is 1 bytes, maximum is - /// `MaxTransactionSize`. Data will be removed after `STORAGE_PERIOD` blocks, unless `renew` + /// Index and store data off chain. Minimum data size is 1 byte, maximum is + /// `MaxTransactionSize`. Data will be removed after `StoragePeriod` blocks, unless `renew` /// is called. + /// + /// Emits [`Stored`](Event::Stored) when successful. + /// /// ## Complexity - /// - O(n*log(n)) of data size, as all data is pushed to an in-memory trie. + /// + /// O(n*log(n)) of data size, as all data is pushed to an in-memory trie. #[pallet::call_index(0)] #[pallet::weight(T::WeightInfo::store(data.len() as u32))] pub fn store(origin: OriginFor, data: Vec) -> DispatchResult { - ensure!(data.len() > 0, Error::::EmptyTransaction); - ensure!( - data.len() <= T::MaxTransactionSize::get() as usize, - Error::::TransactionTooLarge - ); + // In the case of a regular unsigned transaction, this should have been checked by + // pre_dispatch. In the case of a regular signed transaction, this should have been + // checked by pre_dispatch_signed. + Self::ensure_data_size_ok(data.len())?; let sender = ensure_signed(origin)?; Self::apply_fee(sender, data.len() as u32)?; // Chunk data and compute storage root - let chunk_count = num_chunks(data.len() as u32); - let chunks = data.chunks(CHUNK_SIZE).map(|c| c.to_vec()).collect(); + let chunks: Vec<_> = data.chunks(CHUNK_SIZE).map(|c| c.to_vec()).collect(); + let chunk_count = chunks.len() as u32; + debug_assert_eq!(chunk_count, num_chunks(data.len() as u32)); let root = sp_io::trie::blake2_256_ordered_root(chunks, sp_runtime::StateVersion::V1); let content_hash = sp_io::hashing::blake2_256(&data); @@ -299,19 +319,21 @@ pub mod pallet { content_hash: content_hash.into(), block_chunks: total_chunks, }) - .map_err(|_| Error::::TooManyTransactions)?; - Ok(()) + .map_err(|_| Error::::TooManyTransactions) })?; Self::deposit_event(Event::Stored { index, content_hash }); Ok(()) } - /// Renew previously stored data. Parameters are the block number that contains - /// previous `store` or `renew` call and transaction index within that block. - /// Transaction index is emitted in the `Stored` or `Renewed` event. - /// Applies same fees as `store`. + /// Renew previously stored data. Parameters are the block number that contains previous + /// `store` or `renew` call and transaction index within that block. Transaction index is + /// emitted in the `Stored` or `Renewed` event. Applies same fees as `store`. + /// + /// Emits [`Renewed`](Event::Renewed) when successful. + /// /// ## Complexity - /// - O(1). + /// + /// O(1). #[pallet::call_index(1)] #[pallet::weight(T::WeightInfo::renew())] pub fn renew( @@ -320,11 +342,15 @@ pub mod pallet { index: u32, ) -> DispatchResultWithPostInfo { let sender = ensure_signed(origin)?; - let transactions = Transactions::::get(block).ok_or(Error::::RenewedNotFound)?; - let info = transactions.get(index as usize).ok_or(Error::::RenewedNotFound)?; + let info = Self::transaction_info(block, index).ok_or(Error::::RenewedNotFound)?; + + // In the case of a regular unsigned transaction, this should have been checked by + // pre_dispatch. In the case of a regular signed transaction, this should have been + // checked by pre_dispatch_signed. + Self::ensure_data_size_ok(info.size as usize)?; + let extrinsic_index = frame_system::Pallet::::extrinsic_index().ok_or(Error::::BadContext)?; - Self::apply_fee(sender, info.size)?; let content_hash = info.content_hash.into(); sp_io::transaction_index::renew(extrinsic_index, content_hash); @@ -350,12 +376,12 @@ pub mod pallet { Ok(().into()) } - /// Check storage proof for block number `block_number() - StoragePeriod`. - /// If such a block does not exist, the proof is expected to be `None`. + /// Check storage proof for block number `block_number() - StoragePeriod`. If such a block + /// does not exist, the proof is expected to be `None`. /// /// ## Complexity - /// - Linear w.r.t the number of indexed transactions in the proved block for random - /// probing. + /// + /// Linear w.r.t the number of indexed transactions in the proved block for random probing. /// There's a DB read for each transaction. #[pallet::call_index(2)] #[pallet::weight((T::WeightInfo::check_proof_max(), DispatchClass::Mandatory))] @@ -464,9 +490,9 @@ pub mod pallet { #[pallet::genesis_build] impl BuildGenesisConfig for GenesisConfig { fn build(&self) { - ByteFee::::put(&self.byte_fee); - EntryFee::::put(&self.entry_fee); - StoragePeriod::::put(&self.storage_period); + ByteFee::::put(self.byte_fee); + EntryFee::::put(self.entry_fee); + StoragePeriod::::put(self.storage_period); } } @@ -483,10 +509,7 @@ pub mod pallet { proof.map(|proof| Call::check_proof { proof }) } - fn check_inherent( - _call: &Self::Call, - _data: &InherentData, - ) -> result::Result<(), Self::Error> { + fn check_inherent(_call: &Self::Call, _data: &InherentData) -> Result<(), Self::Error> { Ok(()) } @@ -523,6 +546,26 @@ pub mod pallet { Ok(()) } + /// Returns `true` if a blob of the given size can be stored. + fn data_size_ok(size: usize) -> bool { + (size > 0) && (size <= T::MaxTransactionSize::get() as usize) + } + + /// Ensures that the given data size is valid for storage. + fn ensure_data_size_ok(size: usize) -> Result<(), Error> { + ensure!(Self::data_size_ok(size), Error::::BadDataSize); + Ok(()) + } + + /// Returns the [`TransactionInfo`] for the specified store/renew transaction. + fn transaction_info( + block_number: BlockNumberFor, + index: u32, + ) -> Option { + let transactions = Transactions::::get(block_number)?; + transactions.into_iter().nth(index as usize) + } + /// Verifies that the provided proof corresponds to a randomly selected chunk from a list of /// transactions. pub(crate) fn verify_chunk_proof( @@ -533,7 +576,7 @@ pub mod pallet { // Get the random chunk index - from all transactions in the block = [0..total_chunks). let total_chunks: ChunkIndex = TransactionInfo::total_chunks(&infos); ensure!(total_chunks != 0, Error::::UnexpectedProof); - let selected_block_chunk_index = random_chunk(random_hash, total_chunks as _); + let selected_block_chunk_index = random_chunk(random_hash, total_chunks); // Let's find the corresponding transaction and its "local" chunk index for "global" // `selected_block_chunk_index`. @@ -553,7 +596,7 @@ pub mod pallet { // We shouldn't reach this point; we rely on the fact that `fn store` does not allow // empty transactions. Without this check, it would fail anyway below with // `InvalidProof`. - ensure!(!tx_info.block_chunks.is_zero(), Error::::EmptyTransaction); + ensure!(!tx_info.block_chunks.is_zero(), Error::::BadDataSize); // Convert a global chunk index into a transaction-local one. let tx_chunks = num_chunks(tx_info.size); diff --git a/substrate/frame/transaction-storage/src/tests.rs b/substrate/frame/transaction-storage/src/tests.rs index 7f3358430d4d..5b7093e2dd91 100644 --- a/substrate/frame/transaction-storage/src/tests.rs +++ b/substrate/frame/transaction-storage/src/tests.rs @@ -33,11 +33,11 @@ fn discards_data() { let caller = 1; assert_ok!(TransactionStorage::::store( RawOrigin::Signed(caller).into(), - vec![0u8; 2000 as usize] + vec![0u8; 2000] )); assert_ok!(TransactionStorage::::store( RawOrigin::Signed(caller).into(), - vec![0u8; 2000 as usize] + vec![0u8; 2000] )); let proof_provider = || { let block_num = frame_system::Pallet::::block_number(); @@ -176,7 +176,7 @@ fn renews_data() { let caller = 1; assert_noop!( TransactionStorage::::store(RawOrigin::Signed(caller).into(), vec![]), - Error::::EmptyTransaction + Error::::BadDataSize ); assert_ok!(TransactionStorage::::store( RawOrigin::Signed(caller).into(), @@ -201,7 +201,7 @@ fn renews_data() { }; run_to_block(16, proof_provider); assert!(Transactions::::get(1).is_none()); - assert_eq!(Transactions::::get(6).unwrap().get(0), Some(info).as_ref()); + assert_eq!(Transactions::::get(6).unwrap().first(), Some(info).as_ref()); run_to_block(17, proof_provider); assert!(Transactions::::get(6).is_none()); });