descriptor: check whitespace in keys within fragments #31603

brunoerg commented at 2:27 PM on January 4, 2025: contributor

Currently, we successfully parse descriptors which contains spaces in the beginning or end of the public/private key within a fragment (e.g. pk( KEY), pk(KEY ) or pk( KEY )). I have noticed that one of the reasons is that the DecodeBase58 function simply ignore these whitespaces.

This PR changes the ParsePubkeyInner to reject pubkeys that contain a whitespace at the beginning and/or at the end. We will only check the whitespace in some RPCs (e.g. importdescriptors), but an already imported descriptor won't be affected by this check, especially because we store descriptors from ToString.

For context: https://github.com/brunoerg/bitcoinfuzz/issues/72

DrahtBot commented at 2:27 PM on January 4, 2025: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31603.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	rkrux, darosior, sipa

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

Conflicts

No conflicts as of last run.

brunoerg force-pushed on Jan 4, 2025

DrahtBot added the label CI failed on Jan 4, 2025

DrahtBot commented at 2:32 PM on January 4, 2025: contributor

🚧 At least one of the CI tasks failed. <sub>Debug: https://github.com/bitcoin/bitcoin/runs/35146721643</sub>

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

DrahtBot removed the label CI failed on Jan 4, 2025

furszy commented at 3:57 PM on January 4, 2025: member

As you are forbidding something that was previously allowed, this breaks compatibility with earlier versions. Wallets containing any key with a whitespace in the descriptor will fail to parse during the load procedure and shut down abruptly. Will either need a descriptor upgrade procedure or a compatibility + warning window.

brunoerg marked this as a draft on Jan 6, 2025

brunoerg commented at 4:02 PM on January 7, 2025: contributor

As you are forbidding something that was previously allowed, this breaks compatibility with earlier versions. Wallets containing any key with a whitespace in the descriptor will fail to parse during the load procedure and shut down abruptly. Will either need a descriptor upgrade procedure or a compatibility + warning window.

Good point. Moved it to draft to work on these things.

brunoerg force-pushed on Jan 9, 2025

brunoerg renamed this:
~~descriptor: check whitespace in `ParsePubkeyInner`~~
descriptor: check whitespace in pubkeys within fragments
on Jan 9, 2025

brunoerg marked this as ready for review on Jan 9, 2025

brunoerg commented at 5:30 PM on January 9, 2025: contributor

Ready for review!

Thanks, @furszy for the comment. I've changed the approched and described it on description.

darosior commented at 7:09 PM on January 10, 2025: member

Wallets containing any key with a whitespace in the descriptor will fail to parse during the load procedure and shut down abruptly.

As far as i remember we only ever store in the wallet descriptor produced by ToString(), which would mean no such wallet can exist. This would merely be an interface break for RPC's which parse descriptors, given the lack of usecase of using a space inside a pk() expression this would be pretty minor.

furszy commented at 7:36 PM on January 10, 2025: member

Wallets containing any key with a whitespace in the descriptor will fail to parse during the load procedure and shut down abruptly.

As far as i remember we only ever store in the wallet descriptor produced by ToString(), which would mean no such wallet can exist. This would merely be an interface break for RPC's which parse descriptors, given the lack of usecase of using a space inside a pk() expression this would be pretty minor.

Was thinking about that a few days ago and started checking it, but discovered another bug along the way and, because it is related, wanted to fix that before commenting here again. Storing the ToString() output means we store the descriptor with a different checksum, which could be confusing for users calling listdescriptors. However, it's the lesser of the evils, so the compatibility window might not be necessary. A test for this would be useful, verifying that we are actually storing the descriptor without whitespaces, which results in a different checksum from the one provided by the user.

Note about the test: this can't be done for taproot descriptors that do not contain all key/script paths key material (this is the bug I discovered, wip branch).

brunoerg commented at 9:01 PM on January 10, 2025: contributor

As far as i remember we only ever store in the wallet descriptor produced by ToString(), which would mean no such wallet can exist. This would merely be an interface break for RPC's which parse descriptors, given the lack of usecase of using a space inside a pk() expression this would be pretty minor.

