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

ryanofsky · 2023-09-27T17:22:33Z

Originally, this PR fixed a wallet migration bug that could cause the watchonly wallet created by legacy wallet migration to have incorrect transaction height values. A different fix for the bug was implemented in #28609, but that PR did not add any test coverage that would have caught the bug, and didn't include other changes from this PR intended to prevent problems from invalid transaction heights.

This PR adds new asserts to catch invalid transaction heights, which would trigger test failures without bugfix in #28609. This PR also refactors code and adds comments to clarify assumptions and make it less likely a bug from invalid transaction height values would be introduced.

DrahtBot · 2023-09-27T17:22:36Z

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	Sjors, furszy, achow101

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#27307 (wallet: track mempool conflicts with wallet transactions by ishaanam)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

ryanofsky · 2023-09-28T17:45:29Z

Updated 3bb3061 -> e03b0fe (pr/mig.1 -> pr/mig.2, compare) just adding code comment

achow101 · 2023-09-28T18:04:05Z

Hmm, I wonder if we should instead make both the watchonly and solvables wallets with empty contexts (no chain), and then reload them when migration is done? That would also solve this problem.

ryanofsky · 2023-09-28T18:29:46Z

Hmm, I wonder if we should instead make both the watchonly and solvables wallets with empty contexts (no chain), and then reload them when migration is done? That would also solve this problem.

Yes, that would be less fragile than the current approach. Also I'm not sure why the main wallet needs to be created with an empty context anyway. It seems like the best solution would just be to create all the wallets normally and not need to reload any of them.

I do think the other changes in this PR make sense no matter how this issue is fixed, though. And I like that the fix implemented in this PR (0648dbb) is just a one line change.

ryanofsky · 2023-09-28T18:42:55Z

Updated e03b0fe -> dbed701 (pr/mig.2 -> pr/mig.3, compare) fixing a comment typo

achow101 · 2023-09-28T18:48:20Z

Yes, that would be less fragile than the current approach. Also I'm not sure why the main wallet needs to be created with an empty context anyway. It seems like the best solution would just be to create all the wallets normally and not need to reload any of them.

It has an empty context so that there isn't any possibility for an inconsistent state if a block and transaction is found.

ryanofsky · 2023-09-28T19:12:27Z

It has an empty context so that there isn't any possibility for an inconsistent state if a block and transaction is found.

I see, so a way to prevent the wallet from changing while it is being migrated.

