Page MenuHomePhabricator

remove MANDATORY_SCRIPT_VERIFY_FLAGS in favour of explicit forgiveness flags
Needs RevisionPublic

Authored by markblundeberg on Sat, Feb 8, 06:55.

Details

Reviewers
jasonbcox
deadalnix
Group Reviewers
Restricted Project
Summary

After D5221, MANDATORY_SCRIPT_VERIFY_FLAGS is only
used for calculating STANDARD_NOT_MANDATORY_VERIFY_FLAGS,
nothing else. And really the important flag set we care
about for banning is STANDARD_NOT_MANDATORY_VERIFY_FLAGS,
that describes the set of flags which must be removed from
GetStandardScriptFlags in order to determine whether to ban
a peer or not. It's also only indirectly related to
STANDARD_SCRIPT_VERIFY_FLAGS so it doesn't really make
sense to base it on that either.

Note that this exposes exactly what flags we have forgotten
to make mandatory, such as SCRIPT_VERIFY_DERSIG which has
been a consensus rule since ages now. It's not a big deal
since STRICTENC basically includes DERSIG, but still it's
a bit weird that it was missing.

There is no change in the value of these flags here,
they are STANDARD_NOT_MANDATORY_VERIFY_FLAGS == 0x5407A4
both before and after.

Depends on D5221

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

Diff Detail

Repository
rABC Bitcoin ABC
Branch
mandflags
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 9355
Build 16642: Bitcoin ABC Buildbot
Build 16641: arc lint + arc unit

Event Timeline

markblundeberg created this revision.Sat, Feb 8, 06:55
Herald added a reviewer: Restricted Project. · View Herald TranscriptSat, Feb 8, 06:55
jasonbcox accepted this revision.Sat, Feb 8, 20:55
jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
src/policy/policy.h
102

Nit: For what it's worth, I still find this naming confusing. I need to jump back and forth between this and it's only usage in CheckInputs() to understand what it's used for despite the comment (the comment doesn't mention how it's used other than "influences", which IMO isn't useful).

This revision is now accepted and ready to land.Sat, Feb 8, 20:55
markblundeberg added inline comments.Sun, Feb 9, 01:02
src/policy/policy.h
102

yes I was thinking of renaming it as well, just couldn't think of a good name yet

deadalnix requested changes to this revision.Sun, Feb 9, 03:30
deadalnix added a subscriber: deadalnix.
deadalnix added inline comments.
src/policy/policy.h
107

This include a good amount of duplication of infos with the previous definition. This duplication WILL get out of sync. This makes the software more fragile.

This revision now requires changes to proceed.Sun, Feb 9, 03:30