wallet: Abandon descendants of orphaned coinbases #26499
Conversation
|
Approach NACK I think. Shouldn't these already be conflicted? |
Nope, see the linked issue. Anything that is invalidated by a reorg just gets marked as "inactive" which means "unconfirmed and not in mempool". I looked at marking such txs as conflicted, but ran into some issues with setting the conflicting blockhash and height as |
|
I guess in the context of I'm not sure unusual cases like that should get in the way of doing the right thing for the normal conflict case, though. And even that unusual case should resolve cleanly if we handle the normal case, after a competing block is accepting. |
shaavan
left a comment
There was a problem hiding this comment.
In the second commit, you mentioned the unconfirmed ancestors should be marked as abandoned even when the orphan coinbase has been reorged into the main chain.
What's the reasoning behind this?
If, say, I mine a block and create descendant transactions from it, and say it got orphaned, so I further mine over it and say I was successful in making it part of the main chain, why should my transactions, created using this block's coinbase transaction should get abandoned?
It's "should" as in that is the expected behavior given the current code, not "should" as in the correct thing to do.
I've implemented it this way because it's not clear to me what we should actually be doing. The descendants could be added back to the mempool, but that also has a privacy impact - rebroadcasting all of these transactions at the same time can link them all as being related to a single particular wallet. This privacy impact is made worse by the fact that there was a reorg as it is unlikely that other nodes on the network have them in their mempool already. Additionally, there's a technical challenge of identifying which to broadcast. It could be that some descendants also spend other coinbase outputs that were also reorged. One block being reorged back into the chain does not mean that the descendant blocks are as well. Given that this is only relevant in the cases of 100+ block reorgs, I think that it's reasonable to expect users affected by this issue to be actively involved and paying attention. Leaving the transactions abandoned just means that the user will have to rebroadcast the transactions themselves. Considering the rarity of this situation in practice, I don't think it's worth the effort to implement anything more complex either. |
rajarshimaitra
left a comment
There was a problem hiding this comment.
Concept + tACK b5e49a4b31debdc1801a22cc1de1542ab5f205c6
Few nits, questions and suggestions below..
Leaving the transactions abandoned just means that the user will have to rebroadcast the transactions themselves. Considering the rarity of this situation in practice, I don't think it's worth the effort to implement anything more complex either.
Should this be documented somewhere? User maybe actively noticing the situation as you suggested but might not know that the descendant needs another rebroadcast? Also given its a 100 blk reorg, the possibility of this occurring is very low.. But maybe just for sake completeness, makes sense to write a line in the code somewhere explaining the manual broadcasting part?
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. 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. |
b5e49a4 to
aaedf7b
Compare
I've added a comment about that, as well as a comment that the transaction states can still change after abandoning. |
|
CI: |
This error is actually really confusing. I ran into it a few times while writing the test, but whenever I actually tried to debug it, I couldn't get it to reproduce. What's happening is that the unconfirmed transaction descended from the previously orphaned parent is somehow making its way back into node0's mempool. What I don't understand is how that is happening. When attempting to debug this, I added some |
aaedf7b to
1a8f114
Compare
rajarshimaitra
left a comment
There was a problem hiding this comment.
ReACK 1a8f1148f00af8727ab5e2d58ae71bc69a75fdae
aureleoules
left a comment
There was a problem hiding this comment.
ACK 1a8f1148f00af8727ab5e2d58ae71bc69a75fdae
I verified that the test correctly test the behavior. Also executed the test hundreds of times in parallel in case there are intermittent failures and couldn't find one. (they are usually rare to reproduce though)
I suggest this diff (uses auto, structured binding and avoids some indirections)
Details
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 8c5c7e445..a5787b3a7 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -1072,7 +1072,7 @@ CWalletTx* CWallet::AddToWallet(CTransactionRef tx, const TxState& state, const
std::vector<CWalletTx*> txs;
txs.push_back(&wtx);
- TxStateInactive inactive_state = TxStateInactive{/*abandoned=*/true};
+ const auto inactive_state = TxStateInactive{/*abandoned=*/true};
while (!txs.empty()) {
CWalletTx* tx = txs.back();
@@ -1081,10 +1081,11 @@ CWalletTx* CWallet::AddToWallet(CTransactionRef tx, const TxState& state, const
// Break caches since we have changed the state
tx->MarkDirty();
MarkInputsDirty(tx->tx);
+ const auto& tx_hash = tx->GetHash();
for (unsigned int i = 0; i < tx->tx->vout.size(); ++i) {
- COutPoint outpoint(tx->GetHash(), i);
- std::pair<TxSpends::const_iterator, TxSpends::const_iterator> range = mapTxSpends.equal_range(outpoint);
- for (TxSpends::const_iterator it = range.first; it != range.second; ++it) {
+ const COutPoint outpoint(tx_hash, i);
+ const auto& [begin, end] = mapTxSpends.equal_range(outpoint);
+ for (auto it = begin; it != end; ++it) {
const auto wit = mapWallet.find(it->second);
if (wit != mapWallet.end()) {
txs.push_back(&wit->second);
furszy
left a comment
There was a problem hiding this comment.
Seems that the coinbase descendants state modification is not permitted to disk. So after a restart, they will get back to their previous state.
Is that intended because we have a duplicated check somewhere around the wallet load process or just something that is missing?
When an orphaned coinbase is reorged back into the main chain, any unconfirmed ancestors should still be marked as abandoned due to the original reorg that orphaned that coinbase.
1a8f114 to
b0fa598
Compare
Oops, fixed. |
| self.nodes[0].invalidateblock(conflict_block) | ||
| self.nodes[0].reconsiderblock(blk) | ||
| assert_equal(self.nodes[0].getbestblockhash(), orig_chain_tip) | ||
| self.generate(self.nodes[0], 3) |
There was a problem hiding this comment.
non-blocking question:
guess that the 3 blocks generated here are just to make the chain longer, so the other node accepts it and trigger a reorg (in that case, it should also work with 2 blocks).
There was a problem hiding this comment.
Yes, 2 would work, I think I just miscounted when writing this originally.
It does, but I think it's easier to read this way and the performance impact of breaking the caches twice and writing it twice is not that big. |
| desc_tx->m_state = inactive_state; | ||
| // Break caches since we have changed the state | ||
| desc_tx->MarkDirty(); | ||
| batch.WriteTx(*desc_tx); |
There was a problem hiding this comment.
Probably non-blocking: Looks like usually if the transaction can't be written, a nullptr is returned.
| batch.WriteTx(*desc_tx); | |
| if(!batch.WriteTx(*desc_tx)) return nullptr; |
b0fa598 test: Check that orphaned coinbase unconf spend is still abandoned (Andrew Chow) 9addbd7 wallet: Automatically abandon orphaned coinbases and their children (Andrew Chow) Pull request description: When a block is reorged out of the main chain, any descendants of the coinbase will no longer be valid. Currently they are only marked as inactive, which means that our balance calculations will still include them. In order to be excluded from the balance calculation, they need to either be abandoned or conflicted. This PR goes with the abandoned method. Note that even when they are included in balance calculations, coin selection will not select outputs belonging to these transactions because they are not in the mempool. Fixes bitcoin#14148 ACKs for top commit: furszy: ACK b0fa598 with a not-blocking nit. aureleoules: reACK b0fa598 ishaanam: ACK b0fa598 Tree-SHA512: 68f12e7aa8df392d8817dc6ac0becce8dbe83837bfa538f47027e6730e5b2e1b1a090cfcea2dc598398fdb66090e02d321d799f087020d7e1badcf96e598c3ac
When a block is reorged out of the main chain, any descendants of the coinbase will no longer be valid. Currently they are only marked as inactive, which means that our balance calculations will still include them. In order to be excluded from the balance calculation, they need to either be abandoned or conflicted. This PR goes with the abandoned method.
Note that even when they are included in balance calculations, coin selection will not select outputs belonging to these transactions because they are not in the mempool.
Fixes #14148