Skip to content

fix(platform): backfill historical credit nullifiers at pv14 activation - #5341

Merged
shumkov merged 4 commits into
v5.0-devfrom
credit-pool-pv14-backfill
Oct 11, 2026
Merged

shumkov merged 4 commits into
v5.0-devfrom
credit-pool-pv14-backfill

Conversation

@shumkov

@shumkov shumkov commented Oct 8, 2026 •

Copy link
Copy Markdown
Collaborator

Basic explanation

What this does: Before processing the first PV14 candidate block's transitions, add every retained CREDIT note's rho (its note-uniqueness value) to the permanent nullifier set. Existing entries remain intact.

Value: Historical CREDIT notes receive the same rho-reuse protection as newly recorded notes, including notes created by both shielding and shielded spends. Legitimate spends of historical outputs still work.

Risks: This changes consensus state at PV14 activation on networks still below PV14 and scans the full historical CREDIT pool. Fixed pages bound temporary scan allocations, not the cumulative RocksDB transaction write set. The implementation has passed local tests and independent source review. Merge/release remains blocked by the validator-cost and budget gates below. The owner has resolved the already-activated devnet policy by requiring devnet recreation, as recorded below. The historical TokenHistory storage-flags failure found during the local full-wrapper rehearsal is corrected here; both authentic-copy full-wrapper rehearsals pass as detailed below.

Issue being fixed or feature implemented

The permanent nullifier set can omit rhos from pre-PV14 CREDIT notes. The regression reproduces an otherwise valid, freshly signed PV14 transition retaining an actual stored PV13 rho/cmx: unfixed first-block behavior accepts it; the backfill makes the existing nullifier-reuse guard reject it.

What was done?

  • Add the one-time Drive::backfill_historical_credit_pool_nullifiers helper under drive/shielded/migration, following the existing contract migration layout. Call it through the existing upgrade ladder only when crossing <14 to >=14, in the candidate transaction before ordinary transitions; retain wrapper/cache behavior. The helper has no separate method-version dispatcher or table slot.
  • Compute N_after = N_before union {rho of EVERY stored CREDIT note} without encrypted-payload classification. Raw-check matching elements, preserve existing Item bytes/flags, and insert only absent empty Items. Notes, commitments, balances, frontier, anchors and note counts stay unchanged; token and per-block-sync state are outside the write path.
  • Use fixed SystemLimits of 2048 notes/page (one commitment-tree chunk) and 2048 inserts/batch. Sort/deduplicate per page, flush full and final partial batches before the next page, and never apply empty batches. Validate structural shape even for empty/all-present pools, the fixed initial count, complete consecutive pages, checked arithmetic and the full 96-byte prefix; propagate corruption/write errors without committing or external progress state.
  • Preserve historical TokenHistory storage ownership during its existing PV14 schema update. Read flags from the candidate transaction and supply current-epoch flags with the same optional owner to the existing storage-accounting merger. Historical allocations and the version Item flags remain intact; absent/unflagged contracts stay unflagged. No GroveDB, fee, schema or migration-order change.

In-place changes to shipped generations

The added backfill call and TokenHistory flags correction are inside the existing transition_to_version_14 function in first-block-events generation 0. Its ladder guard requires previous_protocol_version < 14 && platform_version.protocol_version >= 14, so replay targeting shipped PV<=13 cannot reach it. The migration has no separate method-version slot. Earlier SystemLimits retain zero placeholders, which the historical block path never reads; PV14 supplies the nonzero page/batch limits. Existing PV<=13 branches and wrapper/cache semantics are unchanged. This does not establish compatibility with already-activated PV14 history. On 2026-10-10, Ivan confirmed that devnet will be dropped and recreated. Existing Sakura PV14 state/history is therefore excluded from supported upgrade and replay targets; devnet must be recreated before running this binary. Mainnet/testnet are the historical migration targets. No devnet reset has been performed by this PR.

How Has This Been Tested?

Original backfill verification at signed 4baeff85, local macOS / Rust 1.98.1, reviewed source based on ad63a4861b6df3ff0a7e8de6879e10bc93221179. All passing runs below exited 0, with no ignored tests. The target branch subsequently advanced; these receipts do not claim testing of an upstream integration.

