Skip to content

fix(key-wallet): re-apply a spend whose coin was funded after it - #1015

Open
ZocoLini wants to merge 1 commit into
devfrom
fix/reapply-spend-funded-late
Open

fix(key-wallet): re-apply a spend whose coin was funded after it#1015
ZocoLini wants to merge 1 commit into
devfrom
fix/reapply-spend-funded-late

Conversation

@ZocoLini

Copy link
Copy Markdown
Collaborator

Two mainnet restores of the same wallet ended at the same balance with 7112 and 7111 wallet records. Replaying each run's logged block applications offline, against the blocks it stored, reproduced both results exactly, with no divergence from the logs. The missing record was e66553f3…8c9e at height 2 185 057: it pays change to the BIP44 account and spends a CoinJoin coin funded at 2 182 877.

When the spend is applied before its funding, the CoinJoin account cannot recognise it. The funding then parks the coin in spent_before_funded (#1001), and that only attributes the spend if its block is delivered again. In one run it was (funding at step 1494, spend at 1516); in the other it was not (spend at 1608, funding at 1627, no redelivery), so the result depended on delivery order.

WalletInfoInterface::unrecorded_spend_heights reports, for a transaction, the heights of the blocks that spent its outputs before it arrived and that the owning account has not recorded yet. process_block_for_wallets returns them per wallet in BlockProcessingResult::reapply_heights, only heights above the block being applied, so re-applying cannot loop. BlocksManager re-applies those blocks from block storage straight away; every downloaded block is stored on arrival. Re-applications emit no SyncEvent::BlockProcessed and so never touch a batch's pending-block accounting.

Offline, both orderings now end with identical per-account records (7112), with about 105 blocks re-applied from disk per restore.

Two mainnet restores of the same wallet ended at the same balance with 7112
and 7111 wallet records. Replaying each run's logged block applications
offline, against the blocks it stored, reproduced both results exactly, with
no divergence from the logs. The missing record was e66553f3…8c9e at height
2 185 057: it pays change to the BIP44 account and spends a CoinJoin coin
funded at 2 182 877.

When the spend is applied before its funding, the CoinJoin account cannot
recognise it. The funding then parks the coin in `spent_before_funded` (#1001),
and that only attributes the spend if its block is delivered again. In one
run it was (funding at step 1494, spend at 1516); in the other it was not
(spend at 1608, funding at 1627, no redelivery), so the result depended on
delivery order.

`WalletInfoInterface::unrecorded_spend_heights` reports, for a transaction,
the heights of the blocks that spent its outputs before it arrived and that
the owning account has not recorded yet. `process_block_for_wallets` returns
them per wallet in `BlockProcessingResult::reapply_heights`, only heights
above the block being applied, so re-applying cannot loop. `BlocksManager`
re-applies those blocks from block storage straight away; every downloaded
block is stored on arrival. Re-applications emit no `SyncEvent::BlockProcessed`
and so never touch a batch's pending-block accounting.

Offline, both orderings now end with identical per-account records (7112),
with about 105 blocks re-applied from disk per restore.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017DruChNTWXwJoWPartZwCf
@coderabbitai

coderabbitai Bot commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

Next included review available in 55 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 95d3e8aa-a977-405c-aa79-f70b63fa94b7

📥 Commits

Reviewing files that changed from the base of the PR and between 82cc8cc and ab0172c.

📒 Files selected for processing (6)
  • dash-spv/src/sync/blocks/manager.rs
  • key-wallet-manager/src/process_block.rs
  • key-wallet-manager/src/test_utils/mock_wallet.rs
  • key-wallet-manager/src/wallet_interface.rs
  • key-wallet/src/tests/observed_spent_outpoints_tests.rs
  • key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs

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.

@codecov

codecov Bot commented Sep 12, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.49123% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.36%. Comparing base (d0671d5) to head (ab0172c).
⚠️ Report is 1 commits behind head on dev.

Files with missing lines Patch % Lines
dash-spv/src/sync/blocks/manager.rs 94.66% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #1015      +/-   ##
==========================================
+ Coverage   72.56%   77.36%   +4.80%     
==========================================
  Files         240      329      +89     
  Lines       63441    83949   +20508     
==========================================
+ Hits        46033    64948   +18915     
- Misses      17408    19001    +1593     
Flag Coverage Δ
core 78.25% <ø> (ø)
ffi 51.77% <ø> (+<0.01%) ⬆️
rpc 20.00% <ø> (ø)
spv 92.18% <94.66%> (?)
wallet 79.83% <100.00%> (+0.05%) ⬆️
Files with missing lines Coverage Δ
key-wallet-manager/src/process_block.rs 93.47% <100.00%> (+0.31%) ⬆️
key-wallet-manager/src/wallet_interface.rs 9.37% <ø> (ø)
...allet/managed_wallet_info/wallet_info_interface.rs 81.60% <100.00%> (+0.90%) ⬆️
dash-spv/src/sync/blocks/manager.rs 96.02% <94.66%> (ø)

... and 90 files with indirect coverage changes

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.

1 participant