validation: Rename CheckInputs to CheckInputScripts #16658

jnewbery · 2019-08-19T16:13:44Z

CheckInputs() used to check no double spends, scripts & sigs and amounts. Since
832e074, the double spend and amount checks
have been moved to CheckTxInputs(), and CheckInputs() now just validates
input scripts. Rename the function to CheckInputScripts().

Also fix incorrect comments.

DrahtBot · 2019-08-19T18:29:00Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17399 (validation: Templatize ValidationState instead of subclassing by jkczyz)

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.

fanquake

Concept ACK

This builds on #13868. Please review that PR first.

I have reviewed the base PR.

Should 046cd75 be a scripted-diff? This should produce the same change:

gsed -i 's/\<CheckInputs\>/CheckInputScripts/g' src/*.h src/*.cpp src/**/*.h src/**/*.cpp

This needs a couple functional test changes for the expected messages:

bitcoin/test/functional/feature_dersig.py

Line 127 in 7d6f63c

    
           with self.nodes[0].assert_debug_log(expected_msgs=['CheckInputs on {} failed with non-mandatory-script-verify-flag (Non-canonical DER signature)'.format(block.vtx[-1].hash)]):

bitcoin/test/functional/feature_cltv.py

Line 141 in 7d6f63c

    
           with self.nodes[0].assert_debug_log(expected_msgs=['CheckInputs on {} failed with non-mandatory-script-verify-flag (Negative locktime)'.format(block.vtx[-1].hash)]):

jnewbery · 2019-09-02T15:25:36Z

Rebased and removed WIP tag.

Thanks for the review @fanquake

This needs a couple functional test changes for the expected messages.

Fixed. Thanks.

Should 046cd75 be a scripted-diff?

I think this is small enough to be a non-scripted commit, and I'd need to add more to your suggested one-liner to make the test changes atomic with the validation.cpp changes. I'm leaving as it is for now, but can change if other reviewers agree that scripted would be better.

ajtowns · 2019-09-03T11:57:49Z

I'd need to add more to your suggested one-liner to make the test changes atomic with the validation.cpp changes

FWIW, I believe a valid scripted-diff line for 3623a8f would be:

sed -i -E -e 's/CheckInputs\b/CheckInputScripts/g' $(git grep -l CheckInputs | grep -v doc/)

src/validation.cpp

jnewbery · 2019-09-04T22:32:40Z

FWIW, I believe a valid scripted-diff line for 3623a8f would be...

I've updated the commit to use your scripted diff one-liner. Thanks!

michaelfolkson · 2019-09-07T12:49:09Z

Concept ACK. I ran a git grep on CheckInputs in this PR branch and apart from old release notes that obviously won't change, there is a ValidateCheckInputsForAllFlags function in src/test/txvalidationcache_tests.cpp, a CheckInputsAndUpdateCoins function in src/txmempool.cpp and CheckInputsFromMempoolAndCache function in src/validation.cpp. Should they be changed too?

fanquake · 2019-09-08T08:10:38Z

@jnewbery Looks like the scripted diff needs fixing:

$ set -o errexit; source ./ci/lint/06_script.sh
Error: script block marker but no scripted-diff in title
Failed

jnewbery · 2019-09-08T14:59:09Z

Looks like the scripted diff needs fixing

Thanks. Will fix next week.

jnewbery · 2019-09-10T12:56:22Z

Looks like the scripted diff needs fixing

should be fixed

jnewbery · 2019-10-01T14:18:31Z

rebased

maflcko · 2019-10-11T19:21:39Z

ACK 3ee9ad7 thanks for fixing documentation

Show signature and timestamp

Signature:

Timestamp of file with hash 2fb4b49625388db44b56838d9617fb9d4d07331a02b3a6db6af05658e13fad54 -

jnewbery · 2019-10-25T13:31:03Z

rebased

maflcko · 2019-10-25T15:44:44Z

re-ACK 44d20b8 only change is rebase the scripted diff

Show signature and timestamp

Signature:

Timestamp of file with hash 0630b39dafbd879abe6610f1a999b11bd28a1e6e9f1c97ebdfa437aa875e56fa -

promag · 2019-10-25T15:55:34Z

TIL git grep -l CheckInputs -- ':!doc'.

Code review ACK.

jnewbery · 2019-10-30T16:47:32Z

Rebased on master

DrahtBot · 2019-11-07T16:31:27Z

Needs rebase

maflcko · 2019-11-07T17:18:25Z

This is a trivial fix that should be ready for merge after rebase

