wallet: abandon orphan coinbase txs, and their descendants, during startup

furszy commented at 4:17 pm on February 4, 2025: member

Since #26499, we mark coinbase transactions and their descendants as abandoned when a reorg arises through the “block disconnection” signal handler. However, this does not cover all scenarios; external wallets could contain coinbase transactions from blocks the node has not seen yet, or the user could have replaced the chain with an earlier or different version (one without the coinbase chain).

This affects balance calculation as well as mempool rebroadcast (descendants shouldn’t be relayed). Fix this by marking orphaned coinbase transactions and their descendants as abandoned during wallet startup.

wallet: abandon inactive coinbase tx and their descendants during startup 474139aa9b

DrahtBot commented at 4:17 pm on February 4, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31794.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	rkrux, mzumsande, achow101
Stale ACK	Eunovo

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

DrahtBot added the label Wallet on Feb 4, 2025

DrahtBot added the label CI failed on Feb 4, 2025

furszy commented at 8:09 pm on February 4, 2025: member

Test failure is unrelated.

DrahtBot removed the label CI failed on Feb 4, 2025

in test/functional/wallet_reorgsrestore.py:76 in 0331060d13 outdated

71+
72+        # Verify both nodes are on a different chain
73+        block0_best_hash, block1_best_hash = wallet0.getbestblockhash(), wallet1.getbestblockhash()
74+        assert(block0_best_hash != block1_best_hash)
75+
76+        # Now both nodes and replace node0 chain entirely for the node1 chain

Eunovo commented at 9:27 am on February 10, 2025:

https://github.com/bitcoin/bitcoin/pull/31794/commits/0331060d1354bc8620564e6352946bd5be38fef3: # Now stop both nodes...

Eunovo commented at 2:09 pm on February 10, 2025: none

ACK https://github.com/bitcoin/bitcoin/pull/31794/commits/0331060d1354bc8620564e6352946bd5be38fef3

While testing, I noticed that trying to abandon a tx in mempool wasn’t covered in the functional tests.
Here’s a diff for that.

 0diff --git a/test/functional/wallet_abandonconflict.py b/test/functional/wallet_abandonconflict.py
 1index ce0f4d099b..2bbfaee3db 100755
 2--- a/test/functional/wallet_abandonconflict.py
 3+++ b/test/functional/wallet_abandonconflict.py
 4@@ -45,6 +45,10 @@ class AbandonConflictTest(BitcoinTestFramework):
 5         txB = alice.sendtoaddress(alice.getnewaddress(), Decimal("10"))
 6         txC = alice.sendtoaddress(alice.getnewaddress(), Decimal("10"))
 7         self.sync_mempools()
 8+
 9+        # Can not abandon transaction in mempool
10+        assert_raises_rpc_error(-5, 'Transaction not eligible for abandonment', lambda: alice.abandontransaction(txid=txA))
11+
12         self.generate(self.nodes[1], 1)
13 
14         # Can not abandon non-wallet transaction

furszy force-pushed on Feb 10, 2025

furszy commented at 6:31 pm on February 10, 2025: member

Thanks for the review Eunovo. Feedback tackled.

While testing, I noticed that trying to abandon a tx in mempool wasn’t covered in the functional tests

That seems to be material for a different PR (a quick one). Happy to ACK it if you push it.

Eunovo commented at 6:39 pm on February 10, 2025: none

That seems to be material for a different PR (a quick one). Happy to ACK it if you push it.

It’d be a really tiny PR. I think you can just add it but I’m happy to open a new one if needed.

Eunovo commented at 6:41 pm on February 10, 2025: none

ReACK https://github.com/bitcoin/bitcoin/pull/31794/commits/409241db5dca0b23f5c7714f99be52411fc5541e

in src/wallet/wallet.cpp:1348 in 474139aa9b outdated

1344@@ -1342,7 +1345,7 @@ bool CWallet::AbandonTransaction(const uint256& hashTx)
1345     // Note: If the reorged coinbase is re-added to the main chain, the descendants that have not had their
1346     // states change will remain abandoned and will require manual broadcast if the user wants them.
1347 
1348-    RecursiveUpdateTxState(hashTx, try_updating_state);
1349+    RecursiveUpdateTxState(tx.GetHash(), try_updating_state);

rkrux commented at 2:36 pm on February 13, 2025:

