Skip to content

Commit

Permalink
remove getter from transaction storage pallet (paritytech#4885)
Browse files Browse the repository at this point in the history
### ISSUE
Link to the issue:
paritytech#3326
cc @muraca 

Deliverables
- [Deprecation] remove pallet::getter usage from
pallet-transaction-storage


### Test Outcomes
___
cargo test -p pallet-transaction-storage --features runtime-benchmarks 

running 9 tests
test mock::test_genesis_config_builds ... ok
test tests::burns_fee ... ok
test mock::__construct_runtime_integrity_test::runtime_integrity_tests
... ok
test tests::discards_data ... ok
test tests::renews_data ... ok
test benchmarking::bench_renew ... ok
test benchmarking::bench_store ... ok
test tests::checks_proof ... ok
test benchmarking::bench_check_proof_max has been running for over 60
seconds
test benchmarking::bench_check_proof_max ... ok

test result: ok. 9 passed; 0 failed; 0 ignored; 0 measured; 0 filtered
out; finished in 72.57s

   Doc-tests pallet-transaction-storage

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered
out; finished in 0.00s

---

Polkadot Address: 16htXkeVhfroBhL6nuqiwknfXKcT6WadJPZqEi2jRf9z4XPY
  • Loading branch information
Aideepakchaudhary authored and Jay Pan committed Dec 27, 2024
1 parent a0510ba commit 279e48b
Show file tree
Hide file tree
Showing 3 changed files with 65 additions and 39 deletions.
14 changes: 14 additions & 0 deletions prdoc/pr_4885.prdoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0
# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json

title: Removed `pallet::getter` usage from the pallet-transaction-storage

doc:
- audience: Runtime Dev
description: |
This PR removed `pallet::getter`s from `pallet-transaction-storage`s storage items.
When accessed inside the pallet, use the syntax `StorageItem::<T, I>::get()`.

crates:
- name: pallet-transaction-storage
bump: minor
78 changes: 45 additions & 33 deletions substrate/frame/transaction-storage/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -159,33 +159,33 @@ pub mod pallet {
fn on_initialize(n: BlockNumberFor<T>) -> Weight {
// Drop obsolete roots. The proof for `obsolete` will be checked later
// in this block, so we drop `obsolete` - 1.
let period = <StoragePeriod<T>>::get();
let period = StoragePeriod::<T>::get();
let obsolete = n.saturating_sub(period.saturating_add(One::one()));
if obsolete > Zero::zero() {
<Transactions<T>>::remove(obsolete);
<ChunkCount<T>>::remove(obsolete);
Transactions::<T>::remove(obsolete);
ChunkCount::<T>::remove(obsolete);
}
// 2 writes in `on_initialize` and 2 writes + 2 reads in `on_finalize`
T::DbWeight::get().reads_writes(2, 4)
}

fn on_finalize(n: BlockNumberFor<T>) {
assert!(
<ProofChecked<T>>::take() || {
ProofChecked::<T>::take() || {
// Proof is not required for early or empty blocks.
let number = <frame_system::Pallet<T>>::block_number();
let period = <StoragePeriod<T>>::get();
let number = frame_system::Pallet::<T>::block_number();
let period = StoragePeriod::<T>::get();
let target_number = number.saturating_sub(period);
target_number.is_zero() || <ChunkCount<T>>::get(target_number) == 0
target_number.is_zero() || ChunkCount::<T>::get(target_number) == 0
},
"Storage proof must be checked once in the block"
);
// Insert new transactions
let transactions = <BlockTransactions<T>>::take();
let transactions = BlockTransactions::<T>::take();
let total_chunks = transactions.last().map_or(0, |t| t.block_chunks);
if total_chunks != 0 {
<ChunkCount<T>>::insert(n, total_chunks);
<Transactions<T>>::insert(n, transactions);
ChunkCount::<T>::insert(n, total_chunks);
Transactions::<T>::insert(n, transactions);
}
}
}
Expand Down Expand Up @@ -215,11 +215,11 @@ pub mod pallet {

let content_hash = sp_io::hashing::blake2_256(&data);
let extrinsic_index =
<frame_system::Pallet<T>>::extrinsic_index().ok_or(Error::<T>::BadContext)?;
frame_system::Pallet::<T>::extrinsic_index().ok_or(Error::<T>::BadContext)?;
sp_io::transaction_index::index(extrinsic_index, data.len() as u32, content_hash);

let mut index = 0;
<BlockTransactions<T>>::mutate(|transactions| {
BlockTransactions::<T>::mutate(|transactions| {
if transactions.len() + 1 > T::MaxBlockTransactions::get() as usize {
return Err(Error::<T>::TooManyTransactions)
}
Expand Down Expand Up @@ -253,17 +253,17 @@ pub mod pallet {
index: u32,
) -> DispatchResultWithPostInfo {
let sender = ensure_signed(origin)?;
let transactions = <Transactions<T>>::get(block).ok_or(Error::<T>::RenewedNotFound)?;
let transactions = Transactions::<T>::get(block).ok_or(Error::<T>::RenewedNotFound)?;
let info = transactions.get(index as usize).ok_or(Error::<T>::RenewedNotFound)?;
let extrinsic_index =
<frame_system::Pallet<T>>::extrinsic_index().ok_or(Error::<T>::BadContext)?;
frame_system::Pallet::<T>::extrinsic_index().ok_or(Error::<T>::BadContext)?;

Self::apply_fee(sender, info.size)?;

sp_io::transaction_index::renew(extrinsic_index, info.content_hash.into());

let mut index = 0;
<BlockTransactions<T>>::mutate(|transactions| {
BlockTransactions::<T>::mutate(|transactions| {
if transactions.len() + 1 > T::MaxBlockTransactions::get() as usize {
return Err(Error::<T>::TooManyTransactions)
}
Expand Down Expand Up @@ -297,15 +297,15 @@ pub mod pallet {
) -> DispatchResultWithPostInfo {
ensure_none(origin)?;
ensure!(!ProofChecked::<T>::get(), Error::<T>::DoubleCheck);
let number = <frame_system::Pallet<T>>::block_number();
let period = <StoragePeriod<T>>::get();
let number = frame_system::Pallet::<T>::block_number();
let period = StoragePeriod::<T>::get();
let target_number = number.saturating_sub(period);
ensure!(!target_number.is_zero(), Error::<T>::UnexpectedProof);
let total_chunks = <ChunkCount<T>>::get(target_number);
let total_chunks = ChunkCount::<T>::get(target_number);
ensure!(total_chunks != 0, Error::<T>::UnexpectedProof);
let parent_hash = <frame_system::Pallet<T>>::parent_hash();
let parent_hash = frame_system::Pallet::<T>::parent_hash();
let selected_chunk_index = random_chunk(parent_hash.as_ref(), total_chunks);
let (info, chunk_index) = match <Transactions<T>>::get(target_number) {
let (info, chunk_index) = match Transactions::<T>::get(target_number) {
Some(infos) => {
let index = match infos
.binary_search_by_key(&selected_chunk_index, |info| info.block_chunks)
Expand Down Expand Up @@ -349,8 +349,7 @@ pub mod pallet {

/// Collection of transaction metadata by block number.
#[pallet::storage]
#[pallet::getter(fn transaction_roots)]
pub(super) type Transactions<T: Config> = StorageMap<
pub type Transactions<T: Config> = StorageMap<
_,
Blake2_128Concat,
BlockNumberFor<T>,
Expand All @@ -360,32 +359,30 @@ pub mod pallet {

/// Count indexed chunks for each block.
#[pallet::storage]
pub(super) type ChunkCount<T: Config> =
pub type ChunkCount<T: Config> =
StorageMap<_, Blake2_128Concat, BlockNumberFor<T>, u32, ValueQuery>;

#[pallet::storage]
#[pallet::getter(fn byte_fee)]
/// Storage fee per byte.
pub(super) type ByteFee<T: Config> = StorageValue<_, BalanceOf<T>>;
pub type ByteFee<T: Config> = StorageValue<_, BalanceOf<T>>;

#[pallet::storage]
#[pallet::getter(fn entry_fee)]
/// Storage fee per transaction.
pub(super) type EntryFee<T: Config> = StorageValue<_, BalanceOf<T>>;
pub type EntryFee<T: Config> = StorageValue<_, BalanceOf<T>>;

/// Storage period for data in blocks. Should match `sp_storage_proof::DEFAULT_STORAGE_PERIOD`
/// for block authoring.
#[pallet::storage]
pub(super) type StoragePeriod<T: Config> = StorageValue<_, BlockNumberFor<T>, ValueQuery>;
pub type StoragePeriod<T: Config> = StorageValue<_, BlockNumberFor<T>, ValueQuery>;

// Intermediates
#[pallet::storage]
pub(super) type BlockTransactions<T: Config> =
pub type BlockTransactions<T: Config> =
StorageValue<_, BoundedVec<TransactionInfo, T::MaxBlockTransactions>, ValueQuery>;

/// Was the proof checked in this block?
#[pallet::storage]
pub(super) type ProofChecked<T: Config> = StorageValue<_, bool, ValueQuery>;
pub type ProofChecked<T: Config> = StorageValue<_, bool, ValueQuery>;

#[pallet::genesis_config]
pub struct GenesisConfig<T: Config> {
Expand All @@ -407,9 +404,9 @@ pub mod pallet {
#[pallet::genesis_build]
impl<T: Config> BuildGenesisConfig for GenesisConfig<T> {
fn build(&self) {
<ByteFee<T>>::put(&self.byte_fee);
<EntryFee<T>>::put(&self.entry_fee);
<StoragePeriod<T>>::put(&self.storage_period);
ByteFee::<T>::put(&self.byte_fee);
EntryFee::<T>::put(&self.entry_fee);
StoragePeriod::<T>::put(&self.storage_period);
}
}

Expand Down Expand Up @@ -439,6 +436,21 @@ pub mod pallet {
}

impl<T: Config> Pallet<T> {
/// Get transaction storage information from outside of this pallet.
pub fn transaction_roots(
block: BlockNumberFor<T>,
) -> Option<BoundedVec<TransactionInfo, T::MaxBlockTransactions>> {
Transactions::<T>::get(block)
}
/// Get ByteFee storage information from outside of this pallet.
pub fn byte_fee() -> Option<BalanceOf<T>> {
ByteFee::<T>::get()
}
/// Get EntryFee storage information from outside of this pallet.
pub fn entry_fee() -> Option<BalanceOf<T>> {
EntryFee::<T>::get()
}

fn apply_fee(sender: T::AccountId, size: u32) -> DispatchResult {
let byte_fee = ByteFee::<T>::get().ok_or(Error::<T>::NotConfigured)?;
let entry_fee = EntryFee::<T>::get().ok_or(Error::<T>::NotConfigured)?;
Expand Down
12 changes: 6 additions & 6 deletions substrate/frame/transaction-storage/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,9 @@ fn discards_data() {
vec![0u8; 2000 as usize]
));
let proof_provider = || {
let block_num = <frame_system::Pallet<Test>>::block_number();
let block_num = frame_system::Pallet::<Test>::block_number();
if block_num == 11 {
let parent_hash = <frame_system::Pallet<Test>>::parent_hash();
let parent_hash = frame_system::Pallet::<Test>::parent_hash();
Some(
build_proof(parent_hash.as_ref(), vec![vec![0u8; 2000], vec![0u8; 2000]])
.unwrap(),
Expand Down Expand Up @@ -92,15 +92,15 @@ fn checks_proof() {
vec![0u8; MAX_DATA_SIZE as usize]
));
run_to_block(10, || None);
let parent_hash = <frame_system::Pallet<Test>>::parent_hash();
let parent_hash = frame_system::Pallet::<Test>::parent_hash();
let proof =
build_proof(parent_hash.as_ref(), vec![vec![0u8; MAX_DATA_SIZE as usize]]).unwrap();
assert_noop!(
TransactionStorage::<Test>::check_proof(RuntimeOrigin::none(), proof,),
Error::<Test>::UnexpectedProof,
);
run_to_block(11, || None);
let parent_hash = <frame_system::Pallet<Test>>::parent_hash();
let parent_hash = frame_system::Pallet::<Test>::parent_hash();

let invalid_proof = build_proof(parent_hash.as_ref(), vec![vec![0u8; 1000]]).unwrap();
assert_noop!(
Expand Down Expand Up @@ -132,9 +132,9 @@ fn renews_data() {
));
assert_eq!(Balances::free_balance(1), 1_000_000_000 - 4000 * 2 - 200 * 2);
let proof_provider = || {
let block_num = <frame_system::Pallet<Test>>::block_number();
let block_num = frame_system::Pallet::<Test>::block_number();
if block_num == 11 || block_num == 16 {
let parent_hash = <frame_system::Pallet<Test>>::parent_hash();
let parent_hash = frame_system::Pallet::<Test>::parent_hash();
Some(build_proof(parent_hash.as_ref(), vec![vec![0u8; 2000]]).unwrap())
} else {
None
Expand Down

0 comments on commit 279e48b

Please sign in to comment.