refactor: only pass future param set to parameters_state module #521
Conversation
…rapper Signed-off-by: William Hankins <[email protected]>
| previous_reward_params.desired_number_of_stake_pools | ||
| ); | ||
| eprintln!( | ||
| " Max Transaction Size: {}", |
There was a problem hiding this comment.
why remove all this output from the test streamer? I use it.
There was a problem hiding this comment.
I've readded the logging for all parameters (except cost models and deprecated params) for the bootstrap params set in 7bd0fcb.
| self.gs_previous_params = Some(gs_previous_params); | ||
| self.gs_current_params = Some(gs_current_params); | ||
| self.gs_future_params = Some(gs_future_params); | ||
| self.previous_reward_params = Some(previous_reward_params); |
There was a problem hiding this comment.
are these names really better?
There was a problem hiding this comment.
I did this in an attempt to signal that the reward params are a subset of the full params for that epoch. Happy to change it back if you don't think it's useful.
There was a problem hiding this comment.
Ah, I see. Because you remove the other parameters, they are now just reward params. I would not have done that because I would have kept all of the params and not removed the others because it separates concerns. The parser should supply all of the fields it can, without implementing any reduction based on usage down-stream.
Signed-off-by: William Hankins <[email protected]>
Signed-off-by: William Hankins <[email protected]>
Signed-off-by: William Hankins <[email protected]>
…alues Signed-off-by: William Hankins <[email protected]>
|
I will be implementing the changes needed to pass reward params to accounts state in a follow up PR. |
|
Great job on this @whankinsiv 🙌🏻 |
| Self(value) | ||
| } | ||
|
|
||
| pub fn to_network_name(&self) -> &str { |
| } | ||
| } | ||
|
|
||
| pub fn apply_bootstrap(&mut self, params: ProtocolParamUpdate) -> Result<()> { |
buddhisthead
left a comment
buddhisthead
left a comment
There was a problem hiding this comment.
Will, this is great work. I don't need any additional changes. Thank you. I recommend asking Dmitry to look at the changes in how you apply the rewards because he wrote the initial function to create the updater.
Signed-off-by: William Hankins <[email protected]>
…snapshot Signed-off-by: William Hankins <[email protected]>
Signed-off-by: William Hankins <[email protected]>
Description
This PR refactors the
GovernanceProtocolParametersBootstrapMessageto publish only the single parameter set required for bootstrappingparameter_state, instead of publishing multiple parameter variants and deriving the active set inside the state module.As part of this, the snapshot bootstrap logic now decodes directly into
ProtocolParamUpdate, removing the snapshot specific conversion logic fromparameter_stateand simplifying the bootstrap path.Related Issue(s)
Relates to #387
How was this tested?
parameter_statefor epoch 507 match Blockfrost's response (with the exception of some missing Plutus V3 cost models).parameter_statenow publishes a singleProtocolParamsMessage, eliminating one potential source of deadlock during bootstrap.Checklist
Impact / Side effects
ProtocolParamUpdate, with current and future parameters merged before publishing the bootstrap message.RewardParamstype.Reviewer notes / Areas to focus
Snapshot decoding changes in
/common/src/snapshot/protocol_parameters.rs.Note: This refactor also makes it explicit which parameter set applies to a given epoch. I suspect part of our existing reward calculation issues stem from using the current parameter set rather than the parameter set that was active for the epoch being rewarded.