Actual RED → GREEN: Before production edits, cargo test -j 2 -p drive-abci --lib should_refuse_historical_credit_rho_reuse_on_first_pv14_candidate -- --nocapture failed (exit 101, 0 passed/1 failed): it received SuccessfulExecution instead of NullifierAlreadySpentError. The same command after the fix passed (1/1). The fixture proves acceptance of that exact newly signed PV14 transition without migration, exact historical-rho rejection after first-block events, and fresh-rho acceptance. It does not rely on rejection of old signatures or wire bytes.

Command Result / meaningful coverage
cargo test -j 2 -p drive --lib backfill_historical_credit_pool_nullifiers -- --nocapture 7 passed: full/partial/empty pages, cross-page repeated rho after partial flush, flagged existing Items, exact union, malformed rows/shapes/count drift, failure after real nonempty batches, drop/retry and commit/reopen/repeat
cargo test -j 2 -p drive-abci --lib backfill -- --nocapture 3 passed: version skipping/PV13 unchanged/fresh PV14 genesis, real mixed shield-and-spend history and a legitimate proof-backed spend of a historical output
cargo test -j 2 -p drive-abci --lib shield::tests::tests::nullifiers -- --nocapture 8 passed, including the reuse regression and existing nullifier guards
cargo test -j 2 -p drive-abci --test strategy_tests should_backfill_credit_nullifiers_when_accepted_activation_survives_a_rejected_round -- --nocapture 1 passed, covering FIVE public ABCI variants listed below
Compiled current test binary: target/debug/deps/drive_abci-2459d754f77b38f7 check_for_desired_protocol_upgrade::v1 --nocapture 11 passed: upgrade-vote threshold/cache behavior
cargo clippy -j 2 -p platform-version -p drive -p drive-abci --all-features --all-targets -- -D warnings Passed at original backfill head 4baeff85
cargo fmt --all -- --check; git diff --check Passed
cargo check -j 2 --workspace --all-targets Passed on resumed run; initial run was intentionally interrupted at the disk reserve, not a compiler failure

The public process_proposal/finalize_block test compares clean A; accepted A → executed/rejected B → finalize A; the same from a reopened committed PV13 DB; A/B → accepted alternate C → finalize A; and teardown/reopen while migrated A is still speculative → retry A → finalize A. All five produce the same committed A root, preserve committed state before finalize, expose the union in the candidate transaction, and retain it after committed DB reopen.

Coverage limits: lifecycle rows are synthetic; the real cryptographic reuse fixture uses first-block events plus transition processing rather than a reuse-containing full ABCI proposal. Fault injection is at the I/O adapter after real applied batches, and restart is application/Drive teardown/reopen, not an OS kill. Untouched descendant subtrees/nonzero-balance sentinels are not exhaustively populated. No authentic finalized-activation replay or local full workspace test suite was run. GitHub Rust workspace tests passed for 4baeff85 in Tests run 37939725600; that prior-head result does not qualify the follow-up commit. Independent reviewers examined source/log evidence; they did not rerun tests.

TokenHistory activation regression

The authentic mainnet wrapper stopped before CREDIT backfill with storage_cost cost mismatch added: 0 replaced: 146 actual:110. Fresh-genesis fixtures lacked the historical flags produced by the PV9 insertion path. The new regression runs the real PV8→PV13 insertion ladder, then the public PV14 first-block dispatcher: RED (exit 101, intended-success test fails with that exact mismatch), then GREEN with the identical tests after the flags correction (2/2).

Follow-up verification on signed commit e5bde732 is recorded separately from the original backfill receipts:

Command (all Cargo invocations use --locked --offline -j 2) Result
cargo test -p drive-abci --lib perform_events_on_first_block_of_protocol_change -- --nocapture 43 passed, including 9 focused TokenHistory tests: historical insertion, owners/ownerless flags, same/cross/existing epochs, absent/current/unflagged contracts, preserved claim/index data, candidate-local reads, drop/retry/reopen and version skipping
cargo test -p drive --lib backfill_historical_credit_pool_nullifiers -- --nocapture 7 passed
cargo test -p drive-abci --test strategy_tests credit_pool_backfill_tests -- --nocapture 2 passed / ten lifecycle variants: original CREDIT scenarios plus historical flagged TokenHistory, exact activation-epoch allocation and cache-aware V2 reads

cargo clippy --locked --offline -p drive-abci --all-targets --all-features -j 2, scoped rustfmt and git diff --check passed. Clippy reported the existing DPP manifest warning about inherited default-features; no lint warning was emitted. These follow-up suites total 52 production/repository tests (43 + 2 + 7), with no ignored tests.

