Remove unused fScriptChecks parameter from CheckInputs #13868

Empact · 2018-08-03T17:59:56Z

fScriptChecks = false just short-circuits the entire function, so
passing it in is entirely useless.

This is extracted from #13233 /cc TheBlueMatt.

Recommend reviewing with git show --ignore-all-space, i.e.:
https://github.com/bitcoin/bitcoin/pull/13868/files?w=1

src/validation.cpp

DrahtBot · 2018-08-03T19:35:15Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#16658 ([WIP] validation: Rename CheckInputs to CheckInputScripts by jnewbery)
#16486 ([consensus] skip bip30 checks when assumevalid is set for the block by pstratem)
#13204 (Faster sigcache nonce by JeremyRubin)

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.

promag

utACK 1d7cc70.

Recommend reviewing with git show --ignore-all-space

Also view with&w=1.

src/validation.cpp

Empact · 2018-08-04T06:01:09Z

@practicalswift @TheBlueMatt deserves the credit. :)

Empact · 2018-08-07T15:32:35Z

Rebased for #13527

Empact · 2018-08-27T16:54:42Z

Rebased for #13083

Empact · 2018-12-12T22:34:15Z

Rebased for #14908, updated to single line coinbase check.

jnewbery · 2019-02-22T19:11:51Z

Concept ACK, but this removes the quite useful comment about how assume valid works:

        // Skip script verification when connecting blocks under the	
         // assumevalid block. Assuming the assumevalid block is valid this
        // is safe because block merkle hashes are still computed and checked,
        // Of course, if an assumed valid block is invalid due to false scriptSigs
        // this optimization would allow an invalid chain to be accepted.

Perhaps you can incorporate that information after the line:

bitcoin/src/validation.cpp

Line 1850 in 169dced

    
           // This block is a member of the assumed verified chain and an ancestor of the best header.

Empact · 2019-05-17T03:33:25Z

Rebased and I split the coinbase change into another commit.

Once again: https://github.com/bitcoin/bitcoin/pull/13868/files?w=1

jnewbery · 2019-05-17T13:47:58Z

Fails tx_validationcache_tests/checkinputs_test (both on travis and locally)

Empact · 2019-08-19T06:04:42Z

Rebased, and squashed the minor coinbase change.
https://github.com/bitcoin/bitcoin/pull/13868/files?w=1

jnewbery · 2019-08-19T15:05:40Z

utACK 5b99467

fanquake · 2019-08-23T13:04:03Z

@TheBlueMatt @BugTheBlueMatt want to take a look here given it's your change.

TheBlueMatt

Concept ACK.

src/validation.cpp

fScriptChecks = false just short-circuits the entire function, so passing it in is entirely useless.

Empact · 2019-08-26T23:39:41Z

@TheBlueMatt thanks for pointing that out. Comment moved to AcceptToMemoryPoolWorker.

TheBlueMatt · 2019-08-31T19:38:38Z

utACK 9b92538. Checked diff had no functional change and new comment copy looks correct.

fanquake

ACK 9b92538 - Notes / testing below.

CheckInputs() is called from:

CheckInputsFromMempoolAndCache() - fScriptChecks was true
AcceptToMemoryPoolWorker() - 3 times and fScriptChecks was always true
CChainState::ConnectBlock() - fScriptChecks is true unless
1. we have an assumeValid hash and
2. GetBlockProofEquivalentTime() is <= 1’209’600 seconds ( 2 weeks)

Changes to CheckInputs():

Drop the fScriptChecks parameter.
IsCoinBase() check is now clearer. i.e

if (!tx.IsCoinBase()) {
    // do input checking
}
return true

is now:

if (tx.IsCoinBase()) return true;
// do input checking
return true

Brackets around the pvChecks check.
Moved the "The first loop above does.." comment to AcceptToMemoryPoolWorker.
Moved "Skip script verification.." to ConnectBlock.

Added some logging and tested:

Given a garbage -assumevalid=garbage hash fScriptChecks was true (early IBD).
Modified CheckInputs() to return false - checked that we drop into the ConnectBlock error.
Modified CheckInputs() to return false and ran with -assumevalid=garbage - checked that we drop into the ConnectBlock error.

Other thought. It looks like everywhere we call CheckInputs() we are already checking prior that the tx is not a coinbase. Although I cannot see a downside to a belt-and-suspenders check inside CheckInputs().

@jnewbery can you re-review here?

