From 700c42b85d20e624bef4228eef062c93084efab5 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Thu, 27 Jul 2017 10:08:31 -0400 Subject: Add height, depth, and hash methods to the Chain interface And use them to remove uses of chainActive and mapBlockIndex in wallet code This commit does not change behavior. Co-authored-by: Ben Woosley --- src/Makefile.am | 1 + src/interfaces/chain.cpp | 30 ++++++++++++++++++++++++++++++ src/interfaces/chain.h | 20 ++++++++++++++++++++ src/interfaces/wallet.cpp | 6 +++--- src/optional.h | 17 +++++++++++++++++ src/wallet/rpcdump.cpp | 6 +++--- src/wallet/rpcwallet.cpp | 7 ++++--- src/wallet/test/wallet_tests.cpp | 4 ++-- src/wallet/wallet.cpp | 36 ++++++++++++++---------------------- src/wallet/wallet.h | 4 ++-- 10 files changed, 96 insertions(+), 35 deletions(-) create mode 100644 src/optional.h diff --git a/src/Makefile.am b/src/Makefile.am index 09daaebd23..4a98a2c1d8 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -145,6 +145,7 @@ BITCOIN_CORE_H = \ netbase.h \ netmessagemaker.h \ noui.h \ + optional.h \ outputtype.h \ policy/feerate.h \ policy/fees.h \ diff --git a/src/interfaces/chain.cpp b/src/interfaces/chain.cpp index 2571a91031..138d1bfd5f 100644 --- a/src/interfaces/chain.cpp +++ b/src/interfaces/chain.cpp @@ -4,7 +4,9 @@ #include +#include #include +#include #include #include @@ -16,6 +18,34 @@ namespace { class LockImpl : public Chain::Lock { + Optional getHeight() override + { + int height = ::chainActive.Height(); + if (height >= 0) { + return height; + } + return nullopt; + } + Optional getBlockHeight(const uint256& hash) override + { + CBlockIndex* block = LookupBlockIndex(hash); + if (block && ::chainActive.Contains(block)) { + return block->nHeight; + } + return nullopt; + } + int getBlockDepth(const uint256& hash) override + { + const Optional tip_height = getHeight(); + const Optional height = getBlockHeight(hash); + return tip_height && height ? *tip_height - *height + 1 : 0; + } + uint256 getBlockHash(int height) override + { + CBlockIndex* block = ::chainActive[height]; + assert(block != nullptr); + return block->GetBlockHash(); + } }; class LockingStateImpl : public LockImpl, public UniqueLock diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h index fe5658de4b..928b8af1f4 100644 --- a/src/interfaces/chain.h +++ b/src/interfaces/chain.h @@ -5,11 +5,14 @@ #ifndef BITCOIN_INTERFACES_CHAIN_H #define BITCOIN_INTERFACES_CHAIN_H +#include + #include #include #include class CScheduler; +class uint256; namespace interfaces { @@ -28,6 +31,23 @@ public: { public: virtual ~Lock() {} + + //! Get current chain height, not including genesis block (returns 0 if + //! chain only contains genesis block, nullopt if chain does not contain + //! any blocks). + virtual Optional getHeight() = 0; + + //! Get block height above genesis block. Returns 0 for genesis block, + //! 1 for following block, and so on. Returns nullopt for a block not + //! included in the current chain. + virtual Optional getBlockHeight(const uint256& hash) = 0; + + //! Get block depth. Returns 1 for chain tip, 2 for preceding block, and + //! so on. Returns 0 for a block not included in the current chain. + virtual int getBlockDepth(const uint256& hash) = 0; + + //! Get block hash. Height must be valid or this function will abort. + virtual uint256 getBlockHash(int height) = 0; }; //! Return Lock interface. Chain is locked when this is called, and diff --git a/src/interfaces/wallet.cpp b/src/interfaces/wallet.cpp index 8db34ed759..489cf0981c 100644 --- a/src/interfaces/wallet.cpp +++ b/src/interfaces/wallet.cpp @@ -333,7 +333,7 @@ public: if (mi == m_wallet.mapWallet.end()) { return false; } - num_blocks = ::chainActive.Height(); + num_blocks = locked_chain->getHeight().value_or(-1); block_time = ::chainActive.Tip()->GetBlockTime(); tx_status = MakeWalletTxStatus(*locked_chain, mi->second); return true; @@ -348,7 +348,7 @@ public: LOCK(m_wallet.cs_wallet); auto mi = m_wallet.mapWallet.find(txid); if (mi != m_wallet.mapWallet.end()) { - num_blocks = ::chainActive.Height(); + num_blocks = locked_chain->getHeight().value_or(-1); in_mempool = mi->second.InMempool(); order_form = mi->second.vOrderForm; tx_status = MakeWalletTxStatus(*locked_chain, mi->second); @@ -379,7 +379,7 @@ public: return false; } balances = getBalances(); - num_blocks = ::chainActive.Height(); + num_blocks = locked_chain->getHeight().value_or(-1); return true; } CAmount getBalance() override { return m_wallet.GetBalance(); } diff --git a/src/optional.h b/src/optional.h new file mode 100644 index 0000000000..1614c89718 --- /dev/null +++ b/src/optional.h @@ -0,0 +1,17 @@ +// Copyright (c) 2017 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#ifndef BITCOIN_OPTIONAL_H +#define BITCOIN_OPTIONAL_H + +#include + +//! Substitute for C++17 std::optional +template +using Optional = boost::optional; + +//! Substitute for C++17 std::nullopt +static auto& nullopt = boost::none; + +#endif // BITCOIN_OPTIONAL_H diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp index ed1e2d3940..f2b91e7c3d 100644 --- a/src/wallet/rpcdump.cpp +++ b/src/wallet/rpcdump.cpp @@ -379,8 +379,7 @@ UniValue importprunedfunds(const JSONRPCRequest& request) if (merkleBlock.txn.ExtractMatches(vMatch, vIndex) == merkleBlock.header.hashMerkleRoot) { auto locked_chain = pwallet->chain().lock(); - const CBlockIndex* pindex = LookupBlockIndex(merkleBlock.header.GetHash()); - if (!pindex || !chainActive.Contains(pindex)) { + if (locked_chain->getBlockHeight(merkleBlock.header.GetHash()) == nullopt) { throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block not found in chain"); } @@ -773,7 +772,8 @@ UniValue dumpwallet(const JSONRPCRequest& request) // produce output file << strprintf("# Wallet dump created by Bitcoin %s\n", CLIENT_BUILD); file << strprintf("# * Created on %s\n", FormatISO8601DateTime(GetTime())); - file << strprintf("# * Best block at time of backup was %i (%s),\n", chainActive.Height(), chainActive.Tip()->GetBlockHash().ToString()); + const Optional tip_height = locked_chain->getHeight(); + file << strprintf("# * Best block at time of backup was %i (%s),\n", tip_height.value_or(-1), tip_height ? locked_chain->getBlockHash(*tip_height).ToString() : "(missing block hash)"); file << strprintf("# mined on %s\n", FormatISO8601DateTime(chainActive.Tip()->GetBlockTime())); file << "\n"; diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 39c17743ec..c835d2928d 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -1607,7 +1607,8 @@ static UniValue listsinceblock(const JSONRPCRequest& request) bool include_removed = (request.params[3].isNull() || request.params[3].get_bool()); - int depth = pindex ? (1 + chainActive.Height() - pindex->nHeight) : -1; + const Optional tip_height = locked_chain->getHeight(); + int depth = tip_height && pindex ? (1 + *tip_height - pindex->nHeight) : -1; UniValue transactions(UniValue::VARR); @@ -1638,8 +1639,8 @@ static UniValue listsinceblock(const JSONRPCRequest& request) paltindex = paltindex->pprev; } - CBlockIndex *pblockLast = chainActive[chainActive.Height() + 1 - target_confirms]; - uint256 lastblock = pblockLast ? pblockLast->GetBlockHash() : uint256(); + int last_height = tip_height ? *tip_height + 1 - target_confirms : -1; + uint256 lastblock = last_height >= 0 ? locked_chain->getBlockHash(last_height) : uint256(); UniValue ret(UniValue::VOBJ); ret.pushKV("transactions", transactions); diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 0db22cf6fe..4377e9f29d 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -276,7 +276,7 @@ static int64_t AddTx(CWallet& wallet, uint32_t lockTime, int64_t mockTime, int64 CWalletTx wtx(&wallet, MakeTransactionRef(tx)); if (block) { - wtx.SetMerkleBranch(block, 0); + wtx.SetMerkleBranch(block->GetBlockHash(), 0); } { LOCK(cs_main); @@ -372,7 +372,7 @@ public: LOCK(wallet->cs_wallet); auto it = wallet->mapWallet.find(tx->GetHash()); BOOST_CHECK(it != wallet->mapWallet.end()); - it->second.SetMerkleBranch(chainActive.Tip(), 1); + it->second.SetMerkleBranch(chainActive.Tip()->GetBlockHash(), 1); return it->second; } diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 098055673b..c34295a11a 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -935,19 +935,19 @@ void CWallet::LoadToWallet(const CWalletTx& wtxIn) } } -bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, const CBlockIndex* pIndex, int posInBlock, bool fUpdate) +bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, const uint256& block_hash, int posInBlock, bool fUpdate) { const CTransaction& tx = *ptx; { AssertLockHeld(cs_wallet); - if (pIndex != nullptr) { + if (!block_hash.IsNull()) { for (const CTxIn& txin : tx.vin) { std::pair range = mapTxSpends.equal_range(txin.prevout); while (range.first != range.second) { if (range.first->second != tx.GetHash()) { - WalletLogPrintf("Transaction %s (in block %s) conflicts with wallet transaction %s (both spend %s:%i)\n", tx.GetHash().ToString(), pIndex->GetBlockHash().ToString(), range.first->second.ToString(), range.first->first.hash.ToString(), range.first->first.n); - MarkConflicted(pIndex->GetBlockHash(), range.first->second); + WalletLogPrintf("Transaction %s (in block %s) conflicts with wallet transaction %s (both spend %s:%i)\n", tx.GetHash().ToString(), block_hash.ToString(), range.first->second.ToString(), range.first->first.hash.ToString(), range.first->first.n); + MarkConflicted(block_hash, range.first->second); } range.first++; } @@ -983,8 +983,8 @@ bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, const CBlockI CWalletTx wtx(this, ptx); // Get merkle branch if transaction was found in a block - if (pIndex != nullptr) - wtx.SetMerkleBranch(pIndex, posInBlock); + if (!block_hash.IsNull()) + wtx.SetMerkleBranch(block_hash, posInBlock); return AddToWallet(wtx, false); } @@ -1071,11 +1071,7 @@ void CWallet::MarkConflicted(const uint256& hashBlock, const uint256& hashTx) auto locked_chain = chain().lock(); LOCK(cs_wallet); - int conflictconfirms = 0; - CBlockIndex* pindex = LookupBlockIndex(hashBlock); - if (pindex && chainActive.Contains(pindex)) { - conflictconfirms = -(chainActive.Height() - pindex->nHeight + 1); - } + int conflictconfirms = -locked_chain->getBlockDepth(hashBlock); // If number of conflict confirms cannot be determined, this means // that the block is still unknown or not yet part of the main chain, // for example when loading the wallet during a reindex. Do nothing in that @@ -1122,7 +1118,7 @@ void CWallet::MarkConflicted(const uint256& hashBlock, const uint256& hashTx) } void CWallet::SyncTransaction(const CTransactionRef& ptx, const CBlockIndex *pindex, int posInBlock, bool update_tx) { - if (!AddToWalletIfInvolvingMe(ptx, pindex, posInBlock, update_tx)) + if (!AddToWalletIfInvolvingMe(ptx, pindex->GetBlockHash(), posInBlock, update_tx)) return; // Not one of ours // If a transaction changes 'conflicted' state, that changes the balance @@ -2573,6 +2569,7 @@ static bool IsCurrentForAntiFeeSniping(interfaces::Chain::Lock& locked_chain) */ static uint32_t GetLocktimeForNewTransaction(interfaces::Chain::Lock& locked_chain) { + uint32_t const height = locked_chain.getHeight().value_or(-1); uint32_t locktime; // Discourage fee sniping. // @@ -2595,7 +2592,7 @@ static uint32_t GetLocktimeForNewTransaction(interfaces::Chain::Lock& locked_cha // now we ensure code won't be written that makes assumptions about // nLockTime that preclude a fix later. if (IsCurrentForAntiFeeSniping(locked_chain)) { - locktime = chainActive.Height(); + locktime = height; // Secondly occasionally randomly pick a nLockTime even further back, so // that transactions that are delayed after signing for whatever reason, @@ -2609,7 +2606,7 @@ static uint32_t GetLocktimeForNewTransaction(interfaces::Chain::Lock& locked_cha // unique "nLockTime fingerprint", set nLockTime to a constant. locktime = 0; } - assert(locktime <= (unsigned int)chainActive.Height()); + assert(locktime <= height); assert(locktime < LOCKTIME_THRESHOLD); return locktime; } @@ -4282,10 +4279,10 @@ CWalletKey::CWalletKey(int64_t nExpires) nTimeExpires = nExpires; } -void CMerkleTx::SetMerkleBranch(const CBlockIndex* pindex, int posInBlock) +void CMerkleTx::SetMerkleBranch(const uint256& block_hash, int posInBlock) { // Update the tx's hashBlock - hashBlock = pindex->GetBlockHash(); + hashBlock = block_hash; // set the position of the transaction in the block nIndex = posInBlock; @@ -4298,12 +4295,7 @@ int CMerkleTx::GetDepthInMainChain(interfaces::Chain::Lock& locked_chain) const AssertLockHeld(cs_main); - // Find the block it claims to be in - CBlockIndex* pindex = LookupBlockIndex(hashBlock); - if (!pindex || !chainActive.Contains(pindex)) - return 0; - - return ((nIndex == -1) ? (-1) : 1) * (chainActive.Height() - pindex->nHeight + 1); + return locked_chain.getBlockDepth(hashBlock) * (nIndex == -1 ? -1 : 1); } int CMerkleTx::GetBlocksToMaturity(interfaces::Chain::Lock& locked_chain) const diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 6872fbad2d..d8981f7385 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -287,7 +287,7 @@ public: READWRITE(nIndex); } - void SetMerkleBranch(const CBlockIndex* pIndex, int posInBlock); + void SetMerkleBranch(const uint256& block_hash, int posInBlock); /** * Return depth of transaction in blockchain: @@ -667,7 +667,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 CBlockIndex* pIndex, int posInBlock, bool fUpdate) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + bool AddToWalletIfInvolvingMe(const CTransactionRef& tx, 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); -- cgit v1.2.3