wallet: abandon orphan coinbase txs, and their descendants, during startup #31794
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31794. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
|
Test failure is unrelated. |
There was a problem hiding this comment.
ACK 0331060
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.
diff --git a/test/functional/wallet_abandonconflict.py b/test/functional/wallet_abandonconflict.py
index ce0f4d099b..2bbfaee3db 100755
--- a/test/functional/wallet_abandonconflict.py
+++ b/test/functional/wallet_abandonconflict.py
@@ -45,6 +45,10 @@ class AbandonConflictTest(BitcoinTestFramework):
txB = alice.sendtoaddress(alice.getnewaddress(), Decimal("10"))
txC = alice.sendtoaddress(alice.getnewaddress(), Decimal("10"))
self.sync_mempools()
+
+ # Can not abandon transaction in mempool
+ assert_raises_rpc_error(-5, 'Transaction not eligible for abandonment', lambda: alice.abandontransaction(txid=txA))
+
self.generate(self.nodes[1], 1)
# Can not abandon non-wallet transaction
furszy
force-pushed
the
2025_wallet_abandon_coinbase_during_startup
branch
from
0331060 to
409241d
Compare
|
Thanks for the review Eunovo. Feedback tackled.
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. |
|
ReACK 409241d |
rkrux
left a comment
rkrux
left a comment
There was a problem hiding this comment.
Cursory review, will look again soon
| // states change will remain abandoned and will require manual broadcast if the user wants them. | ||
|
|
||
| RecursiveUpdateTxState(hashTx, try_updating_state); | ||
| RecursiveUpdateTxState(tx.GetHash(), try_updating_state); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
left a comment
rkrux
left a comment
There was a problem hiding this comment.
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 (review)
I agree with @furszy that this should be in a separate PR because of the separate context.
furszy
force-pushed
the
2025_wallet_abandon_coinbase_during_startup
branch
from
409241d to
3c89cab
Compare
|
ACK 3c89cab06a6259fa70eaf3a78c01ee42780c4e27 |
rkrux
left a comment
rkrux
left a comment
There was a problem hiding this comment.
git 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.
furszy
force-pushed
the
2025_wallet_abandon_coinbase_during_startup
branch
from
3c89cab to
e4dd5a3
Compare
mzumsande
left a comment
mzumsande
left a comment
There was a problem hiding this comment.
Code Review ACK e4dd5a3
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).
|
ACK e4dd5a3 |
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.