diff --git a/zebra-chain/src/history_tree.rs b/zebra-chain/src/history_tree.rs index efa025fb42d..8453adb94cc 100644 --- a/zebra-chain/src/history_tree.rs +++ b/zebra-chain/src/history_tree.rs @@ -198,12 +198,14 @@ impl NonEmptyHistoryTree { let height = block .coinbase_height() .expect("block must have coinbase height during contextual verification"); - if height - self.current_height != 1 { - panic!( - "added block with height {:?} but it must be {:?}+1", - height, self.current_height - ); - } + + assert!( + height - self.current_height == 1, + "added block with height {:?} but it must be {:?}+1", + height, + self.current_height + ); + let network_upgrade = NetworkUpgrade::current(self.network, height); if network_upgrade != self.network_upgrade { // This is the activation block of a network upgrade. diff --git a/zebra-chain/src/orchard/commitment.rs b/zebra-chain/src/orchard/commitment.rs index 303bd819c03..88b26c2ec80 100644 --- a/zebra-chain/src/orchard/commitment.rs +++ b/zebra-chain/src/orchard/commitment.rs @@ -280,7 +280,7 @@ impl ZcashSerialize for ValueCommitment { impl ZcashDeserialize for ValueCommitment { fn zcash_deserialize(mut reader: R) -> Result { - Self::try_from(reader.read_32_bytes()?).map_err(|e| SerializationError::Parse(e)) + Self::try_from(reader.read_32_bytes()?).map_err(SerializationError::Parse) } } diff --git a/zebra-chain/src/orchard/keys.rs b/zebra-chain/src/orchard/keys.rs index 48237a0b2c0..36be4661db1 100644 --- a/zebra-chain/src/orchard/keys.rs +++ b/zebra-chain/src/orchard/keys.rs @@ -1099,6 +1099,6 @@ impl ZcashSerialize for EphemeralPublicKey { impl ZcashDeserialize for EphemeralPublicKey { fn zcash_deserialize(mut reader: R) -> Result { - Self::try_from(reader.read_32_bytes()?).map_err(|e| SerializationError::Parse(e)) + Self::try_from(reader.read_32_bytes()?).map_err(SerializationError::Parse) } } diff --git a/zebra-chain/src/primitives/zcash_history.rs b/zebra-chain/src/primitives/zcash_history.rs index a6527792c4e..f4438faf4c8 100644 --- a/zebra-chain/src/primitives/zcash_history.rs +++ b/zebra-chain/src/primitives/zcash_history.rs @@ -182,12 +182,13 @@ impl Tree { .coinbase_height() .expect("block must have coinbase height during contextual verification"); let network_upgrade = NetworkUpgrade::current(self.network, height); - if self.network_upgrade != network_upgrade { - panic!( - "added block from network upgrade {:?} but history tree is restricted to {:?}", - network_upgrade, self.network_upgrade - ); - } + + assert!( + self.network_upgrade == network_upgrade, + "added block from network upgrade {:?} but history tree is restricted to {:?}", + network_upgrade, + self.network_upgrade + ); let node_data = V::block_to_history_node(block, self.network, sapling_root, orchard_root); let appended = self.inner.append_leaf(node_data)?; diff --git a/zebra-chain/src/sapling/commitment.rs b/zebra-chain/src/sapling/commitment.rs index 047a0000bed..8d0cc56c725 100644 --- a/zebra-chain/src/sapling/commitment.rs +++ b/zebra-chain/src/sapling/commitment.rs @@ -264,7 +264,7 @@ impl ZcashSerialize for ValueCommitment { impl ZcashDeserialize for ValueCommitment { fn zcash_deserialize(mut reader: R) -> Result { - Self::try_from(reader.read_32_bytes()?).map_err(|e| SerializationError::Parse(e)) + Self::try_from(reader.read_32_bytes()?).map_err(SerializationError::Parse) } } diff --git a/zebra-chain/src/sapling/keys.rs b/zebra-chain/src/sapling/keys.rs index 5e00b04c6a8..cb6806f10b6 100644 --- a/zebra-chain/src/sapling/keys.rs +++ b/zebra-chain/src/sapling/keys.rs @@ -1018,6 +1018,6 @@ impl ZcashSerialize for EphemeralPublicKey { impl ZcashDeserialize for EphemeralPublicKey { fn zcash_deserialize(mut reader: R) -> Result { - Self::try_from(reader.read_32_bytes()?).map_err(|e| SerializationError::Parse(e)) + Self::try_from(reader.read_32_bytes()?).map_err(SerializationError::Parse) } } diff --git a/zebra-consensus/src/primitives/groth16/params.rs b/zebra-consensus/src/primitives/groth16/params.rs index 80c2c1d3bc9..cb16b0418e6 100644 --- a/zebra-consensus/src/primitives/groth16/params.rs +++ b/zebra-consensus/src/primitives/groth16/params.rs @@ -60,13 +60,14 @@ impl SaplingParams { io::copy(&mut spend_fs, &mut sink)?; io::copy(&mut output_fs, &mut sink)?; - if spend_fs.into_hash() != SAPLING_SPEND_HASH { - panic!("Sapling spend parameter is not correct."); - } - - if output_fs.into_hash() != SAPLING_OUTPUT_HASH { - panic!("Sapling output parameter is not correct."); - } + assert!( + spend_fs.into_hash() == SAPLING_SPEND_HASH, + "Sapling spend parameter is not correct." + ); + assert!( + output_fs.into_hash() == SAPLING_OUTPUT_HASH, + "Sapling output parameter is not correct." + ); // Prepare verifying keys let spend_prepared_verifying_key = groth16::prepare_verifying_key(&spend.vk); diff --git a/zebra-network/src/peer/connection.rs b/zebra-network/src/peer/connection.rs index 6010243c050..bc0aaa0208d 100644 --- a/zebra-network/src/peer/connection.rs +++ b/zebra-network/src/peer/connection.rs @@ -550,20 +550,21 @@ where // Error slots use a threaded `std::sync::Mutex`, so accessing the slot // can block the async task's current thread. We only perform a single // slot update per `Client`, and panic to enforce this constraint. - if self.error_slot.try_update_error(e).is_err() { - // This panic typically happens due to these bugs: - // * we mark a connection as failed without using fail_with - // * we call fail_with without checking for a failed connection - // state - // * we continue processing messages after calling fail_with - // - // See the original bug #1510 and PR #1531, and the later bug #1599 - // and PR #1600. - panic!("calling fail_with on already-failed connection state: failed connections must stop processing pending requests and responses, then close the connection. state: {:?} client receiver: {:?}", - self.state, - self.client_rx - ); - } + // + // This assertion typically fails due to these bugs: + // * we mark a connection as failed without using fail_with + // * we call fail_with without checking for a failed connection + // state + // * we continue processing messages after calling fail_with + // + // See the original bug #1510 and PR #1531, and the later bug #1599 + // and PR #1600. + assert!( + self.error_slot.try_update_error(e).is_ok(), + "calling fail_with on already-failed connection state: failed connections must stop processing pending requests and responses, then close the connection. state: {:?} client receiver: {:?}", + self.state, + self.client_rx + ); // We want to close the client channel and set State::Failed so // that we can flush any pending client requests. However, we may have diff --git a/zebra-state/src/service/non_finalized_state.rs b/zebra-state/src/service/non_finalized_state.rs index c74c61bfe16..de347847aee 100644 --- a/zebra-state/src/service/non_finalized_state.rs +++ b/zebra-state/src/service/non_finalized_state.rs @@ -83,6 +83,10 @@ impl NonFinalizedState { /// Finalize the lowest height block in the non-finalized portion of the best /// chain and update all side-chains to match. pub fn finalize(&mut self) -> FinalizedBlock { + // Chain::cmp uses the partial cumulative work, and the hash of the tip block. + // Neither of these fields has interior mutability. + // (And when the tip block is dropped for a chain, the chain is also dropped.) + #[allow(clippy::mutable_key_type)] let chains = mem::take(&mut self.chain_set); let mut chains = chains.into_iter(); @@ -240,6 +244,9 @@ impl NonFinalizedState { where F: Fn(&Chain) -> bool, { + // Chain::cmp uses the partial cumulative work, and the hash of the tip block. + // Neither of these fields has interior mutability. + #[allow(clippy::mutable_key_type)] let chains = mem::take(&mut self.chain_set); let mut best_chain_iter = chains.into_iter().rev(); diff --git a/zebra-state/src/service/non_finalized_state/chain.rs b/zebra-state/src/service/non_finalized_state/chain.rs index 2644bd84545..4b82d043d57 100644 --- a/zebra-state/src/service/non_finalized_state/chain.rs +++ b/zebra-state/src/service/non_finalized_state/chain.rs @@ -874,6 +874,11 @@ impl Ord for Chain { /// /// https://zips.z.cash/protocol/protocol.pdf#blockchain /// + /// # Correctness + /// + /// `Chain::cmp` is used in a `BTreeSet`, so the fields accessed by `cmp` must not have + /// interior mutability. + /// /// # Panics /// /// If two chains compare equal.