bitcoin icon indicating copy to clipboard operation
bitcoin copied to clipboard

BIP-322 basic support

Open kallewoof opened this issue 3 years ago • 27 comments

This PR enables support for BIP-322, the sign/verify message upgrade.

Fixes #10542.

This PR is greatly simplified and is supposed to have one or more follow-up PRs to complete functionality.

In this PR:

  • signing using single key (any type)
  • verifying any signed message (auto-detects BIP-322 vs legacy)

Missing features/limitations:

  • proof of funds (i.e. additional inputs) support
  • multisig or other custom address type support (I need feedback on how to do this; I assume some psbt thing would be good?)
  • (RPC) format is restricted to SIMPLE mode now; it may eventually be LEGACY, SIMPLE, or FULL. (In some cases, it will use FULL format but this is not selectable yet.)
  • timelock support

kallewoof avatar Jan 13 '22 12:01 kallewoof

Concept ACK

This should fix https://github.com/bitcoin/bitcoin/issues/10542

ghost avatar Jan 13 '22 13:01 ghost

@kallewoof do you know if any other wallet implemented BIP-322 yet, so we can compare the implementation?

This is really a good time to add test vectors to the BIP (and this PR).

Sjors avatar Jan 13 '22 13:01 Sjors

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK theStack
Concept ACK ghost

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #26008 (wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot avatar Jan 13 '22 14:01 DrahtBot

@kallewoof do you know if any other wallet implemented BIP-322 yet, so we can compare the implementation?

This is really a good time to add test vectors to the BIP (and this PR).

I have an implementation here: https://github.com/bitcoin-s/bitcoin-s/pull/3823

benthecarman avatar Jan 13 '22 18:01 benthecarman

I will make test vectors very soon. Let's compare notes at that point @benthecarman.

kallewoof avatar Jan 13 '22 23:01 kallewoof

I will make test vectors very soon. Let's compare notes at that point @benthecarman.

Sounds good, I made some in pr if you want to try those

benthecarman avatar Jan 13 '22 23:01 benthecarman

Yep. I will try them, once I make my own. I don't want to adjust the code to fit your test vectors, I'd rather both implementations independently passing both sets.

kallewoof avatar Jan 13 '22 23:01 kallewoof

Would like to see BIP 322 address the distinction between the current "signer receives fund with the address" and the long-desired "signer sent a prior transaction" even if not supported by this PR.

luke-jr avatar Jan 15 '22 22:01 luke-jr

@luke-jr I'm not sure if you're suggesting a BIP change or a code feature. If the latter, I think the ideal path forward is for you to open a pull request follow-up to this one (maybe post-merge) to do what you are requesting. If former, open a PR to the BIP?

kallewoof avatar Jan 16 '22 12:01 kallewoof

There is now a WIP with test vectors in https://github.com/bitcoin/bips/pull/1279 which needs to be filled in (possibly with @benthecarman's cases).

kallewoof avatar Jan 26 '22 10:01 kallewoof

@theStack Thanks for the review. You're right, I meant SIMPLE not SINGLE. Fixed.

I did some clean-up and I think this also fixes the issues with HASHER_BIP322 being used before it is added.

kallewoof avatar Jan 31 '22 04:01 kallewoof

No idea why fuzzer runs out of time (2h!). Restarting didn't fix it either.

kallewoof avatar Jan 31 '22 10:01 kallewoof

This was fixed in 9237bdaac196951a437accaefa65638149b25978. Restarting the fuzz now should hopefully fix it.

maflcko avatar Jan 31 '22 10:01 maflcko

Updated to reflect discrepancies in https://github.com/bitcoin/bips/pull/1279#issuecomment-1027545873 (version was explicitly set to 2 even though BIP allow(ed) any appropriate value, resulting in differences between implementations).

kallewoof avatar Feb 02 '22 03:02 kallewoof

