Enum[erate] PSBT version #1218

DanGould · 2022-08-29T13:55:45Z

Reinstates #769

This converts PSBT version into an enum. This rejects PSBTs which are unknown, i.e. not v0 or v2 as per BIP 174 spec.

This is a first of many PRs to discuss PSBTv2 design decisions one-by-one

apoelstra

ACK f98bfd3bb74b8f090cf3b093e40f9abd19c5e49c

tcharding

Nice one man, a few tips and suggestions.

src/util/psbt/mod.rs

src/util/psbt/error.rs

tcharding · 2022-08-30T01:50:17Z

src/util/psbt/mod.rs

For other reviewers: this is too many derives but they have to be here because of psbt::error::Error.

src/util/psbt/mod.rs

src/util/psbt/map/global.rs

src/util/psbt/error.rs

src/util/psbt/mod.rs

Kixunil

I think we should at least rename standard. The other issues would be also good to address but not required. The rest looks good.

bitcoin/src/psbt/mod.rs

Kixunil · 2022-11-29T21:12:50Z

bitcoin/src/psbt/mod.rs

This should say "invalid version". (The whole thing looks weird but I'm not in the mood to figure out a better way.)

Kixunil · 2022-11-29T21:20:05Z

bitcoin/src/psbt/error.rs

I realized this will produce silly-looking messages like unrecognized PSBT version: Unknown future PSBT version 42 but I don't see a clear way how to make them better. A not-very-clear way is to use fmt::Display::fmt(e, f) (with a comment that this is intentionally flattened) and then impl source like this:

Error::FutureVersion(ref e) => e.source(),

Effectively flattening the variant. Any ideas?

I think Error::FutureVersion(ref e) => e.fmt(f) will produce Unknown future PSBT version 42. How's that? Error handling the right way is a big rust blind spot for me to improve.

Exactly. We just pretend e is not a sub-error. But has to be combined with e.source(). I worry though that it'll be confusing since the enum is public.

bitcoin/src/psbt/mod.rs

Kixunil

Unpublishing version looks wrong. The rest looks OK, jut some nits.

bitcoin/src/psbt/map/global.rs

bitcoin/src/psbt/mod.rs

Kixunil · 2022-12-01T09:04:08Z

bitcoin/src/psbt/mod.rs

I'm not sure if we should keep Ord and PartialOrd. We can probably keep the others.

bitcoin/src/psbt/mod.rs

tcharding

I don't know exactly what stage PSBT version 2 is at but wanted to flag that there is a code comment

// We only understand version 0 PSBTs. According to BIP-174 we
// should throw an error if we see anything other than version 0.
if version != Some(0) {
    return Err(encode::Error::ParseFailed("PSBT versions greater than 0 are not supported"))
}

But with this PR we accept v0 or v2

version: match version {
                        Some(v) => Version::from_raw(v).map_err(Error::from)?,
                        None => Version::PsbtV0,
                    },

bitcoin/src/psbt/map/global.rs

Kixunil · 2022-12-01T21:17:34Z

@tcharding nice catch!

It appears to me that we should not have PsbtV2 until we trully support version 2. And I suspect there will be forward-compatibility issues among other things. Super-large and difficult project for sure.

DanGould · 2022-12-04T04:10:36Z

Agree it makes sense to provide Version::V2 only when the interface is available. This was a good place for me to start since Orlovsky did most of the heavy lifting on the original PR. Separating the PRs makes it easier to address decisions about individual pieces before it comes together.

I don't know exactly what stage PSBT version 2 is at

I believe the hardest part will be the refactoring around errors #837 and the move away consensus::{Encodable, Decodable}. After that, the v2 data structure is mostly new map fields and a function to serialize v2 to v0

tcharding · 2023-02-03T03:36:41Z

I can't remember what state this one was up to @DanGould, now that the error stuff is ready to merge did you want to get this into the next release?

DanGould · 2023-02-03T05:14:37Z

I think this change only makes sense in the context of a usable psbt2. If there is time to review psbt2 changes before the next (30.0) release I would love to draft it and get it over the line now. If not, we can get it into the following pre-release.

Kixunil · 2023-02-03T08:02:06Z

PSBT is a huge change, I don't think it can make it in.

tcharding

Thanks @DanGould, an open question about the Ord stuff but also I'm concerned that if this merges the code reads like we support V2 already, I'd be happier if everywhere we use the version that an occurence of v2 gives an explicit "not yet implemented" or "wip, not fully implemented" error.

tcharding · 2023-04-09T02:10:59Z

bitcoin/src/psbt/mod.rs

+        match version {
+            v if v == Version::PsbtV0 as u32 => Ok(Version::PsbtV0),
+            v if v == Version::PsbtV2 as u32 => Ok(Version::PsbtV2),
+            future => Err(FutureVersionError(future))
+        }


I don't have deep PSBT knowledge but this reads as if someone attempting to parse 0 for version will get a future version error but v1 will never exist, right?

0 will get PsbtV0
1 will get Future version which I agree is weird. Maybe just VersionError is more appropriate

Never say never ;).

But I agree, I think we should have a specific error variant for v1 actually which tells the user that they probably meant v0.

tcharding · 2023-04-09T02:14:40Z

bitcoin/src/psbt/map/global.rs


        // Serializing version only for non-default value; otherwise test vectors fail
-        if self.version > 0 {
+        if self.version > Version::PsbtV0 {


I'm not sure exactly why but my gut tells me we should match on the variants explicitly and remove PartialOrd/Ord. For the "keep the highest version" (in combine) we could use to_raw then compare the values.

DanGould · 2023-04-09T02:23:20Z

I'm thinking merging PSBT2 in pieces this small makes less sense than getting a viable implementation done first, even if that's a larger review.

tcharding · 2023-04-09T23:04:14Z

I'm fine to review a massive "psbt v2" PR.

tcharding · 2023-08-25T00:11:11Z

Hi @DanGould whats your workload like at the moment, have you time to get to this? Seems it is needed for #1924, would be good to get it in, I can pick it up and shepherd it through if you like?

DanGould · 2023-08-25T00:49:29Z

I figured it would be added as part of #1924 and then this pr would be closed. Am I mistaken? What kind of attention does this require to facilitate psbt2

tcharding · 2023-08-25T04:15:05Z

AFAICT just the error thing fixing up.

stevenroose · 2023-12-29T23:21:01Z

I guess this PR is deprecated in favor of #1924?

DanGould · 2023-12-30T02:36:10Z

@stevenroose the most active development for psbtv2 is happening in a new psbt crate

DanGould mentioned this pull request Aug 29, 2022

Tracking PSBT refactoring & PSBTv2 Epic #1115

Closed

17 tasks

apoelstra previously approved these changes Aug 29, 2022

View reviewed changes

tcharding changed the title ~~Enum[erate] PSBT vesion~~ Enum[erate] PSBT version Aug 30, 2022

tcharding reviewed Aug 30, 2022

View reviewed changes

Kixunil requested changes Aug 30, 2022

View reviewed changes

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

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

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

DanGould dismissed apoelstra’s stale review via dc5d8b7 November 29, 2022 20:59

DanGould force-pushed the psbt-ver branch from f98bfd3 to dc5d8b7 Compare November 29, 2022 20:59

Kixunil reviewed Nov 29, 2022

View reviewed changes

DanGould force-pushed the psbt-ver branch 2 times, most recently from b8734cb to 2f2c944 Compare December 1, 2022 02:41

Kixunil reviewed Dec 1, 2022

View reviewed changes

tcharding reviewed Dec 1, 2022

View reviewed changes

bitcoin/src/psbt/map/global.rs Outdated Show resolved Hide resolved

Add PSBT Version Enum

14b0737

DanGould force-pushed the psbt-ver branch from 2f2c944 to 14b0737 Compare April 7, 2023 15:33

tcharding reviewed Apr 9, 2023

View reviewed changes

Subhra264 mentioned this pull request Jun 23, 2023

Implement support for PsbtV2 #1924

Open

Enum[erate] PSBT version #1218

Are you sure you want to change the base?

Enum[erate] PSBT version #1218

Uh oh!

Conversation

DanGould commented Aug 29, 2022

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

tcharding left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Kixunil left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanGould Nov 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Kixunil left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tcharding left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Kixunil commented Dec 1, 2022

Uh oh!

DanGould commented Dec 4, 2022

Uh oh!

tcharding commented Feb 3, 2023

Uh oh!

DanGould commented Feb 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kixunil commented Feb 3, 2023

Uh oh!

tcharding 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!

DanGould commented Apr 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcharding commented Apr 9, 2023

Uh oh!

DanGould Nov 29, 2022 •

edited

Loading

DanGould commented Feb 3, 2023 •

edited

Loading

DanGould commented Apr 9, 2023 •

edited

Loading

tcharding commented Aug 25, 2023 •

edited

Loading

DanGould commented Dec 30, 2023 •

edited

Loading