descriptor: check whitespace in keys within fragments

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	darosior
Stale ACK	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. Debug: https://github.com/bitcoin/bitcoin/runs/35146721643

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.

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.

0strprintf(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

descriptor: check whitespace in ParsePubkeyInner

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.

71d7901613

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.

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

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

Nice, done.

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.

test: descriptor: check whitespace into keys e91a9cf9d2

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

docs: add release notes for 31603 35d5adbcd5

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.

 0void CheckUnparsable(const std::string& prv, const std::string& pub, const std::string& expected_error, bool check_prv_error_only = false)
 1{
 2    FlatSigningProvider keys_priv, keys_pub;
 3    std::string error;
 4    auto parse_priv = Parse(prv, keys_priv, error);
 5    BOOST_CHECK_MESSAGE(parse_priv.empty(), prv);
 6
 7    if (check_prv_error_only) {
 8       BOOST_CHECK_EQUAL(error, expected_error);
 9       return;
10    }
11    
12    auto parse_pub = Parse(pub, keys_pub, error);
13    BOOST_CHECK_MESSAGE(parse_pub.empty(), pub);
14    BOOST_CHECK_EQUAL(error, expected_error);
15}

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

0CheckUnparsable("multi(1, L4rK1yDtCWekvXuE6oXD9jCYfFNV2cWRpVuPLBcCU2z8TrisoyY1,5KYZdUEo39z3FPrtuX2QbbwGnNP5zTd7yyr2SC1j299sBCnWjss)", "", "Multi: Key ' L4rK1yDtCWekvXuE6oXD9jCYfFNV2cWRpVuPLBcCU2z8TrisoyY1' is invalid due to whitespace", /* check_prv_error_only*/ true);
1CheckUnparsable("pk(L4rK1yDtCWekvXuE6oXD9jCYfFNV2cWRpVuPLBcCU2z8TrisoyY1 )", "", "pk(): Key 'L4rK1yDtCWekvXuE6oXD9jCYfFNV2cWRpVuPLBcCU2z8TrisoyY1 ' is invalid due to whitespace", /* check_prv_error_only*/ true);
2CheckUnparsable("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

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

0➜  src git:(master) ✗ bitcoinclireg getdescriptorinfo "multi(1, L4rK1yDtCWekvXuE6oXD9jCYfFNV2cWRpVuPLBcCU2z8TrisoyY1,5KYZdUEo39z3FPrtuX2QbbwGnNP5zTd7yyr2SC1j299sBCnWjss)"
1error code: -5
2error message:
3Multi: key ' L4rK1yDtCWekvXuE6oXD9jCYfFNV2cWRpVuPLBcCU2z8TrisoyY1' is not valid
4➜  src git:(2025-01-descriptor-pk) ✗ bitcoinclireg getdescriptorinfo "multi(1, L4rK1yDtCWekvXuE6oXD9jCYfFNV2cWRpVuPLBcCU2z8TrisoyY1,5KYZdUEo39z3FPrtuX2QbbwGnNP5zTd7yyr2SC1j299sBCnWjss)"
5error code: -5
6error message:
7Multi: 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?

 0➜  src git:(master) ✗ bitcoinclireg getdescriptorinfo "multi(1, L4rK1yDtCWekvXuE6oXD9jCYfFNV2cWRpVuPLBcCU2z8TrisoyY1,5KYZdUEo39z3FPrtuX2QbbwGnNP5zTd7yyr2SC1j299sBCnWjss)"
 1error code: -5
 2error message:
 3Multi: key ' L4rK1yDtCWekvXuE6oXD9jCYfFNV2cWRpVuPLBcCU2z8TrisoyY1' is not valid
 4
 5➜  src git:(master) ✗ bitcoinclireg getdescriptorinfo "pkh( 02df706d76d09602b3b34a845ae611c2d77449a6a67fc911053d6c39728b697ddf)#atgzwtd5"
 6error code: -5
 7error message:
 8pkh(): key ' 02df706d76d09602b3b34a845ae611c2d77449a6a67fc911053d6c39728b697ddf' is not valid
 9
10
11➜  src git:(master) ✗ bitcoinclireg importdescriptors "[{\"desc\":\"pkh( 02df706d76d09602b3b34a845ae611c2d77449a6a67fc911053d6c39728b697ddf)#atgzwtd5\",\"timestamp\":\"now\"}]"
12[
13  {
14    "success": false,
15    "error": {
16      "code": -5,
17      "message": "pkh(): key ' 02df706d76d09602b3b34a845ae611c2d77449a6a67fc911053d6c39728b697ddf' is not valid"
18    }
19  }
20]
21
22➜  src git:(master) ✗ bitcoinclireg importdescriptors "[{\"desc\":\"pkh( L4rK1yDtCWekvXuE6oXD9jCYfFNV2cWRpVuPLBcCU2z8TrisoyY1)#e58j46cd\",\"timestamp\":\"now\"}]"
23[
24  {
25    "success": false,
26    "error": {
27      "code": -5,
28      "message": "pkh(): key ' L4rK1yDtCWekvXuE6oXD9jCYfFNV2cWRpVuPLBcCU2z8TrisoyY1' is not valid"
29    }
30  }
31]

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:

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

0./build/src/bitcoin-cli getdescriptorinfo "pk(L4rK1yDtCWekvXuE6oXD9jCYfFNV2cWRpVuPLBcCU2z8TrisoyY1 )"
1{
2  "descriptor": "pk(03a34b99f22c790c4e36b2b3c2c35a36db06226e41c692fc82b8b56ac1c540c5bd)#9pcxlpvx",
3  "checksum": "gzu3vsx5",
4  "isrange": false,
5  "issolvable": true,
6  "hasprivatekeys": true
7}

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

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

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.

 0➜  bitcoin git:(master) ✗ bitcoinclireg getdescriptorinfo "wpkh( tpubDCw3vMU3FSDX7txrkYJ4WMyS8urS4EtwZ3hXGKBg29WcuN2BQ7i4VeCtG16BdgwgLZdjajL36PWj5xMSYQEHooJAdHZhyVogLUfumecqNf6/0/*)#rwhpmnvw"
 1{
 2  "descriptor": "wpkh(tpubDCw3vMU3FSDX7txrkYJ4WMyS8urS4EtwZ3hXGKBg29WcuN2BQ7i4VeCtG16BdgwgLZdjajL36PWj5xMSYQEHooJAdHZhyVogLUfumecqNf6/0/*)#ud465w0w",
 3  "checksum": "rwhpmnvw",
 4  "isrange": true,
 5  "issolvable": true,
 6  "hasprivatekeys": false
 7}
 8➜  bitcoin git:(master) ✗ bitcoinclireg getdescriptorinfo "pkh(cPksSFRHeu9Aah9qYxFkvPqJBgJ4uEhRpjeGwFEH8BS9gSevd3QT )#hwxvtlaa"
 9{
10  "descriptor": "pkh(020c540089aaabec12258e4ea068b1e95a12a87857c8bd10e1e297c6ad11b5609e)#0sd2vs6c",
11  "checksum": "hwxvtlaa",
12  "isrange": false,
13  "issolvable": true,
14  "hasprivatekeys": true
15}

I will test with few more keys.

descriptor: check whitespace in keys within fragments #31603

Code Coverage & Benchmarks

Reviews

Conflicts