Whilst implementing bip322 I noticed a discrepancy in the test vectors in the actual BIP vs the ones used in this P.R. The ones currently in bip322 are incorrect and don't verify against the messages provided. The above-mentioned P.R. is a fix for this

wip-abramson avatar May 17 '22 12:05 wip-abramson

Tested for Legacy, P2SH-SegWit, SegWit, and Taproot. (commit 64ecca3f0e43b9088bf4be842a4096929724480f)

signmessage "address" "mesage" -> signature verifymessage "address" "signature" "message" -> true

Results in true when it should, and errors when messed with.

Except for Taproot addresses, which for some reason when the message is malformed, still results in true

All tests passed

This is a pre-req for BIP 129

Rspigler avatar Jul 08 '22 15:07 Rspigler

Tested for Legacy, P2SH-SegWit, SegWit, and Taproot. (commit 64ecca3) [..] Except for Taproot addresses, which for some reason when the message is malformed, still results in true

Thanks a lot for testing! Looking into taproot issues.

kallewoof avatar Jul 12 '22 00:07 kallewoof

I'm not sure if you're suggesting a BIP change or a code feature.

This is a BIP incompleteness thing that should be addressed before any implementation is considered for merging. The BIP gives a technical way to "sign a message", without defining what the signature actually means.

Current signed messages only mean "the message is from whoever receives/d coins that are/were sent to this address". Yet there's relatively little correct use of it. Instead, people seem to want things like proof-of-funds and proof-of-sending, which are fundamentally different in nature (eg, not associated with an address). The BIP should define each of these use cases distinctly, so they can't be misrepresented as the wrong kind of proof.

luke-jr avatar Jul 12 '22 03:07 luke-jr

The SCRIPT_VERIFY_TAPROOT flag was not added to the required flags, resulting in taproot checks being auto-successes. Fixed, thanks for the help!

Edit: thinking about this, this is not how BIP-322 is meant to behave when it encounters unknown opcodes and such; the "inconclusive" middle state is meant to address exactly this, and not just throw a "true" out blindly. Annoying.

Edit 2: this ends up being a silly special case for taproot, but it's worth taking note of as we may have the same issues in future versions as we upgrade the BIP322_REQUIRED_FLAGS vs BIP322_INCONCLUSIVE_FLAGS. The inconclusive flags checks for upgraded taproot versions and such, which current taproot is not, whereas the required flags neglected to include the current taproot.

kallewoof avatar Jul 12 '22 03:07 kallewoof

This is a BIP incompleteness thing that should be addressed before any implementation is considered for merging. The BIP gives a technical way to "sign a message", without defining what the signature actually means.

The motivation is quite clear on what it means, IMO:

Ultimately no message signing protocol can actually prove control of funds, both because a signature is obsolete as soon as it is created, and because the possessor of a secret key may be willing to sign messages on others' behalf even if it would not sign actual transactions. No signmessage protocol can fix these limitations.

If you have suggestions on improvements to make, please make a pull request to the BIP repository.

kallewoof avatar Jul 12 '22 03:07 kallewoof

Right now its:

signmessage "address" "message" -> signature verifymessage "address" "signature" "message" -> true/false

Would it not be clearer to have the verifymessage command take the arguments this way: verifymessage "address" "message" "signature" -> true/false

Rspigler avatar Jul 12 '22 06:07 Rspigler

This is a BIP incompleteness thing that should be addressed before any implementation is considered for merging. The BIP gives a technical way to "sign a message", without defining what the signature actually means.

This is not a problem specific to BIP-322. It's generally handled at another layer. For example, in Linked Data Signatures, a ProofPurpose is used to clarify why that particular proof is generated. For Verifiable Credentials, that proof purpose must be an attestationMethod, which means the signature was created as an attestation by the signer to the veracity of the signed document.

In contrast, zCaps, a capability mechanism that uses LinkedData signatures uses the two proof purposes of CapabilityInvocation and CapabilityDelegation to indicate the meaning of different signatures. The former to indicate that the signature means to "do" the action that is invoked, the latter that the signature means the previous controller is delegating to someone else (and not, at this time, invoking anything).

