refactor: Make CWalletTx sync state type-safe

ryanofsky commented at 5:08 am on February 17, 2021: contributor

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.

fanquake added the label Refactoring on Feb 17, 2021

fanquake added the label Wallet on Feb 17, 2021

ryanofsky referenced this in commit 2a855bf092 on Feb 17, 2021

ryanofsky force-pushed on Feb 17, 2021

DrahtBot commented at 10:25 am on February 17, 2021: contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#23497 (Add src/node/ and src/wallet/ code to node:: and wallet:: namespaces by ryanofsky)
#23411 (refactor: Avoid integer overflow in ApplyStats when activating snapshot by MarcoFalke)

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.

DrahtBot added the label Needs rebase on Feb 19, 2021

in src/wallet/transaction.h:136 in 7bc16a0b42 outdated

133@@ -140,8 +134,7 @@ class CWalletTx
134     mutable CAmount nChangeCached;
135 
136     CWalletTx(const CWallet* wallet, CTransactionRef arg)

promag commented at 12:52 pm on February 21, 2021:

7bc16a0b421380a1682ca9d97b7f7fc140fb1c5e

nit, CWalletTx(const CWallet*, CTransactionRef arg) to make it clear wallet is unused.

ryanofsky commented at 0:26 am on March 4, 2021:

re: #21206 (review)

nit, CWalletTx(const CWallet*, CTransactionRef arg) to make it clear wallet is unused.

Good catch! Removed unused argument in this commit instead of later commit

promag commented at 12:56 pm on February 21, 2021: member

Approach ACK (also base PR).

This makes it a lot easier to reason about transaction state. Another great achievement is dropping wallet dependency from transaction. 👏

ryanofsky referenced this in commit 11dd250fb1 on Mar 4, 2021

ryanofsky referenced this in commit 01f4bb2693 on Mar 4, 2021

ryanofsky force-pushed on Mar 4, 2021

ryanofsky commented at 0:41 am on March 4, 2021: contributor

Rebased 792d72e2a17fa34e173a82171bdaee3ba88f3e74 -> a4bc501a00293a12f0f6d44d85ffb324823b472d (pr/txstate.2 -> pr/txstate.3, compare) on top of #21207 pr/txmove.2 Rebased a4bc501a00293a12f0f6d44d85ffb324823b472d -> 56aeb9ff7d5942fdb010db3773e26233e0d1612d (pr/txstate.3 -> pr/txstate.4, compare) on top of #21207 pr/txmove.3 Rebased 56aeb9ff7d5942fdb010db3773e26233e0d1612d -> 4c5f91eaf834e3eacc480c8d277e64aebc5ebd4e (pr/txstate.4 -> pr/txstate.5, compare) due to conflicts with #21331 and #18842 Rebased 4c5f91eaf834e3eacc480c8d277e64aebc5ebd4e -> ed04456421d8092457fbefd70b77c13a53a3d20f (pr/txstate.5 -> pr/txstate.6, compare) on top of #21207 pr/txmove.4 due to conflict with #21141 Rebased ed04456421d8092457fbefd70b77c13a53a3d20f -> 045e712a489ed8df4cc15f29a059c8f661759e72 (pr/txstate.6 -> pr/txstate.7, compare) on top of #21207 pr/txmove.5 Rebased 045e712a489ed8df4cc15f29a059c8f661759e72 -> 4e11f88320b644b67db55fe737815451ca7d0681 (pr/txstate.7 -> pr/txstate.8, compare) due to conflict with #21666

DrahtBot removed the label Needs rebase on Mar 4, 2021

DrahtBot added the label Needs rebase on Mar 8, 2021

ryanofsky referenced this in commit 74d45298e5 on Mar 8, 2021

ryanofsky referenced this in commit 065e94fd97 on Mar 8, 2021

ryanofsky force-pushed on Mar 8, 2021

DrahtBot removed the label Needs rebase on Mar 9, 2021

DrahtBot added the label Needs rebase on Mar 10, 2021

ryanofsky referenced this in commit 68e865e6b7 on Mar 10, 2021

ryanofsky force-pushed on Mar 10, 2021

ryanofsky referenced this in commit 8da243cc98 on Mar 10, 2021

DrahtBot removed the label Needs rebase on Mar 10, 2021

DrahtBot added the label Needs rebase on Mar 12, 2021

ryanofsky referenced this in commit e7eebeb6d1 on Mar 21, 2021

ryanofsky referenced this in commit 7c188f3508 on Mar 21, 2021

ryanofsky force-pushed on Mar 21, 2021

DrahtBot removed the label Needs rebase on Mar 22, 2021

DrahtBot added the label Needs rebase on Apr 13, 2021

ryanofsky referenced this in commit a71a7db9a9 on Apr 13, 2021

ryanofsky referenced this in commit 9b6bdfb08a on Apr 13, 2021

ryanofsky force-pushed on Apr 13, 2021

DrahtBot removed the label Needs rebase on Apr 13, 2021

DrahtBot added the label Needs rebase on Apr 14, 2021

ryanofsky force-pushed on Apr 14, 2021

DrahtBot removed the label Needs rebase on Apr 14, 2021

in src/wallet/interfaces.cpp:85 in 4e11f88320 outdated

