diff options
| -rw-r--r-- | src/consensus/tx_check.cpp | 19 | ||||
| -rw-r--r-- | src/consensus/tx_check.h | 2 | ||||
| -rw-r--r-- | src/test/fuzz/transaction.cpp | 7 | ||||
| -rw-r--r-- | src/validation.cpp | 3 |
4 files changed, 13 insertions, 18 deletions
diff --git a/src/consensus/tx_check.cpp b/src/consensus/tx_check.cpp index 1206035839..6793f871cf 100644 --- a/src/consensus/tx_check.cpp +++ b/src/consensus/tx_check.cpp @@ -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()) diff --git a/src/consensus/tx_check.h b/src/consensus/tx_check.h index bcfdf36bf9..6f3f8fe969 100644 --- a/src/consensus/tx_check.h +++ b/src/consensus/tx_check.h @@ -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 diff --git a/src/test/fuzz/transaction.cpp b/src/test/fuzz/transaction.cpp index 96d7947b07..383d879040 100644 --- a/src/test/fuzz/transaction.cpp +++ b/src/test/fuzz/transaction.cpp @@ -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; diff --git a/src/validation.cpp b/src/validation.cpp index 70b847d3b0..f1abcadefc 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -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())); |