Prior-head CI (verified 2026-10-10, before the migration-layout refactor): The complete Tests workflow 37957424836 succeeded on 5da30111, including Rust workspace, E2E, browser and Swift checks. The Kotlin workflow 37957424474 also succeeded on that head. Conditionally skipped jobs are not represented as passed tests.

The uncommitted measurement harness also has an actual RED→GREEN regression for valid growth into an existing activation-epoch allocation. Its 8 tests pass. Independent production/lifecycle reviews found no production blocker. Final source and measurement binary hashes are bound and verified; complete-copy results appear below. No test or review of local tooling is represented as production activation qualification.

One-time migration layout follow-up (2026-10-11)

Signed head a2ec46430e moves the helper and tests to drive/shielded/migration, matching drive/contract/migration. The public method name and mandatory transaction stay the same. The redundant dispatcher, _v0 names and optional method slot/initializers are removed. The existing PV14 ladder guard remains the sole activation boundary; after release the migration behavior still must remain fixed for replay.

Normalized source comparison confirms the complete migration algorithm is identical to 5da30111 apart from documentation, visibility, names and the removed inline attribute. ABCI source, including the activation guard/call, cache wrapper and TokenHistory correction, is unchanged. Numeric SystemLimits are unchanged. The obsolete direct-helper inactive-version assertion is replaced by separate zero-page/zero-batch cases checking the exact error and unchanged candidate/committed roots; commit/reopen/repeat and public historical-version coverage remain.

Fresh local verification on this source (all Cargo commands use --locked --offline -j 2):

Command Result
cargo test -p drive --lib backfill_historical_credit_pool_nullifiers -- --nocapture 8 passed
cargo test -p drive-abci --lib perform_events_on_first_block_of_protocol_change -- --nocapture 43 passed
cargo test -p drive-abci --lib should_refuse_historical_credit_rho_reuse_on_first_pv14_candidate -- --nocapture 1 passed; freshly signed historical-rho regression retained
cargo test -p drive-abci --test strategy_tests credit_pool_backfill_tests -- --nocapture 2 passed / ten public lifecycle scenarios
cargo clippy -p platform-version -p drive -p drive-abci --all-features --all-targets -- -D warnings Passed on retry
Scoped rustfmt --check; git diff --check Passed

54 targeted tests passed, 0 failed, 0 ignored. Two independent actual-diff reviewers found no must-fix; they verified source equality, activation reachability and test preservation without rerunning tests. This is a layout refactor, not a new behavioral bug fix, so no new RED→GREEN claim is made.

The first clippy attempt failed on absent dependency metadata (E0463 / missing .rmeta), not a reported lint finding. Incident 20261011-100435-64321 was recorded before diagnosis. An identical normal Cargo retry regenerated dependencies and passed without source, configuration, cleanup or sccache service changes. The host's automatic sweep had reported removing 12.25 GiB from this target; that is a plausible cause, not independently proven. The existing DPP manifest default-features warning remains.

New-head CI and bot review must be evaluated separately; the successful prior-head workflows above do not qualify this commit. Authentic-copy timings below remain bound to their original source/binary and were not rerun or relabeled as new-head measurements. Validator-cost/memory and budget gates remain open.

Real-state Mac rehearsal (2026-10-09)

All four fresh-copy runs below passed, using the reviewed full public first-block wrapper from production commit e5bde732. The subsequent pre-refactor head 5da30111 only corrected PV14 documentation comments; its non-comment source was unchanged. The independently checked measurement binary SHA256 is 92800a4cf984172fc40b2aa0d01bc86a63ab1602f8186f9b178a27bb56ecb726, with a frozen source manifest. The uncommitted harness's embedded base field remains 4baeff85; the manifest and binary receipt bind the corrected source.

Committed checkpoint Notes / distinct rhos Existing nullifiers Inserted Final union
Mainnet evo1, H449083, PV13 3,686 1,682 2,004 3,686
Testnet dash-testnet-51, H615322, PV13 4,789 2,285 2,504 4,789

Both source archives and all members (14 mainnet / 33 testnet) passed checksums and size verification. Native integrity, persisted/standalone state and matching-height header/AppHash/root checks passed. Neither snapshot contained duplicate rhos or an existing token pool. Original node databases were not opened or modified.

