Improve block version #1351

tcharding · 2022-10-26T03:21:54Z

After initial attempt and review this PR has been re-written.

Patch 1: Make types in block more terse, this is preparatory clean up based on suggestion below.
Patch 2: Make inner value of Version private to hide the i32/u32 discrepancy

This is a follow up to #1240

apoelstra · 2022-10-26T16:17:00Z

This has been proposed from time to time and nacked because Bitcoin Core uses i32, so if we use a different type then we are deviating from the protocol in some philosophical sense.

tcharding · 2022-10-26T22:10:28Z

Oh, did I misunderstand your comment: #1240 (comment)

I interpreted that comment as a concept ack for this PR?

apoelstra · 2022-10-26T22:12:41Z

No, you did interpret me right :). This idea has been nacked in the past, but not by me.

I'm tempted though to just go all the way to a proper newtype here rather than potentially debating u32 vs i32.

tcharding · 2022-10-27T02:03:18Z

I'm tempted though to just go all the way to a proper newtype here rather than potentially debating u32 vs i32.

I don't fully get what you mean by "all the way to a proper newtype" since its a struct already, I had a go at abstracting away the i32/u32 business by making the inner value private. Does that meet your criteria?

apoelstra · 2022-10-27T13:24:17Z

@tcharding yeah, I like this direction (and in this case we could change the internal repr to u32 if we wanted as long as we were convinced there were no API-visible ways to distinguish this).

I think that we need some from_consensus/to_consensus methods that would let you get an i32 out though, because I'm not convinced that the current API is sufficient to do everything that people might want to do.

tcharding · 2022-10-28T01:31:41Z

Added as a separate patch. I was liberal with docs about Bitcoin Core and the oddness of there being a signed inner value.

tcharding · 2022-10-28T01:38:23Z

Forgot to rebase to pick up CI fix.

tcharding · 2022-10-28T02:03:23Z

Its hard to get good help :)

nlanson · 2022-10-28T14:36:09Z

bitcoin/src/blockdata/block.rs

@@ -111,18 +113,40 @@ impl BlockHeader {
 #[derive(Copy, PartialEq, Eq, Clone, Debug, PartialOrd, Ord, Hash)]
 #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
 #[cfg_attr(feature = "serde", serde(crate = "actual_serde"))]
-pub struct BlockVersion(pub i32);
+pub struct Version(i32);


We're still keeping the inner value as i32?

There are a quite a few instances of casting u32 to i32 and vice versa internally so I think the switch to u32 should still be here.

@nlanson the version is conceptually a signed integer. This is how it is represented in the reference implementation of Bitcoin.

For casts, I see two casts to u32 and one to i32, all to deal with the top-bit business. I don't think this is too bad.

I would ACK a PR which changed the internal repr to u32 but I don't think it's necessary and I slightly prefer the current situation.

i32 is ridiculous, what does a negative version even mean? Time travel?

Unless Core somewhere actually uses something like -42 for version and doesn't treat it as a bag of bits I prefer u32. Thankfully newtype will shield the outside code from most of this nonsense.

nlanson · 2022-10-28T14:37:07Z

bitcoin/src/blockdata/block.rs

@@ -301,7 +325,7 @@ impl Block {
        // number (including a sign bit). Height is the height of the mined
        // block in the block chain, where the genesis block is height zero (0).

-        if self.header.version < BlockVersion(2) {
+        if self.header.version < Version(2) {


Suggested change

if self.header.version < Version(2) {

if self.header.version < Version::TWO {

Ah yes, thanks. Used as suggested.

apoelstra

ACK bef3fcb

tcharding · 2022-10-28T20:59:13Z

Rebased and used Version::TWO in block as suggested.

apoelstra

ACK 64ae953

Kixunil

I'm still not sure if I should ACK i32... I think I will but I will give myself some time to be sure.

Kixunil · 2022-11-04T19:58:35Z

bitcoin/src/blockdata/block.rs

@@ -143,21 +167,21 @@ impl BlockVersion {
    }


Potentially interesting method:

/// Returns `true` if it looks like this version is affected by overt use of ASIC BOOST. /// /// Some longer explanation of what's ASIC BOOST and what we check here. pub fn resembles_asic_boost() { // I don't remember which exact bits AB uses }

bitcoin/src/blockdata/block.rs

Kixunil · 2022-11-04T20:01:14Z

bitcoin/src/blockdata/block.rs

+    pub fn to_consensus(self) -> i32 {
+        self.0
+    }
+
    /// Check whether the version number is signalling a soft fork at the given bit.
    ///
    /// A block is signalling for a soft fork under BIP-9 if the first 3 bits are `001` and


Perhaps we should panic for non-sensical values of bit?

Currently the types in the block module have longer names than necessary, "header" and "version" identifiers contain the word "block", this is unnecessary because we can write `block::Header` instead of `BlockHeader` when context is required. This allows us to use the naked type `Header` inside the `block` module with no loss of clarity. We are stuck with `BlockHash` because the type is defined along with all the other hash types in `hash_types`, leave it as is for now but re-export it from the `block` module to assist in putting types that are used together in scope in the same place, making import statements more ergonomic.

The Bitcoin block version is a signed integer for historical reasons, but we bit twiddle it like an unsigned integer and during consensus encode/decode we cast the signed value to an unsigned value. In order to hide this confusion, make the inner value private and add a couple of constants for v1 and v2 block versions.

The `Version` type uses a signed 32 bit integer inner type but we bit twiddle as if it was a `u32`. We recently made the inner type private to hide the data type because of this oddness. Add methods `from_consensus` and `to_consensus` to facilitate any possible thing users may want to do with a consensus version value.

"Bitcoin Core" is conventionally named using capital letters. Audit and fix all mentions of "Bitcoin Core" in the codebase to use capital letters.

tcharding · 2022-11-05T19:56:00Z

Changes in force push:

rebase to pick up clippy fixes
Used correct capitialisation of "Bitcoin Core" (amended patch)
Added an additional patch that fixes capitialisaton of Bitcoin Core codebase wide

Please note, two comments from @Kixunil have not been addressed, waiting on input from others. Also waiting on further input from you Kix re the i32 thing, no rush.

thanks

Kixunil

ACK 248f9a3

These changes contain un-publishing the internal representation which is a good step towards removing signed insanity, so it makes no sense to block this.

I'm kinda tempted into arguing that conversion methods should be something like to_core_like_consensus to signify that they are provided for compatibility with core but it may be that the negative version does have some interesting semantics and I didn't have the time to dig into it yet.

apoelstra

ACK 248f9a3

248f9a3 Use capital letters for Bitcoin Core (Tobin C. Harding) 832169e Add to/from_consensus methods to Version type (Tobin C. Harding) 24984f0 Make block::Version inner value private (Tobin C. Harding) 7e146ed Make types in block module more terse (Tobin C. Harding) Pull request description: After initial attempt and review this PR has been re-written. - Patch 1: Make types in `block` more terse, this is preparatory clean up based on suggestion below. - Patch 2: Make inner value of `Version` private to hide the i32/u32 discrepancy This is a follow up to rust-bitcoin#1240 ACKs for top commit: Kixunil: ACK 248f9a3 apoelstra: ACK 248f9a3 Tree-SHA512: ee031035288a2bcc246a9837a6028c254c51daf78a5cc2441b467ab7f183f1700a63911a2e78b84a20674ce0a83851a7c3bb7e46644a56fdd255685b9a0bf7f2

tcharding changed the title ~~Use u32 for block version instead of i32~~ Use u32 for block version instead of i32 Oct 26, 2022

nlanson previously approved these changes Oct 26, 2022

View reviewed changes

tcharding dismissed nlanson’s stale review via dde911d October 27, 2022 02:00

tcharding force-pushed the 10-26-block-version-use-u32 branch from 5af42eb to dde911d Compare October 27, 2022 02:00

tcharding changed the title ~~Use u32 for block version instead of i32~~ Improve block version Oct 27, 2022

tcharding force-pushed the 10-26-block-version-use-u32 branch from 73d1376 to 2fac25f Compare October 28, 2022 01:38

tcharding force-pushed the 10-26-block-version-use-u32 branch from 2fac25f to bef3fcb Compare October 28, 2022 02:03

nlanson reviewed Oct 28, 2022

View reviewed changes

apoelstra previously approved these changes Oct 28, 2022

View reviewed changes

tcharding dismissed apoelstra’s stale review via 64ae953 October 28, 2022 20:58

tcharding force-pushed the 10-26-block-version-use-u32 branch from bef3fcb to 64ae953 Compare October 28, 2022 20:58

apoelstra previously approved these changes Nov 4, 2022

View reviewed changes

Kixunil reviewed Nov 4, 2022

View reviewed changes

tcharding added 4 commits November 6, 2022 06:54

Use capital letters for Bitcoin Core

248f9a3

"Bitcoin Core" is conventionally named using capital letters. Audit and fix all mentions of "Bitcoin Core" in the codebase to use capital letters.

tcharding dismissed apoelstra’s stale review via 248f9a3 November 5, 2022 19:54

tcharding force-pushed the 10-26-block-version-use-u32 branch from 64ae953 to 248f9a3 Compare November 5, 2022 19:54

Kixunil approved these changes Nov 5, 2022

View reviewed changes

apoelstra approved these changes Nov 6, 2022

View reviewed changes

apoelstra merged commit 4bce69d into rust-bitcoin:master Nov 6, 2022

tcharding deleted the 10-26-block-version-use-u32 branch November 7, 2022 20:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve block version #1351

Improve block version #1351

tcharding commented Oct 26, 2022 •

edited

apoelstra commented Oct 26, 2022

tcharding commented Oct 26, 2022

apoelstra commented Oct 26, 2022

tcharding commented Oct 27, 2022

apoelstra commented Oct 27, 2022

tcharding commented Oct 28, 2022 •

edited

tcharding commented Oct 28, 2022

tcharding commented Oct 28, 2022

nlanson Oct 28, 2022 •

edited

apoelstra Oct 28, 2022

apoelstra Oct 28, 2022

Kixunil Oct 28, 2022

nlanson Oct 28, 2022

tcharding Oct 28, 2022

apoelstra left a comment

tcharding commented Oct 28, 2022

apoelstra left a comment

Kixunil left a comment

Kixunil Nov 4, 2022

Kixunil Nov 4, 2022

tcharding commented Nov 5, 2022 •

edited

Kixunil left a comment

apoelstra left a comment

	if self.header.version < Version(2) {
	if self.header.version < Version::TWO {

Improve block version #1351

Improve block version #1351

Conversation

tcharding commented Oct 26, 2022 • edited

apoelstra commented Oct 26, 2022

tcharding commented Oct 26, 2022

apoelstra commented Oct 26, 2022

tcharding commented Oct 27, 2022

apoelstra commented Oct 27, 2022

tcharding commented Oct 28, 2022 • edited

tcharding commented Oct 28, 2022

tcharding commented Oct 28, 2022

nlanson Oct 28, 2022 • edited

Choose a reason for hiding this comment

apoelstra Oct 28, 2022

Choose a reason for hiding this comment

apoelstra Oct 28, 2022

Choose a reason for hiding this comment

Kixunil Oct 28, 2022

Choose a reason for hiding this comment

nlanson Oct 28, 2022

Choose a reason for hiding this comment

tcharding Oct 28, 2022

Choose a reason for hiding this comment

apoelstra left a comment

Choose a reason for hiding this comment

tcharding commented Oct 28, 2022

apoelstra left a comment

Choose a reason for hiding this comment

Kixunil left a comment

Choose a reason for hiding this comment

Kixunil Nov 4, 2022

Choose a reason for hiding this comment

Kixunil Nov 4, 2022

Choose a reason for hiding this comment

tcharding commented Nov 5, 2022 • edited

Kixunil left a comment

Choose a reason for hiding this comment

apoelstra left a comment

Choose a reason for hiding this comment

tcharding commented Oct 26, 2022 •

edited

tcharding commented Oct 28, 2022 •

edited

nlanson Oct 28, 2022 •

edited

tcharding commented Nov 5, 2022 •

edited