wallet: cache descriptor ID to avoid repeated descriptor string creation #28799

1. 
   theStack commented at 11:05 pm on November 5, 2023: contributor

   Right now a wallet descriptor is converted to its string representation (via Descriptor::ToString) repeatedly at different instances:

   • on finding a DescriptorScriptPubKeyMan for a given descriptor (CWallet::GetDescriptorScriptPubKeyMan, e.g. used by the importdescriptors RPC); the string representation is created once for each spkm in the wallet and at each iteration again for the searched descriptor (DescriptorScriptPubKeyMan::HasWalletDescriptor)
   • whenever DescriptorScriptPubKeyMan::GetID() is called, e.g. in TopUp or any instances where a descriptor is written to the DB to determine the database key, also at less obvious places like FastWalletRescanFilter etc.

   As there is no good reason to calculate a fixed descriptor’s string/ID more than once, add the ID as a field to WalletDescriptor and calculate it immediately at initialization (or deserialization). HasWalletDescriptor is changed to compare the spkm’s and searched descriptor’s ID instead of the string to take use of that.

   This speeds up the functional test wallet_miniscript.py by a factor of 5-6x on my machine (3m30.95s on master vs. 0m38.02s on PR). The recently introduced “max-size TapMiniscript” test-case introduced a descriptor that takes 2-3 seconds to create a string representation, so the repeated calls to that were significantly hurting the performance.

   Fixes #28800.

2. wallet: cache descriptor ID to avoid repeated descriptor string creation

   Right now a wallet descriptor is converted to it's string representation
   (via `Descriptor::ToString`) repeatedly at different instances:
   - on finding a `DescriptorScriptPubKeyMan` for a given descriptor
     (`CWallet::GetDescriptorScriptPubKeyMan`, e.g. used by the
     `importdescriptors` RPC); the string representation is created once
     for each spkm in the wallet and at each iteration again for
     the searched descriptor (`DescriptorScriptPubKeyMan::HasWalletDescriptor`)
   - whenever `DescriptorScriptPubKeyMan::GetID()` is called, e.g. in
     `TopUp` or any instances where a descriptor is written to the DB
     to determine the database key etc.
   
   As there is no good reason to calculate a fixed descriptor's string/ID
   more than once, add the ID as a field to `WalletDescriptor` and
   calculate it immediately at initialization (or deserialization).
   `HasWalletDescriptor` is changed to compare the spkm's and searched
   descriptor's ID instead of the string to take use of that.
   
   This speeds up the functional test `wallet_miniscript.py` by a factor of
   5-6x on my machine (3m30.95s on master vs. 0m38.02s on PR). The recently
   introduced "max-size TapMiniscript" test-case introduced a descriptor
   that takes 2-3 seconds to create a string representation, so the
   repeated calls to that were significantly hurting the performance.

   f811a24421
3. test: remove custom rpc timeout for `wallet_miniscript.py`, reorder in test_runner 5e6bc6d830
4. 
   DrahtBot commented at 11:05 pm on November 5, 2023: contributor

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

   Code Coverage

   For detailed information about the code coverage, see the test coverage report.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	Sjors, S3RK, BrandonOdiwuor, achow101

   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:

   • #28710 (Remove the legacy wallet and BDB dependency by achow101)
   • #22838 (descriptors: Be able to specify change and receiving in a single descriptor string 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.

5. DrahtBot added the label Wallet on Nov 5, 2023
6. 
   Sjors commented at 5:32 am on November 6, 2023: member

   This fixes #28800 for me and runs way faster under --with-sanitizers=thread.

   ACK 5e6bc6d830664a5afeb5d5bd7e7b3818a01376b7

7. 
   S3RK commented at 8:10 am on November 6, 2023: contributor

   Code Review ACK 5e6bc6d830664a5afeb5d5bd7e7b3818a01376b7

   The change looks correct. I also verified that when we update descriptors with importdescriptor command we replace the whole WalletDescriptor object, so ID remains correct in that case.

8. BrandonOdiwuor approved
9. 
   BrandonOdiwuor commented at 1:44 pm on November 6, 2023: contributor

   ACK 5e6bc6d830664a5afeb5d5bd7e7b3818a01376b7

   looks good to me

10. fanquake assigned achow101 on Nov 6, 2023
11. 
    achow101 commented at 7:57 pm on November 6, 2023: member

    ACK 5e6bc6d830664a5afeb5d5bd7e7b3818a01376b7
12. achow101 merged this on Nov 6, 2023
13. achow101 closed this on Nov 6, 2023

    ---

14. theStack deleted the branch on Nov 7, 2023

Contributors
theStack DrahtBot Sjors S3RK BrandonOdiwuor achow101

Labels
Wallet