kallewoof · 2019-09-01T03:30:06Z

ACK 9b92538

Verified that function does essentially nothing if the flag is false.

ajtowns · 2019-09-02T06:46:07Z

ACK 9b92538 ; code review, checked tests work. Looks right to me, and fanquake's notes make sense. Could change the coinbase early exit to assert(!tx.IsCoinBase());.

fanquake · 2019-09-02T07:44:41Z

Could change the coinbase early exit to assert(!tx.IsCoinBase());

I think that can be discussed / done in a follow up. We've got ACKs, so I'm merging this.

Thanks to those of you that I harassed for review for following up. Post-merge ACKs still welcome.

9b92538 Remove unused fScriptChecks parameter from CheckInputs (Matt Corallo) Pull request description: fScriptChecks = false just short-circuits the entire function, so passing it in is entirely useless. This is extracted from #13233 /cc TheBlueMatt. Recommend reviewing with `git show --ignore-all-space`, i.e.: https://github.com/bitcoin/bitcoin/pull/13868/files?w=1 ACKs for top commit: TheBlueMatt: utACK 9b92538. Checked diff had no functional change and new comment copy looks correct. kallewoof: ACK 9b92538 ajtowns: ACK 9b92538 ; code review, checked tests work. Looks right to me, and fanquake's notes make sense. Could change the coinbase early exit to `assert(!tx.IsCoinBase());`. fanquake: ACK 9b92538 - Notes / testing below. Tree-SHA512: add253a3e8cf4b33eddbc49efcec333c14b5ea61c7d34e43230351d40cff6adc919a75b91c72c4de8647a395284db74a61639f4c67848d4b2fec3a705b557790

laanwj · 2019-09-02T09:14:02Z

Posthumous ACK 9b92538

jnewbery

post-merge ACK 9b92538 with one comment nit.

jnewbery · 2019-09-02T14:45:15Z

src/validation.cpp

@@ -773,15 +773,17 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool
        constexpr unsigned int scriptVerifyFlags = STANDARD_SCRIPT_VERIFY_FLAGS;

        // Check against previous transactions
-        // This is done last to help prevent CPU exhaustion denial-of-service attacks.
+        // The first loop above does all the inexpensive checks.


This is a really confusing comment here. "The first loop" refers to the checks inside CheckTxInputs() call on L566, which has been moved around since the comment was first written.

I'll fix this comment in a follow-up PR.

Being addressed in #16658.

…Inputs 9b92538 Remove unused fScriptChecks parameter from CheckInputs (Matt Corallo) Pull request description: fScriptChecks = false just short-circuits the entire function, so passing it in is entirely useless. This is extracted from bitcoin#13233 /cc TheBlueMatt. Recommend reviewing with `git show --ignore-all-space`, i.e.: https://github.com/bitcoin/bitcoin/pull/13868/files?w=1 ACKs for top commit: TheBlueMatt: utACK 9b92538. Checked diff had no functional change and new comment copy looks correct. kallewoof: ACK 9b92538 ajtowns: ACK 9b92538 ; code review, checked tests work. Looks right to me, and fanquake's notes make sense. Could change the coinbase early exit to `assert(!tx.IsCoinBase());`. fanquake: ACK 9b92538 - Notes / testing below. Tree-SHA512: add253a3e8cf4b33eddbc49efcec333c14b5ea61c7d34e43230351d40cff6adc919a75b91c72c4de8647a395284db74a61639f4c67848d4b2fec3a705b557790

Summary: Remove unused fScriptChecks parameter from CheckInputs (Matt Corallo) Pull request description: fScriptChecks = false just short-circuits the entire function, so passing it in is entirely useless. This is extracted from #13233 /cc TheBlueMatt. Recommend reviewing with `git show --ignore-all-space`, i.e.: https://github.com/bitcoin/bitcoin/pull/13868/files?w=1 --- Backport of Core [[bitcoin/bitcoin#13868 | PR13868]] Test Plan: ninja check check-functional Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D7628

maflcko reviewed Aug 3, 2018

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

Empact force-pushed the check-inputs-script-checks branch from b161464 to 1d7cc70 Compare August 3, 2018 18:10

This was referenced Aug 3, 2018

validation: Document where we are intentionally ignoring bool return values from validation related functions #13864

Closed

policy: Remove promiscuousmempoolflags #13527

Merged

promag reviewed Aug 3, 2018

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