In all cases, the proof that is verified is cryptographic, and BIP-322 can provide that verifiable proof, while the proof-purpose is part of the signed payload yet exogenous to the signing itself.

FWIW, @wip-abramson and I are working on mapping BIP-322 to Verifiable Credentials. I'm not sure if he's shared that work with this conversation, but it is a good example of how you can generalize BIP-322 signatures for specific use cases.

jandrieu avatar Jul 12 '22 22:07 jandrieu

This is not a problem specific to BIP-322. It's generally handled at another layer.

It can't be. It's a different person signing in each scenario.

Proof-of-funds can be anyone who could spend the UTXOs. Proof-of-sender can only be the actual person who did send a given transaction. Proof-of-address can only be the person who received or would receive funds sent to an address. These can all be distinct entities, so the signature itself must specify which it is testifying to.

The existing signmessage standard/RPC is only capable of proof-of-address, and cannot prove funds or senders.

luke-jr avatar Jul 12 '22 23:07 luke-jr

This is not a problem specific to BIP-322. It's generally handled at another layer.

It can't be. It's a different person signing in each scenario.

I'm not sure what you mean, but that would seem to support my point that a signature itself does not describe why the signature was made.

Proof-of-funds can be anyone who could spend the UTXOs. Proof-of-sender can only be the actual person who did send a given transaction. Proof-of-address can only be the person who received or would receive funds sent to an address. These can all be distinct entities, so the signature itself must specify which it is testifying to.

The existing signmessage standard/RPC is only capable of proof-of-address, and cannot prove funds or senders.

I see, perhaps your complaint is with the language at https://github.com/bitcoin/bips/blob/master/bip-0322.mediawiki#full-proof-of-funds that "full" signatures can be used to demonstrate proof-of-funds?

On that, I agree. You can apply BIP-322 to get a contributing factor for proof-of-funds, but BIP-322 itself does not establish that.

All it can establish is that the given cryptographic puzzle of a specific script is satisfied with respect to a given message.

Whether or not you treat that valid signature as proof-of-funds or not is at a different layer. At a minimum, you would need to apply the logic that each of the gathered UTXOs is, in fact, still unspent. You will also need to decide if the solved puzzle should be interpreted as proof of funds for your use case, given that BIP-322 does not lock the funds.

If I've understood your point: that BIP-322 does not establish proof of funds, that would be a good candidate for an issue at https://github.com/bitcoin/bips/blob/master/bip-0322.mediawiki

jandrieu avatar Jul 13 '22 15:07 jandrieu

@Rspigler

Would it not be clearer to have the verifymessage command take the arguments this way: verifymessage "address" "message" "signature" -> true/false

It would, but unfortunately this order was set long before this pull request was created, and who knows what systems out there are depending on the order being what it is today. It may be worth it to do some simple analysis on the arguments, and swap them automatically based on which of "message" and "signature" looks like a signature, but I don't know if that would be appreciated, as it's quite an unusual thing to do, at least in this code base.

kallewoof avatar Jul 14 '22 03:07 kallewoof

https://github.com/bitcoin/bips/pull/1347

luke-jr avatar Jul 28 '22 06:07 luke-jr

[Ran out of time; will continue addressing comments and push complete changes later today/tomorrow]

~~[Edited next day: got most of the work done but not quite done yet. Hopefully will get to it later today; otherwise will try over weekend]~~

kallewoof avatar Jul 28 '22 07:07 kallewoof

Rebased and addressed some of @theStack's nits. Thanks for the review.

kallewoof avatar Sep 26 '22 06:09 kallewoof

@kallewoof could you leave a comment explaining why you closed this PR? It is not apparent from looking at the history of this PR what the state is of BIP-322 support in Bitcoin Core.

benma avatar Oct 26 '23 10:10 benma