RecursiveUpdateTxState() accepts a const uint256& tx_hash as the first argument but tx.GetHash() returns a Txid&, which is Txid = transaction_identifier<false>. IIUC, this works because of this conversion function: https://github.com/bitcoin/bitcoin/blob/master/src/util/transaction_identifier.h#L67 A comment suggests for the new code to use ToUint256 instead: https://github.com/bitcoin/bitcoin/blob/master/src/util/transaction_identifier.h#L62

furszy commented at 8:03 pm on February 14, 2025:

We haven’t started following this pattern in the wallet sources yet. Might be better to do it all at once in a single PR and verify that nothing breaks (which should be the case because we don’t use the witness id).

rkrux commented at 3:16 pm on February 13, 2025: none

Cursory review, will look again soon

in test/functional/wallet_reorgsrestore.py:42 in 409241db5d outdated

37+        # Verify the wallet marks coinbase transactions, and their descendants, as abandoned during startup when #
38+        # the block is no longer part of the best chain.                                                         #
39+        ##########################################################################################################
40+        # Sync nodes for the coming test and assert all are at the same block
41+        for i, j in [(0, 1), (1, 2), (2, 0)]:
42+            self.connect_nodes(i, j)

rkrux commented at 8:24 am on February 14, 2025:

Nit: The third node isn’t really used in the test. I ran the test without it and it worked.

in test/functional/wallet_reorgsrestore.py:80 in 409241db5d outdated

75+
76+        # Stop both nodes and replace node0 chain entirely for the node1 chain
77+        self.stop_nodes()
78+        for path in ["chainstate", "blocks"]:
79+            shutil.rmtree(self.nodes[0].chain_path / path)
80+        shutil.copytree(self.nodes[1].chain_path / path, self.nodes[0].chain_path / path)

rkrux commented at 8:25 am on February 14, 2025:

Was this statement supposed to be inside the for loop? Otherwise only blocks seem to be copied to node0 while the chainstate as well is deleted in node0.

furszy commented at 7:38 pm on February 14, 2025:

upps, yes. Thx.

in test/functional/wallet_reorgsrestore.py:63 in 409241db5d outdated

61+
62+        # Verify balance
63+        wallet0.syncwithvalidationinterfacequeue()
64+        assert(wallet0.getbalances()['mine']['trusted'] > 0)
65+
66+        # Now create a fork in node1. This will be used to replace node0's chain later.

rkrux commented at 8:31 am on February 14, 2025:

These comments are very helpful!

in test/functional/wallet_reorgsrestore.py:89 in 409241db5d outdated

84+        wallet0 = self.nodes[0].get_wallet_rpc("w0")
85+        assert_equal(wallet0.getbestblockhash(), block1_best_hash)
86+        assert_raises_rpc_error(-5, "Block not found", wallet0.getblock, block0_best_hash)
87+
88+        # Verify the coinbase tx was marked as abandoned and balance correctly computed
89+        assert_equal(wallet0.gettransaction(node0_coinbase_tx_hash)['details'][0]['abandoned'], True)

rkrux commented at 8:35 am on February 14, 2025:

Would it be helpful to assert for 'category': 'orphan' as well?

furszy commented at 7:41 pm on February 14, 2025:

doesn’t hurt to add it.

in src/wallet/walletdb.cpp:1122 in 474139aa9b outdated

