BIP-322 basic support

kallewoof commented at 12:30 pm on January 13, 2022: member

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

DrahtBot added the label Consensus on Jan 13, 2022

DrahtBot added the label GUI on Jan 13, 2022

DrahtBot added the label interfaces on Jan 13, 2022

DrahtBot added the label RPC/REST/ZMQ on Jan 13, 2022

DrahtBot added the label Utils/log/libs on Jan 13, 2022

DrahtBot added the label Wallet on Jan 13, 2022

kallewoof force-pushed on Jan 13, 2022

ghost commented at 1:04 pm on January 13, 2022: none

Concept ACK

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

Sjors commented at 1:39 pm on January 13, 2022: member

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

in src/script/interpreter.cpp:1714 in ad0002c91b outdated

1710@@ -1707,6 +1711,9 @@ bool GenericTransactionSignatureChecker<T>::CheckSchnorrSignature(Span<const uns
1711 
1712     uint8_t hashtype = SIGHASH_DEFAULT;
1713     if (sig.size() == 65) {
1714+        if (m_require_sighash_all) {

Sjors commented at 1:46 pm on January 13, 2022:

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.

kallewoof commented at 10:18 pm on January 13, 2022:

Isn’t that a malleability? Anyway, gotcha.

in src/rpc/misc.cpp:352 in aed56aa486 outdated

347@@ -348,9 +348,16 @@ static RPCHelpMan verifymessage()
348         throw JSONRPCError(RPC_TYPE_ERROR, "Address does not refer to key");
349     case MessageVerificationResult::ERR_MALFORMED_SIGNATURE:
350         throw JSONRPCError(RPC_TYPE_ERROR, "Malformed base64 encoding");
351+    case MessageVerificationResult::ERR_POF:
352+        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?

Sjors commented at 1:59 pm on January 13, 2022:

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

kallewoof commented at 10:21 pm on January 13, 2022:

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

in src/rpc/misc.cpp:354 in aed56aa486 outdated

347@@ -348,9 +348,16 @@ static RPCHelpMan verifymessage()
348         throw JSONRPCError(RPC_TYPE_ERROR, "Address does not refer to key");
349     case MessageVerificationResult::ERR_MALFORMED_SIGNATURE:
350         throw JSONRPCError(RPC_TYPE_ERROR, "Malformed base64 encoding");
351+    case MessageVerificationResult::ERR_POF:
352+        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?
353+    case MessageVerificationResult::INCONCLUSIVE:
354+        return false; // TODO: switch to a string based result? mix bool and strings?

Sjors commented at 2:00 pm on January 13, 2022:

Why not just throw in these other cases?

kallewoof commented at 10:23 pm on January 13, 2022:

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.

Sjors commented at 12:25 pm on January 14, 2022:

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.

kallewoof commented at 11:09 pm on January 14, 2022:

OK, but what about “OK_TIMELOCKED”?

in src/util/message.h:54 in aed56aa486 outdated

35@@ -36,7 +36,23 @@ enum class MessageVerificationResult {
36     ERR_NOT_SIGNED,
37 
38     //! The message verification was successful.
39-    OK
40+    OK,
41+
42+    //
43+    // BIP-322 extensions

Sjors commented at 2:03 pm on January 13, 2022:

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

kallewoof commented at 11:10 pm on January 13, 2022:

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

DrahtBot commented at 2:58 pm on January 13, 2022: contributor

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 commented at 6:38 pm on January 13, 2022: contributor

@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

kallewoof commented at 11:10 pm on January 13, 2022: member

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

benthecarman commented at 11:14 pm on January 13, 2022: contributor

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 commented at 11:15 pm on January 13, 2022: member

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 force-pushed on Jan 14, 2022

luke-jr commented at 10:06 pm on January 15, 2022: member

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 commented at 12:10 pm on January 16, 2022: member

@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 commented at 6:45 pm on January 19, 2022: contributor

Strong Concept ACK

Thanks for working on this!

kallewoof commented at 10:20 am on January 26, 2022: member

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

in src/util/message.cpp:105 in b23186d9f8 outdated

100+        // (use legacy result)
101+        return std::nullopt;
102+    }
103+
104+    // prepare message hash
105+    uint256 message_hash = (CHashWriter(HASHER_BIP322) << message).GetSHA256();

theStack commented at 1:24 pm on January 30, 2022:

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

 0...
 1  CXX      wallet/libbitcoin_wallet_tool_a-wallettool.o
 2util/message.cpp:105:41: error: use of undeclared identifier 'HASHER_BIP322'
 3    uint256 message_hash = (CHashWriter(HASHER_BIP322) << message).GetSHA256();
 4                                        ^
 5util/message.cpp:116:22: error: use of undeclared identifier 'DecodeTx'
 6    if (signature && DecodeTx(to_sign, *signature, /* try_no_witness */ true, /* try_witness */ true)) {
 7                     ^
 8util/message.cpp:120:49: error: no member named 'ERR_POF' in 'MessageVerificationResult'
 9            result = MessageVerificationResult::ERR_POF;
10                     ~~~~~~~~~~~~~~~~~~~~~~~~~~~^
11util/message.cpp:128:49: error: no member named 'ERR_INVALID' in 'MessageVerificationResult'
12            result = MessageVerificationResult::ERR_INVALID;
13                     ~~~~~~~~~~~~~~~~~~~~~~~~~~~^
14util/message.cpp:141:57: error: no member named 'ERR_INVALID' in 'MessageVerificationResult'
15            result = MessageVerificationResult::ERR_INVALID;
16                     ~~~~~~~~~~~~~~~~~~~~~~~~~~~^
175 errors generated.
18...

theStack commented at 1:27 pm on January 30, 2022: contributor

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?

kallewoof force-pushed on Jan 31, 2022

kallewoof commented at 4:23 am on January 31, 2022: member

@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 force-pushed on Jan 31, 2022

kallewoof commented at 10:11 am on January 31, 2022: member

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

maflcko commented at 10:21 am on January 31, 2022: member

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

kallewoof force-pushed on Jan 31, 2022

kallewoof force-pushed on Feb 1, 2022

kallewoof force-pushed on Feb 2, 2022

kallewoof commented at 3:44 am on February 2, 2022: member

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 force-pushed on Feb 2, 2022

kallewoof force-pushed on Apr 5, 2022

DrahtBot added the label Needs rebase on Apr 27, 2022

in src/test/util_tests.cpp:2712 in 843ce398a1 outdated

2597+        MessageVerify(
2598+            "bc1q9vza2e8x573nczrlzms0wvx3gsqjx7vavgkx0l",
2599+            "AkcwRAIgZRfIY3p7/DoVTty6YZbWS71bc5Vct9p9Fia83eRmw2QCICK/ENGfwLtptFluMGs2KsqoNSk89pO7F29zJLUx9a/sASECx/EgAxlkQpQ9hYjgGu6EBCPMVPwVIVJqO4XCsMvViHI=",
2600+            "Hello World"),
2601+        MessageVerificationResult::OK);
2602+

wip-abramson commented at 4:03 pm on May 13, 2022:

0
1    // BIP322 signature created using buidl-python library with same parameters as test on line 2596
2    BOOST_CHECK_EQUAL(
3        MessageVerify(
4            "bc1q9vza2e8x573nczrlzms0wvx3gsqjx7vavgkx0l",
5         "AkgwRQIhAOzyynlqt93lOKJr+wmmxIens//zPzl9tqIOua93wO6MAiBi5n5EyAcPScOjf1lAqIUIQtr3zKNeavYabHyR8eGhowEhAsfxIAMZZEKUPYWI4BruhAQjzFT8FSFSajuFwrDL1Yhy",
6            "Hello World"),
7        MessageVerificationResult::OK);
8

We tried to create an implementation of BIP322 using the buidl-python library. The signature in this test case was produced using the same message and address. Not sure what is going on here.

I was also able to verify the signature provided in the test on line 2596 through the buidl python code I produced.

So two signatures for the same message for the same address are both returning as valid. This seems strange?

sipa commented at 1:23 pm on May 14, 2022:

@wip-abramson ECDSA signatures have randomness in them, so it is perfectly possible to construct multiple valid ones for the same key and message. Bitcoin Core’s signing logic is based on RFC6979, but additionally grinds the nonce in order to make sure the R and S values in signatures don’t exceed 32 bytes each. Is your signing code doing the same?

kallewoof commented at 6:56 am on May 17, 2022:

Added suggested test and rebased. Thanks!

wip-abramson commented at 12:51 pm on May 17, 2022:

Makes sense thanks! @sipa I am not a hundred per cent sure, but I think so. This is the code in buidl-python to generate the deterministic k value. https://github.com/buidl-bitcoin/buidl-python/blob/e62e0b9fd4271473cf641cde0fe366e65c787403/buidl/pecc.py#L563

jandrieu commented at 0:09 am on May 20, 2022:

Is there some reason it is useful to have different signatures considered valid for the same message?

sipa commented at 0:21 am on May 20, 2022:

@jandrieu It is an inevitability. No unique signature scheme based on the discrete logarithm problem that Bitcoin relies on is known (unique signatures schemes are ones where only a single signature is valid per (message, public key) pair).

kallewoof force-pushed on May 16, 2022

DrahtBot removed the label Needs rebase on May 16, 2022

kallewoof force-pushed on May 16, 2022

kallewoof force-pushed on May 17, 2022

wip-abramson commented at 12:59 pm on May 17, 2022: none

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

laanwj removed the label Consensus on May 26, 2022

laanwj removed the label Utils/log/libs on May 26, 2022

laanwj removed the label interfaces on May 26, 2022

in src/test/util_tests.cpp:2720 in 64ecca3f0e outdated

2686+        MessageVerify(
2687+            "bc1q9vza2e8x573nczrlzms0wvx3gsqjx7vavgkx0l",
2688+         "AkgwRQIhAOzyynlqt93lOKJr+wmmxIens//zPzl9tqIOua93wO6MAiBi5n5EyAcPScOjf1lAqIUIQtr3zKNeavYabHyR8eGhowEhAsfxIAMZZEKUPYWI4BruhAQjzFT8FSFSajuFwrDL1Yhy",
2689+            "Hello World"),
2690+        MessageVerificationResult::OK);
2691+

wip-abramson commented at 11:41 am on July 8, 2022:

Two additional test vectors for p2sh and p2wsh multisig bip322 sigs

 0
 1    // 2-of-3 p2sh multisig BIP322 signature (created with the buidl-python library)
 2    // Keys are defined as (HDRootWIF, bip322_path)
 3    // Key1 (L4DksdGZ4KQJfcLHD5Dv25fu8Rxyv7hHi2RjZR4TYzr8c6h9VNrp, m/45'/0/0/1)
 4    // Key2 (KzSRqnCVwjzY8id2X5oHEJWXkSHwKUYaAXusjwgkES8BuQPJnPNu, m/45'/0/0/3)
 5    // Key3 (L1zt9Rw7HrU7jaguMbVzhiX8ffuVkmMis5wLHddXYuHWYf8u8uRj, m/45'/0/0/6)
 6    // BIP322 includes signs from Key2 and Key3
 7    BOOST_CHECK_EQUAL(
 8        MessageVerify(
 9            "3LnYoUkFrhyYP3V7rq3mhpwALz1XbCY9Uq",
10         "AAAAAAHNcfHaNfl8f/+ZC2gTr8aF+0KgppYjKM94egaNm/u1ZAAAAAD8AEcwRAIhAJ6hdj61vLDP+aFa30qUZQmrbBfE0kiOObYvt5nqPSxsAh9IrOKFwflfPRUcQ/5e0REkdFHVP2GGdUsMgDet+sNlAUcwRAIgH3eW/VyFDoXvCasd8qxgwj5NDVo0weXvM6qyGXLCR5YCIEwjbEV6fS6RWP6QsKOcMwvlGr1/SgdCC6pW4eH87/YgAUxpUiECKJfGy28imLcuAeNBLHCNv3NRP5jnJwFDNRXCYNY/vJ4hAv1RQtaZs7+vKqQeWl2rb/jd/gMxkEjUnjZdDGPDZkMLIQL65cH2X5O7LujjTLDL2l8Pxy0Y2UUR99u1qCfjdz7dklOuAAAAAAEAAAAAAAAAAAFqAAAAAA==",
11            "This will be a p2sh 2-of-3 multisig BIP 322 signed message"),
12        MessageVerificationResult::OK);
13
14    // 3-of-3 p2wsh multisig BIP322 signature (created with the buidl-python library)
15    // Keys are defined as (HDRootWIF, bip322_path)
16    // Key1 (L4DksdGZ4KQJfcLHD5Dv25fu8Rxyv7hHi2RjZR4TYzr8c6h9VNrp, m/45'/0/0/6)
17    // Key2 (KzSRqnCVwjzY8id2X5oHEJWXkSHwKUYaAXusjwgkES8BuQPJnPNu, m/45'/0/0/9)
18    // Key3 (L1zt9Rw7HrU7jaguMbVzhiX8ffuVkmMis5wLHddXYuHWYf8u8uRj, m/45'/0/0/11)
19    BOOST_CHECK_EQUAL(
20        MessageVerify(
21            "bc1qlqtuzpmazp2xmcutlwv0qvggdvem8vahkc333usey4gskug8nutsz53msw",    "BQBIMEUCIQDQoXvGKLH58exuujBOta+7+GN7vi0lKwiQxzBpuNuXuAIgIE0XYQlFDOfxbegGYYzlf+tqegleAKE6SXYIa1U+uCcBRzBEAiATegywVl6GWrG9jJuPpNwtgHKyVYCX2yfuSSDRFATAaQIgTLlU6reLQsSIrQSF21z3PtUO2yAUseUWGZqRUIE7VKoBSDBFAiEAgxtpidsU0Z4u/+5RB9cyeQtoCW5NcreLJmWXZ8kXCZMCIBR1sXoEinhZE4CF9P9STGIcMvCuZjY6F5F0XTVLj9SjAWlTIQP3dyWvTZjUENWJowMWBsQrrXCUs20Gu5YF79CG5Ga0XSEDwqI5GVBOuFkFzQOGH5eTExSAj2Z/LDV/hbcvAPQdlJMhA17FuuJd+4wGuj+ZbVxEsFapTKAOwyhfw9qpch52JKxbU64=",
22            "This will be a p2wsh 3-of-3 multisig BIP 322 signed message"),
23        MessageVerificationResult::OK);
24

kallewoof commented at 0:42 am on July 12, 2022:

Committed.

Rspigler commented at 3:42 pm on July 8, 2022: contributor

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

in src/test/util_tests.cpp:2763 in 64ecca3f0e outdated

2687+            "bc1q9vza2e8x573nczrlzms0wvx3gsqjx7vavgkx0l",
2688+         "AkgwRQIhAOzyynlqt93lOKJr+wmmxIens//zPzl9tqIOua93wO6MAiBi5n5EyAcPScOjf1lAqIUIQtr3zKNeavYabHyR8eGhowEhAsfxIAMZZEKUPYWI4BruhAQjzFT8FSFSajuFwrDL1Yhy",
2689+            "Hello World"),
2690+        MessageVerificationResult::OK);
2691+
2692+    // wrong address

