wallet: Check for uninitialized last processed and conflicting heights in MarkConflicted

achow101 commented at 2:37 am on September 27, 2023: member

MarkConflicted assumes that m_last_block_processed_height is always valid. However it may not be valid when a chain is not attached, as happens in the wallet tool and during migration. In such situations, when the conflicting height is also negative (which occurs on loading when no chain is available), the calculation of the number of conflict confirms results in a non-negative value which passes the existing check for valid values. This will subsequently hit an assertion in GetTxDepthInMainChain.

Furthermore, MarkConflicted is also only called on loading a transaction whose parent has a stored state of TxStateConflicted and was loaded before the child transaction. This depends on the loading order, which for both sqlite and bdb depends on the txids.

We can avoid this by explicitly checking that both m_last_block_processed_height and conflicting_height are non-negative. Both tool_wallet.py and wallet_migration.py are updated to create wallets with a state that triggers the assertion.

Fixes #28510

wallet: Check last block and conflict height are valid in MarkConflicted

MarkConflicted calculates conflict confirmations incorrectly when both
the last block processed height and the conflicting height are negative
(i.e. uninitialized). If either are negative, we should not be marking
conflicts and should exit early.

4660fc82a1

test: Test loading wallets with conflicts without a chain

Loading a wallet with conflicts without a chain (e.g. wallet tool and
migration) would previously result in an assertion due to -1 being both
a valid number of conflict confirmations, and the indicator that that
member has not been set yet.

782701ce7d

DrahtBot commented at 2:37 am on September 27, 2023: 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	ryanofsky, furszy
Concept ACK	MarcoFalke

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 Sep 27, 2023

DrahtBot added the label CI failed on Sep 27, 2023

maflcko commented at 6:14 am on September 27, 2023: member

From CI https://cirrus-ci.com/task/4777376270254080?logs=ci#L2730:

 0 node0 2023-09-27T02:59:50.041782Z [http] [httpserver.cpp:255] [http_request_cb] [http] Received a POST request for / from 127.0.0.1:50492 
 1 node0 2023-09-27T02:59:50.041968Z [httpworker.0] [rpc/request.cpp:181] [parse] [rpc] ThreadRPCServer method=getbestblockhash user=__cookie__ 
 2 node2 2023-09-27T02:59:50.042734Z [http] [httpserver.cpp:255] [http_request_cb] [http] Received a POST request for / from 127.0.0.1:37528 
 3 node2 2023-09-27T02:59:50.042872Z [httpworker.0] [rpc/request.cpp:181] [parse] [rpc] ThreadRPCServer method=getbestblockhash user=__cookie__ 
 4 node0 2023-09-27T02:59:50.043575Z [http] [httpserver.cpp:255] [http_request_cb] [http] Received a POST request for / from 127.0.0.1:50492 
 5 node0 2023-09-27T02:59:50.043722Z [httpworker.2] [rpc/request.cpp:181] [parse] [rpc] ThreadRPCServer method=gettransaction user=__cookie__ 
 6 node0 2023-09-27T02:59:50.045190Z [http] [httpserver.cpp:255] [http_request_cb] [http] Received a POST request for / from 127.0.0.1:50492 
 7 node0 2023-09-27T02:59:50.045301Z [httpworker.1] [rpc/request.cpp:181] [parse] [rpc] ThreadRPCServer method=gettransaction user=__cookie__ 
 8 test  2023-09-27T02:59:50.047000Z TestFramework (ERROR): Assertion failed 
 9                                   Traceback (most recent call last):
10                                     File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 131, in main
11                                       self.run_test()
12                                     File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/wallet_reorgsrestore.py", line 76, in run_test
13                                       assert_equal(conflicted["confirmations"], -9)
14                                     File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/util.py", line 57, in assert_equal
15                                       raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
16                                   AssertionError: not(-8 == -9)

maflcko added this to the milestone 26.0 on Sep 27, 2023

ryanofsky approved

ryanofsky commented at 2:51 pm on September 27, 2023: contributor

Code review ACK 782701ce7d31919dba2241ee43b582d8ae5a2541. Nice catch, and clever test (grinding the txid)

achow101 commented at 3:02 pm on September 27, 2023: member

Can’t reproduce the CI failure.

maflcko commented at 3:09 pm on September 27, 2023: member

Can’t reproduce the CI failure.

10 re-runs were scheduled. Let’s see what happens.

in src/wallet/wallet.cpp:1346 in 782701ce7d

1338@@ -1339,11 +1339,14 @@ void CWallet::MarkConflicted(const uint256& hashBlock, int conflicting_height, c
1339 {
1340     LOCK(cs_wallet);
1341 
1342-    int conflictconfirms = (m_last_block_processed_height - conflicting_height + 1) * -1;
1343     // If number of conflict confirms cannot be determined, this means
1344     // that the block is still unknown or not yet part of the main chain,
1345     // for example when loading the wallet during a reindex. Do nothing in that
1346     // case.
1347+    if (m_last_block_processed_height < 0 || conflicting_height < 0) {

furszy commented at 4:11 pm on September 27, 2023:

Do you know when conflicting_height can be negative? It looks like a different bug if that happens.

As far as I can see, we only set the TxStateConflicted state at AddToWalletIfInvolvingMe, where confirmed_block_height is always positive. In LoadToWallet, we take the conflicting height of the prev tx, so it should never be negative.

achow101 commented at 5:03 pm on September 27, 2023:

When the CWalletTx is loaded, it only contains the hash of conflicting block. The height will be set to -1. LoadToWallet will try to lookup the block by hash, but if there is not chain available (as with the wallet tool and during migration), then it cannot do that. So the height will remain -1. If that transaction has a child, and that child is loaded after the parent is loaded, then MarkConflicted will be called using the parent’s in-memory conflicted state. Since there was no chain, the conflicting_height that is passed to MarkConflicted is -1, hence triggering this error.

DrahtBot removed the label CI failed on Sep 27, 2023

furszy commented at 2:10 pm on September 29, 2023: member

ACK 782701ce

maflcko commented at 2:41 pm on September 29, 2023: member

Looks like tsan CI passed 10 times. I’ve also run the rpc on more random files I found somewhere, but I haven’t reviewed the code.

Concept ACK.

fanquake commented at 10:17 am on October 2, 2023: member

Looks like we should backport this as well?

fanquake added the label Needs backport (25.x) on Oct 2, 2023

fanquake merged this on Oct 2, 2023

fanquake closed this on Oct 2, 2023

fanquake commented at 12:39 pm on October 2, 2023: member

Added to #28487 for backporting into 25.x.

fanquake removed the label Needs backport (25.x) on Oct 2, 2023

fanquake referenced this in commit d63478cb50 on Oct 2, 2023

fanquake referenced this in commit b3517cb1b5 on Oct 2, 2023

fanquake referenced this in commit 9f8d501cb8 on Oct 4, 2023

Frank-GER referenced this in commit 3ddbf89c5a on Oct 5, 2023

bitcoin locked this on Oct 1, 2024

wallet: Check for uninitialized last processed and conflicting heights in MarkConflicted #28542

Reviews