PSBT responsibilities API according to BIP-174

[dr-orlovsky]

BIP-174 defines not just a structure and serialization for PSBTs, but also responsibilities — a well-defined roles of how PSBT can be modified by different actors, which is required for correct workflow organization. Right now PSBT implementation in the current rust-bitcoin code does not address this part of the spec; neither there any other rust implementation of it. Even PSBT signers (Firma and MagicalBitcoin) misses validation parts that are required by BIP-174.

While this library is not a wallet library, it seems that implementation of well-defined API for the responsibilities will be beneficial; plus we can implement non-signing part of PSBT validation described in this section of BIP-174. I am planning to work on that and do it as a number of decorators+facades, one per responsibility, at the same time limiting possible PSBT operations (facade) and implementing common responsibility business logic (like validation of the internal structure) as methods (decorator).

pub trait Responsibility {
  fn from_psbt(psbt: PartiallySignedTransaction) -> Self;
  fn into_psbt(self) -> PartiallySignedTransaction;
}

pub struct PsbtSigner {
  psbt: PartiallySignedTransaction,
}

impl Responsibility for PsbtSigner { /* ... */ }

impl PsbtSigner {
  pub fn valdiate(&self) -> Result<(), ValidationError> {
     // ....
  }

  pub fn sign<F>(&mut self, signer: F) -> Result<PartiallySignedTransaction> 
      where F: FnMut(Message, Fingerprint, DerivationPath) -> Signature {
     // ....
  }
}

// ... other responsibilities

Related PR in miniscript: rust-bitcoin/rust-miniscript#119

[sgeisler]

I think these responsibilities should be modeled as traits that say something has the capability to do some step. E.g.

trait PsbtSigner {
    type Error: Debug;

    fn sign(&self, psbt: PartiallySignedTransaction) -> Result<PartiallySignedTransaction, Self::Error>;
}

fn validate_psbt(&psbt: PartiallySignedTransaction) -> Result<(), PsbtValidationError>;

That would give the flexibility to implement HSM signers and software signers using the same API. Maybe we also want to provide a software signer that holds some keys in a pub->priv key map (as should be outputted from miniscript once rust-bitcoin/rust-miniscript#116) as a default and for testing. The validator could be just a public function that is called by the default signer and can be reused for other people's signers.

enum PsbtSignError { … }

struct SoftwareSigner {
    keys: HashMap<PubKey, PrivKey>,
}

impl PsbtSigner for SoftwareSigner {
    type Error = PsbtSignError;

    fn sign(&self, psbt: PartiallySignedTransaction) -> Result<PartiallySignedTransaction, Self::Error> {
        validate_psbt(&psbt)?;
        // do actual signing using keys in key map
       Ok(psbt)
    }
}

[dr-orlovsky]

@sgeisler I see the idea, but wouldn't it be nice (1) to have all non-wallet specific internal PSBT validation API in the rust-bitcoin (this renders responsibilities as structs, not traits), and (2) why the callback approach from my sample above for the signing procedure would not work in case of HSM signers? (of course it may be extended with Result return type)

[sgeisler]

non-wallet specific internal PSBT validation API in the rust-bitcoin

Imo having a function to do the validation is good enough. It could even be a member of PartiallySignedTransaction. A struct wrapping a PSBT generally seems like overcomplicating things. We can't model the PSBT workflow in the type system anyway without reimplementing PSBT in the type system. Maybe I'm missing somehting. I'm not that well-versed in the details of PSBT.

why the callback approach from my sample above for the signing procedure would not work in case of HSM signers

Your signature is FnMut(Message, Fingerprint, DerivationPath) -> Signature. That means the HSM wouldn't be aware of what it's signing (Message is just a hash after all, just named confusingly). A useful HSM needs nearly all the info from the PSBT anyway, some even speak PSBT. At that point the only thing your sign function would do is call the real sign function and put the signature into the PSBT. I wonder if the added complexity of wrapping the PSBT is worth it or if signers can't just impl the "put signature into PSBT" themselves, leading to my proposed API.

[dr-orlovsky]

I think that the code will be used on hardware device, not from outside. So sign procedure will be called by HSM-based code and the caller can verify PSBT at this stage. Still, there is a lot of work that can be automized on the library side with signing, like properly filling sigScript/witness data per input etc - no reason to let users of the library to repeat this functionality over and over again

[sgeisler]

I see your point of using the callback function now. Still, I find it quite odd that the signer owns the PSBT but nothing actually concerning signing (where to get the sig from). So you have to re-create it for every PSBT and externally supply the signing code.

What do you think about such an API: https://gist.github.com/rust-play/48ecc1455bf2d6e9f15278b37db15c3a ?

It would combine both approaches in a way. It supplies a default signer that can easily be used on a hardware device, while also allowing arbitrary other implementations using the same trait (e.g. a trezor/ledger client).

[dr-orlovsky]

Looks good to me! I will take this approach, if no other suggestions will follow

[sgeisler]

One thing you might want to consider is if you can allocate on the embedded platform you were thinking about, i.e. if you can use Box<dyn T>.

[dr-orlovsky]

There is no allocation planned internally of that type; and for those who use the library I assume they will need to use generics if they have to store different concrete implementations of some responsibility trait.

[Kixunil]

Why not just use a trait SignMessage instead of Box<dyn Fn...> and having generic DefaultSigner?
The trait could be implemented for Box<dyn SignMessage> so that if someone really wants Box, it'd still be possible.

Also, I'd suggest following Rust naming convention and name the trait SignPsbt instead of PsbtSigner.

[sgeisler]

@Kixunil That sounds great! I don't think we'd need Boxes at all anymore. Just impl SignMessage for function pointers and closures that may be able to (=return Option) sign a message with a specified key and also for xprivs. Then implement SignPsbt for all types implementing SignMessage. That gives users good default options, but also leaves the possibility to implement SignPsbt in a completely different manner (e.g. talking to a HW wallet).

[thomaseizinger]

For what it is worth, I actually recently played around with signing a psbt using a Nano Ledger S in Rust: https://github.com/thomaseizinger/rust-ledger-poc/blob/master/examples/sign_psbt.rs

For some weird reason, it doesn't fully work as in: the Ledger is not happy with the way it is communicated with but that is a concern of lib implementing the communication (https://github.com/summa-tx/bitcoins-rs/tree/master/ledger-btc) and may very well be fixed in the future as the lib matures. The main structure is there though :)

My point is, at least for now, what is exposed by this libraries Psbt module is enough to make this work. The major issue is the converting back and forth between the types of this lib and what is used in "bitcoins-rs".

From my experience working with the Nano Ledger S, it is unfortunately not as simple as "sign this message". Instead, the Ledger exposes a stateful protocol in which one has to send over bits of the transaction one by one. Here is the spec: https://blog.ledger.com/btchip-doc/bitcoin-technical-beta.html#_untrusted_hash_transaction_input_start

That means for anyone wanting to integrate with Ledger, the most important thing is that all the data of the transaction is nicely accessible and can easily be converted into whatever byte representation the messages expect.

Another thing to consider is, signing with a HW wallet involves confirmation by the user and hence reading from the USB interface would be blocking, hence the sign function should maybe be async?

[Kixunil]

Ledger exposes a stateful protocol in which one has to send over bits of the transaction one by one.

I guess Ledger implementation would need to have additional code, but maybe all impls will need additional code.

sign function should maybe be async

Oh, good point. Trait functions can't be async now but the trait can return Future instead. There could be another trait for sync signing if the key is in memory and then a trivial implementation of async version for implementors of the sync version.

[thomaseizinger]

I guess Ledger implementation would need to have additional code, but maybe all impls will need additional code.

Yes certainly. I just wanted to mention that for said implementation, being able to pick out specific data is basically a must for now. Maybe we will get a native psbt signing interface at some point though :)

[dr-orlovsky]

I drafted #456, can you pls have a look and give your suggestions? It seems like it meets all of the discussed points. With hardware devices, I think they just have to implement their own version of SignPsbt trait with handling all asynchronicity internally - I doubt that rust-bitcoin maintainers would like to introduce futures to the library just because of PSBTs...

[Kixunil]

When it comes to dependency, the Future trait is now in core. When it comes to complexity, it kinda sucks but from my experience ignoring the problems around blocking makes them worse. Still, blocking versions can exist.

[sgeisler]

What do you think about defining a sync and an async version of this trait? We could define the contract of the sync version in a way that it isn't allowed to block, that would enable us to automatically impl the async version too in these cases (just calling the sync version in an async fn).

[dr-orlovsky]

@Kixunil it's there since 1.36, which does not fit this project MSRV 1.22
It's up to repo maintainers to decide whether they would like to add futures dependency to the project - or lift MSRV (but I doubt they will agree on 1.36 - see #338). Over a multiple occasions before they were strictly against

[sgeisler]

What do you think about defining a sync and an async version of this trait? We could define the contract of the sync version in a way that it isn't allowed to block, that would enable us to automatically impl the async version too in these cases (just calling the sync version in an async fn). That would make for a beautiful hierarchy imo, the looser your requirements the more signing backends you can use, but you don't have to.

We could add the async version at a later time once the MSRV discussion is concluded. I think that futures will be a significant enough language feature to consider 1.36.

[dr-orlovsky]

@sgeisler I'm up for your idea, but it seems we are blocked right now by MSRV. I have already wrote to the discussion on that topic.

[sgeisler]

it seems we are blocked right now by MSRV

How so? We can do the first, sync part now and later add the async trait imo. Or are there any other features we need for the sync version that aren't available right now?

[dr-orlovsky]

Oh, in this way (only sync version). So you mean just to add Sync to trait/struct names?

[Kixunil]

AH, forgot about MSRV, good point. Could be behind a feature flag, I guess?

[sgeisler]

just to add Sync to trait/struct names

Maybe not even that. Let's just build a sync API first that can be easily integrated into an async one later (which will add exactly one trait with async in the name per "complex responsibility"). If our sync traits are specified to not block we can impl the async version for every struct implementing the sync version of the traits later, either feature flagged or in a completely different crate should it be too contentious.

[elichai]

I did not read the whole thread, BUT
we can make whatever we want async without using the async keyword by Using std::future::Future directly.
example: https://play.rust-lang.org/?gist=5573e840a298758194f200691a83747f

[dr-orlovsky]

I did not read the whole thread, BUT

we can make whatever we want async without using the async keyword by Using std::future::Future directly.

example: https://play.rust-lang.org/?gist=5573e840a298758194f200691a83747f

@elichai It wouldn't work on Rust 1.22

[elichai]

I totally forgot that std::future::Future itself only got into in 1.36 :(
but maybe we could use something like https://rust-lang.github.io/futures-rs/blog/2019/04/18/compatibility-layer.html if we really wanted

[dpc]

I think having set of common traits etc for interoperability be part of rust-bitcoin project is great, though I wonder if it should be a part of main rust-bitcoin crate. Is it necessary? Any benefits? Would it work better if it was just another project under main rust-bitcoin umbrella?

[dr-orlovsky]

@dpc I thought about putting PSBTs into a separate crate. Would the community support that?

[sgeisler]

@dpc I thought about putting PSBTs into a separate crate. Would the community support that?

Removing functionality from rust-bitcoin would certainly suck. And the PSBT crate would depend on it, so we couldn't even just reexport the types to avoid that.

I also wrote a partial API proposal in #457. I still have to do some research on finalization, but it will either end up like combine or like sign (depending on if it needs external data). So I'd be happy to hear your thoughts on the proposal.

[dr-orlovsky]

Closing it after all discussions in #457 and #456: seems like it is up to downstream libs and apps to define the best API for specific PSBT workflows