BIP-322 basic support #24058

kallewoof · 2022-01-13T12:30:29Z

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

ghost · 2022-01-13T13:04:42Z

Concept ACK

This should fix #10542

Sjors · 2022-01-13T13:39:51Z

@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 · 2022-01-13T13:46:38Z

src/script/interpreter.cpp

@@ -1707,6 +1711,9 @@ bool GenericTransactionSignatureChecker<T>::CheckSchnorrSignature(Span<const uns

    uint8_t hashtype = SIGHASH_DEFAULT;
    if (sig.size() == 65) {
+        if (m_require_sighash_all) {


Although SIGHASH_DEFAULT implies SIGHASH_ALL, AFAIK using SIGHASH_ALL is still possible for users who wish to waste 1 byte. So I think you need m_require_sighash_all && nHashType != SIGHASH_ALL here too.

Isn't that a malleability? Anyway, gotcha.

Sjors · 2022-01-13T13:59:12Z

src/rpc/misc.cpp

@@ -348,9 +348,16 @@ static RPCHelpMan verifymessage()
        throw JSONRPCError(RPC_TYPE_ERROR, "Address does not refer to key");
    case MessageVerificationResult::ERR_MALFORMED_SIGNATURE:
        throw JSONRPCError(RPC_TYPE_ERROR, "Malformed base64 encoding");
+    case MessageVerificationResult::ERR_POF:
+        throw JSONRPCError(RPC_TYPE_ERROR, "Proof of funds require UTXO set access to verify"); // TODO: get access to UTXO set / mempool to handle this, then remove this error code?


This message implies the user can do something about it. Suggest instead: "BIP-322 Proof of Funds is not supported"

Sounds good. The intention is to actually add this functionality, but I think it should be its own PR.

Sjors · 2022-01-13T14:00:36Z

src/rpc/misc.cpp

+    case MessageVerificationResult::ERR_POF:
+        throw JSONRPCError(RPC_TYPE_ERROR, "Proof of funds require UTXO set access to verify"); // TODO: get access to UTXO set / mempool to handle this, then remove this error code?
+    case MessageVerificationResult::INCONCLUSIVE:
+        return false; // TODO: switch to a string based result? mix bool and strings?


Why not just throw in these other cases?

Because these aren't errors? But yeah, that would be one way to return a message. I think I'd prefer to expand the returned value from simple bool to true|false|"inconclusive", but not sure that would be appreciated.

Some RPCs return false as the result and an error message field. Others just throw with an error code and message. It's an error in the vague sense that the RPC failed to verify what it was asked to verify.

OK, but what about "OK_TIMELOCKED"?

Sjors · 2022-01-13T14:03:16Z

src/util/message.h

+    OK,
+
+    //
+    // BIP-322 extensions


I'm unsure, but perhaps we should make a fresh enum BIP322VerificationResult?

Oftentimes you won't know which type it is until you finish verifying, so unifying the two types seems sensible to me.

DrahtBot · 2022-01-13T14:58:37Z

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.

benthecarman · 2022-01-13T18:38:46Z

@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: bitcoin-s/bitcoin-s#3823

kallewoof · 2022-01-13T23:10:52Z

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

benthecarman · 2022-01-13T23:14:09Z

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

kallewoof · 2022-01-13T23:15:09Z

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.

luke-jr · 2022-01-15T22:06:56Z

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.

kallewoof · 2022-01-16T12:10:59Z

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

theStack

Strong Concept ACK

Thanks for working on this!

kallewoof · 2022-01-26T10:20:29Z

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

theStack

PR description nit:

(RPC) format is restricted to SINGLE mode now; it may eventually be LEGACY, SINGLE, OR FULL

This should likely be SIMPLE rather than SINGLE?

theStack · 2022-01-30T13:24:08Z

src/util/message.cpp

+    }
+
+    // prepare message hash
+    uint256 message_hash = (CHashWriter(HASHER_BIP322) << message).GetSHA256();


Note that HASHER_BIP322 and some other identifiers are not available yet at this commit, i.e. compiling b23186d currently fails:

... CXX wallet/libbitcoin_wallet_tool_a-wallettool.o util/message.cpp:105:41: error: use of undeclared identifier 'HASHER_BIP322' uint256 message_hash = (CHashWriter(HASHER_BIP322) << message).GetSHA256(); ^ util/message.cpp:116:22: error: use of undeclared identifier 'DecodeTx' if (signature && DecodeTx(to_sign, *signature, /* try_no_witness */ true, /* try_witness */ true)) { ^ util/message.cpp:120:49: error: no member named 'ERR_POF' in 'MessageVerificationResult' result = MessageVerificationResult::ERR_POF; ~~~~~~~~~~~~~~~~~~~~~~~~~~~^ util/message.cpp:128:49: error: no member named 'ERR_INVALID' in 'MessageVerificationResult' result = MessageVerificationResult::ERR_INVALID; ~~~~~~~~~~~~~~~~~~~~~~~~~~~^ util/message.cpp:141:57: error: no member named 'ERR_INVALID' in 'MessageVerificationResult' result = MessageVerificationResult::ERR_INVALID; ~~~~~~~~~~~~~~~~~~~~~~~~~~~^ 5 errors generated. ...

kyranjamie · 2023-03-25T09:27:25Z

src/test/util_tests.cpp

+         "AkgwRQIhAOzyynlqt93lOKJr+wmmxIens//zPzl9tqIOua93wO6MAiBi5n5EyAcPScOjf1lAqIUIQtr3zKNeavYabHyR8eGhowEhAsfxIAMZZEKUPYWI4BruhAQjzFT8FSFSajuFwrDL1Yhy",
+            "Hello World"),


I get this value in my implementation, yet it is not reflected in the BIP

benma · 2023-10-26T10:08:40Z

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

Github-Pull: bitcoin#24058 Rebased-From: 3a5816f

Github-Pull: bitcoin#24058 Rebased-From: f0af8bd

Github-Pull: bitcoin#24058 Rebased-From: cf2d75f

Github-Pull: bitcoin#24058 Rebased-From: 35cf639

Github-Pull: bitcoin#24058 Rebased-From: b08d75d

Github-Pull: bitcoin#24058 Rebased-From: 3b00ed9

Github-Pull: bitcoin#24058 Rebased-From: f57ed58

Github-Pull: bitcoin#24058 Rebased-From: b32c238

Github-Pull: bitcoin#24058 Rebased-From: 69bd3c4

Github-Pull: bitcoin#24058 Rebased-From: b8a55e2

…rify tests Co-authored-by: Will Abramson <wip.abramson@gmail.com> Github-Pull: bitcoin#24058 Rebased-From: 63a2f59

Co-authored-by: Will Abramson <wip.abramson@gmail.com> Github-Pull: bitcoin#24058 Rebased-From: de070b3

Co-authored-by: Will Abramson <wip.abramson@gmail.com> Github-Pull: bitcoin#24058 Rebased-From: 29b28d0

DrahtBot added Consensus GUI interfaces RPC/REST/ZMQ Utils/log/libs Wallet labels Jan 13, 2022

kallewoof force-pushed the 202201-bip322 branch from c9ff394 to abc51e1 Compare January 13, 2022 12:45

Sjors reviewed Jan 13, 2022

View reviewed changes

benthecarman mentioned this pull request Jan 13, 2022

BIP 322 utility functions bitcoin-s/bitcoin-s#3823

Open

This was referenced Jan 13, 2022

Simplify BaseSignatureChecker virtual functions and GenericTransactionSignatureChecker constructors #22793

Closed

Implement BIP-119 Validation (CheckTemplateVerify) #21702

Closed

sanket1729 mentioned this pull request Jan 13, 2022

More complete MessageSignature.is_signed_by_address rust-bitcoin/rust-bitcoin#684

Closed

kallewoof force-pushed the 202201-bip322 branch 2 times, most recently from 8eb5031 to 93a9c68 Compare January 14, 2022 04:53

theStack reviewed Jan 19, 2022

View reviewed changes

ghost mentioned this pull request Jan 28, 2022

[RPC] Removal of signmessage and verifymessage #24186

Closed

theStack reviewed Jan 30, 2022

View reviewed changes

kallewoof force-pushed the 202201-bip322 branch 2 times, most recently from 6de4601 to 5053032 Compare January 31, 2022 04:18

johnzweng mentioned this pull request Nov 21, 2022

Add support for message signing/verification with SegWit addresses (BIP 137) bitcoinj/bitcoinj#2657

Closed

mmilata mentioned this pull request Dec 5, 2022

Implement message signing for Taproot trezor/trezor-firmware#1943

Open

2color mentioned this pull request Dec 20, 2022

IPNS: support Ethereum wallet signing ipfs/specs#323

Open

MatthewLM mentioned this pull request Dec 23, 2022

Verify message signatures against addresses Vesta-wallet/coinslib#52

Merged

ghost mentioned this pull request Jan 12, 2023

signmessage and verifymessage : "Address does not refer to key" #26876

Closed

DrahtBot mentioned this pull request Feb 7, 2023

wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets #26008

Merged

windsok mentioned this pull request Feb 15, 2023

Ownership proofs – message signing and verification ordinals/ord#1462

Open

topether21 mentioned this pull request Mar 24, 2023

Add Bip-322 signing to verify ownership of address deezy-inc/deezy-place#71

Open

kyranjamie reviewed Mar 25, 2023

View reviewed changes

achow101 mentioned this pull request Apr 22, 2023

Consider Removing Message Signing #27515

Closed

seannybird approved these changes May 8, 2023

View reviewed changes

TylerOlsen mentioned this pull request Jul 18, 2023

VerifyMessage does not exist MetacoSA/NBitcoin#1172

Open

kallewoof closed this Aug 3, 2023

314159265359879 mentioned this pull request Sep 4, 2023

Questions about signMessage, signing arbitrary messages with Leather leather-io/extension#4174

Open

Czino mentioned this pull request Jan 15, 2024

Message signing only works with legacy addresses ElementsProject/elements#1309

Open

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Mar 12, 2024

make DecodeTx() available to avoid repeated hex conversions

bea146a

Github-Pull: bitcoin#24058 Rebased-From: 3a5816f

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Mar 12, 2024

script: add 'require_sighash_all' flag to signature checker

793c1a0

Github-Pull: bitcoin#24058 Rebased-From: f0af8bd

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Mar 12, 2024

message: add BIP-322 signature format (legacy, simple, full)

34b76ac

Github-Pull: bitcoin#24058 Rebased-From: cf2d75f

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Mar 12, 2024

message: add BIP-322 support to MessageHash function

8e73fb5

Github-Pull: bitcoin#24058 Rebased-From: 35cf639

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Mar 12, 2024

message: extend MessageVerificationResult to support BIP-322

3406fe0

Github-Pull: bitcoin#24058 Rebased-From: b08d75d

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Mar 12, 2024

message: add BIP322Txs class for preparing BIP-322 transactions

50a364e

Github-Pull: bitcoin#24058 Rebased-From: 3b00ed9

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Mar 12, 2024

message: add BIP-322 verification support (without POF)

2dff30e

Github-Pull: bitcoin#24058 Rebased-From: f57ed58

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Mar 12, 2024

wallet: add SignMessageBIP322() helper method to ScriptPubKeyMan

458c5d4

Github-Pull: bitcoin#24058 Rebased-From: b32c238

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Mar 12, 2024

add basic BIP-322 message signing support

efeb6ab

Github-Pull: bitcoin#24058 Rebased-From: 69bd3c4

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Mar 12, 2024

test: add BIP-322 related tests

ba75a16

Github-Pull: bitcoin#24058 Rebased-From: b8a55e2

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Mar 12, 2024

test: Add externally generated signature (buidl-python library) to ve…

f2f4637

…rify tests Co-authored-by: Will Abramson <wip.abramson@gmail.com> Github-Pull: bitcoin#24058 Rebased-From: 63a2f59

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Mar 12, 2024

Taproot tests

112eb4e

Co-authored-by: Will Abramson <wip.abramson@gmail.com> Github-Pull: bitcoin#24058 Rebased-From: de070b3

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Mar 12, 2024

2-of-3 p2sh, 3-of-3 p2wsh

9ab7b8a

Co-authored-by: Will Abramson <wip.abramson@gmail.com> Github-Pull: bitcoin#24058 Rebased-From: 29b28d0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BIP-322 basic support #24058

BIP-322 basic support #24058

kallewoof commented Jan 13, 2022 •

edited

Loading

ghost commented Jan 13, 2022 •

edited by ghost

Loading

Sjors commented Jan 13, 2022 •

edited

Loading

Sjors Jan 13, 2022

kallewoof Jan 13, 2022

Sjors Jan 13, 2022

kallewoof Jan 13, 2022

Sjors Jan 13, 2022

kallewoof Jan 13, 2022

Sjors Jan 14, 2022

kallewoof Jan 14, 2022

Sjors Jan 13, 2022

kallewoof Jan 13, 2022

DrahtBot commented Jan 13, 2022 •

edited

Loading

benthecarman commented Jan 13, 2022

kallewoof commented Jan 13, 2022

benthecarman commented Jan 13, 2022

kallewoof commented Jan 13, 2022

luke-jr commented Jan 15, 2022

kallewoof commented Jan 16, 2022

theStack left a comment

kallewoof commented Jan 26, 2022

theStack left a comment

theStack Jan 30, 2022

kyranjamie Mar 25, 2023

benma commented Oct 26, 2023

		"AkgwRQIhAOzyynlqt93lOKJr+wmmxIens//zPzl9tqIOua93wO6MAiBi5n5EyAcPScOjf1lAqIUIQtr3zKNeavYabHyR8eGhowEhAsfxIAMZZEKUPYWI4BruhAQjzFT8FSFSajuFwrDL1Yhy",
		"Hello World"),

BIP-322 basic support #24058

BIP-322 basic support #24058

Conversation

kallewoof commented Jan 13, 2022 • edited Loading

ghost commented Jan 13, 2022 • edited by ghost Loading

Sjors commented Jan 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DrahtBot commented Jan 13, 2022 • edited Loading

Reviews

Conflicts

benthecarman commented Jan 13, 2022

kallewoof commented Jan 13, 2022

benthecarman commented Jan 13, 2022

kallewoof commented Jan 13, 2022

luke-jr commented Jan 15, 2022

kallewoof commented Jan 16, 2022

theStack left a comment

Choose a reason for hiding this comment

kallewoof commented Jan 26, 2022

theStack left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benma commented Oct 26, 2023

kallewoof commented Jan 13, 2022 •

edited

Loading

ghost commented Jan 13, 2022 •

edited by ghost

Loading

Sjors commented Jan 13, 2022 •

edited

Loading

DrahtBot commented Jan 13, 2022 •

edited

Loading