From a31be09bfd77eed497a8e251d31358e16e2f2eb1 Mon Sep 17 00:00:00 2001 From: Antoine Riard Date: Mon, 12 Aug 2019 18:12:12 -0400 Subject: [PATCH] Encapsulate tx status in a Confirmation struct Instead of relying on combination of hashBlock and nIndex values to manage tx in its lifecycle, we introduce 4 status : CONFIRMED, UNCONFIRMED, CONFLICTED, ABANDONED. hashBlock and nIndex magic values should only be used at serialization/deserialization for backward-compatibility. At block disconnection, we know flag txn as UNCONFIRMED where previously they kept their states until being override by a block connection or abandontransaction call. This is a change in behavior for which user may have to call abandon twice if transaction is disconnected and not accepted back in the mempool. We assert status transitioning right in AddToWallet. Doing so flagged a misbehavior in ComputeTimeSmart unit test where same tx is confirmed twice in different block. To avoid inconsistencies we unconfirmed tx before new connection in different block. We also remove a cs_main lock in test, as AddToWallet and its callees don't rely on locked chain. --- src/interfaces/wallet.cpp | 2 +- src/wallet/rpcdump.cpp | 3 +- src/wallet/rpcwallet.cpp | 6 +-- src/wallet/test/wallet_tests.cpp | 22 ++++---- src/wallet/wallet.cpp | 82 +++++++++++++++--------------- src/wallet/wallet.h | 87 +++++++++++++++++++++++++------- 6 files changed, 127 insertions(+), 75 deletions(-) diff --git a/src/interfaces/wallet.cpp b/src/interfaces/wallet.cpp index 077dc1ab4d2..0c8d92eba50 100644 --- a/src/interfaces/wallet.cpp +++ b/src/interfaces/wallet.cpp @@ -65,7 +65,7 @@ WalletTx MakeWalletTx(interfaces::Chain::Lock& locked_chain, CWallet& wallet, co WalletTxStatus MakeWalletTxStatus(interfaces::Chain::Lock& locked_chain, const CWalletTx& wtx) { WalletTxStatus result; - result.block_height = locked_chain.getBlockHeight(wtx.hashBlock).get_value_or(std::numeric_limits::max()); + result.block_height = 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); result.time_received = wtx.nTimeReceived; diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp index 7707d6233ba..f52e4318c8e 100644 --- a/src/wallet/rpcdump.cpp +++ b/src/wallet/rpcdump.cpp @@ -384,8 +384,7 @@ UniValue importprunedfunds(const JSONRPCRequest& request) throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "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 index 30e767e4899..8a11f766093 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -134,10 +134,10 @@ static void WalletTxToJSON(interfaces::Chain& chain, interfaces::Chain::Lock& lo 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 index 8af05dea458..fc3be2b6abf 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -249,8 +249,7 @@ BOOST_FIXTURE_TEST_CASE(coin_mark_dirty_immature_credit, TestChain100Setup) 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. @@ -281,14 +280,19 @@ static int64_t AddTx(CWallet& wallet, uint32_t lockTime, int64_t mockTime, int64 } 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.GetHash()); + 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.GetHash()).nTimeSmart; } @@ -382,7 +386,7 @@ public: LOCK(wallet->cs_wallet); auto it = wallet->mapWallet.find(tx->GetHash()); 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.cpp b/src/wallet/wallet.cpp index 03acf23508b..098f7990ad6 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1118,22 +1118,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) { @@ -1194,14 +1186,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; { @@ -1248,9 +1240,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); } @@ -1310,7 +1302,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); @@ -1364,8 +1356,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 @@ -1383,8 +1376,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 @@ -1396,7 +1390,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()) { @@ -1425,11 +1419,11 @@ void CWallet::BlockConnected(const CBlock& block, const std::vector 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 @@ -4093,9 +4091,9 @@ void CWallet::GetKeyBirthTimes(interfaces::Chain::Lock& locked_chain, std::map void Serialize(Stream& s) const @@ -502,7 +523,9 @@ public: std::vector dummy_vector1; //!< Used to be vMerkleBranch std::vector dummy_vector2; //!< Used to be vtxPrev bool dummy_bool = false; //!< Used to be fSpent - 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 @@ -513,7 +536,25 @@ public: std::vector dummy_vector1; //!< Used to be vMerkleBranch std::vector dummy_vector2; //!< Used to be vtxPrev bool dummy_bool; //! Used to be fSpent - 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 = uint256(); + setAbandoned(); + } else if (serializedIndex == -1) { + setConflicted(); + } else if (!m_confirm.hashBlock.IsNull()) { + m_confirm.nIndex = serializedIndex; + setConfirmed(); + } ReadOrderPos(nOrderPos, mapValue); nTimeSmart = mapValue.count("timesmart") ? (unsigned int)atoi64(mapValue["timesmart"]) : 0; @@ -590,7 +631,7 @@ public: // in place. std::set GetConflicts() const NO_THREAD_SAFETY_ANALYSIS; - void SetMerkleBranch(const uint256& block_hash, int posInBlock); + void SetConf(Status status, const uint256& block_hash, int posInBlock); /** * Return depth of transaction in blockchain: @@ -607,10 +648,18 @@ public: * >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 (hashBlock == ABANDON_HASH); } - void setAbandoned() { hashBlock = ABANDON_HASH; } - + bool isAbandoned() const { return m_confirm.status == CWalletTx::ABANDONED; } + void setAbandoned() + { + m_confirm.status = CWalletTx::ABANDONED; + m_confirm.hashBlock = uint256(); + 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; } const uint256& GetHash() const { return tx->GetHash(); } bool IsCoinBase() const { return tx->IsCoinBase(); } bool IsImmatureCoinBase(interfaces::Chain::Lock& locked_chain) const; @@ -750,7 +799,7 @@ private: * Abandoned state should probably be more carefully tracked via different * posInBlock signals or by checking mempool presence when necessary. */ - bool AddToWalletIfInvolvingMe(const CTransactionRef& tx, const uint256& block_hash, int posInBlock, bool fUpdate) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + bool AddToWalletIfInvolvingMe(const CTransactionRef& tx, CWalletTx::Status status, const uint256& block_hash, int posInBlock, bool fUpdate) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); /* Mark a transaction (and its in-wallet descendants) as conflicting with a particular block. */ void MarkConflicted(const uint256& hashBlock, const uint256& hashTx); @@ -762,7 +811,7 @@ private: /* Used by TransactionAddedToMemorypool/BlockConnected/Disconnected/ScanForWalletTransactions. * 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 uint256& block_hash, int posInBlock = 0, bool update_tx = true) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + void SyncTransaction(const CTransactionRef& tx, CWalletTx::Status status, const uint256& block_hash, int posInBlock = 0, bool update_tx = true) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); /* the HD chain data model (external chain counters) */ CHDChain hdChain;