bip322: (another) significant overhaul

[apoelstra]

Changes the validation rules to always require (script) standardness checks, except for checking NOPs and version numbers, where incorrect values result in "inconclusive" rather than "invalid". Allow validators to also return "inconclusive" when they encounter a script that they're unable to interpret.

This makes it possible to write a BIP-322 validator using Miniscript, and to write (say) a p2pkwh-only validator while remaining within spec. Both should encourage adoption.

Also removed the to_spend transaction from the wire serialization because it is a pure function of the address and message.

Mailing list discussion: https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2020-December/thread.html

[kallewoof]

Suggested change

	where <code>message_hash</code> is a BIP340-tagged hash of the message, i.e. sha256_tag(m), where tag = <code>BIP0322-signed-message</code>, and <code>message_challenge</code> is the to be proven (public) key script.
	where <code>message_hash</code> is a BIP340-tagged hash of the message, i.e. sha256_tag(m), where tag = <code>"BIP0322-signed-message"</code>, and <code>message_challenge</code> is the to be proven (public) key script.

(It's a string literal, so I think quotes are good)

[kallewoof]

Suggested change

	## <code>LOW_S</code>, <code>STRICTENC</code> and <code>NULLFAIL</code>: valid ECDSA signatures must be strictly DER-encoded and have a low-S value; invalid ECDSA signature must be the empty push
	## <code>LOW_S</code>, <code>STRICTENC</code> and <code>NULLFAIL</code>: valid ECDSA signatures must be strictly DER-encoded and have a low-S value; invalid ECDSA signatures must be the empty push

[dgpv]

The proposal requires the validator to be able to detect violations of standardness flags (MINIMALDATA, STRICTENC, NULLFAIL, etc). I thought that validators will be able to do verification by using bitcoinconsensus library. The problem is that the library explicitly disallows any standardness flags to be passed to it, only the flags related to consensus is allowed.

While this issue is not directly related to the BIP essense, I think it makes implementation significantly harder, because now the implementors will need to either:

1. Have their own script interpreter, that may happen to differ with the one from Core;
2. Pluck script interpreter code out of Core into their own project, and not use libbitcoinconsensus (like libbitcoin-consensus does);
3. Have their own interpreter, but cross-check with libbitcoinconsensus for consensus violations (still might differ from Core in standardness-related code);
4. Use Core via rpc

I think all these options may happen to be unsatisfactory for a standalone project due to added maintenance or dependency burden.

The inclusion of these checks into this BIP says that probably these rules are significant, not likely to change in the future, and it would be good to have an access to checking them with the code from Core, but without depending on whole bitcoind

Maybe there should be an API to do standardness checks in libbitcoinconsensus, just as a different API function.

Related Core issue: bitcoin/bitcoin#5779 and commit: bitcoin/bitcoin@5ca8ef2

[sipa]

@dgpv I think this is exactly why @apoelstra is suggesting to permit validators to only implement a subset of script, and report inconclusive for others. This means a much smaller scope reimplementation is possible.

You do have a good point though, that the inclusion of these rules in the BIP make them sort of canonical. Perhaps there is a way to see it differently. It is not so much that such scripts are not relayed on the network, or even that they're intended for future extensions that matters - it is the fact that they are all effectively "anyone can spend" (as far as the verifier knows) - and thus a signature can't really have any meaning.

Could the rule just be that any script that doesn't perform any signature check at all, is "inconclusive"? That's very concrete, not tied to policy on the network, and perhaps easier to have exported from libbitcoinconsensus too as it doesn't incur the risk of making people building things that depend on the network's policy rules.

[apoelstra]

I agree that these checks are difficult to implement, but without them every BIP322 signature will be extremely malleable.

[apoelstra]

But I also agree that these rules are "significant", in the sense that in a perfect world they would have always been part of consensus, I can't imagine any reason they would ever be relaxed (indeed, in Taproot I think they have all made it into consensus), and I wish that libbitcoinconsensus exposed a way to enforce them. But that discussion would be much wider scope than this BIP..

[dgpv]

I agree that these checks are difficult to implement, but without them every BIP322 signature will be extremely malleable.

They are difficult to implement and at the same time required by the spec ("If any of the above steps failed, the validator should stop and output the ''invalid'' state."), making it difficult for implementation to conform to the spec at all.

[apoelstra]

Well, for what it's worth, these rules are very easy to enforce for the common types of signatures that incomplete validators are likely to template-match on: for p2pkwh signatures it requires your scriptSig be empty and witness stack consist of a pubkey and valid signature and nothing else, etc.

So ironically, I think deliberately incomplete implementations will have an okay time of this, while people who try to implement complete validators will not.

Semi-relatedly, I just realized that I dropped NULLDUMMY (extra CHECKMULTISIG stack item needs to be the empty push).

[apoelstra]

I wonder if, assuming this PR is accepted, Core would accept a PR to add a flag to libbitcoinconsensus specifically for BIP322 validation.

[dgpv]

a flag to libbitcoinconsensus specifically for BIP322 validation.

or a separate function, verify_script_bip322 or the like, that could also take a version of bip322 as a paremeter, and set the flags for VerifyScript accordingly. This way, if the bip would ever be updated with new required flags, the version of the bip will need to be bumped and the verify_script_bip322 function would recognize the new version and would choose the right combination of flags

[dgpv]

That assumes that the "version of the combination of flags" would be the part of the bip, of course. There might not ever be the need for this, but what if there are ? What if the need to enforce certain new flag will emerge ?

Of course such version wouldn't be needed if the caller could just specify their combination of flags, and then would update their code after new flags are included in the BIP

[dgpv]

What I wanted to say, if the "bip322-related" flags are bundled inside libbitcoinconsensus via a single "bip322" flag or inside a "bip322-specific" function, there could be a potential problem between the software that expects to verify with newer set of flags, and the library that bundles the old set of flags. Of course libbitcoinconsensus has an api version, but is the single added flag a good reason to bump it ? So either the flags shouldn't be bundled, or the bundling itself need to be versioned.

(please excuse the slightly off-topic discussion, but since it may influence the BIP itself (adding version/revision ?), it may be relevant)

[apoelstra]

Agreed on all counts.

I think a "reference implementation" of this BIP should include (a) a patch to Bitcoin Core which adds sign/verify RPCs; (b) a patch to libbitcoinconsensus to add a verify_script_322 method; (c) a small C program which uses libbitcoinconsensus to sign and verify a message.

[kallewoof]

Sounds good to me!

[kallewoof]

@luke-jr This is good to merge. It's been discussed on the ML and comments have been addressed.

[harding]

Partial review (ran out of time).

[harding]

This recommendation seems to needlessly create compatibility issues similar to those in @kallewoof's original PR for generic signmessage: bitcoin/bitcoin#16440 (review)

If the default is to create legacy proofs for P2PKH, then it's possible to implement BIP322 as a simple backwards-compatible extension to existing interfaces like Bitcoin Core's signmessage RPC and GUI window. But, if devs adopt the recommendation here to default to using the new proof format, then they'll need to deprecate the old RPC default for P2PKH output and add a checkbox to the GUI for legacy vs new (like is done for selecting between legacy/bech32 addresses).

[Edited to correct attribution]

[kallerosenbaum]

Wrong Kalle I guess. But always flattering to be mistaken for @kallewoof

[apoelstra]

Yeah I think this is reasonable.

My personal preference would be to deprecate the old base64 signmessage format but it's needlessly breaking to recommend people shift away from it for pkh addresses (which I'd also like to see go away :))