Network First full wrapper Retry after drop Independent clean wrapper Storage commit Result
Mainnet 0.646214 s 0.507471 s 0.699384 s 0.048135 s PASS: equal first/retry/clean/reopened roots
Testnet 7.026461 s 6.936789 s 7.065689 s 0.429365 s PASS: equal first/retry/clean/reopened roots

Every candidate/retry/commit/reopen check verifies the exact nullifier union, original Item bytes/flags, protected CREDIT logical contents and native integrity. Dropping candidates restores the original root/state. TokenHistory reaches schema/version 2, retains owner/base/history and exact version-Item flags, and adds 108 storage bytes at the chosen activation epoch; document/index serialized public logical data remains equal. Both networks also pass the PV14→PV14 ladder bypass (not a second backfill execution). Independent fresh controls match the retry roots and TokenHistory results.

These timings cover the complete first-block migration wrapper, including the other PV14 migrations and contract-cache work, on Ivan's ARM64 Mac in dev profile, warm after census/integrity. They are not complete public ABCI block timings. Earlier helper-only measurements at 4baeff85 were approximately 0.375 s mainnet and 0.943 s testnet; those must not be substituted for the full-wrapper numbers above. The original mainnet TokenHistory failure is now traversed successfully on both authentic copies.

Whole-process measurements include repeated full-DB validation and are not migration-call latency or transaction-only peak memory:

Verification invocation Whole wall time Whole-process maximum RSS
Mainnet retry 248.07 s 271,630,336 bytes
Mainnet commit 254.05 s 276,135,936 bytes
Testnet retry 1063.23 s 826,261,504 bytes
Testnet commit 850.24 s 822,165,504 bytes

Activation context is explicitly simulated (H+1, last time+5000 ms, retained Core height, next epoch). Storage-only commit/reopen retains old consensus height/PV/AppHash and proves migration-delta durability, not a finalized network activation. Header consistency is not independent signature/chain-proof verification. Public logical digests do not prove native frontier/reference-metadata byte equality. Specs, the measurement harness, snapshots and receipts remain uncommitted working artifacts.

OPEN merge/release gates

  • Owner-confirmed devnet policy (2026-10-10): Ivan confirmed that devnet will be dropped and recreated. Existing Sakura PV14 state/history is excluded from supported upgrade/replay targets; devnet recreation is an operational prerequisite before running this binary. The public network monitor, observed 2026-10-09 16:41 UTC, reported mainnet/testnet at PV13 and Sakura at PV14. Already-active PV14 skips this crossing-only backfill, and replay of its activation can differ. No affected-rho census or compatibility claim is made for the excluded Sakura history. No live reset or activation was performed; executing the reset remains a separate operational action.
  • Collected and locally validated: Both committed checkpoints and successful full-wrapper rollback/retry/independent commit/reopen. Mainnet canonical archive SHA256: e39a044044024929b06209ae7562ee9f12622ed7ae2c2016d1f6924153e4069f; testnet canonical tar SHA256: 62a492369b1e3f3292004ede8639d1d76ea3fd7948b6b5b1da4cd700ced70980.
  • Open: Cold/release validator-equivalent full ABCI activation costs, transaction write-set and peak allocation memory, seeks/hashes/physical IO, isolated storage growth and retry/growth margins. Bounded scan pages do not bound the complete transaction write set.
  • Open: An agreed numeric validator activation budget and hardware/memory envelope. The owner's approximately five-minute suggestion is tentative; no memory ceiling is agreed. Local passes do not authorize merging or activating a network protocol.

Breaking Changes

No change to PV<=13 replay or public wire formats. PV14 activation intentionally changes historical CREDIT nullifier state and rejects historical rho reuse. Per the owner decision of 2026-10-10, devnet will be dropped and recreated; existing PV14 devnet state/history is excluded from supported upgrade/replay targets. This PR does not supply an in-place repair or later-protocol migration for that state.

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have added "!" to the title and described breaking changes in the corresponding section if my code contains any
  • I have made corresponding changes to the documentation if needed
  • If I added or changed GroveDB structure, I described it in the area's structure.rs, regenerated grovedb-structure.json, and checked the structure viewer link posted on this pull request

Existing tree layout is unchanged; the migration populates the existing CREDIT nullifier tree. Working specs, reports and evidence are intentionally excluded from this PR.