80@@ -81,7 +81,9 @@ WalletTx MakeWalletTx(CWallet& wallet, const CWalletTx& wtx)
81 WalletTxStatus MakeWalletTxStatus(const CWallet& wallet, const CWalletTx& wtx)
82 {
83     WalletTxStatus result;
84-    result.block_height = wtx.m_confirm.block_height > 0 ? wtx.m_confirm.block_height : std::numeric_limits<int>::max();
85+    int height = wtx.state<TxStateConfirmed>() ? wtx.state<TxStateConfirmed>()->confirmed_block_height :

promag commented at 1:12 pm on April 26, 2021:

4e11f88320b644b67db55fe737815451ca7d0681

nit, refactor to wtx.GetBlockHeight() or similar.

ryanofsky commented at 3:22 pm on April 30, 2021:

re: #21206 (review)

4e11f88

nit, refactor to wtx.GetBlockHeight() or similar.

The code does repeat a few places but I think I’d still rather not have this. I think all these positive and negative height and magic height and depth values are bad and we should just pass the actual state information around. It would be good to clean up this code later (would avoid it for now to avoid expanding scope of pr), but I think the cleanup should more try to get rid of the magic height values than to try to construct them quietly.

in src/wallet/wallet.cpp:809 in 4e11f88320 outdated

787@@ -788,7 +788,9 @@ bool CWallet::MarkReplaced(const uint256& originalHash, const uint256& newHash)
788     // notification so the wallet is in an internally consistent state and
789     // immediately knows the old transaction should not be considered trusted
790     // and is eligible to be abandoned
791-    wtx.fInMempool = chain().isInMempool(originalHash);
792+    if (wtx.state<TxStateInMempool>() && !chain().isInMempool(originalHash)) {

promag commented at 1:16 pm on April 26, 2021:

4e11f88320b644b67db55fe737815451ca7d0681

Why the first check?

ryanofsky commented at 3:55 pm on April 30, 2021:

re: #21206 (review)

4e11f88

Why the first check?

The main reason is to be equivalent as possible to previous code. Previous code didn’t change the transactions abandon/unconfirmed/conflicted state and this code isn’t trying to do that either. Also, I think it makes the code more closely match the comment above. Maybe we can make the comment more clear though. It seems like it could be a little more specific but I’m not sure what extra explanation or details might be good to add.

in src/wallet/wallet.cpp:1000 in 4e11f88320 outdated

973-        if (chain().findBlock(wtx.m_confirm.hashBlock, FoundBlock().inActiveChain(active).height(height)) && active) {
974-            // Update cached block height variable since it not stored in the
975-            // serialized transaction.
976-            wtx.m_confirm.block_height = height;
977-        } else if (wtx.isConflicted() || wtx.isConfirmed()) {
978+        auto lookup_block = [&](const uint256& hash, int& height, TxState& state) {

promag commented at 2:09 pm on April 26, 2021:

4e11f88320b644b67db55fe737815451ca7d0681

nit, lookup_block seems misleading, maybe update_tx_state or mark_inactive.

ryanofsky commented at 4:18 pm on April 30, 2021:

re: #21206 (review)

4e11f88

nit, lookup_block seems misleading, maybe update_tx_state or mark_inactive.

I guess it’s not too important because the definition is a few lines away from the usage, but those names seem more misleading to me. The lambda is looking up a block hash and returning the block height in an output parameter. In the error case it also returns a txstate. I think it’s reasonable to name a function after the main thing it does, even if the name doesn’t encompass everything it does in all cases, especially in a local context like this where it’s not going to be called by faraway code.

This code definitely sucks and there will be a lot of opportunities to improve it later (see https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Transaction-Conflict-Tracking). This commit is just trying to make a simple-as-possible global change to the state representation in order to be able to make local improvements in places like this more easy in the future.

in src/util/overloaded.h:12 in 4e11f88320 outdated

 7+
 8+#include <optional>
 9+#include <utility>
10+
11+//! Overloaded helper for std::visit, useful to write code that switches on a
12+//! variant type and triggers a compile error if there are any unhandled cases.

promag commented at 2:16 pm on April 26, 2021:

4e11f88320b644b67db55fe737815451ca7d0681

I’ve tested this removing one case in TxStateSerializedBlockHash and it failed to compile.

ryanofsky commented at 3:12 pm on April 30, 2021:

re: #21206 (review)

4e11f88

I’ve tested this removing one case in TxStateSerializedBlockHash and it failed to compile.

Thanks for testing!

maflcko commented at 8:23 am on July 1, 2021:

Just for reference, our existing use of std::visit will also throw compile errors even without this helper. I guess the goal of the helper is to having to write less code in the switch itself?

ryanofsky commented at 9:00 am on July 1, 2021:

Just for reference, our existing use of std::visit will also throw compile errors even without this helper. I guess the goal of the helper is to having to write less code in the switch itself?

I think all existing uses of uses of std::visit use custom visitor classes which don’t have access to local variables or anything. Or at least they did at the time this was written which was shortly after we enabled c++17. If it makes sense to define visitor classes for a particular variant type, this isn’t really meant to be an alternative to that. This is just meant to be an alternative to using if/else if statements or a switch statement. I can update the comment to make it more clear, unless you’re suggesting there might be a better way to use std::visit without this helper. In that case we could get rid of this.

maflcko commented at 10:21 am on July 2, 2021:

Not saying that this way is better, just saying that it will also emit compile errors even without the helper:

 0diff --git a/src/wallet/transaction.h b/src/wallet/transaction.h
 1index 7dc91bebd8..5a717e498c 100644
 2--- a/src/wallet/transaction.h
 3+++ b/src/wallet/transaction.h
 4@@ -8,12 +8,11 @@
 5 #include <amount.h>
 6 #include <primitives/transaction.h>
 7 #include <serialize.h>
 8-#include <wallet/ismine.h>
 9 #include <threadsafety.h>
10 #include <tinyformat.h>
11-#include <util/overloaded.h>
12 #include <util/strencodings.h>
13 #include <util/string.h>
14+#include <wallet/ismine.h>
15 
16 #include <list>
17 #include <variant>
18@@ -85,25 +84,27 @@ static inline TxState TxStateInterpretSerialized(TxStateUnrecognized data)
19 //! Get TxState serialized block hash. Inverse of TxStateInterpretSerialized.
20 static inline uint256 TxStateSerializedBlockHash(const TxState& state)
21 {
22-    return std::visit(Overloaded{
23-        [](const TxStateConfirmed& confirmed) { return confirmed.confirmed_block_hash; },
24-        [](const TxStateConflicted& conflicted) { return conflicted.conflicting_block_hash; },
25-        [](const TxStateInMempool& in_mempool) { return uint256::ZERO; },
26-        [](const TxStateInactive& inactive) { return inactive.abandoned ? uint256::ONE : uint256::ZERO; },
27-        [](const TxStateUnrecognized& unrecognized) { return unrecognized.block_hash; }
28-    }, state);
29+    struct Overloaded {
30+        uint256 operator()(const TxStateConfirmed& confirmed) { return confirmed.confirmed_block_hash; }
31+        uint256 operator()(const TxStateConflicted& conflicted) { return conflicted.conflicting_block_hash; }
32+        uint256 operator()(const TxStateInMempool& in_mempool) { return uint256::ZERO; }
33+        uint256 operator()(const TxStateInactive& inactive) { return inactive.abandoned ? uint256::ONE : uint256::ZERO; }
34+        uint256 operator()(const TxStateUnrecognized& unrecognized) { return unrecognized.block_hash; }
35+    };
36+    return std::visit(Overloaded{}, state);
37 }
38 
39 //! Get TxState serialized block index. Inverse of TxStateInterpretSerialized.
40 static inline int TxStateSerializedIndex(const TxState& state)
41 {
42-    return std::visit(Overloaded{
43-        [](const TxStateConfirmed& confirmed) { return confirmed.position_in_block; },
44-        [](const TxStateConflicted& conflicted) { return -1; },
45-        [](const TxStateInMempool& in_mempool) { return 0; },
46-        [](const TxStateInactive& inactive) { return inactive.abandoned ? -1 : 0; },
47-        [](const TxStateUnrecognized& unrecognized) { return unrecognized.index; }
48-    }, state);
49+    struct Overloaded {
50+        int operator()(const TxStateConfirmed& confirmed) { return confirmed.position_in_block; }
51+        int operator()(const TxStateConflicted& conflicted) { return -1; }
52+        int operator()(const TxStateInMempool& in_mempool) { return 0; }
53+        int operator()(const TxStateInactive& inactive) { return inactive.abandoned ? -1 : 0; }
54+        int operator()(const TxStateUnrecognized& unrecognized) { return unrecognized.index; }
55+    };
56+    return std::visit(Overloaded{}, state);
57 }
58 
59

in src/wallet/transaction.h:50 in 4e11f88320 outdated

45+//! or be abandoned, never broadcast, or rejected from the mempool for another
46+//! reason.
47+struct TxStateInactive {
48+    bool abandoned;
49+
50+    explicit TxStateInactive(bool abandoned=false) : abandoned(abandoned) {}

promag commented at 2:23 pm on April 26, 2021:

4e11f88320b644b67db55fe737815451ca7d0681

nit, space around =.

ryanofsky commented at 3:40 pm on April 30, 2021:

re: #21206 (review)

4e11f88

nit, space around =.

Thanks, ran clang-format-diff

in src/wallet/wallet.cpp:999 in 4e11f88320 outdated

966     if (!fill_wtx(wtx, ins.second)) {
967         return false;
968     }
969     // If wallet doesn't have a chain (e.g wallet-tool), don't bother to update txn.
970     if (HaveChain()) {
971         bool active;

promag commented at 2:25 pm on April 26, 2021:

4e11f88320b644b67db55fe737815451ca7d0681

Make local in lambda below?

ryanofsky commented at 3:58 pm on April 30, 2021:

re: #21206 (review)

4e11f88

Make local in lambda below?

Can do if another change is required. This line isn’t changing, and I’m trying to minimize the diff. The variable scope is pretty small already too.

promag commented at 2:56 pm on April 26, 2021: member

Code review and tested ACK 4e11f88320b644b67db55fe737815451ca7d0681 (including base PR). Some nits for your consideration.

I think this approach is superior simply because it prevents state inconsistency. Maybe a followup could be to make state members const.

Another nice touch in this PR is SyncTxState type which makes it clear which states are handled/accepted in SyncTransaction.

DrahtBot added the label Needs rebase on Apr 29, 2021

ryanofsky referenced this in commit c9e8a508e5 on Apr 30, 2021

ryanofsky referenced this in commit 4984c4548c on Apr 30, 2021

ryanofsky force-pushed on Apr 30, 2021

ryanofsky commented at 6:15 pm on April 30, 2021: contributor

Thanks for the review! I really appreciate it on a noisy PR like this where there are so many small changes.

Rebased 4e11f88320b644b67db55fe737815451ca7d0681 -> a5a537fd95084a2e2912d9625cef41f5f6732efc (pr/txstate.8 -> pr/txstate.9, compare) on top of #21207 pr/txmove.6 due to conflict with #21759, also making some suggested changes

DrahtBot removed the label Needs rebase on Apr 30, 2021

in src/wallet/receive.h:24 in d1582cdc4e outdated

19+CAmount TxGetCredit(const CWallet& wallet, const CTransaction& tx, const isminefilter& filter);
20+
21+bool ScriptIsChange(const CWallet& wallet, const CScript& script) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);
22+bool OutputIsChange(const CWallet& wallet, const CTxOut& txout) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);
23+CAmount OutputGetChange(const CWallet& wallet, const CTxOut& txout) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);
24+CAmount TxGetChange(const CWallet& wallet, const CTransaction& tx);

S3RK commented at 7:50 am on May 4, 2021:

No need to export OutputGetChange and TxGetChange as they are not used outside of receive.cpp

ryanofsky commented at 10:22 pm on May 20, 2021:

re: #21206 (review)

No need to export OutputGetChange and TxGetChange as they are not used outside of receive.cpp

I think in the future it might be good to move low-level code using IsChange logic inside receive.cpp so IsChange functions could be private. But for GetChange it seems most consistent to try to keeping exposing Get{Credit,Debit,Change} functions as a group. The fact that these two GetChange functions don’t have callers I think is more a reflection of poor unit test coverage than that change amounts should be a private part of transaction accounting.

DrahtBot added the label Needs rebase on May 10, 2021

S3RK commented at 6:33 am on May 19, 2021: contributor

Not a full review, just a small thing I’ve noticed looking at #21207

ryanofsky referenced this in commit 264b945fa7 on May 20, 2021

ryanofsky referenced this in commit 0415a53c9c on May 20, 2021

ryanofsky force-pushed on May 20, 2021

DrahtBot removed the label Needs rebase on May 20, 2021

ryanofsky commented at 10:27 pm on May 20, 2021: contributor

Thanks for checking this!

Rebased a5a537fd95084a2e2912d9625cef41f5f6732efc -> 3416ade015914ddeb84fad23d2455d974e9f4a74 (pr/txstate.9 -> pr/txstate.10, compare) on top of #21207 pr/txmove.7 due to conflicts Rebased 3416ade015914ddeb84fad23d2455d974e9f4a74 -> 7cc2549759c12182063fce56161ce4403502312e (pr/txstate.10 -> pr/txstate.11, compare) on top of #21207 pr/txmove.8 due to conflicts with #17331 Rebased 7cc2549759c12182063fce56161ce4403502312e -> c2b3b4b184148447b695ff6ec56bbc2c24447655 (pr/txstate.11 -> pr/txstate.12, compare) on top of #22100 pr/txfun.1 after #21207 merge Rebased c2b3b4b184148447b695ff6ec56bbc2c24447655 -> c00b540d3e315d52f171678e3e6d5e36211d3c38 (pr/txstate.12 -> pr/txstate.13, compare) on top of #22100 pr/txfun.2 Rebased c00b540d3e315d52f171678e3e6d5e36211d3c38 -> b71a25b492d8ded339351ed770a4360ec2316b6c (pr/txstate.13 -> pr/txstate.14, compare) on top of #22100 pr/txfun.3 due to conflict with #21866 Rebased b71a25b492d8ded339351ed770a4360ec2316b6c -> 730df28e00822f06c9fc11c86a74f764a2f5136c (pr/txstate.14 -> pr/txstate.15, compare) on top of #22100 pr/txfun.4

DrahtBot added the label Needs rebase on May 25, 2021

ryanofsky referenced this in commit ed565f93e2 on May 25, 2021

ryanofsky referenced this in commit ae93b95a68 on May 25, 2021

ryanofsky force-pushed on May 25, 2021

DrahtBot removed the label Needs rebase on May 25, 2021

DrahtBot added the label Needs rebase on May 26, 2021

ryanofsky referenced this in commit c7bd5842e4 on May 26, 2021

ryanofsky referenced this in commit 9ece3479c9 on May 26, 2021

meshcollider referenced this in commit 55a156fca0 on May 30, 2021

ryanofsky force-pushed on May 30, 2021

DrahtBot removed the label Needs rebase on May 30, 2021

DrahtBot added the label Needs rebase on Jun 9, 2021

ryanofsky force-pushed on Jun 11, 2021

DrahtBot removed the label Needs rebase on Jun 11, 2021

DrahtBot added the label Needs rebase on Jun 12, 2021

ryanofsky force-pushed on Jun 14, 2021

DrahtBot removed the label Needs rebase on Jun 14, 2021

DrahtBot added the label Needs rebase on Jun 17, 2021

ryanofsky force-pushed on Jun 17, 2021

DrahtBot removed the label Needs rebase on Jun 18, 2021

rebroad referenced this in commit 7c229d7a74 on Jun 23, 2021

in src/wallet/transaction.h:86 in 730df28e00 outdated

82+    }
83+    return data;
84+}
85+
86+//! Get TxState serialized block hash. Inverse of TxStateInterpretSerialized.
87+static uint256 TxStateSerializedBlockHash(const TxState& state)

jonatack commented at 2:04 pm on June 28, 2021:

730df28e0082 seeing the following warnings when debug-building with debian clang 11

 0./wallet/transaction.h:221:59: warning: field 'tx' will be initialized after field 'm_state' [-Wreorder-ctor]
 1    CWalletTx(CTransactionRef tx, const TxState& state) : tx(std::move(tx)), m_state(state)
 2                                                          ^
 3./wallet/transaction.h:71:16: warning: 'static' function 'TxStateInterpretSerialized' declared in header file should be declared 'static inline' [-Wunneeded-internal-declaration]
 4static TxState TxStateInterpretSerialized(TxStateUnrecognized data)
 5               ^
 6./wallet/transaction.h:86:16: warning: 'static' function 'TxStateSerializedBlockHash' declared in header file should be declared 'static inline' [-Wunneeded-internal-declaration]
 7static uint256 TxStateSerializedBlockHash(const TxState& state)
 8               ^
 9./wallet/transaction.h:98:12: warning: 'static' function 'TxStateSerializedIndex' declared in header file should be declared 'static inline' [-Wunneeded-internal-declaration]
10static int TxStateSerializedIndex(const TxState& state)
11           ^
124 warnings generated.

ryanofsky commented at 10:10 pm on July 1, 2021:

730df28 seeing the following warnings when debug-building with debian clang 11

Thanks! Should be fixed in new push

ryanofsky force-pushed on Jul 1, 2021

ryanofsky commented at 10:13 pm on July 1, 2021: contributor

Updated 730df28e00822f06c9fc11c86a74f764a2f5136c -> c5ee3fb0b1db4095376bcff074741dc7a135ba8a (pr/txstate.15 -> pr/txstate.16, compare) fixing reported compiler warnings and improving std::visit comment Rebased c5ee3fb0b1db4095376bcff074741dc7a135ba8a -> 8f25d3a6b7a6cc308eb2f0ebc944e1dd425902f5 (pr/txstate.16 -> pr/txstate.17, compare) due to conflict with #22492 Rebased 8f25d3a6b7a6cc308eb2f0ebc944e1dd425902f5 -> c5599e514beb28518e4f31f495016cf6d8cd4c24 (pr/txstate.17 -> pr/txstate.18, compare) due to conflict with #22359 Rebased c5599e514beb28518e4f31f495016cf6d8cd4c24 -> 095b1d6f58e2302f229617a2439e5a57e14fe936 (pr/txstate.18 -> pr/txstate.19, compare) on top of pr/txfun.6 #22100 Rebased 095b1d6f58e2302f229617a2439e5a57e14fe936 -> b17ba1c752a65d2f8b3d1485074d08d48c1ffd3e (pr/txstate.19 -> pr/txstate.20, compare) on top of pr/txfun.7 #22100

DrahtBot added the label Needs rebase on Jul 20, 2021

ryanofsky force-pushed on Jul 22, 2021

DrahtBot removed the label Needs rebase on Jul 22, 2021

DrahtBot added the label Needs rebase on Jul 27, 2021

ryanofsky force-pushed on Aug 13, 2021

DrahtBot removed the label Needs rebase on Aug 13, 2021

DrahtBot added the label Needs rebase on Aug 19, 2021

ryanofsky force-pushed on Aug 19, 2021

DrahtBot removed the label Needs rebase on Aug 19, 2021

DrahtBot added the label Needs rebase on Sep 1, 2021

meshcollider referenced this in commit 629c4ab2e3 on Sep 3, 2021

promag commented at 9:28 am on September 3, 2021: member

Base PR is merged, could update OP and rebase to move this change forward.

ryanofsky force-pushed on Sep 3, 2021

ryanofsky commented at 6:34 pm on September 3, 2021: contributor

Base PR is merged, could update OP and rebase to move this change forward.

Thanks, rebased and removed references to base PR in the OP.

Rebased b17ba1c752a65d2f8b3d1485074d08d48c1ffd3e -> 2896ad570523842245d2de77de531776cfcc08d8 (pr/txstate.20 -> pr/txstate.21, compare) after base PR #22100 merge. Rebased 2896ad570523842245d2de77de531776cfcc08d8 -> 541d5f5c775a9aa884c1a8b37fc123933fdbd98e (pr/txstate.21 -> pr/txstate.22, compare) due to conflict with #20591

DrahtBot removed the label Needs rebase on Sep 3, 2021

DrahtBot added the label Needs rebase on Sep 29, 2021

ryanofsky force-pushed on Oct 2, 2021

DrahtBot removed the label Needs rebase on Oct 2, 2021

meshcollider commented at 0:56 am on October 4, 2021: contributor

Code review ACK 541d5f5c775a9aa884c1a8b37fc123933fdbd98e

Looks great!

DrahtBot added the label Needs rebase on Oct 22, 2021

ryanofsky force-pushed on Oct 25, 2021

ryanofsky commented at 4:18 pm on October 25, 2021: contributor

Rebased 541d5f5c775a9aa884c1a8b37fc123933fdbd98e -> bb16e10149c4e1bf2e3d0ee027e9025cd48dd65f (pr/txstate.22 -> pr/txstate.23, compare) due to conflicts with #23288 and #23338 Rebased bb16e10149c4e1bf2e3d0ee027e9025cd48dd65f -> 3c6fdae6ed67426bd9ddb2949a9e66934f71bf0b (pr/txstate.23 -> pr/txstate.24, compare) due to conflict with #23332

DrahtBot removed the label Needs rebase on Oct 25, 2021

DrahtBot added the label Needs rebase on Oct 26, 2021

janus referenced this in commit 6856f8c5c2 on Oct 29, 2021

ryanofsky force-pushed on Oct 29, 2021

DrahtBot removed the label Needs rebase on Oct 29, 2021

DrahtBot added the label Needs rebase on Nov 10, 2021

refactor: Make CWalletTx sync state type-safe

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.

d8ee8f3cd3

ryanofsky force-pushed on Nov 15, 2021

ryanofsky commented at 3:15 pm on November 15, 2021: contributor

Rebased 3c6fdae6ed67426bd9ddb2949a9e66934f71bf0b -> 78c8ac87e2f585772f296b943c733bd26f255ba0 (pr/txstate.24 -> pr/txstate.25, compare) due to conflict with #22928, also making minor MakeWalletTxStatus cleanup

DrahtBot removed the label Needs rebase on Nov 15, 2021

meshcollider commented at 1:42 am on November 22, 2021: contributor

re-utACK 78c8ac87e2f585772f296b943c733bd26f255ba0

maflcko requested review from promag on Nov 22, 2021

maflcko requested review from jonatack on Nov 22, 2021

jonatack commented at 2:16 pm on November 22, 2021: contributor

Concept ACK, wil review today.

in src/wallet/rpcdump.cpp:374 in 78c8ac87e2 outdated

368@@ -369,11 +369,9 @@ RPCHelpMan importprunedfunds()
369 
370     unsigned int txnIndex = vIndex[it - vMatch.begin()];
371 
372-    CWalletTx::Confirmation confirm(CWalletTx::Status::CONFIRMED, height, merkleBlock.header.GetHash(), txnIndex);
373-
374     CTransactionRef tx_ref = MakeTransactionRef(tx);
375     if (pwallet->IsMine(*tx_ref)) {
376-        pwallet->AddToWallet(std::move(tx_ref), confirm);
377+        pwallet->AddToWallet(std::move(tx_ref), TxStateConfirmed{merkleBlock.header.GetHash(), height, (int)txnIndex});

jonatack commented at 6:33 pm on November 22, 2021:

if you retouch, per https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Res-casts-named (unless there is a reason to not follow the guideline)

0        pwallet->AddToWallet(std::move(tx_ref), TxStateConfirmed{merkleBlock.header.GetHash(), height, static_cast<int>(txnIndex)});

(this is also the change suggested by clang)

0wallet/rpcdump.cpp:374:104: error: non-constant-expression cannot be narrowed from type 'unsigned int' to 'int' in initializer list [-Wc++11-narrowing]
1        pwallet->AddToWallet(std::move(tx_ref), TxStateConfirmed{merkleBlock.header.GetHash(), height, txnIndex});
2                                                                                                       ^~~~~~~~
3wallet/rpcdump.cpp:374:104: note: insert an explicit cast to silence this issue
4        pwallet->AddToWallet(std::move(tx_ref), TxStateConfirmed{merkleBlock.header.GetHash(), height, txnIndex});
5                                                                                                       ^~~~~~~~
6                                                                                                       static_cast<int>( )
71 error generated.

ryanofsky commented at 11:51 pm on November 23, 2021:

re: #21206 (review)

Ok. I don’t see how this helps readability or could help catching errors here, but I guess I’ll give benefit of the doubt and change.

0wallet/rpcdump.cpp:374:104: error: non-constant-expression cannot be narrowed from type 'unsigned int' to 'int' in initializer list [-Wc++11-narrowing]

This warning doesn’t happen with either cast, so it doesn’t seem like a good reason to chose the more verbose cast.

I think when you have a specific point to make, it’s good to cite an external guideline to help make the point. But I don’t think it’s good to just parrot a guideline without saying what problem it’s addressing in the relevant context.

jonatack commented at 11:45 am on November 24, 2021:

Yes, the developer-notes mention to follow the CppCoreGuidelines and there are some advantages to named casts (https://github.com/bitcoin/bitcoin/pull/20965#issuecomment-770375623) but I do hesitate to mention them due to the verbosity (though if casts are seen as a code smell, verbosity could be a good thing, I guess). Thanks!

in src/wallet/wallet.h:579 in 78c8ac87e2 outdated

575@@ -576,7 +576,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
576     void CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::vector<std::pair<std::string, std::string>> orderForm);
577 
578     /** Pass this transaction to node for mempool insertion and relay to peers if flag set to true */
579-    bool SubmitTxMemoryPoolAndRelay(const CWalletTx& wtx, std::string& err_string, bool relay) const;
580+    bool SubmitTxMemoryPoolAndRelay(CWalletTx& wtx, std::string& err_string, bool relay) const;

jonatack commented at 6:55 pm on November 22, 2021:

Perhaps update the Doxygen documentation in the previous line to explain why wtx is after this change mutable / an out-param.

ryanofsky commented at 1:06 am on November 24, 2021:

re: #21206 (review)

Perhaps update the Doxygen documentation in the previous line to explain why wtx is after this change mutable / an out-param.

I’ll add your text if you want to make a specific suggestion, but mentioning the wtx state update seems like an unusual detail for this summary comment to focus on when it doesn’t going into other details like this, and there are already lots of comments about refreshing the in-mempool state. Just changing this to a mutable reference already seems more straightforward than the previous version which used a const reference and mutable state member.

in src/wallet/wallet.cpp:1644 in 78c8ac87e2 outdated

1640@@ -1645,7 +1641,7 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc
1641                 break;
1642             }
1643             for (size_t posInBlock = 0; posInBlock < block.vtx.size(); ++posInBlock) {
1644-                SyncTransaction(block.vtx[posInBlock], {CWalletTx::Status::CONFIRMED, block_height, block_hash, (int)posInBlock}, fUpdate, /* rescanning_old_block */ true);
1645+                SyncTransaction(block.vtx[posInBlock], TxStateConfirmed{block_hash, block_height, (int)posInBlock}, fUpdate, /* rescanning_old_block */ true);

jonatack commented at 7:04 pm on November 22, 2021:

Here and elsewhere in this commit, maybe format the named args to be compatible with clang-tidy in a way consistent with current changes in the codebase (see #23546 and #23545).

0                SyncTransaction(block.vtx[posInBlock], TxStateConfirmed{block_hash, block_height, static_cast<int>(posInBlock)}, fUpdate, /*rescanning_old_block=*/true);

also, use a named cast for posInBlock

ryanofsky commented at 0:35 am on November 24, 2021:

re: #21206 (review)

Here and elsewhere in this commit, maybe format the named args to be compatible with clang-tidy in a way consistent with current changes in the codebase (see #23546 and #23545).

also, use a named cast for posInBlock

Ok, made these changes

laanwj commented at 2:01 pm on November 23, 2021: member

I like this refactor, it abstracts the transaction state much better than the boolean-radio-buttons, and agree it makes some kinds of mistake harder to make.

Concept and code review ACK 78c8ac87e2f585772f296b943c733bd26f255ba0 re-ACK d8ee8f3cd32bbfefec931724f5798cbb088ceb6f

in src/wallet/transaction.h:71 in 78c8ac87e2 outdated

66+
67+//! Subset of states transaction sync logic is implemented to handle.
68+using SyncTxState = std::variant<TxStateConfirmed, TxStateInMempool, TxStateInactive>;
69+
70+//! Interpret TxState serialized fields as a recognized state.
71+static inline TxState TxStateInterpretSerialized(TxStateUnrecognized data)

jonatack commented at 6:09 pm on November 23, 2021:

Seems odd to pass a TxStateUnrecognized data type for a recognized state. Edit: maybe suggest the fields are “converted from an unrecognized to recognized state.”

ryanofsky commented at 0:05 am on November 24, 2021:

re: #21206 (review)

Seems odd to pass a TxStateUnrecognized data type for a recognized state. Edit: maybe suggest the fields are “converted from an unrecognized to recognized state.”

Thanks, rephrased now.

Just to explain the previous version: “Interpret TxState serialized fields as a recognized state” was supposed to be read like “Interpret serialized bytes as an integer”

in src/wallet/transaction.h:93 in 78c8ac87e2 outdated

88+    return std::visit(Overloaded{
89+        [](const TxStateConfirmed& confirmed) { return confirmed.confirmed_block_hash; },
90+        [](const TxStateConflicted& conflicted) { return conflicted.conflicting_block_hash; },
91+        [](const TxStateInMempool& in_mempool) { return uint256::ZERO; },
92+        [](const TxStateInactive& inactive) { return inactive.abandoned ? uint256::ONE : uint256::ZERO; },
93+        [](const TxStateUnrecognized& unrecognized) { return unrecognized.block_hash; }

jonatack commented at 6:11 pm on November 23, 2021:

nit, perhaps order the state methods above in the order used here and in the next function, or vice versa.

ryanofsky commented at 0:09 am on November 24, 2021:

re: #21206 (review)

nit, perhaps order the state methods above in the order used here and in the next function, or vice versa.

Good suggestion. Fixed!

(Assuming you meant “previous function” instead of “next function” since next function already used the same order)

promag commented at 8:48 pm on November 23, 2021: member

Code review ACK 78c8ac87e2f585772f296b943c733bd26f255ba0.

In a follow-up, we can consolidate updates to m_state, for instance, the 2nd update can be avoided:

0transactionAddedToMempool
1    SyncTransaction
2        AddToWalletIfInvolvingMe
3            AddToWallet
4                m_state = ...
5    RefreshMempoolStatus
6        m_state = ...

in src/wallet/transaction.h:132 in 78c8ac87e2 outdated

210@@ -129,44 +211,12 @@ class CWalletTx
211         nTimeSmart = 0;
212         fFromMe = false;
213         fChangeCached = false;
214-        fInMempool = false;

jonatack commented at 9:19 pm on November 23, 2021:

There appear to be remaining fInMempool comments that may need updating.

 0src/wallet/wallet.cpp:1733:    // We must set fInMempool here - while it will be re-set to true by the
 1src/wallet/wallet.cpp-1734-    // entered-mempool callback, if we did not there would be a race where a
 2src/wallet/wallet.cpp-1735-    // user could call sendmoney in a loop and hit spurious out of funds errors
 3src/wallet/wallet.cpp-1736-    // because we think that this newly generated transaction's change is
 4src/wallet/wallet.cpp-1737-    // unavailable as we're not yet aware that it is in the mempool.
 5src/wallet/wallet.cpp-1738-    //
 6src/wallet/wallet.cpp:1739:    // Irrespective of the failure reason, un-marking fInMempool
 7src/wallet/wallet.cpp-1740-    // out-of-order is incorrect - it should be unmarked when
 8src/wallet/wallet.cpp-1741-    // TransactionRemovedFromMempool fires.
 9--
10src/wallet/wallet.cpp:1973    // Get the inserted-CWalletTx from mapWallet so that the
11src/wallet/wallet.cpp:1974    // fInMempool flag is cached properly

ryanofsky commented at 0:12 am on November 24, 2021:

re: #21206 (review)

There appear to be remaining fInMempool comments that may need updating.

Nice catch! Updated these

in src/util/overloaded.h:9 in 78c8ac87e2 outdated

0@@ -0,0 +1,23 @@
1+// Copyright (c) 2021 The Bitcoin Core developers
2+// Distributed under the MIT software license, see the accompanying
3+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
4+
5+#ifndef BITCOIN_UTIL_OVERLOADED_H
6+#define BITCOIN_UTIL_OVERLOADED_H
7+
8+#include <optional>
9+#include <utility>

jonatack commented at 9:22 pm on November 23, 2021:

Should these two includes be replaced by #include <variant>?

ryanofsky commented at 11:34 pm on November 23, 2021:

re: #21206 (review)

Should these two includes be replaced by #include <variant>?

Good catch. I think these were just copied and pasted from another header. The variant type is not actually referenced so I believe no includes are necessary.

jonatack commented at 9:26 pm on November 23, 2021: contributor

Approach ACK / light code review ACK 78c8ac87e2f585772f296b943c733bd26f255ba0

in src/wallet/wallet.cpp:1000 in 78c8ac87e2 outdated

1002-        if (chain().findBlock(wtx.m_confirm.hashBlock, FoundBlock().inActiveChain(active).height(height)) && active) {
1003-            // Update cached block height variable since it not stored in the
1004-            // serialized transaction.
1005-            wtx.m_confirm.block_height = height;
1006-        } else if (wtx.isConflicted() || wtx.isConfirmed()) {
1007+        auto lookup_block = [&](const uint256& hash, int& height, TxState& state) {

jonatack commented at 9:33 pm on November 23, 2021:

0        auto lookup_block = [&](const uint256& hash, int height, TxState& state) {

ryanofsky commented at 0:34 am on November 24, 2021:

re: #21206 (review)

Sorry, this is an output variable, so making this change would leave heights unset. I want to scrap this code completely after this PR (see previous review comments), but the goal for this PR is to make the smallest possible code changes in the wallet everywhere that switch to a new CWalletTx state representation without changing behavior. Then future PRs can clean up the code and nonsensical behavior in individual places like this.

jonatack commented at 11:23 am on November 24, 2021:

Oh ok, thanks. I didn’t see that findBlock() uses height as an out-param, but now had a better look at the FoundBlock class.

ryanofsky force-pushed on Nov 24, 2021

ryanofsky commented at 1:26 am on November 24, 2021: contributor

Thanks for reviews and suggestions!

Updated 78c8ac87e2f585772f296b943c733bd26f255ba0 -> d8ee8f3cd32bbfefec931724f5798cbb088ceb6f (pr/txstate.25 -> pr/txstate.26, compare)

in src/wallet/test/wallet_tests.cpp:369 in d8ee8f3cd3

370-
371-    // If transaction is already in map, to avoid inconsistencies, unconfirmation
372-    // is needed before confirm again with different block.
373-    return wallet.AddToWallet(MakeTransactionRef(tx), confirm, [&](CWalletTx& wtx, bool /* new_tx */) {
374-        wtx.setUnconfirmed();
375+    return wallet.AddToWallet(MakeTransactionRef(tx), state, [&](CWalletTx& wtx, bool /* new_tx */) {

jonatack commented at 11:03 am on November 24, 2021:

(if need to retouch)

0    return wallet.AddToWallet(MakeTransactionRef(tx), state, [&](CWalletTx& wtx, /*new_tx=*/bool) {

in src/wallet/wallet.cpp:1003 in d8ee8f3cd3

1005-            wtx.m_confirm.block_height = height;
1006-        } else if (wtx.isConflicted() || wtx.isConfirmed()) {
1007+        auto lookup_block = [&](const uint256& hash, int& height, TxState& state) {
1008             // If tx block (or conflicting block) was reorged out of chain
1009             // while the wallet was shutdown, change tx status to UNCONFIRMED
1010             // and reset block height, hash, and index. ABANDONED tx don't have

jonatack commented at 11:19 am on November 24, 2021:

Should the documentation be updated here as well?

jonatack commented at 11:48 am on November 24, 2021: contributor

Code review ACK d8ee8f3cd32bbfefec931724f5798cbb088ceb6f

laanwj merged this on Nov 25, 2021

laanwj closed this on Nov 25, 2021

sidhujag referenced this in commit ebd73c7089 on Nov 26, 2021

bitcoin locked this on Apr 11, 2023

refactor: Make CWalletTx sync state type-safe #21206

Conflicts