Page MenuHomePhabricator

clean up STANDARD_NOT_MANDATORY_VERIFY_FLAGS
Needs RevisionPublic

Authored by markblundeberg on Feb 8 2020, 07:14.

Details

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

We don't need DERSIG forgivness (STRICTENC implies it anyway
so DERSIG violations do get banned) nor CHECKDATASIG_SIGOPS
(irrelevant to CheckInputs) in there. Remove them just as a
quick cleanup.

TODO in future diffs: make SIGPUSHONLY / CLEANSTACK / CLTV / CSV mandatory too
(requires tests).

Depends on D5222

Test Plan

ninja check-all

Diff Detail

Repository
rABC Bitcoin ABC
Branch
mandflags
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 9356
Build 16644: Default Diff Build & Tests
Build 16643: arc lint + arc unit

Event Timeline

markblundeberg created this revision.Feb 8 2020, 07:14
Herald added a reviewer: Restricted Project. · View Herald TranscriptFeb 8 2020, 07:14
deadalnix requested changes to this revision.Feb 9 2020, 03:42
deadalnix added a subscriber: deadalnix.

I get what you want to do with this whole stack, and this is not a bad idea per, but you need to rework the approach so that flagset getting out of sync is less of a problem.

This revision now requires changes to proceed.Feb 9 2020, 03:42

I get what you want to do with this whole stack, and this is not a bad idea per, but you need to rework the approach so that flagset getting out of sync is less of a problem.

Yeah roughly speaking these are the bases we need to cover:

  • GetStandardScriptFlags is the "base reality" since it's what gets into mempool.
  • Some code can't call GetStandardScriptFlags so it needs STANDARD_SCRIPT_VERIFY_FLAGS to be fairly accurate.
  • For what doesn't get into mempool, there is the (quite secondary) question of what gets a peer to be banned, i.e., the question of which flags should be added/removed when we do the mandatory check.
  • GetStandardScriptFlags may be missing flags that are contained in STANDARD_SCRIPT_VERIFY_FLAGS.
    • Example: SCRIPT_VERIFY_INPUT_SIGCHECKS and any other soft forking change we might do in future.
    • These flags may or may not need to be included in the nonmandatory check.
  • GetStandardScriptFlags may contain flags that are /not/ in STANDARD_SCRIPT_VERIFY_FLAGS.
    • Example: SCRIPT_ENABLE_REPLAY_PROTECTION, and any additive change we might do (such as SCRIPT_ENABLE_REVERSEBYTES).
    • These flags may or may not need to be included in the nonmandatory check.

One way to deal with this is to make everything use GetStandardScriptFlags instead of STANDARD_SCRIPT_VERIFY_FLAGS. However, a quick grep for STANDARD_SCRIPT_VERIFY_FLAGS shows this isn't quite realistic (particularly for tests).

If we keep STANDARD_SCRIPT_VERIFY_FLAGS, one way to keep perfect sync would be to add a unit test that STANDARD_SCRIPT_VERIFY_FLAGS actually agrees with GetStandardScriptFlags (with explicit modifications noted in the test) when called preupgrade / postupgrade. This can be bolstered by functional tests of course.

As for STANDARD_NOT_MANDATORY_VERIFY_FLAGS that is best probed using functional tests since it's only modifying details of p2p behaviour.