From 4078e244d34a6e8f1010d3387a1b9a214766cde1 Mon Sep 17 00:00:00 2001 From: teor Date: Thu, 8 Dec 2022 16:11:33 +1000 Subject: [PATCH] fix(lint): Box large error types to resolve the clippy large result err variant lint (#5759) * Box errors to deal with large error warnings, add accessor methods for error properties * Remove or explain some large enum variant lints * Turn some tickets into TODOs * Allow missing docs on single-field error enum variants Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> --- zebra-chain/src/transaction.rs | 2 - zebra-consensus/src/block.rs | 12 ++++ zebra-consensus/src/chain.rs | 59 +++++++++++++------ zebra-consensus/src/checkpoint.rs | 18 +++++- zebra-consensus/src/error.rs | 30 +++++++--- zebra-rpc/src/methods.rs | 10 ++++ .../src/methods/get_block_template_rpcs.rs | 22 +++---- zebrad/src/components/mempool.rs | 2 + zebrad/src/components/sync.rs | 36 ++++------- 9 files changed, 124 insertions(+), 67 deletions(-) diff --git a/zebra-chain/src/transaction.rs b/zebra-chain/src/transaction.rs index e2b02d27..88770bcf 100644 --- a/zebra-chain/src/transaction.rs +++ b/zebra-chain/src/transaction.rs @@ -59,8 +59,6 @@ use crate::{ /// activation, we do not validate any pre-Sapling transaction types. #[derive(Clone, Debug, PartialEq, Eq)] #[cfg_attr(any(test, feature = "proptest-impl"), derive(Serialize))] -// XXX consider boxing the Optional fields of V4 and V5 txs -#[allow(clippy::large_enum_variant)] pub enum Transaction { /// A fully transparent transaction (`version = 1`). V1 { diff --git a/zebra-consensus/src/block.rs b/zebra-consensus/src/block.rs index b997f939..a822323f 100644 --- a/zebra-consensus/src/block.rs +++ b/zebra-consensus/src/block.rs @@ -71,12 +71,24 @@ pub enum VerifyBlockError { Time(zebra_chain::block::BlockTimeError), #[error("unable to commit block after semantic verification")] + // TODO: make this into a concrete type, and add it to is_duplicate_request() (#2908) Commit(#[source] BoxError), #[error("invalid transaction")] Transaction(#[from] TransactionError), } +impl VerifyBlockError { + /// Returns `true` if this is definitely a duplicate request. + /// Some duplicate requests might not be detected, and therefore return `false`. + pub fn is_duplicate_request(&self) -> bool { + match self { + VerifyBlockError::Block { source, .. } => source.is_duplicate_request(), + _ => false, + } + } +} + /// The maximum allowed number of legacy signature check operations in a block. /// /// This consensus rule is not documented, so Zebra follows the `zcashd` implementation. diff --git a/zebra-consensus/src/chain.rs b/zebra-consensus/src/chain.rs index 9baef5b8..59803180 100644 --- a/zebra-consensus/src/chain.rs +++ b/zebra-consensus/src/chain.rs @@ -91,12 +91,42 @@ where } /// An error while semantically verifying a block. +// +// One or both of these error variants are at least 140 bytes #[derive(Debug, Display, Error)] +#[allow(missing_docs)] pub enum VerifyChainError { - /// block could not be checkpointed - Checkpoint(#[source] VerifyCheckpointError), - /// block could not be verified - Block(#[source] VerifyBlockError), + /// Block could not be checkpointed + Checkpoint { source: Box }, + /// Block could not be full-verified + Block { source: Box }, +} + +impl From for VerifyChainError { + fn from(err: VerifyCheckpointError) -> Self { + VerifyChainError::Checkpoint { + source: Box::new(err), + } + } +} + +impl From for VerifyChainError { + fn from(err: VerifyBlockError) -> Self { + VerifyChainError::Block { + source: Box::new(err), + } + } +} + +impl VerifyChainError { + /// Returns `true` if this is definitely a duplicate request. + /// Some duplicate requests might not be detected, and therefore return `false`. + pub fn is_duplicate_request(&self) -> bool { + match self { + VerifyChainError::Checkpoint { source, .. } => source.is_duplicate_request(), + VerifyChainError::Block { source, .. } => source.is_duplicate_request(), + } + } } impl Service> for ChainVerifier @@ -132,28 +162,19 @@ where // The chain verifier holds one slot in each verifier, for each concurrent task. // Therefore, any shared buffers or batches polled by these verifiers should double // their bounds. (For example, the state service buffer.) - ready!(self - .checkpoint - .poll_ready(cx) - .map_err(VerifyChainError::Checkpoint))?; - ready!(self.block.poll_ready(cx).map_err(VerifyChainError::Block))?; + ready!(self.checkpoint.poll_ready(cx))?; + ready!(self.block.poll_ready(cx))?; Poll::Ready(Ok(())) } fn call(&mut self, block: Arc) -> Self::Future { match block.coinbase_height() { - Some(height) if height <= self.max_checkpoint_height => self - .checkpoint - .call(block) - .map_err(VerifyChainError::Checkpoint) - .boxed(), + Some(height) if height <= self.max_checkpoint_height => { + self.checkpoint.call(block).map_err(Into::into).boxed() + } // This also covers blocks with no height, which the block verifier // will reject immediately. - _ => self - .block - .call(block) - .map_err(VerifyChainError::Block) - .boxed(), + _ => self.block.call(block).map_err(Into::into).boxed(), } } } diff --git a/zebra-consensus/src/checkpoint.rs b/zebra-consensus/src/checkpoint.rs index 8d8c8f4c..155065e9 100644 --- a/zebra-consensus/src/checkpoint.rs +++ b/zebra-consensus/src/checkpoint.rs @@ -880,7 +880,7 @@ pub enum VerifyCheckpointError { #[error(transparent)] CheckpointList(BoxError), #[error(transparent)] - VerifyBlock(BoxError), + VerifyBlock(VerifyBlockError), #[error("too many queued blocks at this height")] QueuedLimit, #[error("the block hash does not match the chained checkpoint hash, expected {expected:?} found {found:?}")] @@ -894,7 +894,7 @@ pub enum VerifyCheckpointError { impl From for VerifyCheckpointError { fn from(err: VerifyBlockError) -> VerifyCheckpointError { - VerifyCheckpointError::VerifyBlock(err.into()) + VerifyCheckpointError::VerifyBlock(err) } } @@ -910,6 +910,20 @@ impl From for VerifyCheckpointError { } } +impl VerifyCheckpointError { + /// Returns `true` if this is definitely a duplicate request. + /// Some duplicate requests might not be detected, and therefore return `false`. + pub fn is_duplicate_request(&self) -> bool { + match self { + VerifyCheckpointError::AlreadyVerified { .. } => true, + // TODO: make this duplicate-incomplete + VerifyCheckpointError::NewerRequest { .. } => true, + VerifyCheckpointError::VerifyBlock(block_error) => block_error.is_duplicate_request(), + _ => false, + } + } +} + /// The CheckpointVerifier service implementation. /// /// After verification, the block futures resolve to their hashes. diff --git a/zebra-consensus/src/error.rs b/zebra-consensus/src/error.rs index dc24b9f7..7b1cd253 100644 --- a/zebra-consensus/src/error.rs +++ b/zebra-consensus/src/error.rs @@ -184,9 +184,17 @@ pub enum TransactionError { #[error("could not validate nullifiers and anchors on best chain")] #[cfg_attr(any(test, feature = "proptest-impl"), proptest(skip))] - ValidateNullifiersAndAnchorsError(#[from] ValidateContextError), + // This error variant is at least 128 bytes + ValidateNullifiersAndAnchorsError(Box), } +impl From for TransactionError { + fn from(err: ValidateContextError) -> Self { + TransactionError::ValidateNullifiersAndAnchorsError(Box::new(err)) + } +} + +// TODO: use a dedicated variant and From impl for each concrete type, and update callers (#5732) impl From for TransactionError { fn from(mut err: BoxError) -> Self { // TODO: handle redpallas::Error, ScriptInvalid, InvalidSignature @@ -212,12 +220,6 @@ impl From for TransactionError { } } -impl From for BlockError { - fn from(err: SubsidyError) -> BlockError { - BlockError::Transaction(TransactionError::Subsidy(err)) - } -} - #[derive(Error, Clone, Debug, PartialEq, Eq)] pub enum BlockError { #[error("block contains invalid transactions")] @@ -290,3 +292,17 @@ pub enum BlockError { source: amount::Error, }, } + +impl From for BlockError { + fn from(err: SubsidyError) -> BlockError { + BlockError::Transaction(TransactionError::Subsidy(err)) + } +} + +impl BlockError { + /// Returns `true` if this is definitely a duplicate request. + /// Some duplicate requests might not be detected, and therefore return `false`. + pub fn is_duplicate_request(&self) -> bool { + matches!(self, BlockError::AlreadyInChain(..)) + } +} diff --git a/zebra-rpc/src/methods.rs b/zebra-rpc/src/methods.rs index 1577ab8d..86d2a9bb 100644 --- a/zebra-rpc/src/methods.rs +++ b/zebra-rpc/src/methods.rs @@ -371,6 +371,7 @@ where Ok(response) } + // TODO: use a generic error constructor (#5548) #[allow(clippy::unwrap_in_result)] fn get_blockchain_info(&self) -> Result { let network = self.network; @@ -473,6 +474,7 @@ where Ok(response) } + // TODO: use a generic error constructor (#5548) fn get_address_balance( &self, address_strings: AddressStrings, @@ -499,6 +501,7 @@ where .boxed() } + // TODO: use HexData to handle transaction data, and a generic error constructor (#5548) fn send_raw_transaction( &self, raw_transaction_hex: String, @@ -553,6 +556,7 @@ where .boxed() } + // TODO: use a generic error constructor (#5548) fn get_block(&self, height: String, verbosity: u8) -> BoxFuture> { let mut state = self.state.clone(); @@ -621,6 +625,7 @@ where .boxed() } + // TODO: use a generic error constructor (#5548) fn get_best_block_hash(&self) -> Result { self.latest_chain_tip .best_tip_hash() @@ -632,6 +637,7 @@ where }) } + // TODO: use a generic error constructor (#5548) fn get_raw_mempool(&self) -> BoxFuture>> { let mut mempool = self.mempool.clone(); @@ -667,6 +673,7 @@ where .boxed() } + // TODO: use HexData to handle the transaction ID, and a generic error constructor (#5548) fn get_raw_transaction( &self, txid_hex: String, @@ -750,6 +757,7 @@ where .boxed() } + // TODO: use a generic error constructor (#5548) fn z_get_treestate(&self, hash_or_height: String) -> BoxFuture> { let mut state = self.state.clone(); @@ -864,6 +872,7 @@ where .boxed() } + // TODO: use a generic error constructor (#5548) fn get_address_tx_ids( &self, request: GetAddressTxIdsRequest, @@ -928,6 +937,7 @@ where .boxed() } + // TODO: use a generic error constructor (#5548) fn get_address_utxos( &self, address_strings: AddressStrings, diff --git a/zebra-rpc/src/methods/get_block_template_rpcs.rs b/zebra-rpc/src/methods/get_block_template_rpcs.rs index ad27f71a..ccf4b221 100644 --- a/zebra-rpc/src/methods/get_block_template_rpcs.rs +++ b/zebra-rpc/src/methods/get_block_template_rpcs.rs @@ -22,8 +22,8 @@ use zebra_chain::{ transparent, }; use zebra_consensus::{ - funding_stream_address, funding_stream_values, miner_subsidy, new_coinbase_script, BlockError, - VerifyBlockError, VerifyChainError, VerifyCheckpointError, MAX_BLOCK_SIGOPS, + funding_stream_address, funding_stream_values, miner_subsidy, new_coinbase_script, + VerifyChainError, MAX_BLOCK_SIGOPS, }; use zebra_node_services::mempool; @@ -261,6 +261,7 @@ where best_chain_tip_height(&self.latest_chain_tip).map(|height| height.0) } + // TODO: use a generic error constructor (#5548) fn get_block_hash(&self, index: i32) -> BoxFuture> { let mut state = self.state.clone(); let latest_chain_tip = self.latest_chain_tip.clone(); @@ -294,6 +295,7 @@ where .boxed() } + // TODO: use HexData to handle block proposal data, and a generic error constructor (#5548) fn get_block_template( &self, options: Option, @@ -498,13 +500,10 @@ where .map(|boxed_chain_error| *boxed_chain_error), }; - Ok(match chain_error { - Ok( - VerifyChainError::Checkpoint(VerifyCheckpointError::AlreadyVerified { .. }) - | VerifyChainError::Block(VerifyBlockError::Block { - source: BlockError::AlreadyInChain(..), - }), - ) => submit_block::ErrorResponse::Duplicate, + let response = match chain_error { + Ok(source) if source.is_duplicate_request() => { + submit_block::ErrorResponse::Duplicate + } // Currently, these match arms return Reject for the older duplicate in a queue, // but queued duplicates should be DuplicateInconclusive. @@ -526,8 +525,9 @@ where // This match arm is currently unreachable, but if future changes add extra error types, // we want to turn them into `Rejected`. Err(_unknown_error_type) => submit_block::ErrorResponse::Rejected, - } - .into()) + }; + + Ok(response.into()) } .boxed() } diff --git a/zebrad/src/components/mempool.rs b/zebrad/src/components/mempool.rs index d8821cba..6c271337 100644 --- a/zebrad/src/components/mempool.rs +++ b/zebrad/src/components/mempool.rs @@ -83,6 +83,8 @@ type InboundTxDownloads = TxDownloads, Timeout, St /// /// Indicates whether it is enabled or disabled and, if enabled, contains /// the necessary data to run it. +// +// Zebra only has one mempool, so the enum variant size difference doesn't matter. #[allow(clippy::large_enum_variant)] enum ActiveState { /// The Mempool is disabled. diff --git a/zebrad/src/components/sync.rs b/zebrad/src/components/sync.rs index 74fa378d..1a502945 100644 --- a/zebrad/src/components/sync.rs +++ b/zebrad/src/components/sync.rs @@ -19,9 +19,6 @@ use zebra_chain::{ chain_tip::ChainTip, parameters::genesis_hash, }; -use zebra_consensus::{ - chain::VerifyChainError, BlockError, VerifyBlockError, VerifyCheckpointError, -}; use zebra_network as zn; use zebra_state as zs; @@ -1008,7 +1005,6 @@ where /// /// See [`Self::handle_response`] for more details. #[allow(unknown_lints)] - #[allow(clippy::result_large_err)] fn handle_block_response( &mut self, response: Result<(Height, block::Hash), BlockDownloadVerifyError>, @@ -1027,7 +1023,6 @@ where /// /// See [`Self::handle_response`] for more details. #[allow(unknown_lints)] - #[allow(clippy::result_large_err)] fn handle_hash_response( response: Result, BlockDownloadVerifyError>, ) -> Result, BlockDownloadVerifyError> { @@ -1044,7 +1039,6 @@ where /// /// Returns `Err` if an unexpected error occurred, to force the synchronizer to restart. #[allow(unknown_lints)] - #[allow(clippy::result_large_err)] fn handle_response( response: Result, ) -> Result<(), BlockDownloadVerifyError> { @@ -1098,23 +1092,10 @@ where fn should_restart_sync(e: &BlockDownloadVerifyError) -> bool { match e { // Structural matches: downcasts - BlockDownloadVerifyError::Invalid { - error: VerifyChainError::Checkpoint(VerifyCheckpointError::AlreadyVerified { .. }), - .. - } => { + BlockDownloadVerifyError::Invalid { error, .. } if error.is_duplicate_request() => { debug!(error = ?e, "block was already verified, possibly from a previous sync run, continuing"); false } - BlockDownloadVerifyError::Invalid { - error: - VerifyChainError::Block(VerifyBlockError::Block { - source: BlockError::AlreadyInChain(_, _), - }), - .. - } => { - debug!(error = ?e, "block is already in chain, possibly from a previous sync run, continuing"); - false - } // Structural matches: direct BlockDownloadVerifyError::CancelledDuringDownload { .. } @@ -1140,12 +1121,15 @@ where } // String matches - BlockDownloadVerifyError::Invalid { - error: VerifyChainError::Block(VerifyBlockError::Commit(ref source)), - .. - } if format!("{source:?}").contains("block is already committed to the state") - || format!("{source:?}") - .contains("block has already been sent to be committed to the state") => + // + // We want to match VerifyChainError::Block(VerifyBlockError::Commit(ref source)), + // but that type is boxed. + // TODO: + // - turn this check into a function on VerifyChainError, like is_duplicate_request() + BlockDownloadVerifyError::Invalid { error, .. } + if format!("{error:?}").contains("block is already committed to the state") + || format!("{error:?}") + .contains("block has already been sent to be committed to the state") => { // TODO: improve this by checking the type (#2908) debug!(error = ?e, "block is already committed or pending a commit, possibly from a previous sync run, continuing");