Skip to content

fix(drive-abci): record unshield and shield-surplus credits in recent address balance changes#4142

Draft
QuantumExplorer wants to merge 4 commits into
v4.1-devfrom
fix/unshield-recent-address-balance-changes
Draft

fix(drive-abci): record unshield and shield-surplus credits in recent address balance changes#4142
QuantumExplorer wants to merge 4 commits into
v4.1-devfrom
fix/unshield-recent-address-balance-changes

Conversation

@QuantumExplorer

@QuantumExplorer QuantumExplorer commented Jul 17, 2026

Copy link
Copy Markdown
Member

Symptom (dashwallet-ios QA)

Platform payments received via Unshield (Type 17) never appear through incremental address sync — the wallet's GetRecentAddressBalanceChanges passes returned 0 entries across 18 consecutive queries with last_known_recent_block stuck at 0, while the same payments showed up immediately after a full trunk/branch rescan (app relaunch / sync-state clear). Receivers see the money "sometimes" — i.e. only after the next full scan.

Root cause

UnshieldAction credits the transparent platform output address by pushing a raw AddressFundsOperationType::AddBalanceToAddress drive op (action_convert_to_operations/shielded/unshield_transition.rs), which mutates the absolute address-funds tree — what full scans read. But its execution event PaidFromShieldedPool carried no output data, and the executor arm never wrote the credit into address_balances_in_update — the sole feed for store_address_balances_for_block, i.e. the recent-address-balance-changes tree (SavedBlockTransactions/'m') that incremental sync queries. The recent tree was genuinely empty for unshield-credited blocks.

PaidFromAssetLockToPool had the identical omission for a Type 18 ShieldFromAssetLock surplus_output credit.

All transparent-side transitions (AddressFundsTransfer, IdentityCreditTransferToAddresses, fundings, withdrawals, identity ops) already record correctly via PaidFromAddressInputs / Paid { added_to_balance_outputs }. The client-side apply in rs-sdk was verified correct — it would have applied entries had any existed.