wip-abramson commented at 6:00 pm on July 11, 2022:

This is a test case that demonstrates the issue with taproot signatures that @richerandprettier refers to. It seems that p2tr signatures validate against any message under the current impl.

Is there some extra configuration required to setup bitcoin with schnorr & taproot, wondering if these sigs are processed at all?

I ran this against the buidl-python code - https://github.com/buidl-bitcoin/buidl-python/pull/140 - and get the expected results. Will dig into the code when i get some time.

 0
 1    // Single key p2tr BIP322 signature (created with the buidl-python library)
 2    // PrivateKeyWIF L3VFeEujGtevx9w18HD1fhRbCH67Az2dpCymeRE1SoPK6XQtaN2k 
 3    BOOST_CHECK_EQUAL(
 4        MessageVerify(
 5            "bc1ppv609nr0vr25u07u95waq5lucwfm6tde4nydujnu8npg4q75mr5sxq8lt3",                                                                "AUHd69PrJQEv+oKTfZ8l+WROBHuy9HKrbFCJu7U1iK2iiEy1vMU5EfMtjc+VSHM7aU0SDbak5IUZRVno2P5mjSafAQ==",
 6            "Hello World"),
 7        MessageVerificationResult::OK);
 8
 9    // Same p2tr BIP322 signature as above (created with the buidl-python library)
