Add ScriptContext to Miniscript #97

sanket1729 · 2020-06-15T05:52:39Z

Adds context to Miniscript. I have made a bunch of design decisions here with minor changes in API for satisfied constraints.

I could not find a clean way to enforce constraints for the only bitcoin::PublicKey type because our code was generic, so had to hack in IS_BITCOIN_PUBKEY to the Miniscript Trait.

This builds on top of #95 , would rebase and squash as needed.

apoelstra · 2020-06-15T18:36:00Z

src/lib.rs

@@ -113,13 +113,15 @@ pub use miniscript::Miniscript;
 pub trait MiniscriptKey:
    Clone + Eq + Ord + str::FromStr + fmt::Debug + fmt::Display + hash::Hash
 {
+    const IS_BITCOIN_PUBKEY: bool = false;


I think it would be better to have method is_uncompressed() which has a default implementation which returns false. Then only for bitcoin::PublicKey do we implement this and have it actually return the uncompressedness.

sanket1729 · 2020-06-16T08:05:41Z

src/miniscript/astelem.rs

@@ -146,11 +147,13 @@ impl<Pk: MiniscriptKey> Terminal<Pk> {
                let keys: Result<Vec<Q>, _> = keys.iter().map(&mut *translatefpk).collect();
                Terminal::Multi(k, keys?)
            }
-        })
+        };
+        Ctx::check_frag_validity(&frag).expect("Translate failed!");


I am not completely sure how to handle this error. There is already one error type because of the Function argument. Secondly, we would also another because the translation may not be valid(for ex: translating string to uncompressed pubkeys in segwit descriptor).

We can

Create a enum sum type for both errors

Return a Result<Result<ms, err1>,err2>

Panic!

sanket1729 · 2020-06-16T09:08:50Z

Fuzz failures seem unrelated, I can make another PR to address those.

apoelstra · 2020-06-26T17:51:12Z

contrib/test.sh

+        rustup set profile minimal
+        rustup default "$MIRI_NIGHTLY"
+        rustup component add miri
+        cargo miri test -- -- any_unsafe_transmute_miri_test


This line filters to only run the any_unsafe_transmute_miri_test test? Maybe we should shorten it to miri_, so that in future, we can add MIRI-executable tests just by prefixing them with miri_

apoelstra · 2020-06-26T21:12:44Z

src/descriptor/mod.rs

@@ -74,29 +75,43 @@ impl<Pk: MiniscriptKey> Descriptor<Pk> {
        &self,
        mut translatefpk: Fpk,
        mut translatefpkh: Fpkh,
-    ) -> Result<Descriptor<Q>, E>
+    ) -> Result<Result<Descriptor<Q>, Error>, E>


I think we should do Result<Result<Descriptor<Q>, E>, Error> instead

Actually, after some discussion on IRC, I think we should panic on uncompressed keys, and put a clear warning in the doccomment for this function.

If the user does not want to panic, this is totally in her control -- she can change translatefpk and translatefpkh to return an error rather than an uncompressed key.

apoelstra · 2020-06-26T21:50:27Z

src/descriptor/satisfied_constraints.rs

    ///The node which is being evaluated
-    node: &'desc Miniscript<bitcoin::PublicKey>,
+    node: &'desc Miniscript<bitcoin::PublicKey, Ctx>,


I think we should put Any here rather than genericizing over Ctx and then using Any down below.

apoelstra · 2020-06-26T21:51:02Z

src/descriptor/satisfied_constraints.rs

-            ms: &'elem Miniscript<bitcoin::PublicKey>,
-        ) -> SatisfiedConstraints<'elem, 'stack, F>
+            ms: &'elem Miniscript<bitcoin::PublicKey, Legacy>,
+        ) -> SatisfiedConstraints<'elem, 'stack, Legacy, F>


Curious why we use Legacy here rather than Any

As you mentioned in a later comment, we should never actually use Any other than whenever required.
As such, there is no way to create an Any type Miniscript from string or decoding, we must specify a Segwitv0 or Legacy context along with it. If we try to create an Any type Miniscript, the context checks would map to unreachable!().

apoelstra · 2020-06-26T21:55:38Z

src/miniscript/context.rs

+}
+
+pub trait ScriptContext:
+    fmt::Debug + Clone + Ord + PartialOrd + Eq + PartialEq + private::Sealed


Lol, do we actually need any of these extra traits beyond private::Sealed?

Turns out we do. Rust is complaining that it can't compare, equate without those

apoelstra · 2020-06-26T21:57:14Z

src/miniscript/context.rs

