Page MenuHomePhabricator

add SCHNORR_MULTISIG to mandatory flags
ClosedPublic

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

Details

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

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

test/functional/abc-schnorrmultisig-activation.py
243 ↗(On Diff #14709)

(well, OK, replay protection flag is additive too)

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

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

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

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.

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