PSBT interface mockup by sgeisler · Pull Request #457 · rust-bitcoin/rust-bitcoin

sgeisler · 2020-08-13T17:20:31Z

Alternative idea for #455

Currently it covers the following functionalities:

Create (PsbtWallet)
Update (PsbtWallet)
Sign (SignPsbt)
Combine (through PartiallySignedTransaction::merge)
Finalize (Psbt: Add finalizer and extractor rust-miniscript#119)
Extract (Psbt: Add finalizer and extractor rust-miniscript#119)

I wouldn't consider merging any of the PSBT interface PRs before every trait was implemented at least once and a working wallet could be built using these. But I consider this a good basis for discussion.

sgeisler · 2020-08-13T17:28:20Z

src/util/psbt/responsibilities.rs

This should be more fine-grained.

sgeisler · 2020-08-13T17:30:42Z

src/util/psbt/responsibilities.rs

Maybe we want to have a different trait that allows this particular privacy optimization to avoid panics/useless error conditions.

dr-orlovsky

Not sure I understand core API differences with the other proposal (#456); and in parts where I see API diffs I left some questions

src/util/psbt/responsibilities.rs

sgeisler · 2020-08-14T17:16:23Z

@dr-orlovsky The main difference to #456 is that here the objects implementing responsibilities never hold the PSBT, but only the state they need to repeatedly perform their functionality and are thus reusable. As noted in the discussion in #456 enforcing a certain PSBT workflow seems futile, so there is no wrapping of PSBTs going on here. The notion of an abstract Responsibility is not present in this PR, but could be added if there's a use case for that. All defined traits only transform PSBTs.

dr-orlovsky · 2020-08-14T17:18:42Z

Oh, I see. My only objection that this leads, as a direct consequence, to the ability to change PSBT outside of normal flow, so we do not have a state machine of responsibilities chaining each other

sgeisler · 2020-08-14T17:23:55Z

Such a state machine sounds nice in theory, but in practice we can't enforce that much without an overly complex representation of the PSBT in the type system. Looking at the example workflows in the BIP I'm pretty sure that PSBT is too general for this to be useful.

dr-orlovsky · 2020-09-10T11:25:16Z

I did an alternative implementation, pls see #456

stevenroose

I don't really understand what the goal is of these kind of traits. Is it just a way to provide some basic signing code that a wallet can like "plug" into? I mean it seems to me that these traits would for almost all applications stay within the application, adding little value by making the traits "common".

The code that generalizes signing over a xpriv is useful though. But it seems to be a hassle to create all this framework just to provide that code?

I'd be curious though to experiment with this in rust-wallet!

stevenroose · 2020-10-10T17:26:10Z

src/util/psbt/responsibilities.rs

+    fn create_transaction(
+        &mut self,
+        outputs: &[TxOut],
+    ) -> Result<PartiallySignedTransaction, PsbtCreationError<Self::Error>>;


Is this method supposed to create a transaction without inputs or is the implementor responsible for adding inputs to fulfill the tx?

Yes, it's the implementor's responsibility to select UTXOs, that's an integral part of every wallet imo: tracking and selecting UTXOs.

stevenroose · 2020-10-10T17:26:44Z

src/util/psbt/responsibilities.rs

+#[derive(Debug)]
+pub enum PsbtCreationError<E: Debug> {
+    /// The wallet doens't control a sufficient amount of Bitcoins to fund the transaction
+    InsufficientFunds,


This probably answers my earlier question.

stevenroose · 2020-10-10T17:27:47Z

src/util/psbt/responsibilities.rs

+    fn sign_psbt(
+        &mut self,
+        psbt: PartiallySignedTransaction,
+    ) -> Result<PartiallySignedTransaction, PsbtSignError<Self::Error>>;


Is it on purpose that this clones the entire PSBT instead of having a mutable reference? Perhaps 2 variants of this method could be provided? One could be provided by using the other.

It doesn't clone. It takes it by value, mutates it and either returns the altered version or an error. The problem with mut references is that it's much too easy to leave the PSBT in an invalid state in case of an early exit due to an error.

stevenroose · 2020-10-10T17:31:14Z

src/util/psbt/responsibilities.rs

+            let key = match self.derive_priv(&ctx, &path) {
+                Ok(key) => key,
+                Err(crate::util::bip32::Error::Ecdsa(e)) => return Err(e),
+                _ => unreachable!(),


This is dangerous.

Oh, yeah, I mostly hacked that together to test the API.

sgeisler · 2020-10-10T18:07:07Z

I'll leave some comments, but I consider this whole endeavor to be futile. rust-bitcoin probably won't be able to provide a sufficiently general interface without too much feature-creep (see this discussion). It might still be worthwhile for the signer trait, I really want to have a native HWI interface somehow, but it would need a lot of deliberation upfront. So I'll close this for now.

dr-orlovsky · 2020-10-10T20:01:33Z

Agree, closing my version as well

sgeisler commented Aug 13, 2020

View reviewed changes

src/util/psbt/responsibilities.rs Outdated

Copy link
Copy Markdown

Contributor Author

sgeisler Aug 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be more fine-grained.

sgeisler commented Aug 13, 2020

View reviewed changes

sgeisler mentioned this pull request Aug 13, 2020

PSBT responsibilities API according to BIP-174 #455

Closed

sgeisler force-pushed the 2020-08-psbt-interfaces branch from 2d9a42b to a7f3149 Compare August 14, 2020 16:39

dr-orlovsky reviewed Aug 14, 2020

View reviewed changes

src/util/psbt/responsibilities.rs Outdated Show resolved Hide resolved

src/util/psbt/responsibilities.rs Outdated Show resolved Hide resolved

src/util/psbt/responsibilities.rs Outdated Show resolved Hide resolved

sgeisler force-pushed the 2020-08-psbt-interfaces branch from a7f3149 to bde457a Compare August 14, 2020 17:07

sgeisler force-pushed the 2020-08-psbt-interfaces branch 3 times, most recently from cecea5d to 649a166 Compare August 14, 2020 17:52

WIP PSBT interface mockup

22d2730

sgeisler force-pushed the 2020-08-psbt-interfaces branch from 649a166 to 22d2730 Compare August 14, 2020 17:53

dr-orlovsky mentioned this pull request Sep 10, 2020

Refactoring PSBT with roles according to BIP 174 #472

Closed

dr-orlovsky mentioned this pull request Sep 10, 2020

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

Closed

13 tasks

sgeisler mentioned this pull request Sep 23, 2020

PSBT workflow and traits bitcoindevkit/bdk#100

Closed

stevenroose reviewed Oct 10, 2020

View reviewed changes

sgeisler closed this Oct 10, 2020

Conversation

sgeisler commented Aug 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dr-orlovsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sgeisler commented Aug 14, 2020

Uh oh!

dr-orlovsky commented Aug 14, 2020

Uh oh!

sgeisler commented Aug 14, 2020

Uh oh!

dr-orlovsky commented Sep 10, 2020

Uh oh!

stevenroose left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sgeisler commented Oct 10, 2020

Uh oh!

dr-orlovsky commented Oct 10, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sgeisler commented Aug 13, 2020 •

edited

Loading