Paths

Table of Contentst

add SCHNORR_MULTISIG to mandatory flags
ClosedPublic
Actions

Authored by markblundeberg on Dec 9 2019, 23:45.

Details

Reviewers

Mengerian
deadalnix

Group Reviewers

Restricted Project

Maniphest Tasks

T653: Clean up past upgrades

Commits

rSTAGINGc6365a66ae57: add SCHNORR_MULTISIG to mandatory flags
rABCc6365a66ae57: add SCHNORR_MULTISIG to mandatory flags

Summary

Note that this makes pre-activation mempool behaviour slightly weird, as
noted in the test. In practice now that the upgrade has passed and is
checkpointed, only a new node during IBD can be in a preactivated state,
and a new node will anyway not request transactions before finishing IBD.

It may be worth adding tests of fresh-node IBD behaviour in a future
commit; similar behaviour occurs for any additive flag that is included
in mandatory flags.

Test Plan

make check
test_runner.py

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

markblundeberg created this revision.Dec 9 2019, 23:45

Herald added a reviewer: Restricted Project. · View Herald TranscriptDec 9 2019, 23:45

Herald added a reviewer: deadalnix. · View Herald Transcript

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B8462: Diff 14709.Dec 10 2019, 00:00

Mengerian added a task: T653: Clean up past upgrades.Dec 10 2019, 03:46

deadalnix requested changes to this revision.Dec 11 2019, 02:28

deadalnix added inline comments.

test/functional/abc-schnorrmultisig-activation.py
243 ↗	(On Diff #14709)	Can you explain this? This isn't supposed to be benign, as there are various check leading to other messages.

This revision now requires changes to proceed.Dec 11 2019, 02:28

update comment

Build Bitcoin-ABC / Diffs / Diff Testing started.

markblundeberg added inline comments.Dec 11 2019, 13:24

test/functional/abc-schnorrmultisig-activation.py
243 ↗	(On Diff #14709)	Yeah the other checks are done using standard flags \| extraflags, which are determined independently of the block flags. I've updated the comment to expand on why this is fine, but it does still feel weird. Alternatively we could just never put SCRIPT_ENABLE_SCHNORR_MULTISIG into mandatory flags, and just keep it part of extraflags. It is currently the only additive flag so it's the only one with weird behaviour like this.

markblundeberg added inline comments.Dec 11 2019, 13:25

test/functional/abc-schnorrmultisig-activation.py
243 ↗	(On Diff #14709)	(well, OK, replay protection flag is additive too)

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B8500: Diff 14766.Dec 11 2019, 13:38

@markblundeberg you say " It is currently the only additive flag so it's the only one with weird behaviour like this."

This makes me wonder if it would be worth inverting the logic, and having a restrictive flag instead. This could also maybe be combined with refactoring NULLDUMMY flag behavior.

In D4673#113120, @Mengerian wrote:

@markblundeberg you say " It is currently the only additive flag so it's the only one with weird behaviour like this."

This makes me wonder if it would be worth inverting the logic, and having a restrictive flag instead. This could also maybe be combined with refactoring NULLDUMMY flag behavior.

Hmm, I think the same issue will exist. The core of the problem is that standardness rules permit some behaviours that are not valid prior to the upgrade. If it were inverted (so it's only turned on pre-upgrade, and not in standard flags) then the problem would be that the flag is subtractive.

It's possible there are some clever retroactive hacks we can do to the guts of the checkmultisig logic, but this feels a bit too dangerous and it would likely just increase code complexity in the end.

In all other cases of additive flags we have dealt with this problem by making the change retroactively true from genesis to current day. Not possible here.

Also, what is NULLDUMMY? No such flag exists. ;-)

By the way it is possible to have a flag in extraFlags that counts as mandatory, but we need to stop using the over-simple MANDATORY_SCRIPT_VERIFY_FLAGS system and have thing specified more clearly.

Oh actually you know what? FORKID flag is the same -- if you are doing IBD and are pre BCH activation and someone unsolicitedly sends you a BCH sighash transaction, it will create that "BUG! PLEASE REPORT THIS" log.

So if this is fine behaviour for FORKID, then it's fine for multisig flag too.

In D4673#113173, @markblundeberg wrote:

Also, what is NULLDUMMY? No such flag exists. ;-)

Ha, oh yeah, I forgot it's already gone :)

markblundeberg mentioned this in D4679: add SCRIPT_VERIFY_MINIMALDATA to mandatory flags.Dec 12 2019, 09:25

rebase due to landing D4679

In D4673#113198, @markblundeberg wrote:

Oh actually you know what? FORKID flag is the same -- if you are doing IBD and are pre BCH activation and someone unsolicitedly sends you a BCH sighash transaction, it will create that "BUG! PLEASE REPORT THIS" log.

So if this is fine behaviour for FORKID, then it's fine for multisig flag too.

This gave me an idea about how this can be prevented, see D4716.

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B8532: Diff 14813.Dec 12 2019, 22:19

markblundeberg mentioned this in D4642: Cleanup graviton activation.Dec 12 2019, 23:17

deadalnix accepted this revision.Dec 16 2019, 16:11

This revision is now accepted and ready to land.Dec 16 2019, 16:11

Closed by commit rABCc6365a66ae57: add SCHNORR_MULTISIG to mandatory flags (authored by Mark Lundeberg <36528214+markblundeberg@users.noreply.github.com>, committed by Mark Lundeberg <markblundeberg@users.noreply.github.com>). · Explain WhyDec 17 2019, 03:57

This revision was automatically updated to reflect the committed changes.

Mark Lundeberg <markblundeberg@users.noreply.github.com> added a commit: rABCc6365a66ae57: add SCHNORR_MULTISIG to mandatory flags.

Mark Lundeberg <markblundeberg@users.noreply.github.com> added a commit: rSTAGINGc6365a66ae57: add SCHNORR_MULTISIG to mandatory flags.Apr 7 2020, 22:05