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

ryanofsky commented at 5:22 pm on September 27, 2023: contributor

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 commented at 5:22 pm on September 27, 2023: contributor

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.

maflcko added the label Wallet on Sep 28, 2023

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

ryanofsky force-pushed on Sep 28, 2023

ryanofsky commented at 5:45 pm on September 28, 2023: contributor

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

achow101 commented at 6:04 pm on September 28, 2023: member

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 commented at 6:29 pm on September 28, 2023: contributor

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 (0648dbbe092169b488e4db0d385aaa9d2726ddfe) is just a one line change.

ryanofsky force-pushed on Sep 28, 2023

ryanofsky commented at 6:42 pm on September 28, 2023: contributor

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

achow101 commented at 6:48 pm on September 28, 2023: member

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 commented at 7:12 pm on September 28, 2023: contributor

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 requested review from Sjors on Oct 6, 2023

achow101 requested review from furszy on Oct 6, 2023

achow101 requested review from ishaanam on Oct 6, 2023

achow101 commented at 7:52 pm on October 6, 2023: member

ACK dbed701d75a4a55cb34d797cfbae6910075ebec0

furszy commented at 1:24 pm on October 9, 2023: member

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.

in src/wallet/wallet.cpp:3907 in dbed701d75 outdated

3902+                    // updateState will look up missing height values so the
3903+                    // watchonly wallet can function correctly after the
3904+                    // migration without needing to be reloaded.
3905+                    wtx->updateState(data.watchonly_wallet->chain());
3906                     // Add to watchonly wallet
3907                     if (!data.watchonly_wallet->AddToWallet(wtx->tx, wtx->m_state)) {

furszy commented at 1:37 pm on October 9, 2023:

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.

furszy commented at 2:03 pm on October 9, 2023:

nvm, just saw #28609 implementing it.

furszy commented at 2:12 pm on October 11, 2023: member

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

achow101 commented at 2:36 pm on October 23, 2023: member

Removing from the 26.0 milestone in favor of #28609.

achow101 removed this from the milestone 26.0 on Oct 23, 2023

wallet, refactor: Add CWalletTx::updateState function

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.

262a78b133

wallet: Add asserts to detect unset transaction height values

Also document GetTxDepthInMainChain preconditions better

f06016d77d

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

ryanofsky force-pushed on Oct 23, 2023

ryanofsky commented at 10:15 pm on October 23, 2023: contributor

Rebased dbed701d75a4a55cb34d797cfbae6910075ebec0 -> f06016d77d848133fc6568f287bb86b644c9fa69 (pr/mig.3 -> pr/mig.4, compare) due to conflict with #28609. Also dropped commit cdd66441a0dddefe566da480a7015dbae299b4bb 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 commented at 11:08 pm on October 23, 2023: member

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 commented at 11:45 pm on October 23, 2023: contributor

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.

DrahtBot added the label CI failed on Oct 24, 2023

furszy commented at 8:58 pm on October 24, 2023: member

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 commented at 9:31 pm on October 24, 2023: contributor

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 b11a195ef450bd138aa03204a5e74fdd3ddced26 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 commented at 10:53 pm on October 24, 2023: member

(CWalletTx used to have a CWallet* backpointer, but this was removed in https://github.com/bitcoin/bitcoin/commit/b11a195ef450bd138aa03204a5e74fdd3ddced26 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.

DrahtBot removed the label CI failed on Oct 25, 2023

in src/wallet/transaction.cpp:35 in 262a78b133 outdated

28@@ -25,6 +29,27 @@ int64_t CWalletTx::GetTxTime() const
29     return n ? n : nTimeReceived;
30 }
31 
32+void CWalletTx::updateState(interfaces::Chain& chain)
33+{
34+    bool active;
35+    auto lookup_block = [&](const uint256& hash, int& height, TxState& state) {

Sjors commented at 2:52 am on November 6, 2023:

262a78b13365e5318741187fde431c4974539494: could expand this comment:

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

Sjors approved

Sjors commented at 3:11 am on November 6, 2023: member

utACK f06016d77d848133fc6568f287bb86b644c9fa69

DrahtBot requested review from achow101 on Nov 6, 2023

furszy commented at 2:54 pm on November 7, 2023: member

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 commented at 4:23 pm on November 7, 2023: member

ACK f06016d77d848133fc6568f287bb86b644c9fa69

DrahtBot removed review request from achow101 on Nov 7, 2023

achow101 merged this on Nov 7, 2023

achow101 closed this on Nov 7, 2023

bitcoin locked this on Nov 6, 2024

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

Code Coverage

Reviews

Conflicts