Make more script validation flags backward compatible
ClosedPublic
Actions

Authored by markblundeberg on Dec 11 2019, 23:06.

Details

Reviewers

deadalnix

Group Reviewers

Restricted Project

Commits

rSTAGING1b3100b9ed90: Make more script validation flags backward compatible
rABC1b3100b9ed90: Make more script validation flags backward compatible

Summary

This is a backport of PR10699 ( https://github.com/bitcoin/bitcoin/pull/10699/files ),
but has to be adapted slightly since we don't just do softforking flags.

Pull request description:

This change makes `SCRIPT_VERIFY_UPGRADABLE_NOPS` not apply to `OP_CHECKLOCKTIMEVERIFY` and `OP_CHECKSEQUENCEVERIFY`. This is a no-op as `UPGRADABLE_NOPS` is only set for mempool transactions, and those always have `SCRIPT_VERIFY_CHECKLOCKTIMEVERIFY` and `SCRIPT_VERIFY_CHECKSEQUENCEVERIFY` set as well. The advantage is that setting more flags now always results in a reduction in acceptable scripts (=softfork).

This results in a nice and testable property for validation, for which a new test is added.

This also means that the introduction of a new definition for a NOP or witness version will likely need the following procedure (example OP_NOP8 here)
* Remove OP_NOP8 from being affected by `SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_NOPS`.
* Add a `SCRIPT_VERIFY_DISCOURAGE_NOP8`, which only applies to `OP_NOP8`.
* Add a `SCRIPT_VERIFY_NOP8` which implements the new consensus logic.
* Before activation, add `SCRIPT_VERIFY_DISCOURAGE_NOP8` to the mempool flags.
* After activation, add `SCRIPT_VERIFY_NOP8` to both the mempool and consensus flags.

Test Plan

make check
test_runner.py

Diff Detail

Repository

rABC Bitcoin ABC

Branch

softflags

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 8514
Build 15027: Default Diff Build & Tests
Build 15026: arc lint + arc unit

Event Timeline

markblundeberg created this revision.Dec 11 2019, 23:06

Herald added a reviewer: Restricted Project. · View Herald TranscriptDec 11 2019, 23:06

Herald added a reviewer: deadalnix. · View Herald Transcript

Build Bitcoin-ABC / Diffs / Diff Testing started.

The first time I saw this PR I marked it as 'skip' in Jason's repo, but having come across it again recently, I think it has value even though we don't just do soft forks. The new discourage_upgradeable_nops is cleaner, and the new test is potentially valuable for catching some script logic bugs.

Fully remove a stale comment (this was not removed in Core ever, but
really should have been...)

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B8514: Diff 14788.Dec 11 2019, 23:21

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B8515: Diff 14789.Dec 11 2019, 23:29

markblundeberg planned changes to this revision.Dec 13 2019, 04:11

markblundeberg added inline comments.

src/test/script_tests.cpp
194 ↗	(On Diff #14789)	hmm, I just realized this would be simpler by having these flags masked out of `extra_flags`

tweak enable_flags freezing slightly; also changed my mind about removing
the stale comment, it can be removed in a later (non-backport) diff.

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B8538: Diff 14823.Dec 13 2019, 04:50

One thing I really like about this test: when we add new script flags and script tests, it will hopefully warn the implementor when they are making an additive flag, and make them decide whether they want to make their flag strictly soft forking. Perhaps this requires an assert(other_flags), like the cleanstack thing.

If the new flag is strictly soft forking, then the flag can be named SCRIPT_VERIFY_xxx; otherwise this sends a strong signal that the new additive flag should be called SCRIPT_ENABLE_xxx and they will need to add it into this test as a special exception frozen flag.

Note, it sometimes also occurs that strictly additive flags are added (for example, assigning meaning to an invalid opcode). For strictly additive flags, there is a similar flag transformation that can be done, but we don't have any examples right now since they are short lived (we always end up making them retroactively always on, after activation is past).

deadalnix accepted this revision.Dec 16 2019, 13:33

This revision is now accepted and ready to land.Dec 16 2019, 13:33

Closed by commit rABC1b3100b9ed90: Make more script validation flags backward compatible (authored by Mark Lundeberg <markblundeberg@users.noreply.github.com>). · Explain WhyDec 17 2019, 03:28

This revision was automatically updated to reflect the committed changes.

Mark Lundeberg <markblundeberg@users.noreply.github.com> added a commit: rABC1b3100b9ed90: Make more script validation flags backward compatible.

Mark Lundeberg <markblundeberg@users.noreply.github.com> added a commit: rSTAGING1b3100b9ed90: Make more script validation flags backward compatible.Apr 7 2020, 22:05