Merge #1553: skip policy compilation when parsing descriptor

a95cfad6d249cffecc25da3d9b31b81946bd297a skip policy compilation when parsing descriptor (Michael Mallan)

Pull request description:

  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. 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.

ACKs for top commit:
  pythcoiner:
    utACK a95cfad6
  edouardparis:
    ACK a95cfad6d249cffecc25da3d9b31b81946bd297a

Tree-SHA512: 690d7b6a14387222c08537940556a2bbae46f4b46905411810b494a6debc3958a3963e9003bbe0fecf2c5530c15bcc3737874d59994ab599a5d5db59a5bf332a

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 {

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)?;