Remove redundant parameter fCheckDuplicateInputs from CheckTransaction(...) #14258

pull practicalswift wants to merge 1 commits into bitcoin:master from practicalswift:fCheckDuplicateInputs changing 3 files +8 −11

practicalswift commented at 3:39 PM on September 18, 2018: contributor

Remove redundant parameter fCheckDuplicateInputs from CheckTransaction(...).

Diff without whitespace noise: ?w=1
Remove redundant parameter fCheckDuplicateInputs from CheckTransaction(...) 0172ba4a68
DrahtBot commented at 5:45 PM on September 18, 2018: contributor

No more conflicts as of last run.
DrahtBot cross-referenced this on Sep 18, 2018 from issue we do check for duplicate inputs in CheckBlock by instagibbs
MarcoFalke commented at 9:28 PM on September 18, 2018: member

utACK 0172ba4a6800ac934f8b4712c27b5ccd66deb658
donaloconnor commented at 9:54 PM on September 18, 2018: contributor

utACK https://github.com/bitcoin/bitcoin/commit/0172ba4a6800ac934f8b4712c27b5ccd66deb658
MarcoFalke added the label Refactoring on Sep 19, 2018
Empact commented at 4:09 AM on September 19, 2018: member

utACK 0172ba4
promag commented at 12:22 PM on September 19, 2018: member

utACK 0172ba4.
TheBlueMatt commented at 1:06 PM on September 19, 2018: contributor
NACK we'll probably re-introduce the optimization at some point, let's avoid the code churn.

On September 18, 2018 3:42:40 PM UTC, practicalswift notifications@github.com wrote:
Remove redundant parameter fCheckDuplicateInputs from CheckTransaction(...).

Diff without whitespace noise: ?w=1 You can view, comment on, or merge this pull request online at:

#14258

-- Commit Summary --

Remove redundant parameter fCheckDuplicateInputs from CheckTransaction(...)

-- File Changes --

M src/consensus/tx_verify.cpp (15) M src/consensus/tx_verify.h (2) M src/validation.cpp (2)

-- Patch Links --

https://github.com/bitcoin/bitcoin/pull/14258.patch https://github.com/bitcoin/bitcoin/pull/14258.diff

-- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/bitcoin/bitcoin/pull/14258
ryanofsky cross-referenced this on Sep 19, 2018 from issue Fix crash bug with duplicate inputs within a transaction by TheBlueMatt
practicalswift closed this on Sep 19, 2018
promag commented at 5:51 PM on September 19, 2018: member

IMO leaving unused code that can lead to bugs is worst than code churn. This can be reverted along with proper review/testing once and if necessary.
donaloconnor commented at 6:16 PM on September 19, 2018: contributor

I am in agreement with @promag and was about to say similar. I don't see the issue with this PR.
practicalswift reopened this on Sep 19, 2018
practicalswift commented at 9:27 PM on September 19, 2018: contributor

Re-opened since consensus opinion seems to closer to be ACK side than the NACK side :-)
promag commented at 9:37 PM on September 19, 2018: member

@practicalswift to avoid noise please gather more comments before changing PR status.
decorumvelox commented at 2:46 AM on September 20, 2018: none

utACK 0172ba4a6800ac934f8b4712c27b5ccd66deb658. It is safer to remove this unused code now. Any future optimizations should be written in a different way that is more clear and not easily misunderstood as benign.
achow101 commented at 3:13 AM on September 20, 2018: member

utACK 0172ba4a6800ac934f8b4712c27b5ccd66deb658
sipa commented at 6:08 AM on September 20, 2018: member

NACK right now.

Let's please take some time to study the reasons that caused the issue first before we touch this code further.
laanwj commented at 6:12 AM on September 20, 2018: member

I agree with @sipa, cleaning up this code is not a priority now.
laanwj closed this on Sep 20, 2018
promag cross-referenced this on Nov 28, 2018 from issue remove fCheckDuplicateInputs by 3s3s
practicalswift deleted the branch on Apr 10, 2021
bitcoin locked this on Aug 16, 2022