Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Moving bunch of stuff from my payjoin crate here #630

Open
Kixunil opened this issue Jul 15, 2021 · 30 comments
Open

Moving bunch of stuff from my payjoin crate here #630

Kixunil opened this issue Jul 15, 2021 · 30 comments

Comments

@Kixunil
Copy link
Collaborator

Kixunil commented Jul 15, 2021

I'm working on a PayJoin (BIP78) crate (not released yet) and I was thinking of migrating some stuff from there over here. But first I'd like to ask whether/which things you find worthy moving.

Stuff that seems to be interesting to me:

  • BIP21 parsing and serializing
  • Extension trait for weight calculation on TxIn, TxOut etc, so that one may for instance predict how much transaction fee will change (would use inherent methods here)
  • Weight data type - newtype around u64 for transaction weight, returned by weight-computing functions
  • FeeRate newtype over u64, returned by Amount / Weight
  • Function for computing fee paid by PSBT (needs UTXO information of course)
  • Function for computing the size of finalized PSBT
  • Helper type replacing (&bitcoin::Txin, &bitcoin::util::psbt::Input) with a method for getting previous TxOut and possibly other helpers

Questionable stuff:

  • IntputType - enum of possible input types (P2PK, P2PKH, P2SH, P2SH-P2W*, P2W*, P2TR), may interact nicely with WitnessVersion type #617
  • OutputType - analogous to InputType

There's also some stuff I'd like to be helped with if someone is interested:

  • writing PSBT finalizer - is someone here experienced to do it?
  • reviewing my code :)
  • if someone here enjoys writing tests I'd be happy to let you write them

If approved, I'd submit the changes in separate PRs.

I'm also willing to move the whole crate to this organization under the condition that I will have commit access for at least a few months during initial development (may have WIP disclaimer).

@thomaseizinger
Copy link
Contributor

writing PSBT finalizer - is someone here experienced to do it?

There is a PSBT finalizer in the rust-miniscript crate!

@Kixunil
Copy link
Collaborator Author

Kixunil commented Jul 15, 2021

@thomaseizinger thanks for heads up! rust-miniscript feels like the wrong place to put it but maybe there's something I don't know. Would appreciate if someone can explain whether it makes sense.

@thomaseizinger
Copy link
Contributor

thomaseizinger commented Jul 15, 2021

What I was getting at is that I think you don't need to write anything!

To my knowledge rust-miniscript can take a PSBT and finalize it to a valid transaction if the PSBT contains all the valid signatures.

(I am writing this with only having glanced over bip78 and may have overlooked some specifics on why this might not work.)

@Kixunil
Copy link
Collaborator Author

Kixunil commented Jul 15, 2021

Yeah, that's right. I don't think BIP78 has anything specific. It just says the transaction has to be finalized and that word seems to be well defined.

@RCasatta
Copy link
Collaborator

not released yet

any ETA? :)

@Kixunil
Copy link
Collaborator Author

Kixunil commented Jul 27, 2021

any ETA? :)

The client (sender) side is essentially written (no correctness guarantees) and an example with bitcoind backend works with loptos and I vaguely remember testing with BTCPayServer but I think it failed for some unrelated reason.

I want to write a bunch of proper tests and for that writing server (receiver) side seems like the best approach but I got dragged by API complexity and some other stuff.

I could try to polish some basic things tomorrow (CET) and publish it as WIP. Especially if you'd like to help. :)

@RCasatta
Copy link
Collaborator

The client (sender) side is essentially written (no correctness guarantees) and an example with bitcoind backend works with loptos and I vaguely remember testing with BTCPayServer but I think it failed for some unrelated reason.

I am going to have a look at loptos

I could try to polish some basic things tomorrow (CET) and publish it as WIP. Especially if you'd like to help. :)

I am not in hurry, but I am interested in handling bip78 in bdk in the future and I will love to use shared libs, so yes I'd like to help

@Kixunil
Copy link
Collaborator Author

Kixunil commented Jul 28, 2021

Done: https://github.com/Kixunil/payjoin

Some updates:

  • I fixed some ridiculous bug so now it works with BTCPayServer 🎉
  • I forgot to mention that min fee rate is not enforced yet. Shouldn't be too much work.
  • Added some docs so hopefully you can get started quickly

