Skip to content

Improve documentation for consensus crate and remove (potentially dangerous) unwrap() #3591

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

Open
wants to merge 2 commits into
base: staging
Choose a base branch
from

Conversation

kaimast
Copy link
Collaborator

@kaimast kaimast commented Apr 10, 2025

I found the role of the snarkos-node-consensus crate and its relation to the snarkos-node-bft crate hard to understand. This pull request adds more documentation to those two crates.

The PR also removes an unwrap() that used to be invoked in production code when calling Consensus::primary_sender().
To do this, the functionality that was formerly in Consensus::run is now in its constructors, so that other code can only interact with the crate's public API after it is fully initialized, whereas before one could potentially not invoke run() and cause a panic.

Finally, the PR makes some functions, which were not used anywhere alse, as non-public or removes them entirely.

@kaimast
Copy link
Collaborator Author

kaimast commented Apr 10, 2025

@acoglio This is (mostly) an improvement on documentation, so you might be a good person to review this.

@@ -1133,6 +1133,13 @@ impl<N: Network> Primary<N> {

impl<N: Network> Primary<N> {
/// Starts the primary handlers.
///
/// For each receiver in the `primary_receiver` struct, there will be a dedicated tasks
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/// For each receiver in the `primary_receiver` struct, there will be a dedicated tasks
/// For each receiver in the `primary_receiver` struct, there will be a dedicated task

Co-authored-by: Alessandro Coglio <[email protected]>
Signed-off-by: Kai Mast <[email protected]>
@raychu86
Copy link
Contributor

The documentation additions are great and the unification of new and run to get around the .expect call makes sense to me, but this brings up the question of consistency around the other modules.

A lot of other structs use a similar new and run interface (BFT, Primary, Sync, gateway, etc); I believe some even have an additional initialize function. There are likely ordering considerations that I have not thought about, but should these be unified for consistency?

@acoglio
Copy link
Collaborator

acoglio commented Apr 11, 2025

From a (formal and informal) reasoning perspective, generally the more (ideally, all) "initialization" (in a broad sense) can be done in the constructor, the better, because we get the strongest invariants just after construction; otherwise, structs between construction and running/initialization may have weaker invariants.

This is not always possible (when there are circular dependencies of sort), and it should be balanced with other considerations, but it does have the advantage mentioned above.

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.

3 participants