Fix

  • Extend ExecutionEvent::PaidFromShieldedPool and ::PaidFromAssetLockToPool with added_to_balance_outputs: Option<BTreeMap<PlatformAddress, Credits>> (same shape as Paid's field).
    • Unshield populates output_address → amount − fee (only when net > 0, mirroring the converter's guard).
    • ShieldFromAssetLock populates the surplus output when set and positive.
    • Type 16 ShieldedTransfer / Type 19 ShieldedWithdrawal pass None — neither credits a transparent platform address (Type 19's output is a Core-chain address).
  • Both executor arms fold the outputs into address_balances_in_update as CreditOperation::AddToCredits on the applied path, using the identical entry/merge idiom as the Paid arm (including the chargeable-failure fallback, whose address is genuinely credited by the applied ops).

Rolling-upgrade safety (protocol v13)

The recording changes what gets written under RootTree::SavedBlockTransactions per block — a state-root change — so the recording method itself is versioned, per the platform_events idiom:

  • record_added_balance_outputs/{mod.rs, v0/, v1/} is its own versioned Platform method, dispatched on a new record_added_balance_outputs: FeatureVersion field (0 in V1..V8, 1 in the new DRIVE_ABCI_METHOD_VERSIONS_V9, which PLATFORM_V13 adopts as its only delta from v12).
  • Call sites pass an AddedBalanceOutputsOrigin discriminator: v0 folds Transparent-origin outputs (the Paid arm's long-standing behavior) and drops ShieldedSpend-origin outputs — so pre-v13 blocks byte-match old behavior; v1 folds both.
  • Event construction is unconditional — events always carry their credits (in-memory only); the version decision lives where the state write happens. Paid-invalid tracking and the PaidConsensusError carry stay unconditional too.
  • store_address_balances_to_recent_block_storage is untouched at Some(0) — its _v0 implementation did not change, so its version does not move; its dispatch file is byte-identical to base.
  • Tests pin the boundary at the recording layer (v0 drops shielded / keeps transparent, v1 records both — driven through get(12)/get(13) dispatch), construction'''s version-independence at both versions, and the version-struct delta itself (field 0 at v12, 1 at v13; store method Some(0) at both).

Tests

  • 4 construction tests (net output populated, net-zero → none, surplus populated, no-surplus → none)
  • 3 executor tests proving address_balances_in_update receives AddToCredits for both variants and stays empty without outputs
  • cargo check -p drive-abci / -p drive clean; touched-module suites green (17 passed drive-abci filter, 43 passed drive saved_block_transactions/converters)

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Added support for recording transparent address credit outputs in paid execution updates, including unshield NET credits and asset-lock surplus credits (enabled starting protocol version 13).
  • Bug Fixes

    • Fixed incremental balance synchronization to include transparent credits from shielded-pool and asset-lock flows, including paid-invalid chargeable-failure (consensus error) paths.
    • Improved paid-consensus error responses to carry through transparent balance-change details when applicable.
  • Tests

    • Added/updated regression and unit tests for credit recording/omission, including NET-zero and pre/post-protocol gating cases.

@github-actions github-actions Bot added this to the v4.1.0 milestone Jul 17, 2026
@coderabbitai

coderabbitai Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Changes

Transparent credit output propagation

Layer / File(s) Summary
Event fields and output construction
packages/rs-platform-version/src/version/feature_initial_protocol_versions.rs, packages/rs-drive-abci/src/execution/types/execution_event/mod.rs
Payment events now carry optional transparent credit maps, gated from protocol version 13 and populated from positive unshield NET amounts or asset-lock surplus outputs.
Execution balance folding
packages/rs-drive-abci/src/execution/platform_events/state_transition_processing/execute_event/v0/mod.rs
Paid execution paths merge transparent credits into address balance updates using shared saturating credit logic.
Paid error balance propagation
packages/rs-drive-abci/src/execution/platform_events/state_transition_processing/process_raw_state_transitions/v0/mod.rs
Invalid and unsuccessful paid paths include tracked address balance changes in PaidConsensusError.
Consensus error aggregation
packages/rs-drive-abci/src/platform_types/state_transitions_processing_result/mod.rs
PaidConsensusError balance changes are merged into aggregate updated balances.
Compatibility updates
packages/rs-drive-abci/src/execution/platform_events/state_transition_processing/validate_fees_of_event/v0/mod.rs, packages/rs-drive-abci/src/abci/app/execution_result.rs, packages/rs-drive-abci/src/execution/validation/.../creation.rs
Event and result construction sites accept the added fields while preserving existing fee validation and ABCI response mapping.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant StateTransition
  participant ExecutionEvent
  participant execute_event_v0
  participant PaidConsensusError
  participant ProcessingResult
  StateTransition->>ExecutionEvent: construct optional credit outputs
  ExecutionEvent->>execute_event_v0: execute paid event
  execute_event_v0->>PaidConsensusError: return address balance changes
  PaidConsensusError->>ProcessingResult: aggregate balance changes
Loading

Possibly related PRs

  • dashpay/platform#3800: Provides the corrected fee and net-credit computation used by this credit propagation.

Suggested reviewers: thepastaclaw

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
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 summarizes the main change: recording unshield and shield-surplus credits in recent address balance changes.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/unshield-recent-address-balance-changes

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.

@thepastaclaw

thepastaclaw commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator

🔍 Review in progress — actively reviewing now (commit 02e9f01)

@codecov

codecov Bot commented Jul 17, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.28008% with 29 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.45%. Comparing base (fc05d62) to head (02e9f01).

Files with missing lines Patch % Lines
...ve-abci/src/execution/types/execution_event/mod.rs 95.66% 12 Missing ⚠️
...tate_transition_processing/execute_event/v0/mod.rs 90.98% 11 Missing ⚠️
...processing/process_raw_state_transitions/v0/mod.rs 92.53% 5 Missing ⚠️
...m_types/state_transitions_processing_result/mod.rs 96.96% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##           v4.1-dev    #4142      +/-   ##
============================================
+ Coverage     87.43%   87.45%   +0.01%     
============================================
  Files          2646     2646              
  Lines        333981   334461     +480     
============================================
+ Hits         292023   292488     +465     
- Misses        41958    41973      +15     
Components Coverage Δ
dpp 88.44% <ø> (ø)
drive 86.14% <ø> (+<0.01%) ⬆️
drive-abci 89.55% <94.28%> (+0.03%) ⬆️
sdk ∅ <ø> (∅)
dapi-client ∅ <ø> (∅)
platform-version ∅ <ø> (∅)
platform-value 92.88% <ø> (ø)
platform-wallet ∅ <ø> (∅)
drive-proof-verifier 49.55% <ø> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
packages/rs-drive-abci/src/execution/platform_events/state_transition_processing/execute_event/v0/mod.rs (1)

596-621: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract the repeated address-credit merge into a helper.

These blocks duplicate each other and the Paid arm’s Set/Add semantics. Centralizing this logic will prevent event paths from silently diverging when merge behavior changes.

Also applies to: 674-699

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@packages/rs-drive-abci/src/execution/platform_events/state_transition_processing/execute_event/v0/mod.rs`
around lines 596 - 621, Extract the address-credit merge logic from the shown
block and the corresponding block around the Paid arm into a shared helper,
using the existing balance-update map and address/credits inputs. Preserve the
current SetCredits + AddToCredits and AddToCredits + AddToCredits saturating-add
semantics, then replace both duplicated loops with calls to the helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/rs-drive-abci/src/execution/types/execution_event/mod.rs`:
- Around line 95-102: Update the documentation for added_to_balance_outputs to
include the identity-create duplicate-key fallback surfaced as an
UnshieldAction, which may credit a transparent address. Keep the existing
Unshield net-positive amount behavior accurate while revising the statement that
all IdentityCreateFromShieldedPool paths produce None.

---

Nitpick comments:
In
`@packages/rs-drive-abci/src/execution/platform_events/state_transition_processing/execute_event/v0/mod.rs`:
- Around line 596-621: Extract the address-credit merge logic from the shown
block and the corresponding block around the Paid arm into a shared helper,
using the existing balance-update map and address/credits inputs. Preserve the
current SetCredits + AddToCredits and AddToCredits + AddToCredits saturating-add
semantics, then replace both duplicated loops with calls to the helper.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 632c9b40-1bdf-47cc-a529-2253dcace728

📥 Commits

Reviewing files that changed from the base of the PR and between b889e05 and 5c335bc.

📒 Files selected for processing (3)
  • packages/rs-drive-abci/src/execution/platform_events/state_transition_processing/execute_event/v0/mod.rs
  • packages/rs-drive-abci/src/execution/platform_events/state_transition_processing/validate_fees_of_event/v0/mod.rs
  • packages/rs-drive-abci/src/execution/types/execution_event/mod.rs

Comment thread packages/rs-drive-abci/src/execution/types/execution_event/mod.rs

@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.

Preliminary review — Codex only

Fully revalidated the cumulative PR range b889e05..87dab29 and latest-push delta 5c335bc..87dab29. Normal Unshield and ShieldFromAssetLock-surplus credits are recorded, but the IdentityCreateFromShieldedPool duplicate-key fallback is an applied, chargeable invalid transition whose caller passes no balance-change map; PaidConsensusError cannot carry the discarded credit and aggregation only consumes SuccessfulExecution payloads. The claimed fallback regression calls execute_event with None and asserts only the paid result and pool debit, while the new map test supplies Some(&mut map) directly, so neither test covers the failing process_raw_state_transitions-to-recent-storage path; local focused test attempts were blocked by the Tenderdash source download requiring unavailable network access.

Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.

Review provenance

  • Codex reviewers: gpt-5.6-sol — general (failed), gpt-5.6-sol — rust-quality (failed), gpt-5.6-sol — general (failed), gpt-5.6-sol — rust-quality (failed), gpt-5.6-sol — general (failed), gpt-5.6-sol — general (failed), gpt-5.6-sol — general (completed), gpt-5.6-sol — rust-quality (completed)
  • Verifier: gpt-5.6-sol — verifier
  • Sonnet: not run (deferred by blocker gate)

🔴 1 blocking

1 additional finding(s) omitted (not in diff).

🤖 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/state_transition_processing/process_raw_state_transitions/v0/mod.rs`:
- [BLOCKING] packages/rs-drive-abci/src/execution/platform_events/state_transition_processing/process_raw_state_transitions/v0/mod.rs:278-284: Propagate fallback address credits from paid-invalid transitions
  The duplicate-key IdentityCreateFromShieldedPool fallback returns an Unshield action with consensus errors and chargeable_failure=true. execute_event applies that event's operations, including the transparent fallback-address credit, and calls record_added_balance_outputs, but this invalid-transition branch passes None for address_balances_in_update, so the credit is discarded. The resulting PaidConsensusError contains only the error and fees, and StateTransitionsProcessingResult::add merges address changes only from SuccessfulExecution; no later path reconstructs the applied credit before address_balances_updated is stored as the block's recent address-balance changes. Incremental sync therefore omits a GroveDB balance credit that the block actually applied. Preserve the balance-change map for applied paid-invalid events, carry it through PaidConsensusError, aggregate it into address_balances_updated, and add a regression that processes the duplicate-key fallback through process_raw_state_transitions and asserts the recent-balance accumulator or stored entry.

@QuantumExplorer

Copy link
Copy Markdown
Member Author

Blocker addressed in the latest push:

  • The invalid-transition branch of process_raw_state_transitions now tracks address balances (the executor records into the map only on the applied path, so ordinary bump-only paid-invalid transitions stay empty).
  • StateTransitionExecutionResult::PaidConsensusError gained address_balance_changes (same shape as SuccessfulExecution's), populated at both construction sites — including the valid-branch UnsuccessfulPaidExecution arm, which had the identical latent omission — and StateTransitionsProcessingResult::add() merges it into address_balances_updated.
  • New tests pin the chain at each seam: the real duplicate-key fallback UnshieldAction produces the credited PaidFromShieldedPool event (construction test), the invalid branch of process_validation_result_v0 carries the credit out in PaidConsensusError, and add() merges it into address_balances_updated. A full process_raw_state_transitions fixture was judged disproportionate: the fallback only arises after Halo 2 proof verification succeeds on a duplicate-key create, and no existing fixture produces a valid-proof serialized ST for it — the seam tests jointly cover the exact links the finding identified as broken.

🤖 Addressed by Claude Code

@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 — Codex + Sonnet

At exact head 23675f4, no in-scope defects remain. The final commit fixes the sole prior blocker by threading a mutable address-balance map through the invalid branch of process_raw_state_transitions, carrying it via a new address_balance_changes field on PaidConsensusError, and merging it into address_balances_updated exactly like SuccessfulExecution. I independently re-read the diff and reran both regression tests (invalid_branch_chargeable_failure_carries_address_credit and add_merges_paid_consensus_error_address_changes) against the working tree at this SHA — both pass, corroborating the Codex and Sonnet lanes' independent findings.

Review provenance

  • Codex reviewers: gpt-5.6-sol — general (failed), gpt-5.6-sol — security-auditor (failed), gpt-5.6-sol — rust-quality (failed), gpt-5.6-sol — general (failed), gpt-5.6-sol — security-auditor (failed), gpt-5.6-sol — rust-quality (failed), gpt-5.6-sol — general (failed), gpt-5.6-sol — security-auditor (failed), gpt-5.6-sol — rust-quality (failed), gpt-5.6-sol — general (completed), gpt-5.6-sol — security-auditor (completed), gpt-5.6-sol — rust-quality (completed)
  • Verifier: claude-sonnet-5 — final-verifier
  • Sonnet reviewers: claude-sonnet-5 — general (failed), claude-sonnet-5 — security-auditor (failed), claude-sonnet-5 — rust-quality (failed), claude-sonnet-5 — general (completed), claude-sonnet-5 — security-auditor (completed), claude-sonnet-5 — rust-quality (completed)

QuantumExplorer and others added 3 commits July 17, 2026 19:00
… address balance changes

A Type 17 Unshield credits its transparent platform output address via
a raw AddBalanceToAddress drive op, but the PaidFromShieldedPool
execution arm never recorded the credit into address_balances_in_update
— the sole feed for store_address_balances_for_block, i.e. the recent
address-balance-changes tree that incremental client sync
(GetRecentAddressBalanceChanges) reads. The absolute address-funds tree
was credited, so full trunk/branch scans saw the payment while every
incremental pass returned 0 entries: wallets showed unshield-received
platform payments only after a full rescan (observed on iOS: 18 recent
queries, 0 entries, last_known_recent_block stuck at 0 while the
balance appeared after relaunch). PaidFromAssetLockToPool had the
identical omission for a Type 18 ShieldFromAssetLock surplus output.

Thread the credited outputs through both execution-event variants
(added_to_balance_outputs, same shape as Paid's) and fold them into
address_balances_in_update on the applied path with the same
entry/merge idiom as the Paid arm. Type 16 ShieldedTransfer and Type 19
ShieldedWithdrawal pass None — neither credits a transparent platform
address.

Tests: construction coverage (net output, net-zero, surplus,
no-surplus) plus executor-level folds proving the map receives
AddToCredits for both variants.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e identity-create fallback credit

- cargo fmt (the macOS CI formatting failure)
- Extract the triplicated Set/Add address-credit merge (Paid /
  PaidFromShieldedPool / PaidFromAssetLockToPool arms) into one
  record_added_balance_outputs helper so the merge semantics can't
  silently diverge between event paths (CodeRabbit)
- Correct the added_to_balance_outputs doc: the
  IdentityCreateFromShieldedPool duplicate-key fallback surfaces as an
  UnshieldAction and DOES credit the fallback address (CodeRabbit)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ess_balances_updated

Review blocker: the IdentityCreateFromShieldedPool duplicate-key
fallback is an APPLIED, chargeable, paid-INVALID transition (a
chargeable_failure UnshieldAction) — but the invalid branch of
process_raw_state_transitions passed None for address-balance
tracking, PaidConsensusError had no field to carry the credit, and
StateTransitionsProcessingResult::add merged address changes only
from SuccessfulExecution. The fallback-address credit the block
actually applied never reached the recent-address-balance-changes
tree, so incremental sync omitted it.

- Track address balances through the invalid branch (the executor
  only records on the applied path, so bump-only paid-invalid
  transitions leave the map empty).
- PaidConsensusError gains address_balance_changes (same shape as
  SuccessfulExecution's) — populated at both construction sites,
  including the valid-branch UnsuccessfulPaidExecution arm, which had
  the identical latent omission — and add() merges it into
  address_balances_updated.
- Tests pin the chain at each seam: the real fallback UnshieldAction
  produces the credited event (construction), the invalid branch
  carries the credit out (process_validation_result seam), and add()
  merges PaidConsensusError maps into address_balances_updated. A
  full process_raw fixture would need a real Halo 2 proof (the
  fallback arises only after proof verification succeeds), which no
  existing fixture produces.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@QuantumExplorer
QuantumExplorer force-pushed the fix/unshield-recent-address-balance-changes branch 3 times, most recently from a80ce1d to 02e9f01 Compare July 17, 2026 12:47
@QuantumExplorer
QuantumExplorer marked this pull request as draft July 17, 2026 13:02
…protocol v13

Recording these credits adds writes to the recent-address-balance-
changes tree, changing the committed state root — mixed-version
validators would fork on the first affected transition. The gate is
the recording method itself, versioned per the platform_events idiom:

record_added_balance_outputs is its own method module with a real
v0 and v1, dispatched on a new
DriveAbciStateTransitionProcessingMethodVersions::
record_added_balance_outputs FeatureVersion (0 in V1..V8, 1 in the
new DRIVE_ABCI_METHOD_VERSIONS_V9, which PLATFORM_V13 adopts as its
only delta from v12). Call sites pass an origin discriminator:
v0 folds Transparent-origin outputs (the Paid arm's long-standing
behavior) and DROPS ShieldedSpend-origin outputs — byte-matching
pre-v13 blocks; v1 folds both.

Event construction populates added_to_balance_outputs
unconditionally (the event is in-memory only; v0 recording discards
shielded-origin credits), and every other downstream stage
(paid-invalid tracking, PaidConsensusError carry) stays
unconditional. store_address_balances_to_recent_block_storage is
untouched at Some(0) — its implementation did not change.

Tests pin the boundary at the recording layer (v0 drops shielded /
keeps transparent; v1 records both), construction's
version-independence at v12 and v13, and the version-struct delta
itself (field 0 at v12, 1 at v13, store method unchanged at both).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@QuantumExplorer
QuantumExplorer force-pushed the fix/unshield-recent-address-balance-changes branch from 02e9f01 to 50ce3bb Compare July 17, 2026 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants