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.

src/Makefile.am

@@ -242,6 +242,7 @@ BITCOIN_CORE_H = \
   util/memory.h \
   util/message.h \
   util/moneystr.h \
+  util/overloaded.h \
   util/rbf.h \
   util/readwritefile.h \
   util/ref.h \

src/Makefile.test.include

@@ -148,6 +148,7 @@ BITCOIN_TESTS =\
 if ENABLE_WALLET
 BITCOIN_TESTS += \
   wallet/test/psbt_wallet_tests.cpp \
+  wallet/test/wallet_transaction_tests.cpp \
   wallet/test/wallet_tests.cpp \
   wallet/test/walletdb_tests.cpp \
   wallet/test/wallet_crypto_tests.cpp \

src/bench/coin_selection.cpp

@@ -18,7 +18,7 @@ static void addCoin(const CAmount& nValue, const CWallet& wallet, std::vector<st
     tx.nLockTime = nextLockTime++; // so all transactions get different hashes
     tx.vout.resize(1);
     tx.vout[0].nValue = nValue;
-    wtxs.push_back(MakeUnique<CWalletTx>(MakeTransactionRef(std::move(tx))));
+    wtxs.push_back(MakeUnique<CWalletTx>(MakeTransactionRef(std::move(tx)), TxStateInactive{}));
 }
 
 // Simple benchmark for wallet coin selection. Note that it maybe be necessary
