fix(drive): credits-not-balanced from shielded nullifier metadata#3624
Merged
Conversation
Fixes #3412: the off-by-N "credits are not balanced after block execution" error during shielded operations was caused by per-block recent-nullifier counts being sum-aggregated as if they were credits. AddressBalances is a SumTree, the shielded credit pool lived under it as a child SumTree, and recent nullifiers under the pool were stored as ItemWithSumItem(serialized, nullifiers.len()) inside a CountSumTree. That nullifier count bubbled up through both parent SumTrees into the AddressBalances aggregate that the corruption check reads. Two complementary fixes, defense in depth: 1. Move the shielded pool out from under AddressBalances into a new top-level RootTree::ShieldedBalances = 52 SumTree, with the main pool parented inside under MAIN_SHIELDED_CREDIT_POOL_KEY = b"M" (renamed from the previous SHIELDED_CREDIT_POOL_KEY = b"s"). TotalCreditsBalance gains total_in_shielded_balances; ok() and total_in_trees() include it. calculate_total_credits_balance_v1 reads the new root. 2. Wrap the recent-nullifiers CountSumTree in Element::NotSummed so its sum side contributes 0 to the enclosing shielded pool SumTree. Compaction trigger code reads through Element::underlying() to peel the wrapper. Bumps the grovedb pin to dbd83dce (the merge commit of dashpay/grovedb#659 that introduced Element::NotSummed). The grovedb bump pulled in orchard 0.13, which changed Action::from_parts to return Option<Self> (rejects identity-randomizer-key actions) — adapted by failing such actions with InvalidShieldedProofError. Tests: cargo test -p drive --lib (3079 passed), cargo test -p drive-abci --lib shielded (126 passed). The latest-protocol AVL characterization test needed one proof-length value updated (286 → 320) for the branch that gained the new root; the other 15 proof lengths in the same test are unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (28)
✨ Finishing Touches🧪 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 |
Collaborator
|
🕓 Ready for review — 4 ahead in queue (commit 944db6a) |
The v12 protocol transition was still parenting the main shielded credit pool under AddressBalances and creating only 4 of the 8 inner subtrees, which would diverge from a fresh genesis-12 chain (created by v3 init) on any node that actually migrated from v11. Rewrite it to mirror v3 init: - Insert RootTree::ShieldedBalances as a top-level SumTree. - Parent the main pool under [ShieldedBalances] / "M". - Insert all 8 inner subtrees in BFS order: notes, nullifiers, anchors_in_pool, total_balance, anchors_by_height, recent_nullifiers (wrapped in NotSummed), compacted_nullifiers, expiration_time. - Update the two post-condition assertions in the in-file tests. Also fix CI: the grovedb bump (PR #659/#656) added a new QueryItem::AggregateCountOnRange variant. wasm-drive-verify wasn't in my local check set and had a non-exhaustive match in token_transition.rs that broke the macOS workspace build. Reject the variant with a clear error message. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ng v1 Adding the ShieldedBalances read to v1 in 2189a91 retroactively changed the meaning of the v1 calculator that DRIVE_VERSION_V6 (protocol v11) is still pinned to. Pre-v12 chains have no ShieldedBalances tree, so reading it there would fail at the corruption check on every block. Revert v1 to its original four-term shape (it now sets total_in_shielded_balances: 0 to keep the struct in sync) and introduce v2 with the five-term equation that includes the ShieldedBalances root. Bump DRIVE_VERSION_V7 (protocol v12, where the shielded pool first exists) to use calculator v2. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The grovedb bump added two new Element variants (NonCounted, NotSummed) that wrap an inner element. The path_elements helper had two large inline matches over Element that became non-exhaustive on macOS CI. Extract those matches into format_element_data and format_element_type helpers and add recursive arms for both wrappers — rendered as non_counted(<inner>) / not_summed(<inner>) so the wrapped element's value and type are still visible to FFI callers. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… values Adding the ShieldedBalances root tree changes the AVL shape of the root NormalTree (17 entries instead of 16), so every operation that touches a top-level root key now navigates a slightly different sibling layout. Processing fees on the v12 path-of-tests dropped by ~49,820 credits each (about 1.6%), with storage fees unchanged. Updated the asserted processing_fee and total_fee values (and their "Was X, now Y" message strings) in each fee_regression test. The _on_version_11 variants are unaffected because protocol v11 still pins DRIVE_VERSION_V6, which predates ShieldedBalances. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…gram
ShieldedBalances (52) is less than AddressBalances (56), so by BST order
it belongs on the left side of AddressBalances — same shape as
Saved Block Transactions (36) hanging left off PreFundedSpecializedBalances
(40). Both are now drawn as left descendants ('/'), not as right
descendants of unrelated parents.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
✅ DashSDKFFI.xcframework built for this PR.
SwiftPM (host the zip at a stable URL, then use): .binaryTarget(
name: "DashSDKFFI",
url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
checksum: "edf436b547f967681c1779fd4ab9c5bc54987eb277ce8d6679c12cbb7abddf69"
)Xcode manual integration:
|
Member
Author
|
Me and Lukazs went over this one. |
QuantumExplorer
added a commit
that referenced
this pull request
May 11, 2026
Conflicts and resolutions: - Cargo.toml across rs-dpp / rs-drive / rs-drive-abci / rs-platform- version / rs-platform-wallet / rs-sdk: grovedb rev bumped on v3.1-dev from 1206049b to dbd83dce. Took the incoming rev — it's the direction the base is going and the count-tree primitives our PR depends on are still present. - `packages/rs-platform-version/src/version/drive_versions/v7.rs` auto-merged: incoming `calculate_total_credits_balance: 2` (PR #3624 shielded-nullifier-metadata fix) sits alongside our `contract: DRIVE_CONTRACT_METHOD_VERSIONS_V3` (count-tree-aware contract-insertion cost estimation). - `rs-drive-abci/.../shielded_common/mod.rs`: took the incoming error message for `Action::from_parts` returning None — the origin version names the actual cause ("action has identity randomizer key") rather than the generic "invalid action parts" we had. Same error type and shape, more informative payload. - `rs-sdk-ffi/.../path_elements.rs`: took the incoming refactor that lifts the inline `Element` rendering into `format_element_data` / `format_element_type` helpers (and adds `Element::NotSummed` handling we didn't have). - `wasm-drive-verify/.../token_transition.rs`: took the incoming strict rejection of `AggregateCountOnRange` in the token- transition path-query shim. That path doesn't drive aggregate- count queries, so erroring is safer than the descriptive fallthrough we had. - `Cargo.lock`: regenerated via `cargo generate-lockfile` after the Cargo.toml resolutions. Verified: - `cargo check -p drive -p drive-abci -p drive-proof-verifier -p dash-sdk` — clean. - 26 `range_countable_index_e2e_tests` pass (no-merge compound semantic + StartsWith + estimation + base range path). - 7 `drive-abci::query::document_count_query` tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
Fixes #3412. During shielded operations on testnet, Drive ABCI halted block execution with
corrupted credits not balanced error: ... off by 2. The mismatch was exactly the count of nullifiers spent in the block, not a fee-rounding artifact.Root cause:
RootTree::AddressBalancesis aSumTree. The shielded credit pool lived under it as a childSumTree. Recent-nullifier metadata was stored under that pool in aCountSumTree, and each per-block entry was anItemWithSumItem(serialized_nullifiers, nullifiers.len() as i64). GroveDB sum-tree aggregation bubbled that per-block count up through the pool SumTree into theAddressBalancesaggregate, which the end-of-block corruption check reads as credits. Result: each spent nullifier inflated the apparent address-credit total by 1.What was done?
Two complementary fixes, defense in depth — either alone would fix the bug; together they enforce the separation structurally and locally.
1. New top-level
RootTree::ShieldedBalances = 52SumTree.The shielded pool moves out from under
AddressBalances. The main pool lives at[ShieldedBalances, b\"M\"](renamed from[AddressBalances, b\"s\"]). The new slot sits betweenPools (48)andAddressBalances (56)for semantic clustering with other credit-bearing roots and preserves the AVL diagram.TotalCreditsBalancegainstotal_in_shielded_balances;ok()enforces>= 0and includes it in the equality,total_in_trees()includes it.calculate_total_credits_balance_v1reads the new root (v0 zeroes the field for legacy paths).2.
Element::NotSummedwrap on the recent-nullifiersCountSumTree.Even with separation, the count sum inside the pool would still pollute the pool's "total shielded credits" aggregate. The recent-nullifiers subtree is now wrapped in
Element::new_not_summed(...)so its sum contributes0to the enclosing pool SumTree while keeping its own internal sum semantics for compaction. The compaction trigger reads throughElement::underlying()to peel the wrapper before pattern-matchingCountSumTree(_, count, sum, _).Constant rename:
SHIELDED_CREDIT_POOL_KEY*→MAIN_SHIELDED_CREDIT_POOL_KEY*, with the inner byte changed fromb\"s\"tob\"M\".Grovedb bump:
8f25b20→dbd83dce(the merge commit of dashpay/grovedb#659 that introducedElement::NotSummed). Side-effect of the bump: orchard 0.12 → 0.13 changedAction::from_partsto returnOption<Self>, rejecting identity-randomizer-key actions. Adapted inshielded_common/mod.rsto fail such actions withInvalidShieldedProofErrorrather than silently dropping them.End-of-block flow already routes through the updated code: process_block_fees_and_validate_sum_trees/v0/mod.rs:175 →
calculate_total_credits_balance(dispatched to_v1on drive_version 6+) →TotalCreditsBalance::ok()(now folds in the new field).How Has This Been Tested?
cargo test -p drive --lib— 3079 passed, 0 failedcargo test -p drive-abci --lib shielded— 126 passed, 0 failedcargo test -p drive --lib initialization— all 8 pass; root tree element count assertion bumped 16 → 17, and one proof-length assertion shifted from 286 → 320 for the AVL branch that gained the new root (the other 15 proof lengths in the same characterization test are unchanged).cargo fmt --allclean.Breaking Changes
State-structure change: pre-release on
v3.1-dev. Existing dev DBs need to rebuild from genesis (the v3 init layout changed). No released network is affected, so the title isfix:rather thanfix!:.Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code