Skip to content

Commit cf24152

Browse files
laanwjlaanwj
committed
Merge bitcoin/bitcoin#21206: refactor: Make CWalletTx sync state type-safe
d8ee8f3 refactor: Make CWalletTx sync state type-safe (Russell Yanofsky) Pull request description: 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. ACKs for top commit: laanwj: re-ACK d8ee8f3 jonatack: Code review ACK d8ee8f3 Tree-SHA512: b9f15e9d99dbdbdd3ef7a76764e11f66949f50e6227e284126f209e4cb106af6d55e9a9e8c7d4aa216ddc92c6d5acc6f4aa4746f209bbd77f03831b51a2841c3
2 parents 681b25e + d8ee8f3 commit cf24152

16 files changed

+257
-174
lines changed

‎src/Makefile.am

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -246,6 +246,7 @@ BITCOIN_CORE_H = \
246246
util/macros.h \
247247
util/message.h \
248248
util/moneystr.h \
249+
util/overloaded.h \
249250
util/rbf.h \
250251
util/readwritefile.h \
251252
util/serfloat.h \

‎src/Makefile.test.include

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -161,6 +161,7 @@ BITCOIN_TESTS += \
161161
wallet/test/wallet_tests.cpp \
162162
wallet/test/walletdb_tests.cpp \
163163
wallet/test/wallet_crypto_tests.cpp \
164+
wallet/test/wallet_transaction_tests.cpp \
164165
wallet/test/coinselector_tests.cpp \
165166
wallet/test/init_tests.cpp \
166167
wallet/test/ismine_tests.cpp \

‎src/bench/coin_selection.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ static void addCoin(const CAmount& nValue, const CWallet& wallet, std::vector<st
1818
tx.nLockTime = nextLockTime++; // so all transactions get different hashes
1919
tx.vout.resize(1);
2020
tx.vout[0].nValue = nValue;
21-
wtxs.push_back(std::make_unique<CWalletTx>(MakeTransactionRef(std::move(tx))));
21+
wtxs.push_back(std::make_unique<CWalletTx>(MakeTransactionRef(std::move(tx)), TxStateInactive{}));
2222
}
2323

2424
// Simple benchmark for wallet coin selection. Note that it maybe be necessary

‎src/util/overloaded.h

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
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+
namespace util {
9+
//! Overloaded helper for std::visit. This helper and std::visit in general are
10+
//! useful to write code that switches on a variant type. Unlike if/else-if and
11+
//! switch/case statements, std::visit will trigger compile errors if there are
12+
//! unhandled cases.
13+
//!
14+
//! Implementation comes from and example usage can be found at
15+
//! https://en.cppreference.com/w/cpp/utility/variant/visit#Example
16+
template<class... Ts> struct Overloaded : Ts... { using Ts::operator()...; };
17+
18+
//! Explicit deduction guide (not needed as of C++20)
19+
template<class... Ts> Overloaded(Ts...) -> Overloaded<Ts...>;
20+
} // namespace util
21+
22+
#endif // BITCOIN_UTIL_OVERLOADED_H

