Header version cleanup #2809
Header version cleanup #2809
Conversation
| #[derive(Clone, Copy, Debug, Eq, PartialEq, Serialize)] | ||
| pub struct HeaderVersion(pub u16); | ||
|
|
||
| impl Default for HeaderVersion { |
There was a problem hiding this comment.
I'm not sure it makes sense to implement Default for HeaderVersion. In 2 months, all new blocks will have to be version 2, and having a default at that point seems like a good way to introduce logical errors.
There was a problem hiding this comment.
Default post-HF should be version 2 at that point. At least that's the intent here.
We may want to make it more explicit though and not use "default".
There was a problem hiding this comment.
Let's assume for a second that it will be version 2 post hard fork. How do you manage that? Do we want default to look up the current height to know if we're >= block 250000? I would assume no. My vote is to remove default.
There was a problem hiding this comment.
Was slightly agreeing with @DavidBurkett but then realized versions are rarely built by themselves, they're built with their headers. And then we have a bunch of code (mostly tests) that just want a header without caring much.
There was a problem hiding this comment.
The way I was thinking about this was in terms of two ways we need to create headers -
- new/current headers - we want the default to be version 2 post hark fork
- for tests (as @ignopeverell points out) it is reasonable to default to version 2 here
- historical headers (say during fast sync) - we want to create non-default headers taking height into consideration
- any tests touching pre and post hard fork behavior will want to use non-default versions
But to be honest I have not thought too deeply about this -
- we only have a single version today
- its not unreasonable to have a default now, until we decide otherwise
- its easy to rework this as necessary in the future
| @@ -239,9 +239,19 @@ impl Default for ProofOfWork { | |||
| } | |||
| } | |||
|
|
|||
| impl ProofOfWork { | |||
| /// Read implementation, can't define as trait impl as we need a version | |||
| pub fn read(_ver: u16, reader: &mut dyn Reader) -> Result<ProofOfWork, ser::Error> { | |||
There was a problem hiding this comment.
I don't think you can do this. You're going to be adding that version back for our scheduled hard fork, aren't you?
There was a problem hiding this comment.
I don't think the check is going to happen here during deserialization.
We have consensus::valid_header_version() and this is happening orthogonal to the ser/deser process (called via pipe::validate_header()).
I think we originally had this passed in here when we first introduced the AR/AT dual PoW impl or when we opened up the C31+ flexibility.
That said - this change was one of the things I wanted to discuss via this PR.
There was a problem hiding this comment.
I don't know much about the format of our PoW, but I assumed the hardfork would change how we have to serialize/deserialize ProofOfWork. If that's not true, then I agree with your change.
There was a problem hiding this comment.
I'd leaning toward simplifying and standardizing how we read (to use our existing traits) for now. If and when we discover this needs to be adjusted we can revisit this decision.
There was a problem hiding this comment.
Ideally the PoW change wouldn't change its format, but just details about graph construction or cycle finding. Not sure we'll be able to keep it that way every time but still a good goal to start with.
|
Note: I added a header version validation step to the header deserialization stage. We definitely want to revisit this as we get closer to the hardfork - I'm guessing (but to be discussed) that a pre-HF peer will want to be banned to keep the network healthy. |
| @@ -239,9 +239,19 @@ impl Default for ProofOfWork { | |||
| } | |||
| } | |||
|
|
|||
| impl ProofOfWork { | |||
| /// Read implementation, can't define as trait impl as we need a version | |||
| pub fn read(_ver: u16, reader: &mut dyn Reader) -> Result<ProofOfWork, ser::Error> { | |||
There was a problem hiding this comment.
Ideally the PoW change wouldn't change its format, but just details about graph construction or cycle finding. Not sure we'll be able to keep it that way every time but still a good goal to start with.
| #[derive(Clone, Copy, Debug, Eq, PartialEq, Serialize)] | ||
| pub struct HeaderVersion(pub u16); | ||
|
|
||
| impl Default for HeaderVersion { |
There was a problem hiding this comment.
Was slightly agreeing with @DavidBurkett but then realized versions are rarely built by themselves, they're built with their headers. And then we have a bunch of code (mostly tests) that just want a header without caring much.
cleanup pow ser/deser (version unused)
antiochp
force-pushed
the
header_version_cleanup
branch
from
349831a to
540681d
Compare
antiochp
added
the
release notes
HeaderVersion(u16)for type safety around header versioningHeader version is converted to the underlying u16 during json serialization for the api.
Header version is serialized/deserialized as the inner u16 vie read/write.
Default header version is
1(currently the only version in use).pow ser/deser was passing an unused version around - cleaned this up so we can now implement
Readable/Writeabletraits onProofOfWorkstruct.This is (should be) purely a refactor - no change to behavior or functionality.
I went down this rabbit hole because I was investigating "protocol version" specific serialization/deserialization and discovered we are already using "versioning" for block headers (even if not for the same reason).