Abstract out script execution out of VerifyWitnessProgram() #18002

sipa · 2020-01-25T21:06:38Z

This is a refactoring cherry-picked out of #17977. As it touches consensus code, I don't think this would ordinarily meet the bar for review cost vs benefit. However, it simplifies the changes for Taproot significantly, and if it's going to be necessitated by inclusion of that code, I may as well give it some additional attention by PRing it independently.

sipa · 2020-01-25T21:10:42Z

A change like this was originally suggested by @JeremyRubin here: sipa#116

DrahtBot · 2020-01-26T01:11:25Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

BIP-325: Signet [consensus] #18267 (BIP-325: Signet [consensus] by kallewoof)
Implement BIP 340-342 validation (Schnorr/taproot/tapscript) #17977 ([WIP] Implement BIP 340-342 validation (Schnorr/taproot/tapscript) by sipa)
script: add simple signature support (checker/creator) #16653 (script: add simple signature support (checker/creator) by kallewoof)
Make script interpreter independent from storage type CScript #13062 (Make script interpreter independent from storage type CScript by sipa)

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.

JeremyRubin · 2020-01-26T02:27:20Z

I'm generally a big fan of cleaning up this consensus logic such that the code is "tree-like" and terminates at leaf branches, rather than having interleaved "dag-like" branches that merge into a common exit point. One of the key advantages of code structured in this way is it's much easier to see, at a glance, exactly how many "versions" (i.e., combinations of flags) we support. Otherwise, with N things that could either be on or off you get something like 2**N potential execution traces which makes it really hard to write tests for all flows (coverage is not enough).

It's a code style that I think we should strive to use when possible, and it makes me a bit extreme in that I go as far as to think we should re-write some other consensus logic (see #15969) to be more in line with this style.

I say the above here not to advocate for more expansive changes at this time, but more so that other reviewers can appropriately discount my ACKs here if you disagree with this viewpoint.

That this makes Taproot simpler to review and implement is a plus, but I think these changes stand on their own.

Concept ACK + lite CR-ack, untested.

src/script/interpreter.cpp

NicolasDorier · 2020-01-26T08:04:38Z

Code review ACK.

I like the approach of trying to get small refactors onto the codebase that can be independently reviewed to make the taproot PR more reviewable.

src/script/interpreter.cpp

Empact · 2020-01-30T00:11:09Z

Code Review ACK 1e1e28c

ajtowns

ACK 1e1e28c ; nits only, none of which seem worth fixing

src/script/interpreter.cpp

theStack

ACK 139f7ff

src/script/interpreter.cpp

jnewbery

Code review ACK 139f7ff

A couple of nits inline.

src/script/interpreter.cpp

jonatack

ACK 139f7ff mod comments

src/script/interpreter.cpp

fjahr · 2020-02-12T18:15:51Z

Code review ACK 139f7ff

Verified the changes are a pure refactor. Also ran test locally. I agree with both of @jnewbery s comments but can also be merged as it is right now.

This removes the unclear reliance on "falling through" to get to the script execution part. Also fix some code style issues.

sipa · 2020-02-12T19:38:00Z

I've made a few invasive changes here, which will need re-review.

fjahr · 2020-02-12T20:31:20Z

Re-ACK c8e24dd

New changes addressed the review comments.

jonatack

ACK c8e24dd

src/script/interpreter.cpp

theStack · 2020-02-16T18:47:32Z

re-ACK c8e24dd
Checked that since my previous ACK 139f7ff the following changes have been made:

s/ExecuteWitnessProgram/ExecuteWitnessScript
pass script stack to ExecuteWitnessScript as const_iterator pair (and create local copy)
instead of rvalue reference
use range-based for loop for stack item size check
in success case, just return true instead of the not needed set_success(...)
removed the assertion for unreachable code, put the bottom part in an else-branch instead and intentionally have no return statement at the end of the function (if it is ever reached, the compiler would spit out a warning, indicating a flawed logic above)

Empact · 2020-02-17T20:58:43Z

Code Review Re-ACK c8e24dd

nit: could use cbegin/cend

ajtowns · 2020-02-19T02:34:08Z

ACK c8e24dd

NicolasDorier · 2020-02-19T12:53:47Z

Code Review reACK

jnewbery · 2020-03-13T21:22:20Z

ACK c8e24dd

…ssProgram() c8e24dd [REFACTOR] Abstract out script execution out of VerifyWitnessProgram() (Pieter Wuille) Pull request description: This is a refactoring cherry-picked out of bitcoin#17977. As it touches consensus code, I don't think this would ordinarily meet the bar for review cost vs benefit. However, it simplifies the changes for Taproot significantly, and if it's going to be necessitated by inclusion of that code, I may as well give it some additional attention by PRing it independently. ACKs for top commit: fjahr: Re-ACK c8e24dd theStack: re-ACK bitcoin@c8e24dd Empact: Code Review Re-ACK bitcoin@c8e24dd ajtowns: ACK c8e24dd jnewbery: ACK c8e24dd jonatack: ACK c8e24dd Tree-SHA512: 96c2aa5d2f9c7c802bcc008f5cde55b1dfedfaf42e34101331e6c0d594acdf6437661102dc939718f0877c20451336855dfbaa8aa8f57d9e722a7fa7329e3a46

