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

Abstract out script execution out of VerifyWitnessProgram() #18002

Open
wants to merge 1 commit into
base: master
from

Conversation

@sipa
Copy link
Member

sipa commented Jan 25, 2020

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

This comment has been minimized.

Copy link
Member Author

sipa commented Jan 25, 2020

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

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Jan 26, 2020

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #17977 ([WIP] Implement BIP 340-342 validation (Schnorr/taproot/tapscript) by sipa)
  • #16653 (script: add simple signature support (checker/creator) by kallewoof)
  • #16440 (BIP-322: Generic signed message format by kallewoof)
  • #16411 (BIP-325: Signet support by kallewoof)
  • #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.

@fanquake fanquake requested review from ajtowns and JeremyRubin Jan 26, 2020
@JeremyRubin

This comment has been minimized.

Copy link
Contributor

JeremyRubin commented Jan 26, 2020

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.

@NicolasDorier

This comment has been minimized.

Copy link
Member

NicolasDorier commented Jan 26, 2020

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 Outdated Show resolved Hide resolved
@Empact

This comment has been minimized.

Copy link
Member

Empact commented Jan 30, 2020

Code Review ACK 1e1e28c

Copy link
Contributor

ajtowns left a comment

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

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 sipa force-pushed the sipa:202001_execute_witness branch from 1e1e28c to 139f7ff Feb 7, 2020
Copy link
Contributor

theStack left a comment

ACK 139f7ff

Copy link
Member

jnewbery left a comment

Code review ACK 139f7ff

A couple of nits inline.

src/script/interpreter.cpp Outdated Show resolved Hide resolved
src/script/interpreter.cpp Outdated Show resolved Hide resolved
Copy link
Member

jonatack left a comment

ACK 139f7ff mod comments

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

This comment has been minimized.

Copy link
Contributor

fjahr commented Feb 12, 2020

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 sipa force-pushed the sipa:202001_execute_witness branch from 139f7ff to c8e24dd Feb 12, 2020
@sipa

This comment has been minimized.

Copy link
Member Author

sipa commented Feb 12, 2020

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

@fjahr

This comment has been minimized.

Copy link
Contributor

fjahr commented Feb 12, 2020

Re-ACK c8e24dd

New changes addressed the review comments.

Copy link
Member

jonatack left a comment

ACK c8e24dd

std::vector<valtype> stack{begin, end};

// Disallow stack item size > MAX_SCRIPT_ELEMENT_SIZE in witness stack
for (const valtype& elem : stack) {

This comment has been minimized.

Copy link
@jonatack

jonatack Feb 12, 2020

Member

For my understanding, is there a reason why auto would not be preferred here?

-    for (const valtype& elem : stack) {
+    for (const auto& elem : stack) {
@theStack

This comment has been minimized.

Copy link
Contributor

theStack commented Feb 16, 2020

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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.