Policy fixes #170
Policy fixes #170
Conversation
| @@ -215,6 +215,19 @@ impl<Pk: SimplicityKey> Policy<Pk> { | |||
| _ => {} | |||
| } | |||
| } | |||
|
|
|||
| /// Return an iterator over the fragments of the policy. | |||
There was a problem hiding this comment.
In a036b0b:
You mention maybe using the dag module in the commit message -- actually we should use the miniscript::iter module which will be available in rust-miniscript 11.0. rust-bitcoin/rust-miniscript#567
But for now it's fine to have a manual implementation.
There was a problem hiding this comment.
I see. Miniscript's iter module is the cousin of Simplicity's dag module. I look forward to using it :)
|
Will ACK, but I'd kinda like the |
| @@ -17,6 +17,7 @@ path = "src/lib.rs" | |||
|
|
|||
| [dependencies] | |||
| bitcoin = { version = "0.30.0", optional = true } | |||
| bitcoin-miniscript = { package = "miniscript", version = "10.0" } | |||
There was a problem hiding this comment.
In eb055fa:
I am tempted to change this from 10.0 to a range from 8.0.1 to 10. Later we will just bump the 10. Using a range will minimize the amount of grief that we'll have with coordinating new crate versions. (Though arguably we have no need to support miniscript 8 and 9 since those are in the past.)
|
Actually I do have one request before ACKing -- @uncomputable can you rearrange the first commits so that the miniscript one comes first? That way I can test the entire PR with the same lockfiles, and not have to worry about Cargo.toml changing partway through. |
This is the easiest solution to use Miniscript keys in Simplicity contexts. We focused on subtraits before, which would require changes to (Elements) Miniscript, but we can also use generic impl blocks, as long as they are inside rust-simplicity (orphan rule). We removed elements-miniscript from rust-simplicity to avoid a cyclic dependency. MiniscriptKey is defined in rust-miniscript, so importing it here does not create a cycle. We get a diamond pattern where both elements-miniscript and rust-simplicity depend on the same library, which should be okay.
We get this for free.
uncomputable
force-pushed
the
policy-fixes
branch
from
a036b0b to
0e8b3a5
Compare
|
Reordered the commits and fixed the two nits. We will need to update the Haskell generators to include a new use statement. |
|
Thanks -- but now the benchmarks are not compiling. |
A generic transaction reference is more flexible and will prevent us from cloning transactions in the upcoming sighash. Because we changed jet/init/elements.rs, we need to update the Haskell code that generated the original file.
The signature hashes are different for Simplicity than for Taproot. Among many things, the Simplicity sighash commits to the annexes of all inputs, while Taproot only commits to the annex of the local input. There is no clean way to compute the sighash at the moment; the code is hidden somewhere in the Elements environment in the C files. We can naively construct a Rust Elements environment in order to extract the sighash from its FFI bindings. This requires information (control block) that we arguably don't need, while other information is left unused (leaf hash). We should write more direct bindings or implement the sighash in pure Rust in the future. This will take time, so we make do with what we have.
The order of fragments doesn't seem to matter, so I chose preorder which is easy to implement. I would have used the iterators from dag.rs, but Dag doesn't cover nodes with arbitrary but fixed outdegree, like Policy::Threshold. It might be worth to extend Dag in the future, although that could make the generic code in dag.rs more complicated than it already is.
uncomputable
force-pushed
the
policy-fixes
branch
from
0e8b3a5 to
d64e3b2
Compare
|
Fixed the benchmarks. Sorry for that. I didn't test them in my local CI. |
|
Heads up that I disabled my "checked-in files match those from Simplicity" check for this ACK. |
Adds fixes to make Policy work with Miniscript, descriptors and the Simplicity wallet.
The only controversial change should be the implementation of Simplicity key traits for every Miniscript key. This introduces a diamond pattern: rust-miniscript → rust-simplicity → elements-miniscript and rust-miniscript → elements-miniscript. The situation is not ideal, but it avoids violating the orphan rule that caused us so much trouble.
In particular we need to convert
elements_miniscript::Satisfier(with Miniscript keys) intosimplicity::Satisfier(with Simplicity keys). We could makeelements_miniscript::Satisfiera subtrait of simplicity::Satisfier, but we would have to do that for every generic implementation ofelements_miniscript::Satisfier. We would need to copy code to rust-simplicity and implementsimplicity::Satisfierfor nonsensical types such asHashMap<Pk, ElementsSig>.The diamond pattern seems like a good solution that will work until we have a better architecture. It allows us to use Miniscript and Simplicity keys interchangeably, while all the messy Simplicity details stay inside rust-simplicity.