Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove duplicatable duplicate-input check from CheckTransaction #9049

Merged
merged 3 commits into from
Nov 10, 2016

Conversation

TheBlueMatt
Copy link
Contributor

Benchmark results indicate this saves about 0.5-0.7ms during CheckBlock.

@gmaxwell
Copy link
Contributor

gmaxwell commented Nov 1, 2016

I believe that with this change if a whitelisted peer gives us a transaction with internally duplicated inputs, we will relay it (CheckTransaction will pass, conflict with the mempool will not set DoS.) and then our peers running without this change will ban us (Their CheckTransaction will fail). Less critically, this change results in txn with internal doublespends getting a misleading log entry of mempool-conflict.

Edit: I suggest creating a CheckTransactionHarder that ATMP uses which will check for transaction internal duplication and still nDoS. This way block processing can still avoid the redundant test.

@TheBlueMatt TheBlueMatt force-pushed the 2016-10-bench-checkblock branch 2 times, most recently from b9718b1 to 1019c03 Compare November 1, 2016 16:30
@TheBlueMatt
Copy link
Contributor Author

Simplified diff and fixed.

if (vInOutPoints.count(txin.prevout))
return state.DoS(100, false, REJECT_INVALID, "bad-txns-inputs-duplicate");
vInOutPoints.insert(txin.prevout);
// Check for duplicate inputs - note that this check is slow so we skip it in CheckBlock
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

point out that it's also redundant there please!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is it redundant? Can you point to the case where it's checked a second time?

@gmaxwell
Copy link
Contributor

gmaxwell commented Nov 2, 2016

utACK +/- comment nit.

@@ -0,0 +1,53 @@
// Copyright (c) 2016 Matt Corallo
// Unlike the rest of Bitcoin Core, this file is
// distributed under the Affero General Public License (AGPL v3)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code under this license cannot be accepted, sorry

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol. I didn't notice that.

@laanwj laanwj added the Tests label Nov 2, 2016
@TheBlueMatt
Copy link
Contributor Author

Eek, license change was a mistake, sorry.

On November 2, 2016 5:29:28 AM EDT, "Wladimir J. van der Laan" notifications@github.com wrote:

laanwj requested changes on this pull request.

@@ -0,0 +1,53 @@
+// Copyright (c) 2016 Matt Corallo
+// Unlike the rest of Bitcoin Core, this file is
+// distributed under the Affero General Public License (AGPL v3)

Code under this license cannot be accepted, sorry

You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub:
#9049 (review)

@TheBlueMatt TheBlueMatt force-pushed the 2016-10-bench-checkblock branch from 1019c03 to 2ced91b Compare November 2, 2016 14:06
@TheBlueMatt
Copy link
Contributor Author

Fixed the broken license header.

@sipa
Copy link
Member

sipa commented Nov 3, 2016

A very interesting duplication of the word "duplicate" in the PR title.

@maflcko maflcko changed the title Remove duplicatble duplicate-input check from CheckTransaction Remove duplicatable duplicate-input check from CheckTransaction Nov 3, 2016
@sipa
Copy link
Member

sipa commented Nov 7, 2016