2b0fcff Make VerifyWitnessProgram use a Span stack (Pieter Wuille) Pull request description: Here is a follow-up to #18002, again with the goal of simplifying (potential) BIP341 code. Instead of passing a begin and end iterator of the initial stack to `ExecuteWitnessScript`, they are turned into a `Span<const valtype>`, representing a span of `valtype`s in memory. This allows `VerifyWitnessProgram` to operate on that span directly, instead of juggling iterators around (which would be exacerbated by #17977 if trying to avoid copying the stack). ACKs for top commit: ajtowns: ACK 2b0fcff elichai: ReACK on the diff 2b0fcff instagibbs: re-ACK 2b0fcff theStack: re-ACK 2b0fcff Empact: ACK 2b0fcff jnewbery: utACK 2b0fcff Tree-SHA512: 38eb4ce17f1947674c1c274caa40feb6ea8266bd96134d9cf1bc41e6fbf1114d4dde6c7a9e26e1ca8f3d0155429ef0911cc8ec0c1037d8fe7d6ec7f9e7184e93

…ssProgram() c8e24dd [REFACTOR] Abstract out script execution out of VerifyWitnessProgram() (Pieter Wuille) Pull request description: This is a refactoring cherry-picked out of bitcoin#17977. As it touches consensus code, I don't think this would ordinarily meet the bar for review cost vs benefit. However, it simplifies the changes for Taproot significantly, and if it's going to be necessitated by inclusion of that code, I may as well give it some additional attention by PRing it independently. ACKs for top commit: fjahr: Re-ACK c8e24dd theStack: re-ACK bitcoin@c8e24dd Empact: Code Review Re-ACK bitcoin@c8e24dd ajtowns: ACK c8e24dd jnewbery: ACK c8e24dd jonatack: ACK c8e24dd Tree-SHA512: 96c2aa5d2f9c7c802bcc008f5cde55b1dfedfaf42e34101331e6c0d594acdf6437661102dc939718f0877c20451336855dfbaa8aa8f57d9e722a7fa7329e3a46

DrahtBot added the Consensus label Jan 25, 2020

fanquake added the Refactoring label Jan 25, 2020

fanquake requested review from ajtowns and JeremyRubin January 26, 2020 01:31

NicolasDorier reviewed Jan 26, 2020

View reviewed changes

src/script/interpreter.cpp Show resolved Hide resolved

Empact reviewed Jan 30, 2020

View reviewed changes

src/script/interpreter.cpp Outdated Show resolved Hide resolved

ajtowns approved these changes Jan 31, 2020

View reviewed changes

src/script/interpreter.cpp Outdated Show resolved Hide resolved

src/script/interpreter.cpp Outdated Show resolved Hide resolved

src/script/interpreter.cpp Outdated Show resolved Hide resolved

sipa force-pushed the 202001_execute_witness branch from 1e1e28c to 139f7ff Compare February 7, 2020 03:25

theStack approved these changes Feb 7, 2020

View reviewed changes

egp reviewed Feb 7, 2020

View reviewed changes

src/script/interpreter.cpp Show resolved Hide resolved

jnewbery reviewed Feb 11, 2020

View reviewed changes

src/script/interpreter.cpp Outdated Show resolved Hide resolved

src/script/interpreter.cpp Outdated Show resolved Hide resolved

jonatack reviewed Feb 12, 2020

View reviewed changes

src/script/interpreter.cpp Outdated Show resolved Hide resolved

src/script/interpreter.cpp Outdated Show resolved Hide resolved

[REFACTOR] Abstract out script execution out of VerifyWitnessProgram()

c8e24dd

This removes the unclear reliance on "falling through" to get to the script execution part. Also fix some code style issues.

sipa force-pushed the 202001_execute_witness branch from 139f7ff to c8e24dd Compare February 12, 2020 19:32

jonatack reviewed Feb 12, 2020

View reviewed changes

src/script/interpreter.cpp Show resolved Hide resolved

DrahtBot mentioned this pull request Mar 5, 2020

BIP-325: Signet [consensus] #18267

Merged

laanwj merged commit e5cb0df into bitcoin:master Mar 13, 2020

sipa mentioned this pull request Mar 19, 2020

Make VerifyWitnessProgram use a Span stack #18388

Merged

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

Abstract out script execution out of VerifyWitnessProgram() #18002

Abstract out script execution out of VerifyWitnessProgram() #18002

Uh oh!

Conversation

sipa commented Jan 25, 2020

Uh oh!

sipa commented Jan 25, 2020

Uh oh!

DrahtBot commented Jan 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

JeremyRubin commented Jan 26, 2020

Uh oh!

Uh oh!

NicolasDorier commented Jan 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Empact commented Jan 30, 2020

Uh oh!

ajtowns left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

fjahr commented Feb 12, 2020

Uh oh!

sipa commented Feb 12, 2020

Uh oh!

fjahr commented Feb 12, 2020

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

theStack commented Feb 16, 2020

Uh oh!

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

Uh oh!

Uh oh!

ajtowns commented Feb 19, 2020

Uh oh!

NicolasDorier commented Feb 19, 2020

Uh oh!

jnewbery commented Mar 13, 2020

Uh oh!

Uh oh!

DrahtBot commented Jan 26, 2020 •

edited

Loading

NicolasDorier commented Jan 26, 2020 •

edited

Loading

Empact commented Feb 17, 2020 •

edited

Loading