Skip to content
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

feat: implement and calculate the current key period using pallas #1541

Merged
merged 24 commits into from Mar 12, 2024

Conversation

falcucci
Copy link
Contributor

@falcucci falcucci commented Feb 28, 2024

Pre-submit checklist

  • Branch
    • Tests are provided (if possible)
    • Crates versions are updated (if relevant)
    • Commit sequence broadly makes sense
    • Key commits have useful messages
  • PR
    • No clippy warnings in the CI
    • Self-reviewed the diff
    • Useful pull request description
    • Reviewer requested
  • Documentation
    • Update README file (if relevant)
    • Update documentation website (if relevant)
    • Add dev blog post (if relevant)

fixes #1315

- update the source of dependencies `pallas-addresses`, `pallas-codec`, `pallas-network`, `pallas-primitives`, `pallas-traverse` to a git repository with branch `feat/genesis-config`
- add a new method `calculate_kes_period` to fetch chain point and genesis config through local statequery
- implement logic in the `calculate_kes_period` method to calculate the kes period based on retrieved data
- remove the `fallback` field from `pallaschainobserver`
- update the `pallaschainobserver::new` method signature to no longer accept `fallback` argument in `pallas_observer.rs`
- update the test functions in the `mod tests` section to no longer use `fallback` in `pallas_observer.rs`
- remove the `fallback` field and related code in `mithrilinfrastructure` in `infrastructure.rs`
- remove unnecessary imports in `chain_observer/pallas_observer.rs`
- fix formatting issues in the code
- simplify the `get_fake_utxo_by_address` function definition
- update the `pallas-addresses`, `pallas-codec`, `pallas-network`, `pallas-primitives`, `pallas-traverse`, and `pallas-crypto` dependencies to use the `txpipe` repository and `main` branch instead of the `falcucci` repository and `feat/genesis-config` branch in `cargo.toml`
- refactor the code to retrieve `slots_per_kes_period` from `config` instead of directly from `genesis_config`
- improve error handling by adding context to the error message
- adjust the calculation of `current_kes_period` to use the updated `slots_per_kes_period` calculation from `config`
- change error message from obtaining genesis config to extracting the config.
@falcucci
Copy link
Contributor Author

hey @jpraynaud ready for review

Copy link
Member

@jpraynaud jpraynaud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @falcucci, thanks for the good work 👏

I left few comments to address before we can merge the PR

- add validation for slots_per_kes_period being greater than 0
- update error message for slots_per_kes_period being 0
- add a new async function `calculate_kes_period` that takes `&self, chain_point: point, slots_per_kes_period: u64` as arguments
- rename the function `calculate_kes_period` to `get_kes_period`
- modify code in the test mod to match the changes in `calculate_kes_period` function
- update the specific point value from 53536042 to 52851885 in the `getchainpoint` request.
- add a comment for the `calculate_kes_period` method
- include `chain_point` and `slots_per_kes_period` as arguments at the method definition
Copy link
Member

@jpraynaud jpraynaud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @falcucci, I left few other comments 🙂

mithril-common/src/chain_observer/pallas_observer.rs Outdated Show resolved Hide resolved
mithril-common/src/chain_observer/pallas_observer.rs Outdated Show resolved Hide resolved
mithril-common/src/chain_observer/pallas_observer.rs Outdated Show resolved Hide resolved
mithril-common/src/chain_observer/pallas_observer.rs Outdated Show resolved Hide resolved
mithril-common/src/chain_observer/pallas_observer.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@Alenar Alenar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work, thanks ! I just have a minor comment.

mithril-common/src/chain_observer/pallas_observer.rs Outdated Show resolved Hide resolved
- change return type of function from `u32` to `kesperiod`
- fix error handling and conversion in the function
- update test case with new chain point slot value
- refactor the `pallaschainobserver` implementation to handle `slots_per_kes_period` being `0` differently
- update the calculation of `current_kes_period` to handle the new logic for `slots_per_kes_period` being `0`
- add detailed documentation for the `calculate_kes_period` function and its usage
- improve the explanation of the calculation formula for the kes period
- include an example with input values for the calculation formula
- fix a typo in the variable description from `s symbolizes` to `s represents`
@falcucci
Copy link
Contributor Author

falcucci commented Mar 1, 2024

@jpraynaud @Alenar done, thank you.

Copy link
Member

@jpraynaud jpraynaud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

I just left a last comment of the computation explanation for the KES Period.

mithril-common/src/chain_observer/pallas_observer.rs Outdated Show resolved Hide resolved
- update the comment and variable names in `mithril-common/src/chain_observer/pallas_observer.rs` to improve clarity.
@jpraynaud
Copy link
Member

