diff options
author | MeshCollider <dobsonsa68@gmail.com> | 2019-09-06 01:28:15 +1200 |
---|---|---|
committer | MeshCollider <dobsonsa68@gmail.com> | 2019-09-06 01:28:54 +1200 |
commit | 5e202382a987351a59d5cda98ea9f2aee99f61a5 (patch) | |
tree | 50a1d4ca1efc3f5973e4edee39cc69b305ff932b /src/wallet/wallet.cpp | |
parent | 5667b0d758f119dadc5b41e0bf24e45da1132e54 (diff) | |
parent | 442a87cc0ae43ebc9b6654a6165778eecb931f74 (diff) | |
download | bitcoin-5e202382a987351a59d5cda98ea9f2aee99f61a5.tar.xz |
Merge #16624: wallet: encapsulate transactions state
442a87cc0ae43ebc9b6654a6165778eecb931f74 Add a test wallet_reorgsrestore (Antoine Riard)
40ede992d97df38282919693dfe851c975c3b1d8 Modify wallet tx status if has been reorged out (Antoine Riard)
7e89994133725125dddbfa8d45484e3b9ed51c6e Remove SyncTransaction for conflicted txn in CWallet::BlockConnected (Antoine Riard)
a31be09bfd77eed497a8e251d31358e16e2f2eb1 Encapsulate tx status in a Confirmation struct (Antoine Riard)
Pull request description:
While working on #15931, I've tried to rationalize tx state management to ease integration of block height tracking per-wallet tx. We currently rely on a combination of `hashBlock` and `nIndex` with magic value to determine tx confirmation, conflicted or abandoned state. It's hard to reason and error-prone. To solve that, we encapsulate these fields in a `TxConfirmation` struct and introduce a `TxState` member that we update accordingly at block connection/disconnection.
Following jnewbery [recommendation](https://github.com/bitcoin/bitcoin/pull/15931#discussion_r312576506), I've taken these changes in its own commit, and open a PR to get them first. It would ease review of aforementioned PR, but above all should ease fixing of long-term issues like :
* https://github.com/bitcoin/bitcoin/issues/7315 (but maybe we should abandon abandontransaction or relieve it to only free outpoints not track the transaction as abandoned in itself, need its own discussion)
* https://github.com/bitcoin/bitcoin/issues/8692 where we should cancel conflicted state of transactions chain smoothly
* `MarkConflicted` in `LoadToWallet` is likely useless if we track conflicts rights at block connection
Main changes of this PR to get right are tx update in `AddToWallet` and serialization/deserialization logic.
ACKs for top commit:
meshcollider:
Light re-Code Review ACK 442a87cc0ae43ebc9b6654a6165778eecb931f74
ryanofsky:
utACK 442a87cc0ae43ebc9b6654a6165778eecb931f74. Changes since last review are switching from `hasChain` to `LockChain` and removing chain lock in `WalletBatch::LoadWallet` that's redundant with the new lock still added in `CWallet::LoadWallet`, and fixing python test race condition.
Tree-SHA512: 029209e006de0240436817204e69e548c5665e2b0721b214510e7aba7eba130a5eab441d3a1ad95bd6426114dd27390492c77bf4560a9610009b32cd0a1f72f7
Diffstat (limited to 'src/wallet/wallet.cpp')
-rw-r--r-- | src/wallet/wallet.cpp | 117 |
1 files changed, 65 insertions, 52 deletions
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 84fd01730d..7629a40c5e 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1110,22 +1110,14 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose) bool fUpdated = false; if (!fInsertedNew) { - // Merge - if (!wtxIn.hashUnset() && wtxIn.hashBlock != wtx.hashBlock) - { - wtx.hashBlock = wtxIn.hashBlock; - fUpdated = true; - } - // If no longer abandoned, update - if (wtxIn.hashBlock.IsNull() && wtx.isAbandoned()) - { - wtx.hashBlock = wtxIn.hashBlock; - fUpdated = true; - } - if (wtxIn.nIndex != -1 && (wtxIn.nIndex != wtx.nIndex)) - { - wtx.nIndex = wtxIn.nIndex; + if (wtxIn.m_confirm.status != wtx.m_confirm.status) { + wtx.m_confirm.status = wtxIn.m_confirm.status; + wtx.m_confirm.nIndex = wtxIn.m_confirm.nIndex; + wtx.m_confirm.hashBlock = wtxIn.m_confirm.hashBlock; fUpdated = true; + } else { + assert(wtx.m_confirm.nIndex == wtxIn.m_confirm.nIndex); + assert(wtx.m_confirm.hashBlock == wtxIn.m_confirm.hashBlock); } if (wtxIn.fFromMe && wtxIn.fFromMe != wtx.fFromMe) { @@ -1172,8 +1164,19 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose) return true; } -void CWallet::LoadToWallet(const CWalletTx& wtxIn) +void CWallet::LoadToWallet(CWalletTx& wtxIn) { + // If wallet doesn't have a chain (e.g wallet-tool), lock can't be taken. + auto locked_chain = LockChain(); + // If tx hasn't been reorged out of chain while wallet being shutdown + // change tx status to UNCONFIRMED and reset hashBlock/nIndex. + if (!wtxIn.m_confirm.hashBlock.IsNull()) { + if (locked_chain && !locked_chain->getBlockHeight(wtxIn.m_confirm.hashBlock)) { + wtxIn.setUnconfirmed(); + wtxIn.m_confirm.hashBlock = uint256(); + wtxIn.m_confirm.nIndex = 0; + } + } uint256 hash = wtxIn.GetHash(); const auto& ins = mapWallet.emplace(hash, wtxIn); CWalletTx& wtx = ins.first->second; @@ -1186,14 +1189,14 @@ void CWallet::LoadToWallet(const CWalletTx& wtxIn) auto it = mapWallet.find(txin.prevout.hash); if (it != mapWallet.end()) { CWalletTx& prevtx = it->second; - if (prevtx.nIndex == -1 && !prevtx.hashUnset()) { - MarkConflicted(prevtx.hashBlock, wtx.GetHash()); + if (prevtx.isConflicted()) { + MarkConflicted(prevtx.m_confirm.hashBlock, wtx.GetHash()); } } } } -bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, const uint256& block_hash, int posInBlock, bool fUpdate) +bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, CWalletTx::Status status, const uint256& block_hash, int posInBlock, bool fUpdate) { const CTransaction& tx = *ptx; { @@ -1240,9 +1243,9 @@ bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, const uint256 CWalletTx wtx(this, ptx); - // Get merkle branch if transaction was found in a block - if (!block_hash.IsNull()) - wtx.SetMerkleBranch(block_hash, posInBlock); + // Block disconnection override an abandoned tx as unconfirmed + // which means user may have to call abandontransaction again + wtx.SetConf(status, block_hash, posInBlock); return AddToWallet(wtx, false); } @@ -1302,7 +1305,7 @@ bool CWallet::AbandonTransaction(interfaces::Chain::Lock& locked_chain, const ui 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.nIndex = -1; + wtx.m_confirm.nIndex = 0; wtx.setAbandoned(); wtx.MarkDirty(); batch.WriteTx(wtx); @@ -1356,8 +1359,9 @@ void CWallet::MarkConflicted(const uint256& hashBlock, const uint256& hashTx) if (conflictconfirms < currentconfirm) { // Block is 'more conflicted' than current confirm; update. // Mark transaction as conflicted with this block. - wtx.nIndex = -1; - wtx.hashBlock = hashBlock; + wtx.m_confirm.nIndex = 0; + wtx.m_confirm.hashBlock = hashBlock; + wtx.setConflicted(); wtx.MarkDirty(); batch.WriteTx(wtx); // Iterate over all its outputs, and mark transactions in the wallet that spend them conflicted too @@ -1375,8 +1379,9 @@ void CWallet::MarkConflicted(const uint256& hashBlock, const uint256& hashTx) } } -void CWallet::SyncTransaction(const CTransactionRef& ptx, const uint256& block_hash, int posInBlock, bool update_tx) { - if (!AddToWalletIfInvolvingMe(ptx, block_hash, posInBlock, update_tx)) +void CWallet::SyncTransaction(const CTransactionRef& ptx, CWalletTx::Status status, const uint256& block_hash, int posInBlock, bool update_tx) +{ + if (!AddToWalletIfInvolvingMe(ptx, status, block_hash, posInBlock, update_tx)) return; // Not one of ours // If a transaction changes 'conflicted' state, that changes the balance @@ -1388,7 +1393,7 @@ void CWallet::SyncTransaction(const CTransactionRef& ptx, const uint256& block_h void CWallet::TransactionAddedToMempool(const CTransactionRef& ptx) { auto locked_chain = chain().lock(); LOCK(cs_wallet); - SyncTransaction(ptx, {} /* block hash */, 0 /* position in block */); + SyncTransaction(ptx, CWalletTx::Status::UNCONFIRMED, {} /* block hash */, 0 /* position in block */); auto it = mapWallet.find(ptx->GetHash()); if (it != mapWallet.end()) { @@ -1408,22 +1413,14 @@ void CWallet::BlockConnected(const CBlock& block, const std::vector<CTransaction const uint256& block_hash = block.GetHash(); auto locked_chain = chain().lock(); LOCK(cs_wallet); - // TODO: Temporarily ensure that mempool removals are notified before - // connected transactions. This shouldn't matter, but the abandoned - // state of transactions in our wallet is currently cleared when we - // receive another notification and there is a race condition where - // notification of a connected conflict might cause an outside process - // to abandon a transaction and then have it inadvertently cleared by - // the notification that the conflicted transaction was evicted. - for (const CTransactionRef& ptx : vtxConflicted) { - SyncTransaction(ptx, {} /* block hash */, 0 /* position in block */); - TransactionRemovedFromMempool(ptx); - } for (size_t i = 0; i < block.vtx.size(); i++) { - SyncTransaction(block.vtx[i], block_hash, i); + SyncTransaction(block.vtx[i], CWalletTx::Status::CONFIRMED, block_hash, i); TransactionRemovedFromMempool(block.vtx[i]); } + for (const CTransactionRef& ptx : vtxConflicted) { + TransactionRemovedFromMempool(ptx); + } m_last_block_processed = block_hash; } @@ -1432,8 +1429,12 @@ void CWallet::BlockDisconnected(const CBlock& block) { auto locked_chain = chain().lock(); LOCK(cs_wallet); + // At block disconnection, this will change an abandoned transaction to + // be unconfirmed, whether or not the transaction is added back to the mempool. + // User may have to call abandontransaction again. It may be addressed in the + // future with a stickier abandoned state or even removing abandontransaction call. for (const CTransactionRef& ptx : block.vtx) { - SyncTransaction(ptx, {} /* block hash */, 0 /* position in block */); + SyncTransaction(ptx, CWalletTx::Status::UNCONFIRMED, {} /* block hash */, 0 /* position in block */); } } @@ -2070,7 +2071,7 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc break; } for (size_t posInBlock = 0; posInBlock < block.vtx.size(); ++posInBlock) { - SyncTransaction(block.vtx[posInBlock], block_hash, posInBlock, fUpdate); + SyncTransaction(block.vtx[posInBlock], CWalletTx::Status::CONFIRMED, block_hash, posInBlock, fUpdate); } // scan succeeded, record block as most recent successfully scanned result.last_scanned_block = block_hash; @@ -3332,6 +3333,11 @@ bool CWallet::CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::ve DBErrors CWallet::LoadWallet(bool& fFirstRunRet) { + // Even if we don't use this lock in this function, we want to preserve + // lock order in LoadToWallet if query of chain state is needed to know + // tx status. If lock can't be taken (e.g wallet-tool), tx confirmation + // status may be not reliable. + auto locked_chain = LockChain(); LOCK(cs_wallet); fFirstRunRet = false; @@ -4042,7 +4048,7 @@ void CWallet::GetKeyBirthTimes(interfaces::Chain::Lock& locked_chain, std::map<C for (const auto& entry : mapWallet) { // iterate over all wallet transactions... const CWalletTx &wtx = entry.second; - if (Optional<int> height = locked_chain.getBlockHeight(wtx.hashBlock)) { + if (Optional<int> height = locked_chain.getBlockHeight(wtx.m_confirm.hashBlock)) { // ... which are already in a block for (const CTxOut &txout : wtx.tx->vout) { // iterate over all their outputs @@ -4085,9 +4091,9 @@ void CWallet::GetKeyBirthTimes(interfaces::Chain::Lock& locked_chain, std::map<C unsigned int CWallet::ComputeTimeSmart(const CWalletTx& wtx) const { unsigned int nTimeSmart = wtx.nTimeReceived; - if (!wtx.hashUnset()) { + if (!wtx.isUnconfirmed() && !wtx.isAbandoned()) { int64_t blocktime; - if (chain().findBlock(wtx.hashBlock, nullptr /* block */, &blocktime)) { + if (chain().findBlock(wtx.m_confirm.hashBlock, nullptr /* block */, &blocktime)) { int64_t latestNow = wtx.nTimeReceived; int64_t latestEntry = 0; @@ -4115,7 +4121,7 @@ unsigned int CWallet::ComputeTimeSmart(const CWalletTx& wtx) const 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.hashBlock.ToString()); + WalletLogPrintf("%s: found %s in block %s not in index\n", __func__, wtx.GetHash().ToString(), wtx.m_confirm.hashBlock.ToString()); } } return nTimeSmart; @@ -4233,6 +4239,11 @@ bool CWallet::Verify(interfaces::Chain& chain, const WalletLocation& location, b // Recover readable keypairs: CWallet dummyWallet(&chain, WalletLocation(), WalletDatabase::CreateDummy()); std::string backup_filename; + // Even if we don't use this lock in this function, we want to preserve + // lock order in LoadToWallet if query of chain state is needed to know + // tx status. If lock can't be taken, tx confirmation status may be not + // reliable. + auto locked_chain = dummyWallet.LockChain(); if (!WalletBatch::Recover(wallet_path, (void *)&dummyWallet, WalletBatch::RecoverKeysOnlyFilter, backup_filename)) { return false; } @@ -4627,21 +4638,23 @@ CKeyPool::CKeyPool(const CPubKey& vchPubKeyIn, bool internalIn) m_pre_split = false; } -void CWalletTx::SetMerkleBranch(const uint256& block_hash, int posInBlock) +void CWalletTx::SetConf(Status status, const uint256& block_hash, int posInBlock) { + // Update tx status + m_confirm.status = status; + // Update the tx's hashBlock - hashBlock = block_hash; + m_confirm.hashBlock = block_hash; // set the position of the transaction in the block - nIndex = posInBlock; + m_confirm.nIndex = posInBlock; } int CWalletTx::GetDepthInMainChain(interfaces::Chain::Lock& locked_chain) const { - if (hashUnset()) - return 0; + if (isUnconfirmed() || isAbandoned()) return 0; - return locked_chain.getBlockDepth(hashBlock) * (nIndex == -1 ? -1 : 1); + return locked_chain.getBlockDepth(m_confirm.hashBlock) * (isConflicted() ? -1 : 1); } int CWalletTx::GetBlocksToMaturity(interfaces::Chain::Lock& locked_chain) const |