PSBT responsibilities #456

dr-orlovsky · 2020-08-11T13:06:21Z

Implements #455

The discussion happens under the original issue (at least until concept Ack)

Kixunil · 2020-08-11T15:14:00Z

src/util/psbt/responsibilities.rs

+/// PSBT signing verification error
+#[derive(Debug)]
+pub enum VerificationError {
+}


Are the variants ignored for the purpose of WIP it's meant to be uninhabited?

This whole PR right now is just a mockup to agree on the planned API, so no actual data here

Kixunil · 2020-08-11T15:14:45Z

src/util/psbt/responsibilities.rs

+}
+impl Display for VerificationError {
+    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
+        Ok(())


If it's uninhabited then match *self {} expresses the intent cleanly.

Kixunil · 2020-08-11T15:17:04Z

src/util/psbt/responsibilities.rs

+    type SigningError: Error + From<Self::VerificationError>;
+
+    /// Verifies PSBT structure against BIp-174 rules
+    fn verify(&self) -> Result<(), Self::VerificationError> {


Would it make sense to take it by value and return it wrapped as Verified<Self> to aid static checking?

I don't think that would be too useful as the verification is part of signing afaik. So it's just a convenience function so that different signer impls can share it. Verified PSBTs aren't supposed to be passed around much afaik.

Kixunil · 2020-08-11T15:18:21Z

src/util/psbt/responsibilities.rs

+        Ok(())
+    }
+
+    /// Signs all PSBT inputs


If it signs all the inputs, shouldn't it return SignedTransaction instead of PartiallySignedTransaction? If not then the doc should be updated.

This is done in accordance to BIP-174 responsibilities; and at this stage we are not producing the final Transaction. Not sure what you mean by SignedTransaction

The problem is that we might not be able to sign all inputs in a multisig setup, so it would take multiple sign phases. That's why I think modelling the PSBT workflow in the type system is not really worth it.

Fair, in that case the doc should be worded as "Sign all required PSBT inputs" or something similar.

Kixunil · 2020-08-11T15:19:49Z

src/util/psbt/responsibilities.rs

+{
+    psbt: PartiallySignedTransaction,
+    /// External signing function
+    pub sign_fn: Option<F>,


Why Option?

Otherwise it's just impossible to do impl Responsibility for DefaultSigner - you will fail at struct initialization in from_psbt

Kixunil · 2020-08-11T15:20:45Z

src/util/psbt/responsibilities.rs

+    psbt: PartiallySignedTransaction,
+    /// External signing function
+    pub sign_fn: Option<F>,
+    _err: PhantomData<E>,


AFAIK the correct way to write it for functions returning E is PhantomData<fn() -> E>.

Kixunil · 2020-08-11T15:21:54Z

src/util/psbt/responsibilities.rs

+    /// External signing function
+    pub sign_fn: Option<F>,
+    _err: PhantomData<E>,
+    _si: PhantomData<SigningInfo>,


AFAIK the correct way to write it is PhantomData<fn(SginingInfo)>

Or maybe it's possible to put them together:

_marker: PhantomData<fn(SigningInfo) -> E>

sgeisler

I'm unconvinced that having a Responsibility trait is useful. The API seems overly cumbersome. A signer should be an object that can do something with a PSBT imo and not the PSBT itself that now has some more features. That might make sense if you are coming from a more traditional OO background but it really doesn't in rust. I'll try to code up an alternative approach soon.

If it turns out having a responsibility trait is actually useful in some case I'd prefer it to define a transformation of a PSBT:

trait Responsibility {
    type Error;

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

sgeisler · 2020-08-11T17:01:13Z

src/util/psbt/responsibilities.rs

+    where F: Fn(SigningInfo, Fingerprint, DerivationPath) -> Result<Option<Signature>, E>,
+          E: Error
+{
+    psbt: PartiallySignedTransaction,


Why would you want the signer to own the PSBT?

Responsibilities add state machine over PBST; meaning that PBST can be only is a particular responsibility state, not their mix. That is why I found decorator/facade more useful

Does explicitly modeling the state machine give us any big advantages? I'm all for enforcing invariants through the type system, but the PSBT process isn't all that complicated and at least in the signing stage the guarantees it can give are limited (multiple signing rounds).

If we really want to go this way of enforcing the correct life cycle of a PSBT through the type system I'd build it using newtypes (kinda like you did), but in the following way:

Every state a PSBT can be is has a newtype with

a constructor that checks the invariants and returns a Result

a function to turn it back into a plain PSBT

side note: e.g. a PSBT that can be signed is in the state updated and stays there until it's finalized

Trivial state transitions that don't require external data (combiner, finalizer, extractor) can be modeled using member functions of the newtypes that represent the source state and transforms it into the target state, possibly returning an error

Nontrivial state transitions (update, sign, maybe create?) that need external data should be modeled as traits that define the state transition as a function consuming the source state newtype and returning a result with the target state newtype

we may supply a default impl, e.g. an extended private key or any type implementing Fn with a certain signature is a signer or an updater based on bitcoincore-rpc (if that makes sense)

but we always leave the door open for other implementations and allow abstracting over them (e.g. this function needs an argument that can act as a PSBT signer, we don't care how it works inside)

This would add a lot of complexity, but might be worth it. One annoying thing with this is that external implementors of these state transition trait's can't be allowed to directly create state newtypes (to guarantee the invariants). They'll have to go through the fallible constructors and probably unwrap if they know they did everything correctly.

EDIT: diagram of the state transitions as I imagine them

Hm, this is exactly what I am trying to do, I do not see where we are different that much (agree with constructor that checks invariants). I just drafted a single state related to signing, but just to align on APIs, before implementing the rest.

EDIT: diagram of the state transitions as I imagine them

My intent was to implement them according to the spec BIP-174, where they are quite different

My intent was to implement them according to the spec BIP-174, where they are quite different

That might be where our disagreement stems from, how do you interpret BIP-174? I see a few points where my model doesn't cover all possible workflows (multiple/out of order updates), but if we take that into account will we still gain any safety from modeling a workflow? When you look at this example from the BIP:

There's a giant state in which arbitrary updates/signatures can happen and there's no sense in modeling two independent states. You'd be left with the created and finalized state. But who cares about the finalized state anyway? All we want is the extracted BTC tx at that point. That's part of the reason why I'm doubtful that we should implement these states in the type system.

sgeisler · 2020-08-11T17:02:45Z

src/util/psbt/responsibilities.rs

+use super::PartiallySignedTransaction;
+
+/// Trait for a generic PSBT responsibility (decorator+facade)
+pub trait Responsibility where Self: Sized {


Is this responsibility abstraction actually useful? What's the intended use case?

There might be functions taking PSBT from a responsibility and internally extracting from it - or packing into it.

dr-orlovsky · 2020-09-10T11:24:10Z

Closed in favor of #472

dr-orlovsky mentioned this pull request Aug 11, 2020

PSBT responsibilities API according to BIP-174 #455

Closed

Mockup for PSBT responsibilities

9a3538f

Kixunil reviewed Aug 11, 2020

View reviewed changes

sgeisler reviewed Aug 11, 2020

View reviewed changes

sgeisler mentioned this pull request Aug 14, 2020

PSBT interface mockup #457

Closed

6 tasks

dr-orlovsky mentioned this pull request Sep 10, 2020

Refactoring PSBT with roles according to BIP 174 #472

Closed

dr-orlovsky closed this Sep 10, 2020

dr-orlovsky mentioned this pull request Sep 10, 2020

[epic] PSBT and related BIP 32 refactoring tracking issue #473

Closed

13 tasks

dr-orlovsky added this to Done in PSBT Jan 30, 2021

clarkmoody mentioned this pull request Sep 22, 2023

Add changelog entries #1952

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PSBT responsibilities #456

PSBT responsibilities #456

dr-orlovsky commented Aug 11, 2020

Kixunil Aug 11, 2020

dr-orlovsky Aug 11, 2020 •

edited

Kixunil Aug 11, 2020

Kixunil Aug 11, 2020

sgeisler Aug 11, 2020

Kixunil Aug 11, 2020

dr-orlovsky Aug 11, 2020

sgeisler Aug 11, 2020

Kixunil Aug 11, 2020

Kixunil Aug 11, 2020

dr-orlovsky Aug 11, 2020

Kixunil Aug 11, 2020

Kixunil Aug 11, 2020

sgeisler left a comment

sgeisler Aug 11, 2020

dr-orlovsky Aug 11, 2020 •

edited

sgeisler Aug 11, 2020 •

edited

dr-orlovsky Aug 11, 2020 •

edited

dr-orlovsky Aug 11, 2020

sgeisler Aug 11, 2020

sgeisler Aug 11, 2020

dr-orlovsky Aug 11, 2020

dr-orlovsky commented Sep 10, 2020

PSBT responsibilities #456

PSBT responsibilities #456

Conversation

dr-orlovsky commented Aug 11, 2020

Choose a reason for hiding this comment

dr-orlovsky Aug 11, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sgeisler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dr-orlovsky Aug 11, 2020 • edited

Choose a reason for hiding this comment

sgeisler Aug 11, 2020 • edited

Choose a reason for hiding this comment

dr-orlovsky Aug 11, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dr-orlovsky commented Sep 10, 2020

dr-orlovsky Aug 11, 2020 •

edited

dr-orlovsky Aug 11, 2020 •

edited

sgeisler Aug 11, 2020 •

edited

dr-orlovsky Aug 11, 2020 •

edited