search

dtr-org / unit-e

A digital currency for a new era of decentralized trust
https://unit-e.io
MIT License
45 stars 15 forks source link

Remove fCheckDuplicateInputs flag from CheckTransaction #1077

Closed scravy closed 5 years ago

scravy commented 5 years ago

This removes the fCheckDuplicateInputs flag from CheckTransaction in consensus/tx_verify.cpp.

This flag was subject to a CVE in bitcoin (See the full disclosure posting in their blog: https://bitcoincore.org/en/2018/09/20/notice/).

Ever since the CVE was fixed this flag is never passed as false and you must not not check it. Also the behavior is not checked with the parameter false in any test, as can be seen from this pull request as no test had to be touched yet everything passes.

Signed-off-by: Julian Fleischer julian@thirdhash.com

kostyantyn commented 5 years ago

Just want to share that such change is the most closed PR in Bitcoin https://github.com/bitcoin/bitcoin/search?q=fCheckDuplicateInputs&type=Issues

Before we approve, we should read the reasons why they close it (don't merge) and see how it's aligned with our code.

scravy commented 5 years ago

In bitcoin everything goes back to this comment:

NACK we'll probably re-introduce the optimization at some point, let's avoid the code churn.

Which I 100% do not agree with. There's an answer to that even which says:

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.

Which I a 100% do agree with.

scravy commented 5 years ago

but again, this is not applicable anymore since the community had time enough to understand what happened there, and in the end they just removed that.

Yepp. The blog post I am referencing in the description was also written after that.