Add epoch parameter in Membership trait's methods
#3751
Conversation
| @@ -232,6 +232,8 @@ pub struct HotShotConfig<KEY: SignatureKey> { | |||
| pub start_voting_time: u64, | |||
| /// Unix time in seconds at which we stop voting on an upgrade. To prevent voting on an upgrade, set stop_voting_time <= start_voting_time. | |||
| pub stop_voting_time: u64, | |||
| /// Number of blocks in an epoch, zero means there are no epochs | |||
| pub epoch_height: u64, | |||
There was a problem hiding this comment.
this might be personal preference, but I feel like this is a protocol-level constant and might be better as an associated constant in the NodeType trait (rather than a configuration value)
There was a problem hiding this comment.
I'm not sure about this. Let's wait for other opinions, ok?
There was a problem hiding this comment.
yeah this one is weird, view timeout is also a parameter of the protocol but is specified in this config. If we were to move this to associated constant, we need to do so for all protocol values (which is at least a few of these values)
There was a problem hiding this comment.
I guess the distinction in my mind was:
nodes could set different view timeouts, round_start_delay etc (within reason) and consensus would probably be fine. nodes could even update it dynamically between views (within reason) and I don't think anything would break
but every single node needs to have exactly the same epoch height, forever, and if we change this at all without an upgrade consensus would break
There was a problem hiding this comment.
Yeah, I tend to agree. Although, there is another point of view here: at least initially we probably want the epoch height to be configurable. For example, in the tests or even in some test deployments we probably don't want the epoch height to be as large as in mainnet. I would suggest: let's leave it as a config field for now and change it to a protocol-level constant at some later stage. Probably with some other values as indicated by Brendon. Would that work?
Closes #3725 #3728
This PR:
epochparameter to most methods of theMembershiptraitepoch_heightconfig field so that the number of blocks in an epoch can be configurable.This PR does not:
This PR does not implement epoch transition. There is no new logic related to epoch transition. This will be added in separate PRs.
Key places to review: