From 3f0ee3e5011430461c7c61f4f2d406bf3cd8b54b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jorge=20Tim=C3=B3n?= Date: Sun, 25 Jun 2017 04:16:21 +0200 Subject: [PATCH 1/4] Proper indentation for CheckTxInputs and other minor fixes --- src/consensus/tx_verify.cpp | 76 +++++++++++++++++++------------------ 1 file changed, 39 insertions(+), 37 deletions(-) diff --git a/src/consensus/tx_verify.cpp b/src/consensus/tx_verify.cpp index 0671cbc13..25647ba6a 100644 --- a/src/consensus/tx_verify.cpp +++ b/src/consensus/tx_verify.cpp @@ -13,7 +13,7 @@ #include "chain.h" #include "coins.h" #include "utilmoneystr.h" - + bool IsFinalTx(const CTransaction &tx, int nBlockHeight, int64_t nBlockTime) { if (tx.nLockTime == 0) @@ -207,44 +207,46 @@ bool CheckTransaction(const CTransaction& tx, CValidationState &state, bool fChe bool Consensus::CheckTxInputs(const CTransaction& tx, CValidationState& state, const CCoinsViewCache& inputs, int nSpendHeight) { - // This doesn't trigger the DoS code on purpose; if it did, it would make it easier - // for an attacker to attempt to split the network. - if (!inputs.HaveInputs(tx)) - return state.Invalid(false, 0, "", "Inputs unavailable"); - - CAmount nValueIn = 0; - CAmount nFees = 0; - for (unsigned int i = 0; i < tx.vin.size(); i++) - { - const COutPoint &prevout = tx.vin[i].prevout; - const Coin& coin = inputs.AccessCoin(prevout); - assert(!coin.IsSpent()); - - // If prev is coinbase, check that it's matured - if (coin.IsCoinBase()) { - if (nSpendHeight - coin.nHeight < COINBASE_MATURITY) - return state.Invalid(false, - REJECT_INVALID, "bad-txns-premature-spend-of-coinbase", - strprintf("tried to spend coinbase at depth %d", nSpendHeight - coin.nHeight)); - } - - // Check for negative or overflow input values - nValueIn += coin.out.nValue; - if (!MoneyRange(coin.out.nValue) || !MoneyRange(nValueIn)) - return state.DoS(100, false, REJECT_INVALID, "bad-txns-inputvalues-outofrange"); + // This doesn't trigger the DoS code on purpose; if it did, it would make it easier + // for an attacker to attempt to split the network. + if (!inputs.HaveInputs(tx)) { + return state.Invalid(false, 0, "", "Inputs unavailable"); + } + + CAmount nValueIn = 0; + CAmount nFees = 0; + for (unsigned int i = 0; i < tx.vin.size(); ++i) { + const COutPoint &prevout = tx.vin[i].prevout; + const Coin& coin = inputs.AccessCoin(prevout); + assert(!coin.IsSpent()); + + // If prev is coinbase, check that it's matured + if (coin.IsCoinBase() && nSpendHeight - coin.nHeight < COINBASE_MATURITY) { + return state.Invalid(false, + REJECT_INVALID, "bad-txns-premature-spend-of-coinbase", + strprintf("tried to spend coinbase at depth %d", nSpendHeight - coin.nHeight)); + } + // Check for negative or overflow input values + nValueIn += coin.out.nValue; + if (!MoneyRange(coin.out.nValue) || !MoneyRange(nValueIn)) { + return state.DoS(100, false, REJECT_INVALID, "bad-txns-inputvalues-outofrange"); } + } + + if (nValueIn < tx.GetValueOut()) { + return state.DoS(100, false, REJECT_INVALID, "bad-txns-in-belowout", false, + strprintf("value in (%s) < value out (%s)", FormatMoney(nValueIn), FormatMoney(tx.GetValueOut()))); + } - if (nValueIn < tx.GetValueOut()) - return state.DoS(100, false, REJECT_INVALID, "bad-txns-in-belowout", false, - strprintf("value in (%s) < value out (%s)", FormatMoney(nValueIn), FormatMoney(tx.GetValueOut()))); - - // Tally transaction fees - CAmount nTxFee = nValueIn - tx.GetValueOut(); - if (nTxFee < 0) - return state.DoS(100, false, REJECT_INVALID, "bad-txns-fee-negative"); - nFees += nTxFee; - if (!MoneyRange(nFees)) - return state.DoS(100, false, REJECT_INVALID, "bad-txns-fee-outofrange"); + // Tally transaction fees + CAmount nTxFee = nValueIn - tx.GetValueOut(); + if (nTxFee < 0) { + return state.DoS(100, false, REJECT_INVALID, "bad-txns-fee-negative"); + } + nFees += nTxFee; + if (!MoneyRange(nFees)) { + return state.DoS(100, false, REJECT_INVALID, "bad-txns-fee-outofrange"); + } return true; } From 832e0744cb8b1e1625cdb19b257f97316ac16a90 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jorge=20Tim=C3=B3n?= Date: Thu, 11 Aug 2016 04:42:36 +0200 Subject: [PATCH 2/4] Optimization: Minimize the number of times it is checked that no money is created by individual transactions to 2 places (but call only once in each): - ConnectBlock ( before calculated fees per txs twice ) - AcceptToMemoryPoolWorker ( before called CheckTxInputs 4 times and calculated fees per tx one extra time ) Also call tx.GetValueOut() only once per call of CheckTxInputs (instead of 2) --- src/consensus/tx_verify.cpp | 24 +++++++++++------------- src/consensus/tx_verify.h | 5 ++++- src/txmempool.cpp | 8 +++++--- src/validation.cpp | 24 +++++++++++------------- 4 files changed, 31 insertions(+), 30 deletions(-) diff --git a/src/consensus/tx_verify.cpp b/src/consensus/tx_verify.cpp index 25647ba6a..f2f85edad 100644 --- a/src/consensus/tx_verify.cpp +++ b/src/consensus/tx_verify.cpp @@ -205,16 +205,15 @@ bool CheckTransaction(const CTransaction& tx, CValidationState &state, bool fChe return true; } -bool Consensus::CheckTxInputs(const CTransaction& tx, CValidationState& state, const CCoinsViewCache& inputs, int nSpendHeight) +bool Consensus::CheckTxInputs(const CTransaction& tx, CValidationState& state, const CCoinsViewCache& inputs, int nSpendHeight, CAmount& txfee) { - // This doesn't trigger the DoS code on purpose; if it did, it would make it easier - // for an attacker to attempt to split the network. + // are the actual inputs available? if (!inputs.HaveInputs(tx)) { - return state.Invalid(false, 0, "", "Inputs unavailable"); + return state.DoS(100, false, REJECT_INVALID, "bad-txns-inputs-missingorspent", false, + strprintf("%s: inputs missing/spent", __func__)); } CAmount nValueIn = 0; - CAmount nFees = 0; for (unsigned int i = 0; i < tx.vin.size(); ++i) { const COutPoint &prevout = tx.vin[i].prevout; const Coin& coin = inputs.AccessCoin(prevout); @@ -234,19 +233,18 @@ bool Consensus::CheckTxInputs(const CTransaction& tx, CValidationState& state, c } } - if (nValueIn < tx.GetValueOut()) { + const CAmount value_out = tx.GetValueOut(); + if (nValueIn < value_out) { return state.DoS(100, false, REJECT_INVALID, "bad-txns-in-belowout", false, - strprintf("value in (%s) < value out (%s)", FormatMoney(nValueIn), FormatMoney(tx.GetValueOut()))); + strprintf("value in (%s) < value out (%s)", FormatMoney(nValueIn), FormatMoney(value_out))); } // Tally transaction fees - CAmount nTxFee = nValueIn - tx.GetValueOut(); - if (nTxFee < 0) { - return state.DoS(100, false, REJECT_INVALID, "bad-txns-fee-negative"); - } - nFees += nTxFee; - if (!MoneyRange(nFees)) { + const CAmount txfee_aux = nValueIn - value_out; + if (!MoneyRange(txfee_aux)) { return state.DoS(100, false, REJECT_INVALID, "bad-txns-fee-outofrange"); } + + txfee = txfee_aux; return true; } diff --git a/src/consensus/tx_verify.h b/src/consensus/tx_verify.h index d46d3294c..288892462 100644 --- a/src/consensus/tx_verify.h +++ b/src/consensus/tx_verify.h @@ -5,6 +5,8 @@ #ifndef BITCOIN_CONSENSUS_TX_VERIFY_H #define BITCOIN_CONSENSUS_TX_VERIFY_H +#include "amount.h" + #include #include @@ -22,9 +24,10 @@ namespace Consensus { /** * Check whether all inputs of this transaction are valid (no double spends and amounts) * This does not modify the UTXO set. This does not check scripts and sigs. + * @param[out] txfee Set to the transaction fee if successful. * Preconditions: tx.IsCoinBase() is false. */ -bool CheckTxInputs(const CTransaction& tx, CValidationState& state, const CCoinsViewCache& inputs, int nSpendHeight); +bool CheckTxInputs(const CTransaction& tx, CValidationState& state, const CCoinsViewCache& inputs, int nSpendHeight, CAmount& txfee); } // namespace Consensus /** Auxiliary functions for transaction validation (ideally should not be exposed) */ diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 8deb703d2..ef955a17e 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -625,7 +625,7 @@ void CTxMemPool::check(const CCoinsViewCache *pcoins) const uint64_t innerUsage = 0; CCoinsViewCache mempoolDuplicate(const_cast(pcoins)); - const int64_t nSpendHeight = GetSpendHeight(mempoolDuplicate); + const int64_t spendheight = GetSpendHeight(mempoolDuplicate); LOCK(cs); std::list waitingOnDependants; @@ -705,8 +705,9 @@ void CTxMemPool::check(const CCoinsViewCache *pcoins) const waitingOnDependants.push_back(&(*it)); else { CValidationState state; + CAmount txfee = 0; bool fCheckResult = tx.IsCoinBase() || - Consensus::CheckTxInputs(tx, state, mempoolDuplicate, nSpendHeight); + Consensus::CheckTxInputs(tx, state, mempoolDuplicate, spendheight, txfee); assert(fCheckResult); UpdateCoins(tx, mempoolDuplicate, 1000000); } @@ -721,8 +722,9 @@ void CTxMemPool::check(const CCoinsViewCache *pcoins) const stepsSinceLastRemove++; assert(stepsSinceLastRemove < waitingOnDependants.size()); } else { + CAmount txfee = 0; bool fCheckResult = entry->GetTx().IsCoinBase() || - Consensus::CheckTxInputs(entry->GetTx(), state, mempoolDuplicate, nSpendHeight); + Consensus::CheckTxInputs(entry->GetTx(), state, mempoolDuplicate, spendheight, txfee); assert(fCheckResult); UpdateCoins(entry->GetTx(), mempoolDuplicate, 1000000); stepsSinceLastRemove = 0; diff --git a/src/validation.cpp b/src/validation.cpp index eb6ea42b6..8899bd2cd 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -484,7 +484,6 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool CCoinsView dummy; CCoinsViewCache view(&dummy); - CAmount nValueIn = 0; LockPoints lp; { LOCK(pool.cs); @@ -519,8 +518,6 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool // Bring the best block into scope view.GetBestBlock(); - nValueIn = view.GetValueIn(tx); - // we have all inputs cached now, so switch back to dummy, so we don't need to keep lock on mempool view.SetBackend(dummy); @@ -531,6 +528,12 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool // CoinsViewCache instead of create its own if (!CheckSequenceLocks(tx, STANDARD_LOCKTIME_VERIFY_FLAGS, &lp)) return state.DoS(0, false, REJECT_NONSTANDARD, "non-BIP68-final"); + + } // end LOCK(pool.cs) + + CAmount nFees = 0; + if (!Consensus::CheckTxInputs(tx, state, view, GetSpendHeight(view), nFees)) { + return error("%s: Consensus::CheckTxInputs: %s, %s", __func__, tx.GetHash().ToString(), FormatStateMessage(state)); } // Check for non-standard pay-to-script-hash in inputs @@ -543,8 +546,6 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool int64_t nSigOpsCost = GetTransactionSigOpCost(tx, view, STANDARD_SCRIPT_VERIFY_FLAGS); - CAmount nValueOut = tx.GetValueOut(); - CAmount nFees = nValueIn-nValueOut; // nModifiedFees includes any fee deltas from PrioritiseTransaction CAmount nModifiedFees = nFees; pool.ApplyDelta(hash, nModifiedFees); @@ -1161,9 +1162,6 @@ static bool CheckInputs(const CTransaction& tx, CValidationState &state, const C { if (!tx.IsCoinBase()) { - if (!Consensus::CheckTxInputs(tx, state, inputs, GetSpendHeight(inputs))) - return false; - if (pvChecks) pvChecks->reserve(tx.vin.size()); @@ -1635,9 +1633,11 @@ static bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockInd if (!tx.IsCoinBase()) { - if (!view.HaveInputs(tx)) - return state.DoS(100, error("ConnectBlock(): inputs missing/spent"), - REJECT_INVALID, "bad-txns-inputs-missingorspent"); + CAmount txfee = 0; + if (!Consensus::CheckTxInputs(tx, state, view, pindex->nHeight, txfee)) { + return error("%s: Consensus::CheckTxInputs: %s, %s", __func__, tx.GetHash().ToString(), FormatStateMessage(state)); + } + nFees += txfee; // Check that transaction is BIP68 final // BIP68 lock checks (as opposed to nLockTime checks) must @@ -1665,8 +1665,6 @@ static bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockInd txdata.emplace_back(tx); if (!tx.IsCoinBase()) { - nFees += view.GetValueIn(tx)-tx.GetValueOut(); - std::vector vChecks; bool fCacheResults = fJustCheck; /* Don't cache results if we're actually connecting blocks (still consult the cache, though) */ if (!CheckInputs(tx, state, view, fScriptChecks, flags, fCacheResults, txdata[i], nScriptCheckThreads ? &vChecks : NULL)) From 3e8c91629e646f1ace700fe394f5bbef88b9cd31 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jorge=20Tim=C3=B3n?= Date: Tue, 18 Apr 2017 22:49:47 +0200 Subject: [PATCH 3/4] Introduce CheckInputsAndUpdateCoins static wrapper in txmempool.cpp --- src/txmempool.cpp | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/txmempool.cpp b/src/txmempool.cpp index ef955a17e..508658e5d 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -611,6 +611,15 @@ void CTxMemPool::clear() _clear(); } +static void CheckInputsAndUpdateCoins(const CTransaction& tx, CCoinsViewCache& mempoolDuplicate, const int64_t spendheight) +{ + CValidationState state; + CAmount txfee = 0; + bool fCheckResult = tx.IsCoinBase() || Consensus::CheckTxInputs(tx, state, mempoolDuplicate, spendheight, txfee); + assert(fCheckResult); + UpdateCoins(tx, mempoolDuplicate, 1000000); +} + void CTxMemPool::check(const CCoinsViewCache *pcoins) const { if (nCheckFrequency == 0) @@ -704,12 +713,7 @@ void CTxMemPool::check(const CCoinsViewCache *pcoins) const if (fDependsWait) waitingOnDependants.push_back(&(*it)); else { - CValidationState state; - CAmount txfee = 0; - bool fCheckResult = tx.IsCoinBase() || - Consensus::CheckTxInputs(tx, state, mempoolDuplicate, spendheight, txfee); - assert(fCheckResult); - UpdateCoins(tx, mempoolDuplicate, 1000000); + CheckInputsAndUpdateCoins(tx, mempoolDuplicate, spendheight); } } unsigned int stepsSinceLastRemove = 0; @@ -722,11 +726,7 @@ void CTxMemPool::check(const CCoinsViewCache *pcoins) const stepsSinceLastRemove++; assert(stepsSinceLastRemove < waitingOnDependants.size()); } else { - CAmount txfee = 0; - bool fCheckResult = entry->GetTx().IsCoinBase() || - Consensus::CheckTxInputs(entry->GetTx(), state, mempoolDuplicate, spendheight, txfee); - assert(fCheckResult); - UpdateCoins(entry->GetTx(), mempoolDuplicate, 1000000); + CheckInputsAndUpdateCoins(entry->GetTx(), mempoolDuplicate, spendheight); stepsSinceLastRemove = 0; } } From 4e955c58e13cfe089208f6b23b195d395ad99baa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jorge=20Tim=C3=B3n?= Date: Tue, 4 Jul 2017 17:45:46 +0200 Subject: [PATCH 4/4] Near-Bugfix: Reestablish consensus check removed in 8d7849b in 8d7849b6db5f54dc32fe4f8c6c7283068473cd21 This can potentially prevent an overflow that could at least in theory allow the creation of money. --- src/validation.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/validation.cpp b/src/validation.cpp index 8899bd2cd..2e2a89bcf 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1638,6 +1638,10 @@ static bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockInd return error("%s: Consensus::CheckTxInputs: %s, %s", __func__, tx.GetHash().ToString(), FormatStateMessage(state)); } nFees += txfee; + if (!MoneyRange(nFees)) { + return state.DoS(100, error("%s: accumulated fee in the block out of range.", __func__), + REJECT_INVALID, "bad-txns-accumulated-fee-outofrange"); + } // Check that transaction is BIP68 final // BIP68 lock checks (as opposed to nLockTime checks) must