-
Notifications
You must be signed in to change notification settings - Fork 6
feat: add governance bootstrap #506
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
Conversation
sandtreader
left a comment
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.
Satellite-view review really - I don't know the governance details enough to verify the decoder properly. @shd may be better placed.
| } | ||
| }; | ||
|
|
||
| // If governance parsing failed, use fallback parsing for pparams |
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.
Is this still needed or just legacy?
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.
It's not, I've removed it here eb263f0
| gov_msg.proposals.len() | ||
| ); | ||
| } | ||
| // Ignore other snapshot messages |
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.
Same comment as DRepState - should we handle the completion message explicitly?
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 think we do want to receive the Complete message and then break. Something like:
Message::Snapshot(SnapshotMessage::Complete) => {
info!("Snapshot complete, exiting governance state bootstrap loop");
break; // done processing snapshot messages
}
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.
Yep! Resolved here
| e | ||
| ); | ||
| // Reset decoder position and try fallback parsing | ||
| remainder_decoder = Decoder::new(&remainder_buffer[gov_state_position..]); |
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.
This is great. Thanks for keeping the fallback, but I think we should log an error here instead of an info. At least a warn is appropriate.
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.
| // We need to re-parse pparams from the gov_state - this is a limitation that could be improved | ||
| // For now, use defaults and rely on the protocol_parameters callback from earlier parsing | ||
| ( | ||
| ProtocolParameters::default(), |
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.
If governance parsing succeeds, do we use these defaults? That will result in the protocol_parameters callback getting default data, which is not good. I don't understand how this would work. If you did parser the protocol params in the governance stat, then it seems you want to extract them here and return them in this expression.
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've removed that here eb263f0, we do not want to be silently passing defaults.
extract protocol parameters from GovernanceState
inside run() (as was initially done)
`parse_gov_state()` failed, the code would silently continue by manually navigating the CBOR structure to extract just the protocol parameters. Now governance parsing failures propagate as errors instead of being silently ignored.
|
# Conflicts: # modules/snapshot_bootstrapper/src/publisher.rs
Description
Adds governance state bootstrapping from CBOR ledger state snapshots. This enables the
governance_statemodule to be initialized from a snapshot dump instead of requiring replay from genesis.Key changes:
common/src/snapshot/governance.rs): Parses thegov_stateportion of NewEpochState including proposals, committee, constitution, votes fromdrep_pulsing_state, and ratify state (enacted/expired actions)GovernanceBootstrapMessagecontaining proposals, votes, committee, constitution, and proposal rootsConwayVotingstatebootstrap_from_snapshot()populates proposals, votes, and action_status from the bootstrap messageRelated Issue(s)
Completes #385
How was this tested?
Checklist
Impact / Side effects
TxHash::default()since original transaction hashes are not preserved in the snapshot formatReviewer notes / Areas to focus
common/src/snapshot/governance.rs: Core CBOR parsing logic - extensive but follows established patterns from other snapshot parsersmodules/governance_state/src/governance_state.rs:287-345: Snapshot subscription handling ininit()- follows pattern fromdrep_state.rsmodules/governance_state/src/conway_voting.rs:77-110:bootstrap_from_snapshot()method - uses default voting length (6 epochs) if Conway params not yet available