From 56424040a469f775f14be0c81d7d62ebcfd8a0d7 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Mon, 22 Oct 2012 09:22:15 +0200 Subject: [PATCH 1/5] Bugfix: off-by-one error in coinbase maturity check --- src/main.cpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index ad5a7b0f1..723b8d75e 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -1319,7 +1319,9 @@ bool CTransaction::CheckInputs(CCoinsViewCache &inputs, enum CheckSig_mode csmod if (!HaveInputs(inputs)) return error("CheckInputs() : %s inputs unavailable", GetHash().ToString().substr(0,10).c_str()); - CBlockIndex *pindexBlock = inputs.GetBestBlock(); + // While checking, GetBestBlock() refers to the parent block. + // This is also true for mempool checks. + int nSpendHeight = inputs.GetBestBlock()->nHeight + 1; int64 nValueIn = 0; int64 nFees = 0; for (unsigned int i = 0; i < vin.size(); i++) @@ -1329,8 +1331,8 @@ bool CTransaction::CheckInputs(CCoinsViewCache &inputs, enum CheckSig_mode csmod // If prev is coinbase, check that it's matured if (coins.IsCoinBase()) { - if (pindexBlock->nHeight - coins.nHeight < COINBASE_MATURITY) - return error("CheckInputs() : tried to spend coinbase at depth %d", pindexBlock->nHeight - coins.nHeight); + if (nSpendHeight - coins.nHeight < COINBASE_MATURITY) + return error("CheckInputs() : tried to spend coinbase at depth %d", nSpendHeight - coins.nHeight); } // Check for negative or overflow input values From 9e957fb3b17e93a37db80544550a6689a3647295 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Mon, 22 Oct 2012 21:46:00 +0200 Subject: [PATCH 2/5] Bugfix: add missing fee check --- src/main.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/main.cpp b/src/main.cpp index 723b8d75e..dd2f491ae 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -1597,6 +1597,9 @@ bool CBlock::ConnectBlock(CBlockIndex* pindex, CCoinsViewCache &view, bool fJust blockundo.vtxundo.push_back(txundo); } + if (vtx[0].GetValueOut() > GetBlockValue(pindex->nHeight, nFees)) + return false; + if (fJustCheck) return true; From 1e64c2d585eeb6217d08a49c4cc7a89e0afd0421 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Tue, 23 Oct 2012 00:10:12 +0200 Subject: [PATCH 3/5] Bugfix: off-by-one in priority calculation --- src/main.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main.cpp b/src/main.cpp index dd2f491ae..4386893ee 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -3830,7 +3830,7 @@ CBlock* CreateNewBlock(CReserveKey& reservekey) int64 nValueIn = coins.vout[txin.prevout.n].nValue; nTotalIn += nValueIn; - int nConf = pindexPrev->nHeight - coins.nHeight; + int nConf = pindexPrev->nHeight - coins.nHeight + 1; dPriority += (double)nValueIn * nConf; } From c2ed184f987fbb157880fce2f91ebc8514fd6e76 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Tue, 23 Oct 2012 00:21:16 +0200 Subject: [PATCH 4/5] Added some comments Some clarifications after a code review by Mike Hearn. --- src/main.cpp | 9 ++++++++- src/main.h | 17 ++++++++++++++++- 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index 4386893ee..26cbab848 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -698,6 +698,8 @@ bool CTxMemPool::accept(CTransaction &tx, bool fCheckInputs, return false; // do all inputs exist? + // Note that this does not check for the presence of actual outputs (see the next check for that), + // only helps filling in pfMissingInputs (to determine missing vs spent). BOOST_FOREACH(const CTxIn txin, tx.vin) { if (!view.HaveCoins(txin.prevout.hash)) { if (pfMissingInputs) @@ -706,6 +708,7 @@ bool CTxMemPool::accept(CTransaction &tx, bool fCheckInputs, } } + // are the actual inputs available? if (!tx.HaveInputs(view)) return error("CTxMemPool::accept() : inputs already spent"); @@ -1956,9 +1959,13 @@ bool CBlock::CheckBlock(bool fCheckPOW, bool fCheckMerkleRoot) const if (!tx.CheckTransaction()) return DoS(tx.nDoS, error("CheckBlock() : CheckTransaction failed")); + // Build the merkle tree already. We need it anyway later, and it makes the + // block cache the transaction hashes, which means they don't need to be + // recalculated many times during this block's validation. + BuildMerkleTree(); + // Check for duplicate txids. This is caught by ConnectInputs(), // but catching it earlier avoids a potential DoS attack: - BuildMerkleTree(); set uniqueTx; for (unsigned int i=0; i &mapCoins, CBlockIndex *pindex); + + // Calculate statistics about the unspent transaction output set virtual bool GetStats(CCoinsStats &stats); }; @@ -1851,14 +1855,25 @@ protected: public: CCoinsViewCache(CCoinsView &baseIn, bool fDummy = false); + + // Standard CCoinsView methods bool GetCoins(uint256 txid, CCoins &coins); bool SetCoins(uint256 txid, const CCoins &coins); bool HaveCoins(uint256 txid); - CCoins &GetCoins(uint256 txid); CBlockIndex *GetBestBlock(); bool SetBestBlock(CBlockIndex *pindex); bool BatchWrite(const std::map &mapCoins, CBlockIndex *pindex); + + // 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(uint256 txid); + + // Push the modifications applied to this cache to its base. + // Failure to call this method before destruction will cause the changes to be forgotten. bool Flush(); + + // Calculate the size of the cache (in number of transactions) unsigned int GetCacheSize(); private: From 4afc0b54112bd990217a4a6fea145574e5947f09 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Tue, 23 Oct 2012 01:16:26 +0200 Subject: [PATCH 5/5] Bugfix: actually use CCoinsViewMemPool --- src/main.cpp | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index 26cbab848..be1e947ad 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -691,7 +691,13 @@ bool CTxMemPool::accept(CTransaction &tx, bool fCheckInputs, if (fCheckInputs) { - CCoinsViewCache &view = *pcoinsTip; + CCoinsView dummy; + CCoinsViewCache view(dummy); + + { + LOCK(cs); + CCoinsViewMemPool viewMemPool(*pcoinsTip, *this); + view.SetBackend(viewMemPool); // do we already have it? if (view.HaveCoins(hash)) @@ -711,6 +717,13 @@ bool CTxMemPool::accept(CTransaction &tx, bool fCheckInputs, // are the actual inputs available? if (!tx.HaveInputs(view)) return error("CTxMemPool::accept() : inputs already spent"); + + // Bring the best block into scope + view.GetBestBlock(); + + // we have all inputs cached now, so switch back to dummy, so we don't need to keep lock on mempool + view.SetBackend(dummy); + } // Check for non-standard pay-to-script-hash in inputs if (!tx.AreInputsStandard(view) && !fTestNet) @@ -741,7 +754,6 @@ bool CTxMemPool::accept(CTransaction &tx, bool fCheckInputs, int64 nNow = GetTime(); { - LOCK(cs); // Use an exponentially decaying ~10-minute window: dFreeCount *= pow(1.0 - 1.0/600.0, (double)(nNow - nLastTime)); nLastTime = nNow;