Page MenuHomePhabricator

Make STANDARD_SCRIPT_VERIFY_FLAGS explicit
Needs RevisionPublic

Authored by markblundeberg on Feb 8 2020, 06:49.

Details

Reviewers
jasonbcox
deadalnix
Group Reviewers
Restricted Project
Summary

Currently STANDARD_SCRIPT_VERIFY_FLAGS is based on
MANDATORY_SCRIPT_VERIFY_FLAGS which doesn't really make
sense as that flag set has to do with banning behaviour.
It's better to be explicit about all the most strict rules
that transactions need to be relayable for the sake of tests
and wallet.

This helps clear up some possible misunderstandings.
For example, SCRIPT_VERIFY_MINIMALDATA was included in the
definition of STANDARD_SCRIPT_VERIFY_FLAGS but it was actually
in MANDATORY_SCRIPT_VERIFY_FLAGS too, which may have given the
wrong impression about the fact that minimaldata violations
are banworthy. (oversight from D4679, when mandatory flags
were defined in another file)

There is no change in the value of the flagset here, it
is 0x7547EF both before and after this Diff.

Depends on D5220

Test Plan
  • ninja check-all
  • Add a static_assert somewhere that STANDARD_SCRIPT_VERIFY_FLAGS == 0x7547EF , and test it both before and after this Diff.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
stdexplicit
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 9354
Build 16640: Default Diff Build & Tests
Build 16639: arc lint + arc unit

Event Timeline

markblundeberg created this revision.Feb 8 2020, 06:49
Herald added a reviewer: Restricted Project. · View Herald TranscriptFeb 8 2020, 06:49
jasonbcox accepted this revision.Feb 8 2020, 20:56
This revision is now accepted and ready to land.Feb 8 2020, 20:56
deadalnix requested changes to this revision.Feb 9 2020, 03:27
deadalnix added a subscriber: deadalnix.

Wait, what did happen here?

Maintaining a bunch of lists this way is 100% guaranteed to fail at some point. Isn't it expected that any flag that is mandatory is also a standard flag, module activation, which are already handled in their own codepath (BTW, these codepath are already showing sign of duplication, which is a sign that design need some work)?

This patch change the code in a way where errors are confusing, to another way, where error may be easier to spot, but also translate in security holes. This is the completely wrong tradeof.

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