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

pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:1909-docCheckInputs changing 4 files +13 −18

MarcoFalke commented at 7:17 pm on October 8, 2019: member

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.
MarcoFalke added the label Consensus on Oct 8, 2019
MarcoFalke added the label Docs on Oct 8, 2019
in src/validation.cpp:3304 in dddd542663 outdated
3306@@ -3307,7 +3307,7 @@ bool CheckBlock(const CBlock& block, CValidationState& state, const Consensus::P 3307 // Check transactions 3308 // Must check for duplicate inputs (see CVE-2018-17144)
promag commented at 8:15 pm on October 8, 2019:

Drop this comment? Option removed.

MarcoFalke commented at 8:33 pm on October 8, 2019:

The check must still be done and is being done, so I think the comment properly reflects that.

promag commented at 8:45 pm on October 8, 2019:

CheckTransaction does more than that but it’s not documented here. The comment was here because of the (now removed) option.

But I agree that, in practice, it still applies.

amitiuttarwar commented at 6:19 pm on October 24, 2019:

+1 think comment is unnecessary here with the caller since its default & explained in the function with the comment here. But not a huge deal.
promag commented at 8:15 pm on October 8, 2019: member

Concept ACK.
DrahtBot commented at 11:05 pm on October 8, 2019: member
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:
- #15921 (validation: Tidy up ValidationState interface by jnewbery)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
jonasschnelli commented at 6:36 am on October 9, 2019: contributor

code review utACK dddd5426630403cc2f9ce1ac3cc3e7660c8c7122 - we don’t use CheckTransaction with fCheckDuplicatedInputs set to false.

codestyle: I would probably try to get rid of the block from L39-46.

in src/consensus/tx_check.cpp:39 in dddd542663 outdated

36+    // Check for duplicate inputs (see CVE-2018-17144)
37+    // While Consensus::CheckTxInputs does check if all inputs of a tx are available, and UpdateCoins marks all inputs
38+    // of a tx as spent, it does not check if the tx has duplicate inputs.
39+    // Failure to run this check will result in either a crash or an inflation bug, depending on the implementation of
40+    // the underlying coins database.
41+    {

Empact commented at 12:56 pm on October 9, 2019:

nit: IMO removing the codeblock leaves the code simpler overall, and reviewing without it is simple enough with ?w=1

Empact commented at 1:17 pm on October 9, 2019: member

ACK dddd5426630403cc2f9ce1ac3cc3e7660c8c7122
DrahtBot commented at 9:35 am on October 24, 2019: member
DrahtBot added the label Needs rebase on Oct 24, 2019
mzumsande commented at 11:55 am on October 24, 2019: member

From the PR/commit title I expected a doc-only change. I realize that this is no functional change, but I think that the fact that consensus code is changed to remove the ability of skipping the duplicate input check should be made more clear.
jnewbery commented at 5:04 pm on October 24, 2019: member

Code review ACK dddd5426630403cc2f9ce1ac3cc3e7660c8c7122.

I agree with the other reviewers that the code block should be removed (https://github.com/bitcoin/bitcoin/pull/17080#issuecomment-539858898 and #17080 (review)) and that the PR title should be updated to show that this isn’t just a documentation change (https://github.com/bitcoin/bitcoin/pull/17080#issuecomment-545883258).
MarcoFalke renamed this:
~~consensus: Explain why fCheckDuplicateInputs can not be skipped~~
consensus: Explain why fCheckDuplicateInputs can not be skipped and remove it
on Oct 24, 2019
MarcoFalke force-pushed on Oct 24, 2019
MarcoFalke force-pushed on Oct 24, 2019
MarcoFalke commented at 6:00 pm on October 24, 2019: member

changed whitespace (as requested by three reviewers) because I had to rebase anyway and invalidate all their ACKs.
fanquake removed the label Needs rebase on Oct 24, 2019
amitiuttarwar commented at 6:28 pm on October 24, 2019: contributor

code review utACK fa8130459799ccd4baca5b6565ce57cddd5b4b01.
MarcoFalke added the label Refactoring on Oct 24, 2019
consensus: Explain why fCheckDuplicateInputs can not be skipped and remove it fa92813407
MarcoFalke force-pushed on Oct 24, 2019
MarcoFalke commented at 6:59 pm on October 24, 2019: member

Removed the duplicate comment (as requested by two reviewers)
jnewbery commented at 7:10 pm on October 24, 2019: member

ACK fa928134075220254a15107c1d9702f4e66271f8
amitiuttarwar commented at 7:26 pm on October 24, 2019: contributor

utACK fa928134075220254a15107c1d9702f4e66271f8
Empact commented at 8:50 pm on October 24, 2019: member

Code review ACK https://github.com/bitcoin/bitcoin/commit/fa928134075220254a15107c1d9702f4e66271f8
promag commented at 8:04 am on October 25, 2019: member

ACK fa928134075220254a15107c1d9702f4e66271f8.
laanwj referenced this in commit 90ed98ae9a on Oct 25, 2019
laanwj merged this on Oct 25, 2019
laanwj closed this on Oct 25, 2019
MarcoFalke deleted the branch on Oct 25, 2019
deadalnix referenced this in commit 28928b0275 on Jul 9, 2020
DrahtBot locked this on Dec 16, 2021

Contributors
MarcoFalke promag amitiuttarwar DrahtBot jonasschnelli Empact mzumsande jnewbery

Labels
Refactoring Docs Consensus

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

Conflicts