Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
laanwj merged 4 commits into from
Oct 11, 2017
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3f0ee3e
Proper indentation for CheckTxInputs and other minor fixes
jtimon Jun 25, 2017
832e074
Optimization: Minimize the number of times it is checked that no mone…
jtimon Aug 11, 2016
3e8c916
Introduce CheckInputsAndUpdateCoins static wrapper in txmempool.cpp
jtimon Apr 18, 2017
4e955c5
Near-Bugfix: Reestablish consensus check removed in 8d7849b
jtimon Jul 4, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 38 additions & 38 deletions src/consensus/tx_verify.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -205,46 +205,46 @@ 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.
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");
// are the actual inputs available?
if (!inputs.HaveInputs(tx)) {
return state.DoS(100, false, REJECT_INVALID, "bad-txns-inputs-missingorspent", false,
Copy link
Contributor

@danra danra Sep 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current code has an explicit comment about not triggering the DoS code on purpose, did you remove it (and invoke the DoS code) intentionally?

Copy link
Contributor Author

@jtimon jtimon Sep 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment was moved to the separated HaveInputs call in AcceptToMemoryPoolWorker.
Perhaps lost in rebase, I'll look at it.
Perhaps it's fine since we now have: c93ceb2#diff-24efdb00bfbe56b140fb006b562cc70bR514

Copy link
Contributor

@danra danra Sep 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not just the comment, as you can see in the diff previously the code used state.Invalid (and stated it does so on purpose), and now both the comment is gone and the code uses state.DoS

I don't know to say on my own whether that change is valid, but it definitely requires explanation due to the fact that previously there was a specific comment suggesting using state.DoS there is bad.

Copy link
Contributor

@TheBlueMatt TheBlueMatt Sep 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be OK, the DoS from ConnectBlock is important, but the DoS shouldn't be trigger'able in ATMP - we should have already returned false int he pfMissingInputs branch further up.

Copy link
Contributor

@danra danra Sep 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TheBlueMatt

It should be OK, the DoS from ConnectBlock is important

Which DoS? The one previously returned from CheckTxInputs in case of missing inputs? If so, IIUC it wouldn't trigger after this change, there would just be an error returned, since it replaced the previous DoS in ConnectBlock c93ceb2#diff-24efdb00bfbe56b140fb006b562cc70bR1638

Copy link
Contributor

@TheBlueMatt TheBlueMatt Sep 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DoS values persist through the state object (and should only be set once through the entire call tree). Thus, when ConnectBlock calls CheckTxInputs, which sets DoS, ConnectBlock should return only error() and not call DoS again, but the DoS points will still be returned through the state object.

Copy link
Contributor

@danra danra Sep 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TheBlueMatt Ah, I see. Thanks!

strprintf("%s: inputs missing/spent", __func__));
}

CAmount nValueIn = 0;
for (unsigned int i = 0; i < tx.vin.size(); ++i) {
Copy link
Contributor

@danra danra Sep 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest using const auto& to iterate over tx.vin. (This is already the case in other functions in this file)

Copy link
Contributor Author

@jtimon jtimon Sep 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line wasn't touched besides indentation until @promag asked s/i++/++i/
Not sure it's worth it to make that change here, to be honest...

Copy link
Contributor

@danra danra Sep 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jtimon IMHO it would fit nicely in the minor changes commit

const COutPoint &prevout = tx.vin[i].prevout;
const Coin& coin = inputs.AccessCoin(prevout);
Copy link
Contributor

@TheBlueMatt TheBlueMatt Sep 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please dont add needless auto& unless you have to. If you're saving 5 chars of typing it just makes things less readable.

Copy link
Contributor

@danra danra Sep 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TheBlueMatt I disagree, and AFAIK so do most of the C++ gurus. The (character) typing isn't the issue here, rather code flexibility. See https://softwareengineering.stackexchange.com/questions/180216/does-auto-make-c-code-harder-to-understand for an example of a discussion on the topic.

Copy link
Contributor

@TheBlueMatt TheBlueMatt Sep 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

auto, here, AFAIU, is going to be slightly more likely to force you to change the line if you change AccessCoin's return type (at least because its a const-ref type, so conversion should only be an inheritance thing). In consensus code, making sure you're forced to change all the places something is used if you change a type strikes me as a very, very good idea. If you want to take Herb's stance on it, I'd also be ok with his recommended auto coin = const Coin&{inputs.AccessCoin(prevout)}; to force it, though I find that somewhat unreadable (which is bias towards what I'm used to, I admit).

Copy link
Contributor

@danra danra Sep 15, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consensus code seems hard enough to change as it is without adding extra hurdles.

In consensus code, making sure you're forced to change all the places something is used if
you change a type strikes me as a very, very good idea.

By that logic, instead of
const COutPoint &prevout = tx.vin[i].prevout;

it would be better to write

const CTxIn& txin = tx.vin[i];
const COutPoint &prevout = txin.prevout;

Since in the first version, tx.vin[i] is used without specifying its type :)

Copy link
Contributor Author

@jtimon jtimon Sep 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's please leave this for another PR

Copy link
Contributor

@danra danra Sep 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, deleting this comment.

Copy link
Contributor

@TheBlueMatt TheBlueMatt Sep 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please dont delete comments. Just because a PR author decided against taking a suggestion doesnt mean review content should be deleted. Others may wish to agree with the suggestion or otherwise just view the history of what was discussed. Unless a comment is updated immediately afterwards, generally I'd prefer they not be edited.

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");
}
}

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())
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");
txfee = txfee_aux;
return true;
}
5 changes: 4 additions & 1 deletion src/consensus/tx_verify.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
#ifndef BITCOIN_CONSENSUS_TX_VERIFY_H
#define BITCOIN_CONSENSUS_TX_VERIFY_H

