From d8ee8f3cd32bbfefec931724f5798cbb088ceb6f Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Tue, 16 Feb 2021 22:36:26 -0500 Subject: 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/wallet/wallet.cpp | 142 ++++++++++++++++++++++++++------------------------ 1 file changed, 75 insertions(+), 67 deletions(-) (limited to 'src/wallet/wallet.cpp') diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 7f60dd6906..5b8a174fcb 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -99,7 +99,11 @@ static void UpdateWalletSetting(interfaces::Chain& chain, */ static void RefreshMempoolStatus(CWalletTx& tx, interfaces::Chain& chain) { - tx.fInMempool = chain.isInMempool(tx.GetHash()); + if (chain.isInMempool(tx.GetHash())) { + tx.m_state = TxStateInMempool(); + } else if (tx.state()) { + tx.m_state = TxStateInactive(); + } } bool AddWallet(WalletContext& context, const std::shared_ptr& wallet) @@ -885,7 +889,7 @@ bool CWallet::IsSpentKey(const uint256& hash, unsigned int n) const return false; } -CWalletTx* CWallet::AddToWallet(CTransactionRef tx, const CWalletTx::Confirmation& confirm, const UpdateWalletTxFn& update_wtx, bool fFlushOnClose, bool rescanning_old_block) +CWalletTx* CWallet::AddToWallet(CTransactionRef tx, const TxState& state, const UpdateWalletTxFn& update_wtx, bool fFlushOnClose, bool rescanning_old_block) { LOCK(cs_wallet); @@ -906,12 +910,11 @@ CWalletTx* CWallet::AddToWallet(CTransactionRef tx, const CWalletTx::Confirmatio } // Inserts only if not already there, returns tx inserted or tx found - auto ret = mapWallet.emplace(std::piecewise_construct, std::forward_as_tuple(hash), std::forward_as_tuple(tx)); + auto ret = mapWallet.emplace(std::piecewise_construct, std::forward_as_tuple(hash), std::forward_as_tuple(tx, state)); CWalletTx& wtx = (*ret.first).second; bool fInsertedNew = ret.second; bool fUpdated = update_wtx && update_wtx(wtx, fInsertedNew); if (fInsertedNew) { - wtx.m_confirm = confirm; wtx.nTimeReceived = chain().getAdjustedTime(); wtx.nOrderPos = IncOrderPosNext(&batch); wtx.m_it_wtxOrdered = wtxOrdered.insert(std::make_pair(wtx.nOrderPos, &wtx)); @@ -921,16 +924,12 @@ CWalletTx* CWallet::AddToWallet(CTransactionRef tx, const CWalletTx::Confirmatio if (!fInsertedNew) { - if (confirm.status != wtx.m_confirm.status) { - wtx.m_confirm.status = confirm.status; - wtx.m_confirm.nIndex = confirm.nIndex; - wtx.m_confirm.hashBlock = confirm.hashBlock; - wtx.m_confirm.block_height = confirm.block_height; + if (state.index() != wtx.m_state.index()) { + wtx.m_state = state; fUpdated = true; } else { - assert(wtx.m_confirm.nIndex == confirm.nIndex); - assert(wtx.m_confirm.hashBlock == confirm.hashBlock); - assert(wtx.m_confirm.block_height == confirm.block_height); + assert(TxStateSerializedIndex(wtx.m_state) == TxStateSerializedIndex(state)); + assert(TxStateSerializedBlockHash(wtx.m_state) == TxStateSerializedBlockHash(state)); } // If we have a witness-stripped version of this transaction, and we // see a new version with a witness, then we must be upgrading a pre-segwit @@ -964,10 +963,10 @@ CWalletTx* CWallet::AddToWallet(CTransactionRef tx, const CWalletTx::Confirmatio if (!strCmd.empty()) { boost::replace_all(strCmd, "%s", hash.GetHex()); - if (confirm.status == CWalletTx::Status::CONFIRMED) + if (auto* conf = wtx.state()) { - boost::replace_all(strCmd, "%b", confirm.hashBlock.GetHex()); - boost::replace_all(strCmd, "%h", ToString(confirm.block_height)); + boost::replace_all(strCmd, "%b", conf->confirmed_block_hash.GetHex()); + boost::replace_all(strCmd, "%h", ToString(conf->confirmed_block_height)); } else { boost::replace_all(strCmd, "%b", "unconfirmed"); boost::replace_all(strCmd, "%h", "-1"); @@ -990,7 +989,7 @@ CWalletTx* CWallet::AddToWallet(CTransactionRef tx, const CWalletTx::Confirmatio bool CWallet::LoadToWallet(const uint256& hash, const UpdateWalletTxFn& fill_wtx) { - const auto& ins = mapWallet.emplace(std::piecewise_construct, std::forward_as_tuple(hash), std::forward_as_tuple(nullptr)); + const auto& ins = mapWallet.emplace(std::piecewise_construct, std::forward_as_tuple(hash), std::forward_as_tuple(nullptr, TxStateInactive{})); CWalletTx& wtx = ins.first->second; if (!fill_wtx(wtx, ins.second)) { return false; @@ -998,22 +997,21 @@ bool CWallet::LoadToWallet(const uint256& hash, const UpdateWalletTxFn& fill_wtx // If wallet doesn't have a chain (e.g wallet-tool), don't bother to update txn. if (HaveChain()) { bool active; - int height; - if (chain().findBlock(wtx.m_confirm.hashBlock, FoundBlock().inActiveChain(active).height(height)) && active) { - // Update cached block height variable since it not stored in the - // 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 // associated blocks and don't need to be updated. The case where a // transaction was reorged out while online and then reconfirmed // while offline is covered by the rescan logic. - wtx.setUnconfirmed(); - wtx.m_confirm.hashBlock = uint256(); - wtx.m_confirm.block_height = 0; - wtx.m_confirm.nIndex = 0; + if (!chain().findBlock(hash, FoundBlock().inActiveChain(active).height(height)) || !active) { + state = TxStateInactive{}; + } + }; + if (auto* conf = wtx.state()) { + lookup_block(conf->confirmed_block_hash, conf->confirmed_block_height, wtx.m_state); + } else if (auto* conf = wtx.state()) { + lookup_block(conf->conflicting_block_hash, conf->conflicting_block_height, wtx.m_state); } } if (/* insertion took place */ ins.second) { @@ -1024,27 +1022,27 @@ bool CWallet::LoadToWallet(const uint256& hash, const UpdateWalletTxFn& fill_wtx auto it = mapWallet.find(txin.prevout.hash); if (it != mapWallet.end()) { CWalletTx& prevtx = it->second; - if (prevtx.isConflicted()) { - MarkConflicted(prevtx.m_confirm.hashBlock, prevtx.m_confirm.block_height, wtx.GetHash()); + if (auto* prev = prevtx.state()) { + MarkConflicted(prev->conflicting_block_hash, prev->conflicting_block_height, wtx.GetHash()); } } } return true; } -bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, CWalletTx::Confirmation confirm, bool fUpdate, bool rescanning_old_block) +bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, const SyncTxState& state, bool fUpdate, bool rescanning_old_block) { const CTransaction& tx = *ptx; { AssertLockHeld(cs_wallet); - if (!confirm.hashBlock.IsNull()) { + if (auto* conf = std::get_if(&state)) { for (const CTxIn& txin : tx.vin) { std::pair range = mapTxSpends.equal_range(txin.prevout); while (range.first != range.second) { if (range.first->second != tx.GetHash()) { - WalletLogPrintf("Transaction %s (in block %s) conflicts with wallet transaction %s (both spend %s:%i)\n", tx.GetHash().ToString(), confirm.hashBlock.ToString(), range.first->second.ToString(), range.first->first.hash.ToString(), range.first->first.n); - MarkConflicted(confirm.hashBlock, confirm.block_height, range.first->second); + WalletLogPrintf("Transaction %s (in block %s) conflicts with wallet transaction %s (both spend %s:%i)\n", tx.GetHash().ToString(), conf->confirmed_block_hash.ToString(), range.first->second.ToString(), range.first->first.hash.ToString(), range.first->first.n); + MarkConflicted(conf->confirmed_block_hash, conf->confirmed_block_height, range.first->second); } range.first++; } @@ -1070,7 +1068,8 @@ bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, CWalletTx::Co // Block disconnection override an abandoned tx as unconfirmed // which means user may have to call abandontransaction again - return AddToWallet(MakeTransactionRef(tx), confirm, /* update_wtx= */ nullptr, /* fFlushOnClose= */ false, rescanning_old_block); + TxState tx_state = std::visit([](auto&& s) -> TxState { return s; }, state); + return AddToWallet(MakeTransactionRef(tx), tx_state, /*update_wtx=*/nullptr, /*fFlushOnClose=*/false, rescanning_old_block); } } return false; @@ -1126,7 +1125,7 @@ bool CWallet::AbandonTransaction(const uint256& hashTx) if (currentconfirm == 0 && !wtx.isAbandoned()) { // If the orig tx was not in block/mempool, none of its spends can be in mempool assert(!wtx.InMempool()); - wtx.setAbandoned(); + wtx.m_state = TxStateInactive{/*abandoned=*/true}; wtx.MarkDirty(); batch.WriteTx(wtx); NotifyTransactionChanged(wtx.GetHash(), CT_UPDATED); @@ -1178,10 +1177,7 @@ void CWallet::MarkConflicted(const uint256& hashBlock, int conflicting_height, c if (conflictconfirms < currentconfirm) { // Block is 'more conflicted' than current confirm; update. // Mark transaction as conflicted with this block. - wtx.m_confirm.nIndex = 0; - wtx.m_confirm.hashBlock = hashBlock; - wtx.m_confirm.block_height = conflicting_height; - wtx.setConflicted(); + wtx.m_state = TxStateConflicted{hashBlock, conflicting_height}; wtx.MarkDirty(); batch.WriteTx(wtx); // Iterate over all its outputs, and mark transactions in the wallet that spend them conflicted too @@ -1199,9 +1195,9 @@ void CWallet::MarkConflicted(const uint256& hashBlock, int conflicting_height, c } } -void CWallet::SyncTransaction(const CTransactionRef& ptx, CWalletTx::Confirmation confirm, bool update_tx, bool rescanning_old_block) +void CWallet::SyncTransaction(const CTransactionRef& ptx, const SyncTxState& state, bool update_tx, bool rescanning_old_block) { - if (!AddToWalletIfInvolvingMe(ptx, confirm, update_tx, rescanning_old_block)) + if (!AddToWalletIfInvolvingMe(ptx, state, update_tx, rescanning_old_block)) return; // Not one of ours // If a transaction changes 'conflicted' state, that changes the balance @@ -1212,7 +1208,7 @@ void CWallet::SyncTransaction(const CTransactionRef& ptx, CWalletTx::Confirmatio void CWallet::transactionAddedToMempool(const CTransactionRef& tx, uint64_t mempool_sequence) { LOCK(cs_wallet); - SyncTransaction(tx, {CWalletTx::Status::UNCONFIRMED, /*block_height=*/0, /*block_hash=*/{}, /*block_index=*/0}); + SyncTransaction(tx, TxStateInMempool{}); auto it = mapWallet.find(tx->GetHash()); if (it != mapWallet.end()) { @@ -1253,7 +1249,7 @@ void CWallet::transactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRe // distinguishing between conflicted and unconfirmed transactions are // imperfect, and could be improved in general, see // https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Transaction-Conflict-Tracking - SyncTransaction(tx, {CWalletTx::Status::UNCONFIRMED, /*block_height=*/0, /*block_hash=*/{}, /*block_index=*/0}); + SyncTransaction(tx, TxStateInactive{}); } } @@ -1265,7 +1261,7 @@ void CWallet::blockConnected(const CBlock& block, int height) m_last_block_processed_height = height; m_last_block_processed = block_hash; for (size_t index = 0; index < block.vtx.size(); index++) { - SyncTransaction(block.vtx[index], {CWalletTx::Status::CONFIRMED, height, block_hash, (int)index}); + SyncTransaction(block.vtx[index], TxStateConfirmed{block_hash, height, static_cast(index)}); transactionRemovedFromMempool(block.vtx[index], MemPoolRemovalReason::BLOCK, 0 /* mempool_sequence */); } } @@ -1281,7 +1277,7 @@ void CWallet::blockDisconnected(const CBlock& block, int height) m_last_block_processed_height = height - 1; m_last_block_processed = block.hashPrevBlock; for (const CTransactionRef& ptx : block.vtx) { - SyncTransaction(ptx, {CWalletTx::Status::UNCONFIRMED, /*block_height=*/0, /*block_hash=*/{}, /*block_index=*/0}); + SyncTransaction(ptx, TxStateInactive{}); } } @@ -1645,7 +1641,7 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc break; } for (size_t posInBlock = 0; posInBlock < block.vtx.size(); ++posInBlock) { - SyncTransaction(block.vtx[posInBlock], {CWalletTx::Status::CONFIRMED, block_height, block_hash, (int)posInBlock}, fUpdate, /* rescanning_old_block */ true); + SyncTransaction(block.vtx[posInBlock], TxStateConfirmed{block_hash, block_height, static_cast(posInBlock)}, fUpdate, /*rescanning_old_block=*/true); } // scan succeeded, record block as most recent successfully scanned result.last_scanned_block = block_hash; @@ -1720,7 +1716,7 @@ void CWallet::ReacceptWalletTransactions() } } -bool CWallet::SubmitTxMemoryPoolAndRelay(const CWalletTx& wtx, std::string& err_string, bool relay) const +bool CWallet::SubmitTxMemoryPoolAndRelay(CWalletTx& wtx, std::string& err_string, bool relay) const { // Can't relay if wallet is not broadcasting if (!GetBroadcastTransactions()) return false; @@ -1734,17 +1730,17 @@ bool CWallet::SubmitTxMemoryPoolAndRelay(const CWalletTx& wtx, std::string& err_ // Submit transaction to mempool for relay WalletLogPrintf("Submitting wtx %s to mempool for relay\n", wtx.GetHash().ToString()); - // We must set fInMempool here - while it will be re-set to true by the + // We must set TxStateInMempool here. Even though it will also be set later by the // entered-mempool callback, if we did not there would be a race where a // user could call sendmoney in a loop and hit spurious out of funds errors // because we think that this newly generated transaction's change is // unavailable as we're not yet aware that it is in the mempool. // - // Irrespective of the failure reason, un-marking fInMempool - // out-of-order is incorrect - it should be unmarked when + // If broadcast fails for any reason, trying to set wtx.m_state here would be incorrect. + // If transaction was previously in the mempool, it should be updated when // TransactionRemovedFromMempool fires. bool ret = chain().broadcastTransaction(wtx.tx, m_default_max_tx_fee, relay, err_string); - wtx.fInMempool |= ret; + if (ret) wtx.m_state = TxStateInMempool{}; return ret; } @@ -1831,7 +1827,8 @@ bool CWallet::SignTransaction(CMutableTransaction& tx) const return false; } const CWalletTx& wtx = mi->second; - coins[input.prevout] = Coin(wtx.tx->vout[input.prevout.n], wtx.m_confirm.block_height, wtx.IsCoinBase()); + int prev_height = wtx.state() ? wtx.state()->confirmed_block_height : 0; + coins[input.prevout] = Coin(wtx.tx->vout[input.prevout.n], prev_height, wtx.IsCoinBase()); } std::map input_errors; return SignTransaction(tx, coins, SIGHASH_DEFAULT, input_errors); @@ -1956,7 +1953,7 @@ void CWallet::CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::ve // Add tx to wallet, because if it has change it's also ours, // otherwise just for transaction history. - AddToWallet(tx, {}, [&](CWalletTx& wtx, bool new_tx) { + AddToWallet(tx, TxStateInactive{}, [&](CWalletTx& wtx, bool new_tx) { CHECK_NONFATAL(wtx.mapValue.empty()); CHECK_NONFATAL(wtx.vOrderForm.empty()); wtx.mapValue = std::move(mapValue); @@ -1974,7 +1971,7 @@ void CWallet::CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::ve } // Get the inserted-CWalletTx from mapWallet so that the - // fInMempool flag is cached properly + // wtx cached mempool state is updated correctly CWalletTx& wtx = mapWallet.at(tx->GetHash()); if (!fBroadcastTransactions) { @@ -2321,10 +2318,10 @@ void CWallet::GetKeyBirthTimes(std::map& mapKeyBirth) const { mapKeyBirth.clear(); // map in which we'll infer heights of other keys - std::map mapKeyFirstBlock; - CWalletTx::Confirmation max_confirm; - max_confirm.block_height = GetLastBlockHeight() > 144 ? GetLastBlockHeight() - 144 : 0; // the tip can be reorganized; use a 144-block safety margin - CHECK_NONFATAL(chain().findAncestorByHeight(GetLastBlockHash(), max_confirm.block_height, FoundBlock().hash(max_confirm.hashBlock))); + std::map mapKeyFirstBlock; + TxStateConfirmed max_confirm{uint256{}, /*height=*/-1, /*index=*/-1}; + max_confirm.confirmed_block_height = GetLastBlockHeight() > 144 ? GetLastBlockHeight() - 144 : 0; // the tip can be reorganized; use a 144-block safety margin + CHECK_NONFATAL(chain().findAncestorByHeight(GetLastBlockHash(), max_confirm.confirmed_block_height, FoundBlock().hash(max_confirm.confirmed_block_hash))); { LegacyScriptPubKeyMan* spk_man = GetLegacyScriptPubKeyMan(); @@ -2352,15 +2349,15 @@ void CWallet::GetKeyBirthTimes(std::map& mapKeyBirth) const { for (const auto& entry : mapWallet) { // iterate over all wallet transactions... const CWalletTx &wtx = entry.second; - if (wtx.m_confirm.status == CWalletTx::CONFIRMED) { + if (auto* conf = wtx.state()) { // ... which are already in a block for (const CTxOut &txout : wtx.tx->vout) { // iterate over all their outputs for (const auto &keyid : GetAffectedKeys(txout.scriptPubKey, *spk_man)) { // ... and all their affected keys auto rit = mapKeyFirstBlock.find(keyid); - if (rit != mapKeyFirstBlock.end() && wtx.m_confirm.block_height < rit->second->block_height) { - rit->second = &wtx.m_confirm; + if (rit != mapKeyFirstBlock.end() && conf->confirmed_block_height < rit->second->confirmed_block_height) { + rit->second = conf; } } } @@ -2371,7 +2368,7 @@ void CWallet::GetKeyBirthTimes(std::map& mapKeyBirth) const { // Extract block timestamps for those keys for (const auto& entry : mapKeyFirstBlock) { int64_t block_time; - CHECK_NONFATAL(chain().findBlock(entry.second->hashBlock, FoundBlock().time(block_time))); + CHECK_NONFATAL(chain().findBlock(entry.second->confirmed_block_hash, FoundBlock().time(block_time))); mapKeyBirth[entry.first] = block_time - TIMESTAMP_WINDOW; // block times can be 2h off } } @@ -2401,11 +2398,18 @@ void CWallet::GetKeyBirthTimes(std::map& mapKeyBirth) const { */ unsigned int CWallet::ComputeTimeSmart(const CWalletTx& wtx, bool rescanning_old_block) const { + std::optional block_hash; + if (auto* conf = wtx.state()) { + block_hash = conf->confirmed_block_hash; + } else if (auto* conf = wtx.state()) { + block_hash = conf->conflicting_block_hash; + } + unsigned int nTimeSmart = wtx.nTimeReceived; - if (!wtx.isUnconfirmed() && !wtx.isAbandoned()) { + if (block_hash) { int64_t blocktime; int64_t block_max_time; - if (chain().findBlock(wtx.m_confirm.hashBlock, FoundBlock().time(blocktime).maxTime(block_max_time))) { + if (chain().findBlock(*block_hash, FoundBlock().time(blocktime).maxTime(block_max_time))) { if (rescanning_old_block) { nTimeSmart = block_max_time; } else { @@ -2437,7 +2441,7 @@ unsigned int CWallet::ComputeTimeSmart(const CWalletTx& wtx, bool rescanning_old nTimeSmart = std::max(latestEntry, std::min(blocktime, latestNow)); } } else { - WalletLogPrintf("%s: found %s in block %s not in index\n", __func__, wtx.GetHash().ToString(), wtx.m_confirm.hashBlock.ToString()); + WalletLogPrintf("%s: found %s in block %s not in index\n", __func__, wtx.GetHash().ToString(), block_hash->ToString()); } } return nTimeSmart; @@ -2944,9 +2948,13 @@ CKeyPool::CKeyPool(const CPubKey& vchPubKeyIn, bool internalIn) int CWallet::GetTxDepthInMainChain(const CWalletTx& wtx) const { AssertLockHeld(cs_wallet); - if (wtx.isUnconfirmed() || wtx.isAbandoned()) return 0; - - return (GetLastBlockHeight() - wtx.m_confirm.block_height + 1) * (wtx.isConflicted() ? -1 : 1); + if (auto* conf = wtx.state()) { + return GetLastBlockHeight() - conf->confirmed_block_height + 1; + } else if (auto* conf = wtx.state()) { + return -1 * (GetLastBlockHeight() - conf->conflicting_block_height + 1); + } else { + return 0; + } } int CWallet::GetTxBlocksToMaturity(const CWalletTx& wtx) const -- cgit v1.2.3