It means I can go back to the previous approach (simpler one) and only change the ParsePubkeyInner, there is no need to control it in the descriptor Parse anymore.

brunoerg force-pushed on Jan 15, 2025

brunoerg commented at 8:20 PM on January 15, 2025: contributor

Force-pushed simplifying the approach. Now it just checks the whitespace in the ParsePubkeyInner function.

brunoerg force-pushed on Jan 15, 2025

in src/script/descriptor.cpp:1514 in 66911a02e8 outdated

1514 | @@ -1515,6 +1515,9 @@ std::vector<std::unique_ptr<PubkeyProvider>> ParsePubkeyInner(uint32_t key_exp_i
1515 |      if (str.size() == 0) {
1516 |          error = "No key provided";
1517 |          return {};
1518 | +    } else if (IsSpace(str[0]) || IsSpace(str.back())) {
1519 | +        error = strprintf("Pubkey '%s' is invalid due to whitespace", str);
1520 | +        return {};

furszy commented at 3:31 PM on January 17, 2025:

nit: Because there is a return statement in the if block that is above, you could remove the else and use str.front() instead of str[0].

brunoerg commented at 5:38 PM on January 17, 2025:

Done.

brunoerg force-pushed on Jan 17, 2025

in doc/release-notes-31603.md:4 in 250692de3b outdated

   0 | @@ -0,0 +1,6 @@
   1 | +Updated RPCs
   2 | +--------
   3 | +
   4 | +- Any RPC which one of the parameters are descriptors will throw an error

sipa commented at 5:33 PM on January 17, 2025:

Any RPC in which one of the parameters...

brunoerg commented at 5:37 PM on January 17, 2025:

Done

brunoerg force-pushed on Jan 17, 2025

brunoerg commented at 5:38 PM on January 17, 2025: contributor

Force-pushed addressing #31603 (review) and #31603 (review)

in src/script/descriptor.cpp:1515 in 4b87013931 outdated

1515 | @@ -1516,6 +1516,10 @@ std::vector<std::unique_ptr<PubkeyProvider>> ParsePubkeyInner(uint32_t key_exp_i
1516 |          error = "No key provided";
1517 |          return {};
1518 |      }
1519 | +    if (IsSpace(str.front()) || IsSpace(str.back())) {
1520 | +        error = strprintf("Pubkey '%s' is invalid due to whitespace", str);
1521 | +        return {};
1522 | +    }

furszy commented at 5:54 PM on January 17, 2025:

This could also be a WIP formatted private key right? Should use the general term "key" here. E.g.

strprintf(key '%s is invalid due to an extra whitespace at %s of the string', key_str, (IsSpace(str.front())?"beginning" : "end"));`

brunoerg commented at 6:21 PM on January 17, 2025:

Yes, it could be a WIF private key. I will change it to use "key".

brunoerg commented at 6:27 PM on January 17, 2025:

Done.

brunoerg force-pushed on Jan 17, 2025

brunoerg commented at 6:28 PM on January 17, 2025: contributor

Force-pushed addressing #31603 (review) (thanks, @furszy)

DrahtBot added the label CI failed on Jan 17, 2025

test: fix descriptors in `ismine_tests`

Some descriptors contain whitespace in public keys
within fragments. This fixes it.

50856695ef

brunoerg force-pushed on Jan 21, 2025

brunoerg commented at 6:12 PM on January 21, 2025: contributor

Forgot to fix a test, force-pushed doing it. Now CI is happy!

DrahtBot removed the label CI failed on Jan 21, 2025

brunoerg requested review from sipa on Jan 24, 2025

brunoerg requested review from furszy on Jan 24, 2025

in src/test/descriptor_tests.cpp:905 in 0d15a74437 outdated

 898 | @@ -899,6 +899,11 @@ BOOST_AUTO_TEST_CASE(descriptor_test)
 899 |      CheckUnparsable("sh(sh(pk(L4rK1yDtCWekvXuE6oXD9jCYfFNV2cWRpVuPLBcCU2z8TrisoyY1)))", "sh(sh(pk(03a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd)))", "Can only have sh() at top level"); // Cannot embed P2SH inside P2SH
 900 |      CheckUnparsable("wsh(wsh(pk(L4rK1yDtCWekvXuE6oXD9jCYfFNV2cWRpVuPLBcCU2z8TrisoyY1)))", "wsh(wsh(pk(03a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd)))", "Can only have wsh() at top level or inside sh()"); // Cannot embed P2WSH inside P2WSH
 901 |  
 902 | +    // Check for whitespace into pubkeys
 903 | +    CheckUnparsable("", "multi(1, L4rK1yDtCWekvXuE6oXD9jCYfFNV2cWRpVuPLBcCU2z8TrisoyY1,5KYZdUEo39z3FPrtuX2QbbwGnNP5zTd7yyr2SC1j299sBCnWjss)", "Multi: Key ' L4rK1yDtCWekvXuE6oXD9jCYfFNV2cWRpVuPLBcCU2z8TrisoyY1' is invalid due to whitespace");
 904 | +    CheckUnparsable("", "pk(L4rK1yDtCWekvXuE6oXD9jCYfFNV2cWRpVuPLBcCU2z8TrisoyY1 )", "pk(): Key 'L4rK1yDtCWekvXuE6oXD9jCYfFNV2cWRpVuPLBcCU2z8TrisoyY1 ' is invalid due to whitespace");
 905 | +    CheckUnparsable("", "pk( L4rK1yDtCWekvXuE6oXD9jCYfFNV2cWRpVuPLBcCU2z8TrisoyY1 )", "pk(): Key ' L4rK1yDtCWekvXuE6oXD9jCYfFNV2cWRpVuPLBcCU2z8TrisoyY1 ' is invalid due to whitespace");

rkrux commented at 12:16 PM on February 18, 2025:

WIF private keys are passed in the second argument of CheckUnparsable, which accepts a public key const std::string& pub instead. The comment also mentions pubkeys instead of privkeys.

A simple swapping of the arguments doesn't work because the error of the second argument overwrites the error of the first argument making the CheckUnparsable() brittle.

brunoerg commented at 2:28 PM on February 18, 2025:

Done. I changed pubkeys to keys since it can affect both ones.

in doc/release-notes-31603.md:5 in bdc6ac10ca outdated

   0 | @@ -0,0 +1,6 @@
   1 | +Updated RPCs
   2 | +--------
   3 | +
   4 | +- Any RPC in which one of the parameters are descriptors will throw an error
   5 | +if the provided descriptor contains an whitespace at the beginning or end of

rkrux commented at 12:18 PM on February 18, 2025:

contains a whitespace at the beginning or the end of

brunoerg commented at 2:28 PM on February 18, 2025:

Done.

in test/functional/rpc_getdescriptorinfo.py:1 in 5fd1686f02 outdated

rkrux commented at 12:21 PM on February 18, 2025:

Can add 2 more assertions: one where the middle key in a multi descriptor contains a whitespace, another that tests on a private key.

        assert_raises_rpc_error(-5,
                                "Multi: Key ' 03774ae7f858a9411e5ef4246b70c65aac5649980be5c17891bbec17895da008cb' is invalid due to whitespace",
                                self.nodes[0].getdescriptorinfo,
                                "wsh(multi(2,03a0434d9e47f3c86235477c7b1ae6ae5d3442d49b1943c2b752a68e2a47e247c7, 03774ae7f858a9411e5ef4246b70c65aac5649980be5c17891bbec17895da008cb,03d01115d548e7561b15c38f004d734633687cf4419620095bc5b0f47070afe85a))")
        priv_key = generate_keypair(wif=True)[0]
        assert_raises_rpc_error(-5,
                                f"pk(): Key ' {priv_key}' is invalid due to whitespace",
                                self.nodes[0].getdescriptorinfo,
                                f"pk( {priv_key})")

brunoerg commented at 2:28 PM on February 18, 2025:

Nice, done.

rkrux commented at 12:10 PM on February 24, 2025:

Due to Base58, pubkeys with whitespace at the beginning end/or at the end are successfully parsed. This commit add a parameter into ParsePubkeyInner to control whether it should return an error if the first or last char of the pubkey is a space.

Comment about the commit message: The latest code doesn't contain any parameter in ParsePubkeyInner that controls if the error should be returned.

Also, pubkeys with whitespace should be changed to keys with whitespace as private keys are also affected and pubkeys barring extended keys are not parsed successfully currently in master.

brunoerg commented at 12:59 PM on February 24, 2025:

The commit message was outdated, I just pushed fixing it, thanks for noticing!

rkrux commented at 12:38 PM on February 18, 2025: contributor

Somewhere either in the PR title or description, it should be mentioned that this works for descriptors containing private keys as well as observed in the tests and in the ParsePubKeyInner code: https://github.com/bitcoin/bitcoin/blob/28dec6c5f8bd35ef4e6cb8d7aa3f21b6879acf98/src/script/descriptor.cpp#L1540-L1550

sipa commented at 12:42 PM on February 18, 2025: member

utACK bdc6ac10caf92901657b4d95bf2c11cccc8ac9ea

It's unfortunate that this in theory breaks RPC importing for (very limited) positions where spaces were allowed before, but it makes sense for consistency. Given that wallets always store descriptors in normalized form, I don't believe this will result in wallet loading breaking.

brunoerg force-pushed on Feb 18, 2025

brunoerg renamed this:
~~descriptor: check whitespace in pubkeys within fragments~~
descriptor: check whitespace in keys within fragments
on Feb 18, 2025

brunoerg commented at 2:30 PM on February 18, 2025: contributor

Force-pushed addressing #31603 (review), #31603 (review) and #31603 (review).

It adds 2 more test cases and fix a comment. Thanks, @rkrux.

in src/script/descriptor.cpp:1512 in 71d7901613 outdated

1508 | @@ -1509,6 +1509,10 @@ std::vector<std::unique_ptr<PubkeyProvider>> ParsePubkeyInner(uint32_t key_exp_i
1509 |          error = "No key provided";
1510 |          return {};
1511 |      }
1512 | +    if (IsSpace(str.front()) || IsSpace(str.back())) {

darosior commented at 3:24 PM on February 18, 2025:

(comment about the commit, not this line)

nit: in commit e91a9cf9d2ca50ee31ed36a975d2e01faa666c92 you state:

Due to Base58, pubkeys with whitespace at the beginning end/or at the end are successfully parsed.

But pubkeys are never base58 encoded.

darosior commented at 3:53 PM on February 18, 2025:

Interestingly this line is correct only because we check IsHex before ParseHex below, as otherwise we would need to check for any space character across the string since ParseHex would skip those even if they happen in the middle.

Checking for a space character at any position would also improve the error message for instance when parsing pk(03 a34b...). But no need to retouch.

brunoerg commented at 4:41 PM on February 18, 2025:

Ok.

brunoerg commented at 4:45 PM on February 18, 2025:

I meant due to DecodeExt{Pub}Key which calls DecodeBase58Check.

rkrux commented at 3:47 PM on February 18, 2025: contributor

One way to ensure code structure correctness and get rid of the brittleness noticed here (https://github.com/bitcoin/bitcoin/pull/31603#discussion_r1959636120) is to incorporate the following change. In a cursory attempt, I have this that has minimal diff that passes the tests. LMK how does this sound.

void CheckUnparsable(const std::string& prv, const std::string& pub, const std::string& expected_error, bool check_prv_error_only = false)
{
    FlatSigningProvider keys_priv, keys_pub;
    std::string error;
    auto parse_priv = Parse(prv, keys_priv, error);
    BOOST_CHECK_MESSAGE(parse_priv.empty(), prv);

    if (check_prv_error_only) {
       BOOST_CHECK_EQUAL(error, expected_error);
       return;
    }
    
    auto parse_pub = Parse(pub, keys_pub, error);
    BOOST_CHECK_MESSAGE(parse_pub.empty(), pub);
    BOOST_CHECK_EQUAL(error, expected_error);
}

The function call site with the private key descriptor passed in the correct function argument.

CheckUnparsable("multi(1, L4rK1yDtCWekvXuE6oXD9jCYfFNV2cWRpVuPLBcCU2z8TrisoyY1,5KYZdUEo39z3FPrtuX2QbbwGnNP5zTd7yyr2SC1j299sBCnWjss)", "", "Multi: Key ' L4rK1yDtCWekvXuE6oXD9jCYfFNV2cWRpVuPLBcCU2z8TrisoyY1' is invalid due to whitespace", /* check_prv_error_only*/ true);
CheckUnparsable("pk(L4rK1yDtCWekvXuE6oXD9jCYfFNV2cWRpVuPLBcCU2z8TrisoyY1 )", "", "pk(): Key 'L4rK1yDtCWekvXuE6oXD9jCYfFNV2cWRpVuPLBcCU2z8TrisoyY1 ' is invalid due to whitespace", /* check_prv_error_only*/ true);
CheckUnparsable("pk( L4rK1yDtCWekvXuE6oXD9jCYfFNV2cWRpVuPLBcCU2z8TrisoyY1 )", "", "pk(): Key ' L4rK1yDtCWekvXuE6oXD9jCYfFNV2cWRpVuPLBcCU2z8TrisoyY1 ' is invalid due to whitespace", /* check_prv_error_only*/ true);

Edit: I will verify again its correctness once I am back online.

darosior approved

darosior commented at 3:55 PM on February 18, 2025: member

ACK 35d5adbcd58e04991bb4651d2dd97af6e186d1f9

DrahtBot requested review from sipa on Feb 18, 2025

darosior commented at 3:57 PM on February 18, 2025: member

is to incorporate the following change.

Improving the descriptor unit test utilities can be done in a followup. It would be nice to get the user-facing fix in first.

brunoerg commented at 4:45 PM on February 18, 2025: contributor

is to incorporate the following change.

Improving the descriptor unit test utilities can be done in a followup. It would be nice to get the user-facing fix in first.

Yes, any other minor/test stuff I will leave for a follow-up.

rkrux commented at 3:16 PM on February 19, 2025: contributor

is to incorporate the following change.

Improving the descriptor unit test utilities can be done in a followup. It would be nice to get the user-facing fix in first.

Yes, any other minor/test stuff I will leave for a follow-up.

A separate PR is fine. I raised this point because this PR introduces a way of using CheckUnparsable in a way that leads to some difficulty in reading the test.

rkrux commented at 3:17 PM on February 19, 2025: contributor

Review @ 35d5adb

I reviewed range-diff

git range-diff bdc6ac1...35d5adb

Newer changes are fixing adding new assertions in the functional tests and improving comments.

I find the new error more detailed and helpful compared to the previous one.

➜  src git:(master) ✗ bitcoinclireg getdescriptorinfo "multi(1, L4rK1yDtCWekvXuE6oXD9jCYfFNV2cWRpVuPLBcCU2z8TrisoyY1,5KYZdUEo39z3FPrtuX2QbbwGnNP5zTd7yyr2SC1j299sBCnWjss)"
error code: -5
error message:
Multi: key ' L4rK1yDtCWekvXuE6oXD9jCYfFNV2cWRpVuPLBcCU2z8TrisoyY1' is not valid
➜  src git:(2025-01-descriptor-pk) ✗ bitcoinclireg getdescriptorinfo "multi(1, L4rK1yDtCWekvXuE6oXD9jCYfFNV2cWRpVuPLBcCU2z8TrisoyY1,5KYZdUEo39z3FPrtuX2QbbwGnNP5zTd7yyr2SC1j299sBCnWjss)"
error code: -5
error message:
Multi: Key ' L4rK1yDtCWekvXuE6oXD9jCYfFNV2cWRpVuPLBcCU2z8TrisoyY1' is invalid due to whitespace

Re: #31603#pullrequestreview-2623448041 I do need to spend some time in understanding how storing descs in normalised form helps in the wallet loading not breaking, will verify and then approve.

rkrux commented at 2:03 PM on February 20, 2025: contributor

Currently, we successfully parse descriptors which contains spaces in the beginning or end of the public/private key within a fragment

On master, I am not able to successfully import a descriptor or get the descriptor info in case a space is passed in the desc. Is this statement incorrect or is there any other flow I don't know about?

<details> <summary> `getdescriptorinfo` and `importdescriptors` with both public key and private keys on master </summary>

➜  src git:(master) ✗ bitcoinclireg getdescriptorinfo "multi(1, L4rK1yDtCWekvXuE6oXD9jCYfFNV2cWRpVuPLBcCU2z8TrisoyY1,5KYZdUEo39z3FPrtuX2QbbwGnNP5zTd7yyr2SC1j299sBCnWjss)"
error code: -5
error message:
Multi: key ' L4rK1yDtCWekvXuE6oXD9jCYfFNV2cWRpVuPLBcCU2z8TrisoyY1' is not valid

➜  src git:(master) ✗ bitcoinclireg getdescriptorinfo "pkh( 02df706d76d09602b3b34a845ae611c2d77449a6a67fc911053d6c39728b697ddf)#atgzwtd5"
error code: -5
error message:
pkh(): key ' 02df706d76d09602b3b34a845ae611c2d77449a6a67fc911053d6c39728b697ddf' is not valid


➜  src git:(master) ✗ bitcoinclireg importdescriptors "[{\"desc\":\"pkh( 02df706d76d09602b3b34a845ae611c2d77449a6a67fc911053d6c39728b697ddf)#atgzwtd5\",\"timestamp\":\"now\"}]"
[
  {
    "success": false,
    "error": {
      "code": -5,
      "message": "pkh(): key ' 02df706d76d09602b3b34a845ae611c2d77449a6a67fc911053d6c39728b697ddf' is not valid"
    }
  }
]

➜  src git:(master) ✗ bitcoinclireg importdescriptors "[{\"desc\":\"pkh( L4rK1yDtCWekvXuE6oXD9jCYfFNV2cWRpVuPLBcCU2z8TrisoyY1)#e58j46cd\",\"timestamp\":\"now\"}]"
[
  {
    "success": false,
    "error": {
      "code": -5,
      "message": "pkh(): key ' L4rK1yDtCWekvXuE6oXD9jCYfFNV2cWRpVuPLBcCU2z8TrisoyY1' is not valid"
    }
  }
]

</details>

brunoerg commented at 7:04 PM on February 21, 2025: contributor

Currently, we successfully parse descriptors which contains spaces in the beginning or end of the public/private key within a fragment

On master, I am not able to successfully import a descriptor or get the descriptor info in case a space is passed in the desc. Is this statement incorrect or is there any other flow I don't know about?

getdescriptorinfo and importdescriptors with both public key and private keys on master @rkrux Some examples I just tried:

./build/src/bitcoin-cli getdescriptorinfo "sh(multi(2, 5KASdZKHt7MBqD6tDHTyTsLMfX6E8TXfygKanpPsi5T5xxV7NWR, L1uf6enQ3adcTFdTsiU9ESFpifcYudGS7dh9NhMxUJZZoGvMwh4A))"
{
  "descriptor": "sh(multi(2,04ca1af27a61a0bb32210056e14cf4a9af7429ceebacab06fe8366a020c758395aeb0560daeba81a705c3ae537ca37f3a254bc9a95de1eab897c7ae17c7ccee425,03cacf75b5d27844a3181275c523baf8069707f53f894967d3b549219d0b5880b7))#q6gqlkrs",
  "checksum": "gt4hkzw7",
  "isrange": false,
  "issolvable": true,
  "hasprivatekeys": true
}

./build/src/bitcoin-cli getdescriptorinfo "pk(L4rK1yDtCWekvXuE6oXD9jCYfFNV2cWRpVuPLBcCU2z8TrisoyY1 )"
{
  "descriptor": "pk(03a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd)#9pcxlpvx",
  "checksum": "gzu3vsx5",
  "isrange": false,
  "issolvable": true,
  "hasprivatekeys": true
}

in doc/release-notes-31603.md:5 in 35d5adbcd5 outdated

   0 | @@ -0,0 +1,6 @@
   1 | +Updated RPCs
   2 | +--------
   3 | +
   4 | +- Any RPC in which one of the parameters are descriptors will throw an error
   5 | +if the provided descriptor contains an whitespace at the beginning or the end

rkrux commented at 1:46 PM on February 22, 2025:

In case you retouch

contains a whitespace

brunoerg commented at 12:58 PM on February 24, 2025:

Done.

rkrux commented at 2:22 PM on February 22, 2025: contributor

Re: #31603 (comment)

My CLI command called the regtest node in which those mainnet valid private keys failed to parse, which is correct behaviour. I tried getting that descriptor info in the mainnet and was able to do so in master, which is erroneous behaviour, and I see the newly added error in this branch when I try to get the descriptor info or import it.

I noticed that I could import a space containing descriptor in master but when I listed the descriptor it naturally contained the key without space and the checksum of the descriptor changed as well, rightfully so. But this behaviour is inconsistent with user expectations. Failing the descriptor RPCs when a descriptor with space is provided seems to be the right approach.

<details> <summary> Examples in master - one each with pub and WIF priv </summary>

➜  bitcoin git:(master) ✗ bitcoinclireg getdescriptorinfo "wpkh( tpubDCw3vMU3FSDX7txrkYJ4WMyS8urS4EtwZ3hXGKBg29WcuN2BQ7i4VeCtG16BdgwgLZdjajL36PWj5xMSYQEHooJAdHZhyVogLUfumecqNf6/0/*)#rwhpmnvw"
{
  "descriptor": "wpkh(tpubDCw3vMU3FSDX7txrkYJ4WMyS8urS4EtwZ3hXGKBg29WcuN2BQ7i4VeCtG16BdgwgLZdjajL36PWj5xMSYQEHooJAdHZhyVogLUfumecqNf6/0/*)#ud465w0w",
  "checksum": "rwhpmnvw",
  "isrange": true,
  "issolvable": true,
  "hasprivatekeys": false
}
➜  bitcoin git:(master) ✗ bitcoinclireg getdescriptorinfo "pkh(cPksSFRHeu9Aah9qYxFkvPqJBgJ4uEhRpjeGwFEH8BS9gSevd3QT )#hwxvtlaa"
{
  "descriptor": "pkh(020c540089aaabec12258e4ea068b1e95a12a87857c8bd10e1e297c6ad11b5609e)#0sd2vs6c",
  "checksum": "hwxvtlaa",
  "isrange": false,
  "issolvable": true,
  "hasprivatekeys": true
}

</details>

I will test with few more keys.

rkrux commented at 12:40 PM on February 24, 2025: contributor

Ok, I tested the behaviour on master with more keys and found that the RPC was successful for space containing WIF private keys, xprvs, ans xpubs, but a space containing pubkey returned an error related to invalidity as seen in the below examples.

For a space containing pubkey, in the code all the IsHex(), DecodeSecret(), DecodeExtKey(), and DecodeExtPubKey() functions fail because of the space, the 33/34 bytes size check, presence of 0, presence of 0 respectively. Thus, the key '%s' is not valid error is thrown.

Concept ACK 35d5adbcd58e04991bb4651d2dd97af6e186d1f9

I will ack once the commit message for 71d7901613988a43e6faee5deeb8c004c2bf5059 is updated as it is outdated.

<details> <summary> Examples tested on master </summary>

➜  bitcoin git:(master) ✗ bitcoinclireg getdescriptorinfo "pkh( tprv8ZgxMBicQKsPeNLUGrbv3b7qhUk1LQJZAGMuk9gVuKh9sd4BWGp1eMsehUni6qGb8bjkdwBxCbgNGdh2bYGACK5C5dRTaif9KBKGVnSezxV/1)#qwgdxsx8"
{
  "descriptor": "pkh(tpubD6NzVbkrYhZ4XqNGAWGWSzmxGWFwVjVTjZxh2fioKbVYi7Jx8fdbprVWsdW7mHwqjchBVas8TLZG4Xwuz4RKU4iaCqiCvoSkFCzQptqk5Y1/1)#4uzxxr48",
  "checksum": "qwgdxsx8",
  "isrange": false,
  "issolvable": true,
  "hasprivatekeys": true
}
➜  bitcoin git:(master) ✗ bitcoinclireg getdescriptorinfo "pkh( tpubD6NzVbkrYhZ4XqNGAWGWSzmxGWFwVjVTjZxh2fioKbVYi7Jx8fdbprVWsdW7mHwqjchBVas8TLZG4Xwuz4RKU4iaCqiCvoSkFCzQptqk5Y1/1)#vwk8d6fa"
{
  "descriptor": "pkh(tpubD6NzVbkrYhZ4XqNGAWGWSzmxGWFwVjVTjZxh2fioKbVYi7Jx8fdbprVWsdW7mHwqjchBVas8TLZG4Xwuz4RKU4iaCqiCvoSkFCzQptqk5Y1/1)#4uzxxr48",
  "checksum": "vwk8d6fa",
  "isrange": false,
  "issolvable": true,
  "hasprivatekeys": false
}
➜  bitcoin git:(master) ✗ bitcoinclireg getdescriptorinfo "pkh( 92MC1SSqgEqXtAQBtirfWsBFsamrY9hGq5S9n4XdRz3Wq9iS7Js)#qffggmax"
{
  "descriptor": "pkh(04a4f476c536dffa2454145e16e106e0511bfd842cc512e5f1ca761184301db1a84de2cc560cec7ddb309ac94f52d74717bd9595fa1c1fbb8aec37cb9fed551e50)#4nn9zvsj",
  "checksum": "qffggmax",
  "isrange": false,
  "issolvable": true,
  "hasprivatekeys": true
}
➜  bitcoin git:(master) ✗ bitcoinclireg getdescriptorinfo "pkh( 04a4f476c536dffa2454145e16e106e0511bfd842cc512e5f1ca761184301db1a84de2cc560cec7ddb309ac94f52d74717bd9595fa1c1fbb8aec37cb9fed551e50)#p9pgdcsv"
error code: -5
error message:
pkh(): key ' 04a4f476c536dffa2454145e16e106e0511bfd842cc512e5f1ca761184301db1a84de2cc560cec7ddb309ac94f52d74717bd9595fa1c1fbb8aec37cb9fed551e50' is not valid

</details>

descriptor: check whitespace in ParsePubkeyInner

Due to Base58, keys with whitespace at the beginning or
at the end are successfully parsed. This commit adds a
check into `ParsePubkeyInner` to verify whether if the
first or last char of the key is a space.

cb722a3cea

test: descriptor: check whitespace into keys c7afca3d62

test: `getdescriptorinfo`/`importdescriptors` with whitespace in pubkeys a8b548d75d

docs: add release notes for 31603 21e9d39a37

brunoerg force-pushed on Feb 24, 2025

brunoerg commented at 1:00 PM on February 24, 2025: contributor

Force-pushed addressing #31603 (review) and #31603 (review). Fixed an outdated commit message.

rkrux approved

rkrux commented at 1:28 PM on February 24, 2025: contributor

tACK 21e9d39a3725cd6107b742f0cb97f65b3640201b

Thanks for addressing the feedback.

DrahtBot requested review from darosior on Feb 24, 2025

brunoerg commented at 2:17 PM on February 24, 2025: contributor

CI failure is unrelated.

brunoerg commented at 1:55 PM on March 4, 2025: contributor

friendly ping: @sipa @darosior

darosior approved

darosior commented at 4:10 PM on March 4, 2025: member

re-ACK 21e9d39a3725cd6107b742f0cb97f65b3640201b

sipa commented at 4:34 PM on March 4, 2025: member

utACK 21e9d39a3725cd6107b742f0cb97f65b3640201b

brunoerg commented at 9:55 PM on March 12, 2025: contributor

rfm?

fanquake assigned achow101 on Mar 13, 2025

ryanofsky commented at 3:52 PM on March 17, 2025: contributor

rfm?

Agree this looks ready and I could merge it, but it's assigned to @achow101 and she has the most expertise here and didn't comment on the PR at all yet, so I'd be inclined to wait. But feel free to ping me if anyone thinks this should be merged sooner.

Update: will merge now, got feedback it should be ok

ryanofsky assigned ryanofsky on Mar 18, 2025

ryanofsky merged this on Mar 18, 2025

ryanofsky closed this on Mar 18, 2025

brunoerg deleted the branch on Mar 18, 2025