CheckInputs() used to check no double spends, scripts & sigs and amounts. Since 832e074, the double spend and amount checks have been moved to CheckTxInputs(), and CheckInputs() now just validates input scripts. Rename the function to CheckInputScripts(). -BEGIN VERIFY SCRIPT- sed -i -E -e 's/CheckInputs\b/CheckInputScripts/g' $(git grep -l CheckInputs | grep -v doc/) -END VERIFY SCRIPT-

jnewbery · 2019-11-07T18:53:28Z

Rebased

maflcko · 2019-11-07T19:04:26Z

re-ACK 3bd8db8, did the rebase myself, checked the scripted diff 👡

Show signature and timestamp

Signature:

Timestamp of file with hash 4aa6710c657c2553e2de927e5918efcd786d8c666d5ddcdb193f75684907f609 -

promag · 2019-11-07T19:06:58Z

ACK 3bd8db8

3bd8db8 [validation] fix comments in CheckInputScripts() (John Newbery) 6f6465c scripted-diff: [validation] Rename CheckInputs to CheckInputScripts (John Newbery) Pull request description: CheckInputs() used to check no double spends, scripts & sigs and amounts. Since 832e074, the double spend and amount checks have been moved to CheckTxInputs(), and CheckInputs() now just validates input scripts. Rename the function to CheckInputScripts(). Also fix incorrect comments. ACKs for top commit: MarcoFalke: re-ACK 3bd8db8, did the rebase myself, checked the scripted diff 👡 promag: ACK 3bd8db8

Tree-SHA512: 7b3f8597d210492798fb784ee8ea47ea6377519111190161c7cc34a967509013f4337304f52e9bedc97b7710de7b0ff8880e08cd7f867754567f82e7b02c794c

3bd8db8 [validation] fix comments in CheckInputScripts() (John Newbery) 6f6465c scripted-diff: [validation] Rename CheckInputs to CheckInputScripts (John Newbery) Pull request description: CheckInputs() used to check no double spends, scripts & sigs and amounts. Since 832e074, the double spend and amount checks have been moved to CheckTxInputs(), and CheckInputs() now just validates input scripts. Rename the function to CheckInputScripts(). Also fix incorrect comments. ACKs for top commit: MarcoFalke: re-ACK 3bd8db8, did the rebase myself, checked the scripted diff 👡 promag: ACK 3bd8db8

Tree-SHA512: 7b3f8597d210492798fb784ee8ea47ea6377519111190161c7cc34a967509013f4337304f52e9bedc97b7710de7b0ff8880e08cd7f867754567f82e7b02c794c

3bd8db8 [validation] fix comments in CheckInputScripts() (John Newbery) 6f6465c scripted-diff: [validation] Rename CheckInputs to CheckInputScripts (John Newbery) Pull request description: CheckInputs() used to check no double spends, scripts & sigs and amounts. Since 832e074, the double spend and amount checks have been moved to CheckTxInputs(), and CheckInputs() now just validates input scripts. Rename the function to CheckInputScripts(). Also fix incorrect comments. ACKs for top commit: MarcoFalke: re-ACK 3bd8db8, did the rebase myself, checked the scripted diff 👡 promag: ACK 3bd8db8

Tree-SHA512: 7b3f8597d210492798fb784ee8ea47ea6377519111190161c7cc34a967509013f4337304f52e9bedc97b7710de7b0ff8880e08cd7f867754567f82e7b02c794c

