Browse Source

Merge #8498: Near-Bugfix: Optimization: Minimize the number of times it is checked that no money...

4e955c5 Near-Bugfix: Reestablish consensus check removed in 8d7849b (Jorge Timón)
3e8c916 Introduce CheckInputsAndUpdateCoins static wrapper in txmempool.cpp (Jorge Timón)
832e074 Optimization: Minimize the number of times it is checked that no money is created (Jorge Timón)
3f0ee3e Proper indentation for CheckTxInputs and other minor fixes (Jorge Timón)

Pull request description:

  ...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)

  For more motivation:

  ~~https://github.com/bitcoin/bitcoin/blob/master/src/main.cpp#L1493~~
  https://github.com/jtimon/bitcoin/compare/0.13-consensus-inputs...jtimon:0.13-consensus-inputs-comments

  EDIT: partially replaces #6445

  Near-Bugfix as pointed out in https://github.com/bitcoin/bitcoin/pull/8498#discussion_r124346132

Tree-SHA512: c71188e7c7c2425c9170ed7b803896755a92fd22f43b136eedaa6e554106696f0b10271d0ef0d0127c1eaafbc31d12eb19143df4f1b6882feecedf6ef05ea346
0.16
Wladimir J. van der Laan 7 years ago
parent
commit
0e3a411351
No known key found for this signature in database
GPG Key ID: 1E4AED62986CD25D
  1. 74
      src/consensus/tx_verify.cpp
  2. 5
      src/consensus/tx_verify.h
  3. 22
      src/txmempool.cpp
  4. 28
      src/validation.cpp

74
src/consensus/tx_verify.cpp

@ -205,46 +205,46 @@ bool CheckTransaction(const CTransaction& tx, CValidationState &state, bool fChe
return true; 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 // are the actual inputs available?
// for an attacker to attempt to split the network. if (!inputs.HaveInputs(tx)) {
if (!inputs.HaveInputs(tx)) return state.DoS(100, false, REJECT_INVALID, "bad-txns-inputs-missingorspent", false,
return state.Invalid(false, 0, "", "Inputs unavailable"); strprintf("%s: inputs missing/spent", __func__));
}
CAmount nValueIn = 0;
CAmount nFees = 0; CAmount nValueIn = 0;
for (unsigned int i = 0; i < tx.vin.size(); i++) for (unsigned int i = 0; i < tx.vin.size(); ++i) {
{ const COutPoint &prevout = tx.vin[i].prevout;
const COutPoint &prevout = tx.vin[i].prevout; const Coin& coin = inputs.AccessCoin(prevout);
const Coin& coin = inputs.AccessCoin(prevout); assert(!coin.IsSpent());
assert(!coin.IsSpent());
// If prev is coinbase, check that it's matured
// If prev is coinbase, check that it's matured if (coin.IsCoinBase() && nSpendHeight - coin.nHeight < COINBASE_MATURITY) {
if (coin.IsCoinBase()) { return state.Invalid(false,
if (nSpendHeight - coin.nHeight < COINBASE_MATURITY) REJECT_INVALID, "bad-txns-premature-spend-of-coinbase",
return state.Invalid(false, strprintf("tried to spend coinbase at depth %d", nSpendHeight - coin.nHeight));
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");
// 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");
} }
}
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(value_out)));
}
// Tally transaction fees
const CAmount txfee_aux = nValueIn - value_out;
if (!MoneyRange(txfee_aux)) {
return state.DoS(100, false, REJECT_INVALID, "bad-txns-fee-outofrange");
}
if (nValueIn < tx.GetValueOut()) txfee = txfee_aux;
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");
return true; return true;
} }

5
src/consensus/tx_verify.h

