From ddf8d03018837e6b571ce86e3bf52599ed96490e Mon Sep 17 00:00:00 2001 From: Gilad Chase Date: Tue, 5 Nov 2024 20:26:10 +0200 Subject: [PATCH] chore(consensus): add lints to consensus 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. --- crates/sequencing/papyrus_consensus/Cargo.toml | 3 +++ .../papyrus_consensus/src/bin/run_simulation.rs | 7 +++++-- crates/sequencing/papyrus_consensus/src/manager.rs | 1 + .../src/simulation_network_receiver.rs | 2 ++ .../papyrus_consensus/src/single_height_consensus.rs | 10 ++++++++-- 5 files changed, 19 insertions(+), 4 deletions(-) diff --git a/crates/sequencing/papyrus_consensus/Cargo.toml b/crates/sequencing/papyrus_consensus/Cargo.toml index 25be426ad6..f20976f497 100644 --- a/crates/sequencing/papyrus_consensus/Cargo.toml +++ b/crates/sequencing/papyrus_consensus/Cargo.toml @@ -34,3 +34,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 diff --git a/crates/sequencing/papyrus_consensus/src/bin/run_simulation.rs b/crates/sequencing/papyrus_consensus/src/bin/run_simulation.rs index 90b877f18b..f3f1122fa7 100644 --- a/crates/sequencing/papyrus_consensus/src/bin/run_simulation.rs +++ b/crates/sequencing/papyrus_consensus/src/bin/run_simulation.rs @@ -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"); } @@ -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)] // FIXME: precision loss possible ("cache_size", papyrus_args.cache_size.map(|v| v as f64)), + #[allow(clippy::as_conversions)] // FIXME: precision loss possible. ("random_seed", papyrus_args.random_seed.map(|v| v as f64)), ]; for (key, value) in conditional_test_params { diff --git a/crates/sequencing/papyrus_consensus/src/manager.rs b/crates/sequencing/papyrus_consensus/src/manager.rs index 94a1606803..7c4cc6e448 100644 --- a/crates/sequencing/papyrus_consensus/src/manager.rs +++ b/crates/sequencing/papyrus_consensus/src/manager.rs @@ -60,6 +60,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); diff --git a/crates/sequencing/papyrus_consensus/src/simulation_network_receiver.rs b/crates/sequencing/papyrus_consensus/src/simulation_network_receiver.rs index 28fe2f8fc5..876032bf97 100644 --- a/crates/sequencing/papyrus_consensus/src/simulation_network_receiver.rs +++ b/crates/sequencing/papyrus_consensus/src/simulation_network_receiver.rs @@ -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 } @@ -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; } diff --git a/crates/sequencing/papyrus_consensus/src/single_height_consensus.rs b/crates/sequencing/papyrus_consensus/src/single_height_consensus.rs index dfc1c7195b..ba2e4396c0 100644 --- a/crates/sequencing/papyrus_consensus/src/single_height_consensus.rs +++ b/crates/sequencing/papyrus_consensus/src/single_height_consensus.rs @@ -60,7 +60,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, @@ -445,7 +447,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 })) } }