Page MenuHomePhabricator

Update mandatory flags after Magnetic anomaly has activated
AbandonedPublic

Authored by florian on Feb 6 2019, 06:59.

Details

Reviewers
deadalnix
Mengerian
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Maniphest Tasks
T653: Clean up past upgrades
Summary

Non-CLEANSTACK and non-SIGPUSHONLY transactions are currently being accepted into the mempool when standardness checks are disabled with -promiscuousmempoolflags. This diff updates the Mandatory script flags to avoid this issue. Updated abc-mempool-accept-txn.py to test the current state of the mempool acceptance rules when standardness checks are disabled.

Test Plan

test_runner abc-mempool-accept-txn

Diff Detail

Event Timeline

Owners added a reviewer: Restricted Owners Package.Feb 6 2019, 06:59
deadalnix requested changes to this revision.Feb 6 2019, 14:30

While this is good to do per se, I'd rather not do that when everybody is trying to get the features for the May's upgrade in place. Please request review for this after Feb, 15.

src/test/checkdatasig_tests.cpp
71

You should change flagset instead.

src/test/sigopcount_tests.cpp
33

This lead me to think that CHECKDATASIG is not a standard flag either at the moment. This sounds like a logical first step before making it mandatory.

This revision now requires changes to proceed.Feb 6 2019, 14:30
florian marked an inline comment as done.

I will resubmit this diff for review after Feb 15th

florian planned changes to this revision.Feb 6 2019, 15:07
src/test/sigopcount_tests.cpp
33

At least regarding mempool acceptance, CHECKDATASIG is standard because of the activation code there that forces this flag. Making it mandatory automatically sets it in the standard flag. Is there any practical reasoning for breaking it in two steps?

Mengerian requested changes to this revision.Feb 6 2019, 19:40
Mengerian added a subscriber: Mengerian.
Mengerian added inline comments.
src/script/standard.h
37 ↗(On Diff #7200)

While you're in here, it seems this comment should be updated to remove the parts about P2SH, etc.

47 ↗(On Diff #7200)

Following on from Amaury's comments, it seems the SCRIPT_ENABLE_CHECKDATASIG is out of place here. It is not a "VERIFY" flag, it's an "ENABLE" flag... Ie, it permits previously disallowed transactions. All the other flags do the opposite.

So I don't think it should be added here.

markblundeberg added inline comments.
src/script/standard.h
47 ↗(On Diff #7200)

We were discussing this off-phab a bit today. I think it's good to include here since MANDATORY_SCRIPT_VERIFY_FLAGS has the function of deciding whether or not to ban a peer, and having it in the ATMP-activated flags means you will sometimes ban peers who send CDS transactions.

Namely if you're doing IBD and still pre-magnetic anomaly, you don't want to be banning peers who send a CDS transaction to you..