10    // Signature should not verify against the message
11    BOOST_CHECK_EQUAL(
12        MessageVerify(
13            "bc1ppv609nr0vr25u07u95waq5lucwfm6tde4nydujnu8npg4q75mr5sxq8lt3",
14         "AUHd69PrJQEv+oKTfZ8l+WROBHuy9HKrbFCJu7U1iK2iiEy1vMU5EfMtjc+VSHM7aU0SDbak5IUZRVno2P5mjSafAQ==",
15            "Hello World - This should fail"),
16        MessageVerificationResult::ERR_INVALID);
17
18    // wrong address

kallewoof commented at 0:38 am on July 12, 2022:

Thanks for the test! Immensely helpful. Looking into what’s causing the issue now.

kallewoof commented at 0:39 am on July 12, 2022: member

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 force-pushed on Jul 12, 2022

luke-jr commented at 3:10 am on July 12, 2022: member

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.

kallewoof commented at 3:18 am on July 12, 2022: member

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 commented at 3:32 am on July 12, 2022: member

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.

Rspigler commented at 6:03 am on July 12, 2022: contributor

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

DrahtBot added the label Needs rebase on Jul 12, 2022

jandrieu commented at 10:42 pm on July 12, 2022: none

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.

luke-jr commented at 11:06 pm on July 12, 2022: member

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.

