BIP-322 basic support #24058

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.

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.

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

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.

Note that HASHER_BIP322 and some other identifiers are not available yet at this commit, i.e. compiling b23186d9f816aefe2fcf95e6f77d941c65e9c71c 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.
...

    // BIP322 signature created using buidl-python library with same parameters as test on line 2596
    BOOST_CHECK_EQUAL(
        MessageVerify(
            "bc1q9vza2e8x573nczrlzms0wvx3gsqjx7vavgkx0l",
         "AkgwRQIhAOzyynlqt93lOKJr+wmmxIens//zPzl9tqIOua93wO6MAiBi5n5EyAcPScOjf1lAqIUIQtr3zKNeavYabHyR8eGhowEhAsfxIAMZZEKUPYWI4BruhAQjzFT8FSFSajuFwrDL1Yhy",
            "Hello World"),
        MessageVerificationResult::OK);
        

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?

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

Added suggested test and rebased. Thanks!

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

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

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

Two additional test vectors for p2sh and p2wsh multisig bip322 sigs

    // 2-of-3 p2sh multisig BIP322 signature (created with the buidl-python library)
    // Keys are defined as (HDRootWIF, bip322_path)
    // Key1 (L4DksdGZ4KQJfcLHD5Dv25fu8Rxyv7hHi2RjZR4TYzr8c6h9VNrp, m/45'/0/0/1)
    // Key2 (KzSRqnCVwjzY8id2X5oHEJWXkSHwKUYaAXusjwgkES8BuQPJnPNu, m/45'/0/0/3)
    // Key3 (L1zt9Rw7HrU7jaguMbVzhiX8ffuVkmMis5wLHddXYuHWYf8u8uRj, m/45'/0/0/6)
    // BIP322 includes signs from Key2 and Key3
    BOOST_CHECK_EQUAL(
        MessageVerify(
            "3LnYoUkFrhyYP3V7rq3mhpwALz1XbCY9Uq",
         "AAAAAAHNcfHaNfl8f/+ZC2gTr8aF+0KgppYjKM94egaNm/u1ZAAAAAD8AEcwRAIhAJ6hdj61vLDP+aFa30qUZQmrbBfE0kiOObYvt5nqPSxsAh9IrOKFwflfPRUcQ/5e0REkdFHVP2GGdUsMgDet+sNlAUcwRAIgH3eW/VyFDoXvCasd8qxgwj5NDVo0weXvM6qyGXLCR5YCIEwjbEV6fS6RWP6QsKOcMwvlGr1/SgdCC6pW4eH87/YgAUxpUiECKJfGy28imLcuAeNBLHCNv3NRP5jnJwFDNRXCYNY/vJ4hAv1RQtaZs7+vKqQeWl2rb/jd/gMxkEjUnjZdDGPDZkMLIQL65cH2X5O7LujjTLDL2l8Pxy0Y2UUR99u1qCfjdz7dklOuAAAAAAEAAAAAAAAAAAFqAAAAAA==",
            "This will be a p2sh 2-of-3 multisig BIP 322 signed message"),
        MessageVerificationResult::OK);

    // 3-of-3 p2wsh multisig BIP322 signature (created with the buidl-python library)
    // Keys are defined as (HDRootWIF, bip322_path)
    // Key1 (L4DksdGZ4KQJfcLHD5Dv25fu8Rxyv7hHi2RjZR4TYzr8c6h9VNrp, m/45'/0/0/6)
    // Key2 (KzSRqnCVwjzY8id2X5oHEJWXkSHwKUYaAXusjwgkES8BuQPJnPNu, m/45'/0/0/9)
    // Key3 (L1zt9Rw7HrU7jaguMbVzhiX8ffuVkmMis5wLHddXYuHWYf8u8uRj, m/45'/0/0/11)
    BOOST_CHECK_EQUAL(
        MessageVerify(
            "bc1qlqtuzpmazp2xmcutlwv0qvggdvem8vahkc333usey4gskug8nutsz53msw",    "BQBIMEUCIQDQoXvGKLH58exuujBOta+7+GN7vi0lKwiQxzBpuNuXuAIgIE0XYQlFDOfxbegGYYzlf+tqegleAKE6SXYIa1U+uCcBRzBEAiATegywVl6GWrG9jJuPpNwtgHKyVYCX2yfuSSDRFATAaQIgTLlU6reLQsSIrQSF21z3PtUO2yAUseUWGZqRUIE7VKoBSDBFAiEAgxtpidsU0Z4u/+5RB9cyeQtoCW5NcreLJmWXZ8kXCZMCIBR1sXoEinhZE4CF9P9STGIcMvCuZjY6F5F0XTVLj9SjAWlTIQP3dyWvTZjUENWJowMWBsQrrXCUs20Gu5YF79CG5Ga0XSEDwqI5GVBOuFkFzQOGH5eTExSAj2Z/LDV/hbcvAPQdlJMhA17FuuJd+4wGuj+ZbVxEsFapTKAOwyhfw9qpch52JKxbU64=",
            "This will be a p2wsh 3-of-3 multisig BIP 322 signed message"),
        MessageVerificationResult::OK);
        

