Page MenuHomePhabricator

VerifyScript : return success from only one location
Needs RevisionPublic

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 OK
Unit
No Unit Test Coverage
Build Status
Buildable 9144
Build 16246: Bitcoin ABC Buildbot
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
deadalnix requested changes to this revision.Jan 27 2020, 01:44
deadalnix added a subscriber: deadalnix.

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 ?

This revision now requires changes to proceed.Jan 27 2020, 01:44