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

achow101 · 2023-09-27T02:37:19Z

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

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.

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.

DrahtBot · 2023-09-27T02:37:22Z

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.

maflcko · 2023-09-27T06:14:12Z

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

 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 
 node0 2023-09-27T02:59:50.041968Z [httpworker.0] [rpc/request.cpp:181] [parse] [rpc] ThreadRPCServer method=getbestblockhash user=__cookie__ 
 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 
 node2 2023-09-27T02:59:50.042872Z [httpworker.0] [rpc/request.cpp:181] [parse] [rpc] ThreadRPCServer method=getbestblockhash user=__cookie__ 
 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 
 node0 2023-09-27T02:59:50.043722Z [httpworker.2] [rpc/request.cpp:181] [parse] [rpc] ThreadRPCServer method=gettransaction user=__cookie__ 
 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 
 node0 2023-09-27T02:59:50.045301Z [httpworker.1] [rpc/request.cpp:181] [parse] [rpc] ThreadRPCServer method=gettransaction user=__cookie__ 
 test  2023-09-27T02:59:50.047000Z TestFramework (ERROR): Assertion failed 
                                   Traceback (most recent call last):
                                     File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 131, in main
                                       self.run_test()
                                     File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/wallet_reorgsrestore.py", line 76, in run_test
                                       assert_equal(conflicted["confirmations"], -9)
                                     File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/util.py", line 57, in assert_equal
                                       raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
                                   AssertionError: not(-8 == -9)

ryanofsky

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

achow101 · 2023-09-27T15:02:23Z

Can't reproduce the CI failure.

maflcko · 2023-09-27T15:09:36Z

Can't reproduce the CI failure.

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

furszy · 2023-09-27T16:11:59Z

src/wallet/wallet.cpp

    // If number of conflict confirms cannot be determined, this means
    // that the block is still unknown or not yet part of the main chain,
    // for example when loading the wallet during a reindex. Do nothing in that
    // case.
+    if (m_last_block_processed_height < 0 || conflicting_height < 0) {


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.

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.

furszy

ACK 782701c

maflcko · 2023-09-29T14:41:11Z

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 · 2023-10-02T10:17:35Z

Looks like we should backport this as well?

fanquake · 2023-10-02T12:39:49Z

Added to #28487 for backporting into 25.x.

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. Github-Pull: bitcoin#28542 Rebased-From: 4660fc8

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. Github-Pull: bitcoin#28542 Rebased-From: 782701c

45a5fcb http: bugfix: track closed connection (stickies-v) 752a456 http: log connection instead of request count (stickies-v) ae86ada http: refactor: use encapsulated HTTPRequestTracker (stickies-v) f31899d gui: macOS, make appMenuBar part of the main app window (furszy) 64ffa94 gui: macOS, do not process dock icon actions during shutdown (furszy) e270f3f depends: fix unusable memory_resource in macos qt build (fanquake) a668394 build, macos: Fix `qt` package build with new Xcode 15 linker (Hennadii Stepanov) b3517cb test: Test loading wallets with conflicts without a chain (Andrew Chow) d63478c wallet: Check last block and conflict height are valid in MarkConflicted (Andrew Chow) 5e51a9c ci: Nuke Android APK task, Use credits for tsan (MarcoFalke) 910c362 test: ensure old fee_estimate.dat not read on restart and flushed (ismaelsadeeq) 37764d3 tx fees, policy: read stale fee estimates with a regtest-only option (ismaelsadeeq) 16bb916 tx fees, policy: do not read estimates of old fee_estimates.dat (ismaelsadeeq) c4dd598 tx fees, policy: periodically flush fee estimates to fee_estimates.dat (ismaelsadeeq) c36770c test: wallet, verify migration doesn't crash for an invalid script (furszy) 0d2a33e wallet: disallow migration of invalid or not-watched scripts (furszy) 2c51a07 Do not use std::vector = {} to release memory (Pieter Wuille) Pull request description: Further backports for the `25.x` branch. Currently: * #27622 * #27834 * #28125 * #28452 * #28542 * #28543 * #28551 * #28571 * bitcoin-core/gui#751 ACKs for top commit: hebasto: re-ACK 45a5fcb, only #28551 has been backported with since my recent [review](#28487 (review)). dergoegge: reACK 45a5fcb willcl-ark: reACK 45a5fcb Tree-SHA512: 0f5807aa364b7c2a2039fef11d5cd5e168372c3bf5b0e941350fcd92e7db4a1662801b97bb4f68e29788c77d24bbf97385a483c4501ca72d93fa25327d5694fa

…nd conflicting heights in MarkConflicted 782701c test: Test loading wallets with conflicts without a chain (Andrew Chow) 4660fc8 wallet: Check last block and conflict height are valid in MarkConflicted (Andrew Chow) Pull request description: `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 bitcoin#28510 ACKs for top commit: ryanofsky: Code review ACK 782701c. Nice catch, and clever test (grinding the txid) furszy: ACK 782701c Tree-SHA512: 1344e0279ec5413a43a2819d101fb571fbf4821de2d13958a0fdffc99f57082ef3243ec454c8343f97dc02ed1fce8c8b0fd89388420ab2e55618af42ad5630a9

achow101 added 2 commits September 26, 2023 21:28

DrahtBot added Wallet CI failed labels Sep 27, 2023

maflcko added this to the 26.0 milestone Sep 27, 2023

ryanofsky approved these changes Sep 27, 2023

View reviewed changes

furszy reviewed Sep 27, 2023

View reviewed changes

DrahtBot removed the CI failed label Sep 27, 2023

ryanofsky mentioned this pull request Sep 27, 2023

wallet: prevent bugs from invalid transaction heights with asserts, comments, and refactoring #28546

Merged

furszy reviewed Sep 29, 2023

View reviewed changes

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

fanquake merged commit 50f250a into bitcoin:master Oct 2, 2023

fanquake mentioned this pull request Oct 2, 2023

[25.1] Final backports #28487

Merged

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

bitcoin locked and limited conversation to collaborators Oct 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

achow101 commented Sep 27, 2023

Uh oh!

DrahtBot commented Sep 27, 2023 •

edited

Loading

Uh oh!

maflcko commented Sep 27, 2023

Uh oh!

ryanofsky left a comment

Uh oh!

achow101 commented Sep 27, 2023

Uh oh!

maflcko commented Sep 27, 2023

Uh oh!

furszy Sep 27, 2023 •

edited

Loading

Uh oh!

achow101 Sep 27, 2023

Uh oh!

furszy left a comment

Uh oh!

maflcko commented Sep 29, 2023

Uh oh!

fanquake commented Oct 2, 2023

Uh oh!

fanquake commented Oct 2, 2023

Uh oh!

Uh oh!

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

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

Uh oh!

Conversation

achow101 commented Sep 27, 2023

Uh oh!

DrahtBot commented Sep 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Uh oh!

maflcko commented Sep 27, 2023

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented Sep 27, 2023

Uh oh!

maflcko commented Sep 27, 2023

Uh oh!

furszy Sep 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

achow101 Sep 27, 2023

Choose a reason for hiding this comment

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Sep 29, 2023

Uh oh!

fanquake commented Oct 2, 2023

Uh oh!

fanquake commented Oct 2, 2023

Uh oh!

Uh oh!

DrahtBot commented Sep 27, 2023 •

edited

Loading

furszy Sep 27, 2023 •

edited

Loading