highperfocused/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-11-12 15:09:59 +01:00
Code Issues Packages Projects Releases Wiki Activity

refactor: Make CWalletTx sync state type-safe

Browse Source 
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.
This commit is contained in:
Russell Yanofsky
2021-02-16 22:36:26 -05:00
parent 2efc8c0999
commit d8ee8f3cd3
16 changed files with 256 additions and 173 deletions
Download Patch File Download Diff File Expand all files Collapse all files

22
src/wallet/test/wallet_tests.cpp
View File

@@ -330,7 +330,7 @@ BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup)
BOOST_FIXTURE_TEST_CASE(coin_mark_dirty_immature_credit, TestChain100Setup)
{
CWallet wallet(m_node.chain.get(), "", m_args, CreateDummyWalletDatabase());
CWalletTx wtx(m_coinbase_txns.back());
CWalletTx wtx{m_coinbase_txns.back(), TxStateConfirmed{m_node.chainman->ActiveChain().Tip()->GetBlockHash(), m_node.chainman->ActiveChain().Height(), /*position_in_block=*/0}};
LOCK(wallet.cs_wallet);
wallet.SetWalletFlag(WALLET_FLAG_DESCRIPTORS);
@@ -338,9 +338,6 @@ BOOST_FIXTURE_TEST_CASE(coin_mark_dirty_immature_credit, TestChain100Setup)
wallet.SetLastBlockProcessed(m_node.chainman->ActiveChain().Height(), m_node.chainman->ActiveChain().Tip()->GetBlockHash());
CWalletTx::Confirmation confirm(CWalletTx::Status::CONFIRMED, m_node.chainman->ActiveChain().Height(), m_node.chainman->ActiveChain().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);
@@ -355,7 +352,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;
@@ -367,13 +364,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;
}
@@ -534,8 +531,7 @@ public:
wallet->SetLastBlockProcessed(wallet->GetLastBlockHeight() + 1, m_node.chainman->ActiveChain().Tip()->GetBlockHash());
auto it = wallet->mapWallet.find(tx->GetHash());
BOOST_CHECK(it != wallet->mapWallet.end());
CWalletTx::Confirmation confirm(CWalletTx::Status::CONFIRMED, m_node.chainman->ActiveChain().Height(), m_node.chainman->ActiveChain().Tip()->GetBlockHash(), 1);
it->second.m_confirm = confirm;
it->second.m_state = TxStateConfirmed{m_node.chainman->ActiveChain().Tip()->GetBlockHash(), m_node.chainman->ActiveChain().Height(), /*position_in_block=*/1};
return it->second;
}