From a95cfad6d249cffecc25da3d9b31b81946bd297a Mon Sep 17 00:00:00 2001
From: Michael Mallan <michaelmallan@proton.me>
Date: Tue, 21 Jan 2025 11:07:10 +0000
Subject: [PATCH] skip policy compilation when parsing descriptor

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.
---
 liana/src/descriptors/analysis.rs | 32 +++++++++++++++++++++++++++----
 liana/src/descriptors/mod.rs      |  3 +++
 2 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/liana/src/descriptors/analysis.rs b/liana/src/descriptors/analysis.rs
index 48d142c6..88476110 100644
--- a/liana/src/descriptors/analysis.rs
+++ b/liana/src/descriptors/analysis.rs
@@ -464,10 +464,14 @@ pub struct LianaPolicy {
 
 impl LianaPolicy {
     /// Create a new Liana policy from a given configuration.
+    ///
+    /// `compile` controls whether to check the policy compiles
+    /// to miniscript before returning.
     fn _new(
         primary_path: PathInfo,
         recovery_paths: BTreeMap<u16, PathInfo>,
         is_taproot: bool,
+        compile: bool,
     ) -> Result<LianaPolicy, LianaPolicyError> {
         if recovery_paths.is_empty() {
             return Err(LianaPolicyError::MissingRecoveryPath);
@@ -523,7 +527,9 @@ impl LianaPolicy {
             recovery_paths,
             is_taproot,
         };
-        policy.clone().into_multipath_descriptor_fallible()?;
+        if compile {
+            policy.clone().into_multipath_descriptor_fallible()?;
+        }
         Ok(policy)
     }
 
@@ -532,7 +538,12 @@ impl LianaPolicy {
         primary_path: PathInfo,
         recovery_paths: BTreeMap<u16, PathInfo>,
     ) -> Result<LianaPolicy, LianaPolicyError> {
-        Self::_new(primary_path, recovery_paths, /* is_taproot = */ true)
+        Self::_new(
+            primary_path,
+            recovery_paths,
+            /* is_taproot = */ true,
+            /* compile = */ true,
+        )
     }
 
     /// Create a new Liana policy for use under a P2WSH context.
@@ -540,7 +551,12 @@ impl LianaPolicy {
         primary_path: PathInfo,
         recovery_paths: BTreeMap<u16, PathInfo>,
     ) -> Result<LianaPolicy, LianaPolicyError> {
-        Self::_new(primary_path, recovery_paths, /* is_taproot = */ false)
+        Self::_new(
+            primary_path,
+            recovery_paths,
+            /* is_taproot = */ false,
+            /* compile = */ true,
+        )
     }
 
     /// Create a Liana policy from a descriptor. This will check the descriptor is correctly formed
@@ -631,7 +647,15 @@ impl LianaPolicy {
         // Use the constructor for sanity checking the keys and the Miniscript policy. Note this
         // makes sure the recovery paths mapping isn't empty, too.
         let prim_path = primary_path.ok_or(LianaPolicyError::IncompatibleDesc)?;
-        LianaPolicy::_new(prim_path, recovery_paths, is_taproot)
+        // 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,
+        )
     }
 
     pub fn primary_path(&self) -> &PathInfo {
diff --git a/liana/src/descriptors/mod.rs b/liana/src/descriptors/mod.rs
index dadf38fb..e628d92a 100644
--- a/liana/src/descriptors/mod.rs
+++ b/liana/src/descriptors/mod.rs
@@ -115,7 +115,10 @@ impl str::FromStr for LianaDescriptor {
     fn from_str(s: &str) -> Result<LianaDescriptor, Self::Err> {
         // Parse a descriptor and check it is a multipath descriptor corresponding to a valid Liana
         // spending policy.
+        // Sanity checks are not always performed when calling `Descriptor::from_str`, so we perform
+        // them explicitly. See https://github.com/rust-bitcoin/rust-miniscript/issues/734.
         let desc = descriptor::Descriptor::<descriptor::DescriptorPublicKey>::from_str(s)
+            .and_then(|desc| desc.sanity_check().map(|_| desc))
             .map_err(LianaDescError::Miniscript)?;
         LianaPolicy::from_multipath_descriptor(&desc)?;