Skip to content

Centralize renewal mechanics in do_renew_in_memory#515

Merged
bkontur merged 9 commits into
paritytech:mainfrom
rosarp:rs-centralize-do-renew
May 22, 2026
Merged

Centralize renewal mechanics in do_renew_in_memory#515
bkontur merged 9 commits into
paritytech:mainfrom
rosarp:rs-centralize-do-renew

Conversation

@rosarp
Copy link
Copy Markdown
Member

@rosarp rosarp commented May 10, 2026

Centralize renewal mechanics in a single helper do_renew_in_memory:
stamp kind = Renew, push onto BlockTransactions, call transaction_index::renew, update TransactionByContentHash. Returns None at the per-block MaxBlockTransactions cap.

All three entry points go through it:

  • renew / renew_content_hash via do_renew (BlockTransactions::try_mutate).
  • Auto-renewal drain via do_process_auto_renewals (BlockTransactions::mutate wrapping the whole loop, so the BoundedVec is SCALE-encoded once per block instead of n times — preserves the O(n) batching).

Hard-cap accounting (per-account bytes_permanent, chain-wide PermanentStorageUsed) is unchanged: check_authorization handles it on the extension's pre_dispatch for manual renewals, and inline in do_process_auto_renewals for auto.

do_process_auto_renewals's failure path now uniformly drops the registration and emits AutoRenewalFailed whether the gate failed on expired/missing auth, per-account cap, chain-wide cap, or block-slot cap; the on-chain data expires normally on its existing RetentionPeriod. When a cycle that had already been charged (a paid = true prepayment, or the per-cycle charge applied for the failing recurring case) is rejected by the per-block slot cap, the new refund_renewal_charge helper reverses the chain-wide PermanentStorageUsed bump so the cap does not leak; the per-account bytes_permanent / transactions increments are intentionally left in place — slot-cap rejection at inherent time is pathological (the inherent runs before user txs and len(pending) <= MaxBlockTransactions), and refunding into the current Authorizations entry would silently apply across auth roll-overs.

Also fixes the check_permanent_storage_accounting try_state invariant: renew / enable_auto_renew charge PermanentStorageUsed up front in check_signed, but the matching Renew entry only lands in Transactions once the cycle fires at the next retention boundary. The invariant now reconciles PermanentStorageUsed against Σ Renew sizes in Transactions + Σ paid AutoRenewals sizes, so try_state passes during the prepayment window.

Adds an AUTO_RENEWAL_ALREADY_ENABLED rejection in the extension's validate_signed for renew, mirroring the existing enable_auto_renew guard — duplicate registrations are rejected at pool admission before any charge, instead of charging in pre_dispatch and then erroring in the dispatch body.

Lays groundwork for the TODO items in docs/001_authorizations.md.

@rosarp rosarp force-pushed the rs-centralize-do-renew branch from b921b5d to 2754589 Compare May 10, 2026 19:43
@rosarp
Copy link
Copy Markdown
Member Author

rosarp commented May 10, 2026

/cmd fmt

Comment thread pallets/transaction-storage/src/lib.rs Outdated
Comment thread pallets/transaction-storage/src/lib.rs Outdated
TransactionKind::Renew,
)?;
sp_io::transaction_index::renew(extrinsic_index, content_hash);
let mut transactions = <BlockTransactions<T>>::get();
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rosarp instead of get/put, we should be able to use <BlockTransactions<T>>::try_mutate(|transactions| { pattern - do we have similar places?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also looks "strange" that "do_renew_in_memory" modifies some "transactions.try_push(new_info).ok()?;, why it cannot mutate directly BlockTransactions? BlockTransactions` are never stored in DB right, it is transient stuff, so the performance shouldnt degradate?

Copy link
Copy Markdown
Member Author

@rosarp rosarp May 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get/put changed to try_mutate.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're correct that no disk write happens. BlockTransactions is cleared via take() in on_finalize, so its value never reaches the on-disk trie at block commit. The overlay holds it in memory for the block's duration. The cost being paid is SCALE encode/decode, not disk I/O.

Why an &mut accumulator avoids it. mutate(|t| { … }) decodes once when entering the closure and encodes once when leaving it. Inside the closure, mutations against &mut BoundedVec are pure in-memory Rust — no codec, no storage layer. So we open the closure, run the whole loop with do_renew_in_memory(t, …) inside it, and close. That's the whole purpose of do_renew_in_memory taking &mut BoundedVec: it's the in-memory shape that both try_mutate (single call) and mutate (loop) hand to their closures, so the same helper serves both paths without forcing a per-call codec round-trip.

@rosarp rosarp force-pushed the rs-centralize-do-renew branch from 332d351 to 0a38270 Compare May 21, 2026 10:45
bkontur added 7 commits May 21, 2026 23:05
`renew` and `enable_auto_renew` charge `PermanentStorageUsed` up front in
`check_signed`, but the matching `Renew` entry only lands in `Transactions`
once the cycle fires at the next retention boundary. The
`check_permanent_storage_accounting` invariant now sums paid `AutoRenewals`
sizes alongside on-chain `Renew` entries so try_state passes during the
prepayment window.
Slot-cap rejection at inherent time is pathological — inherent runs
before user extrinsics, and `len(pending) <= MaxBlockTransactions`.
Reaching into the current `Authorizations` entry on the refund path
would silently apply across auth roll-overs, so leave the per-account
`bytes_permanent` / `transactions` increments burned and only restore
the chain-wide counter that `try_state` reconciles.
Drops the single-call `refund_chain_wide_renewal_charge` helper and moves
the rationale to a comment at the call site in `do_process_auto_renewals`.

Switches the two `block_chunks` calculations in `do_renew_in_memory` and
`append_to_block_transactions` from raw `+` to `saturating_add`, matching
the saturating discipline used elsewhere in the pallet.
@bkontur bkontur enabled auto-merge (squash) May 22, 2026 12:16
@bkontur bkontur merged commit 067ff20 into paritytech:main May 22, 2026
47 checks passed
@bkontur bkontur deleted the rs-centralize-do-renew branch May 22, 2026 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants