I pushed some experiments I've been doing with the Signer trait. I still don't feel 100% confident about the interface, specifically if it's better to take a &mut psbt::PartiallySignedTransaction and apply the signatures inside the signer (what's currently implemented) as opposed to taking a normal reference, returning the signature and letting the caller handle everything else.

I initially liked the first approach better because it hides the logic of pushing the signature in the partial_sigs map from the caller, but that actually forces me to clone the psbt somewhere, which is not optimal.

I guess the &mut approach is better if we expect people to directly use the signers outside of the code provided by this crate, while the other one is less "user-friendly" but it allows for some minor optimizations inside this code.

The other big question is whether I should add an explicit flag to ask for a BIP143 signature or try to automatically determine it by looking at the witness_utxo/non_witness_utxo fields like I'm doing now. Considering the recent issue with Trezors and the fact that they decided to require non_witness_utxos for segwit psbts too, I think it might be necessary to have an explicit flag to avoid issues down the road.

The other big question is whether I should add an explicit flag to ask for a BIP143 signature or try to automatically determine it by looking at the witness_utxo/non_witness_utxo fields like I'm doing now. Considering the recent issue with Trezors and the fact that they decided to require non_witness_utxos for segwit psbts too, I think it might be necessary to have an explicit flag to avoid issues down the road.

I don't completely follow this, what is the issue to determine by looking at witness_utxo field? Can you elaborate how this might lead to more issues ahead?

As for the role of miniscript in PSBT stack, we originally thought that miniscript should only be required for finalizing psbt. Other roles must fill in the partial sigs as required. I read BIP32, BIP 174 a couple days ago, so maybe I am wrong with these. We only considered the following workflow

