Add transaction::Version data type #2006

tcharding · 2023-08-18T01:21:32Z

BIP-68 activated a fair while ago (circa 2019) and since then only transaction versions 1 and 2 have been considered standard.

Currently in our Transaction struct we use an i32, this means users can construct a non-standard transaction if they do not first look up what the value should be. We can help folk out here by abstracting over the version number.

Add transaction::Version data type that simply provides two consts ONE and TWO.

Add a Default impl on Version that returns Version::TWO.

In tests that used version 0, instead use Version::default because the test obviously does not care.

apoelstra · 2023-08-18T14:54:59Z

5ae7806 looks good except that we should add a constructor for creating arbitrary version numbers. We can comment that there is no reason (on mainnet at least) to use this, but it should be possible.

Alternately we could make the inner i32 public.

sanket1729 · 2023-08-18T20:05:40Z

bitcoin/src/blockdata/transaction.rs

+
+impl Decodable for Version {
+    fn consensus_decode<R: io::Read + ?Sized>(r: &mut R) -> Result<Self, encode::Error> {
+        Decodable::consensus_decode(r).map(Version)


Should this not check that these are the only values we support. I think we should allow for other values too, perhaps under another constructor. People are prototyping tx ver 3 in signet for mempool relay stuff.

Just in case you missed it, current version of this PR includes transaction::Version::non_standard constructor as requested.

tcharding · 2023-08-18T22:14:40Z

Good reviewing lads, thank you. Changes in force push

Make the inner i32 public and document that we do not enforce any invariants on the version number.
Add non_standard constructor
Add is_standard (eg, in case folk want to check after decoding)

apoelstra

ACK 6cf6f62

apoelstra

ACK 95297f4

BIP-68 activated a fair while ago (circa 2019) and since then only transaction versions 1 and 2 have been considered standard. Currently in our `Transaction` struct we use an `i32`, this means users can construct a non-standard transaction if they do not first look up what the value should be. We can help folk out here by abstracting over the version number. Since the version number only governs standardness elect to make the inner `i32` public (ie., not an invariant). The aim of the type is to make life easy not restrict what versions are used. Add transaction::Version data type that simply provides two consts `ONE` and `TWO`. Add a `Default` impl on `Version` that returns `Version::TWO`. In tests that used version 0, instead use `Version::default` because the test obviously does not care.

apoelstra

ACK c950ef4

bitcoin/src/blockdata/transaction.rs

clarkmoody

ACK c950ef4

apoelstra · 2023-09-23T18:44:35Z

Thanks for merging @clarkmoody!

In future though could you use the Bitcoin Core merge script (it is not actually Core-specific at all). It creates a more uniform merge commit message, gpg-signs the merge commits, does a few sanity checks, and looks for ACKs in the PR comments.

sanket1729 · 2023-09-24T00:45:55Z

bitcoin/src/blockdata/transaction.rs

+    pub fn is_standard(&self) -> bool { *self == Version::ONE || *self == Version::TWO }
+}
+
+impl Default for Version {


post merge comment:

Is this a reasonable default. We are expecting transaction version 3 sometime in the future. If we silently change this to three, it will break a lot of code downstream.

I don't think this should implement Default as there is no sane default that we can represent. If we want a default I think it should be zero.

@tcharding, @apoelstra, I feel this needs to addressed/discussed.

If version 3 transactions ship and the default version changes, then we would simply release a new breaking version of the library with release notes.

I think a sane default would be what Core uses. Version zero would be nonstandard, right?

Some interesting points, I could easily have changed a default value and not thought it a breaking change. I don't see a lot of negative in removing the Default so if there is the slightest concern I think we should do that.

I didn't realize that version 3 was coming down the pike. I agree, we should drop the default (though I'd like to put it back as 3 whenever 3 comes out, because that default will last for several years at least).

@clarkmoody it is a breaking change so in principle we're justified in messing with the value in any breaking release, but I think changing this value would be extremely hard to notice -- if we did this to Liquid I'd bet we'd go literally years before we noticed that we were producing transactions with a different version number, and then we'd need some forensics to see when it happened (and we'd only even notice because we happen to know that Liquid long predates tx v3 so it can't have always been 3..). Because there's no "visible" effect other than a potential change in privacy set.

In the case of Liquid it wouldn't matter at all, but if we were a user who cared for some reason, we'd be upset.

clarkmoody · 2023-09-24T02:35:16Z

Thanks for merging @clarkmoody!

In future though could you use the Bitcoin Core merge script (it is not actually Core-specific at all). It creates a more uniform merge commit message, gpg-signs the merge commits, does a few sanity checks, and looks for ACKs in the PR comments.

Oops! Will use that script for future merges.

tcharding · 2023-09-24T08:47:35Z

Clearly a sacking offence, @clarkmoody please step down and return all salary payments already received ;)