[kallewoof]

FWIW, I chose to be backwards compatible, but later several people suggested we should recommend using the new format so we can sunset the old one at some point.

[harding]

@kallewoof legacy format will be automatically sunset when almost nobody uses P2PKH any more. At that point, it won't be disruptive to deprecate legacy proof generation and verification code. Until then, I think BIP322 needs all the support it can get and maintaining backwards compatibility with the tools people already use is one way to attract support.

[harding]

A couple comments and some nits.

[harding]

Nit: I think I'd find this document easier to follow if the Simple section followed the Full (proof of funds) section. It's not possible to understand what Simple is doing until you've learned about the fields described in Full.

[harding]

Nit: this is the only case in the document where the format "BIP-nnn" is used. In the rest of the paragraph text, "BIPnnn" is used.

[harding]

Nit not introduced in this patch: 2/3 items in this list are introduced by "where". It would be better to use where either 1/3 or 3/3 times.

[harding]

Is it actually supposed to say "at time T and age S" if no locktime was used? E.g., if the case of nLockTime = 0; nSequence = 0xffffffff, is it supposed to say "valid at time block 0 and age same block as parent"?

[harding]

Also, I just realized that the default here is for nSequence to be 0 in both psuedo-transactions instead of the usual wallet default for nSequence to be set to its IsFinal() value of 0xffffffff. Since version >= 2 is required to get BIP68 behavior, that seems fine, but I don't know if it could cause a problem for any PSBT signers (e.g. my Trezor model T warns me about the non-zero nLockTimes created by Bitcoin Core's anti-fee sniping; I don't know how it handles nSequences below 0xfffffffe).

[apoelstra]

Ah! Yes, I forgot that nSequence has non-timelock values. I will update the text to reflect this.

[kallewoof]

@apoelstra Did you get a chance to address feedback on this? Would love to get this merged so we avoid multiple specifications for too long (PR vs master).

[apoelstra]

I did not but I'll try to open a followup PR in the next week.

[sanket1729]

I think this needs the following note:
"In case of p2sh wrapped segwit scripts, the validator should compute the scriptsig based on witness script"

[sanket1729]

This would not work for covenants because fixed validation rules like input spent must be at index 0 etc.

All we really care about is that we are spending the input, we don't really care about the position of the input and output. This is futuristic but worth highlighting and not complicated to implement.