New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ADR-016: Add versions to Block and State #2644
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #2644 +/- ##
===========================================
+ Coverage 61.29% 61.43% +0.13%
===========================================
Files 203 203
Lines 16739 16757 +18
===========================================
+ Hits 10261 10294 +33
+ Misses 5611 5591 -20
- Partials 867 872 +5
|
types/block.go
Outdated
@@ -538,6 +543,9 @@ func (sh SignedHeader) ValidateBasic(chainID string) error { | |||
if sh.Commit == nil { | |||
return errors.New("SignedHeader missing commit (precommit votes).") | |||
} | |||
|
|||
// TODO: Check Version? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this be addressed in a followup issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops this is actually checked in the state.ValidateBlock so nothing to check here, can remove this comment
- `Block (uint64)`: Protocol version of the blockchain data structures. | ||
- `App (uint64)`: Protocol version of the application. | ||
- **Usage**: | ||
- Block version should be static in the life of a blockchain. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: Does that mean a block version can only be upgraded / changed via a fork?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's the idea for now. We can probably relax it somewhat later but needs more work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I definitely think this can be relaxed #postlaunch. My intuition is that only changes to the merkle tree structure absolutely require going back to genesis files, from a block version pov. Header formats and Block Structures can be changed in a live-upgrade, but probably shouldn't be due to how it would break a ton of other things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🍰 🌮 🍉
## Version | ||
|
||
The `Version` contains the protocol version for the blockchain and the | ||
application as two `uint64` values: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to duplicate type information imho
application as two `uint64` values: | |
application: |
Implements part of ADR-016 - #2468.