Improve block version #1351
Improve block version #1351
Conversation
u32 for block version instead of i32
|
This has been proposed from time to time and nacked because Bitcoin Core uses |
|
Oh, did I misunderstand your comment: #1240 (comment) I interpreted that comment as a concept ack for this PR? |
|
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. |
5af42eb to
dde911d
Compare
u32 for block version instead of 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? |
|
@tcharding yeah, I like this direction (and in this case we could change the internal repr to I think that we need some |
|
Added as a separate patch. I was liberal with docs about Bitcoin Core and the oddness of there being a signed inner value. |
73d1376 to
2fac25f
Compare
|
Forgot to rebase to pick up CI fix. |
2fac25f to
bef3fcb
Compare
|
Its hard to get good help :) |
| @@ -111,18 +113,40 @@ impl BlockHeader { | |||
| #[derive(Copy, PartialEq, Eq, Clone, Debug, PartialOrd, Ord, Hash)] | |||
| #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] | 10000|||
| #[cfg_attr(feature = "serde", serde(crate = "actual_serde"))] | |||
| pub struct BlockVersion(pub i32); | |||
| pub struct Version(i32); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@nlanson the version is conceptually a signed integer. This is how it is represented in the reference implementation of Bitcoin.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
bitcoin/src/blockdata/block.rs
Outdated
| @@ -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) { | |||
There was a problem hiding this comment.
| if self.header.version < Version(2) { | |
| if self.header.version < Version::TWO { |
There was a problem hiding this comment.
Ah yes, thanks. Used as suggested.
bef3fcb to
64ae953
Compare
|
Rebased and used |
| @@ -143,21 +167,21 @@ impl BlockVersion { | |||
| } | |||
There was a problem hiding this comment.
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
}| 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 |
There was a problem hiding this comment.
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.
64ae953 to
248f9a3
Compare
|
Changes in force push:
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 thanks |
There was a problem hiding this comment.
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.
After initial attempt and review this PR has been re-written.
blockmore terse, this is preparatory clean up based on suggestion below.Versionprivate to hide the i32/u32 discrepancyThis is a follow up to #1240