skip policy compilation when parsing descriptor #1553
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
darosior
reviewed
Jan 22, 2025
Comment on lines
+650
to
+658
| // We don't compile the policy as we assume it compiles given we started with a descriptor. | ||
| // This will still perform all other checks to make sure the descriptor conforms to | ||
| // a Liana policy. | ||
| LianaPolicy::_new( | ||
| prim_path, | ||
| recovery_paths, | ||
| is_taproot, | ||
| /* compile = */ false, | ||
| ) |
There was a problem hiding this comment.
The policy compilation would perform Miniscript sanity checks. Is it necessary to manually call sanity_check on the descriptor instead? Or maybe it's already called at parsing time (although technically this function is not aware of that).
There was a problem hiding this comment.
Thanks for pointing this out 🙏
Yes I think then it makes sense to call this sanity_check method on the descriptor within the LianaDescriptor::from_str method.
I've checked that doing so does not noticeably harm performance when running the tests.
There was a problem hiding this comment.
It looks like they're only called for Taproot: rust-bitcoin/rust-miniscript#734.
To be on the safe side, I've added a call to this method in all cases.
jp1ac4
force-pushed
the
liana-policy-skip-compile-when-parsing
branch
from
1f1a014 to
700ff94
Compare
This allows to create a new `LianaPolicy` without checking that it compiles into miniscript. This compilation check is used as a sanity check on the policy in addition to other Liana-specific checks, but this compiled policy is not otherwise used. When parsing an existing descriptor, we will assume that the policy can be compiled. Otherwise, the method `LianaPolicy::from_multipath_descriptor` ends up calling `LianaPolicy::into_multipath_descriptor_fallible`. A `LianaPolicy` is created from a descriptor each time we call `LianaDescriptor::from_str`, which is done often, e.g. when updating derivation indices in the DB. The sanity check of the descriptor previously performed during compilation of the policy will now be done explicitly as part of the `LianaDescriptor::from_str` method. Removing the compilation check won't affect the `LianaDescriptor` that we end up with when parsing a string as we already use the `Descriptor` obtained from parsing the string rather than that obtained by compilation of the policy. This change is particularly important with the upcoming upgrade to miniscript 12.0, which has been found to increase Liana policy compilation times significantly. An additional option would be to reduce the number of times we call `LianaDescriptor::from_str`, e.g. by passing the descriptor as a parameter rather than reading from DB, but this current change should address the main performance issues.
jp1ac4
force-pushed
the
liana-policy-skip-compile-when-parsing
branch
from
700ff94 to
a95cfad
Compare
|
utACK a95cfad |
edouardparis
approved these changes
Jan 23, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This allows to create a new
LianaPolicywithout checking that it compiles into miniscript. This compilation check is used as a sanity check on the policy in addition to other Liana-specific checks, but this compiled policy is not otherwise used.When parsing an existing descriptor, we will assume that the policy can be compiled. Otherwise, the method
LianaPolicy::from_multipath_descriptorends up callingLianaPolicy::into_multipath_descriptor_fallible.A
LianaPolicyis created from a descriptor each time we callLianaDescriptor::from_str, which is done often, e.g. when updating derivation indices in the DB. Removing the compilation check won't affect theLianaDescriptorthat we end up with when parsing a string as we already use theDescriptorobtained from parsing the string rather than that obtained by compilation of the policy.This change is particularly important with the upcoming upgrade to miniscript 12.0, which has been found to increase Liana policy compilation times significantly.
An additional option would be to reduce the number of times we call
LianaDescriptor::from_str, e.g. by passing the descriptor as a parameter rather than reading from DB, but this current change should address the main performance issues.