f31fb08 Remove default impl from transaction version (Tobin C. Harding) Pull request description: There is no logical default for the transaction version number, there is only pre-bip68 (v1) and post-bip68 (v2). Uses should specify the version they want not rely on us making the choice. (I originally added this impl to support testing, this was in hindsight the wrong thing to do, props to Sanket for noticing.) Discussed in comments here: #2006 (comment) ACKs for top commit: clarkmoody: ACK f31fb08 apoelstra: ACK f31fb08 Tree-SHA512: 1112c35173d6b2e7d6c50f058b1964fc8b633615ae703b0c1231e701a3e322fd55d3a8377c7ea078f28f523b15b7de6e9eca844226fcd19528206161d75dcfdf

Reason: rust-bitcoin/rust-bitcoin#2006

2a891e0 Add a missing link to #2006 in 'bitcoin/CHANGELOG.md' (lateminer) Pull request description: In the changelog, there is a wrong link to #2006. Additionally, a link and description for #2020 are missing. This PR addresses these minor issues. ACKs for top commit: apoelstra: ACK 2a891e0 tcharding: ACK 2a891e0 Tree-SHA512: c6dc1362af5bb2e5f11c2ed48371a0236ab512aeb0f6075674994c054baf606d9cf30390d7021f9c02c3b7cc59a70c60edb64799afb35cc5ac716a3582aa8cd0

tcharding force-pushed the 08-18-tx-version branch from afbbcfe to 5ae7806 Compare August 18, 2023 03:31

This was referenced Aug 18, 2023

WIP: Add Sighash signing API #2007

Closed

WIP: Add sighash signing API #2008

Draft

sanket1729 reviewed Aug 18, 2023

View reviewed changes

tcharding force-pushed the 08-18-tx-version branch from 5ae7806 to 6cf6f62 Compare August 18, 2023 22:13

apoelstra approved these changes Aug 20, 2023

View reviewed changes

tcharding force-pushed the 08-18-tx-version branch from 6cf6f62 to 95297f4 Compare August 31, 2023 03:20

apoelstra previously approved these changes Sep 3, 2023

View reviewed changes

tcharding dismissed apoelstra’s stale review via c950ef4 September 21, 2023 05:02

tcharding force-pushed the 08-18-tx-version branch from 95297f4 to c950ef4 Compare September 21, 2023 05:02

apoelstra approved these changes Sep 21, 2023

View reviewed changes

clarkmoody reviewed Sep 22, 2023

View reviewed changes

bitcoin/src/blockdata/transaction.rs Show resolved Hide resolved

clarkmoody approved these changes Sep 23, 2023

View reviewed changes

clarkmoody merged commit 72a7280 into rust-bitcoin:master Sep 23, 2023
29 checks passed

sanket1729 reviewed Sep 24, 2023

View reviewed changes

tcharding deleted the 08-18-tx-version branch September 24, 2023 19:48

tcharding mentioned this pull request Sep 24, 2023

Remove default impl from transaction version #2091

Merged

lateminer added a commit to CoinBlack/rust-blackcoinmore-rpc that referenced this pull request Oct 26, 2023

Use transaction::Version data type

838c71c

Reason: rust-bitcoin/rust-bitcoin#2006

lateminer added a commit to lateminer/rust-bitcoin that referenced this pull request Oct 26, 2023

Add a missing link to rust-bitcoin#2006 in 'bitcoin/CHANGELOG.md'

2a891e0

lateminer mentioned this pull request Oct 26, 2023

Add a missing link to #2006 in 'bitcoin/CHANGELOG.md' #2143

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add transaction::Version data type #2006

Add transaction::Version data type #2006

tcharding commented Aug 18, 2023

apoelstra commented Aug 18, 2023

sanket1729 Aug 18, 2023

tcharding Sep 3, 2023

tcharding commented Aug 18, 2023

apoelstra left a comment

apoelstra left a comment

apoelstra left a comment

clarkmoody left a comment

apoelstra commented Sep 23, 2023

sanket1729 Sep 24, 2023 •

edited

sanket1729 Sep 24, 2023

clarkmoody Sep 24, 2023

tcharding Sep 24, 2023

apoelstra Sep 24, 2023

clarkmoody commented Sep 24, 2023

tcharding commented Sep 24, 2023

Add transaction::Version data type #2006

Add transaction::Version data type #2006

Conversation

tcharding commented Aug 18, 2023

apoelstra commented Aug 18, 2023

sanket1729 Aug 18, 2023

Choose a reason for hiding this comment

tcharding Sep 3, 2023

Choose a reason for hiding this comment

tcharding commented Aug 18, 2023

apoelstra left a comment

Choose a reason for hiding this comment

apoelstra left a comment

Choose a reason for hiding this comment

apoelstra left a comment

Choose a reason for hiding this comment

clarkmoody left a comment

Choose a reason for hiding this comment

apoelstra commented Sep 23, 2023

sanket1729 Sep 24, 2023 • edited

Choose a reason for hiding this comment

sanket1729 Sep 24, 2023

Choose a reason for hiding this comment

clarkmoody Sep 24, 2023

Choose a reason for hiding this comment

tcharding Sep 24, 2023

Choose a reason for hiding this comment

apoelstra Sep 24, 2023

Choose a reason for hiding this comment

clarkmoody commented Sep 24, 2023

tcharding commented Sep 24, 2023

sanket1729 Sep 24, 2023 •

edited