Skip to content

Skip bytes_permanent on re-renewal using existing TransactionKind#505

Open
mudigal wants to merge 12 commits into
mainfrom
nm-enhance-tests
Open

Skip bytes_permanent on re-renewal using existing TransactionKind#505
mudigal wants to merge 12 commits into
mainfrom
nm-enhance-tests

Conversation

@mudigal
Copy link
Copy Markdown
Contributor

@mudigal mudigal commented May 7, 2026

#506

Summary

  • Replace the AccountRenewals storage map and live-tracking machinery with a simple is_re_renewal: bool derived from the existing TransactionInfo.kind field. When the entry being renewed already has kind ==
  • Renew, it was previously renewed — skip the bytes_permanent capacity check and increment to avoid double-counting across renewal cycles. This is essential for content kept alive permanently: without the skip, every RetentionPeriod re-renewal would exhaust the per-account quota even though no new distinct content was added.
  • Make renew_content_hash feeless (consistent with renew which was already feeless)
  • Fix enable_auto_renew snapshot check to use the renew-axis (bytes_permanent) instead of the store-axis (bytes/transactions)
  • Refactor on_initialize obsolete-block cleanup to a single pass (avoids a redundant iteration and removes a .clone())

Test plan

  • cargo test -p pallet-bulletin-transaction-storage — 92 tests pass
  • cargo clippy --all-targets --all-features --workspace -- -D warnings — clean
  • Verify re-renewal across multiple RetentionPeriod cycles on a local testnet

Replace the stale increment-only bytes_permanent counter for account
scopes with a live-tracking approach using a new AccountRenewals
StorageDoubleMap keyed by (AccountId, ContentHash).

Key behavioral changes:
- Re-renewing the same content is a capacity no-op (entry overwritten)
- Entries that age out of the retention window automatically free quota
- No on_initialize overhead (live computation at check time)

Preimage scopes retain the cumulative bytes_permanent counter.

Updates check_authorization, authorization_extent, authorize, and
remove_expired_authorization. Adds validate_renew benchmark parameter
for AccountRenewals iteration cost. Updates docs/001_authorizations.md.
@mudigal mudigal requested review from bkontur, franciscoaguirre and karolk91 and removed request for bkontur May 7, 2026 19:04
Resolve conflicts in weights.rs (add r parameter to main's
benchmarked weights), lib.rs (keep live_permanent_bytes for account
scopes while adopting has_permanent_capacity helper for preimage
scopes), and docs (combine main's soft-limit wording with our
live-tracking hard-limit description).
@bkontur
Copy link
Copy Markdown
Collaborator

bkontur commented May 7, 2026

I don't think we need this, we decrement/reset bytes_permanent when authorization window is expired and when creating new one for the same account:

if Self::expired(authorization.expiration) {
// Expired-but-present: re-grant the caps, reset all consumed counters.
// The new window's `bytes_permanent` quota is independent of any
// renewed bytes still on chain from the old window.
authorization.expiration = expiration;
authorization.extent.bytes = 0;
authorization.extent.bytes_permanent = 0;

We had "live per-account bytes_permanent tracking", but we decided to remove it and simplify stuff here: #486

mudigal added 2 commits May 8, 2026 10:41
Replace the AccountRenewals storage map and live-tracking machinery
with a simple is_re_renewal bool derived from the existing
TransactionInfo.kind field. When the entry being renewed already has
kind == Renew, it was previously renewed — skip the bytes_permanent
capacity check and increment to avoid double-counting across renewal
cycles. This is essential for content kept alive permanently: without
the skip, every RetentionPeriod re-renewal would exhaust the
per-account quota even though no new distinct content was added.
The prior commit included re-benchmarked weight values from different
hardware. Revert all three weight files to main so the PR diff only
contains changes to functions we actually modified.
@mudigal mudigal changed the title Live per-account bytes_permanent tracking via AccountRenewals map Skip bytes_permanent on re-renewal using existing TransactionKind May 8, 2026
mudigal added 7 commits May 8, 2026 11:16
Resolve conflict in bulletin-paseo weight file by taking main's version
(this branch has no weight file changes).
Replace the per-entry `kind == Renew` re-renewal detection with an
AccountRenewals StorageDoubleMap keyed by (AccountId, ContentHash).
This ensures different accounts renewing the same content are each
charged bytes_permanent independently, matching store semantics.

Same account + same content = free (re-renewal skip).
Different account + same content = charged.

AccountRenewals entries are cleared on authorization expiry
(re-authorize path) and remove_expired_authorization.
Add Authorization Slots section describing the upcoming model from
RFC10: immutable 14-day slots from People Chain with fresh counters
per slot, eliminating the need for re-renewal detection.

Annotate existing sections (motivation, per-account quota,
authorize_account semantics) with notes on how Slots will simplify
the current kind-based skip and additive/refresh mechanisms.
Reset lib.rs and tests.rs to main. The kind-based re-renewal skip
and associated tests are no longer needed — Authorization Slots
(RFC10) will provide fresh counters per 14-day slot, making
re-renewal detection unnecessary at the protocol level.
Only docs/001_authorizations.md (Authorization Slots documentation)
remains as the sole change on this branch.
@mudigal
Copy link
Copy Markdown
Contributor Author

mudigal commented May 11, 2026

I don't think we need this, we decrement/reset bytes_permanent when authorization window is expired and when creating new one for the same account:

if Self::expired(authorization.expiration) {
// Expired-but-present: re-grant the caps, reset all consumed counters.
// The new window's `bytes_permanent` quota is independent of any
// renewed bytes still on chain from the old window.
authorization.expiration = expiration;
authorization.extent.bytes = 0;
authorization.extent.bytes_permanent = 0;

We had "live per-account bytes_permanent tracking", but we decided to remove it and simplify stuff here: #486

After an elaborate discussion about the slots. Due to the tight coupling between Poeple Chain slots and Bulletin Chain slots, the scenario doesnt even arise. As the problem is only within the Authorisation period. So reverted everything and updated the Authorisations design doc.

Comment on lines -209 to -213
- **Centralize accounting in `do_renew`.** Hard-cap checks (per-account, chain-wide) and the `kind = Renew` stamp must live in `do_renew`, called by `renew`, `renew_content_hash`, and `process_auto_renewals`.
- **Specify `process_auto_renewals` behavior on chain-wide cap rejection.** If `do_renew` rejects an auto-renewal because of `MaxPermanentStorageSize`, treat it the same as PR #313's "block full" path: remove the registration, emit `AutoRenewalFailed`, let the data expire normally.
- **Drop the snapshot check in `enable_auto_renew`** (or replace with a real reservation). The current check (`extent.transactions > 0 && extent.bytes >= tx_info.size`) is misleading and the per-window quota framing makes it even less meaningful — it suggests "this will work" while making no guarantees beyond the current block.
- **Reserve block-transaction slots for user txs.** `process_auto_renewals` is mandatory and pushes into the same `BlockTransactions` slot as user `store`/`renew`. Cap auto-renewals to a fraction of `MaxBlockTransactions` or partition the slot budget.
- **Per-content dedup of re-renewals (nice-to-have).** On `renew(X)`, look up the previous `(block, idx)` for `X` via `TransactionByContentHash` and cancel its pending decrement — drops the per-content double-count when the same content is renewed in multiple consecutive windows.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

all these are being worked on as TODO.
These are still needed to be done right @bkontur ?

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.

3 participants