Committed.

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.

    // Single key p2tr BIP322 signature (created with the buidl-python library)
    // PrivateKeyWIF L3VFeEujGtevx9w18HD1fhRbCH67Az2dpCymeRE1SoPK6XQtaN2k 
    BOOST_CHECK_EQUAL(
        MessageVerify(
            "bc1ppv609nr0vr25u07u95waq5lucwfm6tde4nydujnu8npg4q75mr5sxq8lt3",                                                                "AUHd69PrJQEv+oKTfZ8l+WROBHuy9HKrbFCJu7U1iK2iiEy1vMU5EfMtjc+VSHM7aU0SDbak5IUZRVno2P5mjSafAQ==",
            "Hello World"),
        MessageVerificationResult::OK);

    // Same p2tr BIP322 signature as above (created with the buidl-python library)
    // Signature should not verify against the message
    BOOST_CHECK_EQUAL(
        MessageVerify(
            "bc1ppv609nr0vr25u07u95waq5lucwfm6tde4nydujnu8npg4q75mr5sxq8lt3",
         "AUHd69PrJQEv+oKTfZ8l+WROBHuy9HKrbFCJu7U1iK2iiEy1vMU5EfMtjc+VSHM7aU0SDbak5IUZRVno2P5mjSafAQ==",
            "Hello World - This should fail"),
        MessageVerificationResult::ERR_INVALID);

    // wrong address

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

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

HASHER_BIP322 is not defined in this commit.

Moved HASHER_BIP322.

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

MessageVerificationResult::ERR_POF is not defined in this commit.

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

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

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

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

MessageVerificationResult::ERR_INVALID is not defined in this commit.

Resolved above.

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.

Switched to using optional.

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.

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

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 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 5bc21371947340a35039cb9ce95571b45bc535a4 "message: add BIP322Txs class for preparing BIP-322 transactions"

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

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

In 8f9c561c7b0887b98ce5a6670428fdeb1590deb1 "add basic BIP-322 message signing support"

Missing QString("</nobr>".

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.

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

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

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.

Switched to asserting.

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.

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

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.

nit:

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

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:

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

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

class BIP322Txs {
private:

nit:

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

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

    auto signature_bytes = DecodeBase64(signature);

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

3b00ed9a52d91864665f074eadaafc179344ba56

    if (message_challenge.empty()) {

3b00ed9a52d91864665f074eadaafc179344ba56

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

f57ed58ef7b5381c595b7833b52775ef948bb7da

    const CTxDestination& destination,

b32c238dd5bc413b4b03f99ae68f7e2b78b42d62

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

nit: could also name the last parameter here

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

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

@kallewoof do you remember how you generated these signatures? Or how buidl-python parses the WIF to arrive at a private key? I tried reproducing this here, but can't figure out how you got from the three WIFs you posted to the public keys in the script of the signature.

If I parse the signature, I get the following script and public keys:

raw_script = 532103f77725af4d98d410d589a3031606c42bad7094b36d06bb9605efd086e466b45d2103c2a23919504eb85905cd03861f97931314808f667f2c357f85b72f00f41d949321035ec5bae25dfb8c06ba3f996d5c44b056a94ca00ec3285fc3daa9721e7624ac5b53ae

parsed_script = multi(3,03f77725af4d98d410d589a3031606c42bad7094b36d06bb9605efd086e466b45d,03c2a23919504eb85905cd03861f97931314808f667f2c357f85b72f00f41d9493,035ec5bae25dfb8c06ba3f996d5c44b056a94ca00ec3285fc3daa9721e7624ac5b)#3tva9u77

key1 = L4DksdGZ4KQJfcLHD5Dv25fu8Rxyv7hHi2RjZR4TYzr8c6h9VNrp --> 027568b11f122ff8a7bc1c57e5c7642055bc618967b2f7bfe8e11fe99903c94dd3
key2 = KzSRqnCVwjzY8id2X5oHEJWXkSHwKUYaAXusjwgkES8BuQPJnPNu --> 020a8bdf79cfa421d9655e9282800f115ff1d9db1e721ceb4248a3fcfec7faa67c
key3 = L1zt9Rw7HrU7jaguMbVzhiX8ffuVkmMis5wLHddXYuHWYf8u8uRj --> 030c529e0ea40a00975d202624e39915daf7bdd2b71f31aa08596838781ce5f33a