diff --git a/src/interfaces/wallet.cpp b/src/interfaces/wallet.cpp --- a/src/interfaces/wallet.cpp +++ b/src/interfaces/wallet.cpp @@ -62,7 +62,7 @@ const CWalletTx &wtx) { WalletTxStatus result; result.block_height = - locked_chain.getBlockHeight(wtx.hashBlock) + locked_chain.getBlockHeight(wtx.m_confirm.hashBlock) .get_value_or(std::numeric_limits::max()); result.blocks_to_maturity = wtx.GetBlocksToMaturity(locked_chain); result.depth_in_main_chain = wtx.GetDepthInMainChain(locked_chain); diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp --- a/src/wallet/rpcdump.cpp +++ b/src/wallet/rpcdump.cpp @@ -432,8 +432,8 @@ "Something wrong with merkleblock"); } - wtx.nIndex = txnIndex; - wtx.hashBlock = merkleBlock.header.GetHash(); + wtx.SetConf(CWalletTx::Status::CONFIRMED, merkleBlock.header.GetHash(), + txnIndex); auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -135,11 +135,11 @@ entry.pushKV("generated", true); } if (confirms > 0) { - entry.pushKV("blockhash", wtx.hashBlock.GetHex()); - entry.pushKV("blockindex", wtx.nIndex); + entry.pushKV("blockhash", wtx.m_confirm.hashBlock.GetHex()); + entry.pushKV("blockindex", wtx.m_confirm.nIndex); int64_t block_time; - bool found_block = - chain.findBlock(wtx.hashBlock, nullptr /* block */, &block_time); + bool found_block = chain.findBlock(wtx.m_confirm.hashBlock, + nullptr /* block */, &block_time); assert(found_block); entry.pushKV("blocktime", block_time); } else { diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -280,8 +280,8 @@ LockAssertion lock(::cs_main); LOCK(wallet.cs_wallet); - wtx.hashBlock = ::ChainActive().Tip()->GetBlockHash(); - wtx.nIndex = 0; + wtx.SetConf(CWalletTx::Status::CONFIRMED, + ::ChainActive().Tip()->GetBlockHash(), 0); // Call GetImmatureCredit() once before adding the key to the wallet to // cache the current immature credit amount, which is 0. @@ -313,14 +313,19 @@ } CWalletTx wtx(&wallet, MakeTransactionRef(tx)); - if (block) { - wtx.SetMerkleBranch(block->GetBlockHash(), 0); - } - { - LOCK(cs_main); + LOCK(cs_main); + LOCK(wallet.cs_wallet); + // If transaction is already in map, to avoid inconsistencies, + // unconfirmation is needed before confirm again with different block. + std::map::iterator it = wallet.mapWallet.find(wtx.GetId()); + if (it != wallet.mapWallet.end()) { + wtx.setUnconfirmed(); wallet.AddToWallet(wtx); } - LOCK(wallet.cs_wallet); + if (block) { + wtx.SetConf(CWalletTx::Status::CONFIRMED, block->GetBlockHash(), 0); + } + wallet.AddToWallet(wtx); return wallet.mapWallet.at(wtx.GetId()).nTimeSmart; } @@ -496,7 +501,8 @@ LOCK(wallet->cs_wallet); auto it = wallet->mapWallet.find(tx->GetId()); BOOST_CHECK(it != wallet->mapWallet.end()); - it->second.SetMerkleBranch(::ChainActive().Tip()->GetBlockHash(), 1); + it->second.SetConf(CWalletTx::Status::CONFIRMED, + ::ChainActive().Tip()->GetBlockHash(), 1); return it->second; } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -402,7 +402,10 @@ private: const CWallet *pwallet; - /** Constant used in hashBlock to indicate tx has been abandoned */ + /** + * Constant used in hashBlock to indicate tx has been abandoned, only used + * at serialization/deserialization to avoid ambiguity with conflicted. + */ static const BlockHash ABANDON_HASH; public: @@ -472,7 +475,7 @@ mutable Amount nChangeCached; CWalletTx(const CWallet *pwalletIn, CTransactionRef arg) - : tx(std::move(arg)), hashBlock(BlockHash()), nIndex(-1) { + : tx(std::move(arg)) { Init(pwalletIn); } @@ -488,17 +491,37 @@ fInMempool = false; nChangeCached = Amount::zero(); nOrderPos = -1; + m_confirm = Confirmation{}; } CTransactionRef tx; - BlockHash hashBlock; + /** - * An nIndex == -1 means that hashBlock (in nonzero) refers to the earliest - * block in the chain we know this or any in-wallet dependency conflicts - * with. Older clients interpret nIndex == -1 as unconfirmed for backward - * compatibility. + * New transactions start as UNCONFIRMED. At BlockConnected, + * they will transition to CONFIRMED. In case of reorg, at + * BlockDisconnected, they roll back to UNCONFIRMED. If we detect a + * conflicting transaction at block connection, we update conflicted tx and + * its dependencies as CONFLICTED. If tx isn't confirmed and outside of + * mempool, the user may switch it to ABANDONED by using the + * abandontransaction call. This last status may be override by a CONFLICTED + * or CONFIRMED transition. */ - int nIndex; + enum Status { UNCONFIRMED, CONFIRMED, CONFLICTED, ABANDONED }; + + /** + * Confirmation includes tx status and a pair of {block hash/tx index in + * block} at which tx has been confirmed. This pair is both 0 if tx hasn't + * confirmed yet. Meaning of these fields changes with CONFLICTED state + * where they instead point to block hash and index of the deepest + * conflicting tx. + */ + struct Confirmation { + Status status = UNCONFIRMED; + BlockHash hashBlock = BlockHash(); + int nIndex = 0; + }; + + Confirmation m_confirm; template void Serialize(Stream &s) const { mapValue_t mapValueCopy = mapValue; @@ -515,9 +538,13 @@ std::vector dummy_vector2; //! Used to be fSpent bool dummy_bool = false; - s << tx << hashBlock << dummy_vector1 << nIndex << dummy_vector2 - << mapValueCopy << vOrderForm << fTimeReceivedIsTxTime - << nTimeReceived << fFromMe << dummy_bool; + uint256 serializedHash = + isAbandoned() ? ABANDON_HASH : m_confirm.hashBlock; + int serializedIndex = + isAbandoned() || isConflicted() ? -1 : m_confirm.nIndex; + s << tx << serializedHash << dummy_vector1 << serializedIndex + << dummy_vector2 << mapValueCopy << vOrderForm + << fTimeReceivedIsTxTime << nTimeReceived << fFromMe << dummy_bool; } template void Unserialize(Stream &s) { @@ -529,9 +556,29 @@ std::vector dummy_vector2; //! Used to be fSpent bool dummy_bool; - s >> tx >> hashBlock >> dummy_vector1 >> nIndex >> dummy_vector2 >> - mapValue >> vOrderForm >> fTimeReceivedIsTxTime >> nTimeReceived >> - fFromMe >> dummy_bool; + int serializedIndex; + s >> tx >> m_confirm.hashBlock >> dummy_vector1 >> serializedIndex >> + dummy_vector2 >> mapValue >> vOrderForm >> fTimeReceivedIsTxTime >> + nTimeReceived >> fFromMe >> dummy_bool; + + /* + * At serialization/deserialization, an nIndex == -1 means that + * hashBlock refers to the earliest block in the chain we know this or + * any in-wallet ancestor conflicts with. If nIndex == -1 and hashBlock + * is ABANDON_HASH, it means transaction is abandoned. In same context, + * an nIndex >= 0 refers to a confirmed transaction (if hashBlock set) + * or unconfirmed one. Older clients interpret nIndex == -1 as + * unconfirmed for backward compatibility (pre-commit 9ac63d6). + */ + if (serializedIndex == -1 && m_confirm.hashBlock == ABANDON_HASH) { + m_confirm.hashBlock = BlockHash(); + setAbandoned(); + } else if (serializedIndex == -1) { + setConflicted(); + } else if (!m_confirm.hashBlock.IsNull()) { + m_confirm.nIndex = serializedIndex; + setConfirmed(); + } ReadOrderPos(nOrderPos, mapValue); nTimeSmart = mapValue.count("timesmart") @@ -615,7 +662,7 @@ // in place. std::set GetConflicts() const NO_THREAD_SAFETY_ANALYSIS; - void SetMerkleBranch(const BlockHash &block_hash, int posInBlock); + void SetConf(Status status, const BlockHash &block_hash, int posInBlock); /** * Return depth of transaction in blockchain: @@ -634,12 +681,23 @@ * >0 : is a coinbase transaction which matures in this many blocks */ int GetBlocksToMaturity(interfaces::Chain::Lock &locked_chain) const; - bool hashUnset() const { - return (hashBlock.IsNull() || hashBlock == ABANDON_HASH); + bool isAbandoned() const { + return m_confirm.status == CWalletTx::ABANDONED; } - bool isAbandoned() const { return (hashBlock == ABANDON_HASH); } - void setAbandoned() { hashBlock = ABANDON_HASH; } - + void setAbandoned() { + m_confirm.status = CWalletTx::ABANDONED; + m_confirm.hashBlock = BlockHash(); + m_confirm.nIndex = 0; + } + bool isConflicted() const { + return m_confirm.status == CWalletTx::CONFLICTED; + } + void setConflicted() { m_confirm.status = CWalletTx::CONFLICTED; } + bool isUnconfirmed() const { + return m_confirm.status == CWalletTx::UNCONFIRMED; + } + void setUnconfirmed() { m_confirm.status = CWalletTx::UNCONFIRMED; } + void setConfirmed() { m_confirm.status = CWalletTx::CONFIRMED; } TxId GetId() const { return tx->GetId(); } bool IsCoinBase() const { return tx->IsCoinBase(); } bool IsImmatureCoinBase(interfaces::Chain::Lock &locked_chain) const; @@ -808,6 +866,7 @@ * when necessary. */ bool AddToWalletIfInvolvingMe(const CTransactionRef &tx, + CWalletTx::Status status, const BlockHash &block_hash, int posInBlock, bool fUpdate) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); @@ -834,8 +893,9 @@ * Should be called with non-zero block_hash and posInBlock if this is for a * transaction that is included in a block. */ - void SyncTransaction(const CTransactionRef &tx, const BlockHash &block_hash, - int posInBlock = 0, bool update_tx = true) + void SyncTransaction(const CTransactionRef &tx, CWalletTx::Status status, + const BlockHash &block_hash, int posInBlock = 0, + bool update_tx = true) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); /* the HD chain data model (external chain counters) */ diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1189,21 +1189,14 @@ 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) { @@ -1255,14 +1248,15 @@ auto it = mapWallet.find(txin.prevout.GetTxId()); if (it != mapWallet.end()) { CWalletTx &prevtx = it->second; - if (prevtx.nIndex == -1 && !prevtx.hashUnset()) { - MarkConflicted(prevtx.hashBlock, wtx.GetId()); + if (prevtx.isConflicted()) { + MarkConflicted(prevtx.m_confirm.hashBlock, wtx.GetId()); } } } } bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef &ptx, + CWalletTx::Status status, const BlockHash &block_hash, int posInBlock, bool fUpdate) { const CTransaction &tx = *ptx; @@ -1326,10 +1320,9 @@ 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); } @@ -1391,7 +1384,7 @@ // 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); @@ -1449,8 +1442,9 @@ 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 @@ -1472,9 +1466,11 @@ } void CWallet::SyncTransaction(const CTransactionRef &ptx, + CWalletTx::Status status, const BlockHash &block_hash, int posInBlock, bool update_tx) { - if (!AddToWalletIfInvolvingMe(ptx, block_hash, posInBlock, update_tx)) { + if (!AddToWalletIfInvolvingMe(ptx, status, block_hash, posInBlock, + update_tx)) { // Not one of ours return; } @@ -1488,7 +1484,8 @@ void CWallet::TransactionAddedToMempool(const CTransactionRef &ptx) { auto locked_chain = chain().lock(); LOCK(cs_wallet); - SyncTransaction(ptx, BlockHash(), 0 /* position in block */); + SyncTransaction(ptx, CWalletTx::Status::UNCONFIRMED, + BlockHash() /* block hash */, 0 /* position in block */); auto it = mapWallet.find(ptx->GetId()); if (it != mapWallet.end()) { @@ -1518,12 +1515,15 @@ // transaction and then have it inadvertently cleared by the notification // that the conflicted transaction was evicted. for (const CTransactionRef &ptx : vtxConflicted) { - SyncTransaction(ptx, BlockHash(), 0 /* position in block */); + SyncTransaction(ptx, CWalletTx::Status::CONFLICTED, + BlockHash() /* 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]); } @@ -1534,8 +1534,15 @@ 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, BlockHash(), 0 /* position in block */); + SyncTransaction(ptx, CWalletTx::Status::UNCONFIRMED, + BlockHash() /* block hash */, + 0 /* position in block */); } } @@ -2229,8 +2236,9 @@ } 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 @@ -4469,7 +4477,8 @@ for (const auto &entry : mapWallet) { // iterate over all wallet transactions... const CWalletTx &wtx = entry.second; - if (Optional height = locked_chain.getBlockHeight(wtx.hashBlock)) { + if (Optional 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... @@ -4518,9 +4527,10 @@ */ 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; @@ -4550,7 +4560,8 @@ nTimeSmart = std::max(latestEntry, std::min(blocktime, latestNow)); } else { WalletLogPrintf("%s: found %s in block %s not in index\n", __func__, - wtx.GetId().ToString(), wtx.hashBlock.ToString()); + wtx.GetId().ToString(), + wtx.m_confirm.hashBlock.ToString()); } } return nTimeSmart; @@ -5147,21 +5158,26 @@ m_pre_split = false; } -void CWalletTx::SetMerkleBranch(const BlockHash &block_hash, int posInBlock) { +void CWalletTx::SetConf(Status status, const BlockHash &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()) { + 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(