sikkienl/dash
1
0
Fork 0
mirror of https://github.com/dashpay/dash.git synced 2024-12-25 12:02:48 +01:00
Code Issues Actions 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 committed by munkybooty
parent 4ecc49fb73
commit 1d1cca6fb0
3 changed files with 6 additions and 8 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
src/consensus/tx_check.cpp
View File

@ -40,7 +40,11 @@ bool CheckTransaction(const CTransaction& tx, CValidationState& state)
return state.DoS(100, false, REJECT_INVALID, "bad-txns-txouttotal-toolarge");
}
// Check for duplicate inputs
// 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)

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

@ -54,12 +54,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);
CValidationState state_without_dupe_check;
const bool valid_without_dupe_check = CheckTransaction(tx, state_without_dupe_check);
if (valid_with_dupe_check) {
assert(valid_without_dupe_check);
}
(void)CheckTransaction(tx, state_with_dupe_check);
std::string reason;
const bool is_standard_with_permit_bare_multisig = IsStandardTx(tx, reason);

1
src/validation.cpp
View File

@ -3764,7 +3764,6 @@ bool CheckBlock(const CBlock& block, CValidationState& state, const Consensus::P
return state.DoS(100, false, REJECT_INVALID, "bad-cb-multiple", false, "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))
return state.Invalid(false, state.GetRejectCode(), state.GetRejectReason(),