Remove protocol.Params.ProtocolVersion field
#6955
Conversation
jordanschalm
requested review from
zhangchiqing,
AlexHentschel and
peterargue
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature/efm-recovery #6955 +/- ##
========================================================
- Coverage 41.14% 40.74% -0.41%
========================================================
Files 2122 1977 -145
Lines 187169 173120 -14049
========================================================
- Hits 77011 70534 -6477
+ Misses 103724 96789 -6935
+ Partials 6434 5797 -637
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| // TODO: should we replicate protocol state version here, or just remove this field? | ||
| ProtocolVersion uint64 |
There was a problem hiding this comment.
Should we replicate protocol state version here, or omit this field?
There was a problem hiding this comment.
At the moment, we are using the protocol state synonymously with the version of the protocol, so it might be useful to report it here.
There was a problem hiding this comment.
I think we should keep it and have it use the protocol state version. We should also include all other important versions here as well (e.g. execution engine version)
There was a problem hiding this comment.
I have no concerns with this PR, but I am also not really knowledgeable about the broader implications of removing this Version.
Just in case there are concerns with removing it, if we wanted to retain the old counter, I think a more refined description and documentation would be good: essentially we were tracking the spork counter, so we should name it accordingly.
| flagRootParent string | ||
| flagRootHeight uint64 | ||
| flagRootTimestamp string | ||
| // Deprecated: Replaced by ProtocolState KVStore version |
There was a problem hiding this comment.
following up on my suggestion for the previous PR #6953:
| // Deprecated: Replaced by ProtocolState KVStore version | |
| // Deprecated: Replaced by ProtocolStateVersion |
| // a spork has exactly one protocol version for its duration, so this is | ||
| // inserted exactly once, when bootstrapping the state. | ||
| func InsertProtocolVersion(version uint) func(*badger.Txn) error { | ||
| return insert(makePrefix(codeProtocolVersion), version) |
There was a problem hiding this comment.
I feel we should also Deprecate the corresponding storage code:
| sc.nodeInfo.WithLabelValues(versionLabel, version) | ||
| sc.nodeInfo.WithLabelValues(commitLabel, commit) | ||
| sc.nodeInfo.WithLabelValues(sporkIDLabel, sporkID) | ||
| sc.nodeInfo.WithLabelValues(protocolVersionLabel, strconv.FormatUint(uint64(protocolVersion), 10)) |
There was a problem hiding this comment.
@vishalchangrani do we use this for partner tracking, or just the version?
There was a problem hiding this comment.
actually, nm. this isn't related to the ping service
This PR removes the
ProtocolVersionfield from theprotocol.Paramsstructure.This field is set to the major software version during a spork; it was intended to be a spork identifier that corresponded to the software version used for that spork. (For example, for version
v0.38.x, theProtocolVersionwould be set to 38.)As part of the
Paramsstructure, it does not change during the course of a spork. However, we commonly increment the major software within sporks now, so theProtocolVersionmay not actually reflect the major software version currently being run. Also, the naming collides with the new Protocol KVStore version field, which will be used to coordinate Protocol HCUs.