fanquake added Refactoring Validation labels Aug 4, 2018

DrahtBot added the Needs rebase label Aug 7, 2018

Empact force-pushed the check-inputs-script-checks branch from 1d7cc70 to 7b471ea Compare August 7, 2018 15:32

DrahtBot removed the Needs rebase label Aug 7, 2018

DrahtBot added the Needs rebase label Aug 25, 2018

Empact force-pushed the check-inputs-script-checks branch from 7b471ea to 852c7aa Compare August 27, 2018 16:52

DrahtBot removed the Needs rebase label Aug 27, 2018

This was referenced Aug 31, 2018

Minor style enhancement in documentation #14091

Closed

refactor: Make explicit CMutableTransaction -> CTransaction conversion. #14156

Closed

Faster sigcache nonce #13204

Merged

DrahtBot mentioned this pull request Oct 9, 2018

Remove redundant run time assertions for locks already checked at compile time #14443

Closed

DrahtBot mentioned this pull request Oct 23, 2018

test: Add linter to make sure single parameter constructors are marked explicit #14505

Merged

DrahtBot added the Needs rebase label Dec 12, 2018

Empact force-pushed the check-inputs-script-checks branch from 852c7aa to b5f7e32 Compare December 12, 2018 22:33

DrahtBot removed the Needs rebase label Dec 12, 2018

DrahtBot added the Needs rebase label Mar 2, 2019

Empact force-pushed the check-inputs-script-checks branch 2 times, most recently from 099f32e to d99bb14 Compare March 4, 2019 09:11

DrahtBot added the Needs rebase label May 4, 2019

Empact force-pushed the check-inputs-script-checks branch from d99bb14 to dab9e84 Compare May 17, 2019 03:33

DrahtBot removed the Needs rebase label May 17, 2019

Empact force-pushed the check-inputs-script-checks branch from dab9e84 to 195dfcf Compare May 17, 2019 18:28

DrahtBot added the Needs rebase label Aug 15, 2019

Empact force-pushed the check-inputs-script-checks branch 2 times, most recently from 8fed5a1 to 5b99467 Compare August 19, 2019 06:01

DrahtBot removed the Needs rebase label Aug 19, 2019

jnewbery mentioned this pull request Aug 19, 2019

validation: Rename CheckInputs to CheckInputScripts #16658

Merged

TheBlueMatt reviewed Aug 26, 2019

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

Remove unused fScriptChecks parameter from CheckInputs

9b92538

fScriptChecks = false just short-circuits the entire function, so passing it in is entirely useless.

Empact force-pushed the check-inputs-script-checks branch from 5b99467 to 9b92538 Compare August 26, 2019 23:32

fanquake approved these changes Sep 1, 2019

View reviewed changes

fanquake merged commit 9b92538 into bitcoin:master Sep 2, 2019

jnewbery reviewed Sep 2, 2019

View reviewed changes

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Remove unused fScriptChecks parameter from CheckInputs #13868

Remove unused fScriptChecks parameter from CheckInputs #13868

Uh oh!

Conversation

Empact commented Aug 3, 2018 • edited by fanquake Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

DrahtBot commented Aug 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Empact commented Aug 4, 2018

Uh oh!

Empact commented Aug 7, 2018

Uh oh!

Empact commented Aug 27, 2018

Uh oh!

Empact commented Dec 12, 2018

Uh oh!

jnewbery commented Feb 22, 2019

Uh oh!

Empact commented May 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery commented May 17, 2019

Uh oh!

Empact commented Aug 19, 2019

Uh oh!

jnewbery commented Aug 19, 2019

Uh oh!

fanquake commented Aug 23, 2019

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Empact commented Aug 26, 2019

Uh oh!

TheBlueMatt commented Aug 31, 2019

Uh oh!

fanquake left a comment

Choose a reason for hiding this comment

Uh oh!

kallewoof commented Sep 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ajtowns commented Sep 2, 2019

Uh oh!

fanquake commented Sep 2, 2019

Uh oh!

laanwj commented Sep 2, 2019

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery Sep 2, 2019

Choose a reason for hiding this comment

Uh oh!

fanquake Sep 3, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Empact commented Aug 3, 2018 •

edited by fanquake

Loading

DrahtBot commented Aug 3, 2018 •

edited

Loading

Empact commented May 17, 2019 •

edited

Loading

kallewoof commented Sep 1, 2019 •

edited

Loading