Page MenuHomePhabricator

add SCHNORR_MULTISIG to mandatory flags
ClosedPublic

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
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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; 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
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

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)

Mengerian added a comment.EditedDec 11 2019, 19:33

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

markblundeberg added a comment.EditedDec 11 2019, 21:42

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

markblundeberg added a comment.EditedDec 11 2019, 22:41

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.

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

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

rebase due to landing D4679

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.

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