parallelchain-io / hotstuff_rs

Rust implementation of the HotStuff consensus algorithm.
34 stars 4 forks source link

0.4 pacemaker #33

Closed karolinagrzeszkiewicz closed 2 months ago

karolinagrzeszkiewicz commented 3 months ago

This PR primary focuses on the Pacemakerimplementation but also includes the following:

  1. Re-naming of the newtype pattern struct methods to comply with Rust's official recommendations,
  2. networking.rs implementation.

It is also intended to include proper documentation, and tentative error handling for all of the above.

The error handling is tentative, since I need to put more thought on how to do it consistently across the entire repository. For instance, currently the PacemakerError is of this from:

#[derive(Debug)]
pub enum PacemakerError {
    UpdateViewError(UpdateViewError),
    ExtendViewError(ExtendViewError),
}

but inside the Pacemaker methods we often call a method of the BlockTree, e.g., committed_validator_set or highest_qc/highest_tc, which can panic. To be consistent, and for the sake of better error handling and testing, the BlockTree getters and setters should also return Result<T, E>, and the E should be propagated to PacemakerError for example like this:

#[derive(Debug)]
pub enum PacemakerError {
    UpdateViewError(UpdateViewError),
    ExtendViewError(ExtendViewError),
    BlockTreeGetError(BlockTreeGetError),
}

Yet building an enum error for this requires some more thought. I am also thinking of implementing std::error::Error, possibly with thiserror crate derive macros, but again this needs to be done consistently across the repository, so I am thinking of doing that in one of the later milestones.

Naming and nesting errors itself is tricky, and ideally should be done at the stage of defining the type signature of methods... Having a fresh (potentially wrapping around another error) type for each caller function is an overkill I think, so often the lower level callee error type should also be the error type of the caller. This is what I did for the Pacemaker. Some other way of grouping errors might be based on the associated struct name of the method, e.g., PacemakerStateError, ViewInfoError.

@lyulka thoughts?