For repository code-owners and collaborators only

  • I have assigned this pull request to a milestone

PR Hygiene · a2ec464

  • Bots — coderabbitai ✓ · thepastaclaw ✓
  • Self-review — post /self-reviewed
  • Build green
  • Approvals — you own every area touched; none needed

When every merge requirement is met, the PR Hygiene check passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.

Summary by CodeRabbit

  • New Features
    • Protocol 14 activation now records nullifiers for retained CREDIT notes, including historical outputs, in bounded batches. Existing nullifiers are preserved.
    • TokenHistory contracts retain their stored flags and document data during the upgrade.
  • Bug Fixes
    • Historical outputs can no longer be spent again after activation; valid spends of previously unspent outputs remain supported.
  • Tests
    • Added coverage for migration behavior across activation, retries, restarts, and repeated upgrades.

Union every stored CREDIT rho into permanent nullifiers in the first eligible PV14 candidate transaction, inserting only absent empty Items and preserving existing bytes and flags. Use fixed chunk-aligned pages and bounded batches with structural corruption checks and transactional failure propagation.

Test would have caught this in CI: ✖ before fix, ✔ after. The regression retains a real PV13 rho/cmx in a valid newly signed PV14 transition; unfixed execution accepts it and the migration produces the exact nullifier-reuse error. Cover real mixed shield/spend history and five public ABCI commit/reject/restart variants.

The added call in the existing first-block generation is reachable only when the ladder crosses from below 14 to at least 14; replay targeting shipped PV<=13 cannot reach it and frozen method tables remain inactive. Authentic per-network snapshots, full cumulative activation measurements and the agreed validator budget remain shipping gates.
@coderabbitai

coderabbitai Bot commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: dashpay/platform/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 71f1d808-3efc-40c4-bc4f-f5e439bc9d2c


📥 Commits

Reviewing files that changed from the base of the PR and between e5bde73 and a2ec464.



📒 Files selected for processing (7)
  • packages/rs-drive/src/drive/shielded/migration/backfill_historical_credit_pool_nullifiers.rs
  • packages/rs-drive/src/drive/shielded/migration/backfill_historical_credit_pool_nullifiers/tests.rs
  • packages/rs-drive/src/drive/shielded/migration/mod.rs
  • packages/rs-drive/src/drive/shielded/mod.rs
  • packages/rs-platform-version/src/version/mocks/v2_test.rs
  • packages/rs-platform-version/src/version/system_limits/mod.rs
  • packages/rs-platform-version/src/version/v14.rs


💤 Files with no reviewable changes (1)
  • packages/rs-platform-version/src/version/mocks/v2_test.rs


🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/rs-platform-version/src/version/system_limits/mod.rs


Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.




📝 Walkthrough
📝 Walkthrough

Walkthrough

The protocol activation transition now backfills missing nullifiers for retained CREDIT notes. The migration reads notes in configured pages and inserts nullifiers in batches within the caller’s transaction. The transition also reapplies TokenHistory with its stored flags when they exist.

Changes

CREDIT Nullifier Migration