Summary: 3bd8db80d8d335ab63ece4f110b0fadd562e80b7 [validation] fix comments in CheckInputScripts() (John Newbery) 6f6465cefcd599c89c00f7b51f42a4b87a5ffb0b scripted-diff: [validation] Rename CheckInputs to CheckInputScripts (John Newbery) Pull request description: CheckInputs() used to check no double spends, scripts & sigs and amounts. Since 832e074, the double spend and amount checks have been moved to CheckTxInputs(), and CheckInputs() now just validates input scripts. Rename the function to CheckInputScripts(). Also fix incorrect comments. --- Backport of Core [[bitcoin/bitcoin#16658 | PR16658]] Test Plan: ninja all check check-functional Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Subscribers: deadalnix Differential Revision: https://reviews.bitcoinabc.org/D8762

This comment has been minimized.

Sign in to view

DrahtBot added Tests Validation labels Aug 19, 2019

jnewbery force-pushed the 2019-08-rename-CheckInputs branch from 4618cf2 to bf5b449 Compare August 25, 2019 19:47

jnewbery changed the title ~~[validation] Rename CheckInputs to CheckInputScripts~~ validation: Rename CheckInputs to CheckInputScripts Aug 25, 2019

jnewbery changed the title ~~validation: Rename CheckInputs to CheckInputScripts~~ [WIP] validation: Rename CheckInputs to CheckInputScripts Aug 26, 2019

michaelfolkson mentioned this pull request Sep 1, 2019

doc: Update comment describing CheckInputs in validation.cpp #16781

Closed

fanquake reviewed Sep 2, 2019

View reviewed changes

DrahtBot added the Needs rebase label Sep 2, 2019

jnewbery force-pushed the 2019-08-rename-CheckInputs branch from bf5b449 to 41d1528 Compare September 2, 2019 15:23

jnewbery changed the title ~~[WIP] validation: Rename CheckInputs to CheckInputScripts~~ validation: Rename CheckInputs to CheckInputScripts Sep 2, 2019

DrahtBot removed the Needs rebase label Sep 2, 2019

fanquake mentioned this pull request Sep 3, 2019

Remove unused fScriptChecks parameter from CheckInputs #13868

Merged

maflcko reviewed Sep 3, 2019

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

jnewbery force-pushed the 2019-08-rename-CheckInputs branch from 41d1528 to d0dd908 Compare September 4, 2019 22:32

jnewbery force-pushed the 2019-08-rename-CheckInputs branch from d0dd908 to 3386c65 Compare September 10, 2019 12:55

DrahtBot added the Needs rebase label Sep 18, 2019

jnewbery force-pushed the 2019-08-rename-CheckInputs branch from 3386c65 to 3ee9ad7 Compare October 1, 2019 14:18

DrahtBot removed the Needs rebase label Oct 1, 2019

maflcko removed the Tests label Oct 11, 2019

maflcko added the Docs label Oct 11, 2019

maflcko mentioned this pull request Oct 17, 2019

bench: Add BIP 143 sighash benchmark #17175

Open

DrahtBot added the Needs rebase label Oct 25, 2019

jnewbery force-pushed the 2019-08-rename-CheckInputs branch from 3ee9ad7 to 44d20b8 Compare October 25, 2019 13:31

fanquake removed the Needs rebase label Oct 25, 2019

DrahtBot added the Needs rebase label Oct 30, 2019

jnewbery force-pushed the 2019-08-rename-CheckInputs branch from 44d20b8 to ff6fa17 Compare October 30, 2019 16:47

DrahtBot removed the Needs rebase label Oct 30, 2019

DrahtBot added the Needs rebase label Nov 7, 2019

jnewbery added 2 commits November 7, 2019 13:50

[validation] fix comments in CheckInputScripts()

3bd8db8

jnewbery force-pushed the 2019-08-rename-CheckInputs branch from ff6fa17 to 3bd8db8 Compare November 7, 2019 18:52

fanquake removed the Needs rebase label Nov 7, 2019

maflcko merged commit 3bd8db8 into bitcoin:master Jan 2, 2020

jnewbery deleted the 2019-08-rename-CheckInputs branch January 2, 2020 16:36

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

validation: Rename CheckInputs to CheckInputScripts #16658

validation: Rename CheckInputs to CheckInputScripts #16658

Uh oh!

Conversation

jnewbery commented Aug 19, 2019

Uh oh!

This comment has been minimized.

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

Uh oh!

Conflicts

Uh oh!

fanquake left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Sep 2, 2019

Uh oh!

ajtowns commented Sep 3, 2019

Uh oh!

Uh oh!

jnewbery commented Sep 4, 2019

Uh oh!

michaelfolkson commented Sep 7, 2019

Uh oh!

fanquake commented Sep 8, 2019

Uh oh!

jnewbery commented Sep 8, 2019

Uh oh!

jnewbery commented Sep 10, 2019

Uh oh!

jnewbery commented Oct 1, 2019

Uh oh!

maflcko commented Oct 11, 2019

Uh oh!

jnewbery commented Oct 25, 2019

Uh oh!

maflcko commented Oct 25, 2019

Uh oh!

promag commented Oct 25, 2019

Uh oh!

jnewbery commented Oct 30, 2019

Uh oh!

DrahtBot commented Nov 7, 2019

Uh oh!

maflcko commented Nov 7, 2019

Uh oh!

jnewbery commented Nov 7, 2019

Uh oh!

maflcko commented Nov 7, 2019

Uh oh!

promag commented Nov 7, 2019

Uh oh!

Uh oh!

DrahtBot commented Aug 19, 2019 •

edited

Loading