wallet: Abandon descendants of orphaned coinbases

achow101 commented at 9:22 pm on November 14, 2022: member

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

DrahtBot added the label Wallet on Nov 14, 2022

luke-jr commented at 11:40 pm on November 14, 2022: member

Approach NACK I think. Shouldn’t these already be conflicted?

achow101 commented at 0:15 am on November 15, 2022: member

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 invalidateblock just marks something as invalid (and reorgs out everything) without having a conflicting block.

luke-jr commented at 1:47 am on November 15, 2022: member

I guess in the context of invalidateblock, the transaction should be entirely invalid and thus not in the wallet at all. But that would be ugly, especially after it’s already there.

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 commented at 1:28 pm on November 15, 2022: contributor

@achow101

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?

achow101 commented at 4:34 pm on November 15, 2022: member

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.

It’s “should” as in that is the expected behavior given the current code, not “should” as in the correct thing to do.

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?

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.

in test/functional/wallet_orphanedreward.py:42 in b5e49a4b31 outdated

49-        # lines succeed, and probably should not be needed; see
50-        # https://github.com/bitcoin/bitcoin/issues/14148.
51-        self.nodes[1].abandontransaction(txid)
52+        blocks = self.generate(self.nodes[0], 152)
53+        conflict_block = blocks[0]
54+        # We expect the desendants of orphaned rewards to no longer be considered

rajarshimaitra commented at 7:23 am on November 16, 2022:

nit:

0        # We expect the descendants of orphaned rewards to no longer be considered

achow101 commented at 4:12 pm on November 16, 2022:

Done

in test/functional/wallet_orphanedreward.py:52 in b5e49a4b31 outdated

60-        self.nodes[1].sendtoaddress(self.nodes[0].getnewaddress(), 9)
61+        # And the unconfirmed tx to be abandoned
62+        assert_equal(self.nodes[1].gettransaction(txid)["details"][0]["abandoned"], True)
63+
64+        # If the orphaned reward is reorged back into the main chain, any unconfirmed txs
65+        # at the time of the original reorg remain abandoned.

rajarshimaitra commented at 7:32 am on November 16, 2022:

Should it be more clear to say “any unconfirmed descendant transactions remains abandoned”?

achow101 commented at 4:12 pm on November 16, 2022:

Done

in test/functional/wallet_orphanedreward.py:33 in b5e49a4b31 outdated

31@@ -32,28 +32,31 @@ def run_test(self):
32         self.generate(self.nodes[0], 150)
33         assert_equal(self.nodes[1].getbalance(), 10 + 25)

rajarshimaitra commented at 7:41 am on November 16, 2022:

Maybe I am dumb and missing something very obvious, why is the block reward 25 here?

achow101 commented at 4:01 pm on November 16, 2022:

Regtest’s halving interval is 150 blocks.

rajarshimaitra commented at 2:06 pm on November 17, 2022:

Ohh.. My bad..

in test/functional/wallet_orphanedreward.py:63 in b5e49a4b31 outdated

64+        # If the orphaned reward is reorged back into the main chain, any unconfirmed txs
65+        # at the time of the original reorg remain abandoned.
66+        self.nodes[0].invalidateblock(conflict_block)
67+        self.nodes[0].reconsiderblock(blk)
68+        assert_equal(self.nodes[0].getbestblockhash(), orig_chain_tip)
69+        self.generate(self.nodes[0], 3)

rajarshimaitra commented at 7:43 am on November 16, 2022:

Should we let the block reward mature again?? Then show that the reward is included in the balance but the descendant is still abandoned? I manually verified that’s the case..

achow101 commented at 4:13 pm on November 16, 2022:

The reward does actually mature since reconsiderblock will reactivate the entire reorged chain, not just that particular block. I’ve added a check that the balances are as we expect for the matured reward.

rajarshimaitra commented at 7:46 am on November 16, 2022: contributor

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?

DrahtBot commented at 4:01 pm on November 16, 2022: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	furszy, aureleoules, ishaanam
Approach NACK	luke-jr
Stale ACK	rajarshimaitra

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