@laanwj I believe you need to dismiss the requested changes (as they're addressed).

@laanwj
Copy link
Member

laanwj commented Nov 8, 2016

Approved the requested change.
BTW: block413567.hex.h is a 6MB file - I'd prefer generating this on the fly as we do with the other .X.h files.
Edit: working on build system change for this...

@laanwj
Copy link
Member

laanwj commented Nov 8, 2016

Can you please squash the top commit of https://github.com/laanwj/bitcoin/tree/2016_11_generate_bench_file into the benchmark commit?

This has the necessary build system change to generate the block.hex file on the fly from the raw data, like with the tests.

@theuni can you have take a look too perhaps?

@theuni
Copy link
Member

theuni commented Nov 8, 2016

@laanwj looks good to me. Only slight criticism is that we may want to generate .h/.cpp pairs for blocks to ensure that they end up in separate compilation units, but we can always address that later if it becomes an issue.

set<COutPoint> vInOutPoints;
for (const auto& txin : tx.vin)
{
if (vInOutPoints.count(txin.prevout))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also worth noting (though not really related here), I measure a ~20%-25% time reduction by avoiding doing 2 lookups:

std::set<COutPoint> vInOutPoints;
for (const auto& txin : tx.vin)
{
    if (!vInOutPoints.insert(txin.prevout).second)
        return state.DoS(100, false, REJECT_INVALID, "bad-txns-inputs-duplicate");
}

@laanwj
Copy link
Member

laanwj commented Nov 9, 2016

@laanwj looks good to me. Only slight criticism is that we may want to generate .h/.cpp pairs for blocks to ensure that they end up in separate compilation units, but we can always address that later if it becomes an issue.

Fully agree. On the long run, I'd like to forego the conversion to hex and subsequent compilation entirely on build chains that supports this, by embedding the binary directly as mentioned in a comment here #6658 (comment). The current way wastes a lot of time and memory while compiling.

@TheBlueMatt TheBlueMatt force-pushed the 2016-10-bench-checkblock branch from 2ced91b to e2b3fb3 Compare November 9, 2016 19:30
@TheBlueMatt
Copy link
Contributor Author

Picked up (and squashed) @laanwj's changes and went ahead and remove the duplicate-lookup for ATMP uses.

@laanwj laanwj merged commit e2b3fb3 into bitcoin:master Nov 10, 2016
laanwj added a commit that referenced this pull request Nov 10, 2016
…saction

e2b3fb3 Optimize vInOutPoints insertion a bit (Matt Corallo)
eecffe5 Remove redundant duplicate-input check from CheckTransaction (Matt Corallo)
b2e178a Add deserialize + CheckBlock benchmarks, and a full block hex (Matt Corallo)
UdjinM6 added a commit to dashpay/dash that referenced this pull request Sep 19, 2018
* Fix crash bug with duplicate inputs within a transaction

Introduced by bitcoin#9049

* Remove redundant parameter fCheckDuplicateInputs from CheckTransaction
EvgenijM86 referenced this pull request in emercoin/emercoin Sep 19, 2018
CryptoDJ added a commit to CryptoDJ/BitcoinDiamond that referenced this pull request Sep 22, 2018
CryptoDJ added a commit to CryptoDJ/namecoin-core that referenced this pull request Sep 22, 2018
CryptoDJ added a commit to CryptoDJ/GameCredits that referenced this pull request Sep 22, 2018
CryptoDJ added a commit to CryptoDJ/Feathercoin that referenced this pull request Sep 22, 2018
CryptoDJ added a commit to CryptoDJ/huntercore that referenced this pull request Sep 22, 2018
@ripper234
Copy link

So, if I understand correctly, this caused CVE-2018-17144.

Can we do a root cause analysis / 5 whys explaining how this was introduced?

@thijstriemstra
Copy link

It's scary to see how this code was merged with so much confidence. The fact there wasn't a unit test covering this piece of code makes me feel little better though, a bad unit test would be even worse. I hope btc core will never accept contributions without tests again.

@dfoderick
Copy link

dfoderick commented Sep 22, 2018

Can we do a root cause analysis / 5 whys explaining how this was introduced?

Short version.
This pull request added a boolean parameter to CheckTransaction method
bool CheckTransaction(const CTransaction& tx, CValidationState &state, bool fCheckDuplicateInputs)

Then in CheckBlock method the flag was passed in as false bypassing the check for duplicate inputs. Assumption was that transaction would already be validated by other code so there was no need to validate a second time.
if (!CheckTransaction(tx, state, false))

Why there was no unit test?
Why there are so many assumptions in code?
Why did programmer not put the flag on transaction class instead?

@ripper234
Copy link

Why this passed code review? (How many people reviewed it?)

@adlai
Copy link
Contributor

adlai commented Sep 23, 2018

@ripper234 commented 5 hours ago

Why this passed code review? (How many people reviewed it?) [sic]

I'm sure Microsoft is the right corporate person to ask.

@ripper234
Copy link

?

@scravy
Copy link
Contributor

scravy commented Sep 24, 2018

@dfoderick I question that these questions are in the spirit of a 5 whys root-cause analysis. You are supposed to go deeper, not wider as you do.

Anyhow, the PR merged here was a refactoring and performance improvement, and there already was no test that could have been broken by this; which I believe to be the real culprit.

@dfoderick
Copy link

Anyhow, the PR merged here was a refactoring and performance improvement, and there already was no test that could have been broken by this; which I believe to be the real culprit.

Correct. The test for this bug did not exist at the time and was only recently added.
9b4a36e

There was a bench test to prove the performance gain but no test for correctness. The bug allows a block with an invalid tx to pass block validation.

willyko pushed a commit to syscoin/syscoin that referenced this pull request Sep 24, 2018
@adlai
Copy link
Contributor

adlai commented Sep 25, 2018

@ripper234 commented 2 days ago

?

I'm gonna assume your eloquence was directed at me; if you're not the antecedent of that pronoun, or my assumption was asinine, please ignore this message!

@ripper234, if you truly care about "why this passed code review" (See how here the grammar is correct, and there's no need for editorialization? Amazing, this whole language business!), then you should be able to assign a lower bound to "How many people reviewed it" with confidence equal to your mnemonic fidelity. If this message is unclear, please go back to code review, and leave the Github issue brigading to the professional trolls (or perhaps, take your concerns to the mailing list).

@jnewbery
Copy link
Contributor

I suggest we lock this PR. For all the drive-by commenters two years after the fact: if you're serious about wanting to improve security and code quality in Bitcoin Core, you can start by:

  • testing release candidates
  • reviewing open PRs
  • contributing code

@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.