-
Notifications
You must be signed in to change notification settings - Fork 35.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
wallet: cache descriptor ID to avoid repeated descriptor string creation #28799
wallet: cache descriptor ID to avoid repeated descriptor string creation #28799
Conversation
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.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
Code Review ACK 5e6bc6d The change looks correct. I also verified that when we update descriptors with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 5e6bc6d
looks good to me
ACK 5e6bc6d |
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. Github-Pull: bitcoin#28799 Rebased-From: f811a24
Right now a wallet descriptor is converted to its string representation (via
Descriptor::ToString
) repeatedly at different instances:DescriptorScriptPubKeyMan
for a given descriptor (CWallet::GetDescriptorScriptPubKeyMan
, e.g. used by theimportdescriptors
RPC); the string representation is created once for each spkm in the wallet and at each iteration again for the searched descriptor (DescriptorScriptPubKeyMan::HasWalletDescriptor
)DescriptorScriptPubKeyMan::GetID()
is called, e.g. inTopUp
or any instances where a descriptor is written to the DB to determine the database key, also at less obvious places likeFastWalletRescanFilter
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.