Feel free to open an issue there for discussion, if you need to!

@DanGould
Copy link
Contributor

DanGould commented Jul 4, 2022

Hey @Kixunil This PayJoin crate is awesome. I've written the PayJoin receiver a couple times now in C# so I know where integration snags are. I'm interested in seeing sender-receiver functioning as a way to advance the PSBT epic here, too. I bet rust-bitcoin has changed in the year since you posted this. Now seems to be a good time to revisit. What's your plan to license it?

writing PSBT finalizer - is someone here experienced to do it?

Finalizer code is sneaking into rust-bitcoin via the cold storage example #940 and is also in bdk at this point. Maybe that project would be a better fit for a PayJoin crate? I'm only now starting to figure out how the two projects coordinate.

Helper type replacing (&bitcoin::Txin, &bitcoin::util::psbt::Input) with a method for getting previous TxOut and possibly other helpers

PSBTv2 includes this data if I understand correctly

@Kixunil
Copy link
Collaborator Author

Kixunil commented Jul 4, 2022

@DanGould cool! I don't have much time for working on it lately but will be happy to help with reviews and maybe other things if possible.

What's your plan to license it?

My default license for libraries is MITNFA, but will happily re-license relevant parts for this library if needed.

Finalizer code is sneaking into rust-bitcoin via the cold storage example

I believe it'd be best to provide finalizer as a first-class PSBT method. Then all other projects can just call that and we remove a bunch of duplication.

PSBTv2 includes this data if I understand correctly

Yep, the v2 API specifically. It's even better than my helpers. But maybe there will still be useful things.

@apoelstra
Copy link
Member

rust-miniscript feels like the wrong place to put it

You need to be able to understand scripts and how to construct witnesses -- I'm not sure what crate in this ecosystem would be more suited. I'm also surprised at how many people here are reimplementing finalization logic when (one of the) whole point of Miniscript was to make this a turnkey thing.

Since I seem to have missed this issue when it was opened, I like all the ideas you propose for bringing into this crate. BIP21 parsing I worry might need an external url parsing crate? And computing fee deltas based on change seems like it might require miniscript. But I'd have to see the details in both cases.

@Kixunil
Copy link
Collaborator Author

Kixunil commented Jul 4, 2022

I guess I will have to study Miniscript more. :)

BIP21 parsing I worry might need an external url parsing crate?

The url crate is unsuitable because it implements a different standard of percent encoding. I ended up forking percent_encoding to support RFC3968 and wrote bip21 on top of that. Will be happy to share maintainership.

@DanGould
Copy link
Contributor

DanGould commented Jul 5, 2022

rust-miniscript feels like the wrong place to put it [a psbt finalizer]

You need to be able to understand scripts and how to construct witnesses -- I'm not sure what crate in this ecosystem would be more suited.

Understanding scripts and witnesses doesn't scream rust-miniscript to me. Miniscript crate is not the first place I'd look, since it's additional. Miniscript still seems esoteric and advanced to me even though it applies to the majority of applications by design.

Anywhere there's PSBT code I expect a finalizer, even if it had documented limitations.

@apoelstra
Copy link
Member

Understanding scripts and witnesses doesn't scream rust-miniscript to me. Miniscript crate is not the first place I'd look, since it's additional.

We should probably add it to the README here or something. If I needed to deal with scripts or witnesses in any way beyond reserializing them or hashing scriptpubkeys into addresses, I'd immediately turn to Miniscript. You basically can't work on scripts without it, unless you are doing something very specific.

Anywhere there's PSBT code I expect a finalizer, even if it had documented limitations.

Not to be too glib, but suppose we had a finalizer that just always returned an error, and was documented not to finalize any transactions. Would that meet your expectations? It is hard to do much more without Miniscript.

@DanGould
Copy link
Contributor

DanGould commented Jul 6, 2022

Are there examples of rust-bitcoin psbt uses that do not require a finalizer? Maybe psbt belongs there too, or else I'm otherwise misunderstanding the separation. You basically can't work on psbts without it

@apoelstra
Copy link
Member

Sure, every role except Finalizer, plus parsing/serialization, would be an example.

apoelstra added a commit that referenced this issue Jul 17, 2022
896ca42 Document PSBT roles and limitation (DanGould)

