Skip to content

Commit

Permalink
chore(consensus): add lints to consensus (#1851)
Browse files Browse the repository at this point in the history
Lior banned `as` repo-wide, unless absolutely necessary.

Nearly all as uses can be replaced with [Try]From which doesn't have
implicit coercions like as (we encountered several bugs due to these coercions).

Motivation: we are standardizing lints across the repo and CI, instead
of each crate having separate sets of lints.

Co-Authored-By: Gilad Chase <[email protected]>
  • Loading branch information
giladchase and Gilad Chase authored Nov 10, 2024
1 parent a3e7efc commit 9c61e2f
Show file tree
Hide file tree
Showing 5 changed files with 19 additions and 4 deletions.
3 changes: 3 additions & 0 deletions crates/sequencing/papyrus_consensus/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -35,3 +35,6 @@ papyrus_network_types = { workspace = true, features = ["testing"] }
papyrus_storage = { workspace = true, features = ["testing"] }
papyrus_test_utils.workspace = true
test-case.workspace = true

[lints]
workspace = true
7 changes: 5 additions & 2 deletions crates/sequencing/papyrus_consensus/src/bin/run_simulation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,9 +62,10 @@ impl Node {

async fn stop(&mut self) {
let process = self.process.as_mut().expect("Process not found");
let pid = process.id().unwrap();
let pid = i32::try_from(process.id().unwrap())
.expect("Max PIDs on unix are way smaller than i32::MAX");
// Send SIGINT to the entire process group to terminate the process and its subprocesses
nix::sys::signal::killpg(Pid::from_raw(pid as i32), nix::sys::signal::Signal::SIGINT)
nix::sys::signal::killpg(Pid::from_raw(pid), nix::sys::signal::Signal::SIGINT)
.expect("Failed to kill process group");
}

Expand Down Expand Up @@ -296,7 +297,9 @@ async fn build_node(data_dir: &str, logs_dir: &str, i: usize, papyrus_args: &Pap
("invalid_probability", papyrus_args.invalid_probability),
// Convert optional parameters to f64 for consistency in the vector,
// types were validated during parsing.
#[allow(clippy::as_conversions)] // Note: no precision loss if cache size is < ~8PetaBytes.
("cache_size", papyrus_args.cache_size.map(|v| v as f64)),
#[allow(clippy::as_conversions)] // Note: precision loss for random doesn't matter.
("random_seed", papyrus_args.random_seed.map(|v| v as f64)),
];
for (key, value) in conditional_test_params {
Expand Down
1 change: 1 addition & 0 deletions crates/sequencing/papyrus_consensus/src/manager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ where
tokio::time::sleep(consensus_delay).await;
let mut current_height = start_height;
let mut manager = MultiHeightManager::new(validator_id, timeouts);
#[allow(clippy::as_conversions)] // FIXME: use int metrics so `as f64` may be removed.
loop {
metrics::gauge!(PAPYRUS_CONSENSUS_HEIGHT, current_height.0 as f64);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ impl NetworkReceiver {
}

fn should_drop_msg(&self, msg_hash: u64) -> bool {
#[allow(clippy::as_conversions)]
let prob = (msg_hash as f64) / (u64::MAX as f64);
prob <= self.drop_probability
}
Expand All @@ -104,6 +105,7 @@ impl NetworkReceiver {
mut msg: ConsensusMessage,
msg_hash: u64,
) -> ConsensusMessage {
#[allow(clippy::as_conversions)]
if (msg_hash as f64) / (u64::MAX as f64) > self.invalid_probability {
return msg;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,9 @@ impl SingleHeightConsensus {
timeouts: TimeoutsConfig,
) -> Self {
// TODO(matan): Use actual weights, not just `len`.
let state_machine = StateMachine::new(id, validators.len() as u32);
let n_validators =
u32::try_from(validators.len()).expect("Should have way less than u32::MAX validators");
let state_machine = StateMachine::new(id, n_validators);
Self {
height,
validators,
Expand Down Expand Up @@ -566,7 +568,11 @@ impl SingleHeightConsensus {
})
.collect();
// TODO(matan): Check actual weights.
assert!(supporting_precommits.len() >= self.state_machine.quorum_size() as usize);
assert!(
supporting_precommits.len()
>= usize::try_from(self.state_machine.quorum_size())
.expect("u32 should fit in usize")
);
Ok(ShcReturn::Decision(Decision { precommits: supporting_precommits, block }))
}
}

0 comments on commit 9c61e2f

Please sign in to comment.