From ec1271f2bea52d43bd24fb93d32d24168f8c6a74 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 5 Jun 2017 11:42:24 -0400 Subject: Remove useless mapNextTx lookup in CTxMemPool::TrimToSize. Prior to per-utxo CCoins, we checked that no other in-mempool tx spent any of the given transaction's outputs, as we don't want to uncache that entire tx in such a case. However, we now are checking only that there exists no other mempool spends of the same output, which should clearly be impossible after we removed the transaction which was spending said output (barring massive mempool inconsistency). Thanks to @sdaftuar for the suggestion. --- src/txmempool.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 17389db9f0..1f2cd958ee 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -1050,9 +1050,7 @@ void CTxMemPool::TrimToSize(size_t sizelimit, std::vector* pvNoSpends BOOST_FOREACH(const CTransaction& tx, txn) { BOOST_FOREACH(const CTxIn& txin, tx.vin) { if (exists(txin.prevout.hash)) continue; - if (!mapNextTx.count(txin.prevout)) { - pvNoSpendsRemaining->push_back(txin.prevout); - } + pvNoSpendsRemaining->push_back(txin.prevout); } } } -- cgit v1.2.3 From 3533fb4d33ecc94a789cb5d4489da22a68fb2168 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 5 Jun 2017 11:50:47 -0400 Subject: Return a bool in SpendCoin to restore pre-per-utxo assert semantics Since its free to do so, assert that Spends succeeded when we expect them to. --- src/coins.cpp | 5 +++-- src/coins.h | 2 +- src/validation.cpp | 7 ++++--- 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/coins.cpp b/src/coins.cpp index 5b7c562678..b75bd27577 100644 --- a/src/coins.cpp +++ b/src/coins.cpp @@ -91,9 +91,9 @@ void AddCoins(CCoinsViewCache& cache, const CTransaction &tx, int nHeight) { } } -void CCoinsViewCache::SpendCoin(const COutPoint &outpoint, Coin* moveout) { +bool CCoinsViewCache::SpendCoin(const COutPoint &outpoint, Coin* moveout) { CCoinsMap::iterator it = FetchCoin(outpoint); - if (it == cacheCoins.end()) return; + if (it == cacheCoins.end()) return false; cachedCoinsUsage -= it->second.coin.DynamicMemoryUsage(); if (moveout) { *moveout = std::move(it->second.coin); @@ -104,6 +104,7 @@ void CCoinsViewCache::SpendCoin(const COutPoint &outpoint, Coin* moveout) { it->second.flags |= CCoinsCacheEntry::DIRTY; it->second.coin.Clear(); } + return true; } static const Coin coinEmpty; diff --git a/src/coins.h b/src/coins.h index 476db8f37c..4a0eaadc82 100644 --- a/src/coins.h +++ b/src/coins.h @@ -238,7 +238,7 @@ public: * If no unspent output exists for the passed outpoint, this call * has no effect. */ - void SpendCoin(const COutPoint &outpoint, Coin* moveto = nullptr); + bool SpendCoin(const COutPoint &outpoint, Coin* moveto = nullptr); /** * Push the modifications applied to this cache to its base. diff --git a/src/validation.cpp b/src/validation.cpp index de65839eef..c708a476de 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1118,7 +1118,8 @@ void UpdateCoins(const CTransaction& tx, CCoinsViewCache& inputs, CTxUndo &txund txundo.vprevout.reserve(tx.vin.size()); BOOST_FOREACH(const CTxIn &txin, tx.vin) { txundo.vprevout.emplace_back(); - inputs.SpendCoin(txin.prevout, &txundo.vprevout.back()); + bool is_spent = inputs.SpendCoin(txin.prevout, &txundo.vprevout.back()); + assert(is_spent); } } // add outputs @@ -1358,8 +1359,8 @@ static DisconnectResult DisconnectBlock(const CBlock& block, const CBlockIndex* if (!tx.vout[o].scriptPubKey.IsUnspendable()) { COutPoint out(hash, o); Coin coin; - view.SpendCoin(out, &coin); - if (tx.vout[o] != coin.out) { + bool is_spent = view.SpendCoin(out, &coin); + if (!is_spent || tx.vout[o] != coin.out) { fClean = false; // transaction output mismatch } } -- cgit v1.2.3 From f58349ca856fc59704fb78c19267b1cef1c9650b Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 5 Jun 2017 20:16:18 -0400 Subject: Restore some assert semantics in sigop cost calculations There are some similar asserts which are left removed in policy and ATMP (policy code being broken isn't a huge deal, but if we fail to verify some consensus rules, we should most definitely crash). --- src/consensus/tx_verify.cpp | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/consensus/tx_verify.cpp b/src/consensus/tx_verify.cpp index bf68f8754b..0671cbc132 100644 --- a/src/consensus/tx_verify.cpp +++ b/src/consensus/tx_verify.cpp @@ -126,7 +126,9 @@ unsigned int GetP2SHSigOpCount(const CTransaction& tx, const CCoinsViewCache& in unsigned int nSigOps = 0; for (unsigned int i = 0; i < tx.vin.size(); i++) { - const CTxOut &prevout = inputs.AccessCoin(tx.vin[i].prevout).out; + const Coin& coin = inputs.AccessCoin(tx.vin[i].prevout); + assert(!coin.IsSpent()); + const CTxOut &prevout = coin.out; if (prevout.scriptPubKey.IsPayToScriptHash()) nSigOps += prevout.scriptPubKey.GetSigOpCount(tx.vin[i].scriptSig); } @@ -146,7 +148,9 @@ int64_t GetTransactionSigOpCost(const CTransaction& tx, const CCoinsViewCache& i for (unsigned int i = 0; i < tx.vin.size(); i++) { - const CTxOut &prevout = inputs.AccessCoin(tx.vin[i].prevout).out; + const Coin& coin = inputs.AccessCoin(tx.vin[i].prevout); + assert(!coin.IsSpent()); + const CTxOut &prevout = coin.out; nSigOps += CountWitnessSigOps(tx.vin[i].scriptSig, prevout.scriptPubKey, &tx.vin[i].scriptWitness, flags); } return nSigOps; -- cgit v1.2.3 From 9417d7a336e71ee31ce673a6de5abb3d56941204 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 5 Jun 2017 20:28:08 -0400 Subject: Be much more agressive in AccessCoin docs. While the current implementation is pretty free, there is a lot of possibility for this to blow up in our face with future changes, especially as the backing map gets tweaked. --- src/coins.h | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/coins.h b/src/coins.h index 4a0eaadc82..89613a61b4 100644 --- a/src/coins.h +++ b/src/coins.h @@ -222,8 +222,13 @@ public: /** * Return a reference to Coin in the cache, or a pruned one if not found. This is - * more efficient than GetCoin. Modifications to other cache entries are - * allowed while accessing the returned pointer. + * more efficient than GetCoin. + * + * Generally, do not hold the reference returned for more than a short scope. + * While the current implementation allows for modifications to the contents + * of the cache while holding the reference, this behavior should not be relied + * on! To be safe, best to not hold the returned reference through any other + * calls to this cache. */ const Coin& AccessCoin(const COutPoint &output) const; -- cgit v1.2.3