Page MenuHomePhabricator

VerifyScript : return success from only one location
Needs RevisionPublic

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


Group Reviewers
Restricted Project

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

rABC Bitcoin ABC
Lint OK
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:

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.


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