Page MenuHomePhabricator

Invert meaning of Segwit Recovery flag.

Authored by Mengerian on May 29 2019, 04:23.


Group Reviewers
Restricted Project
Maniphest Tasks
T653: Clean up past upgrades

which has exactly the opposite meaning.

Functionality is unchanged.

Test Plan

make check
Did IBD on main net and testnet with checkpoints=0 -assumevalid=0.

Functional tests are unchanged, as this Diff is not intended to change
any functional behavior.

Diff Detail

rABC Bitcoin ABC
Lint OK
No Unit Test Coverage
Build Status
Buildable 6072
Build 10193: Bitcoin ABC Buildbot (legacy)
Build 10192: arc lint + arc unit

Event Timeline

Mengerian created this revision.May 29 2019, 04:23
Herald added a reviewer: Restricted Project. · View Herald TranscriptMay 29 2019, 04:23

Looks good, indeed this appears to invert the flag without changing behaviour. However, just to be sure I'd quickly follow it up with a Diff that makes it an ordinary Standard flag.

deadalnix requested changes to this revision.May 29 2019, 15:12
deadalnix added inline comments.
1641 ↗(On Diff #8983)

Why is this checking for the stack to be empty ?

3408 ↗(On Diff #8983)

There should be tests for the disallow case. Because right now, the flag is completely untested.

727 ↗(On Diff #8983)

Make it a standard flag.

1633 ↗(On Diff #8983)

The whole point is to simplify, not to add new codepath. And incidentally, if you add new codepath, there should be new tests.

It's more robust to enable the flag no matter what, and then disable it when the fork is activated.

This revision now requires changes to proceed.May 29 2019, 15:12
markblundeberg added inline comments.May 29 2019, 15:31
1641 ↗(On Diff #8983)

This is a consensus rule now. Do not change this.

1641 ↗(On Diff #8983)

(the stack.empty I mean)

@deadalnix I am trying to follow the step you outlined here:

The idea of this Diff is to change the meaning of the flag without changing the functional behavior. This helps to avoid making mistakes, since the functional test stays the same. Then the next Diff can change behavior with a very small change that is easier to check for correctness.

If I remove activation and add to Standard now, that will change how the code behaves, and require also changing the functional test.

1641 ↗(On Diff #8983)

IDK, but my understanding is that changing that would break consensus.

It would mean you could do segwit recoveries with extra unnecessary pushes, which new ABC would accept and old ABC would reject.

3408 ↗(On Diff #8983)

OK, I can add tests for the disallow case.

727 ↗(On Diff #8983)

The issue with this is that adding it to standard flags would change the behavior of the code.

The goal of this Diff is to change the meaning of the flag without changing behavior.

Mengerian added inline comments.May 29 2019, 19:42
3267 ↗(On Diff #8983)

@deadalnix This tests the "disabled" case..

3286 ↗(On Diff #8983)

And this also test the disabled case.

Mengerian updated this revision to Diff 9016.May 30 2019, 17:37

Add unit tests for "0 left on stack" case with segwit recovery disallowed.
Clarify descriptions of script tests so that it's easier to see tests with the flag enabled.

Mengerian abandoned this revision.Jun 5 2019, 05:06

Abandon in favor of D3194

Abandon in favor of D3194