@ -5,6 +5,8 @@
#ifndef BITCOIN_CONSENSUS_TX_VERIFY_H #ifndef BITCOIN_CONSENSUS_TX_VERIFY_H
#define BITCOIN_CONSENSUS_TX_VERIFY_H #define BITCOIN_CONSENSUS_TX_VERIFY_H
#include "amount.h"
#include <stdint.h> #include <stdint.h>
#include <vector> #include <vector>
@ -22,9 +24,10 @@ namespace Consensus {
/** /**
* Check whether all inputs of this transaction are valid (no double spends and amounts) * 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. * 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. * 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 } // namespace Consensus
/** Auxiliary functions for transaction validation (ideally should not be exposed) */ /** Auxiliary functions for transaction validation (ideally should not be exposed) */

22
src/txmempool.cpp

@ -607,6 +607,15 @@ void CTxMemPool::clear()
_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 void CTxMemPool::check(const CCoinsViewCache *pcoins) const
{ {
if (nCheckFrequency == 0) if (nCheckFrequency == 0)
@ -621,7 +630,7 @@ void CTxMemPool::check(const CCoinsViewCache *pcoins) const
uint64_t innerUsage = 0; uint64_t innerUsage = 0;
CCoinsViewCache mempoolDuplicate(const_cast<CCoinsViewCache*>(pcoins)); CCoinsViewCache mempoolDuplicate(const_cast<CCoinsViewCache*>(pcoins));
const int64_t nSpendHeight = GetSpendHeight(mempoolDuplicate); const int64_t spendheight = GetSpendHeight(mempoolDuplicate);
LOCK(cs); LOCK(cs);
std::list<const CTxMemPoolEntry*> waitingOnDependants; std::list<const CTxMemPoolEntry*> waitingOnDependants;
@ -700,11 +709,7 @@ void CTxMemPool::check(const CCoinsViewCache *pcoins) const
if (fDependsWait) if (fDependsWait)
waitingOnDependants.push_back(&(*it)); waitingOnDependants.push_back(&(*it));
else { else {
CValidationState state; CheckInputsAndUpdateCoins(tx, mempoolDuplicate, spendheight);
bool fCheckResult = tx.IsCoinBase() ||
Consensus::CheckTxInputs(tx, state, mempoolDuplicate, nSpendHeight);
assert(fCheckResult);
UpdateCoins(tx, mempoolDuplicate, 1000000);
} }
} }
unsigned int stepsSinceLastRemove = 0; unsigned int stepsSinceLastRemove = 0;
@ -717,10 +722,7 @@ void CTxMemPool::check(const CCoinsViewCache *pcoins) const
stepsSinceLastRemove++; stepsSinceLastRemove++;
assert(stepsSinceLastRemove < waitingOnDependants.size()); assert(stepsSinceLastRemove < waitingOnDependants.size());
} else { } else {
bool fCheckResult = entry->GetTx().IsCoinBase() || CheckInputsAndUpdateCoins(entry->GetTx(), mempoolDuplicate, spendheight);
Consensus::CheckTxInputs(entry->GetTx(), state, mempoolDuplicate, nSpendHeight);
assert(fCheckResult);
UpdateCoins(entry->GetTx(), mempoolDuplicate, 1000000);
stepsSinceLastRemove = 0; stepsSinceLastRemove = 0;
} }
} }

28
src/validation.cpp

@ -534,7 +534,6 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool
CCoinsView dummy; CCoinsView dummy;
CCoinsViewCache view(&dummy); CCoinsViewCache view(&dummy);
CAmount nValueIn = 0;
LockPoints lp; LockPoints lp;
{ {
LOCK(pool.cs); LOCK(pool.cs);
@ -565,8 +564,6 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool
// Bring the best block into scope // Bring the best block into scope
view.GetBestBlock(); 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 // we have all inputs cached now, so switch back to dummy, so we don't need to keep lock on mempool
view.SetBackend(dummy); view.SetBackend(dummy);
@ -577,6 +574,12 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool
// CoinsViewCache instead of create its own // CoinsViewCache instead of create its own
if (!CheckSequenceLocks(tx, STANDARD_LOCKTIME_VERIFY_FLAGS, &lp)) if (!CheckSequenceLocks(tx, STANDARD_LOCKTIME_VERIFY_FLAGS, &lp))
return state.DoS(0, false, REJECT_NONSTANDARD, "non-BIP68-final"); 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 // Check for non-standard pay-to-script-hash in inputs
@ -589,8 +592,6 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool
int64_t nSigOpsCost = GetTransactionSigOpCost(tx, view, STANDARD_SCRIPT_VERIFY_FLAGS); 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 // nModifiedFees includes any fee deltas from PrioritiseTransaction
CAmount nModifiedFees = nFees; CAmount nModifiedFees = nFees;
pool.ApplyDelta(hash, nModifiedFees); pool.ApplyDelta(hash, nModifiedFees);
@ -1247,9 +1248,6 @@ bool CheckInputs(const CTransaction& tx, CValidationState &state, const CCoinsVi
{ {
if (!tx.IsCoinBase()) if (!tx.IsCoinBase())
{ {
if (!Consensus::CheckTxInputs(tx, state, inputs, GetSpendHeight(inputs)))
return false;
if (pvChecks) if (pvChecks)
pvChecks->reserve(tx.vin.size()); pvChecks->reserve(tx.vin.size());
@ -1762,9 +1760,15 @@ static bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockInd
if (!tx.IsCoinBase()) if (!tx.IsCoinBase())
{ {
if (!view.HaveInputs(tx)) CAmount txfee = 0;
return state.DoS(100, error("ConnectBlock(): inputs missing/spent"), if (!Consensus::CheckTxInputs(tx, state, view, pindex->nHeight, txfee)) {
REJECT_INVALID, "bad-txns-inputs-missingorspent"); 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 // Check that transaction is BIP68 final
// BIP68 lock checks (as opposed to nLockTime checks) must // BIP68 lock checks (as opposed to nLockTime checks) must
@ -1792,8 +1796,6 @@ static bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockInd
txdata.emplace_back(tx); txdata.emplace_back(tx);
if (!tx.IsCoinBase()) if (!tx.IsCoinBase())
{ {
nFees += view.GetValueIn(tx)-tx.GetValueOut();
std::vector<CScriptCheck> vChecks; std::vector<CScriptCheck> vChecks;
bool fCacheResults = fJustCheck; /* Don't cache results if we're actually connecting blocks (still consult the cache, though) */ 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, fCacheResults, txdata[i], nScriptCheckThreads ? &vChecks : nullptr)) if (!CheckInputs(tx, state, view, fScriptChecks, flags, fCacheResults, fCacheResults, txdata[i], nScriptCheckThreads ? &vChecks : nullptr))

Loading…
Cancel
Save