Pull request description:

  The READEME claims rust-bitcoin supports PSBT finalization, but really needs rust-miniscript for that. I think we should make this clear in this crate's PSBT examples as well.

  > > Understanding scripts and witnesses doesn't scream rust-miniscript to me. Miniscript crate is not the first place I'd look, since it's additional.
  >
  > We should probably add it to the README here or something. If I needed to deal with scripts or witnesses in any way beyond reserializing them or hashing scriptpubkeys into addresses, I'd immediately turn to Miniscript. You basically can't work on scripts without it, unless you are doing something very specific.
  >
  > >Anywhere there's PSBT code I expect a finalizer, even if it had documented limitations.
  >
  > Not to be too glib, but suppose we had a finalizer that just always returned an error, and was documented not to finalize any transactions. Would that meet your expectations? It is hard to do much more without Miniscript.
  >

  _Originally posted by apoelstra in #630 (comment)

ACKs for top commit:
  apoelstra:
    ACK 896ca42
  tcharding:
    ACK 896ca42

Tree-SHA512: e71a65b8c04134d9b3406ea76bb915fa116e4a961f9f6cb24350422f9d550cba26a630e02f9ba352fae63076926532bc4bf2d1001488666a05f18d7774ddda9e
@sanket1729
Copy link
Member

@DanGould, the finalizer would basically be

pub fn finalize_psbt(&mut self, finalize: F) where F: Fn(&Pbst) -> (Script, Witness) {
	/// construct witness from components. 
	let (final_script, final_witness) = finalize(&self);
	self.final_script_sig = final_script;
	self.final_witness = final_witness;
	
	// Clear out all the unnecessary elements from psbt.
	self.partial_sigs().clear(); // and all other fields.
}

I don't see how this is helpful for the user because they have to supply the finalize function. Or, are you suggesting that we add finalization only for p2pkh, p2wpkh? And not deal with other cases. The issue is really that Psbts are most helpful when dealing with multiple parties. And when dealing with multiple parties, these scripts are complicated.

@DanGould
Copy link
Contributor

The latter was what I was thinking. That would support PayJoin senders to single sig receivers of existing implementations (btcpayserver, sparrow, joinmarket, bluewallet, wasabi, loptos lnd channel opening plugin). Such a finalizer would also support single sig cold storage spend as in our psbt example prs.

After this back and forth I'm not as gung ho to add it just for those uses. rust-miniscript is so much more complete and compatible anyway.

@Kixunil
Copy link
Collaborator Author

Kixunil commented Jul 19, 2022

@sanket1729 that explains my confusion as well. I guess that there is a bunch of cases where only standard key spends are used - payjoins, cold wallets and probably also CoinJoins. Seemed worth writing such a simple finalizer as a method on Psbt but it may still be too much work or I may still be misunderstanding something.

@apoelstra
Copy link
Member

I wouldn't mind supporting only p2pkh and p2wpkh in this crate. I was not aware that this would be useful.

@sanket1729
Copy link
Member

I wouldn't mind supporting only p2pkh and p2wpkh in this crate. I was not aware that this would be useful.

+1. I am also not opposed to adding this functionality.

I also agree that miniscript is not the first place you look for the finalization code. We should add documentation regarding the finalization code being present in rust-miniscript. Maybe we can even add a miniscript dev-dependency and add an example miniscript finalization here.

ChallengeDev210 pushed a commit to ChallengeDev210/rust-bitcoin that referenced this issue Aug 1, 2022
896ca42 Document PSBT roles and limitation (DanGould)

Pull request description:

  The READEME claims rust-bitcoin supports PSBT finalization, but really needs rust-miniscript for that. I think we should make this clear in this crate's PSBT examples as well.

  > > Understanding scripts and witnesses doesn't scream rust-miniscript to me. Miniscript crate is not the first place I'd look, since it's additional.
  >
  > We should probably add it to the README here or something. If I needed to deal with scripts or witnesses in any way beyond reserializing them or hashing scriptpubkeys into addresses, I'd immediately turn to Miniscript. You basically can't work on scripts without it, unless you are doing something very specific.
  >
  > >Anywhere there's PSBT code I expect a finalizer, even if it had documented limitations.
  >
  > Not to be too glib, but suppose we had a finalizer that just always returned an error, and was documented not to finalize any transactions. Would that meet your expectations? It is hard to do much more without Miniscript.
  >

  _Originally posted by apoelstra in rust-bitcoin/rust-bitcoin#630 (comment)

ACKs for top commit:
  apoelstra:
    ACK 896ca42
  tcharding:
    ACK 896ca42

Tree-SHA512: e71a65b8c04134d9b3406ea76bb915fa116e4a961f9f6cb24350422f9d550cba26a630e02f9ba352fae63076926532bc4bf2d1001488666a05f18d7774ddda9e
@DanGould DanGould mentioned this issue Feb 1, 2023
4 tasks
@DanGould
Copy link
Contributor

DanGould commented Feb 2, 2023

I see the new FeeRate type abstracts over u64. It seems that would exclude rational Amount / Weight values. Would it not make more sense to use a float? Or abstract over Amount / Weight as an exact rational number?

@Kixunil
Copy link
Collaborator Author

Kixunil commented Feb 3, 2023

In my new project I realized the same thing and decided to solve it the same way Core did: I operate on sat/1kwu. I suggest we do it here as well.

Kixunil added a commit to Kixunil/rust-bitcoin that referenced this issue Feb 6, 2023
Use of general-purpose integers is often error-prone and annoying. We're
working towards improving it by introducing newtypes.

This adds newtypes for weight and fee rate to make fee computation
easier and more readable. Note however that this dosn't change the type
for individual parts of the transaction since computing the total weight
is not as simple as summing them up and we want to avoid such confusion.

Part of rust-bitcoin#630
Kixunil added a commit to Kixunil/rust-bitcoin that referenced this issue Feb 6, 2023
Use of general-purpose integers is often error-prone and annoying. We're
working towards improving it by introducing newtypes.

This adds newtypes for weight and fee rate to make fee computation
easier and more readable. Note however that this dosn't change the type
for individual parts of the transaction since computing the total weight
is not as simple as summing them up and we want to avoid such confusion.

Part of rust-bitcoin#630
Kixunil added a commit to Kixunil/rust-bitcoin that referenced this issue Feb 7, 2023
Use of general-purpose integers is often error-prone and annoying. We're
working towards improving it by introducing newtypes.

This adds newtypes for weight and fee rate to make fee computation
easier and more readable. Note however that this dosn't change the type
for individual parts of the transaction since computing the total weight
is not as simple as summing them up and we want to avoid such confusion.

Part of rust-bitcoin#630
Kixunil added a commit to Kixunil/rust-bitcoin that referenced this issue Feb 7, 2023
Use of general-purpose integers is often error-prone and annoying. We're
working towards improving it by introducing newtypes.

This adds newtypes for weight and fee rate to make fee computation
easier and more readable. Note however that this dosn't change the type
for individual parts of the transaction since computing the total weight
is not as simple as summing them up and we want to avoid such confusion.

Part of rust-bitcoin#630
Kixunil added a commit to Kixunil/rust-bitcoin that referenced this issue Feb 7, 2023
Use of general-purpose integers is often error-prone and annoying. We're
working towards improving it by introducing newtypes.

This adds newtypes for weight and fee rate to make fee computation
easier and more readable. Note however that this dosn't change the type
for individual parts of the transaction since computing the total weight
is not as simple as summing them up and we want to avoid such confusion.

Part of rust-bitcoin#630
Kixunil added a commit to Kixunil/rust-bitcoin that referenced this issue Feb 7, 2023
Use of general-purpose integers is often error-prone and annoying. We're
working towards improving it by introducing newtypes.

This adds newtypes for weight and fee rate to make fee computation
easier and more readable. Note however that this dosn't change the type
for individual parts of the transaction since computing the total weight
is not as simple as summing them up and we want to avoid such confusion.

Part of rust-bitcoin#630
Kixunil added a commit to Kixunil/rust-bitcoin that referenced this issue Feb 8, 2023
Use of general-purpose integers is often error-prone and annoying. We're
working towards improving it by introducing newtypes.

This adds newtypes for weight and fee rate to make fee computation
easier and more readable. Note however that this dosn't change the type
for individual parts of the transaction since computing the total weight
is not as simple as summing them up and we want to avoid such confusion.

Part of rust-bitcoin#630
Kixunil added a commit to Kixunil/rust-bitcoin that referenced this issue Feb 8, 2023
Use of general-purpose integers is often error-prone and annoying. We're
working towards improving it by introducing newtypes.

