diff --git a/docs/001_authorizations.md b/docs/001_authorizations.md index 17486b4b..50054643 100644 --- a/docs/001_authorizations.md +++ b/docs/001_authorizations.md @@ -213,7 +213,7 @@ When `PermanentStorageNearCap` fires governance can either: Auto-renewal reuses the manual renew code path so the [Hard limit on renewed storage](#hard-limit-on-renewed-storage) accounting fires consistently — per-account `bytes_permanent` increment, chain-wide `PermanentStorageUsed` cap check, `kind = Renew` stamp in `Transactions`, obsolete-cleanup decrement. Hard-cap checks live in `check_authorization` (called by the extension's `check_signed` for the manual flow and by `do_process_auto_renewals` for the auto flow); the unified renewal mechanics live in `do_renew_in_memory` (called by `do_renew` and by the auto-renewal drain loop). -Behaviour on auto-renewal failure (per-account quota or chain-wide cap rejected at cycle time, or `MaxBlockTransactions` reached): the registration is dropped from `AutoRenewals`, `Event::AutoRenewalFailed` is emitted, and the data expires normally. +Behaviour on auto-renewal failure (per-account quota or chain-wide cap rejected at cycle time, or `MaxBlockTransactions` reached): the registration is dropped from `AutoRenewals`, `Event::AutoRenewalFailed` is emitted, and the data expires normally. If the slot cap rejects a cycle after the charge has been applied, `do_process_auto_renewals` refunds the chain-wide `PermanentStorageUsed` bump so the cap does not leak; the per-account `bytes_permanent` / `transactions` increments are left in place — slot-cap rejection at inherent time is pathological (the inherent runs before user txs and `len(pending) <= MaxBlockTransactions`), so the simpler accounting is preferred over a per-auth refund that would silently apply across roll-overs. The latest-entry guard in `on_initialize` skips an obsolete entry when `TransactionByContentHash[hash]` points to a later block — a manual `force_renew` may have moved the latest reference forward; the renewal cycle then fires from the new entry's expiry, not the original. diff --git a/pallets/transaction-storage/src/lib.rs b/pallets/transaction-storage/src/lib.rs index 7699b308..3ac822a3 100644 --- a/pallets/transaction-storage/src/lib.rs +++ b/pallets/transaction-storage/src/lib.rs @@ -1302,8 +1302,7 @@ pub mod pallet { Ok(()) } - /// Drain [`PendingAutoRenewals`], renewing each entry whose owner has sufficient - /// authorization. Returns the count drained. + /// Drain [`PendingAutoRenewals`] and return the count drained. /// /// Batches the [`BlockTransactions`] read/write across all `n` renewals by threading /// an in-memory accumulator through repeated [`Self::do_renew_in_memory`] calls. @@ -1348,64 +1347,72 @@ pub mod pallet { return n_actual; }, }; - let mut transactions = >::get(); - - for (content_hash, tx_info, renewal_data) in pending.into_iter() { - // `paid = true` means the cycle was already charged at registration - // (the one-shot `renew` path and the first cycle after - // `enable_auto_renew`). All other recurring cycles charge here. - let was_paid = renewal_data.paid; - let new_index = if was_paid { - Self::do_renew_in_memory(&mut transactions, &tx_info, extrinsic_index) - } else { + >::mutate(|transactions| { + for (content_hash, tx_info, renewal_data) in pending.into_iter() { + // `paid = true` means the cycle was already charged at registration + // (the one-shot `renew` path and the first cycle after + // `enable_auto_renew`). All other recurring cycles charge here. + let was_paid = renewal_data.paid; let scope = AuthorizationScope::Account(renewal_data.account.clone()); - if Self::check_authorization(&scope, tx_info.size, true, true).is_ok() { - Self::do_renew_in_memory(&mut transactions, &tx_info, extrinsic_index) + let charged = was_paid || + Self::check_authorization(&scope, tx_info.size, true, true).is_ok(); + let new_index = if charged { + Self::do_renew_in_memory(transactions, &tx_info, extrinsic_index) } else { None - } - }; + }; - if let Some(new_index) = new_index { - if !renewal_data.recurring { - // One-shot: registration is consumed. + if let Some(new_index) = new_index { + if !renewal_data.recurring { + // One-shot: registration is consumed. + AutoRenewals::::remove(content_hash); + } else if was_paid { + // Recurring: consume the prepayment so subsequent cycles + // charge per-cycle, and unblock `disable_auto_renew` for the + // owner now that the prepaid renewal has been delivered. + // `mutate` (not `insert`) so a Root `disable_auto_renew` + // executed earlier in the same block — between the + // `on_initialize` queue and this inherent — is not silently + // re-armed by a fresh insert. + AutoRenewals::::mutate(content_hash, |entry| { + if let Some(data) = entry { + data.paid = false; + } + }); + } + Self::deposit_event(Event::DataAutoRenewed { + index: new_index, + content_hash, + account: renewal_data.account, + }); + } else { + if charged { + // Reverse the chain-wide `PermanentStorageUsed` bump that + // `check_authorization` applied for this cycle. The per-account + // `bytes_permanent` / `transactions` increments are intentionally + // left burned: slot-cap rejection at inherent time is a chain-level + // pathological event (the inherent runs before any user extrinsics, + // and `len(pending) <= MaxBlockTransactions`), and reaching into the + // current `Authorizations` entry to refund would silently apply + // across auth roll-overs. + let size_u64: u64 = tx_info.size.into(); + Self::update_permanent_storage_used(|used| { + used.saturating_sub(size_u64) + }); + } AutoRenewals::::remove(content_hash); - } else if was_paid { - // Recurring: consume the prepayment so subsequent cycles - // charge per-cycle, and unblock `disable_auto_renew` for the - // owner now that the prepaid renewal has been delivered. - // `mutate` (not `insert`) so a Root `disable_auto_renew` - // executed earlier in the same block — between the - // `on_initialize` queue and this inherent — is not silently - // re-armed by a fresh insert. - AutoRenewals::::mutate(content_hash, |entry| { - if let Some(data) = entry { - data.paid = false; - } + Self::deposit_event(Event::AutoRenewalFailed { + content_hash, + account: renewal_data.account, }); } - Self::deposit_event(Event::DataAutoRenewed { - index: new_index, - content_hash, - account: renewal_data.account, - }); - } else { - AutoRenewals::::remove(content_hash); - Self::deposit_event(Event::AutoRenewalFailed { - content_hash, - account: renewal_data.account, - }); } - } - - >::put(transactions); + }); n_actual } - /// Centralized renewal mechanics: stamp the entry as `kind = Renew`, push it onto - /// the in-memory `BlockTransactions` accumulator, call `transaction_index::renew`, - /// and update [`TransactionByContentHash`]. Returns `None` at the per-block - /// `MaxBlockTransactions` cap. + /// Push a `kind = Renew` entry onto the in-memory accumulator and update + /// [`TransactionByContentHash`]. Returns `None` at `MaxBlockTransactions`. /// /// Called by: /// - [`Self::do_renew`] for the single-renewal manual flow (`force_renew`). @@ -1421,7 +1428,8 @@ pub mod pallet { info: &TransactionInfo, extrinsic_index: u32, ) -> Option { - let block_chunks = TransactionInfo::total_chunks(transactions) + num_chunks(info.size); + let block_chunks = + TransactionInfo::total_chunks(transactions).saturating_add(num_chunks(info.size)); let new_index = transactions.len() as u32; let new_info = TransactionInfo { chunk_root: info.chunk_root, @@ -1559,11 +1567,10 @@ pub mod pallet { fn do_renew(info: TransactionInfo) -> Result> { let extrinsic_index = >::extrinsic_index().ok_or(Error::::BadContext)?; - let mut transactions = >::get(); - let new_index = Self::do_renew_in_memory(&mut transactions, &info, extrinsic_index) - .ok_or(Error::::TooManyTransactions)?; - >::put(transactions); - Ok(new_index) + >::try_mutate(|transactions| { + Self::do_renew_in_memory(transactions, &info, extrinsic_index) + .ok_or(Error::::TooManyTransactions) + }) } /// Append a new entry to [`BlockTransactions`] (with the cumulative `block_chunks`) @@ -1579,22 +1586,24 @@ pub mod pallet { extrinsic_index: u32, kind: TransactionKind, ) -> Result> { - let mut transactions = >::get(); - let block_chunks = TransactionInfo::total_chunks(&transactions) + num_chunks(size); - let new_index = transactions.len() as u32; - transactions - .try_push(TransactionInfo { - chunk_root, - size, - content_hash, - hashing, - cid_codec, - extrinsic_index, - block_chunks, - kind, - }) - .map_err(|_| Error::::TooManyTransactions)?; - >::put(transactions); + let new_index = >::try_mutate(|transactions| { + let block_chunks = + TransactionInfo::total_chunks(transactions).saturating_add(num_chunks(size)); + let new_index = transactions.len() as u32; + transactions + .try_push(TransactionInfo { + chunk_root, + size, + content_hash, + hashing, + cid_codec, + extrinsic_index, + block_chunks, + kind, + }) + .map_err(|_| Error::::TooManyTransactions)?; + Ok::<_, Error>(new_index) + })?; TransactionByContentHash::::insert(content_hash, (Self::now(), new_index)); Ok(new_index) } @@ -1912,7 +1921,7 @@ pub mod pallet { } /// Returns the [`TransactionInfo`] for the specified store/renew transaction. - fn transaction_info( + pub(crate) fn transaction_info( block_number: BlockNumberFor, index: u32, ) -> Option { @@ -2236,9 +2245,13 @@ pub mod pallet { }, Call::::renew { entry } => { // Pre-paid one-shot: charges the same as `force_renew`. Cycle delivers - // without re-charging (see `do_process_auto_renewals`). + // without re-charging (see `do_process_auto_renewals`). Reject + // duplicates before charging — mirrors `enable_auto_renew` below. let info = Self::resolve_transaction_ref(entry).map_err(|_| RENEWED_NOT_FOUND)?; + if AutoRenewals::::contains_key(info.content_hash) { + return Err(AUTO_RENEWAL_ALREADY_ENABLED.into()); + } Self::check_authorization( &AuthorizationScope::Account(who.clone()), info.size, @@ -2545,10 +2558,10 @@ impl Pallet { } /// Verify the chain-wide permanent-storage accounting invariants: - /// - `PermanentStorageUsed == Σ Transactions[block][i].size where kind == Renew` — the counter - /// is exactly the sum of currently-on-chain renewed bytes; if these ever desync, the - /// chain-wide hard cap would over- or under-subscribe. - /// - `PermanentStorageUsed <= MaxPermanentStorageSize` — the chain-wide hard cap is honored. + /// - `PermanentStorageUsed == Σ Renew sizes in Transactions + Σ paid AutoRenewals sizes` — the + /// paid term covers the prepayment window between `renew` / `enable_auto_renew` charging the + /// counter and `do_process_auto_renewals` writing the `Renew` entry. + /// - `PermanentStorageUsed <= MaxPermanentStorageSize`. fn check_permanent_storage_accounting( _n: BlockNumberFor, ) -> Result<(), sp_runtime::TryRuntimeError> { @@ -2560,9 +2573,18 @@ impl Pallet { .filter(|t| matches!(t.kind, TransactionKind::Renew)) .fold(acc, |inner, t| inner.saturating_add(t.size as u64)) }); + let prepaid_sum: u64 = + AutoRenewals::::iter() + .filter(|(_, data)| data.paid) + .fold(0u64, |acc, (hash, _)| { + let size = TransactionByContentHash::::get(hash) + .and_then(|(block, index)| Self::transaction_info(block, index)) + .map_or(0, |info| info.size as u64); + acc.saturating_add(size) + }); ensure!( - renewed_sum == used, - "PermanentStorageUsed != Σ size of renewed Transactions entries", + renewed_sum.saturating_add(prepaid_sum) == used, + "PermanentStorageUsed != Σ renewed sizes + Σ paid auto-renewal sizes", ); ensure!( diff --git a/pallets/transaction-storage/src/tests.rs b/pallets/transaction-storage/src/tests.rs index 5f1bfeee..3133276b 100644 --- a/pallets/transaction-storage/src/tests.rs +++ b/pallets/transaction-storage/src/tests.rs @@ -1606,6 +1606,54 @@ fn try_state_passes_after_renew() { }); } +/// `renew` / `enable_auto_renew` charge `PermanentStorageUsed` up front but the +/// matching `Renew` entry only lands at the next retention boundary; `try_state` +/// must reconcile the counter against paid registrations in the meantime. +#[test] +fn try_state_passes_during_paid_auto_renewal_prepayment_window() { + new_test_ext().execute_with(|| { + run_to_block(1, || None); + let who = 1; + assert_ok!(TransactionStorage::authorize_account(RuntimeOrigin::root(), who, 0, 4000)); + let data = vec![42u8; 2000]; + let content_hash = blake2_256(&data); + let store_call = Call::store { data }; + assert_ok!(TransactionStorage::pre_dispatch_signed(&who, &store_call)); + assert_ok!(Into::::into(store_call).dispatch(RuntimeOrigin::none())); + run_to_block(3, || None); + + assert_ok!(renew_via_extension(who, TransactionRef::ContentHash(content_hash))); + assert_eq!(PermanentStorageUsed::get(), 2000); + assert!(AutoRenewals::get(content_hash).unwrap().paid); + + run_to_block(4, || None); + assert_ok!(TransactionStorage::do_try_state(System::block_number())); + }); +} + +/// As above, for `enable_auto_renew`. +#[test] +fn try_state_passes_during_enable_auto_renew_prepayment_window() { + new_test_ext().execute_with(|| { + run_to_block(1, || None); + let who = 1; + assert_ok!(TransactionStorage::authorize_account(RuntimeOrigin::root(), who, 0, 4000)); + let data = vec![42u8; 2000]; + let content_hash = blake2_256(&data); + let store_call = Call::store { data }; + assert_ok!(TransactionStorage::pre_dispatch_signed(&who, &store_call)); + assert_ok!(Into::::into(store_call).dispatch(RuntimeOrigin::none())); + run_to_block(3, || None); + + assert_ok!(enable_auto_renew_via_extension(who, content_hash)); + assert_eq!(PermanentStorageUsed::get(), 2000); + assert!(AutoRenewals::get(content_hash).unwrap().paid); + + run_to_block(4, || None); + assert_ok!(TransactionStorage::do_try_state(System::block_number())); + }); +} + /// `PermanentStorageUsed` desync from `Σ size of renewed Transactions entries` is caught. #[test] fn try_state_detects_permanent_used_mismatch_with_transactions() { @@ -1615,7 +1663,7 @@ fn try_state_detects_permanent_used_mismatch_with_transactions() { PermanentStorageUsed::put(2000); assert_err!( TransactionStorage::do_try_state(System::block_number()), - "PermanentStorageUsed != Σ size of renewed Transactions entries" + "PermanentStorageUsed != Σ renewed sizes + Σ paid auto-renewal sizes" ); }); } @@ -2657,6 +2705,64 @@ fn process_auto_renewals_continues_on_per_item_failure() { }); } +/// `paid = true` cycle rejected by the per-block slot cap refunds chain-wide +/// `PermanentStorageUsed`. Per-account `bytes_permanent` / `transactions` are +/// intentionally left burned — see the inline rationale in `do_process_auto_renewals`. +#[test] +fn paid_cycle_refunds_on_block_slot_cap() { + new_test_ext().execute_with(|| { + run_to_block(1, || None); + let who = 1; + let data = vec![7u8; 2000]; + let content_hash = blake2_256(&data); + + assert_ok!(TransactionStorage::authorize_account(RuntimeOrigin::root(), who, 10, 100_000)); + assert_ok!(TransactionStorage::store(RuntimeOrigin::none(), data)); + run_to_block(2, || None); + + assert_ok!(enable_auto_renew_via_extension(who, content_hash)); + assert_eq!(PermanentStorageUsed::get(), 2000); + let auth = Authorizations::get(AuthorizationScope::Account(who)).expect("auth exists"); + let permanent_before = auth.extent.bytes_permanent; + let transactions_before = auth.extent.transactions; + + init_block(12); + assert_eq!(PendingAutoRenewals::get().len(), 1); + + // Fill `BlockTransactions` so the paid drain has no slot to land in. + let max_txns = <::MaxBlockTransactions as Get>::get(); + BlockTransactions::mutate(|txns| { + for _ in 0..max_txns { + let _ = txns.try_push(TransactionInfo { + chunk_root: Default::default(), + size: 100, + content_hash: [0u8; 32], + hashing: crate::HashingAlgorithm::Blake2b256, + cid_codec: 0x55, + extrinsic_index: 0, + block_chunks: 0, + kind: crate::TransactionKind::Store, + }); + } + }); + + assert_ok!(TransactionStorage::apply_block_inherents(RuntimeOrigin::none(), None)); + + System::assert_has_event(RuntimeEvent::TransactionStorage(Event::AutoRenewalFailed { + content_hash, + account: who, + })); + assert!(AutoRenewals::get(content_hash).is_none()); + + assert_eq!(PermanentStorageUsed::get(), 0); + let auth = Authorizations::get(AuthorizationScope::Account(who)).expect("auth exists"); + assert_eq!(auth.extent.bytes_permanent, permanent_before); + assert_eq!(auth.extent.transactions, transactions_before); + + assert_ok!(TransactionStorage::do_try_state(System::block_number())); + }); +} + /// `renew` registers a one-shot renewal — `AutoRenewals[hash]` is created with /// `recurring = false` and `RenewalEnabled { recurring: false }` fires. #[test] @@ -2807,11 +2913,18 @@ fn renew_and_enable_auto_renew_conflict() { assert_ok!(TransactionStorage::store(RuntimeOrigin::none(), data)); run_to_block(2, || None); - // Schedule one-shot. assert_ok!(renew_via_extension(who, TransactionRef::Position { block: 1, index: 0 })); + let permanent_used_before = PermanentStorageUsed::get(); + + // Duplicate `renew` is rejected at the extension before any charge. + let dup_call = Call::renew { entry: TransactionRef::Position { block: 1, index: 0 } }; + assert_eq!( + TransactionStorage::validate_signed(&who, &dup_call).map(|_| ()), + Err(crate::AUTO_RENEWAL_ALREADY_ENABLED.into()), + ); + assert_eq!(PermanentStorageUsed::get(), permanent_used_before); - // Second `renew` for the same hash: rejected at dispatch (registration exists). - // Bypass the pool (pre_dispatch_signed) to land in dispatch with `Origin::Authorized`. + // Defensive dispatch-level guard still rejects if the extension is bypassed. let origin: RuntimeOrigin = Origin::::Authorized { who, scope: AuthorizationScope::Account(who) }.into(); assert_noop!( @@ -2819,7 +2932,6 @@ fn renew_and_enable_auto_renew_conflict() { Error::AutoRenewalAlreadyEnabled, ); - // `enable_auto_renew` for the same hash: also rejected (at the extension). let call = Call::enable_auto_renew { content_hash }; assert_eq!( TransactionStorage::validate_signed(&who, &call).map(|_| ()),