+        frag: &Terminal<Pk, Ctx>,
+    ) -> Result<(), ScriptContextError> {
+        Segwitv0::check_frag_non_malleable(frag)?;
+        Legacy::check_frag_non_malleable(frag)


I think these should be unimplemented!() rather than doing both checks. We shouldn't ever actually use Any except in places where the context doesn't matter.

Changed to !unreachable!()

apoelstra · 2020-06-26T21:57:40Z

src/miniscript/context.rs

+        // checking the same.
+        unsafe {
+            use std::mem::transmute;
+            transmute::<&Miniscript<Pk, Legacy>, &Miniscript<Pk, Any>>(ms)


FWIW you don't need to provide types here, Rust can infer them

apoelstra · 2020-06-26T22:06:30Z

src/lib.rs

+    /// Check if the publicKey is uncompressed. The default
+    /// implementation returns false for all Generic types of
+    /// MiniscriptKey, and returns the true only for
+    /// bitcoin::Publickey if the underlying key is uncompressed.


This comment could be shortened to just "The default implementation returns false"

src/lib.rs

src/miniscript/astelem.rs

src/miniscript/mod.rs

src/policy/compiler.rs

src/descriptor/mod.rs

src/descriptor/satisfied_constraints.rs

apoelstra · 2020-06-29T20:41:19Z

src/miniscript/mod.rs

    ///The correctness and malleability type information for the AST node
    pub ty: types::Type,
    ///Additional information helpful for extra analysis.
    pub ext: types::extra_props::ExtData,
+    /// Context PhantomData
+    pub phantom: PhantomData<Ctx>,


I think we shouldn't make this public.

This has the weird effect of making it impossible to construct a Miniscript without going through the constructor, but I'm not sure we ever wanted that. The other fields are public to make introspection possible, but ideally we don't want the user to be modifying these things.

apoelstra · 2020-06-29T20:45:14Z

src/miniscript/mod.rs

-    pub fn translate_pk<FPk, FPkh, Q, Error>(
+impl<Pk: MiniscriptKey, Ctx: ScriptContext> Miniscript<Pk, Ctx> {
+    /// This will panic while converting to Segwit descriptor using
+    /// using uncompressed pubkeys.


Can you expand on this? I suggest "This will panic if translatefpk returns an uncompressed key when converting to a Segwit descriptor. To prevent this panic, ensure translatefpk returns an error in this case instead."

In the compiler we have a function which inserts the "cast closure" of an expression. It accepts the expression as a candidate as well as a large list of cast-wrapped variants of the expression. For each candidate it rejects it if it is consensus-invalid or has malleable satisfactions. In the original compiler (PR #29) it would decide whether an expression was malleable by checking ty.mall.non_malleable on the miniscript. It had the confusing comment "return malleable types directly" which I believe was supposed to mean "return early if the node is malleable". The comment also observes that "If a elem is malleable, all the casts to it are also going to be malleable" but the code doesn't actually use this fact anywhere. It always tries all the casts. But ok, whatever. Later in #97 (add context to Miniscript) we weakened the malleability check in a bizarre way -- now it checks that the node is malleable according to the type system AND that it is non-malleable according to the context system. That is, we whitelist or_i and d: as "acceptable malleability" in a legacy/bare context. This change, which was not commented on in the original PR, seems like it's just totally wrong. I suspect it was supposed to be an or: if the node is malleable according to the type system OR according to the contextual checks, then skip it. But I'm unsure. (The check is also wrong because the contextual check is wrong; see the last commit.) Anyway, after some fairly heavy fuzzing I was unable to find any instance where this check affects compiler output. I suspect that it's merely an optimization. So change it back to the stronger, simpler check and update the comment.

In the compiler we have a function which inserts the "cast closure" of an expression. It accepts the expression as a candidate as well as a large list of cast-wrapped variants of the expression. For each candidate it rejects it if it is consensus-invalid or has malleable satisfactions. In the original compiler (PR rust-bitcoin#29) it would decide whether an expression was malleable by checking ty.mall.non_malleable on the miniscript. It had the confusing comment "return malleable types directly" which I believe was supposed to mean "return early if the node is malleable". The comment also observes that "If a elem is malleable, all the casts to it are also going to be malleable" but the code doesn't actually use this fact anywhere. It always tries all the casts. But ok, whatever. Later in rust-bitcoin#97 (add context to Miniscript) we weakened the malleability check in a bizarre way -- now it checks that the node is malleable according to the type system AND that it is non-malleable according to the context system. That is, we whitelist or_i and d: as "acceptable malleability" in a legacy/bare context. This change, which was not commented on in the original PR, seems like it's just totally wrong. I suspect it was supposed to be an or: if the node is malleable according to the type system OR according to the contextual checks, then skip it. But I'm unsure. (The check is also wrong because the contextual check is wrong; see the last commit.) Anyway, after some fairly heavy fuzzing I was unable to find any instance where this check affects compiler output. I suspect that it's merely an optimization. So change it back to the stronger, simpler check and update the comment.

sanket1729 force-pushed the context branch from 85b2f10 to 8ef294a Compare June 15, 2020 05:52

apoelstra reviewed Jun 15, 2020

View reviewed changes

sanket1729 force-pushed the context branch 2 times, most recently from 0aaa34f to be2faa1 Compare June 16, 2020 08:21

sanket1729 commented Jun 16, 2020

View reviewed changes

sanket1729 marked this pull request as ready for review June 16, 2020 08:24

sanket1729 force-pushed the context branch from be2faa1 to 21e9d5f Compare June 16, 2020 08:54

sanket1729 changed the title ~~Context~~ Add ScriptContext to Miniscript Jun 16, 2020

sanket1729 force-pushed the context branch 5 times, most recently from ea36951 to 1989671 Compare June 18, 2020 21:24

sanket1729 mentioned this pull request Jun 18, 2020

Do not allow uncompressed pubkey in wpkh(KEY) output descriptor #88

Closed

sanket1729 force-pushed the context branch 4 times, most recently from c07e9cf to bbd1218 Compare June 19, 2020 02:15

apoelstra reviewed Jun 26, 2020

View reviewed changes

sanket1729 force-pushed the context branch from bbd1218 to aed7009 Compare June 26, 2020 18:03

apoelstra reviewed Jun 26, 2020

View reviewed changes

src/lib.rs Show resolved Hide resolved