achow101 force-pushed on Nov 16, 2022

achow101 commented at 4:14 pm on November 16, 2022: member

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?

I’ve added a comment about that, as well as a comment that the transaction states can still change after abandoning.

maflcko commented at 12:29 pm on November 17, 2022: member

CI:

 0  File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/wallet_orphanedreward.py", line 57, in run_test
 1    self.generate(self.nodes[0], 3)
 2  File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 650, in generate
 3    sync_fun() if sync_fun else self.sync_all()
 4  File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 715, in sync_all
 5    self.sync_mempools(nodes)
 6  File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 708, in sync_mempools
 7    raise AssertionError("Mempool sync timed out after {}s:{}".format(
 8AssertionError: Mempool sync timed out after 2400s:
 9  {'22b1bf7d8f18b9a08c8f5ad676f3372000a5ad247f4fae848b2a76309c0dba3b'}
10  set()

achow101 commented at 4:43 pm on November 17, 2022: member

CI:

 0  File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/wallet_orphanedreward.py", line 57, in run_test
 1    self.generate(self.nodes[0], 3)
 2  File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 650, in generate
 3    sync_fun() if sync_fun else self.sync_all()
 4  File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 715, in sync_all
 5    self.sync_mempools(nodes)
 6  File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 708, in sync_mempools
 7    raise AssertionError("Mempool sync timed out after {}s:{}".format(
 8AssertionError: Mempool sync timed out after 2400s:
 9  {'22b1bf7d8f18b9a08c8f5ad676f3372000a5ad247f4fae848b2a76309c0dba3b'}
10  set()

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 print(self.nodes[0].getrawmempool()) statements before the generate and those indicated that node0’s mempool is empty, as it should be. The transaction is removed from the mempool when its parent is reorged out of the chain, and the wallet is not rebroadcasting it either, so I don’t understand how node0 becomes aware of it again.

achow101 force-pushed on Nov 17, 2022

rajarshimaitra approved

rajarshimaitra commented at 4:53 am on November 19, 2022: contributor

ReACK 1a8f1148f00af8727ab5e2d58ae71bc69a75fdae

aureleoules approved

aureleoules commented at 4:14 pm on November 22, 2022: member

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)

 0diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
 1index 8c5c7e445..a5787b3a7 100644
 2--- a/src/wallet/wallet.cpp
 3+++ b/src/wallet/wallet.cpp
 4@@ -1072,7 +1072,7 @@ CWalletTx* CWallet::AddToWallet(CTransactionRef tx, const TxState& state, const
 5         std::vector<CWalletTx*> txs;
 6         txs.push_back(&wtx);
 7 
 8-        TxStateInactive inactive_state = TxStateInactive{/*abandoned=*/true};
 9+        const auto inactive_state = TxStateInactive{/*abandoned=*/true};
10 
11         while (!txs.empty()) {
12             CWalletTx* tx = txs.back();
13@@ -1081,10 +1081,11 @@ CWalletTx* CWallet::AddToWallet(CTransactionRef tx, const TxState& state, const
14             // Break caches since we have changed the state
15             tx->MarkDirty();
16             MarkInputsDirty(tx->tx);
17+            const auto& tx_hash = tx->GetHash();
18             for (unsigned int i = 0; i < tx->tx->vout.size(); ++i) {
19-                COutPoint outpoint(tx->GetHash(), i);
20-                std::pair<TxSpends::const_iterator, TxSpends::const_iterator> range = mapTxSpends.equal_range(outpoint);
21-                for (TxSpends::const_iterator it = range.first; it != range.second; ++it) {
22+                const COutPoint outpoint(tx_hash, i);
23+                const auto& [begin, end] = mapTxSpends.equal_range(outpoint);
24+                for (auto it = begin; it != end; ++it) {
25                     const auto wit = mapWallet.find(it->second);
26                     if (wit != mapWallet.end()) {
27                         txs.push_back(&wit->second);

in src/wallet/wallet.cpp:1078 in 1449a44bc1 outdated

1073+        txs.push_back(&wtx);
1074+
1075+        TxStateInactive inactive_state = TxStateInactive{/*abandoned=*/true};
1076+
1077+        while (!txs.empty()) {
1078+            CWalletTx* tx = txs.back();

furszy commented at 8:38 pm on January 8, 2023:

would be better to rename this variable, it’s shadowing the method’s argument.

achow101 commented at 11:36 pm on January 10, 2023:

Fixed.

in src/wallet/wallet.cpp:1073 in 1449a44bc1 outdated

1066@@ -1067,6 +1067,33 @@ CWalletTx* CWallet::AddToWallet(CTransactionRef tx, const TxState& state, const
1067         }
1068     }
1069 
1070+    // Mark inactive coinbase transactions and their descendants as abandoned
1071+    if (wtx.IsCoinBase() && wtx.isInactive()) {
1072+        std::vector<CWalletTx*> txs;
1073+        txs.push_back(&wtx);

furszy commented at 8:45 pm on January 8, 2023:

nit: could inline this

0std::vector<CWalletTx*> txs{&wtx};

achow101 commented at 11:36 pm on January 10, 2023:

Done

furszy commented at 8:59 pm on January 8, 2023: member

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?

wallet: Automatically abandon orphaned coinbases and their children 9addbd7890

test: Check that orphaned coinbase unconf spend is still abandoned

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.

b0fa5989e1

achow101 force-pushed on Jan 10, 2023

achow101 commented at 11:36 pm on January 10, 2023: member

Seems that the coinbase descendants state modification is not permitted to disk. So after a restart, they will get back to their previous state.

Oops, fixed.

in test/functional/wallet_orphanedreward.py:63 in b0fa5989e1

60+        # If the orphaned reward is reorged back into the main chain, any unconfirmed
61+        # descendant txs at the time of the original reorg remain abandoned.
62+        self.nodes[0].invalidateblock(conflict_block)
63+        self.nodes[0].reconsiderblock(blk)
64+        assert_equal(self.nodes[0].getbestblockhash(), orig_chain_tip)
65+        self.generate(self.nodes[0], 3)

furszy commented at 6:53 pm on January 11, 2023:

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).

achow101 commented at 7:37 pm on January 23, 2023:

Yes, 2 would work, I think I just miscounted when writing this originally.

furszy approved

furszy commented at 8:18 pm on January 11, 2023: member

ACK b0fa5989 with a not-blocking nit.

The initial round of the loop (when we only have the orphan coinbase in the vector) isn’t needed. We update the state and reset the cached amounts below.

aureleoules approved

aureleoules commented at 3:07 pm on January 16, 2023: member

reACK b0fa5989e1b77a343349bd36f8bc407f9366a589

achow101 commented at 7:38 pm on January 23, 2023: member

The initial round of the loop (when we only have the orphan coinbase in the vector) isn’t needed. We update the state and reset the cached amounts below.

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.

in src/wallet/wallet.cpp:1082 in b0fa5989e1

1077+            CWalletTx* desc_tx = txs.back();
1078+            txs.pop_back();
1079+            desc_tx->m_state = inactive_state;
1080+            // Break caches since we have changed the state
1081+            desc_tx->MarkDirty();
1082+            batch.WriteTx(*desc_tx);

ishaanam commented at 11:57 pm on January 29, 2023:

Probably non-blocking: Looks like usually if the transaction can’t be written, a nullptr is returned.

0            if(!batch.WriteTx(*desc_tx)) return nullptr;

ishaanam commented at 2:40 am on January 30, 2023: contributor

ACK b0fa5989e1b77a343349bd36f8bc407f9366a589

glozow merged this on Jan 30, 2023

glozow closed this on Jan 30, 2023

sidhujag referenced this in commit 918e7371bc on Jan 30, 2023

bitcoin locked this on Jan 30, 2024

wallet: Abandon descendants of orphaned coinbases #26499

Reviews