Skip to content

Commit 832e074

Browse files
jtimonjtimon
committed
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)
1 parent 3f0ee3e commit 832e074

File tree

4 files changed

+31
-30
lines changed

4 files changed

+31
-30
lines changed

‎src/consensus/tx_verify.cpp‎

Lines changed: 11 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -205,16 +205,15 @@ bool CheckTransaction(const CTransaction& tx, CValidationState &state, bool fChe
205205
return true;
206206
}
207207

208-
bool Consensus::CheckTxInputs(const CTransaction& tx, CValidationState& state, const CCoinsViewCache& inputs, int nSpendHeight)
208+
bool Consensus::CheckTxInputs(const CTransaction& tx, CValidationState& state, const CCoinsViewCache& inputs, int nSpendHeight, CAmount& txfee)
209209
{
210-
// This doesn't trigger the DoS code on purpose; if it did, it would make it easier
211-
// for an attacker to attempt to split the network.
210+
// are the actual inputs available?
212211
if (!inputs.HaveInputs(tx)) {
213-
return state.Invalid(false, 0, "", "Inputs unavailable");
212+
return state.DoS(100, false, REJECT_INVALID, "bad-txns-inputs-missingorspent", false,
213+
strprintf("%s: inputs missing/spent", __func__));
214214
}
215215

216216
CAmount nValueIn = 0;
217-
CAmount nFees = 0;
218217
for (unsigned int i = 0; i < tx.vin.size(); ++i) {
219218
const COutPoint &prevout = tx.vin[i].prevout;
220219
const Coin& coin = inputs.AccessCoin(prevout);
@@ -234,19 +233,18 @@ bool Consensus::CheckTxInputs(const CTransaction& tx, CValidationState& state, c
234233
}
235234
}
236235

237-
if (nValueIn < tx.GetValueOut()) {
236+
const CAmount value_out = tx.GetValueOut();
237+
if (nValueIn < value_out) {
238238
return state.DoS(100, false, REJECT_INVALID, "bad-txns-in-belowout", false,
239-
strprintf("value in (%s) < value out (%s)", FormatMoney(nValueIn), FormatMoney(tx.GetValueOut())));
239+
strprintf("value in (%s) < value out (%s)", FormatMoney(nValueIn), FormatMoney(value_out)));
240240
}
241241

242242
// Tally transaction fees
243-
CAmount nTxFee = nValueIn - tx.GetValueOut();
244-
if (nTxFee < 0) {
245-
return state.DoS(100, false, REJECT_INVALID, "bad-txns-fee-negative");
246-
}
247-
nFees += nTxFee;
248-
if (!MoneyRange(nFees)) {
243+
const CAmount txfee_aux = nValueIn - value_out;
244+
if (!MoneyRange(txfee_aux)) {
249245
return state.DoS(100, false, REJECT_INVALID, "bad-txns-fee-outofrange");
250246
}
247+
248+
txfee = txfee_aux;
251249
return true;
252250
}

‎src/consensus/tx_verify.h‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,8 @@
55
#ifndef BITCOIN_CONSENSUS_TX_VERIFY_H
66
#define BITCOIN_CONSENSUS_TX_VERIFY_H
77

8+
#include "amount.h"
9+
810
#include <stdint.h>
911
#include <vector>
1012

@@ -22,9 +24,10 @@ namespace Consensus {
2224
/**
2325
* Check whether all inputs of this transaction are valid (no double spends and amounts)
2426
* This does not modify the UTXO set. This does not check scripts and sigs.
27+
* @param[out] txfee Set to the transaction fee if successful.
2528
* Preconditions: tx.IsCoinBase() is false.
2629
*/
27-
bool CheckTxInputs(const CTransaction& tx, CValidationState& state, const CCoinsViewCache& inputs, int nSpendHeight);
30+
bool CheckTxInputs(const CTransaction& tx, CValidationState& state, const CCoinsViewCache& inputs, int nSpendHeight, CAmount& txfee);
2831
} // namespace Consensus
2932

3033
/** Auxiliary functions for transaction validation (ideally should not be exposed) */

‎src/txmempool.cpp‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -625,7 +625,7 @@ void CTxMemPool::check(const CCoinsViewCache *pcoins) const
625625
uint64_t innerUsage = 0;
626626

627627
CCoinsViewCache mempoolDuplicate(const_cast<CCoinsViewCache*>(pcoins));
628-
const int64_t nSpendHeight = GetSpendHeight(mempoolDuplicate);
628+
const int64_t spendheight = GetSpendHeight(mempoolDuplicate);
629629

630630
LOCK(cs);
631631
std::list<const CTxMemPoolEntry*> waitingOnDependants;
@@ -705,8 +705,9 @@ void CTxMemPool::check(const CCoinsViewCache *pcoins) const
705705
waitingOnDependants.push_back(&(*it));
706706
else {
707707
CValidationState state;
708+
CAmount txfee = 0;
708709
bool fCheckResult = tx.IsCoinBase() ||
709-
Consensus::CheckTxInputs(tx, state, mempoolDuplicate, nSpendHeight);
710+
Consensus::CheckTxInputs(tx, state, mempoolDuplicate, spendheight, txfee);
710711
assert(fCheckResult);
711712
UpdateCoins(tx, mempoolDuplicate, 1000000);
712713
}
@@ -721,8 +722,9 @@ void CTxMemPool::check(const CCoinsViewCache *pcoins) const
721722
stepsSinceLastRemove++;
722723
assert(stepsSinceLastRemove < waitingOnDependants.size());
723724
} else {
725+
CAmount txfee = 0;
724726
bool fCheckResult = entry->GetTx().IsCoinBase() ||
725-
Consensus::CheckTxInputs(entry->GetTx(), state, mempoolDuplicate, nSpendHeight);
727+
Consensus::CheckTxInputs(entry->GetTx(), state, mempoolDuplicate, spendheight, txfee);
726728
assert(fCheckResult);
727729
UpdateCoins(entry->GetTx(), mempoolDuplicate, 1000000);
728730
stepsSinceLastRemove = 0;

‎src/validation.cpp‎

Lines changed: 11 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -484,7 +484,6 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool
484484
CCoinsView dummy;
485485
CCoinsViewCache view(&dummy);
486486

487-
CAmount nValueIn = 0;
488487
LockPoints lp;
489488
{
490489
LOCK(pool.cs);
@@ -519,8 +518,6 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool
519518
// Bring the best block into scope
520519
view.GetBestBlock();
521520

522-
nValueIn = view.GetValueIn(tx);
523-
524521
// we have all inputs cached now, so switch back to dummy, so we don't need to keep lock on mempool
525522
view.SetBackend(dummy);
526523

@@ -531,6 +528,12 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool
531528
// CoinsViewCache instead of create its own
532529
if (!CheckSequenceLocks(tx, STANDARD_LOCKTIME_VERIFY_FLAGS, &lp))
533530
return state.DoS(0, false, REJECT_NONSTANDARD, "non-BIP68-final");
531+
532+
} // end LOCK(pool.cs)
533+
534+
CAmount nFees = 0;
535+
if (!Consensus::CheckTxInputs(tx, state, view, GetSpendHeight(view), nFees)) {
536+
return error("%s: Consensus::CheckTxInputs: %s, %s", __func__, tx.GetHash().ToString(), FormatStateMessage(state));
534537
}
535538

536539
// Check for non-standard pay-to-script-hash in inputs
@@ -543,8 +546,6 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool
543546

544547
int64_t nSigOpsCost = GetTransactionSigOpCost(tx, view, STANDARD_SCRIPT_VERIFY_FLAGS);
545548

546-
CAmount nValueOut = tx.GetValueOut();
547-
CAmount nFees = nValueIn-nValueOut;
548549
// nModifiedFees includes any fee deltas from PrioritiseTransaction
549550
CAmount nModifiedFees = nFees;
550551
pool.ApplyDelta(hash, nModifiedFees);
@@ -1161,9 +1162,6 @@ static bool CheckInputs(const CTransaction& tx, CValidationState &state, const C
11611162
{
11621163
if (!tx.IsCoinBase())
11631164
{
1164-
if (!Consensus::CheckTxInputs(tx, state, inputs, GetSpendHeight(inputs)))
1165-
return false;
1166-
11671165
if (pvChecks)
11681166
pvChecks->reserve(tx.vin.size());
11691167

@@ -1635,9 +1633,11 @@ static bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockInd
16351633

16361634
if (!tx.IsCoinBase())
16371635
{
1638-
if (!view.HaveInputs(tx))
1639-
return state.DoS(100, error("ConnectBlock(): inputs missing/spent"),
1640-
REJECT_INVALID, "bad-txns-inputs-missingorspent");
1636+
CAmount txfee = 0;
1637+
if (!Consensus::CheckTxInputs(tx, state, view, pindex->nHeight, txfee)) {
1638+
return error("%s: Consensus::CheckTxInputs: %s, %s", __func__, tx.GetHash().ToString(), FormatStateMessage(state));
1639+
}
1640+
nFees += txfee;
16411641

16421642
// Check that transaction is BIP68 final
16431643
// BIP68 lock checks (as opposed to nLockTime checks) must
@@ -1665,8 +1665,6 @@ static bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockInd
16651665
txdata.emplace_back(tx);
16661666
if (!tx.IsCoinBase())
16671667
{
1668-
nFees += view.GetValueIn(tx)-tx.GetValueOut();
1669-
16701668
std::vector<CScriptCheck> vChecks;
16711669
bool fCacheResults = fJustCheck; /* Don't cache results if we're actually connecting blocks (still consult the cache, though) */
16721670
if (!CheckInputs(tx, state, view, fScriptChecks, flags, fCacheResults, txdata[i], nScriptCheckThreads ? &vChecks : NULL))

0 commit comments

Comments
 (0)