refactor: Make CWalletTx sync state type-safe #21206
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ryanofsky
added a commit
to ryanofsky/bitcoin
that referenced
this pull request
Feb 17, 2021
This commit just moves code without making any changes. It can be reviewed with `git log -p -n1 --color-moved=dimmed_zebra` Motivation for this change is to make wallet.cpp/h less monolithic and start to make wallet transaction state tracking comprehensible so bugs in https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Transaction-Conflict-Tracking can be fixed safely without introducing new problems. This commit moves wallet classes and methods that deal with transactions out of wallet.cpp/.h into better organized files: - transaction.cpp/.h - CWalletTx and CMerkleTX class definitions - receive.cpp/.h - functions checking received transactions and computing balances - spend.cpp/.h - functions creating transactions and finding spendable coins After bitcoin#20773, when loading is separated from syncing it will also be possible to move more wallet.cpp/.h functions to: - sync.cpp/.h - functions handling chain notifications and rescanning This commit arranges receive.cpp and spend.cpp functions in dependency order so it's possible to skim receive.cpp and get an idea of how computing balances works, and skim spend.cpp and get an idea of how transactions are created, without having to jump all over wallet.cpp where functions are not in order and there is a lot of unrelated uncode. Followup commit "refactor: Detach wallet transaction methods" in bitcoin#21206 follows up this PR and tweaks function names and arguments to reflect new locations. The two commits are split into separate PRs because this commit is more work to maintain and less work to review, while the other commit is less work to maintain and more work to review, so hopefully this commit can be merged earlier.
ryanofsky
force-pushed
the
pr/txstate
branch
from
eb7814d
to
792d72e
Compare
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
This was referenced Feb 17, 2021
promag
reviewed
Feb 21, 2021
ryanofsky
added a commit
to ryanofsky/bitcoin
that referenced
this pull request
Mar 4, 2021
This commit just moves code without making any changes. It can be reviewed with `git log -p -n1 --color-moved=dimmed_zebra` Motivation for this change is to make wallet.cpp/h less monolithic and start to make wallet transaction state tracking comprehensible so bugs in https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Transaction-Conflict-Tracking can be fixed safely without introducing new problems. This commit moves wallet classes and methods that deal with transactions out of wallet.cpp/.h into better organized files: - transaction.cpp/.h - CWalletTx and CMerkleTX class definitions - receive.cpp/.h - functions checking received transactions and computing balances - spend.cpp/.h - functions creating transactions and finding spendable coins After bitcoin#20773, when loading is separated from syncing it will also be possible to move more wallet.cpp/.h functions to: - sync.cpp/.h - functions handling chain notifications and rescanning This commit arranges receive.cpp and spend.cpp functions in dependency order so it's possible to skim receive.cpp and get an idea of how computing balances works, and skim spend.cpp and get an idea of how transactions are created, without having to jump all over wallet.cpp where functions are not in order and there is a lot of unrelated uncode. Followup commit "refactor: Detach wallet transaction methods" in bitcoin#21206 follows up this PR and tweaks function names and arguments to reflect new locations. The two commits are split into separate PRs because this commit is more work to maintain and less work to review, while the other commit is less work to maintain and more work to review, so hopefully this commit can be merged earlier.
ryanofsky
added a commit
to ryanofsky/bitcoin
that referenced
this pull request
Mar 4, 2021
This commit just moves functions without making any changes. It can be reviewed with `git log -p -n1 --color-moved=dimmed_zebra` Motivation for this change is to make wallet.cpp/h less monolithic and start to make wallet transaction state tracking comprehensible so bugs in https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Transaction-Conflict-Tracking can be fixed safely without introducing new problems. This commit moves wallet classes and methods that deal with transactions out of wallet.cpp/.h into better organized files: - transaction.cpp/.h - CWalletTx and CMerkleTx class definitions - receive.cpp/.h - functions checking received transactions and computing balances - spend.cpp/.h - functions creating transactions and finding spendable coins After bitcoin#20773, when loading is separated from syncing it will also be possible to move more wallet.cpp/.h functions to: - sync.cpp/.h - functions handling chain notifications and rescanning This commit arranges receive.cpp and spend.cpp functions in dependency order so it's possible to skim receive.cpp and get an idea of how computing balances works, and skim spend.cpp and get an idea of how transactions are created, without having to jump all over wallet.cpp where functions are not in order and there is a lot of unrelated code. Followup commit "refactor: Detach wallet transaction methods" in bitcoin#21206 follows up this PR and tweaks function names and arguments to reflect new locations. The two commits are split into separate PRs because this commit is more work to maintain and less work to review, while the other commit is less work to maintain and more work to review, so hopefully this commit can be merged earlier.
This was referenced Nov 2, 2021
Current CWalletTx state representation makes it possible to set inconsistent states that won't be handled correctly by wallet sync code or serialized & deserialized back into the same form. For example, it is possible to call setConflicted without setting a conflicting block hash, or setConfirmed with no transaction index. And it's possible update individual m_confirm and fInMempool data fields without setting an overall consistent state that can be serialized and handled correctly. Fix this without changing behavior by using std::variant, instead of an enum and collection of fields, to represent sync state, so state tracking code is safer and more legible. This is a first step to fixing state tracking bugs https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Transaction-Conflict-Tracking, by adding an extra margin of safety that can prevent new bugs from being introduced as existing bugs are fixed.
ryanofsky
force-pushed
the
pr/txstate
branch
from
3c6fdae
to
78c8ac8
Compare
|
Rebased 3c6fdae -> 78c8ac8 ( |
|
Concept ACK, wil review today. |
promag
reviewed
Nov 23, 2021
There was a problem hiding this comment.
Code review ACK 78c8ac8.
In a follow-up, we can consolidate updates to m_state, for instance, the 2nd update can be avoided:
transactionAddedToMempool
SyncTransaction
AddToWalletIfInvolvingMe
AddToWallet
m_state = ...
RefreshMempoolStatus
m_state = ...
jonatack
reviewed
Nov 23, 2021
ryanofsky
force-pushed
the
pr/txstate
branch
from
78c8ac8
to
d8ee8f3
Compare
ryanofsky
commented
Nov 24, 2021
There was a problem hiding this comment.
Thanks for reviews and suggestions!
Updated 78c8ac8 -> d8ee8f3 (pr/txstate.25 -> pr/txstate.26, compare)
jonatack
reviewed
Nov 24, 2021
| // is needed before confirm again with different block. | ||
| return wallet.AddToWallet(MakeTransactionRef(tx), confirm, [&](CWalletTx& wtx, bool /* new_tx */) { | ||
| wtx.setUnconfirmed(); | ||
| return wallet.AddToWallet(MakeTransactionRef(tx), state, [&](CWalletTx& wtx, bool /* new_tx */) { |
There was a problem hiding this comment.
(if need to retouch)
Suggested change
| return wallet.AddToWallet(MakeTransactionRef(tx), state, [&](CWalletTx& wtx, bool /* new_tx */) { | |
| return wallet.AddToWallet(MakeTransactionRef(tx), state, [&](CWalletTx& wtx, /*new_tx=*/bool) { |
| // serialized transaction. | ||
| wtx.m_confirm.block_height = height; | ||
| } else if (wtx.isConflicted() || wtx.isConfirmed()) { | ||
| auto lookup_block = [&](const uint256& hash, int& height, TxState& state) { | ||
| // If tx block (or conflicting block) was reorged out of chain | ||
| // while the wallet was shutdown, change tx status to UNCONFIRMED | ||
| // and reset block height, hash, and index. ABANDONED tx don't have |
There was a problem hiding this comment.
Should the documentation be updated here as well?
hebasto
reviewed
Jan 4, 2024
| //! https://en.cppreference.com/w/cpp/utility/variant/visit#Example | ||
| template<class... Ts> struct Overloaded : Ts... { using Ts::operator()...; }; | ||
|
|
||
| //! Explicit deduction guide (not needed as of C++20) |
There was a problem hiding this comment.
As for Clang, this explicit deduction guide can be dropped for versions >=17 only, unfortunately.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Current
CWalletTxstate representation makes it possible to set inconsistent states that won't be handled correctly by wallet sync code or serialized & deserialized back into the same form.For example, it is possible to call
setConflictedwithout setting a conflicting block hash, orsetConfirmedwith no transaction index. And it's possible update individualm_confirmandfInMempooldata fields without setting an overall consistent state that can be serialized and handled correctly.Fix this without changing behavior by using
std::variant, instead of an enum and collection of fields, to represent sync state, so state tracking code is safer and more legible.This is a first step to fixing state tracking bugs https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Transaction-Conflict-Tracking, by adding an extra margin of safety that can prevent new bugs from being introduced as existing bugs are fixed.