This replaces SCRIPT_ALLOW_SEGWIT_RECOVERY with SCRIPT_DISALLOW_SEGWIT_RECOVERY,
which has exactly the opposite meaning.
Functionality is unchanged.
Details
Details
- Reviewers
deadalnix florian markblundeberg Fabien - Group Reviewers
Restricted Project - Maniphest Tasks
- T653: Clean up past upgrades
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
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- segwitrecovery-invert
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 6047 Build 10153: Bitcoin ABC Buildbot (legacy) Build 10152: arc lint + arc unit
Event Timeline
Comment Actions
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.
| src/script/interpreter.cpp | ||
|---|---|---|
| 1641 | Why is this checking for the stack to be empty ? | |
| src/test/data/script_tests.json | ||
| 3408 | There should be tests for the disallow case. Because right now, the flag is completely untested. | |
| src/validation.cpp | ||
| 727 | Make it a standard flag. | |
| 1633 | 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. | |
Comment Actions
@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 | 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 | OK, I can add tests for the disallow case. | |
| src/validation.cpp | ||
| 727 | 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. | |
| src/test/data/script_tests.json | ||
|---|---|---|
| 3267 | @deadalnix This tests the "disabled" case.. | |
| 3286 | And this also test the disabled case. | |
Comment Actions
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.