wallet: removed duplicate call to GetDescriptorScriptPubKeyMan #32023

pull saikiran57 wants to merge 1 commits into bitcoin:master from saikiran57:remove_duplicate_getdescriptorscriptpubkeyman changing 12 files +45 −29
  1. saikiran57 commented at 9:53 am on March 10, 2025: none

    Removed duplicate call to GetDescriptorScriptPubKeyMan and Instead of checking linearly I have used find method so time complexity reduced significantly for GetDescriptorScriptPubKeyMan after this fix improved performance of importdescriptor part refs #32013.

    Steps to reproduce in testnet environment

    Input size: 2 million address in the wallet

    Step1: call importaddresdescriptor rpc method observe the time it has taken.

    With the provided fix: Do the same steps again observe the time it has taken.

    There is a huge improvement in the performance. (previously it may take 5 to 6 seconds now it will take 1 seconds or less)

    main changes i’ve made during this pr:

    1. remove duplicate call to GetDescriptorScriptPubKeyMan method
    2. And inside GetDescriptorScriptPubKeyMan method previously we checking each address linearly so each time it is calling HasWallet method which has aquired lock.
    3. Now i’ve modified this logic call find method on the map (O(logn)) time it is taking, so only once we calling HasWallet method.

    Note: Smaller inputs in the wallet you may not see the issue but huge wallet size it will definitely impact the performance.

  2. DrahtBot commented at 9:53 am on March 10, 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/32023.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    Stale ACK furszy

    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:

    • #30343 (wallet, logging: Replace WalletLogPrintf() with LogInfo() by ryanofsky)
    • #28333 (wallet: Construct ScriptPubKeyMans with all data rather than loaded progressively 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.

  3. DrahtBot renamed this:
    removed duplicate call to GetDescriptorScriptPubKeyMan
    removed duplicate call to GetDescriptorScriptPubKeyMan
    on Mar 10, 2025
  4. in src/wallet/wallet.cpp:3960 in d53f4d8b43 outdated 
    3951@@ -3950,8 +3952,13 @@ ScriptPubKeyMan* CWallet::AddWalletDescriptor(WalletDescriptor& desc, const Flat
3952         return nullptr;
3953     }
3954 
3955+    std::string error;
3956     auto spk_man = GetDescriptorScriptPubKeyMan(desc);
3957     if (spk_man) {
3958+        if (!spk_man->CanUpdateToWalletDescriptor(desc, error)) {
3959+            WalletLogPrintf("Invalid param: %s\n", error);
3960+            throw JSONRPCError(RPC_INVALID_PARAMETER, error);
    furszy commented at 1:42 pm on March 10, 2025:
    This introduces a cyclic dependency. Please do not add RPC dependencies to the wallet. These are two separate components, and the wallet operates at a lower level.
    pablomartin4btc commented at 3:46 pm on March 10, 2025:

    Shouldn’t return null as other validations like the previous one (IsWalletFlagSet)?

    0            return nullptr;

    Moreover, DescriptorScriptPubKeyMan::UpdateWalletDescriptor also calls DescriptorScriptPubKeyMan::CanUpdateToWalletDescriptor… could this call be removed?

    saikiran57 commented at 7:49 am on March 11, 2025:
    your right CanUpdateToWalletDescriptor removed completely no need now. I’ve used UpdateWalletDescriptor only.
  5. laanwj added the label Wallet on Mar 10, 2025
  6. in src/wallet/wallet.cpp:3959 in d53f4d8b43 outdated 
    3951@@ -3950,8 +3952,13 @@ ScriptPubKeyMan* CWallet::AddWalletDescriptor(WalletDescriptor& desc, const Flat
3952         return nullptr;
3953     }
3954 
3955+    std::string error;
3956     auto spk_man = GetDescriptorScriptPubKeyMan(desc);
3957     if (spk_man) {
3958+        if (!spk_man->CanUpdateToWalletDescriptor(desc, error)) {
3959+            WalletLogPrintf("Invalid param: %s\n", error);
    furszy commented at 1:45 pm on March 10, 2025:
    No need to log the error message. It will be retrieved to the user via the RPC response.
    pablomartin4btc commented at 3:44 pm on March 10, 2025:
    But CWallet::AddWalletDescriptor could be called from other places (at the moment only tests and bench)? Doesn’t matter?
    saikiran57 commented at 7:50 am on March 11, 2025:
    there is no issue.
  7. mprenditore commented at 2:23 pm on March 12, 2025: none
    @furszy @pablomartin4btc Can this be merged or there’s still something missing?
  9. in src/wallet/scriptpubkeyman.cpp:2835 in e40d850403 outdated 
    2831@@ -2832,7 +2832,7 @@ void DescriptorScriptPubKeyMan::UpdateWalletDescriptor(WalletDescriptor& descrip
2832     LOCK(cs_desc_man);
2833     std::string error;
2834     if (!CanUpdateToWalletDescriptor(descriptor, error)) {
2835-        throw std::runtime_error(std::string(__func__) + ": " + error);
2836+        throw std::invalid_argument(error);
    furszy commented at 4:56 am on March 13, 2025:

    What about returning util::Result<void> instead and return the error message here? (see how we do it in other places).

    Also, if you’re going down this route, you should check any other usages of this function, as it would no longer be throwing an exception.

    And you would also have to return an util::Result<ScriptPubKeyMan*> in AddWalletDescriptor too. Which I think that is cleaner than catching the specific exception.

    saikiran57 commented at 10:51 am on March 13, 2025:

    My primary goal is to give fix with minor changes to code, that’s why I’ve updated this throw std::runtime_error(std::string(__func__) + ": " + error); to throw std::invalid_argument(error); so it does its satisfy the test case. During rpc call if we face issue I cached this exception(invalid_argument) return with existing response message to user if any other exception happened I just left with default implementation.

    Regarding your suggestion using util::Result<void>. Inside AddWalletDescriptor method we are calling this UpdateWalletDescriptor method if it throws exception currently I’m handing it rpc related calls, but AddWalletDescriptor is called from multiple places ex: data->solvable_wallet->AddWalletDescriptor(w_desc, keys, "", false); If i modify this then we need to handle in multiple places.

    So kindly suggest do I’ve to change to existing behavior.

    furszy commented at 2:22 pm on March 13, 2025:

    It depends on how much effort you want to put on it. Ideally, the function signature should clearly indicate if the procedure can fail or not. Exceptions should be reserved for unexpected situations. In this case, we’re throwing an specific exception in a wallet internal function solely to notify the user about an invalid parameter provided by the RPC interface. This creates unintended coupling between components (the RPC code needs to knows about the internal wallet function throwing this specific exception).

    If you don’t want to go into the util::Result<void> direction due to the extra work, you could leave this as an std::runtime_error and just change the error message so it does not print the function name. E.g. throw std::runtime_error("Error during descriptor update: " + error);. You don’t need to catch this at the RPC side neither; the std::runtime_error will be wrapped into JSONRPCError and returned to the user. Will just need to change the error return code.

    saikiran57 commented at 1:10 pm on March 14, 2025:
    hi @furszy can you please check the fix.
  10. furszy commented at 5:01 am on March 13, 2025: member
    Please try to avoid introducing and reverting changes within the same PR. The first two commits should be squashed. Aside from that, I left you a comment suggesting a possible alternative implementation.
  11. saikiran57 force-pushed on Mar 13, 2025
  12. saikiran57 renamed this:
    removed duplicate call to GetDescriptorScriptPubKeyMan
    wallet: removed duplicate call to GetDescriptorScriptPubKeyMan
    on Mar 13, 2025
  13. saikiran57 force-pushed on Mar 14, 2025
  14. in src/wallet/scriptpubkeyman.cpp:2835 in c986b43f0c outdated 
    2831@@ -2832,7 +2832,7 @@ void DescriptorScriptPubKeyMan::UpdateWalletDescriptor(WalletDescriptor& descrip
2832     LOCK(cs_desc_man);
2833     std::string error;
2834     if (!CanUpdateToWalletDescriptor(descriptor, error)) {
2835-        throw std::runtime_error(std::string(__func__) + ": " + error);
2836+        throw std::runtime_error(error);
    furszy commented at 2:28 pm on March 17, 2025:

    could make this more descriptive so that the user is aware of which specific descriptor failed when multiple are being imported.

    0throw std::runtime_error(strprintf("Can't update descriptor %s, error: %s", descriptor.descriptor->ToString(), error))

    This will need modifications in the test side too.

    saikiran57 commented at 3:25 pm on March 17, 2025:
    done
  15. furszy commented at 2:31 pm on March 17, 2025: member
    Code review ACK c986b43f0c45388b87106bcbf2534c75eebe7b72.
  16. saikiran57 force-pushed on Mar 17, 2025
  17. mabu44 commented at 10:50 pm on March 17, 2025: none

    In some scenarios the master branch returns a JSON with “success”:false while the proposed code throws an error. I think changing RPC behavior without a specific reason should be avoided. Example: (in both cases the same descriptor was imported before using a wider [0, 1000] range)

    On master

    0./bitcoin-cli importdescriptors '[{"desc": "wpkh(tprv8ZgxMBicQKsPebj9z5pERWfTpb4RSrqr9tnvGeNPvTV2iaHNdxz3kTcqMmmHabiXpk9DLKE6v24Q6mJwwaLYFLdjtyWs2wtRQUAo3xTGN7i/84h/1h/0h/0/*)#3v9ceyxw", "timestamp": 1742248939, "range": [0, 900]}]'
1[
2  {
3    "success": false,
4    "error": {
5      "code": -8,
6      "message": "new range must include current range = [0,1000]"
7    }
8  }
9]

    PR branch

    0./bitcoin-cli importdescriptors '[{"desc": "wpkh(tprv8ZgxMBicQKsPebj9z5pERWfTpb4RSrqr9tnvGeNPvTV2iaHNdxz3kTcqMmmHabiXpk9DLKE6v24Q6mJwwaLYFLdjtyWs2wtRQUAo3xTGN7i/84h/1h/0h/0/*)#3v9ceyxw", "timestamp": 1742248939, "range": [0, 900]}]'
1error code: -1
2error message:
3Can't update descriptor wpkh(tpubD6NzVbkrYhZ4Y4kwsjUppvKaPcaMcC2kjCPhZAQhLjHRZ4Y9GModvxEhXumCmQxJqdrtXn8CTP2CPitXDoZDHJHF9Rm6CqwirnxtXcNKfpY/84h/1h/0h/0/*)#m0f8wmee, error: new range must include current range = [0,1000]
  18. saikiran57 commented at 7:18 am on March 19, 2025: none
    HI @furszy can you please give some clarity on this comment #32023 (comment)
  19. furszy commented at 2:07 pm on March 19, 2025: member

    HI @furszy can you please give some clarity on this comment #32023 (comment)

    It is a valid concern. Bubbling up the error seems to be the best option to not couple components. Could go back to implementing #32023 (review) or a bare error string return approach from each of the wallet methods.

  20. saikiran57 force-pushed on Mar 19, 2025
  21. saikiran57 commented at 3:43 pm on March 19, 2025: none
    HI @mabu44 I’ve reverted the changes kindly confirm that. Can you please ACK on this.
  22. saikiran57 force-pushed on Mar 19, 2025
  23. furszy commented at 3:59 pm on March 19, 2025: member

    HI @mabu44 I’ve reverted the changes kindly confirm that. Can you please ACK on this.

    You are still changing the RPC behavior. importdescriptors returns errors within the response object, which is why your current changes are forcing you to modify the functional test code. When I said “bare error string return approach”, I basically meant making the underlying wallet methods return a string in case of an error, so it can be wrapped into the response object at the RPC level — similar to the util::Result approach I mentioned.

  24. mabu44 commented at 9:27 pm on March 19, 2025: none
    In reference to my first comment #32023 (comment), I note that the imported descriptor and the one returned in the error message are not equal. I imagine that the one in the error message represents the public key for the provided private key. This could reduce the clarity of the error message that no longer enables a direct comparison with the imported descriptors to understand which one generated the error. On the other hand, we do not return a private key in an error message, that may be logged.
  25. DrahtBot added the label CI failed on Mar 20, 2025
  26. DrahtBot commented at 1:14 pm on March 20, 2025: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/39107833824

    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.

  27. saikiran57 force-pushed on Mar 21, 2025
  28. fanquake commented at 7:28 am on March 21, 2025: member

    https://cirrus-ci.com/task/5112314648592384?logs=ci#L5356:

    0[01:20:03.641] Run scriptpubkeyman with args ['/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/bin/fuzz', '-runs=1', PosixPath('/ci_container_base/ci/scratch/qa-assets/fuzz_corpora/scriptpubkeyman')]INFO: Running with entropic power schedule (0xFF, 100).
1[01:20:03.641] INFO: Seed: 2231869594
2[01:20:03.641] INFO: Loaded 1 modules   (623423 inline 8-bit counters): 623423 [0x556e6d276398, 0x556e6d30e6d7), 
3[01:20:03.641] INFO: Loaded 1 PC tables (623423 PCs): 623423 [0x556e6d30e6d8,0x556e6dc91ac8), 
4[01:20:03.641] INFO:    10168 files found in /ci_container_base/ci/scratch/qa-assets/fuzz_corpora/scriptpubkeyman
5[01:20:03.641] INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 713594 bytes
6[01:20:03.641] INFO: seed corpus: files: 10168 min: 1b max: 713594b total: 110333808b rss: 164Mb
7[01:20:03.641] wallet/test/fuzz/scriptpubkeyman.cpp:83 CreateDescriptor: Assertion `!keystore.AddWalletDescriptor(wallet_desc, keys, "", false)' failed.
  29. saikiran57 force-pushed on Mar 21, 2025
  30. DrahtBot removed the label CI failed on Mar 21, 2025
  31. in src/wallet/test/fuzz/scriptpubkeyman.cpp:84 in 39a2006300 outdated 
    79@@ -80,7 +80,7 @@ static std::optional<std::pair<WalletDescriptor, FlatSigningProvider>> CreateWal
80 static DescriptorScriptPubKeyMan* CreateDescriptor(WalletDescriptor& wallet_desc, FlatSigningProvider& keys, CWallet& keystore)
81 {
82     LOCK(keystore.cs_wallet);
83-    keystore.AddWalletDescriptor(wallet_desc, keys, /*label=*/"", /*internal=*/false);
84+    Assert(keystore.AddWalletDescriptor(wallet_desc, keys, /*label=*/"", /*internal=*/false));
85     return keystore.GetDescriptorScriptPubKeyMan(wallet_desc);
    furszy commented at 1:37 pm on March 21, 2025:
    not for this PR but there is no need to call GetDescriptorScriptPubKeyMan() here, can just return AddWalletDescriptor() output.
  32. in src/bench/wallet_ismine.cpp:57 in 39a2006300 outdated 
    53@@ -54,8 +54,8 @@ static void WalletIsMine(benchmark::Bench& bench, bool legacy_wallet, int num_co
54             std::string error;
55             std::vector<std::unique_ptr<Descriptor>> desc = Parse("combo(" + EncodeSecret(key) + ")", keys, error, /*require_checksum=*/false);
56             WalletDescriptor w_desc(std::move(desc.at(0)), /*creation_time=*/0, /*range_start=*/0, /*range_end=*/0, /*next_index=*/0);
57-            auto spkm = wallet->AddWalletDescriptor(w_desc, keys, /*label=*/"", /*internal=*/false);
58-            assert(spkm);
59+            auto spkm = Assert(wallet->AddWalletDescriptor(w_desc, keys, /*label=*/"", /*internal=*/false));
    furszy commented at 1:38 pm on March 21, 2025:
    nano nit: maybe call it spkm_res to clarify that this is not the spkm anymore.
  33. in src/test/fuzz/util/wallet.h:64 in 39a2006300 outdated 
    58@@ -59,8 +59,9 @@ struct FuzzedWallet {
59                 WalletDescriptor w_desc{std::move(parsed_desc), /*creation_time=*/0, /*range_start=*/0, /*range_end=*/1, /*next_index=*/0};
60                 assert(!wallet->GetDescriptorScriptPubKeyMan(w_desc));
61                 LOCK(wallet->cs_wallet);
62-                auto spk_manager{wallet->AddWalletDescriptor(w_desc, keys, /*label=*/"", internal)};
63-                assert(spk_manager);
64+                auto spkm{wallet->AddWalletDescriptor(w_desc, keys, /*label=*/"", internal)};
65+                assert(spkm);
66+                auto spk_manager = spkm.value();
    furszy commented at 1:40 pm on March 21, 2025:

    We are no longer asserting that spk_manager!=nullptr. Could unify these lines + add the missing check using:

    0auto spk_manager = *Assert(wallet->AddWalletDescriptor(w_desc, keys, /*label=*/"", internal));
1assert(spk_manager);
  34. in src/wallet/rpc/backup.cpp:1583 in 39a2006300 outdated 
    1585+            auto spk_manager_res = wallet.AddWalletDescriptor(w_desc, keys, label, desc_internal);
1586+
1587+            if (!spk_manager_res)
1588+            {
1589+                throw JSONRPCError(RPC_INVALID_PARAMETER, util::ErrorString(spk_manager_res).original);
1590             }
    furszy commented at 1:44 pm on March 21, 2025:
    Would be good to follow the same code style we have in this function. No jump line after the condition statement.
  35. in src/wallet/wallet.cpp:3962 in 39a2006300 outdated 
    3957@@ -3956,7 +3958,10 @@ ScriptPubKeyMan* CWallet::AddWalletDescriptor(WalletDescriptor& desc, const Flat
3958     auto spk_man = GetDescriptorScriptPubKeyMan(desc);
3959     if (spk_man) {
3960         WalletLogPrintf("Update existing descriptor: %s\n", desc.descriptor->ToString());
3961-        spk_man->UpdateWalletDescriptor(desc);
3962+        if (auto res = spk_man->UpdateWalletDescriptor(desc); !res)
3963+        {
    furszy commented at 1:49 pm on March 21, 2025:

    Same as above; would be good to not mix coding styles. No jump line after the condition statement.

    And the res variable is shadowing another variable in this function. Would be better to rename it to res_spkm or spkm_res.

  36. in src/wallet/wallet.cpp:4362 in 39a2006300 outdated 
    4358@@ -4354,7 +4359,10 @@ bool DoMigration(CWallet& wallet, WalletContext& context, bilingual_str& error,
4359 
4360                 // Add to the wallet
4361                 WalletDescriptor w_desc(std::move(descs.at(0)), creation_time, 0, 0, 0);
4362-                data->watchonly_wallet->AddWalletDescriptor(w_desc, keys, "", false);
4363+                if(auto res = data->watchonly_wallet->AddWalletDescriptor(w_desc, keys, "", false); !res)
    furszy commented at 1:56 pm on March 21, 2025:

    The res variable is already used in this function, by declaring it here again you are shadowing the other one. Would be better to rename it to res_spkm or spkm_res.

    Also, missing whitespace after if.

  37. furszy commented at 2:00 pm on March 21, 2025: member

    Code reviewed, thanks for changing the approach. Looking good with a few nuances.

    There are two more places where we call AddWalletDescriptor that haven’t been considered:

    1. AddKey() in wallet_tests.cpp.
    2. CreateSyncedWallet() in wallet/test/util.cpp
  38. maflcko commented at 3:17 pm on March 21, 2025: member

    after this fix improved performance of importdescriptor part refs #32013.

    If this is a performance improvement, please explain exact steps to reproduce the speedup and how much it is.

  39. saikiran57 force-pushed on Mar 24, 2025
  40. DrahtBot added the label CI failed on Mar 24, 2025
  41. DrahtBot commented at 9:14 am on March 24, 2025: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/39279339189

    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.

  42. saikiran57 force-pushed on Mar 24, 2025
  43. saikiran57 commented at 11:28 am on March 24, 2025: none
    Hi @maflcko I’ve updated the steps to reproduces the issue. Have a look into the the experiment files for the behavior (I’ve mimicked the code) https://github.com/saikiran57/cpp17/blob/develop/experiment/map_with_high_load.cpp
  44. removed duplicate calling of GetDescriptorScriptPubKeyMan 
    Removed duplicate call to GetDescriptorScriptPubKeyMan and
Instead of checking linearly I have used find method so time complexity reduced significantly for GetDescriptorScriptPubKeyMan
after this fix improved performance of importdescriptor part refs #32013.
    55b931934a
  45. saikiran57 force-pushed on Mar 24, 2025
  46. DrahtBot removed the label CI failed on Mar 24, 2025
  47. saikiran57 commented at 6:38 am on March 31, 2025: none
    Hi @maflcko could you please ACK if your okay with response.


saikiran57 DrahtBot furszy pablomartin4btc mprenditore maflcko mabu44 fanquake

Labels
Wallet

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-03-31 09:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me