github
/
bitcoin
mirror of https://github.com/bitcoin/bitcoin
1
Fork 0
Code Releases Activity

consensus: Explain why fCheckDuplicateInputs can not be skipped and remove it

This commit is contained in:
MarcoFalke 2019-10-08 14:42:17 -04:00
parent d53828cb79
commit fa92813407
No known key found for this signature in database
GPG Key ID: CE2B75697E69A548
4 changed files with 13 additions and 18 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

19
src/consensus/tx_check.cpp
View File

@ -7,7 +7,7 @@
#include <primitives/transaction.h>
#include <consensus/validation.h>
bool CheckTransaction(const CTransaction& tx, CValidationState &state, bool fCheckDuplicateInputs)
bool CheckTransaction(const CTransaction& tx, CValidationState& state)
{
// Basic checks that don't depend on any context
if (tx.vin.empty())
@ -31,14 +31,15 @@ bool CheckTransaction(const CTransaction& tx, CValidationState &state, bool fChe
return state.Invalid(ValidationInvalidReason::CONSENSUS, false, "bad-txns-txouttotal-toolarge");
}
// Check for duplicate inputs - note that this check is slow so we skip it in CheckBlock
if (fCheckDuplicateInputs) {
std::set<COutPoint> vInOutPoints;
for (const auto& txin : tx.vin)
{
if (!vInOutPoints.insert(txin.prevout).second)
return state.Invalid(ValidationInvalidReason::CONSENSUS, false, "bad-txns-inputs-duplicate");
}
// Check for duplicate inputs (see CVE-2018-17144)
// While Consensus::CheckTxInputs does check if all inputs of a tx are available, and UpdateCoins marks all inputs
// of a tx as spent, it does not check if the tx has duplicate inputs.
// Failure to run this check will result in either a crash or an inflation bug, depending on the implementation of
// the underlying coins database.
std::set<COutPoint> vInOutPoints;
for (const auto& txin : tx.vin) {
if (!vInOutPoints.insert(txin.prevout).second)
return state.Invalid(ValidationInvalidReason::CONSENSUS, false, "bad-txns-inputs-duplicate");
}
if (tx.IsCoinBase())

2
src/consensus/tx_check.h
View File

@ -15,6 +15,6 @@
class CTransaction;
class CValidationState;
bool CheckTransaction(const CTransaction& tx, CValidationState& state, bool fCheckDuplicateInputs=true);
bool CheckTransaction(const CTransaction& tx, CValidationState& state);
#endif // BITCOIN_CONSENSUS_TX_CHECK_H

7
src/test/fuzz/transaction.cpp
View File

@ -43,12 +43,7 @@ void test_one_input(const std::vector<uint8_t>& buffer)
}
CValidationState state_with_dupe_check;
const bool valid_with_dupe_check = CheckTransaction(tx, state_with_dupe_check, /* fCheckDuplicateInputs= */ true);
CValidationState state_without_dupe_check;
const bool valid_without_dupe_check = CheckTransaction(tx, state_without_dupe_check, /* fCheckDuplicateInputs= */ false);
if (valid_with_dupe_check) {
assert(valid_without_dupe_check);
}
(void)CheckTransaction(tx, state_with_dupe_check);
const CFeeRate dust_relay_fee{DUST_RELAY_TX_FEE};
std::string reason;

3
src/validation.cpp
View File

@ -3301,9 +3301,8 @@ bool CheckBlock(const CBlock& block, CValidationState& state, const Consensus::P
return state.Invalid(ValidationInvalidReason::CONSENSUS, false, "bad-cb-multiple", "more than one coinbase");
// Check transactions
// Must check for duplicate inputs (see CVE-2018-17144)
for (const auto& tx : block.vtx)
if (!CheckTransaction(*tx, state, true))
if (!CheckTransaction(*tx, state))
return state.Invalid(state.GetReason(), false, state.GetRejectReason(),
strprintf("Transaction check failed (tx hash %s) %s", tx->GetHash().ToString(), state.GetDebugMessage()));