-
Notifications
You must be signed in to change notification settings - Fork 38
Documentation: add network configuration and consensus doc + fix cargo test --doc #1263
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: develop
Are you sure you want to change the base?
Conversation
1815fc6
to
532c918
Compare
cb0e598
to
366efbc
Compare
@@ -5,35 +5,53 @@ Derives `[openmina_core::ActionEvent]` trait implementation for action. | |||
For action containers, it simply delegates to inner actions. | |||
|
|||
```rust | |||
# use openmina_core::ActionEvent; | |||
# | |||
use openmina_core::{ActionEvent, log::EventContext}; |
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 Rust in this markdown gets compiled and checked by make test-docs
.
366efbc
to
375b2e8
Compare
…ead of std Move inline `use` statements from function bodies to the top of consensus.rs to follow Rust best practices. Replace `std::cmp` with `core::cmp` for basic comparison functionality that doesn't require the full standard library, making the code more suitable for no_std environments. Changes: - Move `use std::cmp::{max, min}` from `relative_min_window_density()` to file top - Move `use std::cmp::Ordering::*` from fork choice functions to file top - Replace `std::cmp` with `core::cmp` - Use fully qualified `Ordering::` variants instead of wildcard imports - Update all match expressions to use explicit `Ordering::Greater/Less/Equal` All tests pass and documentation builds successfully.
8b58b20
to
5be465c
Compare
/// ## Timing Hierarchy | ||
/// | ||
/// The consensus protocol operates on a hierarchical timing structure: | ||
/// - **Slots**: Basic time units (3 minutes each) |
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.
Note that we want to lower to 90 seconds, see https://github.com/o1-labs/openmina/issues/1259?issue=o1-labs%7Copenmina%7C1265
/// attempt to create blocks during their assigned slots. The duration affects | ||
/// network synchronization requirements and transaction confirmation times. | ||
/// | ||
/// **Value**: 180,000ms (3 minutes) |
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 will be decreased to 90 seconds, see https://github.com/o1-labs/openmina/issues/1259?issue=o1-labs%7Copenmina%7C1265
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.
Looks great. My comments are mostly minor, feel free to merge.
/// ## Timing Hierarchy | ||
/// | ||
/// The consensus protocol operates on a hierarchical timing structure: | ||
/// - **Slots**: Basic time units (3 minutes each) |
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.
Subjective but a bit too detailed for my taste. Slot time might change with hardfork, I'd just add "minutes" and "days" instead.
/// | ||
/// Two chains are considered short-range forks if: | ||
/// 1. They are in the same epoch with matching staking epoch lock checkpoints, OR | ||
/// 2. One chain is exactly one epoch ahead and the trailing chain is not in seed update range |
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.
What is "seed update range"? 🤔
/// | ||
/// ## Returns | ||
/// | ||
/// `true` if the states represent a short-range fork, `false` for long-range forks. |
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.
What does it return if there is no fork = states are identical or one is prefix of another?
/// | ||
/// ## Consensus Protocol Overview | ||
/// | ||
/// Mina uses Ouroboros Samasika, a succinct blockchain consensus algorithm that provides: |
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 sounds a bit offtop to me, I'd say the doc is not the place to discuss pros and cons of our choice of consensus protocol? I'm assuming all these comments are cowritten by claude because they are so well-formatted (correct me if I'm wrong?), but I think some of them are a bit too verbose? wdyt
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.
Yup, cowritten by claude by providing the OCaml codebase and technical documents. The initial idea was to get a mapping between the OCaml and the Rust codebase, and ease the changes from one to the other.
I should have set this PR in draft. I did not review.
No description provided.