From 629d75faac84bc0a00533d01dd291a4e6394a51f Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Tue, 2 Sep 2014 21:21:15 +0200 Subject: [PATCH] Combine CCoinsViewCache's HaveCoins and const GetCoins into AccessCoins. The efficient version of CCoinsViewCache::GetCoins only works for known-to-exist cache entries, requiring a separate HaveCoins call beforehand. This is inefficient as both perform a hashtable lookup. Replace the non-mutable GetCoins with AccessCoins, which returns a potentially-NULL pointer. This also decreases the overloading of GetCoins. Also replace some copying (inefficient) GetCoins calls with equivalent AccessCoins, decreasing the copying. --- src/bitcoin-tx.cpp | 6 +++--- src/coins.cpp | 38 ++++++++++++++++++-------------------- src/coins.h | 8 +++++--- src/main.cpp | 30 ++++++++++++++++-------------- src/miner.cpp | 7 ++++--- src/rpcrawtransaction.cpp | 12 ++++++------ src/txmempool.cpp | 4 ++-- 7 files changed, 54 insertions(+), 51 deletions(-) diff --git a/src/bitcoin-tx.cpp b/src/bitcoin-tx.cpp index 5f547bba83..c8cd9edfa4 100644 --- a/src/bitcoin-tx.cpp +++ b/src/bitcoin-tx.cpp @@ -418,12 +418,12 @@ static void MutateTxSign(CMutableTransaction& tx, const string& flagStr) // Sign what we can: for (unsigned int i = 0; i < mergedTx.vin.size(); i++) { CTxIn& txin = mergedTx.vin[i]; - CCoins coins; - if (!view.GetCoins(txin.prevout.hash, coins) || !coins.IsAvailable(txin.prevout.n)) { + const CCoins* coins = view.AccessCoins(txin.prevout.hash); + if (!coins || !coins->IsAvailable(txin.prevout.n)) { fComplete = false; continue; } - const CScript& prevPubKey = coins.vout[txin.prevout.n].scriptPubKey; + const CScript& prevPubKey = coins->vout[txin.prevout.n].scriptPubKey; txin.scriptSig.clear(); // Only sign SIGHASH_SINGLE if there's a corresponding output: diff --git a/src/coins.cpp b/src/coins.cpp index 7bfb84ef3e..34485db2bd 100644 --- a/src/coins.cpp +++ b/src/coins.cpp @@ -110,9 +110,13 @@ CCoins &CCoinsViewCache::GetCoins(const uint256 &txid) { return it->second; } -const CCoins &CCoinsViewCache::GetCoins(const uint256 &txid) const { - /* Avoid redundant implementation with the const-cast. */ - return const_cast(this)->GetCoins(txid); +const CCoins* CCoinsViewCache::AccessCoins(const uint256 &txid) const { + CCoinsMap::const_iterator it = FetchCoins(txid); + if (it == cacheCoins.end()) { + return NULL; + } else { + return &it->second; + } } bool CCoinsViewCache::SetCoins(const uint256 &txid, const CCoins &coins) { @@ -162,9 +166,9 @@ unsigned int CCoinsViewCache::GetCacheSize() const { const CTxOut &CCoinsViewCache::GetOutputFor(const CTxIn& input) const { - const CCoins &coins = GetCoins(input.prevout.hash); - assert(coins.IsAvailable(input.prevout.n)); - return coins.vout[input.prevout.n]; + const CCoins* coins = AccessCoins(input.prevout.hash); + assert(coins && coins->IsAvailable(input.prevout.n)); + return coins->vout[input.prevout.n]; } int64_t CCoinsViewCache::GetValueIn(const CTransaction& tx) const @@ -182,19 +186,12 @@ int64_t CCoinsViewCache::GetValueIn(const CTransaction& tx) const bool CCoinsViewCache::HaveInputs(const CTransaction& tx) const { if (!tx.IsCoinBase()) { - // first check whether information about the prevout hash is available for (unsigned int i = 0; i < tx.vin.size(); i++) { const COutPoint &prevout = tx.vin[i].prevout; - if (!HaveCoins(prevout.hash)) - return false; - } - - // then check whether the actual outputs are available - for (unsigned int i = 0; i < tx.vin.size(); i++) { - const COutPoint &prevout = tx.vin[i].prevout; - const CCoins &coins = GetCoins(prevout.hash); - if (!coins.IsAvailable(prevout.n)) + const CCoins* coins = AccessCoins(prevout.hash); + if (!coins || !coins->IsAvailable(prevout.n)) { return false; + } } } return true; @@ -207,10 +204,11 @@ double CCoinsViewCache::GetPriority(const CTransaction &tx, int nHeight) const double dResult = 0.0; BOOST_FOREACH(const CTxIn& txin, tx.vin) { - const CCoins &coins = GetCoins(txin.prevout.hash); - if (!coins.IsAvailable(txin.prevout.n)) continue; - if (coins.nHeight < nHeight) { - dResult += coins.vout[txin.prevout.n].nValue * (nHeight-coins.nHeight); + const CCoins* coins = AccessCoins(txin.prevout.hash); + assert(coins); + if (!coins->IsAvailable(txin.prevout.n)) continue; + if (coins->nHeight < nHeight) { + dResult += coins->vout[txin.prevout.n].nValue * (nHeight-coins->nHeight); } } return tx.ComputePriority(dResult); diff --git a/src/coins.h b/src/coins.h index d338e3172e..c25393f1e0 100644 --- a/src/coins.h +++ b/src/coins.h @@ -344,11 +344,13 @@ public: bool SetBestBlock(const uint256 &hashBlock); bool BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock); + // Return a pointer to CCoins in the cache, or NULL if not found. This is + // more efficient than GetCoins. Modifications to other cache entries are + // allowed while accessing the returned pointer. + const CCoins* AccessCoins(const uint256 &txid) const; + // Return a modifiable reference to a CCoins. Check HaveCoins first. - // Many methods explicitly require a CCoinsViewCache because of this method, to reduce - // copying. CCoins &GetCoins(const uint256 &txid); - const CCoins &GetCoins(const uint256 &txid) const; // Push the modifications applied to this cache to its base. // Failure to call this method before destruction will cause the changes to be forgotten. diff --git a/src/main.cpp b/src/main.cpp index 4aebdadd3b..bea01ab7c9 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -1017,9 +1017,9 @@ bool GetTransaction(const uint256 &hash, CTransaction &txOut, uint256 &hashBlock int nHeight = -1; { CCoinsViewCache &view = *pcoinsTip; - CCoins coins; - if (view.GetCoins(hash, coins)) - nHeight = coins.nHeight; + const CCoins* coins = view.AccessCoins(hash); + if (coins) + nHeight = coins->nHeight; } if (nHeight > 0) pindexSlow = chainActive[nHeight]; @@ -1371,19 +1371,20 @@ bool CheckInputs(const CTransaction& tx, CValidationState &state, const CCoinsVi for (unsigned int i = 0; i < tx.vin.size(); i++) { const COutPoint &prevout = tx.vin[i].prevout; - const CCoins &coins = inputs.GetCoins(prevout.hash); + const CCoins *coins = inputs.AccessCoins(prevout.hash); + assert(coins); // If prev is coinbase, check that it's matured - if (coins.IsCoinBase()) { - if (nSpendHeight - coins.nHeight < COINBASE_MATURITY) + if (coins->IsCoinBase()) { + if (nSpendHeight - coins->nHeight < COINBASE_MATURITY) return state.Invalid( - error("CheckInputs() : tried to spend coinbase at depth %d", nSpendHeight - coins.nHeight), + error("CheckInputs() : tried to spend coinbase at depth %d", nSpendHeight - coins->nHeight), REJECT_INVALID, "bad-txns-premature-spend-of-coinbase"); } // Check for negative or overflow input values - nValueIn += coins.vout[prevout.n].nValue; - if (!MoneyRange(coins.vout[prevout.n].nValue) || !MoneyRange(nValueIn)) + nValueIn += coins->vout[prevout.n].nValue; + if (!MoneyRange(coins->vout[prevout.n].nValue) || !MoneyRange(nValueIn)) return state.DoS(100, error("CheckInputs() : txin values out of range"), REJECT_INVALID, "bad-txns-inputvalues-outofrange"); @@ -1413,10 +1414,11 @@ bool CheckInputs(const CTransaction& tx, CValidationState &state, const CCoinsVi if (fScriptChecks) { for (unsigned int i = 0; i < tx.vin.size(); i++) { const COutPoint &prevout = tx.vin[i].prevout; - const CCoins &coins = inputs.GetCoins(prevout.hash); + const CCoins* coins = inputs.AccessCoins(prevout.hash); + assert(coins); // Verify signature - CScriptCheck check(coins, tx, i, flags, 0); + CScriptCheck check(*coins, tx, i, flags, 0); if (pvChecks) { pvChecks->push_back(CScriptCheck()); check.swap(pvChecks->back()); @@ -1428,7 +1430,7 @@ bool CheckInputs(const CTransaction& tx, CValidationState &state, const CCoinsVi // arguments; if so, don't trigger DoS protection to // avoid splitting the network between upgraded and // non-upgraded nodes. - CScriptCheck check(coins, tx, i, + CScriptCheck check(*coins, tx, i, flags & ~STANDARD_NOT_MANDATORY_VERIFY_FLAGS, 0); if (check()) return state.Invalid(false, REJECT_NONSTANDARD, "non-mandatory-script-verify-flag"); @@ -1614,8 +1616,8 @@ bool ConnectBlock(CBlock& block, CValidationState& state, CBlockIndex* pindex, C (pindex->nHeight==91880 && pindex->GetBlockHash() == uint256("0x00000000000743f190a18c5577a3c2d2a1f610ae9601ac046a38084ccb7cd721"))); if (fEnforceBIP30) { BOOST_FOREACH(const CTransaction& tx, block.vtx) { - const uint256& hash = tx.GetHash(); - if (view.HaveCoins(hash) && !view.GetCoins(hash).IsPruned()) + const CCoins* coins = view.AccessCoins(tx.GetHash()); + if (coins && !coins->IsPruned()) return state.DoS(100, error("ConnectBlock() : tried to overwrite transaction"), REJECT_INVALID, "bad-txns-BIP30"); } diff --git a/src/miner.cpp b/src/miner.cpp index 96dc80a26d..d05ddbeb1f 100644 --- a/src/miner.cpp +++ b/src/miner.cpp @@ -167,12 +167,13 @@ CBlockTemplate* CreateNewBlock(const CScript& scriptPubKeyIn) nTotalIn += mempool.mapTx[txin.prevout.hash].GetTx().vout[txin.prevout.n].nValue; continue; } - const CCoins &coins = view.GetCoins(txin.prevout.hash); + const CCoins* coins = view.AccessCoins(txin.prevout.hash); + assert(coins); - int64_t nValueIn = coins.vout[txin.prevout.n].nValue; + int64_t nValueIn = coins->vout[txin.prevout.n].nValue; nTotalIn += nValueIn; - int nConf = pindexPrev->nHeight - coins.nHeight + 1; + int nConf = pindexPrev->nHeight - coins->nHeight + 1; dPriority += (double)nValueIn * nConf; } diff --git a/src/rpcrawtransaction.cpp b/src/rpcrawtransaction.cpp index 7cd704193d..dc75caeabc 100644 --- a/src/rpcrawtransaction.cpp +++ b/src/rpcrawtransaction.cpp @@ -565,7 +565,7 @@ Value signrawtransaction(const Array& params, bool fHelp) BOOST_FOREACH(const CTxIn& txin, mergedTx.vin) { const uint256& prevHash = txin.prevout.hash; CCoins coins; - view.GetCoins(prevHash, coins); // this is certainly allowed to fail + view.AccessCoins(prevHash); // this is certainly allowed to fail } view.SetBackend(viewDummy); // switch back to avoid locking mempool for too long @@ -669,12 +669,12 @@ Value signrawtransaction(const Array& params, bool fHelp) // Sign what we can: for (unsigned int i = 0; i < mergedTx.vin.size(); i++) { CTxIn& txin = mergedTx.vin[i]; - CCoins coins; - if (!view.GetCoins(txin.prevout.hash, coins) || !coins.IsAvailable(txin.prevout.n)) { + const CCoins* coins = view.AccessCoins(txin.prevout.hash); + if (coins == NULL || !coins->IsAvailable(txin.prevout.n)) { fComplete = false; continue; } - const CScript& prevPubKey = coins.vout[txin.prevout.n].scriptPubKey; + const CScript& prevPubKey = coins->vout[txin.prevout.n].scriptPubKey; txin.scriptSig.clear(); // Only sign SIGHASH_SINGLE if there's a corresponding output: @@ -732,9 +732,9 @@ Value sendrawtransaction(const Array& params, bool fHelp) fOverrideFees = params[1].get_bool(); CCoinsViewCache &view = *pcoinsTip; - CCoins existingCoins; + const CCoins* existingCoins = view.AccessCoins(hashTx); bool fHaveMempool = mempool.exists(hashTx); - bool fHaveChain = view.GetCoins(hashTx, existingCoins) && existingCoins.nHeight < 1000000000; + bool fHaveChain = existingCoins && existingCoins->nHeight < 1000000000; if (!fHaveMempool && !fHaveChain) { // push to local node and sync with wallets CValidationState state; diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 238d5bab16..f059e69ac7 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -509,8 +509,8 @@ void CTxMemPool::check(const CCoinsViewCache *pcoins) const const CTransaction& tx2 = it2->second.GetTx(); assert(tx2.vout.size() > txin.prevout.n && !tx2.vout[txin.prevout.n].IsNull()); } else { - const CCoins &coins = pcoins->GetCoins(txin.prevout.hash); - assert(coins.IsAvailable(txin.prevout.n)); + const CCoins* coins = pcoins->AccessCoins(txin.prevout.hash); + assert(coins && coins->IsAvailable(txin.prevout.n)); } // Check whether its inputs are marked in mapNextTx. std::map::const_iterator it3 = mapNextTx.find(txin.prevout);