VerifyScript : return success from only one location
Needs RevisionPublic
Actions

Authored by markblundeberg on Jan 26 2020, 14:30.

Details

Reviewers

deadalnix

Group Reviewers

Restricted Project

Summary

Returning success from two separate locations and implicitly bypassing
a bunch of checks seems to be asking for trouble -- it's better if we are
explicit about what is being skipped. This also lets us consolidate the
metrics-related final actions to avoid code duplication.

(this Diff is meant to be an exact transformation)

Test Plan

ninja check

Diff Detail

Repository

rABC Bitcoin ABC

Branch

verifyscript_singlesuccess

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 9144
Build 16246: Default Diff Build & Tests
Build 16245: arc lint + arc unit

Event Timeline

markblundeberg created this revision.Jan 26 2020, 14:30

Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 26 2020, 14:30

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B9144: Diff 15794.Jan 26 2020, 14:34

markblundeberg added a child revision: D5071: remove segwit recovery exemption from SCRIPT_VERIFY_INPUT_SIGCHECKS check.Jan 26 2020, 14:40

While I agree with you n the problem, the solution is not really solving anything because it adds a condition to every single codepath after the fact. You are effectively adding 3 codepath while removing 2 here which is not a very clear win.

Early exit is generally not a problem, unless there is something you need to do at every exit. In which case you want to leverage RAII. I would suggest using something like scope exit: https://www.boost.org/doc/libs/1_64_0/libs/scope_exit/doc/html/index.html

You can check if the return code is a success and if it is, do the bookkeeping you need to do in case of success.

src/script/interpreter.cpp
1728	Can you avoid the double negative problem? It's really not clear what `segwit_exempt` means here, but it is clearly defining a negative, and then you check `!segwit_exempt` all over the place. Double negative are notoriously confusing and an endless source of bugs. It seems to me like this is true when the script is a segwit program, so why not `is_segwit_program` ?