In that case, I think building all the wallets offline and then reloading them like you suggested does make sense. Would be happy to review a PR if you have an idea of how to implement that. (Feel free to take any changes from this PR if they'd be useful. Or we could fix the bug directly with this PR and then make the change the loading in a later PR.)

achow101 · 2023-10-06T19:52:52Z

ACK dbed701

furszy

It has an empty context so that there isn't any possibility for an inconsistent state if a block and transaction is found.

I see, so a way to prevent the wallet from changing while it is being migrated.

In that case, I think building all the wallets offline and then reloading them like you suggested does make sense. Would be happy to review a PR if you have an idea of how to implement that. (Feel free to take any changes from this PR if they'd be useful. Or we could fix the bug directly with this PR and then make the change the loading in a later PR.)

+1.
Not seeing a reason to create the watch-only and solvables wallets with a chain context then. They could also enter in an inconsistent state if/when a block reorg or transaction is found during migration.

furszy · 2023-10-09T13:37:44Z

src/wallet/wallet.cpp

+                    wtx->updateState(data.watchonly_wallet->chain());
                    // Add to watchonly wallet
                    if (!data.watchonly_wallet->AddToWallet(wtx->tx, wtx->m_state)) {


not really for this PR but.. shouldn't we also need to transfer the other wtx members too?
Like mapValue (for the "replaced_by_txid" and "replaces_txid"), nTimeReceived, nTimeSmart, fFromMe.

nvm, just saw #28609 implementing it.

furszy · 2023-10-11T14:12:59Z

I believe we can move to #28609, it fixes this issue and others in a more comprehensive manner.

achow101 · 2023-10-23T14:36:38Z

Removing from the 26.0 milestone in favor of #28609.

No change in behavior, this just moves code which updates transaction state to a new method so it can be used after offline processes such as wallet migration.

Also document GetTxDepthInMainChain preconditions better

ryanofsky · 2023-10-23T22:15:41Z

Rebased dbed701 -> f06016d (pr/mig.3 -> pr/mig.4, compare) due to conflict with #28609. Also dropped commit cdd6644 which is no longer neccessary after #28609.

Also updated title and PR description since the original bug was fixed #28609, and now this PR is just adding asserts and cleanup to prevent related bugs. Original description was:

bugfix: watchonly wallets created after migration have incorrect height values

During migration, CWalletTx objects that no longer belong to the migrated wallet are moved to a watchonly wallet. But because the migrated wallet is offline (has null m_chain), the confirmed_block_height and conflicting_block_height values in these transactions are -1. These values need to be updated before they are moved to the watchonly wallet, because the watchonly wallet is online, and remains loaded after the migration is complete. Not updating these values could lead to the watchonly wallet returning incorrect information and potentially triggering assert failures.

The problem is only temporary since the height values would be refreshed the next time the watchonly wallet is loaded, but it is worth fixing to avoid unpredictable behavior.

Noticed while reviewing #28542, and looking for other cases where negative confirmed_block_height and conflicting_block_height values might be used.

An easy way to test this bug is to revert the bugfix in the second commit. Then the assert added in the third commit will trigger a crash when getwalletinfo is called in wallet_migration.py

furszy

Do you think the first commit is still necessary? I'm not seeing a reason for doing it when no other piece of code will use it.

ryanofsky · 2023-10-23T23:45:02Z

Do you think the first commit is still necessary? I'm not seeing a reason for doing it when no other piece of code will use it.

I think the first commit is useful to make it clear that some of the CWalletTx fields aren't serialized in the wallet and need to be loaded separately from the blocks database, and to have one function responsible for loading them. Otherwise the fields are just set conditionally in LoadToWallet away from the rest of transaction state code. The new function could also be useful in the future if we want to add more fields to transaction objects to improve conflict tracking, or if we want to add features that avoid keeping all transactions in memory. It's also a move-only change that should be easy to review.

furszy

I think the first commit is useful to make it clear that some of the CWalletTx fields aren't serialized in the wallet and need to be loaded separately from the blocks database, and to have one function responsible for loading them.

Wouldn't be clearer to move the non-serialized members to optional?

But other than that, what make some noise on me about the approach is that we would be "hiding" a cs_main locking cause. Because chain.findBlock() locks the mutex internally.
Which, I know that it is currently not really important, but maybe in the future we would like to batch calls that lock cs_main as much as possible in the wallet.

Still, isn't that I'm against the refactoring, just would like to discuss alternatives.

ryanofsky · 2023-10-24T21:31:26Z

Wouldn't be clearer to move the non-serialized members to optional?

But other than that, what make some noise on me about the approach is that we would be "hiding" a cs_main locking cause. Because chain.findBlock() locks the mutex internally. Which, I know that it is currently not really important, but maybe in the future we would like to batch calls that lock cs_main as much as possible in the wallet.

Very much agree it makes sense to make locking and lookups explicit, and this change helps that, doesn't hurt it. It moves lookups to a function that explicitly is meant to do lookups, and explicitly takes a Chain parameter and doesn't have access to other wallet state. (CWalletTx used to have a CWallet* backpointer, but this was removed in b11a195 to force this explicitness.) The current block lookup code before this PR is in the middle of a CWallet::LoadToWallet function which is messier and is able to do the lookup because it has full access to the monolithic CWallet class.

Taking your batching example, if you wanted to look up the block heights as a batch, instead of between loading each transaction, this change gets closer to that goal, because it moves the lookup code out of the per-transaction LoadToWallet to function to a separate function exactly like you would do if you were making that change.

On your first idea of making heights use std::optional instead of -1 when they are uninitialized, that would be a fine change but it would not make code more explicit or improve code organization, so I see it as being more shallow. It would be complementary though.

furszy · 2023-10-24T22:53:45Z

(CWalletTx used to have a CWallet* backpointer, but this was removed in b11a195 to force this explicitness.)

Thanks for that. Looks ugly.

Taking your batching example, if you wanted to look up the block heights as a batch, instead of between loading each transaction, this change gets closer to that goal, because it moves the lookup code out of the per-transaction LoadToWallet to function to a separate function exactly like you would do if you were making that change.

In this example, I'm not sure if I would place it inside transaction.h. I would rather keep that file exclusively for primitive data.
This way, we can avoid providing access to the chain interface class (or to its declaration) in places that shouldn't have access to it, such as the walletdb.cpp loading process. The function wouldn't be member of CWalletTx; It would probably take the chain + a vector of wtx references for updating.

edit:
still.. thinking it further, walletdb.cpp will continue having access to the chain interface class because it includes wallet.h..
so, cross that point out.

Sjors

utACK f06016d

Sjors · 2023-11-06T02:52:44Z

src/wallet/transaction.cpp

+void CWalletTx::updateState(interfaces::Chain& chain)
+{
+    bool active;
+    auto lookup_block = [&](const uint256& hash, int& height, TxState& state) {


262a78b: could expand this comment:

// Find block by hash and fill in the state height.

furszy

Code review ACK f06016d

Have to admit that I'm still not sure about the first commit, but it isn't blocking if others are happy with it.

achow101 · 2023-11-07T16:23:05Z

ACK f06016d

This was referenced Sep 27, 2023

wallet: Track no-longer-spendable TXOs separately #27865

Draft

wallet: track mempool conflicts with wallet transactions #27307

Merged

maflcko added the Wallet label Sep 28, 2023

maflcko added this to the 26.0 milestone Sep 28, 2023

ryanofsky force-pushed the pr/mig branch from 3bb3061 to e03b0fe Compare September 28, 2023 17:43

ryanofsky force-pushed the pr/mig branch from e03b0fe to dbed701 Compare September 28, 2023 18:42

achow101 requested review from Sjors, furszy and ishaanam October 6, 2023 19:47

DrahtBot mentioned this pull request Oct 7, 2023

wallet: Reload watchonly and solvables wallets after migration #28609

Merged

furszy reviewed Oct 9, 2023

View reviewed changes

achow101 mentioned this pull request Oct 20, 2023

Proposed Timeline for Legacy Wallet and BDB removal #20160

Open

39 tasks

achow101 removed this from the 26.0 milestone Oct 23, 2023

ryanofsky added 2 commits October 23, 2023 17:35

wallet, refactor: Add CWalletTx::updateState function

262a78b

No change in behavior, this just moves code which updates transaction state to a new method so it can be used after offline processes such as wallet migration.

wallet: Add asserts to detect unset transaction height values

f06016d

Also document GetTxDepthInMainChain preconditions better

ryanofsky changed the title ~~bugfix: watchonly wallets created after migration have incorrect height values~~ wallet: prevent bugs from invalid transaction heights with asserts, comments, and refactoring Oct 23, 2023

ryanofsky force-pushed the pr/mig branch from dbed701 to f06016d Compare October 23, 2023 22:13

furszy reviewed Oct 23, 2023

View reviewed changes

DrahtBot added the CI failed label Oct 24, 2023

furszy reviewed Oct 24, 2023

View reviewed changes

DrahtBot removed the CI failed label Oct 25, 2023

Sjors approved these changes Nov 6, 2023

View reviewed changes

DrahtBot requested a review from achow101 November 6, 2023 03:12

furszy reviewed Nov 7, 2023

View reviewed changes

DrahtBot removed the request for review from achow101 November 7, 2023 16:23

achow101 merged commit 3da69c4 into bitcoin:master Nov 7, 2023
16 checks passed

Sjors mentioned this pull request Nov 9, 2023

Show transactions as not fully confirmed during background validation #28616

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ryanofsky commented Sep 27, 2023 •

edited

DrahtBot commented Sep 27, 2023 •

edited

ryanofsky commented Sep 28, 2023

achow101 commented Sep 28, 2023

ryanofsky commented Sep 28, 2023

ryanofsky commented Sep 28, 2023

achow101 commented Sep 28, 2023

ryanofsky commented Sep 28, 2023

achow101 commented Oct 6, 2023

furszy left a comment

furszy Oct 9, 2023 •

edited

furszy Oct 9, 2023

furszy commented Oct 11, 2023

achow101 commented Oct 23, 2023

ryanofsky commented Oct 23, 2023 •

edited

furszy left a comment

ryanofsky commented Oct 23, 2023

furszy left a comment

ryanofsky commented Oct 24, 2023

furszy commented Oct 24, 2023 •

edited

Sjors left a comment

Sjors Nov 6, 2023

furszy left a comment

achow101 commented Nov 7, 2023

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

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

Conversation

ryanofsky commented Sep 27, 2023 • edited

DrahtBot commented Sep 27, 2023 • edited

Code Coverage

Reviews

Conflicts

ryanofsky commented Sep 28, 2023

achow101 commented Sep 28, 2023

ryanofsky commented Sep 28, 2023

ryanofsky commented Sep 28, 2023

achow101 commented Sep 28, 2023

ryanofsky commented Sep 28, 2023

achow101 commented Oct 6, 2023

furszy left a comment

Choose a reason for hiding this comment

furszy Oct 9, 2023 • edited

Choose a reason for hiding this comment

furszy Oct 9, 2023

Choose a reason for hiding this comment

furszy commented Oct 11, 2023

achow101 commented Oct 23, 2023

ryanofsky commented Oct 23, 2023 • edited

furszy left a comment

Choose a reason for hiding this comment

ryanofsky commented Oct 23, 2023

furszy left a comment

Choose a reason for hiding this comment

ryanofsky commented Oct 24, 2023

furszy commented Oct 24, 2023 • edited

Sjors left a comment

Choose a reason for hiding this comment

Sjors Nov 6, 2023

Choose a reason for hiding this comment

furszy left a comment

Choose a reason for hiding this comment

achow101 commented Nov 7, 2023

ryanofsky commented Sep 27, 2023 •

edited

DrahtBot commented Sep 27, 2023 •

edited

furszy Oct 9, 2023 •

edited

ryanofsky commented Oct 23, 2023 •

edited

furszy commented Oct 24, 2023 •

edited