New crate psbt-v2 released

[tcharding]

I just released a new crate I've been working on psbt-v2. It has two things:

• PSBT v0: Code and API pulled out of rust-bitcoin and rust-miniscript (with minimal cough f**king Tobin, refactoring). I re-imported the code, there is now no refactoring.
• PSBT v2: Totally new API design, I hacked the v0 code to pieces and implemented BIP-370

I'd love to get some feedback on the v2::Psbt API design please if anyone has spare clock cycles to take a look.

I would also like to move the repo to rust-bitcoin if/when folk are ok with that.

EDIT: version 0.1.1 is out.

Thanks

[Kixunil]

Definitely not enough energy to give proper feedback right now but I at least took a quick look. I really like the transaction go t split up, though maybe too much? I think using OutPoint would be nicer than having two fields.

In Input:

If omitted, assumed to be the final sequence number (Sequence::MAX).

What? Is this a BIP thing?

min_time/min_height - these should be mutually exclusive, you can't enforce both.

[tcharding]

Definitely not enough energy to give proper feedback right now but I at least took a quick look. I really like the transaction go t split up, though maybe too much? I think using OutPoint would be nicer than having two fields.

That's a bip-370 thing - I agree the two fields is odd and the spent_output_index instead of vout.

In Input:

If omitted, assumed to be the final sequence number (Sequence::MAX).

What? Is this a BIP thing?

Yep.

min_time/min_height - these should be mutually exclusive, you can't enforce both.

This is a bip thing too, the lock is determined by v2::Psbt::determine_lock_time (and that algo is specified in bip-370)

Thanks for looking!

[Kixunil]

That's a bip-370 thing

BIP370 doesn't say how our API should look. It's only concerned about encoding.

Yep.

That's annoying but since I'm going to rewrite AsTransaction anyway it's not a big deal I think.

This is a bip thing too

I'm starting to dislike how wobbly the BIP is. At this point, they could've used Json and put everything in strings...

I'd personally prefer for obvious inconsistencies to be straight up deserialization errors. It's much easier to work with, especially in statically typed languages. And while I kind of understand having these inconsistent across inputs it's hard to understand why it would ever be OK within one input.

[apoelstra]

@Kixunil it's not an "obvious inconsistency" because on a given input you can specify both a minimum height and a minimum time, and this is interpreted as either minimum being acceptable. (Since the input is presumably owned by one entity we are not concerned that two parties would update the input and have conflicting requirements.)

[Kixunil]

Ah, OK, that does make sense.

Oh, wait, but the crate doesn't implement it that way. It requires that only one is used. @tcharding you seem to have a bug there.

[tcharding]

I'll give you the benefit of the doubt but can you point me to the line of code that makes you think this please?

[Kixunil]

Oh, I misread the code, didn't notice requires_time_based_lock_time != min_time

[tcharding]

FTR I made the Psbt, Input, Output, and Global structs match the bip (field names and optional etc.) same as we did in rust-bitcoin. I agree that we could use the Rust type system better and not do so but I think that looses some clarity between the bip and the code - which is already pretty hard to see we get right.

[Kixunil]

Honestly I more and more think this would benefit from serde-like appraoch - that you define a struct with only the required/optional fields and get deserialization derived. E.g. if your application absolutely has to know previous output of an input you verify it and reject during derserialization.

[tcharding]

If I understand you correctly the current code functionally does the same thing but fails in the constructor of each of the role types instead of having, as you suggest, a separate deser for the type that fails. I'd have to see both approaches in code really to see if one is better than the other.

[Kixunil]

Also for applications that use PSBTs in custom ways (IOW, mostly as a serialization format; e.g. payjoin) it's better to operate on pre-checked PSBTs.