Page MenuHomePhabricator

Invert meaning of Segwit Recovery flag.
AbandonedPublic

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

Details

Reviewers
deadalnix
florian
markblundeberg
Fabien
Group Reviewers
Restricted Project
Maniphest Tasks
T653: Clean up past upgrades
Summary

This replaces SCRIPT_ALLOW_SEGWIT_RECOVERY with SCRIPT_DISALLOW_SEGWIT_RECOVERY,
which has exactly the opposite meaning.

Functionality is unchanged.

Test Plan

make check
./test/functional/test_runner.py
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

Repository
rABC Bitcoin ABC
Branch
sr-1
Lint
Lint OK
Unit
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.
src/script/interpreter.cpp
1641 ↗(On Diff #8983)

Why is this checking for the stack to be empty ?

src/test/data/script_tests.json
3408 ↗(On Diff #8983)

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

src/validation.cpp
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
src/script/interpreter.cpp
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: https://reviews.bitcoinabc.org/D3065#72372

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.

src/script/interpreter.cpp
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.

src/test/data/script_tests.json
3408 ↗(On Diff #8983)

OK, I can add tests for the disallow case.

src/validation.cpp
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
src/test/data/script_tests.json
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