highperfocused/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-06-17 20:32:33 +02:00
Code Issues Packages Projects Releases Wiki Activity

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

Browse Source 
fa928134075220254a15107c1d9702f4e66271f8 consensus: Explain why fCheckDuplicateInputs can not be skipped and remove it (MarcoFalke)

Pull request description:

  As a follow up to CVE-2018-17144, this removes the unused `fCheckDuplicateInputs` parameter and explains why the test can not be disabled. Apart from protecting against a dumb accident in the future, this should document the logic in the code. There is a technical write-up that explains how the underlying coins database behaves if this test is skipped: https://bitcoincore.org/en/2018/09/20/notice/#technical-details. However, it does not explicitly mention why the test can not be skipped. I hope my code comment does that.

ACKs for top commit:
  jnewbery:
    ACK fa928134075220254a15107c1d9702f4e66271f8
  amitiuttarwar:
    utACK fa928134075220254a15107c1d9702f4e66271f8
  Empact:
    Code review ACK fa92813407
  promag:
    ACK fa928134075220254a15107c1d9702f4e66271f8.

Tree-SHA512: fc1ef670f1a467c543b84f704b9bd8cc7a59a9f707be048bd9b4e85fe70830702aa560a880efa2c840bb43818ab44dfdc611104df04db2ddc14ff92f46bfb28e
This commit is contained in:
Wladimir J. van der Laan 2019-10-25 13:46:41 +02:00
commit 90ed98ae9a
No known key found for this signature in database
GPG Key ID: 1E4AED62986CD25D
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()));