jandrieu commented at 3:24 pm on July 13, 2022: none

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

kallewoof commented at 3:59 am on July 14, 2022: member

@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 force-pushed on Jul 14, 2022

DrahtBot removed the label Needs rebase on Jul 14, 2022

DrahtBot added the label Needs rebase on Jul 22, 2022

in src/util/message.cpp:89 in 85ed08c22c outdated

89+        }
90+
91+    case MessageSignatureFormat::SIMPLE:
92+    case MessageSignatureFormat::FULL:
93+        {
94+            CHashWriter hasher(HASHER_BIP322);

achow101 commented at 7:12 pm on July 25, 2022:

In 85ed08c22ca51ed4b9d8de0a402280b9401811af “message: add BIP-322 support to MessageHash function”

HASHER_BIP322 is not defined in this commit.

kallewoof commented at 6:15 am on July 28, 2022:

Moved HASHER_BIP322.

in src/util/message.cpp:213 in 5bc2137194 outdated

134+    CMutableTransaction to_sign;
135+    if (signature && DecodeTx(to_sign, *signature, /* try_no_witness */ true, /* try_witness */ true)) {
136+        // validate decoded transaction
137+        // multiple inputs (proof of funds) are not supported as we do not have UTXO set access
138+        if (to_sign.vin.size() > 1) {
139+            result = MessageVerificationResult::ERR_POF;

achow101 commented at 7:19 pm on July 25, 2022:

In 5bc21371947340a35039cb9ce95571b45bc535a4 “message: add BIP322Txs class for preparing BIP-322 transactions”

MessageVerificationResult::ERR_POF is not defined in this commit.

kallewoof commented at 5:34 am on July 29, 2022:

Moved the extension into a separate commit that comes before this one.

in src/util/message.cpp:201 in 5bc2137194 outdated

130+    to_spend.nLockTime = 0;
131+    to_spend.vin.emplace_back(COutPoint(uint256::ZERO, 0xFFFFFFFF), (CScript() << OP_0 << message_hash_vec), 0);
132+    to_spend.vout.emplace_back(0, message_challenge);
133+
134+    CMutableTransaction to_sign;
135+    if (signature && DecodeTx(to_sign, *signature, /* try_no_witness */ true, /* try_witness */ true)) {

achow101 commented at 7:20 pm on July 25, 2022:

In 5bc21371947340a35039cb9ce95571b45bc535a4 “message: add BIP322Txs class for preparing BIP-322 transactions”

Missing an #include <core_io.h> for DecodeTx.

in src/util/message.cpp:221 in 5bc2137194 outdated

142+        if ((to_sign.vin.size() == 0 || to_sign.vin[0].prevout.hash != to_spend.GetHash()) ||
143+            (to_sign.vin[0].prevout.n != 0) ||
144+            (to_sign.vout.size() != 1) ||
145+            (to_sign.vout[0].nValue != 0) ||
146+            (to_sign.vout[0].scriptPubKey != (CScript() << OP_RETURN))) {
147+            result = MessageVerificationResult::ERR_INVALID;

achow101 commented at 7:20 pm on July 25, 2022:

In 5bc21371947340a35039cb9ce95571b45bc535a4 “message: add BIP322Txs class for preparing BIP-322 transactions”

MessageVerificationResult::ERR_INVALID is not defined in this commit.

kallewoof commented at 5:36 am on July 29, 2022:

Resolved above.

in src/util/message.h:112 in 5bc2137194 outdated

91+class BIP322Txs {
92+    template<class T1, class T2>
93+    BIP322Txs(const T1& to_spend, const T2& to_sign) : m_to_spend{to_spend}, m_to_sign{to_sign} { }
94+
95+public:
96+    static std::optional<BIP322Txs> Create(const CTxDestination& destination, const std::string& message, MessageVerificationResult& result, const std::vector<unsigned char>* signature = nullptr);

achow101 commented at 7:21 pm on July 25, 2022:

In 5bc21371947340a35039cb9ce95571b45bc535a4 “message: add BIP322Txs class for preparing BIP-322 transactions”

I don’t like using pointers as essentially an optional. It should just be a std::optional with a default value of std::nullopt.

kallewoof commented at 5:47 am on July 29, 2022:

Switched to using optional.

in src/rpc/signmessage.cpp:55 in 30642a1859 outdated

48@@ -49,9 +49,16 @@ static RPCHelpMan verifymessage()
49                 throw JSONRPCError(RPC_TYPE_ERROR, "Address does not refer to key");
50             case MessageVerificationResult::ERR_MALFORMED_SIGNATURE:
51                 throw JSONRPCError(RPC_TYPE_ERROR, "Malformed base64 encoding");
52+            case MessageVerificationResult::ERR_POF:
53+                throw JSONRPCError(RPC_TYPE_ERROR, "BIP-322 Proof of funds is not yet supported"); // TODO: get access to UTXO set / mempool to handle this, then remove this error code?
54+            case MessageVerificationResult::INCONCLUSIVE:
55+                return false; // TODO: switch to a string based result? mix bool and strings?

achow101 commented at 7:24 pm on July 25, 2022:

In 30642a185971555ea68bd5b80f87d633ddd09593 “message: add BIP-322 verification support (without POF)”

This could just fallthrough to the return false below instead of having it’s own return false.

kallewoof commented at 5:23 am on July 27, 2022:

The idea is that the return false; statement itself is a placeholder for when a decision is made on how to deal with inconclusive proofs.

in src/util/message.cpp:179 in 5bc2137194 outdated

108@@ -109,3 +109,65 @@ std::string SigningResultString(const SigningResult res)
109     }
110     assert(false);
111 }
112+
113+std::optional<BIP322Txs> BIP322Txs::Create(const CTxDestination& destination, const std::string& message, MessageVerificationResult& result, const std::vector<unsigned char>* signature)

achow101 commented at 7:33 pm on July 25, 2022:

In 5bc21371947340a35039cb9ce95571b45bc535a4 “message: add BIP322Txs class for preparing BIP-322 transactions”

Since both BIP 322 and BIP 325 have very similar transaction construction functions, what do you think about de-deduplicating and having SignetTxs::Create call this function? It would require a minor tweak to this function to take a vector of bytes to push onto to_spend’s input’s scriptSig.

kallewoof commented at 5:25 am on July 27, 2022:

That is the plan yeah, but I was considering it to be the follow-up next step after this PR is merged, to keep this minimal. As you say the follow-up PR to fix this should be quite trivial and easy to review.

in src/script/interpreter.h:303 in 62f307f442 outdated

299@@ -299,6 +300,7 @@ class GenericTransactionSignatureChecker : public BaseSignatureChecker
300     bool CheckSchnorrSignature(Span<const unsigned char> sig, Span<const unsigned char> pubkey, SigVersion sigversion, ScriptExecutionData& execdata, ScriptError* serror = nullptr) const override;
301     bool CheckLockTime(const CScriptNum& nLockTime) const override;
302     bool CheckSequence(const CScriptNum& nSequence) const override;
303+    void RequireSighashAll() { m_require_sighash_all = true; }

achow101 commented at 7:49 pm on July 25, 2022:

In 62f307f442050b1149fb326efcd5d76252787f19 “script: add ‘require_sighash_all’ flag to signature checker”

I think it would be better to have this in the constructor rather than a separate setter function.

in src/util/message.h:110 in 5bc2137194 outdated

84@@ -85,4 +85,18 @@ uint256 MessageHash(const std::string& message, MessageSignatureFormat format);
85 
86 std::string SigningResultString(const SigningResult res);
87 
88+/**
89+ * Generate the BIP-322 tx corresponding to the given challenge
90+ */
91+class BIP322Txs {
92+    template<class T1, class T2>
93+    BIP322Txs(const T1& to_spend, const T2& to_sign) : m_to_spend{to_spend}, m_to_sign{to_sign} { }

achow101 commented at 7:49 pm on July 25, 2022:

In 5bc21371947340a35039cb9ce95571b45bc535a4 “message: add BIP322Txs class for preparing BIP-322 transactions”

I don’t see why this needs to be templated?

kallewoof commented at 5:51 am on July 29, 2022:

If it is merged with the BIP-325 variant, it does need to be.

in src/qt/signverifymessagedialog.cpp:228 in 8f9c561c7b outdated

223+        );
224+        return;
225+    case MessageVerificationResult::ERR_POF:
226+        // TODO: support proof of funds verifications
227+        ui->statusLabel_VM->setText(
228+            tr("Proof of funds verification unavailable right now.")

achow101 commented at 8:10 pm on July 25, 2022:

In 8f9c561c7b0887b98ce5a6670428fdeb1590deb1 “add basic BIP-322 message signing support”

Missing QString("</nobr>".

in src/qt/signverifymessagedialog.cpp:222 in 8f9c561c7b outdated

217+            QString("<nobr>") + tr("Inconclusive.") + QString("</nobr>")
218+        );
219+        return;
220+    case MessageVerificationResult::ERR_INVALID:
221+        ui->statusLabel_VM->setText(
222+            tr("Some check failed.")

achow101 commented at 8:11 pm on July 25, 2022:

In 8f9c561c7b0887b98ce5a6670428fdeb1590deb1 “add basic BIP-322 message signing support”

I think this should just be Message verification failed. It could passthrough to the ERR_NOT_SIGNED result.

kallewoof commented at 7:04 am on July 29, 2022:

I would like to avoid using the same message for different errors, as it makes troubleshooting more difficult. That said, maybe “Some check failed” is not the right wording here…

in src/util/message.cpp:42 in e100fed327 outdated

39@@ -40,6 +40,7 @@ static constexpr unsigned int BIP322_REQUIRED_FLAGS =
40 |   SCRIPT_VERIFY_CLEANSTACK
41 |   SCRIPT_VERIFY_P2SH
42 |   SCRIPT_VERIFY_WITNESS
43+|   SCRIPT_VERIFY_TAPROOT

achow101 commented at 8:21 pm on July 25, 2022:

In e100fed327a0bd3ce11a882782c6cce3487fb28a “fix: add missing SCRIPT_VERIFY_TAPROOT to BIP322_REQUIRED_FLAGS”

Could be squashed into 30642a185971555ea68bd5b80f87d633ddd09593 “message: add BIP-322 verification support (without POF)”

in src/wallet/scriptpubkeyman.cpp:217 in f0747683eb outdated

212+{
213+    assert(format != MessageSignatureFormat::LEGACY);
214+
215+    MessageVerificationResult result; // unused
216+    auto txs = BIP322Txs::Create(address, message, result);
217+    if (!txs) return SigningResult::PRIVATE_KEY_NOT_AVAILABLE;

achow101 commented at 8:23 pm on July 25, 2022:

In f0747683eb73f2052b9ef18b8246e81411c30e20 “wallet: add SignMessageBIP322() helper method to ScriptPubKeyMan”

This could be an assert. It should not be possible to reach this point without having a valid destination, and as we do not provide a signature argument, Create should never fail.

kallewoof commented at 7:09 am on July 29, 2022:

I’m not sure that’s the case - what if address does not contain a destination?

achow101 commented at 2:48 pm on July 29, 2022:

It shouldn’t. First, both the RPC and the GUI will validate that address is a valid destination before passing it on. Then CWallet::SignMessage also ensures that there is a valid destination by getting the script for the destination and using the ScriptPubKeyMan that can provide for that script. So by the time we get to this line, we’ve already checked at least twice that the destination is valid.

kallewoof commented at 5:49 am on August 1, 2022:

Switched to asserting.

in src/util/message.cpp:120 in 30642a1859 outdated

123+        return MessageVerifyBIP322(destination, *signature_bytes, message, MessageVerificationResult::ERR_PUBKEY_NOT_RECOVERED);
124     }
125 
126     if (!(CTxDestination(PKHash(pubkey)) == destination)) {
127-        return MessageVerificationResult::ERR_NOT_SIGNED;
128+        return MessageVerifyBIP322(destination, *signature_bytes, message, MessageVerificationResult::ERR_NOT_SIGNED);

achow101 commented at 8:36 pm on July 25, 2022:

In 30642a185971555ea68bd5b80f87d633ddd09593 “message: add BIP-322 verification support (without POF)”

This is unnecessary and will always fail.

By the time we reach this line, the destination must be PKHash and we must have been able to recover a pubkey from the signature, which means that the signature must have been 65 bytes, otherwise RecoverCompact will have failed. However, a BIP322 signature for PKHash must be greater than 65 bytes because it will include a full transaction for the p2pkh address. So if we do reach this line, the signature is just invalid and there’s no need to attempt a MessageVerifyBIP322.

kallewoof commented at 5:28 am on July 27, 2022:

Good catch. Didn’t think about the >65b part.

kallewoof commented at 7:13 am on July 29, 2022:

I took the liberty of pasting your reasoning as a comment in case future reviewers wonder why a BIP322 attempt isn’t made. I can drop that if it seems overkill.

kallewoof force-pushed on Jul 28, 2022

DrahtBot removed the label Needs rebase on Jul 28, 2022

luke-jr commented at 6:56 am on July 28, 2022: member

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

kallewoof commented at 7:07 am on July 28, 2022: member

[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 force-pushed on Jul 29, 2022

kallewoof force-pushed on Aug 1, 2022

in src/util/message.cpp:31 in 11f2a152d1 outdated

24@@ -25,6 +25,11 @@
25  */
26 const std::string MESSAGE_MAGIC = "Bitcoin Signed Message:\n";
27 
28+/**
29+ * BIP-322 tagged hash
30+ */
31+const HashWriter HASHER_BIP322 = TaggedHash("BIP0322-signed-message");

theStack commented at 10:14 pm on September 20, 2022:

nit:

0static const HashWriter HASHER_BIP322{TaggedHash("BIP0322-signed-message")};

in src/util/message.cpp:165 in 11f2a152d1 outdated

 95+    case MessageSignatureFormat::SIMPLE:
 96+    case MessageSignatureFormat::FULL:
 97+        {
 98+            HashWriter hasher{HASHER_BIP322};
 99+            if (!message.empty()) {
100+                hasher.write(AsBytes(Span{message.data(), message.size() * sizeof(char)}));

theStack commented at 10:15 pm on September 20, 2022:

nit: Considering that sizeof(char) is always 1 (see e.g. https://www.cs.technion.ac.il/users/yechiel/c++-faq/sizeof-char.html), this multiplication is not needed:

0                hasher.write(AsBytes(Span{message.data(), message.size()}));

in src/util/message.h:107 in ff121f7af5 outdated

100@@ -101,4 +101,18 @@ uint256 MessageHash(const std::string& message, MessageSignatureFormat format);
101 
102 std::string SigningResultString(const SigningResult res);
103 
104+/**
105+ * Generate the BIP-322 tx corresponding to the given challenge
106+ */
107+class BIP322Txs {

theStack commented at 10:34 pm on September 20, 2022:

nit: Default visibility for classes in C++ is private, but IMHO it would still be more readable if that is stated explicitly:

0class BIP322Txs {
1private:

in src/util/message.cpp:200 in ff121f7af5 outdated

135+    to_spend.nLockTime = 0;
136+    to_spend.vin.emplace_back(COutPoint(uint256::ZERO, 0xFFFFFFFF), (CScript() << OP_0 << message_hash_vec), 0);
137+    to_spend.vout.emplace_back(0, message_challenge);
138+
139+    CMutableTransaction to_sign;
140+    if (signature.has_value() && DecodeTx(to_sign, signature.value(), /* try_no_witness */ true, /* try_witness */ true)) {

theStack commented at 10:42 pm on September 20, 2022:

nit:

0    if (signature.has_value() && DecodeTx(to_sign, signature.value(), /* try_no_witness= */ true, /* try_witness= */ true)) {

in src/util/message.cpp:98 in 5f651713e9 outdated

94 MessageVerificationResult MessageVerify(
95     const std::string& address,
96     const std::string& signature,
97     const std::string& message)
98 {
99+    auto signature_bytes = DecodeBase64(signature.c_str());

theStack commented at 10:47 pm on September 20, 2022:

nit: DecodeBase64 takes a std::string_view, i.e. passing a string directly is fine:

0    auto signature_bytes = DecodeBase64(signature);

in src/util/message.cpp:118 in 5f651713e9 outdated

121-        return MessageVerificationResult::ERR_PUBKEY_NOT_RECOVERED;
122+        return MessageVerifyBIP322(destination, *signature_bytes, message, MessageVerificationResult::ERR_PUBKEY_NOT_RECOVERED);
123     }
124 
125     if (!(CTxDestination(PKHash(pubkey)) == destination)) {
126+        // By the time we reach this line, the destination must be `PKHash` and we must have been able to recover a pubkey from the signature, which means that the signature must have been 65 bytes, otherwise `RecoverCompact` will have failed. However, a BIP322 signature for `PKHash` must be greater than 65 bytes because it will include a full transaction for the p2pkh address. So if we do reach this line, the signature is just invalid and there's no need to attempt a `MessageVerifyBIP322`.

theStack commented at 10:51 pm on September 20, 2022:

With almost 500 characters, this comment is definitely too long for one line and should be split into multiple ones.

theStack commented at 10:57 pm on September 20, 2022: contributor

Left a bunch of code-review comments (mostly nits) below. I’m planning to do a second more in-depth review round within the next days.

in src/util/message.cpp:191 in e54e5eef78 outdated

174@@ -93,3 +175,65 @@ std::string SigningResultString(const SigningResult res)
175     }
176     assert(false);
177 }
178+
179+std::optional<BIP322Txs> BIP322Txs::Create(const CTxDestination& destination, const std::string& message, MessageVerificationResult& result, std::optional<const std::vector<unsigned char>> signature)
180+{
181+    // attempt to get script pub key for destination
182+    CScript message_challenge = GetScriptForDestination(destination);
183+    if (message_challenge.size() == 0) {

aureleoules commented at 2:22 pm on September 21, 2022:

3b00ed9a52d91864665f074eadaafc179344ba56

0    if (message_challenge.empty()) {

in src/util/message.cpp:216 in e54e5eef78 outdated

203+        // multiple inputs (proof of funds) are not supported as we do not have UTXO set access
204+        if (to_sign.vin.size() > 1) {
205+            result = MessageVerificationResult::ERR_POF;
206+            return std::nullopt;
207+        }
208+        if ((to_sign.vin.size() == 0 || to_sign.vin[0].prevout.hash != to_spend.GetHash()) ||

aureleoules commented at 2:23 pm on September 21, 2022:

3b00ed9a52d91864665f074eadaafc179344ba56

0        if ((to_sign.vin.empty() || to_sign.vin[0].prevout.hash != to_spend.GetHash()) ||

in src/util/message.cpp:53 in e54e5eef78 outdated

49+|   SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_PUBKEYTYPE
50+|   SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_TAPROOT_VERSION
51+|   SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_WITNESS_PROGRAM;
52+
53+MessageVerificationResult MessageVerifyBIP322(
54+    CTxDestination& destination,

aureleoules commented at 2:23 pm on September 21, 2022:

f57ed58ef7b5381c595b7833b52775ef948bb7da

0    const CTxDestination& destination,

DrahtBot added the label Needs rebase on Sep 24, 2022

make DecodeTx() available to avoid repeated hex conversions 3a5816f0e6

script: add 'require_sighash_all' flag to signature checker f0af8bd40b

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

kallewoof force-pushed on Sep 26, 2022

DrahtBot removed the label Needs rebase on Sep 26, 2022

message: add BIP-322 support to MessageHash function 35cf639f99

message: extend MessageVerificationResult to support BIP-322 b08d75d1a9

message: add BIP322Txs class for preparing BIP-322 transactions 3b00ed9a52

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

wallet: add SignMessageBIP322() helper method to ScriptPubKeyMan b32c238dd5

kallewoof force-pushed on Sep 26, 2022

kallewoof commented at 6:44 am on September 26, 2022: member

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

add basic BIP-322 message signing support 69bd3c4fe8

test: add BIP-322 related tests b8a55e2545

test: Add externally generated signature (buidl-python library) to verify tests

Co-authored-by: Will Abramson <wip.abramson@gmail.com>

63a2f592c3

Taproot tests

Co-authored-by: Will Abramson <wip.abramson@gmail.com>

de070b3a95

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

Co-authored-by: Will Abramson <wip.abramson@gmail.com>

29b28d07fa

kallewoof force-pushed on Sep 26, 2022

in src/wallet/scriptpubkeyman.cpp:234 in 29b28d07fa

229+        // TODO: this may be a multisig which successfully signed but needed additional signatures
230+        return SigningResult::SIGNING_FAILED;
231+    }
232+
233+    // We force the format to FULL, if this turned out to be a legacy format (p2pkh) signature
234+    if (to_sign.vin[0].scriptSig.size() > 0 || to_sign.vin[0].scriptWitness.IsNull()) {

aureleoules commented at 10:56 am on November 7, 2022:

b32c238dd5bc413b4b03f99ae68f7e2b78b42d62

0    if (!to_sign.vin[0].scriptSig.empty() || to_sign.vin[0].scriptWitness.IsNull()) {

aureleoules approved

aureleoules commented at 10:58 am on November 7, 2022: member

I verified that signmessage/verifymessage works by manually creating signatures using every output type (legacy, p2sh-segwit, bech32, and bech32m). I also verified that the scheme used in this PR matches the BIP322 specification. Left some nits.

in src/util/message.h:113 in 3b00ed9a52 outdated

108+private:
109+    template<class T1, class T2>
110+    BIP322Txs(const T1& to_spend, const T2& to_sign) : m_to_spend{to_spend}, m_to_sign{to_sign} { }
111+
112+public:
113+    static std::optional<BIP322Txs> Create(const CTxDestination& destination, const std::string& message, MessageVerificationResult& result, std::optional<const std::vector<unsigned char>> = std::nullopt);

theStack commented at 5:30 pm on November 18, 2022:

nit: could also name the last parameter here

0    static std::optional<BIP322Txs> Create(const CTxDestination& destination, const std::string& message, MessageVerificationResult& result, std::optional<const std::vector<unsigned char>> signature = std::nullopt);

theStack commented at 5:32 pm on November 18, 2022: contributor

Light code-review ACK 29b28d07fa958b89e1c7916fda5d8654474cf495 (modulo GUI-related code)

FWIW, here is the missing BIP322 tx-creation test for bech32m (taproot) using a key-path spend (though the internals don’t really matter here, since we don’t sign anyway), feel free to pick it up:

 0diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp
 1index 70a78a4d3..94d8a837f 100644
 2--- a/src/test/util_tests.cpp
 3+++ b/src/test/util_tests.cpp
 4@@ -10,6 +10,7 @@
 5 #include <key.h>  // For CKey
 6 #include <key_io.h> // EncodeDestination
 7 #include <outputtype.h> // For BIP-322 tests
 8+#include <script/standard.h>
 9 #include <sync.h>
10 #include <test/util/logging.h>
11 #include <test/util/setup_common.h>
12@@ -2638,7 +2639,14 @@ BOOST_AUTO_TEST_CASE(message_sign)
13         BOOST_FAIL("Failed to create BIP-322 txs for bech32 address");
14     }
15
16-    // TODO: BECH32M
17+    // BECH32M
18+    TaprootBuilder builder;
19+    auto dest_bech32m = builder.Finalize(XOnlyPubKey{pubkey}).GetOutput();
20+    BOOST_CHECK_EQUAL("bc1ptk7g2ztd0fxmjtn09lkr245tmrjjn2vjftrwps6hqwrd7s7w22jspf7a33", EncodeDestination(dest_bech32m));
21+    auto txs_bech32m = BIP322Txs::Create(dest_bech32m, message, mvr);
22+    if (!txs_bech32m || mvr != MessageVerificationResult::OK) {
23+        BOOST_FAIL("Failed to create BIP-322 txs for bech32m address");
24+    }
25 }
26
27 BOOST_AUTO_TEST_CASE(message_verify)

in src/test/util_tests.cpp:2718 in 29b28d07fa

2713+    // BIP322 signature created using buidl-python library with same parameters as test on line 2596
2714+    BOOST_CHECK_EQUAL(
2715+        MessageVerify(
2716+            "bc1q9vza2e8x573nczrlzms0wvx3gsqjx7vavgkx0l",
2717+         "AkgwRQIhAOzyynlqt93lOKJr+wmmxIens//zPzl9tqIOua93wO6MAiBi5n5EyAcPScOjf1lAqIUIQtr3zKNeavYabHyR8eGhowEhAsfxIAMZZEKUPYWI4BruhAQjzFT8FSFSajuFwrDL1Yhy",
2718+            "Hello World"),

kyranjamie commented at 9:27 am on March 25, 2023:

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

seannybird approved

kallewoof closed this on Aug 3, 2023

benma commented at 10:08 am on October 26, 2023: none

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

luke-jr referenced this in commit bea146a3d4 on Mar 12, 2024

luke-jr referenced this in commit 793c1a06eb on Mar 12, 2024

luke-jr referenced this in commit 34b76ac4ef on Mar 12, 2024

luke-jr referenced this in commit 8e73fb5740 on Mar 12, 2024

luke-jr referenced this in commit 3406fe093f on Mar 12, 2024

luke-jr referenced this in commit 50a364e878 on Mar 12, 2024

luke-jr referenced this in commit 2dff30e28c on Mar 12, 2024

luke-jr referenced this in commit 458c5d414f on Mar 12, 2024

luke-jr referenced this in commit efeb6ab6a4 on Mar 12, 2024

luke-jr referenced this in commit ba75a16dad on Mar 12, 2024

luke-jr referenced this in commit f2f4637b16 on Mar 12, 2024

luke-jr referenced this in commit 112eb4e5a9 on Mar 12, 2024

luke-jr referenced this in commit 9ab7b8ada6 on Mar 12, 2024

BIP-322 basic support #24058

Reviews

Conflicts