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 18be3a6ea53..22a5f7e2491 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 84fd01730dd..7629a40c5ea 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 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 @@ -4085,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; @@ -897,6 +946,9 @@ public: bool IsLocked() const; bool Lock(); + /** Interface to assert chain access and if successful lock it */ + std::unique_ptr LockChain() { return m_chain ? m_chain->lock() : nullptr; } + std::map mapWallet GUARDED_BY(cs_wallet); typedef std::multimap TxItems; @@ -1042,7 +1094,7 @@ public: void MarkDirty(); bool AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose=true); - void LoadToWallet(const CWalletTx& wtxIn) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + void LoadToWallet(CWalletTx& wtxIn) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); void TransactionAddedToMempool(const CTransactionRef& tx) override; void BlockConnected(const CBlock& block, const std::vector& vtxConflicted) override; void BlockDisconnected(const CBlock& block) override; diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index ad5673e03a2..dd61efa7577 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -131,6 +131,7 @@ BASE_SCRIPTS = [ 'wallet_createwallet.py --usecli', 'wallet_watchonly.py', 'wallet_watchonly.py --usecli', + 'wallet_reorgsrestore.py', 'interface_http.py', 'interface_rpc.py', 'rpc_psbt.py', diff --git a/test/functional/wallet_reorgsrestore.py b/test/functional/wallet_reorgsrestore.py new file mode 100755 index 00000000000..f48018e9fbe --- /dev/null +++ b/test/functional/wallet_reorgsrestore.py @@ -0,0 +1,105 @@ +#!/usr/bin/env python3 +# Copyright (c) 2019 The Bitcoin Core developers +# Distributed under the MIT software license, see the accompanying +# file COPYING or http://www.opensource.org/licenses/mit-license.php. + +"""Test tx status in case of reorgs while wallet being shutdown. + +Wallet txn status rely on block connection/disconnection for its +accuracy. In case of reorgs happening while wallet being shutdown +block updates are not going to be received. At wallet loading, we +check against chain if confirmed txn are still in chain and change +their status if block in which they have been included has been +disconnected. +""" + +from decimal import Decimal +import os +import shutil + +from test_framework.test_framework import BitcoinTestFramework +from test_framework.util import ( + assert_equal, + connect_nodes, + disconnect_nodes, +) + +class ReorgsRestoreTest(BitcoinTestFramework): + def set_test_params(self): + self.num_nodes = 3 + + def skip_test_if_missing_module(self): + self.skip_if_no_wallet() + + def run_test(self): + # Send a tx from which to conflict outputs later + txid_conflict_from = self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), Decimal("10")) + self.nodes[0].generate(1) + self.sync_blocks() + + # Disconnect node1 from others to reorg its chain later + disconnect_nodes(self.nodes[0], 1) + disconnect_nodes(self.nodes[1], 2) + connect_nodes(self.nodes[0], 2) + + # Send a tx to be unconfirmed later + txid = self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), Decimal("10")) + tx = self.nodes[0].gettransaction(txid) + self.nodes[0].generate(4) + tx_before_reorg = self.nodes[0].gettransaction(txid) + assert_equal(tx_before_reorg["confirmations"], 4) + + # Disconnect node0 from node2 to broadcast a conflict on their respective chains + disconnect_nodes(self.nodes[0], 2) + nA = next(tx_out["vout"] for tx_out in self.nodes[0].gettransaction(txid_conflict_from)["details"] if tx_out["amount"] == Decimal("10")) + inputs = [] + inputs.append({"txid": txid_conflict_from, "vout": nA}) + outputs_1 = {} + outputs_2 = {} + + # Create a conflicted tx broadcast on node0 chain and conflicting tx broadcast on node1 chain. Both spend from txid_conflict_from + outputs_1[self.nodes[0].getnewaddress()] = Decimal("9.99998") + outputs_2[self.nodes[0].getnewaddress()] = Decimal("9.99998") + conflicted = self.nodes[0].signrawtransactionwithwallet(self.nodes[0].createrawtransaction(inputs, outputs_1)) + conflicting = self.nodes[0].signrawtransactionwithwallet(self.nodes[0].createrawtransaction(inputs, outputs_2)) + + conflicted_txid = self.nodes[0].sendrawtransaction(conflicted["hex"]) + self.nodes[0].generate(1) + conflicting_txid = self.nodes[2].sendrawtransaction(conflicting["hex"]) + self.nodes[2].generate(9) + + # Reconnect node0 and node2 and check that conflicted_txid is effectively conflicted + connect_nodes(self.nodes[0], 2) + self.sync_blocks([self.nodes[0], self.nodes[2]]) + conflicted = self.nodes[0].gettransaction(conflicted_txid) + conflicting = self.nodes[0].gettransaction(conflicting_txid) + assert_equal(conflicted["confirmations"], -9) + assert_equal(conflicted["walletconflicts"][0], conflicting["txid"]) + + # Node0 wallet is shutdown + self.stop_node(0) + self.start_node(0) + + # The block chain re-orgs and the tx is included in a different block + self.nodes[1].generate(9) + self.nodes[1].sendrawtransaction(tx["hex"]) + self.nodes[1].generate(1) + self.nodes[1].sendrawtransaction(conflicted["hex"]) + self.nodes[1].generate(1) + + # Node0 wallet file is loaded on longest sync'ed node1 + self.stop_node(1) + self.nodes[0].backupwallet(os.path.join(self.nodes[0].datadir, 'wallet.bak')) + shutil.copyfile(os.path.join(self.nodes[0].datadir, 'wallet.bak'), os.path.join(self.nodes[1].datadir, 'regtest', 'wallet.dat')) + self.start_node(1) + tx_after_reorg = self.nodes[1].gettransaction(txid) + # Check that normal confirmed tx is confirmed again but with different blockhash + assert_equal(tx_after_reorg["confirmations"], 2) + assert(tx_before_reorg["blockhash"] != tx_after_reorg["blockhash"]) + conflicted_after_reorg = self.nodes[1].gettransaction(conflicted_txid) + # Check that conflicted tx is confirmed again with blockhash different than previously conflicting tx + assert_equal(conflicted_after_reorg["confirmations"], 1) + assert(conflicting["blockhash"] != conflicted_after_reorg["blockhash"]) + +if __name__ == '__main__': + ReorgsRestoreTest().main()