Hi @falcucci, it looks like some clippy warnings need also to be fixed: https://github.com/input-output-hk/mithril/actions/runs/8113314179/job/22176650787#step:4:1110

- remove unused imports related to chainobserver in infrastructure.rs
- update the import to use pallaschainobserver directly
@falcucci
Copy link
Contributor Author

falcucci commented Mar 1, 2024

@jpraynaud true, fixed.

- update the version number in `mithril-common/cargo.toml` from `0.3.15` to `0.3.16`
- change code block syntax in comments to `no_run`
- update comments in code for example clarity
///
/// ## Example:
///
///! ```no_run

Check warning

Code scanning / clippy

this is an outer doc comment and does not apply to the parent module or crate

this is an outer doc comment and does not apply to the parent module or crate
///
/// ## Example:
///
///! ```no_run

Check warning

Code scanning / clippy

this is an outer doc comment and does not apply to the parent module or crate

this is an outer doc comment and does not apply to the parent module or crate
- update comments for the `calculate current kes period` function in `mithril-common/src/chain_observer/pallas_observer.rs`
Comment on lines 237 to 259
///! # Calculate Current KES Period
///!
///! It calculates the current Key Evolving Signature (KES) period
///! based on the provided `chain_point` and `slots_per_kes_period`.
///!
///! The calculation formula is represented as:
///!
///! `current_kes_period = ⌊current_slot_number/slots_per_kes_period⌋`
///!
///! where:
///! - `current_slot_number` represents the current slot number given by the `point` on the chain.
///! - `slots_per_kes_period` represents the number of slots in a KES period.
///! - `⌊x⌋` is the floor function which rounds the greatest integer less than or equal to `x`.
///!
///! ## Example:
///!
///!! ```no_run
///! let (chain_point, slots_per_kes_period) = (Point::new(1), 10);
///! match calculate_kes_period(&self, chain_point, slots_per_kes_period) {
///! Ok(kes_period) => println!("Current KES Period: {}", kes_period),
///! Err(e) => println!("Error occurred: {}", e),
///! }
///! ```

Check warning

Code scanning / clippy

this is an outer doc comment and does not apply to the parent module or crate

this is an outer doc comment and does not apply to the parent module or crate
Comment on lines 237 to 259
///! # Calculate Current KES Period
///!
///! It calculates the current Key Evolving Signature (KES) period
///! based on the provided `chain_point` and `slots_per_kes_period`.
///!
///! The calculation formula is represented as:
///!
///! `current_kes_period = ⌊current_slot_number/slots_per_kes_period⌋`
///!
///! where:
///! - `current_slot_number` represents the current slot number given by the `point` on the chain.
///! - `slots_per_kes_period` represents the number of slots in a KES period.
///! - `⌊x⌋` is the floor function which rounds the greatest integer less than or equal to `x`.
///!
///! ## Example:
///!
///!! ```no_run
///! let (chain_point, slots_per_kes_period) = (Point::new(1), 10);
///! match calculate_kes_period(&self, chain_point, slots_per_kes_period) {
///! Ok(kes_period) => println!("Current KES Period: {}", kes_period),
///! Err(e) => println!("Error occurred: {}", e),
///! }
///! ```

Check warning

Code scanning / clippy

this is an outer doc comment and does not apply to the parent module or crate

this is an outer doc comment and does not apply to the parent module or crate
- updated the documentation for the `calculate current kes period` function in `pallas_observer.rs`
Comment on lines 253 to 259
///! ```no_run
///! let (chain_point, slots_per_kes_period) = (Point::new(1), 10);
///! match calculate_kes_period(&self, chain_point, slots_per_kes_period) {
///! Ok(kes_period) => println!("Current KES Period: {}", kes_period),
///! Err(e) => println!("Error occurred: {}", e),
///! }
///! ```

Check warning

Code scanning / clippy

this is an outer doc comment and does not apply to the parent module or crate

this is an outer doc comment and does not apply to the parent module or crate
Comment on lines 253 to 259
///! ```no_run
///! let (chain_point, slots_per_kes_period) = (Point::new(1), 10);
///! match calculate_kes_period(&self, chain_point, slots_per_kes_period) {
///! Ok(kes_period) => println!("Current KES Period: {}", kes_period),
///! Err(e) => println!("Error occurred: {}", e),
///! }
///! ```

Check warning

Code scanning / clippy

this is an outer doc comment and does not apply to the parent module or crate

this is an outer doc comment and does not apply to the parent module or crate
- update `calculate_kes_period` function in `pallas_observer.rs` to include a new example with chain_point and slots_per_kes_period values
- remove the old example from the `calculate_kes_period` function in `pallas_observer.rs`
@jpraynaud jpraynaud merged commit 0945639 into input-output-hk:main Mar 12, 2024
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhance Mithril/Cardano node communication
3 participants