Update mandatory flags after Magnetic anomaly has activated
AbandonedPublic
Actions

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

Repository

rABC Bitcoin ABC

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 4885
Build 7833: Bitcoin ABC Buildbot (legacy)
Build 7832: arc lint + arc unit

Event Timeline

florian created this revision.Feb 6 2019, 06:59

Owners added a reviewer: Restricted Owners Package.Feb 6 2019, 06:59

Herald added a reviewer: Restricted Project. · View Herald TranscriptFeb 6 2019, 06:59

Herald added a reviewer: deadalnix. · View Herald Transcript

Herald added a subscriber: schancel. · View Herald Transcript

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

florian added a child revision: D2479: Add activation code for SEGWIT_RECOVERY.Feb 6 2019, 07:08

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B4885: Diff 7195.Feb 6 2019, 07:14

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

I will resubmit this diff for review after Feb 15th

florian planned changes to this revision.Feb 6 2019, 15:07

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B4890: Diff 7200.Feb 6 2019, 15:23

florian added inline comments.Feb 6 2019, 16:58

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 a subscriber: markblundeberg.Feb 11 2019, 01:56

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..