Having a separate loop here to check pubkey encoding results is strange behavior: It would check all pubkeys for successful Schnorr multisigs, but not for the "0-of-N" or "false" cases. (Or for the legacy case).

Two potential alternatives are:

1. Check pubkey encoding only when it has a corresponding signature (ie, when the checkbit is "1"), and don't check the others. I can't think of any reason not to do this. It is similar to the current multisig operation, and seems to have minimal side effects. It would render some previously unspendable coins become spendable (when unused pubkeys encoding was sometimes checked due to how legacy multisig works).

2. Move the loop that checks pubkey encoding to prior to the dummy element "mode selection" check. This would make it apply to both the legacy case (including 0-of-N and "false") as well as the new Schnorr mode. This disadvantage of this is that it would affect already existing coins, rendering them unspendable if they have invalidly encoded pubkeys. I'm not sure if this is common or not, so it's possible it's not a major problem.

A separate loop to validate pubkey encoding would likely mean a change in behaviour -- all pubkeys must be correctly encoded. Is that intended?

When the predicate here is false, we aren't bailing. There are still statements that need to be executed.

I agree, any suggestions?

It doesn't check for 0 of n anyways and what is the false case ? If there is a discrepancy in behavior, this can make a good test case. I can't derive one from that comment.

Execute them before and them continue.

Thinking about it, it's a nullfail or a nulldummy error, not really a new kind of error.

The appropriate test cases are in place. If you want to arc patch this Diff and parents, and try the modifications you suggest, you can see how it affects the test cases.

They can't be executed before since they modify values used in the body of this if-statement. It is possible to rewrite this to use if(!x) continue; but it just gets more verbose.

Indeed, that's exactly what I had in a prior version of this Diff. :-D I'm happy to revert if that's desirable.

Todo: also add a test for the M=0 case for both legacy & new mode.

Note these two lines are identical to the loop below, which you reviewed in D3675. It was clear enough then, has something changed?

The loop order starts on the top of the stack and moves deeper. The least significant bit of the initial checkbits corresponds to the top public key (the last public key pushed on stack).

just nullfail

Ah, I forgot to change this comment - should just read NEW MULTISIG (SCHNORR)

NULLFAIL

oops, this & ~SCRIPT_VERIFY_MINIMALDATA isn't needed here.

What's "Uxc'loo"? Also Antony pointed out that "first" / "last" is a bit vague since it could be interpreted as "first pushed" or "first popped".

Also the text says 011 but the test does 110

ditto on bitfield

ditto on first/last

ditto on bitfield, first/last

I just realized there is an important missing test case: run a new mode multisig with non-null bitfield but put in an ECDSA signature. (Error -> SIG_NONSCHNORR)

tests.push_back(TestBuilder(CScript() << OP_1 << ToByteVector(keys.pubkey0)
                                      << OP_1 << OP_CHECKMULTISIG,
                            "CHECKMULTISIG 1-of-1 ECDSA signature in Schnorr mode",
                            newmultisigflags)
                    .Num(0b1)
                    .PushSigECDSA(keys.key0).SetScriptError(ScriptError::SIG_NONSCHNORR));
tests.push_back(TestBuilder(CScript() << OP_3 << ToByteVector(keys.pubkey0C)
                                      << ToByteVector(keys.pubkey1C)
                                      << ToByteVector(keys.pubkey2C) << OP_3
                                      << OP_CHECKMULTISIG,
                            "CHECKMULTISIG 3-of-3 Schnorr with mixed-in ECDSA signature",
                            newmultisigflags)
                    .Num(0b111)
                    .PushSigECDSA(keys.key0)
                    .PushSigSchnorr(keys.key1)
                    .PushSigSchnorr(keys.key2).SetScriptError(ScriptError::SIG_NONSCHNORR));

bit -> bits