@@ -74,7 +74,7 @@ static void add_coin(const CAmount& nValue, int nInput, std::vector<OutputGroup>
     CMutableTransaction tx;
     tx.vout.resize(nInput + 1);
     tx.vout[nInput].nValue = nValue;
-    std::unique_ptr<CWalletTx> wtx = MakeUnique<CWalletTx>(MakeTransactionRef(std::move(tx)));
+    std::unique_ptr<CWalletTx> wtx = MakeUnique<CWalletTx>(MakeTransactionRef(std::move(tx)), TxStateInactive{});
     set.emplace_back();
     set.back().Insert(COutput(testWallet, *wtx, nInput, 0, true, true, true).GetInputCoin(), 0, true, 0, 0, false);
     wtxn.emplace_back(std::move(wtx));

src/util/overloaded.h

@@ -0,0 +1,21 @@
+// Copyright (c) 2021 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#ifndef BITCOIN_UTIL_OVERLOADED_H
+#define BITCOIN_UTIL_OVERLOADED_H
+
+#include <optional>
+#include <utility>
+
+//! Overloaded helper for std::visit, useful to write code that switches on a
+//! variant type and triggers a compile error if there are any unhandled cases.
+//!
+//! Implementation comes from and example usage can be found at
+//! 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)
+template<class... Ts> Overloaded(Ts...) -> Overloaded<Ts...>;
+
+#endif // BITCOIN_UTIL_OVERLOADED_H

src/wallet/interfaces.cpp

@@ -83,7 +83,9 @@ WalletTx MakeWalletTx(CWallet& wallet, const CWalletTx& wtx)
 WalletTxStatus MakeWalletTxStatus(const CWallet& wallet, const CWalletTx& wtx)
 {
     WalletTxStatus result;
-    result.block_height = wtx.m_confirm.block_height > 0 ? wtx.m_confirm.block_height : std::numeric_limits<int>::max();
+    int height = wtx.state<TxStateConfirmed>() ? wtx.state<TxStateConfirmed>()->confirmed_block_height :
+                 wtx.state<TxStateConflicted>() ? wtx.state<TxStateConflicted>()->conflicting_block_height : 0;
+    result.block_height = height > 0 ? height : std::numeric_limits<int>::max();
     result.blocks_to_maturity = wallet.GetTxBlocksToMaturity(wtx);
     result.depth_in_main_chain = wallet.GetTxDepthInMainChain(wtx);
     result.time_received = wtx.nTimeReceived;

src/wallet/rpcdump.cpp

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

src/wallet/rpcwallet.cpp

@@ -154,13 +154,13 @@ static void WalletTxToJSON(const CWallet& wallet, const CWalletTx& wtx, UniValue
     entry.pushKV("confirmations", confirms);
     if (wtx.IsCoinBase())
         entry.pushKV("generated", true);
-    if (confirms > 0)
+    if (auto* conf = wtx.state<TxStateConfirmed>())
     {
-        entry.pushKV("blockhash", wtx.m_confirm.hashBlock.GetHex());
-        entry.pushKV("blockheight", wtx.m_confirm.block_height);
-        entry.pushKV("blockindex", wtx.m_confirm.nIndex);
+        entry.pushKV("blockhash", conf->confirmed_block_hash.GetHex());
+        entry.pushKV("blockheight", conf->confirmed_block_height);
+        entry.pushKV("blockindex", conf->position_in_block);
         int64_t block_time;
-        CHECK_NONFATAL(chain.findBlock(wtx.m_confirm.hashBlock, FoundBlock().time(block_time)));
+        CHECK_NONFATAL(chain.findBlock(conf->confirmed_block_hash, FoundBlock().time(block_time)));
         entry.pushKV("blocktime", block_time);
     } else {
         entry.pushKV("trusted", CachedTxIsTrusted(wallet, wtx));

src/wallet/test/coinselector_tests.cpp

@@ -74,7 +74,7 @@ static void add_coin(CWallet& wallet, const CAmount& nValue, int nAge = 6*24, bo
         // so stop vin being empty, and cache a non-zero Debit to fake out IsFromMe()
         tx.vin.resize(1);
     }
-    CWalletTx* wtx = wallet.AddToWallet(MakeTransactionRef(std::move(tx)), /* confirm= */ {});
+    CWalletTx* wtx = wallet.AddToWallet(MakeTransactionRef(std::move(tx)), TxStateInactive{});
     if (fIsFromMe)
     {
         wtx->m_amounts[CWalletTx::DEBIT].Set(ISMINE_SPENDABLE, 1);

src/wallet/test/psbt_wallet_tests.cpp

@@ -22,12 +22,12 @@ BOOST_AUTO_TEST_CASE(psbt_updater_test)
     CDataStream s_prev_tx1(ParseHex("0200000000010158e87a21b56daf0c23be8e7070456c336f7cbaa5c8757924f545887bb2abdd7501000000171600145f275f436b09a8cc9a2eb2a2f528485c68a56323feffffff02d8231f1b0100000017a914aed962d6654f9a2b36608eb9d64d2b260db4f1118700c2eb0b0000000017a914b7f5faf40e3d40a5a459b1db3535f2b72fa921e88702483045022100a22edcc6e5bc511af4cc4ae0de0fcd75c7e04d8c1c3a8aa9d820ed4b967384ec02200642963597b9b1bc22c75e9f3e117284a962188bf5e8a74c895089046a20ad770121035509a48eb623e10aace8bfd0212fdb8a8e5af3c94b0b133b95e114cab89e4f7965000000"), SER_NETWORK, PROTOCOL_VERSION);
     CTransactionRef prev_tx1;
     s_prev_tx1 >> prev_tx1;
-    m_wallet.mapWallet.emplace(std::piecewise_construct, std::forward_as_tuple(prev_tx1->GetHash()), std::forward_as_tuple(prev_tx1));
+    m_wallet.mapWallet.emplace(std::piecewise_construct, std::forward_as_tuple(prev_tx1->GetHash()), std::forward_as_tuple(prev_tx1, TxStateInactive{}));
 
     CDataStream s_prev_tx2(ParseHex("0200000001aad73931018bd25f84ae400b68848be09db706eac2ac18298babee71ab656f8b0000000048473044022058f6fc7c6a33e1b31548d481c826c015bd30135aad42cd67790dab66d2ad243b02204a1ced2604c6735b6393e5b41691dd78b00f0c5942fb9f751856faa938157dba01feffffff0280f0fa020000000017a9140fb9463421696b82c833af241c78c17ddbde493487d0f20a270100000017a91429ca74f8a08f81999428185c97b5d852e4063f618765000000"), SER_NETWORK, PROTOCOL_VERSION);
     CTransactionRef prev_tx2;
     s_prev_tx2 >> prev_tx2;
-    m_wallet.mapWallet.emplace(std::piecewise_construct, std::forward_as_tuple(prev_tx2->GetHash()), std::forward_as_tuple(prev_tx2));
+    m_wallet.mapWallet.emplace(std::piecewise_construct, std::forward_as_tuple(prev_tx2->GetHash()), std::forward_as_tuple(prev_tx2, TxStateInactive{}));
 
     // Add scripts
     CScript rs1;

src/wallet/test/wallet_tests.cpp

@@ -314,14 +314,11 @@ BOOST_FIXTURE_TEST_CASE(coin_mark_dirty_immature_credit, TestChain100Setup)
 {
     CWallet wallet(m_node.chain.get(), "", CreateDummyWalletDatabase());
     auto spk_man = wallet.GetOrCreateLegacyScriptPubKeyMan();
-    CWalletTx wtx(m_coinbase_txns.back());
+    CWalletTx wtx{m_coinbase_txns.back(), TxStateConfirmed{::ChainActive().Tip()->GetBlockHash(), ::ChainActive().Height(), /* position_in_block= */ 0}};
 
     LOCK2(wallet.cs_wallet, spk_man->cs_KeyStore);
     wallet.SetLastBlockProcessed(::ChainActive().Height(), ::ChainActive().Tip()->GetBlockHash());
 
-    CWalletTx::Confirmation confirm(CWalletTx::Status::CONFIRMED, ::ChainActive().Height(), ::ChainActive().Tip()->GetBlockHash(), 0);
-    wtx.m_confirm = confirm;
-
     // Call GetImmatureCredit() once before adding the key to the wallet to
     // cache the current immature credit amount, which is 0.
     BOOST_CHECK_EQUAL(CachedTxGetImmatureCredit(wallet, wtx), 0);
@@ -336,7 +333,7 @@ BOOST_FIXTURE_TEST_CASE(coin_mark_dirty_immature_credit, TestChain100Setup)
 static int64_t AddTx(ChainstateManager& chainman, CWallet& wallet, uint32_t lockTime, int64_t mockTime, int64_t blockTime)
 {
     CMutableTransaction tx;
-    CWalletTx::Confirmation confirm;
+    TxState state = TxStateInactive{};
     tx.nLockTime = lockTime;
     SetMockTime(mockTime);
     CBlockIndex* block = nullptr;
@@ -348,13 +345,13 @@ static int64_t AddTx(ChainstateManager& chainman, CWallet& wallet, uint32_t lock
         block = inserted.first->second;
         block->nTime = blockTime;
         block->phashBlock = &hash;
-        confirm = {CWalletTx::Status::CONFIRMED, block->nHeight, hash, 0};
+        state = TxStateConfirmed{hash, block->nHeight, /* position_in_block= */ 0};
     }
-
-    // If transaction is already in map, to avoid inconsistencies, unconfirmation
-    // 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 */) {
+        // Assign wtx.m_state to simplify test and avoid the need to simulate
+        // reorg events. Without this, AddToWallet asserts false when the same
+        // transaction is confirmed in different blocks.
+        wtx.m_state = state;
         return true;
     })->nTimeSmart;
 }
@@ -532,8 +529,7 @@ class ListCoinsTestingSetup : public TestChain100Setup
         wallet->SetLastBlockProcessed(wallet->GetLastBlockHeight() + 1, ::ChainActive().Tip()->GetBlockHash());
         auto it = wallet->mapWallet.find(tx->GetHash());
         BOOST_CHECK(it != wallet->mapWallet.end());
-        CWalletTx::Confirmation confirm(CWalletTx::Status::CONFIRMED, ::ChainActive().Height(), ::ChainActive().Tip()->GetBlockHash(), 1);
-        it->second.m_confirm = confirm;
+        it->second.m_state = TxStateConfirmed{::ChainActive().Tip()->GetBlockHash(), ::ChainActive().Height(), /* position_in_block= */ 1};
         return it->second;
     }
 

src/wallet/test/wallet_transaction_tests.cpp

@@ -0,0 +1,24 @@
+// Copyright (c) 2021 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include <wallet/transaction.h>
+
+#include <wallet/test/wallet_test_fixture.h>
+
+#include <boost/test/unit_test.hpp>
+
+BOOST_FIXTURE_TEST_SUITE(wallet_transaction_tests, WalletTestingSetup)
+
+BOOST_AUTO_TEST_CASE(roundtrip)
+{
+    for (uint8_t hash = 0; hash < 5; ++hash) {
+        for (int index = -2; index < 3; ++index) {
+            TxState state = TxStateInterpretSerialized(TxStateUnrecognized{uint256{hash}, index});
+            BOOST_CHECK_EQUAL(TxStateSerializedBlockHash(state), uint256{hash});
+            BOOST_CHECK_EQUAL(TxStateSerializedIndex(state), index);
+        }
+    }
+}
+
+BOOST_AUTO_TEST_SUITE_END()

src/wallet/transaction.cpp

@@ -15,7 +15,7 @@ bool CWalletTx::IsEquivalentTo(const CWalletTx& _tx) const
 
 bool CWalletTx::InMempool() const
 {
-    return fInMempool;
+    return state<TxStateInMempool>();
 }
 
 int64_t CWalletTx::GetTxTime() const