1. The creator creates a psbt with unsigned tx and passes it on for signing.
2. The signers can analyze the Miniscript, infer which keys are in the miniscript; sign the tx with appropriate sighash flag and add the partial sigs to the pbst.
3. The finalizer finally looks at the partial sigs and each inputs and creates the transaction. The role of satisfier triat is to construct the final pbst as implemented here.

   rust-miniscript/src/psbt/mod.rs

   Line 125 in 87a47e2

   pub fn finalize(psbt: &mut Psbt) -> Result<(), super::Error> {

Please note that the pbst implementation is incomplete and not tested.

My understanding that the overall flow should be as follows: (cc @apoelstra )

fn main() {
    // get descriptor and signers
    let descriptor, signers = .....

    /// Create unsigned initial psbt
    let psbt: bitcoin::util::psbt::PartiallySignedTransaction = .......

    // Note that this could be a distributed process
    // Sign each input and
    for input in psbt.inputs{
    // Add sig for each input. Again could be distributed process.
        for signer in signers{
        //Add partial sig here.
        signer.sign(&mut psbt, input_index)    
        }
    }
    // Pass on to the finalizer. The Satisfier trait is already implemented for psbt input.
    pbst::finalize(& mut psbt);
}

I did not look at all of the code yet, but most of it looks good.
A couple of questions that I am still unclear about:
Is there any special need for DescriptorWithSigners struct to implement FromTree. I think that this only requires FromStr and then the struct MiniscriptWithSigners should no longer be required.

I don't completely follow this, what is the issue to determine by looking at witness_utxo field? Can you elaborate how this might lead to more issues ahead?

I don't know how realistic this is, I was just thinking that maybe some wallets could decide in the future to always include non_witness_utxos and potentially remove witness_utxos entirely to save some space, because after all you can find everyting you need from the non_witness_utxo. If that was the case, PSBTs built by these wallets would be mistaken for non-segwit with the logic I have implemented currently.

As for the role of miniscript in PSBT stack, we originally thought that miniscript should only be required for finalizing psbt. Other roles must fill in the partial sigs as required.

If you feel like the signers are out of scope I can totally remove them. There was #93 (comment) that said "and have our [signers] map use the Satisfier trait to allow users to produce signatures etc with it." so I interpreted that as "implement Satisfier on SignersContaner so that it can produce signatures on-demand", but I might have just misunderstood something.

I think that this only requires FromStr and then the struct MiniscriptWithSigners should no longer be required.

Yeah, I think you are right. I was trying to keep the structs "parallel" (Miniscript -> MiniscriptWithSigners, Descriptor -> DescriptorWithSigners) but now that I think about it, the signers pretty much only make sense with descriptors, so I could remove it MiniscriptWithSigners. I'll try working on that today.

I agree that PSBT signing is out of scope ... if only because it is so simple. As Sanket says, you just compute the sighash (which rust-bitcoin has functionality for), produce a signature (i.e. call sign in rust-secp) and attach it to the PSBT (and the PSBT struct exposes a BTreeMap for you to insert the signature in). Given this, I'm not sure I see the purpose of the Signer trait.

As for implementing Satisfier on a map from public keys to private keys (and from Pk::Hash to private keys), I think this seems totally reasonable. And I agree that the way to do this is to create something like the PSBTSigningContext structure which contains a reference to a PSBT, a reference to a map, and an input index. But the implementation in this PR seems overly complicated. I will do a detailed review with more comments.

Some more thoughts:

• I think DescriptorWithSigners and and MiniscriptWithSigners and DescriptorKeyWithSecrets is too clever by half. I think it'd be better to have two concrete types, DescriptorPublicKey and DescriptorSecretKey, which are basically the same except that one has secret-key variants of the other.
• Then I think rather than having a separate type DescriptorWithSigners where we impl FromStr on it, we should just add a method to Descriptor called parse_secret or something, which returns a Descriptor<DescriptorPublicKey> as well as a HashMap<DescriptorPublicKey, DescriptorPrivateKey>. I think it's fine to wrap the HashMap in a new type, maybe call it KeyMap
• Then you can implement a method on KeyMap which takes an input index and a mutable reference to a PSBT, which computes a signature and inserts it in the right place. (Notice that this does not actually involve Miniscript or descriptors at all, it's just a convenience function...I am ambivalent about whether we ought to have it in this library at all.)
• Then you can implement Satisfier directly on Psbt, which takes signatures from the input map, scripts from the input map, and outputs a valid witness.

Regarding my comment in 93 about "using Satisfier to produce signatures", I was just wrong. The Satisfier trait's job is to produce a witness, given signatures, not to produce signatures (which it cannot do because the API provides nowhere to input a sighash nor anywhere to output a signature). If we add such a function, and I don't necessarily think we should, it should be a standalone function.

The reason why I kinda liked the idea of connecting the signing flow with the Satisfier trait is that it seemed nice to be able to only generate the signatures you require and nothing more, on-demand, but that really makes a difference only when somebody has the ability to provide multiple signatures in the same script, which is kind of a weird situation and probably doesn't justify all the complexity that signing carries: it's much easier to just sign with every key you have (probably just one in most cases) and then the satisfier will only pick the ones it needs.

So here's what I'll try to do for this PR:

• Remove the Signer trait, PSBTSigningContext, etc
• Rename DescriptorKey to DescriptorPublicKey
• Rename DescriptorKeyWithSecrets to DescriptorSecretKey, but don't make it MiniscriptKey since we won't care about parsing it directly in Descriptor::from_str (which also means removing all the custom implementation for Eq, Ord, etc since we won't need them anymore)
• Add Descriptor::parse_secret that parses the string as Descriptor<String> internally first, then parses and maps every individual key using translate_pk() and at the same time populates the KeyMap with secrets, if they keys contain any
• Modify ScriptContext::sighash to use transactions instead of PSBTs and move the PSBT stuff in a separate, standalone function

Hope I understood everything right, thanks for the feedback!

Just so we are on the same page: Descriptor::parse_secret would return Result<(Descriptor<DescriptorPublicKey>, HashMap<DescriptorPublicKey, DescriptorSecretKey>), _>?

Another thing I noticed is that we might want to add a Hash variant to the DescriptorPublicKey enum. It's not pretty as some methods will need to panic or we'll need to sacrifice the ToPublicKey impl. But not being able to build a pkh(<hash>) descriptor is very annoying because one doesn't even know the pubkey.

That's a neat idea to generate only the actually-used signatures. Unfortunately it doesn't really fit into the workflows we imagined or the API of the Satisfier trait, and I don't think it's worth the added complexity to try to support it.

If you wanted to do this, it is possible to do outside the library, by writing your own type that knows how to sign (I guess it would include a sighash or means to compute this) and implementing Satisfier on it. But my feeling is that this is too niche to be worth including in the library.

Everything else you say @afilini sounds good to me! @sgeisler that does sound like the right method signature.

@sgeisler regarding a Hash variant, can you bring this up on IRC and ping me and sipa? Even with PSBT, I don't think there should be any situations where you don't know a public key but still want to create a descriptor.

Even I don't know why but there is an addr descriptor in bitcoin-core that is roughly equivalent to Hash. But, even I am curious about use-cases where one might need Hash descriptor.

Also, the current implementation of PSBT finalizer is incomplete. I hope to have PR for it tomorrow.

@afilini what do you think about extending xpub/xpriv derivation paths with arbitrary tweaks, which can be useful for situations like in RGB?

It's probably out of scope in my opinion, considering that you can very easily define your own MiniscriptKey types outside of this library. It's trivial to wrap DescriptorPublicKey and store whatever you need in your custom struct.

Hi @afilini, i'd be pretty interested to see this moving forward (at least the xpub part).
I'd happily help you with it, thought about making a PR for the DescriptorPublicKey of your above list mainly based on #93. What do you think ?

I think I will have some time this weekend to come back to this and finish it up. I think most of the code is already there, it needs cleanup and docs. I was also planning to add commit d9fa945 from @sgeisler in this branch since I think it could be useful.

The only thing that's missing with the "refactoring" I've done is the sighash computation part, which I was trying to rewrite as suggested to take a transaction instead of a PSBT, but for whatever reason I got kinda stuck (IIRC the interface was awful and I couldn't figure out a way to improve it) and never ended up finishing it. But since this PR doesn't do signing anymore maybe we could also go ahead without sighash and open a separate PR if we really want it.

The only thing (other than me rebasing and finishing the code) that could block merging this PR is the fact that it needs a git version of rust-bitcoin, since I don't think there has been a release yet since the required changes have been merged. I don't know the release schedule of rust-bitcoin, but I think it's been a while since the last release, so hopefully that's coming soon.

As a side note, I've been using this branch in some of my projects for the past few weeks and I don't think I've noticed any particular issue/regression with it, which is nice I guess.

The only thing that's missing with the "refactoring" I've done is the sighash computation part, which I was trying to rewrite as suggested to take a transaction instead of a PSBT, but for whatever reason I got kinda stuck (IIRC the interface was awful and I couldn't figure out a way to improve it) and never ended up finishing it. But since this PR doesn't do signing anymore maybe we could also go ahead without sighash and open a separate PR if we really want it.

Yeah, @sanket1729 has been doing some PSBT stuff related to sighash too lately if i'm not mistaking.

The only thing (other than me rebasing and finishing the code) that could block merging this PR is the fact that it needs a git version of rust-bitcoin, since I don't think there has been a release yet since the required changes have been merged. I don't know the release schedule of rust-bitcoin, but I think it's been a while since the last release, so hopefully that's coming soon.

It has just been bumped, and my cleaned up / improved version of #93 is rebased on top of #130 can be found here https://github.com/darosior/rust-miniscript/commits/descriptor_publickey (currently WIP, need some more tests squashed and raw pubkey with origin implementation: working on it locally).

Just rebased this on master, it's ready to be reviewed :)

Thanks, sorry it took me a while to fix this!

I think most of the comments should have been addressed: everything public is now documented and I removed pretty much all the ephemeral secp contexts except for one in Descriptor::parse_secret, mostly because I think it's not a critical method in terms of performance, and in general I think it keeps the public API a bit more "clean". But if you think this should also be removed and taken as argument I'm happy to change it, I don't have any strong opinion on that and it was just my personal preference.

Other than that, I don't think I see what you mean with your comment on Descriptor::to_string_with_secret: I mean, I can see that it does many allocations and it clones the public keys needlessly, but I don't think &String is a MiniscriptKey, so I don't think you can avoid cloning them, unless you completely change approach. If it's good for you to do that in a separate PR I could try to work on it, I guess I just need a bit of help!

What I was thinking with to_string_with_secret was that you'd make a type like

struct MapKey<'a, 'b> {
    map: &'a KeyMap,
    key: &'b DescriptorPublicKey,
}

then impl fmt::Display on this in a way that did the lookup and then either wrote the secret key or public key to the fmt::Formatter without allocating.

Unfortunately you still have to translate the input descriptor to a descriptor which uses this weird type as keys, so you can't avoid the allocations inherent in that.

Lol, I meant "audit" not "allocate"