#include "amount.h"

#include <stdint.h>
#include <vector>

Expand All @@ -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) */
Expand Down
22 changes: 12 additions & 10 deletions src/txmempool.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -625,7 +634,7 @@ void CTxMemPool::check(const CCoinsViewCache *pcoins) const
uint64_t innerUsage = 0;

CCoinsViewCache mempoolDuplicate(const_cast<CCoinsViewCache*>(pcoins));
const int64_t nSpendHeight = GetSpendHeight(mempoolDuplicate);
const int64_t spendheight = GetSpendHeight(mempoolDuplicate);

LOCK(cs);
std::list<const CTxMemPoolEntry*> waitingOnDependants;
Expand Down Expand Up @@ -704,11 +713,7 @@ void CTxMemPool::check(const CCoinsViewCache *pcoins) const
if (fDependsWait)
waitingOnDependants.push_back(&(*it));
else {
CValidationState state;
bool fCheckResult = tx.IsCoinBase() ||
Consensus::CheckTxInputs(tx, state, mempoolDuplicate, nSpendHeight);
assert(fCheckResult);
UpdateCoins(tx, mempoolDuplicate, 1000000);
CheckInputsAndUpdateCoins(tx, mempoolDuplicate, spendheight);
}
}
unsigned int stepsSinceLastRemove = 0;
Expand All @@ -721,10 +726,7 @@ void CTxMemPool::check(const CCoinsViewCache *pcoins) const
stepsSinceLastRemove++;
assert(stepsSinceLastRemove < waitingOnDependants.size());
} else {
bool fCheckResult = entry->GetTx().IsCoinBase() ||
Consensus::CheckTxInputs(entry->GetTx(), state, mempoolDuplicate, nSpendHeight);
assert(fCheckResult);
UpdateCoins(entry->GetTx(), mempoolDuplicate, 1000000);
CheckInputsAndUpdateCoins(entry->GetTx(), mempoolDuplicate, spendheight);
stepsSinceLastRemove = 0;
}
}
Expand Down
28 changes: 15 additions & 13 deletions src/validation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -484,7 +484,6 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool
CCoinsView dummy;
CCoinsViewCache view(&dummy);

CAmount nValueIn = 0;
LockPoints lp;
{
LOCK(pool.cs);
Expand Down Expand Up @@ -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);

Expand All @@ -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;
Copy link
Contributor

@promag promag Sep 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tx_fees or just fees?

Copy link
Contributor Author

@jtimon jtimon Sep 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be more extra disruption only for style. We can do that at any time. Including potentially more rebase work fixing conflicts with other things touching AcceptToMemoryPoolWorker. Can we leave that for later? Note we're not creating a new variable but just moving its declaration here.

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
Expand All @@ -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);
Expand Down Expand Up @@ -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());

Expand Down Expand Up @@ -1635,9 +1633,15 @@ 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;
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
Expand Down Expand Up @@ -1665,8 +1669,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<CScriptCheck> 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))
Expand Down