feat(drive)!: add the contract credits root sum tree to genesis, upgrade and credit conservation - #4716
feat(drive)!: add the contract credits root sum tree to genesis, upgrade and credit conservation#4716DCG-Claude wants to merge 11 commits into
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository: dashpay/platform/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: dashpay/platform/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (38)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughProtocol version 17 adds contract-credit storage, genesis and upgrade initialization, and balance accounting. The change also updates runner-image candidate status selection and pins reusable workflow revisions. ChangesProtocol 17 Contract Credits
Runner-image candidate selection
PR review policy workflow pin
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~60 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant Drive
participant GroveDB
participant TotalCreditsBalance
Drive->>GroveDB: Read contract-credit root sum
GroveDB-->>Drive: Return aggregate excluding not-summed trees
Drive->>TotalCreditsBalance: Include aggregate in balance result
Merge Risk: ⚪ Minimal · up to Protocol 17 adds a contract-credit tree at genesis and during upgrades, and includes live contract credits in the balance check. Earlier protocol versions keep their existing behavior. A test checks that fresh genesis and an upgraded chain produce the same database root hash. No blocking issues remain, and the change appears ready to merge once CI passes. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to The credit-state migration preserves the inspected conservation, rollback and retry guarantees, and runner-status selection is tightened. No introduced security vulnerability was established. Risk remains moderate because the change affects shared financial state and the newly pinned privileged workflows could not be verified. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 58.70% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 92 functions across 31 files. (7 skipped: 7 unsupported.) ✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
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. Comment |
|
📖 Book Preview built successfully. Download the preview from the workflow artifacts. Updated at 2026-09-30T02:45:13.579Z |
|
✅ Final review complete — no blockers (commit baaef2e) · triage: critical |
|
The red 🤖 Posted autonomously by DashVM (Claude Fable 5.1) on behalf of pasta. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v5.0-dev #4716 +/- ##
============================================
- Coverage 86.36% 78.43% -7.94%
============================================
Files 2766 2770 +4
Lines 366105 403383 +37278
============================================
+ Hits 316191 316397 +206
- Misses 49914 86986 +37072
🚀 New features to boost your workflow:
|
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
The PR consistently introduces the ContractCredits root across protocol 17 genesis, upgrade, conservation accounting, and versioned behavior. The reviewed suggestions do not identify actionable defects: the root-shape concern is not demonstrated by the actual sequential insertion paths, the by-value path helper is a small fixed-size copy already covered by its test and does not affect correctness, and the broader RootTree round-trip coverage request concerns pre-existing variants rather than this PR.
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: security-auditor); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 5: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
criticalbygpt-6-astra(effort low) — This is a large, intricate diff that changes consensus-critical storage initialization and upgrade migrations plus the credit conservation equation in Drive, directly affecting funds accounting and state transition behavior. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— security-auditor (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 11% left, 5h 100% left),glm-5.3-flash(zai below 15% reserve: 5h 99% left, weekly 13% left) - Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort xhigh); agentphase2-reviewer
|
The red 🤖 Posted autonomously by DashVM (Claude Fable 5.1) on behalf of pasta. |
|
Friendly nudge: this PR has been green and bot-approved for 5 days and awaits a human review. 🤖 Posted autonomously by DashVM (Claude Fable 5.1) on behalf of pasta. |
…r the contract credits root Protocol version 17 is the provisional 5.0 version; 15 and 16 are placeholders identical to 14 so the registry, which is indexed by number, can hold 17. The new drive table selects the genesis and credit conservation generations that create and read the contract credits root sum tree. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ade and credit conservation RootTree::ContractCredits (key 100) is an ordinary sum tree created by the v5 initial state structure (v4 is the protocol 14 generation that adds the ContractGroups root tree) and, on upgraded nodes, by the first block at protocol version 17. The v3 credit conservation calculator reads its aggregate as the sixth term of the equation; the earlier generations backfill the new field with zero. The tree is described in the area's structure.rs with a fixture that builds a live and a wiped contract, and grovedb-structure.json is regenerated at protocol version 17. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… root The root Merk now holds 19 keys. ContractCredits (100) lands as the left child of Misc (104), so the Misc proof carries one more child hash and the new key proves at the same size as its level 4 siblings. The protocol 13 heights test now builds a protocol 13 genesis, as its name says, so its Versions pin is the 285 bytes of a node without the ContractGroups child. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… root key Every write under Misc hashes one more child from protocol version 17, which moves the processing fee of the identity create, top up, token burn confirmer and direct purchase pins and the solitude strategy balance by 1_480 credits at latest. The protocol 14 values are kept as frozen pins next to the moved ones. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Move the activation test's imports to the test module, import TransactionArg there, and state in the versioning chapter that a field a later version overrides explicitly (protocol 17 names its own drive table) must be reconciled by hand on a forward merge. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
14a4e35 to
d5ea5ce
Compare
|
🌳 GroveDB structure This pull request changes the described GroveDB structure. Open it in the structure viewer: new nodes glow, removed ones stay as ghosts, and the tour walks through each change. Added (3 nodes)
Changed (28 nodes)
Compared |
|
Rebased onto
Fee pins: the base moved every latest pin this PR had re-pinned (documents expirations tree under Gate run locally on the rebased head, each command redirected to a file with its exit code checked: fmt, clippy on drive, drive-abci, dpp and platform-version with all targets and warnings denied, the workspace check with all targets, the verify-only drive check, and the targeted dpp, drive, drive-abci and strategy tests listed in the description. 🤖 Posted autonomously by DashVM (Claude Fable 5.1) on behalf of pasta. |
thepastaclaw
left a comment
There was a problem hiding this comment.
⚠️ DEGRADED — Re-review — Final validation — Phase 1 + Phase 2
⚠️ DEGRADED review. The primary review models were unavailable (gpt-6.1-solunavailable: Request rejected (429) · All credentials for model gpt-6.1-sol are cooling down (last error: usage_limit_reached: The us), so this review ran on stand-in models:gpt-5.6-luna→muse-spark-1.3-contributor,gpt-5.6-sol→muse-spark-1.3-contributor,gpt-5.6-terra→muse-spark-1.3-contributor,gpt-6-astra→muse-spark-1.3-contributor,gpt-6.1-sol→muse-spark-1.3-contributor. Both review phases and the independent verifiers still ran, but on weaker models, with Phase 1 capped athigheffort. Treat the verdict as provisional; a full-strength re-review will run on the next push once the primary models are back.
Part 1 of the contract-credits work is a sound foundation: the new ContractCredits root (key 100), genesis v5, the idempotent v17 upgrade hook, and the sixth conservation term are all correctly versioned with prior generations untouched. Three in-scope suggestions remain: an inconsistent by-value parameter on a new public path helper, a stale v14 version-array example in the book chapter this PR already edited, and a root-hash assertion missing from the genesis/upgrade equivalence test despite differing insertion orders.
🟡 3 suggestion(s)
1 finding(s) not shown inline (the lines are not part of this PR's diff)
🟡 Suggestion: Version-array example still describes fourteen versions ending at v14
book/src/versioning/platform-version.md:59-101
This PR establishes protocol 17 in the committed tables (LATEST_VERSION is already PROTOCOL_VERSION_17 in code) and adds a placeholder-version note later in this same chapter, but the authoritative Version Array section above it still says the platform "has fourteen versions", shows LATEST_VERSION = PROTOCOL_VERSION_14, and lists PLATFORM_VERSIONS ending at PLATFORM_V14 with LATEST_PLATFORM_VERSION = &PLATFORM_V14. A contributor reading the chapter top-down gets a stale snapshot boundary that contradicts both the code and the new note. Update the counts and code examples to run through PLATFORM_V17 (V15/V16 as struct-update placeholders, matching the real files), or explicitly label the v14 listing as a historical example.
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, platform-versioning, rust-quality, security-auditor); muse-spark-1.3-contributor (phase2-reviewer: general, architecture-layering, platform-versioning, rust-quality, security-auditor)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: architecture-layering); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: platform-versioning); reviewer 4: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 5: muse-spark-1.3-contributor (agent: phase1-reviewer, role: security-auditor); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 11: muse-spark-1.3-contributor (standing in for gpt-6.1-sol) (agent: phase2-reviewer, role: general); reviewer 12: muse-spark-1.3-contributor (standing in for gpt-6.1-sol) (agent: phase2-reviewer, role: architecture-layering); reviewer 13: muse-spark-1.3-contributor (standing in for gpt-6.1-sol) (agent: phase2-reviewer, role: platform-versioning); reviewer 14: muse-spark-1.3-contributor (standing in for gpt-6.1-sol) (agent: phase2-reviewer, role: rust-quality); reviewer 15: muse-spark-1.3-contributor (standing in for gpt-6.1-sol) (agent: phase2-reviewer, role: security-auditor); final verifier: muse-spark-1.3-contributor (standing in for gpt-6.1-sol) (agent: sol-verifier, role: final-verifier)
- Degraded mode:
gpt-6.1-solunavailable: Request rejected (429) · All credentials for model gpt-6.1-sol are cooling down (last error: usage_limit_reached: The us (detected by lane, since 2026-09-29T23:52:49Z); stand-insgpt-5.6-luna→muse-spark-1.3-contributor,gpt-5.6-sol→muse-spark-1.3-contributor,gpt-5.6-terra→muse-spark-1.3-contributor,gpt-6-astra→muse-spark-1.3-contributor,gpt-6.1-sol→muse-spark-1.3-contributor; Phase 1 effort capped athigh - Triage:
criticalbymuse-spark-1.3-contributor(standing in forgpt-6.1-sol) (effort low) — The large cross-cutting diff changes consensus-critical storage migrations and credit conservation across genesis, protocol upgrade hooks, and balance calculations, directly affecting funds accounting in functions such as transition_to_version_17 and calculate_total_credits_balance_v3. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— architecture-layering (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— platform-versioning (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— security-auditor (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 13% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
muse-spark-1.3-contributor(standing in forgpt-6.1-sol) — final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,muse-spark-1.3-contributor(standing in forgpt-6.1-sol) — general (completed, effort xhigh); agentphase2-reviewer,muse-spark-1.3-contributor(standing in forgpt-6.1-sol) — architecture-layering (completed, effort xhigh); agentphase2-reviewer,muse-spark-1.3-contributor(standing in forgpt-6.1-sol) — platform-versioning (completed, effort xhigh); agentphase2-reviewer,muse-spark-1.3-contributor(standing in forgpt-6.1-sol) — rust-quality (completed, effort xhigh); agentphase2-reviewer,muse-spark-1.3-contributor(standing in forgpt-6.1-sol) — security-auditor (completed, effort xhigh); agentphase2-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/src/drive/contract/balances/mod.rs`:
- [SUGGESTION] packages/rs-drive/src/drive/contract/balances/mod.rs:38-41: contract_credits_path_vec takes contract_id by value, unlike its sibling and every analogous helper
The new `contract_credits_path_vec(contract_id: [u8; 32])` takes the 32-byte id by value, while its sibling `contract_credits_path(&[u8; 32])` in the same file and every analogous helper (`contract_root_path_vec(&[u8])`, `identity_contract_info_root_path_vec(&[u8; 32])`, the prefunded fee-pot paths) borrow. Callers holding a `&[u8; 32]` or `&Identifier` must copy 32 bytes to call it, and parts 2-4 will bake in callers against whichever signature ships. Borrow to match the module's own convention; the fix also touches the test at line 65 (`contract_credits_path_vec(contract_id)` becomes `contract_credits_path_vec(&contract_id)`).
In `book/src/versioning/platform-version.md`:
- [SUGGESTION] book/src/versioning/platform-version.md:59-101: Version-array example still describes fourteen versions ending at v14
This PR establishes protocol 17 in the committed tables (`LATEST_VERSION` is already `PROTOCOL_VERSION_17` in code) and adds a placeholder-version note later in this same chapter, but the authoritative Version Array section above it still says the platform "has fourteen versions", shows `LATEST_VERSION = PROTOCOL_VERSION_14`, and lists `PLATFORM_VERSIONS` ending at `PLATFORM_V14` with `LATEST_PLATFORM_VERSION = &PLATFORM_V14`. A contributor reading the chapter top-down gets a stale snapshot boundary that contradicts both the code and the new note. Update the counts and code examples to run through `PLATFORM_V17` (V15/V16 as struct-update placeholders, matching the real files), or explicitly label the v14 listing as a historical example.
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:3615-3698: Equivalence test compares the subtree but not the root hash, while genesis and upgrade insert key 100 in different orders
The v16-to-v17 equivalence test asserts the `[ContractCredits]` element and the subtree under it are identical, but never compares the whole-DB root hash. That matters because the insertion sequences genuinely differ: genesis v5 inserts key 100 right after ShieldedBalances and before SavedBlockTransactions (36) and the contract-groups structure (124), while the upgrade path inserts 100 last into a root layer that already contains both. AVL/Merk tree shape is insertion-order sensitive in general, so the same key set can yield different root hashes, and the drive/mod.rs layout comment's claim that both paths share the insertion order does not hold for keys 100/36/124. If the shapes diverge, fresh-genesis and upgraded nodes fork and the re-pinned fee values (the uniform +1,480 deltas) diverge between populations. Extend the empty-platform comparison with a root-hash equality assertion (genesis-at-17 vs genesis-at-16-plus-transition, e.g. `root_hash(None)` vs `root_hash(Some(&txn_b))`); that settles the question either way and matches the strength of the CONSENSUS FORK assertions already in the test.
Match the sibling contract_credits_path and the other path helpers, which all borrow the id. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…s at v17 Genesis inserts the contract credits root key before SavedBlockTransactions and ContractGroups while the upgrade inserts it last, and the root Merk shape depends on insertion order. After aligning the genesis epoch's recorded protocol version, the whole-database root hashes of both paths must match. Correct the root tree comment that claimed both paths share one insertion order. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
On the review note about 🤖 Posted autonomously by DashVM (Claude) on behalf of pasta. |
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
At head 62618c7, the complete diff correctly gates the contract-credits root and conservation term behind protocol 17 while preserving historical behavior; no blocking defect was confirmed. Independent validation passed 129 targeted tests, the verify-only Drive check, and git diff --check. All three prior findings are fixed; three non-blocking documentation and version-table convention suggestions remain.
🟡 3 suggestion(s)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: architecture-layering); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: platform-versioning); reviewer 4: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 5: muse-spark-1.3-contributor (agent: phase1-reviewer, role: security-auditor); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 11: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 12: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 13: gpt-6-astra (agent: phase2-reviewer, role: platform-versioning); reviewer 14: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 15: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); reviewer 16: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 17: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 18: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 19: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 20: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — This intricate cross-crate change modifies consensus-visible storage initialization and migration in create_initial_state_structure and transition_to_version_17, and changes the end-of-block credit conservation equation in calculate_total_credits_balance_v3 and TotalCreditsBalance::ok(). - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— architecture-layering (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— platform-versioning (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— security-auditor (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 13% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-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-platform-version/src/version/v15.rs`:
- [SUGGESTION] packages/rs-platform-version/src/version/v15.rs:15-17: Account for v17's explicit Drive override in forward-merge guidance
The statement that every incoming table change flows into v17 without another edit is incorrect for Drive. PLATFORM_V17 explicitly replaces the inherited drive field with DRIVE_VERSION_V10, so Drive changes arriving in the real v15 or v16 table will not propagate into v17 automatically. The book already documents this exception at book/src/versioning/platform-version.md:121–126; mirror that qualification here so a maintainer following this rustdoc does not omit incoming Drive behavior during a forward merge.
In `packages/rs-platform-version/src/version/drive_versions/v10.rs`:
- [SUGGESTION] packages/rs-platform-version/src/version/drive_versions/v10.rs:42-46: DRIVE_VERSION_V10 should use a struct-update diff
DRIVE_VERSION_V10 copies the entire V9 table even though only initialization.create_initial_state_structure and balances.calculate_total_credits_balance differ. book/src/versioning/feature-versions.md:518 explicitly directs new table constants to use struct-update syntax. Define this table with nested updates and ..DRIVE_VERSION_V9 so the two intended behavior changes are visible without auditing the copied, unchanged slots. This is a maintainability suggestion, not a current consensus defect; it also does not eliminate the separate need to reconcile Drive changes from real v15/v16 tables during forward merges.
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:122-124: Protocol-upgrade ladder edit lacks the required in-place-generation justification in the PR description
The new rung is a permitted protocol-upgrade-ladder edit: its current-version guard makes it unreachable before protocol 17, and the tests confirm transactional retry and historical behavior. However, book/src/contributing/coding-conventions.md:109–114 requires an "In-place changes to shipped generations" section in the PR description, and the supplied description has no such section. Add it to identify the edited v0 hook, the protocol versions that reach it directly or through the v1/v2 wrappers, and why the new guard preserves historical execution. Include the output-preserving zero backfills in the shipped v0–v2 balance calculators in the same inventory. This documents the replay-safety proof; it does not require replacing the guarded migration with another hook generation.
…rsion 9 Only the genesis structure and the total credits calculator differ from DRIVE_VERSION_V9, so name those two slots and inherit the rest, as the feature versions chapter asks for new table constants. Note on PLATFORM_V17 that its explicit drive table does not follow a real v15 or v16 on a forward merge. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Independently reviewed the complete diff at baaef2e and found no remaining in-scope defects. Protocol 17 selects the new genesis and conservation generations, while the guarded upgrade and zero-valued historical backfills preserve earlier behavior; all six prior findings are addressed. Local validation passed 129 targeted tests, the verify-only Drive build, formatting, and diff checks.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: architecture-layering); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: platform-versioning); reviewer 4: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 5: muse-spark-1.3-contributor (agent: phase1-reviewer, role: security-auditor); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 13: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 14: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 15: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — This intricate cross-package change directly alters consensus-critical credit conservation in Drive::calculate_total_credits_balance_v3 and TotalCreditsBalance, and introduces a persistent root-tree migration in Platform::transition_to_version_17 that must match genesis state. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— architecture-layering (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— platform-versioning (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— security-auditor (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 13% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.
No unresolved findings remain from the prior review on this head.
|
@coderabbitai review No review for |
2 similar comments
|
@coderabbitai review No review for |
|
@coderabbitai review No review for |
Issue being fixed or feature implemented
Part 1 of 4 for R04-01 of the smart contract plan in #4626 (workstream issue #4690, section 4: contract credit and token buckets).
Ordinary data contracts are not identities and stay that way. When a contract needs to hold credits (to pay for scheduled work, to escrow a purchase, to fund its own storage) those credits must live in a structure that is separate from every identity tree, must be provable per bucket and per contract, and must take part in the end-of-block credit conservation check so a contract can never hold credits that the equation does not see.
This first part lays the foundation: the root tree that will hold every contract's credit buckets, its creation at genesis and on upgrade, and its term in the conservation equation. Bucket identifiers and rules, the bucket storage operations, the proofs and the matching token holdings follow in the next three parts.
What was done?
Protocol versions 15 to 17 (
packages/rs-platform-version/src/version/{v15,v16,v17}.rs,mod.rs,protocol_version.rs). The registry is indexed by number, so the 5.0 version cannot exist without 15 and 16. Those two are placeholders written as struct updates over their predecessor, byte-identical to the files the other 5.0 branches carry, so a forward merge resolves by taking either side.PLATFORM_V17selects the new drive table and nothing else.Drive table
DRIVE_VERSION_V10(packages/rs-platform-version/src/version/drive_versions/v10.rs): a struct update over V9 that names onlyinitialization.create_initial_state_structure: 5andbalances.calculate_total_credits_balance: 3. No struct gains a field, so the shipped tables and the mocks are untouched.Root tree
RootTree::ContractCredits = 100(packages/rs-drive/src/drive/mod.rs), an ordinarySumTree, added toDisplay,TryFrom<u8>and the byte conversion, plusKnownPath::ContractCreditsRootin the batch debug printer (packages/rs-drive/src/util/batch/grovedb_op_batch/mod.rs), where a 32-byte key under it prints as a contract id. The layout comment now shows the key as the left child ofMisc, which is where AVL rebalancing places it on both the genesis and the upgrade path.Path helpers in the new
packages/rs-drive/src/drive/contract/balances/mod.rs(built for both theserverandverifyfeatures):contract_credits_root_path()andcontract_credits_path(contract_id)with their_vecforms.Genesis
Drive::create_initial_state_structure_v4(packages/rs-drive/src/drive/initialization/v4/mod.rs): a copy of v3 that inserts the empty sum tree as a standalone root insert right afterShieldedBalances, before the lower-layer batch. v0 to v3 are byte-identical to before.Upgrade
Platform::transition_to_version_17in the protocol change hook (packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs): an insert-if-not-exists of the sameElement::empty_sum_tree(), guarded byprevious < 17 && current >= 17like every earlier transition in that file. Idempotent, so a validator that ran the hook inside a rejected proposal and runs it again in the next round produces the same state.Credit conservation
Drive::calculate_total_credits_balance_v3(packages/rs-drive/src/drive/balances/calculate_total_credits_balance/v3/mod.rs) reads the root aggregate as the sixth term.TotalCreditsBalanceinpackages/rs-dpp/src/balances/total_credits_balance/mod.rsgainstotal_in_contract_credits, checked for sign, included inok(),total_in_trees()andDisplay. v0 to v2 receive only the struct-literal backfilltotal_in_contract_credits: 0, the same edit the shielded term made.Book: a new chapter
book/src/drive/contract-credit-buckets.md(layout, why a separate ordinary sum tree, genesis and upgrade, the live and wiped lifecycle the layout is designed for, the conservation term, what is not here yet) and the placeholder-version note inbook/src/versioning/platform-version.mdthat the other 5.0 branches also carry.Fee pins. The new root key is the left child of
Misc, so every write underMisc(system credits on identity create and top up, token total supply on mint and burn) hashes one more child. Six latest-version pins indrive-abcimove by 1 480 credits; the protocol 14 values are kept as frozen pins next to them where the test already had a per-version runner, and the moved literals carry the pre-17 value in a comment:identity_createvalidation processing feeidentity_createasset lock reuse and replay processing feeidentity_top_upvalidation processing feerun_chain_one_identity_in_solitude_latest_protocol_versionbalanceThe drive-level genesis shape test moved from 17 to 18 root keys, the
Miscproof grew by one child hash (285 to 319 bytes) and the new key proves at 285 bytes like its level 4 siblings. Pins for shipped versions are unchanged because their genesis has no new key.Review finding carried to the next part. Plan review left one finding open: how repeated updates of the same bucket inside one operation batch observe each other's pending writes (the batch converter reads every operation from the pre-batch snapshot and GroveDB keeps the last write to a key). The disposition in the plan is that every bucket write takes the pending-operations vector, takes over a pending replacement of the same sum item, and emits exactly one replacement per bucket per batch, with tests for two debits from one stored amount. That lands with the bucket storage operations in part 2; nothing in this part writes a bucket.
How Has This Been Tested?
New tests:
drive,initialization/v4:should_create_contract_credits_root_at_latest_genesis(the root element is an empty sum tree without flags) andshould_not_create_contract_credits_root_at_protocol_14_genesis.drive,calculate_total_credits_balance/v3:should_balance_credits_with_empty_contract_credits_root,should_read_contract_credits_as_a_term_of_the_equation(two raw contract subtrees unbalance the equation until the matching system credits are added) andshould_exclude_a_not_summed_contract_tree_from_the_term(aNotSummedsubtree with a retained bucket contributes nothing).dpp,total_credits_balance: the new term counts, a negative value is rejected, an overflowing value reports overflow.drive,contract/balances: the path helpers.drive-abci, protocol change hook:test_genesis_v17_and_upgrade_to_v17_build_identical_contract_credits_tree(fresh genesis at 17 against genesis at 16 plus the real transition, compared withcollect_subtree_diffs),should_pass_credit_conservation_after_upgrade_to_v17(the v17 and the frozen v16 calculator both hold after the upgrade) andshould_activate_the_contract_credits_root_through_the_protocol_change_hook(populated v16 state with a funded identity and a document, the public dispatcher run inside a dropped transaction, then again and committed, then a third idempotent run, plus a same-version negative control).Commands run locally, each redirected to a file with the exit code checked:
In-place changes to shipped generations
perform_events_on_first_block_of_protocol_change_v0(drive-abci) gains the rungprevious_protocol_version < 17 && platform_version.protocol_version >= 17, which callstransition_to_version_17. Protocol versions 4 to 12 select v0 directly, 13 reaches it through the v1 wrapper and 14 to 17 through the v2 wrapper. For every version below 17 the guard is false, so the shipped ladder runs exactly as before; the rung only executes on the first block at 17. The activation test covers a dropped transaction, a committed retry, an idempotent rerun and a negative control below 17.calculate_total_credits_balancev0, v1 and v2 (drive, selected by protocol versions 1 to 16) set the newTotalCreditsBalance::total_in_contract_creditsfield to 0. A zero term leavesok()andtotal_in_trees()unchanged and passes the non-negative check, so neither the conservation verdict nor the daily withdrawal limit input can change.TotalCreditsBalanceis not serialized (it derives onlyCopy,CloneandDebug); the one other effect is an extra line in itsDisplayandDebugoutput, which only appears in the node-local error for unbalanced credits.Breaking Changes
Consensus-breaking from protocol version 17 only: a new root key changes the state root of every node at the first block of that version, and the credit conservation equation gains a term. Nothing changes for protocol versions 1 to 16; their genesis builds no new key, their calculators do not read it, and their fee pins are unchanged.
Decisions taken (provisional values)
100forRootTree::ContractCredits. The allocation register leaves new root values unallocated; 100 is free in the enum, the byte conversions and the batch debug printer, and sits next to the other balance trees. Revised, if at all, before any network is asked to propose version 17.DRIVE_VERSION_V10for protocol 17. Another open 5.0 branch also claims V10 (for protocol 15); whichever lands first keeps the number and the other is renumbered to the next free generation at rebase, with no table content change.SumTreefor the root and for each contract subtree, per the engineering recommendation on the task. The whole-contract total is the parent element's stored sum and a single bucket is aSumItem; range sums across buckets stay with the specialized provable-sum collections.Element::NotSummedaround a contract's subtree, chosen because GroveDB reports such an element's contribution as zero while its buckets keep their values for cleanup. This part only proves the exclusion in a test; the write that wraps a populated tree and moves its live total belongs to the wipe work of a later task, and the equation here is what that write must preserve.Checklist:
For repository code-owners and collaborators only
Refs #4690
🤖 Generated with Claude Code
Automated reviewer consensus (Fable 5.1 implementer, GPT-6 Astra reviewer)
Reviewer consensus
Plan Review consensus
still_openresolvedresolvedresolvedReview consensus
notednotednotedSummary by CodeRabbit