BIP-0322: remove address optionality for PoF

[guggero]

This is cleaning up an artifact that was left over from #1352.
The alternative to removing the optionality of the address would be to assume OP_TRUE in case of an empty address.
But that sounds like it could open up any number of potential vulnerabilities.
And a user still has the ability to use an OP_TRUE script in a p2wsh or p2tr address.

[guggero]

Also pushed a commit that clarifies that even for Proof of Funds signatures the
sequence of the first input is considered as "age S".
The reasoning behind this is that only the first input is a synthetic
one that doesn't reference a real transaction to which the height could
be compared against. Even though the Proof of Funds inputs could have
higher sequence values, those heights might have already been long
reached and would make the sequence values irrelevant compared to the potential
restrictions in the challenge script.

Relevance: btcsuite/btcd#2521 (comment)

[jonatack]

Do you think a changelog entry is needed for this change?

(Edit, for informational purposes here is the relevant section of BIP 3:)

A Complete BIP can only move to Deployed or Closed. Any necessary changes to the specification should be minimal and interfere as little as possible with ongoing adoption. If a Complete BIP is found to need substantial functional changes, it may be preferable to move it to Closed[^new-BIP], and to start a new BIP with the changes instead. Otherwise, it could cause confusion as to what being compliant with the BIP means.

[guggero]

@scgbckbone pinging you here since you have a PoF implementation. How did you interpret the address optionality for PoF in the ColdCard implementation? Do you allow the address (message_challenge) to be empty?

[scgbckbone]

@scgbckbone pinging you here since you have a PoF implementation. How did you interpret the address optionality for PoF in the ColdCard implementation? Do you allow the address (message_challenge) to be empty?

not allowed in released version of Coldcard firmware. Also not planning to allow it - do not see the use of it. Am I missing something ? what would be the point of allowing empty spk ?

[guggero]

not allowed in released version of Coldcard firmware. Also not planning to allow it - do not see the use of it. Am I missing something ? what would be the point of allowing empty spk ?

Okay, good to know, thanks. I don't see a use case either, that's why I'm removing that (probably left-over from an earlier change) fragment in 91c6685.
I just wanted to make sure this doesn't directly interfere with your current or future implementation.

[guggero]

Do you think a changelog entry is needed for this change?

(Edit, for informational purposes here is the relevant section of BIP 3:)

A Complete BIP can only move to Deployed or Closed. Any necessary changes to the specification should be minimal and interfere as little as possible with ongoing adoption. If a Complete BIP is found to need substantial functional changes, it may be preferable to move it to Closed[^new-BIP], and to start a new BIP with the changes instead. Otherwise, it could cause confusion as to what being compliant with the BIP means.

You're right, the removal of the address optionality is not a minor change and might affect existing implementations. I added a changelog entry to make that clear.
And I also did a bit of research into existing implementations, who would be affected:

• Coldcard: address is already mandatory, no change required, see message above
• https://github.com/rust-bitcoin/bip322: doesn't currently implement PoF
• https://github.com/bitcoindevkit/bdk-message-signer: implements PoF but requires address to be set
• https://github.com/ACken2/bip322-js: doesn't currently implement PoF

Therefore I hope that the actual impact of this change is minimal at this point in time.

[jonatack]

@guggero LGTM, with one question on the changelog entry.

Given this change could theoretically break compatibility for any implementation, not known at this time, that allowed address-less PoFs, per BIP 3 it would be a major version bump? Perhaps there is enough confidence per #2188 (comment) that currently no such implementations are in use. OTOH version numbers are cheap.

The MAJOR version is incremented if changes to the BIP’s Specification are introduced that are incompatible with prior versions (which should be rare after a BIP is Complete, and only happen in well-grounded exceptional cases to a BIP that is Deployed). The MINOR version is incremented whenever the specification of the BIP is changed or extended in a backward-compatible way. The PATCH version is incremented for other changes to the BIP that are noteworthy (bug fixes, test vectors, important clarifications, etc.)

[murchandamus]

@guggero LGTM, with one question on the changelog entry.

Given this change could theoretically break compatibility for any implementation, not known at this time, that allowed address-less PoFs, per BIP 3 it would be a major version bump?

I agree that this should have been more than a PATCH bump, but I don’t understand why you leave feedback that should be addressed within minutes of merging a change. It would have been better to just leave the feedback and merge it after it was addressed, there wasn’t any time pressure here.

[guggero]

@jonatack okay, addressed in #2190.