1114@@ -1115,6 +1115,14 @@ static DBErrors LoadTxRecords(CWallet* pwallet, DatabaseBatch& batch, std::vecto
1115     });
1116     result = std::max(result, order_pos_res.m_result);
1117 
1118+    // After loading all tx records, abandon any coinbase that is no longer in the active chain.
1119+    // This could happen during an external wallet load, or if the user replaced the chain data.
1120+    for (auto& [id, wtx] : pwallet->mapWallet) {
1121+        if (wtx.IsCoinBase() && wtx.isInactive()) {
1122+            pwallet->AbandonTransaction(wtx);

rkrux commented at 8:48 am on February 14, 2025:

Trying to understand why the AbandonTransaction was overloaded. IIUC, the older one AbandonTransaction(const uint256& hashTx) can be used as well here with the id passed as the argument.

Am I missing something? I do notice EXCLUSIVE_LOCKS_REQUIRED in the new function signature but I also see it being used already in the older function before this change.

furszy commented at 7:22 pm on February 14, 2025:

Trying to understand why the AbandonTransaction was overloaded.

Just to avoid the extra map lookup when we already have access to the wtx.

rkrux commented at 7:37 am on February 15, 2025:

Hmm fair enough.

in src/wallet/walletdb.cpp:1119 in 474139aa9b outdated

1114@@ -1115,6 +1115,14 @@ static DBErrors LoadTxRecords(CWallet* pwallet, DatabaseBatch& batch, std::vecto
1115     });
1116     result = std::max(result, order_pos_res.m_result);
1117 
1118+    // After loading all tx records, abandon any coinbase that is no longer in the active chain.
1119+    // This could happen during an external wallet load, or if the user replaced the chain data.

rkrux commented at 8:50 am on February 14, 2025:

Does an external wallet here mean when a wallet is loaded from wallet file that was manually added by the user in the path?

rkrux commented at 9:10 am on February 14, 2025:

Is the following scenario handled by the usual handling or via this new one?

User has their node running and receive a coinbase transaction, and then the node is stopped. User restarts the node after a long time and meanwhile that transaction was invalidated (block invalidated), and at the time of loading the wallet the correct balance is shown.

furszy commented at 7:58 pm on February 14, 2025:

“External wallets” are wallets that were created on a different node instance. You can either load them by path, using the dump/import functionality or just by moving them manually into the wallets/ directory.

furszy commented at 8:11 pm on February 14, 2025:

Is the following scenario handled by the usual handling or via this new one?

User has their node running and receive a coinbase transaction, and then the node is stopped. User restarts the node after a long time and meanwhile that transaction was invalidated (block invalidated), and at the time of loading the wallet the correct balance is shown.

This new one.

rkrux approved

rkrux commented at 9:11 am on February 14, 2025: none

ACK 409241db5dca0b23f5c7714f99be52411fc5541e

Build and tests successful.

Seems to be an edge case that’s fixed but I’m in favour because it ensures data correctness.

Re: this comment #31794#pullrequestreview-2605174936 I agree with @furszy that this should be in a separate PR because of the separate context.

furszy force-pushed on Feb 14, 2025

achow101 commented at 10:09 pm on February 14, 2025: member

ACK 3c89cab06a6259fa70eaf3a78c01ee42780c4e27

DrahtBot requested review from Eunovo on Feb 14, 2025

DrahtBot requested review from rkrux on Feb 14, 2025

in test/functional/wallet_reorgsrestore.py:46 in 3c89cab06a outdated

41+        # Test setup: Sync nodes for the coming test, ensuring both are at the same block, then disconnect them to
42+        # generate two competing chains. After disconnection, verify no other peer connection exists.
43+        self.connect_nodes(1, 0)
44+        self.sync_blocks(self.nodes[:1])
45+        self.disconnect_nodes(1, 0)
46+        assert all(len(node.getpeerinfo()) == 0 for node in self.nodes[:1])

rkrux commented at 7:53 am on February 15, 2025:

Need to use :2, otherwise it just syncs 1 block and iterates over 1 block - the first one.

0self.sync_blocks(self.nodes[:2])
1self.disconnect_nodes(1, 0)
2assert all(len(node.getpeerinfo()) == 0 for node in self.nodes[:2])

furszy commented at 3:50 pm on February 15, 2025:

Fixed. Thx.

rkrux commented at 8:41 am on February 15, 2025: none

0git range-diff 409241db5dca0b23f5c7714f99be52411fc5541e...3c89cab06a6259fa70eaf3a78c01ee42780c4e27

Newer changes are updating the functional tests based on the comments suggested earlier. I will ACK once a small bug is corrected.

test: wallet, abandon coinbase txs and their descendants during startup e4dd5a351b

furszy force-pushed on Feb 15, 2025

rkrux approved

rkrux commented at 5:50 pm on February 15, 2025: none

tACK e4dd5a351bde88a94326824945f4c8b1e4c15df2

DrahtBot requested review from achow101 on Feb 15, 2025

mzumsande commented at 11:41 pm on February 17, 2025: contributor

Code Review ACK e4dd5a351bde88a94326824945f4c8b1e4c15df2

Another scenario that wasn’t explicitly mentioned in the OP is a simple reorg: Wallet has a coinbase tx, then it’s unloaded, then the node undergoes a reorg, then the wallet is loaded again, and the coinbase tx should be abandoned. That would also make for a simpler test (no need to copy datadirs).

achow101 commented at 2:13 am on February 19, 2025: member

ACK e4dd5a351bde88a94326824945f4c8b1e4c15df2

achow101 merged this on Feb 19, 2025

achow101 closed this on Feb 19, 2025

wallet: abandon orphan coinbase txs, and their descendants, during startup #31794

Code Coverage & Benchmarks

Reviews