This adds newtypes for weight and fee rate to make fee computation
easier and more readable. Note however that this dosn't change the type
for individual parts of the transaction since computing the total weight
is not as simple as summing them up and we want to avoid such confusion.

Part of rust-bitcoin#630
apoelstra added a commit that referenced this issue Feb 10, 2023
70cf451 Add `Weight` and `FeeRate` newtypes (Martin Habovstiak)

Pull request description:

  Use of general-purpose integers is often error-prone and annoying. We're working towards improving it by introducing newtypes.

  This adds newtypes for weight and fee rate to make fee computation easier and more readable. Note however that this dosn't change the type for individual parts of the transaction since computing the total weight is not as simple as summing them up and we want to avoid such confusion.

  Part of #630
  Replaces #1607 (I want to get this in quickly and don't want to be blocked on DanGould's availability.)

ACKs for top commit:
  apoelstra:
    ACK 70cf451
  tcharding:
    ACK 70cf451

Tree-SHA512: ab9cc9f554a52ab0109ff23565b3e2cb2d3f609b557457b4afd8763e3e1b418aecbb3d22733e33304e858ecf900904a1af6e6fdc16dc21483b9ef84f56f103b2
@Kixunil
Copy link
Collaborator Author

Kixunil commented Jan 27, 2024

It seems that everything that made sense was already moved. Am I right @DanGould ?

@DanGould
Copy link
Contributor

You're reminding me I need to downstream some of the changes that rust-bitcoin has made

Some of these seem not to be included yet:

  • Extension trait for weight calculation on TxIn, TxOut etc, so that one may for instance predict how much transaction fee will change (would use inherent methods here)

plain methods were used instead

  • Function for computing the size of finalized PSBT

would be nice

  • Helper type replacing (&bitcoin::Txin, &bitcoin::util::psbt::Input) with a method for getting previous TxOut and possibly other helpers

I use re-implement this all the time and it would be great to have (even if in the TBD psbt crate)

@Kixunil
Copy link
Collaborator Author

Kixunil commented Jan 30, 2024

plain methods were used instead

I originally meant for them to be plain methods here. I see no reason a trait would be needed. Do you?

Function for computing the size of finalized PSBT

Can this be done generally? IIRC payjoin just uses its knowledge of the input kinds. We could do this for ExtendedTransaction though. And maybe payjoin should convert from PSBT to ExtendedTransaction before doing any business logic. (I want to do proper serialization/deserialization but that's more complex so I'd start simply with conversions.)

I use re-implement this all the time and it would be great to have (even if in the TBD psbt crate)

I thought iter_funding_utxos should be enough, it isn't?

@DanGould
Copy link
Contributor

DanGould commented Jan 31, 2024

  • plain methods are even better
  • I'm not sure. Conversion before size/fee estimation makes sense. I remember liking how it's done in NBitcoin on their tx builder
  • iter_funding_utxo does not return OutPoint Data, which is why we need the helper returning a tuple in payjoin. But that's really a shortcoming of psbt v0 Input map fixed by v2. wontfix is ok for the v0 interface imo.

@Kixunil
Copy link
Collaborator Author

Kixunil commented Jan 31, 2024

I'm not sure.

I took a look and I believe it can IFF appropriate fields are filled.

iter_funding_utxo does not return OutPoint Data

Is there anything wrong with psbt.iter_funding_utxo().join(&psbt.unsigned_tx.input)?

@DanGould
Copy link
Contributor

DanGould commented Feb 1, 2024

Is there anything wrong with psbt.iter_funding_utxo().join(&psbt.unsigned_tx.input)?

That's around what I've come to use downstream (before even iter_funding_utxo became available). Specifically psbt.unsigned_tx.input.iter().zip(&mut psbt.inputs)

Here's an example (where I think I could get rid of the Box and instaed return impl Iterator):

https://github.com/MutinyWallet/mutiny-node/blob/d3ead1295929360d185c40991062d81ff2149ef6/mutiny-core/src/onchain.rs#L493-L520

@Kixunil
Copy link
Collaborator Author

Kixunil commented Feb 2, 2024

Oh, we're talking about slightly different things. I had mainly TxOut in mind. But sure, having both makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants