Skip to content

fix(zebra-state): saturate non-finalized transparent received totals - #11295

Open
natalieesk wants to merge 2 commits into
mainfrom
nonfinalized_received_saturating_10556
Open

fix(zebra-state): saturate non-finalized transparent received totals#11295
natalieesk wants to merge 2 commits into
mainfrom
nonfinalized_received_saturating_10556

Conversation

@natalieesk

Copy link
Copy Markdown
Contributor

Motivation

Closes #10556.

getaddressbalance combines finalized transparent address accounting with non-finalized chain accounting. The finalized path treats received as a saturating counter, but the non-finalized path used ordinary u64 addition.

TransparentTransfers::received() sums every UTXO ever created for the address in the partial chain. Spent UTXOs are not removed from created_utxos (only block-revert removes them), so received() is a cumulative total that grows with self-transfer churn while the address balance stays bounded. With enough churn inside the non-finalized window (~8785 max-money receipts to the same address) the total crosses u64::MAX: debug/test builds panic, release builds wrap and getaddressbalance returns a wrapped received until those blocks finalize.

This is not a consensus issue — it is address-index RPC correctness plus debug-build availability hardening for nodes serving address-index RPCs over recent non-finalized state. Reported by @zmanian during post-v4.4.0 audit follow-up.

Solution

Use saturating arithmetic consistently for non-finalized transparent received totals, matching the finalized path:

  • TransparentTransfers::received() folds the created-UTXO values with u64::saturating_add;
  • Chain::partial_transparent_balance_change() combines per-address totals with received.saturating_add(transfers.received()).

The finalized state already saturates at each combine point (per-output, per-Add, and cross-address); the non-finalized path now does too, and the final finalized + non-finalized combine in read/address/balance.rs was already saturating.

Tests

Added received_saturates_past_u64_max, a unit test that inserts enough max-money created UTXOs (8786) into a TransparentTransfers to exceed u64::MAX when summed exactly, and asserts received() returns u64::MAX. Against the old .sum() code it panics with "attempt to add with overflow" in this debug build (and would wrap in release), so it fails without the fix and passes with it. The test asserts inline that the chosen count really does exceed u64::MAX.

cargo test -p zebra-state --lib non_finalized (33 passed), cargo fmt --all -- --check, and cargo clippy -p zebra-state --lib --all-features are clean.

Specifications & References

  • ZIP-based transparent value handling; the finalized saturating behavior this matches is documented at zebra-state/src/service/read/address/balance.rs (an address can receive more than the max money supply by sending to itself).

Follow-up Work

The cross-address fold in partial_transparent_balance_change is asserted by inspection (it is structurally identical to the finalized cross-address fold and to the covered received() site); a dedicated multi-address Chain-level test for that combine point could be added for symmetry but is not included here.

AI Disclosure

  • No AI tools were used in this PR
  • AI tools were used: Claude (Claude Code) — wrote the fix and regression test.

PR Checklist

  • The PR title follows conventional commits format: type(scope): description
  • The PR follows the contribution guidelines.
  • This change was discussed in an issue or with the team beforehand.
  • The solution is tested.
  • The documentation and changelogs are up to date.

TransparentTransfers::received() sums every UTXO ever created for an
address in the partial chain; spent UTXOs are not removed from that set,
so the cumulative total grows with self-transfer churn while the balance
stays bounded. With enough churn the total crosses u64::MAX, panicking in
debug builds and wrapping in release. Sum with saturating_add, and
saturate the per-address fold in partial_transparent_balance_change too,
matching the finalized state's saturating accounting.

Closes #10556.
@v12-auditor

v12-auditor Bot commented Aug 19, 2026

Copy link
Copy Markdown

Note

Complete: Audit complete. V12 found four issues worth reviewing.

Open the full results here.

FindingSeverityDetails
F-235490 🟡 Medium
Balance queries clone non-finalized state

chain_transparent_balance_change obtains mutable access with Arc::make_mut before its already-finalized early return and before the pop_root loop that is the only mutation this read needs. latest_best_chain creates the request's Arc by cloning it from the watched NonFinalizedState, while the writer publishes a clone of its own state, so the allocation is shared during steady-state operation and make_mut performs a structural clone. That clone duplicates the owned block-index, UTXO, nullifier, and transparent-transfer maps in ChainInner, which can cover the 1,000-block rollback window. Both JSON-RPC getaddressbalance and the optionally enabled, unauthenticated lightwalletd balance endpoint reach this path. Inference: concurrent small balance requests can therefore multiply chain-sized allocation and map-copy work, exhausting CPU or memory on nodes exposing the endpoint.

F-235492 🔵 Low
Fork race mixes incompatible balance histories

An address-balance request clones the current best non-finalized chain independently from its later finalized database query. The finalized query verifies that the complete (height, hash) tip stayed stable, but it then returns only the height and discards the hash. The writer publishes a newly selected best chain before finalizing its over-limit root, so an in-flight request can retain losing fork A while a competing fork B becomes the stable finalized state. chain_transparent_balance_change then pops A solely by height and applies its remaining deltas without checking that its root is a child of B's finalized hash. The response can consequently combine mutually exclusive fork histories even though the function's correctness contract requires matching chains.

F-235493 🔵 Low
Self-transfer intermediate aborts block processing

TransparentTransfers.balance is constrained to -MAX_MONEY..=MAX_MONEY, and adding a created output outside that range is converted into a process-fatal expect. Block application indexes every transaction's created outputs before indexing its spent inputs, so a self-transfer temporarily adds both the address's existing positive partial-chain delta and the recreated output before subtracting the spent value. A protocol-valid sequence that first transfers more than half of MAX_MONEY to an address inside the non-finalized window and then spends it back to that address makes this intermediate exceed MAX_MONEY, although the transaction's final delta remains valid. Repeating ordinary self-transfer churn does not accumulate this net balance; the precondition is control of more than approximately 10.5 million ZEC in recently created transparent outputs. The new saturation protects only the cumulative received field and leaves this sibling arithmetic panic reachable under that economic condition.

F-235494 🔵 Low
Migration wraps valid received totals

The format-27 database migration still accumulates all outputs to an address within one block using plain u64 += before any saturating merge occurs. Consensus allows an output from an earlier transaction in a block to be spent by a later transaction, so a miner can repeatedly recreate the same value at the same address and make gross per-block receipts exceed the money supply. For example, 18,447 one-input/one-output P2SH-OP_TRUE transactions that each recreate a 10,000,000 ZEC output occupy about 1.845 MB and sum to 18,447,000,000,000,000,000 zatoshi, which exceeds u64::MAX. A node upgrading a pre-format-27 database containing such a valid block wraps this accumulator in release or aborts in an overflow-checked build. The later saturating RocksDB merge and query-path folds cannot recover the high bits already lost by the migration.

And two more auto-invalidated findings.

Analyzed two files, diff 4b4d9ec...32e713b.

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.

bug: Non-finalized transparent received totals can overflow before finalized saturation

2 participants