‎src/wallet/interfaces.cpp

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,10 @@ WalletTx MakeWalletTx(CWallet& wallet, const CWalletTx& wtx)
8282
WalletTxStatus MakeWalletTxStatus(const CWallet& wallet, const CWalletTx& wtx)
8383
{
8484
WalletTxStatus result;
85-
result.block_height = wtx.m_confirm.block_height > 0 ? wtx.m_confirm.block_height : std::numeric_limits<int>::max();
85+
result.block_height =
86+
wtx.state<TxStateConfirmed>() ? wtx.state<TxStateConfirmed>()->confirmed_block_height :
87+
wtx.state<TxStateConflicted>() ? wtx.state<TxStateConflicted>()->conflicting_block_height :
88+
std::numeric_limits<int>::max();
8689
result.blocks_to_maturity = wallet.GetTxBlocksToMaturity(wtx);
8790
result.depth_in_main_chain = wallet.GetTxDepthInMainChain(wtx);
8891
result.time_received = wtx.nTimeReceived;

‎src/wallet/rpcdump.cpp

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -369,11 +369,9 @@ RPCHelpMan importprunedfunds()
369369

370370
unsigned int txnIndex = vIndex[it - vMatch.begin()];
371371

372-
CWalletTx::Confirmation confirm(CWalletTx::Status::CONFIRMED, height, merkleBlock.header.GetHash(), txnIndex);
373-
374372
CTransactionRef tx_ref = MakeTransactionRef(tx);
375373
if (pwallet->IsMine(*tx_ref)) {
376-
pwallet->AddToWallet(std::move(tx_ref), confirm);
374+
pwallet->AddToWallet(std::move(tx_ref), TxStateConfirmed{merkleBlock.header.GetHash(), height, static_cast<int>(txnIndex)});
377375
return NullUniValue;
378376
}
379377

‎src/wallet/rpcwallet.cpp

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -166,13 +166,13 @@ static void WalletTxToJSON(const CWallet& wallet, const CWalletTx& wtx, UniValue
166166
entry.pushKV("confirmations", confirms);
167167
if (wtx.IsCoinBase())
168168
entry.pushKV("generated", true);
169-
if (confirms > 0)
169+
if (auto* conf = wtx.state<TxStateConfirmed>())
170170
{
171-
entry.pushKV("blockhash", wtx.m_confirm.hashBlock.GetHex());
172-
entry.pushKV("blockheight", wtx.m_confirm.block_height);
173-
entry.pushKV("blockindex", wtx.m_confirm.nIndex);
171+
entry.pushKV("blockhash", conf->confirmed_block_hash.GetHex());
172+
entry.pushKV("blockheight", conf->confirmed_block_height);
173+
entry.pushKV("blockindex", conf->position_in_block);
174174
int64_t block_time;
175-
CHECK_NONFATAL(chain.findBlock(wtx.m_confirm.hashBlock, FoundBlock().time(block_time)));
175+
CHECK_NONFATAL(chain.findBlock(conf->confirmed_block_hash, FoundBlock().time(block_time)));
176176
entry.pushKV("blocktime", block_time);
177177
} else {
178178
entry.pushKV("trusted", CachedTxIsTrusted(wallet, wtx));

‎src/wallet/test/coinselector_tests.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,7 @@ static void add_coin(std::vector<COutput>& coins, CWallet& wallet, const CAmount
7474
uint256 txid = tx.GetHash();
7575

7676
LOCK(wallet.cs_wallet);
77-
auto ret = wallet.mapWallet.emplace(std::piecewise_construct, std::forward_as_tuple(txid), std::forward_as_tuple(MakeTransactionRef(std::move(tx))));
77+
auto ret = wallet.mapWallet.emplace(std::piecewise_construct, std::forward_as_tuple(txid), std::forward_as_tuple(MakeTransactionRef(std::move(tx)), TxStateInactive{}));
7878
assert(ret.second);
7979
CWalletTx& wtx = (*ret.first).second;
8080
if (fIsFromMe)

‎src/wallet/test/psbt_wallet_tests.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,12 +34,12 @@ BOOST_AUTO_TEST_CASE(psbt_updater_test)
3434
CDataStream s_prev_tx1(ParseHex("0200000000010158e87a21b56daf0c23be8e7070456c336f7cbaa5c8757924f545887bb2abdd7501000000171600145f275f436b09a8cc9a2eb2a2f528485c68a56323feffffff02d8231f1b0100000017a914aed962d6654f9a2b36608eb9d64d2b260db4f1118700c2eb0b0000000017a914b7f5faf40e3d40a5a459b1db3535f2b72fa921e88702483045022100a22edcc6e5bc511af4cc4ae0de0fcd75c7e04d8c1c3a8aa9d820ed4b967384ec02200642963597b9b1bc22c75e9f3e117284a962188bf5e8a74c895089046a20ad770121035509a48eb623e10aace8bfd0212fdb8a8e5af3c94b0b133b95e114cab89e4f7965000000"), SER_NETWORK, PROTOCOL_VERSION);
3535
CTransactionRef prev_tx1;
3636
s_prev_tx1 >> prev_tx1;
37-
m_wallet.mapWallet.emplace(std::piecewise_construct, std::forward_as_tuple(prev_tx1->GetHash()), std::forward_as_tuple(prev_tx1));
37+
m_wallet.mapWallet.emplace(std::piecewise_construct, std::forward_as_tuple(prev_tx1->GetHash()), std::forward_as_tuple(prev_tx1, TxStateInactive{}));
3838

3939
CDataStream s_prev_tx2(ParseHex("0200000001aad73931018bd25f84ae400b68848be09db706eac2ac18298babee71ab656f8b0000000048473044022058f6fc7c6a33e1b31548d481c826c015bd30135aad42cd67790dab66d2ad243b02204a1ced2604c6735b6393e5b41691dd78b00f0c5942fb9f751856faa938157dba01feffffff0280f0fa020000000017a9140fb9463421696b82c833af241c78c17ddbde493487d0f20a270100000017a91429ca74f8a08f81999428185c97b5d852e4063f618765000000"), SER_NETWORK, PROTOCOL_VERSION);
4040
CTransactionRef prev_tx2;
4141
s_prev_tx2 >> prev_tx2;
42-
m_wallet.mapWallet.emplace(std::piecewise_construct, std::forward_as_tuple(prev_tx2->GetHash()), std::forward_as_tuple(prev_tx2));
42+
m_wallet.mapWallet.emplace(std::piecewise_construct, std::forward_as_tuple(prev_tx2->GetHash()), std::forward_as_tuple(prev_tx2, TxStateInactive{}));
4343

4444
// Import descriptors for keys and scripts
4545
import_descriptor(m_wallet, "sh(multi(2,xprv9s21ZrQH143K2LE7W4Xf3jATf9jECxSb7wj91ZnmY4qEJrS66Qru9RFqq8xbkgT32ya6HqYJweFdJUEDf5Q6JFV7jMiUws7kQfe6Tv4RbfN/0h/0h/0h,xprv9s21ZrQH143K2LE7W4Xf3jATf9jECxSb7wj91ZnmY4qEJrS66Qru9RFqq8xbkgT32ya6HqYJweFdJUEDf5Q6JFV7jMiUws7kQfe6Tv4RbfN/0h/0h/1h))");

‎src/wallet/test/wallet_tests.cpp

Lines changed: 9 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -330,17 +330,14 @@ BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup)
330330
BOOST_FIXTURE_TEST_CASE(coin_mark_dirty_immature_credit, TestChain100Setup)
331331
{
332332
CWallet wallet(m_node.chain.get(), "", m_args, CreateDummyWalletDatabase());
333-
CWalletTx wtx(m_coinbase_txns.back());
333+
CWalletTx wtx{m_coinbase_txns.back(), TxStateConfirmed{m_node.chainman->ActiveChain().Tip()->GetBlockHash(), m_node.chainman->ActiveChain().Height(), /*position_in_block=*/0}};
334334

335335
LOCK(wallet.cs_wallet);
336336
wallet.SetWalletFlag(WALLET_FLAG_DESCRIPTORS);
337337
wallet.SetupDescriptorScriptPubKeyMans();
338338

339339
wallet.SetLastBlockProcessed(m_node.chainman->ActiveChain().Height(), m_node.chainman->ActiveChain().Tip()->GetBlockHash());
340340

341-
CWalletTx::Confirmation confirm(CWalletTx::Status::CONFIRMED, m_node.chainman->ActiveChain().Height(), m_node.chainman->ActiveChain().Tip()->GetBlockHash(), 0);
342-
wtx.m_confirm = confirm;
343-
344341
// Call GetImmatureCredit() once before adding the key to the wallet to
345342
// cache the current immature credit amount, which is 0.
346343
BOOST_CHECK_EQUAL(CachedTxGetImmatureCredit(wallet, wtx), 0);
@@ -355,7 +352,7 @@ BOOST_FIXTURE_TEST_CASE(coin_mark_dirty_immature_credit, TestChain100Setup)
355352
static int64_t AddTx(ChainstateManager& chainman, CWallet& wallet, uint32_t lockTime, int64_t mockTime, int64_t blockTime)
356353
{
357354
CMutableTransaction tx;
358-
CWalletTx::Confirmation confirm;
355+
TxState state = TxStateInactive{};
359356
tx.nLockTime = lockTime;
360357
SetMockTime(mockTime);
361358
CBlockIndex* block = nullptr;
@@ -367,13 +364,13 @@ static int64_t AddTx(ChainstateManager& chainman, CWallet& wallet, uint32_t lock
367364
block = inserted.first->second;
368365
block->nTime = blockTime;
369366
block->phashBlock = &hash;
370-
confirm = {CWalletTx::Status::CONFIRMED, block->nHeight, hash, 0};
367+
state = TxStateConfirmed{hash, block->nHeight, /*position_in_block=*/0};
371368
}
372-
373-
// If transaction is already in map, to avoid inconsistencies, unconfirmation
374-
// is needed before confirm again with different block.
375-
return wallet.AddToWallet(MakeTransactionRef(tx), confirm, [&](CWalletTx& wtx, bool /* new_tx */) {
376-
wtx.setUnconfirmed();
369+
return wallet.AddToWallet(MakeTransactionRef(tx), state, [&](CWalletTx& wtx, bool /* new_tx */) {
370+
// Assign wtx.m_state to simplify test and avoid the need to simulate
371+
// reorg events. Without this, AddToWallet asserts false when the same
372+
// transaction is confirmed in different blocks.
373+
wtx.m_state = state;
377374
return true;
378375
})->nTimeSmart;
379376
}
@@ -534,8 +531,7 @@ class ListCoinsTestingSetup : public TestChain100Setup
534531
wallet->SetLastBlockProcessed(wallet->GetLastBlockHeight() + 1, m_node.chainman->ActiveChain().Tip()->GetBlockHash());
535532
auto it = wallet->mapWallet.find(tx->GetHash());
536533
BOOST_CHECK(it != wallet->mapWallet.end());
537-
CWalletTx::Confirmation confirm(CWalletTx::Status::CONFIRMED, m_node.chainman->ActiveChain().Height(), m_node.chainman->ActiveChain().Tip()->GetBlockHash(), 1);
538-
it->second.m_confirm = confirm;
534+
it->second.m_state = TxStateConfirmed{m_node.chainman->ActiveChain().Tip()->GetBlockHash(), m_node.chainman->ActiveChain().Height(), /*position_in_block=*/1};
539535
return it->second;
540536
}
541537

0 commit comments

Comments
 (0)