Layer / File(s) Summary
Backfill limits and module wiring
packages/rs-platform-version/src/version/system_limits/*, packages/rs-platform-version/src/version/v14.rs, packages/rs-drive/src/drive/shielded/...
SystemLimits adds page-size and batch-size limits. Versions 1–3 set both to zero; version 4 sets both to 2,048. Drive declares the server-side migration module.
Read notes and insert nullifiers
packages/rs-drive/src/drive/shielded/migration/*
Drive validates note pages and tree types, then inserts missing rho nullifiers in batches within the supplied transaction. Tests cover pagination, existing entries, invalid data, retries, persistence, and idempotent calls.
Protocol activation and TokenHistory update
packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/*, packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs
The transition preserves stored TokenHistory flags and applies current-epoch flags when present. It also runs the CREDIT nullifier backfill. Tests cover TokenHistory storage and data, activation versions, replay, and transaction behavior.
Shield and proposal lifecycle tests
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shield/tests.rs, packages/rs-drive-abci/tests/strategy_tests/test_cases/*
Shield tests verify historical nullifiers after activation. Strategy tests exercise candidate acceptance, rejection, restart, finalization, and persisted state.

Priority: ➖ Normal

Estimated code review effort: 4 (Complex) | ~45 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant ProtocolUpgradeTransition
  participant Drive
  participant CommitmentTree
  participant ProvableCountTree
  ProtocolUpgradeTransition->>Drive: backfill historical CREDIT nullifiers
  Drive->>CommitmentTree: read ordered note pages
  CommitmentTree-->>Drive: return note rows and positions
  Drive->>ProvableCountTree: insert missing rho Items in batches
Loading

Suggested reviewers: thepastaclaw



Merge Risk: 🟡 Moderate · up to a2ec4

At PV14 activation, every historical CREDIT note's rho is added to the nullifier set in a single block transaction. The logic is well tested, but its time and memory cost on real network state has not been measured against an agreed budget. Measure that cost before merging to make sure the activation block can complete on validators.

Pre-merge checks | Passed 4 | Failed 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 60.29% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 68 functions across 31 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main change: backfilling historical CREDIT nullifiers during PV14 activation.

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR



🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR



  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added this to the v5.0.0 milestone Oct 8, 2026
@thepastaclaw

thepastaclaw commented Oct 8, 2026 •

Copy link
Copy Markdown
Collaborator

✅ Final review complete — no blockers (commit a2ec464) · triage: critical

@shumkov
shumkov marked this pull request as ready for review October 9, 2026 13:50
@github-actions

github-actions Bot commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

Bots are done — your move: post /self-reviewed.
Full checklist in the description.

@github-actions github-actions Bot added waiting-self-review Waiting for the author to post /self-reviewed bot-review-skipped A required review bot did not report; it was skipped by the window or by a person. labels Oct 9, 2026

@thepastaclaw thepastaclaw left a comment

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.

Final validation — Phase 1 + Phase 2

Static verification at 4baeff8 confirms one documentation inconsistency: the PV14 changelog excludes historical nullifiers that this PR now backfills. No in-scope implementation blocker was verified, and no builds or tests were run in this static lane. This is not merge or activation readiness approval: the author-reported complete-PV14 wrapper failure and cumulative activation-budget gates remain open, while Rust workspace tests and PR Hygiene were pending in the supplied CI snapshot.

🟡 1 suggestion(s)

Review provenance

Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 6: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

  • Triage: critical by gpt-6.1-sol (effort low) — The intricate paginated storage backfill in Drive::backfill_historical_credit_pool_nullifiers and its transition_to_version_14 activation call change consensus-persisted nullifier state, requiring scrutiny of migration completeness, transaction atomicity, and validator resource limits.
  • Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
  • Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity below 15% reserve: weekly 24% left, 5h 0% left), glm-5.3-flash (not used above high effort; tier asks max)
  • Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
  • Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
  • Phase 2 reviewers: gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — platform-versioning (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort xhigh); agent phase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-platform-version/src/version/drive_versions/v9.rs`:
- [SUGGESTION] packages/rs-platform-version/src/version/drive_versions/v9.rs:160: Update the PV14 changelog to describe the historical backfill
  Enabling this slot causes PV14 activation to add every retained CREDIT note's rho to the permanent nullifier set, including historical shielding outputs. However, change 76 in packages/rs-platform-version/src/version/v14.rs still explicitly states at lines 2052–2053: "Nullifiers revealed by shields before this version are not added." That now contradicts the activation behavior and its intentional state-root and rho-reuse acceptance changes. Update change 76 or add a correcting numbered entry, and extend PLATFORM_V14's drive and system_limits annotations to describe the historical union, Some(0) activation, and fixed 2048-note page/2048-insert batch limits. The Platform Version and Versioned Dispatch chapters explicitly require these changelog and snapshot annotations for consensus changes.

Comment thread packages/rs-platform-version/src/version/drive_versions/v9.rs Outdated
Read storage flags from the activation transaction and preserve the existing optional owner while accounting schema growth at the current epoch. The existing PV14 ladder guard excludes shipped PV<=13 replay.

Test would have caught this in CI: ✖ before fix, ✔ after. The real historical insertion ladder reproduces replaced146/actual110 before the change; identical tests pass afterward. Add owner/allocation preservation, transaction-local reads, rollback/retry/reopen, version-skip and public candidate/finalize regression coverage.

Validation: 43 first-block tests, 2 public lifecycle tests (10 scenarios), 7 Drive backfill tests, scoped rustfmt and all-feature/all-target drive-abci clippy pass. Authentic full-wrapper and validator-budget qualification remain separate gates.
@github-actions

github-actions Bot commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

Waiting for bot review — coderabbitai ✓ · thepastaclaw not yet. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.

@github-actions github-actions Bot added waiting-bots Waiting for the review bots to report on this head and removed waiting-self-review Waiting for the author to post /self-reviewed bot-review-skipped A required review bot did not report; it was skipped by the window or by a person. labels Oct 9, 2026
Correct the stale nullifier changelog and annotate PV14 method activation and 2048-entry page/batch limits. Addresses the review suggestion on PR #5341.

Tests intentionally not rerun: comments only, with unchanged non-comment source verified and standalone rustfmt/diff checks passing. No behavior, version table or storage value changes.
@github-actions

github-actions Bot commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

Waiting for bot review — coderabbitai not yet · thepastaclaw requested changes — dismiss the review or push a fix, 1 thread unresolved — resolve it. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.

@thepastaclaw thepastaclaw left a comment

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.

Re-review — Final validation — Phase 1 + Phase 2

The historical CREDIT backfill uses versioned dispatch and candidate-local writes, and the prior changelog issue is fixed. One blocking compatibility issue remains: the PR changes PV14 activation behavior while the supplied evidence reports an already-active PV14 network whose replay/reset policy is unresolved. This was static verification only; the exact-head CI snapshot reports successful Rust workspace tests, with other checks pending, and does not close the documented activation-resource qualification gates.

🔴 1 blocking

Review provenance

Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 8: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 9: gpt-6-astra (agent: phase2-reviewer, role: platform-versioning); reviewer 10: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 11: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); reviewer 12: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

  • Triage: critical by gpt-6.1-sol (effort low) — The paginated backfill in backfill_historical_credit_pool_nullifiers/v0/mod.rs and its transition_to_version_14 activation modify consensus-persisted nullifier state through an intricate storage migration, alongside historical TokenHistory storage-flag preservation.
  • Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
  • Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity below 15% reserve: weekly 24% left, 5h 1% left), glm-5.3-flash (not used above high effort; tier asks max)
  • Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
  • Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
  • Phase 2 reviewers: gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — platform-versioning (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — general (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — architecture-layering (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — platform-versioning (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — security-auditor (completed, effort xhigh); agent phase2-reviewer
  • Model comparison: every Phase-2 reviewer also ran on gpt-6-astra; the verifier weighed both sets without knowing which model wrote which
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs`:
- [BLOCKING] packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs:877-880: Resolve already-active PV14 compatibility before redefining activation
  The supplied PR evidence reports Sakura already on PV14 and explicitly leaves its replay/reset policy unresolved. The `previous_protocol_version < 14 && platform_version.protocol_version >= 14` guard preserves PV<=13 execution, but it does not preserve an already-finalized PV14 activation. Compared with the base implementation, this call inserts additional permanent nullifier keys whenever retained CREDIT notes have missing rhos; replaying such an activation therefore produces a different state root. Conversely, `run_block_proposal` does not run first-block events when resuming at the same protocol version, and this ladder rung remains skipped when upgrading from PV14, so an existing PV14 database receives no repair. The TokenHistory correction also modifies this same activation rung; proving that PV<=13 cannot reach it does not establish PV14 compatibility. This does not prove Sakura has affected history, but it prevents treating PV14 as safely mutable without resolving that history and support policy. Establish an owner-approved reset/exclusion policy and verify the affected network history before merging. If existing PV14 history must remain supported, preserve its activation behavior and introduce the corrections through a later protocol snapshot and migration that reaches already-PV14 databases, with replay and upgrade regression coverage.

@github-actions

github-actions Bot commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

Your move: thepastaclaw requested changes on this head; dismiss the review or push a fix.
Full checklist in the description.

@github-actions github-actions Bot added waiting-self-review Waiting for the author to post /self-reviewed bot-review-skipped A required review bot did not report; it was skipped by the window or by a person. and removed waiting-bots Waiting for the review bots to report on this head labels Oct 9, 2026
Follow the existing one-shot contract migration layout. The PV14 upgrade
ladder remains the activation gate; remove the extra method dispatcher
and optional version-table slot without changing the migration algorithm,
candidate transaction, call ordering or numeric limits.

Preserve commit/reopen/repeat coverage and check both zero-limit failures
before candidate writes. No new bug-fix RED/GREEN is claimed for this
layout refactor; the existing historical-rho regression remains passing.

Validation: 54 targeted tests passed, scoped rustfmt and all-feature,
all-target clippy with -D warnings passed; two independent diff reviews.
Normalized algorithm equality and unchanged ABCI source verified.
@github-actions

github-actions Bot commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor

Waiting for bot review — coderabbitai ✓ · thepastaclaw not yet. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.

@github-actions github-actions Bot added waiting-bots Waiting for the review bots to report on this head and removed waiting-self-review Waiting for the author to post /self-reviewed bot-review-skipped A required review bot did not report; it was skipped by the window or by a person. labels Oct 11, 2026

@thepastaclaw thepastaclaw left a comment

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.

Re-review — Final validation — Phase 1 + Phase 2

Static verification at a2ec464 found no in-scope code blocker, but the TokenHistory activation correction is missing from the required PV14 protocol changelog. The prior CREDIT changelog issue is fixed; the recorded devnet exclusion/recreation policy removes the prior compatibility objection without establishing compatibility with existing PV14 history. No local builds or tests were run: the supplied exact-head CI snapshot shows Rust workspace, Swift and Kotlin checks passing, with the main test suite and browser shard 1 still pending; validator-resource qualification, an agreed activation budget and devnet recreation remain outstanding prerequisites.

🟡 1 suggestion(s)

Review provenance

Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 6: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

  • Triage: critical by gpt-6.1-sol (effort low) — The intricate backfill_historical_credit_pool_nullifiers migration and transition_to_version_14 changes alter consensus-persisted nullifier state and historical storage ownership at protocol activation, with pagination, batching, corruption validation, and transaction semantics.
  • Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
  • Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity below 15% reserve: weekly 14% left, 5h 100% left), glm-5.3-flash (not used above high effort; tier asks max)
  • Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
  • Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
  • Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
  • Phase 2 reviewers: gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — platform-versioning (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — platform-versioning (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort xhigh); agent phase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs:820-827: Document the TokenHistory activation ownership correction
  Supplying these storage flags changes the PV14 activation outcome for historically flagged TokenHistory contracts: it preserves the optional storage owner, lets the existing merger retain historical allocations and account for growth at the activation epoch, and avoids the previous unflagged-replacement accounting failure. The numbered changelog in packages/rs-platform-version/src/version/v14.rs documents the CREDIT backfill but does not describe this separate persisted-state correction. book/src/versioning/platform-version.md requires that changelog to record every consensus change hosted by the version. Add a numbered entry describing the TokenHistory correction, its existing previous_protocol_version < 14 && platform_version.protocol_version >= 14 boundary, and the unchanged unflagged handling of absent or unflagged contracts. This is a documentation omission, not a request for another dispatcher or version-table slot.

Comment on lines 820 to 827
self.drive.apply_contract(
&token_history_contract,
*block_info,
true,
None,
token_history_storage_flags.map(Cow::Owned),
Some(transaction),
platform_version,
)?;

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.

🟡 Suggestion: Document the TokenHistory activation ownership correction

Supplying these storage flags changes the PV14 activation outcome for historically flagged TokenHistory contracts: it preserves the optional storage owner, lets the existing merger retain historical allocations and account for growth at the activation epoch, and avoids the previous unflagged-replacement accounting failure. The numbered changelog in packages/rs-platform-version/src/version/v14.rs documents the CREDIT backfill but does not describe this separate persisted-state correction. book/src/versioning/platform-version.md requires that changelog to record every consensus change hosted by the version. Add a numbered entry describing the TokenHistory correction, its existing previous_protocol_version < 14 && platform_version.protocol_version >= 14 boundary, and the unchanged unflagged handling of absent or unflagged contracts. This is a documentation omission, not a request for another dispatcher or version-table slot.

source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, platform-versioning, rust-quality, security-auditor)

@github-actions

Copy link
Copy Markdown
Contributor

Bots are done — your move: post /self-reviewed.
Full checklist in the description.

@github-actions github-actions Bot added waiting-self-review Waiting for the author to post /self-reviewed and removed waiting-bots Waiting for the review bots to report on this head labels Oct 11, 2026
@shumkov
shumkov merged commit 81e1e77 into v5.0-dev Oct 11, 2026
51 of 52 checks passed
@shumkov
shumkov deleted the credit-pool-pv14-backfill branch October 11, 2026 04:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

waiting-self-review Waiting for the author to post /self-reviewed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants