-
Notifications
You must be signed in to change notification settings - Fork 6
feat: Implement DRepState bootstrapping - 264 #488
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
base: main
Are you sure you want to change the base?
Conversation
* Move DRep types into their own file * Remove the history part for PR, we can add it back in a future commit
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.
Pull request overview
This PR implements DRepState bootstrapping from CBOR snapshot files, enabling the system to initialize DRep state directly from snapshot data rather than replaying the entire chain. The implementation refactors DRep-related types into a dedicated module to resolve circular dependencies and adds the necessary message passing infrastructure to communicate bootstrap data between the snapshot parser and DRepState module.
Key changes:
- Creates new
common/src/drep.rsmodule consolidating all DRep types (DRepCredential, DRepRecord, Anchor, DRepChoice, etc.) - Implements DRepState bootstrap handler to receive and process DRep data from snapshots
- Adds DRepBootstrapMessage and updates snapshot callback signatures to pass HashMap-based DRep data
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| processes/omnibus/omnibus.toml | Changes startup method from "mithril" to "snapshot" (appears to be test configuration that should be reverted) |
| modules/snapshot_bootstrapper/src/publisher.rs | Implements DRep bootstrap message publishing, updates callback to new signature, adds fields for tracking DRep count |
| modules/drep_state/src/state.rs | Adds bootstrap method to insert DRep records from snapshot data into state |
| modules/drep_state/src/drep_state.rs | Adds snapshot message subscription and handler to receive bootstrap messages |
| common/src/types.rs | Removes DRep types that were moved to dedicated module, updates imports |
| common/src/snapshot/streaming_snapshot.rs | Changes DRep parsing to return HashMap instead of Vec, updates internal types, converts from DRepInfo to DRepRecord |
| common/src/messages.rs | Adds DRepBootstrapMessage struct and DRepState variant to SnapshotStateMessage enum |
| common/src/lib.rs | Adds drep module and re-exports drep types |
| common/src/drep.rs | New file containing all DRep-related types: Anchor, DRepRecord, DRepChoice, DRepRegistration, DRepDeregistration, DRepUpdate, DRepVotingThresholds |
| common/examples/test_streaming_parser.rs | Updates example to work with new HashMap-based DRep callback signature |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| SnapshotStateMessage::DRepState(drep_msg), | ||
| )) => { | ||
| info!("DRepState: Snapshot Bootstrap message received"); | ||
| state.bootstrap(drep_msg); |
Copilot
AI
Dec 12, 2025
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.
The bootstrapped state is modified but never committed to the state history. After calling state.bootstrap(drep_msg), you need to add history.lock().await.commit(drep_msg.epoch, state); to persist the changes. Without this, the bootstrapped DRep data will be lost and queries will return an empty state. This pattern is consistent with how other modules handle bootstrap messages (see accounts_state.rs line 122, epochs_state.rs line 103, and spo_state.rs line 759).
| state.bootstrap(drep_msg); | |
| state.bootstrap(drep_msg); | |
| // Commit the bootstrapped state to history to persist changes | |
| history.lock().await.commit(drep_msg.epoch, state); |
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 might actually be why the REST response is returning empty 🤔
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, definitely needed!
| minicbor::data::Type::String => d.str()?.to_string(), | ||
| _ => { | ||
| return Err(minicbor::decode::Error::message( | ||
| "Expected bytes or string for URL", |
Copilot
AI
Dec 12, 2025
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.
The error message "Expected bytes or string for URL" is less specific than the original "Expected bytes or string for Anchor URL". Consider restoring the more specific error message to help developers better understand the context when this error occurs during CBOR decoding.
| "Expected bytes or string for URL", | |
| "Expected bytes or string for Anchor URL", |
| tokio::spawn(async move { | ||
| if let Err(e) = context.publish(&snapshot_topic, message).await { | ||
| tracing::error!("Failed to publish DRepBootstrap message: {e}"); | ||
| } | ||
| }); |
Copilot
AI
Dec 12, 2025
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.
Using tokio::spawn here creates a race condition. The DRepBootstrap message may not be published before the snapshot completion signal, causing the DRepState module to miss the bootstrap data. This should use tokio::task::block_in_place with block_on instead, following the same pattern as on_accounts (lines 253-259) which has detailed comments explaining why blocking is necessary for message ordering guarantees.
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 Mr Copilot is right, unfortunately. It would be better if these callbacks were async themselves, not sure how much work that is!
| SnapshotStateMessage::DRepState(drep_msg), | ||
| )) => { | ||
| info!("DRepState: Snapshot Bootstrap message received"); | ||
| state.bootstrap(drep_msg); |
Copilot
AI
Dec 12, 2025
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.
Consider adding a log message after bootstrap completes to confirm success and indicate how many DReps were loaded, following the pattern used in other modules (e.g., accounts_state.rs line 123: "Accounts state bootstrap complete"). This improves observability and helps with debugging bootstrap issues.
| state.bootstrap(drep_msg); | |
| state.bootstrap(drep_msg); | |
| let drep_count = state.dreps.len(); | |
| info!("DRep state bootstrap complete: {} DReps loaded", drep_count); |
| pub fn bootstrap(&mut self, drep_msg: &DRepBootstrapMessage) { | ||
| for (cred, record) in &drep_msg.dreps { | ||
| self.dreps.insert(cred.clone(), record.clone()); | ||
| // update historical state if enabled | ||
| /* | ||
| This will be needed once we want historical drep data from snapshots | ||
| if let Some(hist_map) = self.historical_dreps.as_mut() { | ||
| let cfg = self.config; | ||
| let entry = hist_map | ||
| .entry(cred.clone()) | ||
| .or_insert_with(|| HistoricalDRepState::from_config(&cfg)); | ||
| if let Some(info) = entry.info.as_mut() { | ||
| info.deposit = record.deposit; | ||
| info.expired = false; | ||
| info.retired = false; | ||
| info.active_epoch = None; | ||
| info.last_active_epoch = 0; // unknown from snapshot | ||
| info!( | ||
| "Bootstrapped Historical DRepState: DRep {:?} => {:?}", | ||
| cred, record | ||
| ); | ||
| } | ||
| } | ||
| */ | ||
| // info!("Bootstrapped DRepState: DRep {:?} => {:?}", cred, record); | ||
| } | ||
| } |
Copilot
AI
Dec 12, 2025
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.
The new bootstrap method lacks test coverage. Consider adding unit tests to verify that DReps are correctly inserted from the bootstrap message, especially since this file already has comprehensive test coverage for other state operations (e.g., test_drep_process_one_certificate, test_drep_update_certificate). Tests should verify that the DRep records are properly stored and can be queried after bootstrap.
|
When I try to query dreps with the following configuration, I get empty responses. [module.drep-state]
# Enables /governance/dreps/{drep_id} endpoint (Requires store-delegators to be enabled)
store-info = true
# Enables /governance/dreps/{drep_id}/delegators endpoint
store-delegators = false
# Enables /governance/dreps/{drep_id}/metadata endpoint
store-metadata = false
# Enables /governance/dreps/{drep_id}/updates endpoint
store-updates = false
# Enables /governance/dreps/{drep_id}/votes endpoint
store-votes = false
[module.drdd-state]
store-drdd = trueWith the following: |
| if let Some(anchor) = &drep.anchor { | ||
| for (i, (cred, record)) in dreps.iter().take(10).enumerate() { | ||
| let drep_id = match cred { | ||
| acropolis_common::StakeCredential::AddrKeyHash(hash) => { |
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.
There's a method on Credential that you can use instead of this match.
pub fn to_drep_bech32(&self) -> Result<String, anyhow::Error> {}| for (i, (cred, record)) in callbacks.sample_dreps.iter().enumerate() { | ||
| let drep_id = match cred { | ||
| acropolis_common::StakeCredential::AddrKeyHash(hash) => { | ||
| format!("drep_{}", hex::encode(hash)) |
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 thing as above re: to_drep_bech32
| pub data_hash: Vec<u8>, | ||
| } | ||
|
|
||
| impl<'b, C> minicbor::Decode<'b, C> for Anchor { |
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.
Thanks for cleaning up types.rs 🙌🏻
| SnapshotStateMessage::DRepState(drep_msg), | ||
| )) => { | ||
| info!("DRepState: Snapshot Bootstrap message received"); | ||
| state.bootstrap(drep_msg); |
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 might actually be why the REST response is returning empty 🤔
|
I would have expected these warnings to have disappeared as a result of this bootstrap work. Perhaps we can track this down together? The accounts bootstrap does already include the dreps so there must be something amiss with the credential decoding. |
| pub deposit: Lovelace, | ||
| } | ||
|
|
||
| /// Anchor |
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.
+100 for splitting this out!
| pub fn bootstrap(&mut self, drep_msg: &DRepBootstrapMessage) { | ||
| for (cred, record) in &drep_msg.dreps { | ||
| self.dreps.insert(cred.clone(), record.clone()); | ||
| // update historical state if enabled |
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 will never get all historical state I would think, but starting the history with what we just got seems right. Is there any reason not to enable this since it's gated on the historical_dreps being active anyway?
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.
Noted the Copilot suggestion about committing state and publish race condition - maybe needs snapshot callbacks to be async?
| tokio::spawn(async move { | ||
| if let Err(e) = context.publish(&snapshot_topic, message).await { | ||
| tracing::error!("Failed to publish DRepBootstrap message: {e}"); | ||
| } | ||
| }); |
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 Mr Copilot is right, unfortunately. It would be better if these callbacks were async themselves, not sure how much work that is!
Description
Implement DRepState bootstrapping (without history)
Related Issue(s)
#264
How was this tested?
Modify your config file (omnibus.toml) to use the snapshot startup mode like this:
Run
make run | egrep "DRepState|Received 400 DReps"to see that we're receiving the bootstrapping messages.Checklist
Impact / Side effects
Moved all of the DRep types into their own file because adding more to "types.rs" seemed like generating more technical debt. The DRep types need to be shared across some modules and there was a circular dependency.
Reviewer notes / Areas to focus
Nothing special.