Closes #31728
This PR updates the DescriptorScriptPubKeyMan to skip range checks for non-ranged descriptors, which previously caused errors when updating a non-ranged descriptor with the range [0,0]
A unit test was added to test the new behaviour
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32344.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | rkrux, achow101 |
| Concept ACK | w0xlt, shahsb |
| Stale ACK | BrandonOdiwuor |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/41113093377
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.
Concept ACK.
Functional tests might be necessary.
Non-range desc are always added to the wallet with the range [0,0]. After the descriptor is added, the wallet will TopUp the keypool. For non-range descriptors, this process updates the desc range to [0,1].
Any attempts to update this non-range descriptor with a [0,0] range will result in an error because the range checks rejects new ranges not included in the old range.
Since this is a non-range desc, the range information should be disregarded and AddWalletDescriptor should always succeed regardless of provided range information
Functional tests might be necessary.
The importdescriptors RPC sets the range for all non-ranged descriptors to [0,1] in every request so this error cannot be triggered using the RPC.
In #31728 @JeremyRubin explains that this error may be unexpectedly triggered when " experimenting with non-ranged descriptors in custom wallet workflows."
86+ FlatSigningProvider provider;
87+ std::string error;
88+ auto descs{Parse(desc_str, provider, error, /* require_checksum=*/ false)};
89+ auto& desc{descs.at(0)};
90+ WalletDescriptor w_desc{std::move(desc), 0, 0, 0, 0};
91+ auto spk_manager{*Assert(wallet.AddWalletDescriptor(w_desc, provider, "", false))};
DescriptorScriptPubKeyMan::CanUpdateToWalletDescriptor() being made in this test case?
The importdescriptors RPC sets the range for all non-ranged descriptors to [0,1] in every request so this error cannot be triggered using the RPC.
I checked that the importdescriptors adds a 1 to range end if a range is given in the request.
that this error may be unexpectedly triggered when " experimenting with non-ranged descriptors in custom wallet workflows.
The issue seems quite peculiar in that it highlights an issue within an internal function instead of the full functionality, makes sense to verify the fix using a unit test.
79+ {
80+ LOCK(wallet.cs_wallet);
81+ wallet.SetWalletFlag(WALLET_FLAG_DESCRIPTORS);
82+ auto key{GenerateRandomKey()};
83+ auto desc_str{"combo(" + EncodeSecret(key) + ")"};
84+ for (size_t i = 0; i < 2; i++)
AddWalletDescriptor multiple times with the same descriptor.
2851@@ -2852,6 +2852,11 @@ bool DescriptorScriptPubKeyMan::CanUpdateToWalletDescriptor(const WalletDescript
2852 return false;
2853 }
2854
2855+ if (!descriptor.descriptor->IsRange()) {
2856+ // Skip range check for non-range descriptors
2857+ return true;
2858+ }
2859+
2860 if (descriptor.range_start > m_wallet_descriptor.range_start ||
else if would be nice because these two are conceptually related, although not necessary because the first if returns.
Concept ACK fa0993462b3bc8db40c1a6ea2e7f5fd4a22353a6
I notice while running the unit tests that it warns about a lack of assertions. Maybe it’s expecting a boost check?
0wallet/test/wallet_tests.cpp:76: Entering test case "update_non_range_descriptor"
1Test case wallet_tests/update_non_range_descriptor did not check any assertions
2wallet/test/wallet_tests.cpp:76: Leaving test case "update_non_range_descriptor"; testing time: 38813us
Rebased https://github.com/bitcoin/bitcoin/pull/32344/commits/fa0993462b3bc8db40c1a6ea2e7f5fd4a22353a6 to https://github.com/bitcoin/bitcoin/pull/32344/commits/97d383af6d54b463da64f45680a146d7e93eb146
Simplified “update non-range descriptor” unit test and fixed this warning:
I notice while running the unit tests that it warns about a lack of assertions. Maybe it’s expecting a boost check?
0wallet/test/wallet_tests.cpp:76: Entering test case "update_non_range_descriptor" 1Test case wallet_tests/update_non_range_descriptor did not check any assertions 2wallet/test/wallet_tests.cpp:76: Leaving test case "update_non_range_descriptor"; testing time: 38813us
86+ auto descs{Parse(desc_str, provider, error, /* require_checksum=*/ false)};
87+ auto& desc{descs.at(0)};
88+ WalletDescriptor w_desc{std::move(desc), 0, 0, 0, 0};
89+ BOOST_CHECK(wallet.AddWalletDescriptor(w_desc, provider, "", false));
90+ // Wallet should update the non-range descriptor successfully
91+ BOOST_CHECK(wallet.AddWalletDescriptor(w_desc, provider, "", false));
Nice, I did consider removing the loop in the previous review but decided against it so as to simulate a similar flow like this one in a loop: https://github.com/bitcoin/bitcoin/blob/64697529523705f5a411ca893060429afcd5cd47/src/wallet/wallet.cpp#L4131-L4132
However, I did check now that in GetDescriptorScriptPubKeyMan the ScriptPubKeyMan is gotten via descriptor.id, which would remain same whether the w_desc reference is same or not.
https://github.com/bitcoin/bitcoin/blob/64697529523705f5a411ca893060429afcd5cd47/src/wallet/wallet.cpp#L3686-L3688
So, overall fine with this approach.
It would be good to describe the steps to replicate the issue via RPC. Including a range in a non-ranged descriptor import should cause the process to fail before reaching the wallet descriptor update function.
This step-by-step description would also allow us to convert the unit test into a functional test, which would be ideal, as it’s simpler to maintain.
Including a range in a non-ranged descriptor import should cause the process to fail before reaching the wallet descriptor update function.
AFAICT this will cause importdescriptor to throw an error with the message “Range should not be specified for an un-ranged descriptor”
Including a range in a non-ranged descriptor import should cause the process to fail before reaching the wallet descriptor update function.
AFAICT this will cause
importdescriptorto throw an error with the message “Range should not be specified for an un-ranged descriptor”
Then how can the user trigger the error being solved here? If it can’t be triggered by the user, we’re talking about an extra guard rather than a fix — which is fine as well, but it would be better to label the PR differently, since it won’t need to be backported.
Then how can the user trigger the error being solved here? If it can’t be triggered by the user, we’re talking about an extra guard rather than a fix — which is fine as well, but it would be better to label the PR differently, since it won’t need to be backported.
Agree that the PR title can be labeled differently.
When attempting to add a non-ranged descriptor with a start/end range of [0,0] via AddWalletDescriptor, an error may appear
The issue #31728 description states that it is triggered via AddWalletDescriptor, which is an internal function & not exposed to the end user. I agree with the expectation set in the issue and this corresponding fix does seem to improve the internal functions and their understanding.
it can lead to confusion and error messages when using or experimenting with non-ranged descriptors in custom wallet workflows.
I feel that the issue need not be labeled as a ‘Bug’ because the author mentions it is triggered while experimenting with custom flows.