diff options
-rw-r--r-- | src/interfaces/wallet.cpp | 2 | ||||
-rw-r--r-- | src/wallet/rpcdump.cpp | 3 | ||||
-rw-r--r-- | src/wallet/rpcwallet.cpp | 6 | ||||
-rw-r--r-- | src/wallet/test/wallet_tests.cpp | 22 | ||||
-rw-r--r-- | src/wallet/wallet.cpp | 82 | ||||
-rw-r--r-- | 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 077dc1ab4d..0c8d92eba5 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<int>::max()); + result.block_height = locked_chain.getBlockHeight(wtx.m_confirm.hashBlock).get_value_or(std::numeric_limits<int>::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 7707d6233b..f52e4318c8 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 30e767e489..8a11f76609 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 8af05dea45..fc3be2b6ab 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<uint256, CWalletTx>::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 03acf23508..098f7990ad 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<CTransaction // the notification that the conflicted transaction was evicted. for (const CTransactionRef& ptx : vtxConflicted) { - SyncTransaction(ptx, {} /* block hash */, 0 /* position in block */); + SyncTransaction(ptx, CWalletTx::Status::CONFLICTED, {} /* 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]); } @@ -1440,8 +1434,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 */); } } @@ -2078,7 +2076,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; @@ -4050,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 @@ -4093,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; @@ -4123,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; @@ -4634,21 +4632,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 diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 984be3e301..d50ba2fb91 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -396,7 +396,9 @@ class CWalletTx 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 uint256 ABANDON_HASH; public: @@ -457,9 +459,7 @@ public: mutable CAmount nChangeCached; CWalletTx(const CWallet* pwalletIn, CTransactionRef arg) - : tx(std::move(arg)), - hashBlock(uint256()), - nIndex(-1) + : tx(std::move(arg)) { Init(pwalletIn); } @@ -477,16 +477,37 @@ public: fInMempool = false; nChangeCached = 0; nOrderPos = -1; + m_confirm = Confirmation{}; } CTransactionRef tx; - uint256 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. + */ + 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. */ - int nIndex; + struct Confirmation { + Status status = UNCONFIRMED; + uint256 hashBlock = uint256(); + int nIndex = 0; + }; + + Confirmation m_confirm; template<typename Stream> void Serialize(Stream& s) const @@ -502,7 +523,9 @@ public: std::vector<char> dummy_vector1; //!< Used to be vMerkleBranch std::vector<char> 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<typename Stream> @@ -513,7 +536,25 @@ public: std::vector<uint256> dummy_vector1; //!< Used to be vMerkleBranch std::vector<CMerkleTx> 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<uint256> 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; |