From 9f9c12346b34992dc903c0ab797a1f61d17a7e06 Mon Sep 17 00:00:00 2001 From: Quantum Explorer Date: Wed, 20 Nov 2024 21:55:28 +0100 Subject: [PATCH] fixes for coderabbit --- packages/rs-dpp/src/bls/native_bls.rs | 48 ++++++++++++------- .../src/core_types/validator_set/v0/mod.rs | 18 +++---- packages/rs-dpp/src/errors/protocol_error.rs | 10 ++++ packages/rs-dpp/src/signing.rs | 16 +++++-- .../update_operator_identity/v0/mod.rs | 12 ++--- .../platform_state/v0/old_structures/mod.rs | 4 +- .../signature_verification_quorum_set/mod.rs | 2 +- .../v0/for_saving.rs | 8 ++-- .../v0/for_saving_v1.rs | 9 ++-- .../masternode_list_item_helpers.rs | 8 ++-- .../tests/strategy_tests/masternodes.rs | 8 ++-- packages/simple-signer/src/signer.rs | 2 +- 12 files changed, 90 insertions(+), 55 deletions(-) diff --git a/packages/rs-dpp/src/bls/native_bls.rs b/packages/rs-dpp/src/bls/native_bls.rs index e8e1fd939f..492f563511 100644 --- a/packages/rs-dpp/src/bls/native_bls.rs +++ b/packages/rs-dpp/src/bls/native_bls.rs @@ -2,7 +2,7 @@ use crate::bls_signatures::{ Bls12381G2Impl, Pairing, PublicKey, SecretKey, Signature, SignatureSchemes, }; use crate::{BlsModule, ProtocolError, PublicKeyValidationError}; -use anyhow::anyhow; +use std::array::TryFromSliceError; #[derive(Default)] pub struct NativeBlsModule; @@ -20,43 +20,57 @@ impl BlsModule for NativeBlsModule { data: &[u8], public_key: &[u8], ) -> Result { - let public_key = - PublicKey::::try_from(public_key).map_err(anyhow::Error::msg)?; - let signature_96_bytes = signature - .try_into() - .map_err(|_| anyhow!("signature wrong size"))?; - let g2_element = + let public_key = PublicKey::::try_from(public_key)?; + let signature_96_bytes = + signature + .try_into() + .map_err(|_| ProtocolError::BlsSignatureSizeError { + got: signature.len() as u32, + })?; + let Some(g2_element) = ::Signature::from_compressed(&signature_96_bytes) .into_option() - .ok_or(anyhow!("signature derivation failed"))?; + else { + return Ok(false); // We should not error because the signature could be given by an invalid source + }; let signature = Signature::Basic(g2_element); match signature.verify(&public_key, data) { Ok(_) => Ok(true), - Err(_) => Err(anyhow!("Verification failed").into()), + Err(_) => Ok(false), } } fn private_key_to_public_key(&self, private_key: &[u8]) -> Result, ProtocolError> { - let fixed_len_key: [u8; 32] = private_key - .try_into() - .map_err(|_| anyhow!("the BLS private key must be 32 bytes long"))?; + let fixed_len_key: [u8; 32] = + private_key + .try_into() + .map_err(|_| ProtocolError::PrivateKeySizeError { + got: private_key.len() as u32, + })?; let pk = SecretKey::::from_be_bytes(&fixed_len_key) .into_option() - .ok_or(anyhow!("Incorrect Priv Key"))?; + .ok_or(ProtocolError::InvalidBLSPrivateKeyError( + "key not valid".to_string(), + ))?; let public_key = pk.public_key(); let public_key_bytes = public_key.0.to_compressed().to_vec(); Ok(public_key_bytes) } fn sign(&self, data: &[u8], private_key: &[u8]) -> Result, ProtocolError> { - let fixed_len_key: [u8; 32] = private_key - .try_into() - .map_err(|_| anyhow!("the BLS private key must be 32 bytes long"))?; + let fixed_len_key: [u8; 32] = + private_key + .try_into() + .map_err(|_| ProtocolError::PrivateKeySizeError { + got: private_key.len() as u32, + })?; let pk = SecretKey::::from_be_bytes(&fixed_len_key) .into_option() - .ok_or(anyhow!("Incorrect Priv Key"))?; + .ok_or(ProtocolError::InvalidBLSPrivateKeyError( + "key not valid".to_string(), + ))?; Ok(pk .sign(SignatureSchemes::Basic, data)? .as_raw_value() diff --git a/packages/rs-dpp/src/core_types/validator_set/v0/mod.rs b/packages/rs-dpp/src/core_types/validator_set/v0/mod.rs index d500fb0426..4789d53dd6 100644 --- a/packages/rs-dpp/src/core_types/validator_set/v0/mod.rs +++ b/packages/rs-dpp/src/core_types/validator_set/v0/mod.rs @@ -120,10 +120,11 @@ impl Decode for ValidatorSetV0 { let bytes = <[u8; 48]>::decode(decoder)?; public_key_bytes.copy_from_slice(&bytes); let threshold_public_key = - BlsPublicKey::try_from(public_key_bytes.as_slice()).map_err(|_| { - bincode::error::DecodeError::OtherString( - "Failed to decode BlsPublicKey".to_string(), - ) + BlsPublicKey::try_from(public_key_bytes.as_slice()).map_err(|e| { + bincode::error::DecodeError::OtherString(format!( + "Failed to decode BlsPublicKey: {}", + e + )) })?; Ok(ValidatorSetV0 { @@ -167,10 +168,11 @@ impl<'de> BorrowDecode<'de> for ValidatorSetV0 { let bytes = <[u8; 48]>::decode(decoder)?; public_key_bytes.copy_from_slice(&bytes); let threshold_public_key = - BlsPublicKey::try_from(public_key_bytes.as_slice()).map_err(|_| { - bincode::error::DecodeError::OtherString( - "Failed to decode BlsPublicKey in borrow decode".to_string(), - ) + BlsPublicKey::try_from(public_key_bytes.as_slice()).map_err(|e| { + bincode::error::DecodeError::OtherString(format!( + "Failed to decode BlsPublicKey in borrow decode: {}", + e + )) })?; Ok(ValidatorSetV0 { diff --git a/packages/rs-dpp/src/errors/protocol_error.rs b/packages/rs-dpp/src/errors/protocol_error.rs index 0d4f6cae6a..1ed9888821 100644 --- a/packages/rs-dpp/src/errors/protocol_error.rs +++ b/packages/rs-dpp/src/errors/protocol_error.rs @@ -1,3 +1,4 @@ +use std::array::TryFromSliceError; use thiserror::Error; use crate::consensus::basic::state_transition::InvalidStateTransitionTypeError; @@ -252,6 +253,15 @@ pub enum ProtocolError { #[cfg(feature = "bls-signatures")] #[error(transparent)] BlsError(#[from] dashcore::blsful::BlsError), + + #[error("Private key wrong size: expected 32, got {got}")] + PrivateKeySizeError { got: u32 }, + + #[error("Private key invalid error: {0}")] + InvalidBLSPrivateKeyError(String), + + #[error("Signature wrong size: expected 96, got {got}")] + BlsSignatureSizeError { got: u32 }, } impl From<&str> for ProtocolError { diff --git a/packages/rs-dpp/src/signing.rs b/packages/rs-dpp/src/signing.rs index 1986cb16ea..d637c0480b 100644 --- a/packages/rs-dpp/src/signing.rs +++ b/packages/rs-dpp/src/signing.rs @@ -55,7 +55,7 @@ impl PlatformMessageSignable for &[u8] { ); } }; - let signature_bytes: [u8; 96] = match signature.to_vec().try_into() { + let signature_bytes: [u8; 96] = match signature.try_into() { Ok(bytes) => bytes, Err(_) => { return SimpleConsensusValidationResult::new_with_error( @@ -67,8 +67,18 @@ impl PlatformMessageSignable for &[u8] { ) } }; - let g2 = ::Signature::from_compressed(&signature_bytes) - .expect("G2 projective"); + let g2 = match ::Signature::from_compressed( + &signature_bytes, + ) + .into_option() + { + Some(g2) => g2, + None => { + return SimpleConsensusValidationResult::new_with_error( + SignatureError::BasicBLSError(BasicBLSError::new("bls signature does not conform to proper bls signature serialization".to_string())).into(), + ); + } + }; let signature = Signature::::Basic(g2); if signature.verify(&public_key, signable_data).is_err() { diff --git a/packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_identities/update_operator_identity/v0/mod.rs b/packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_identities/update_operator_identity/v0/mod.rs index 1b3e5ef3dd..7c901380b0 100644 --- a/packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_identities/update_operator_identity/v0/mod.rs +++ b/packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_identities/update_operator_identity/v0/mod.rs @@ -438,9 +438,9 @@ mod tests { .to_bytes() .to_vec(); let private_key_operator = BlsPrivateKey::::from_be_bytes( - &private_key_operator_bytes.try_into().unwrap(), + &private_key_operator_bytes.try_into().expect("expected the secret key to be 32 bytes"), ) - .unwrap(); + .expect("expected the conversion between bls signatures library and blsful to happen without failing"); let pub_key_operator = private_key_operator.public_key().0.to_compressed().to_vec(); let operator_key: IdentityPublicKey = IdentityPublicKeyV0 { @@ -957,9 +957,9 @@ mod tests { .to_bytes() .to_vec(); let private_key_operator = BlsPrivateKey::::from_be_bytes( - &private_key_operator_bytes.try_into().unwrap(), + &private_key_operator_bytes.try_into().expect("expected the secret key to be 32 bytes"), ) - .unwrap(); + .expect("expected the conversion between bls signatures library and blsful to happen without failing"); let new_pub_key_operator = private_key_operator.public_key().0.to_compressed().to_vec(); // Create an old masternode state @@ -1049,9 +1049,9 @@ mod tests { .to_bytes() .to_vec(); let private_key_operator = BlsPrivateKey::::from_be_bytes( - &private_key_operator_bytes.try_into().unwrap(), + &private_key_operator_bytes.try_into().expect("expected the secret key to be 32 bytes"), ) - .unwrap(); + .expect("expected the conversion between bls signatures library and blsful to happen without failing"); let new_pub_key_operator = private_key_operator.public_key().0.to_compressed().to_vec(); // Create an old masternode state with original public key operator diff --git a/packages/rs-drive-abci/src/platform_types/platform_state/v0/old_structures/mod.rs b/packages/rs-drive-abci/src/platform_types/platform_state/v0/old_structures/mod.rs index d64e1e7723..ebd0e85875 100644 --- a/packages/rs-drive-abci/src/platform_types/platform_state/v0/old_structures/mod.rs +++ b/packages/rs-drive-abci/src/platform_types/platform_state/v0/old_structures/mod.rs @@ -51,7 +51,7 @@ impl From for dpp::core_types::validator_set::v0::ValidatorSetV0 .map(|(pro_tx_hash, validator)| (pro_tx_hash, validator.into())) .collect(), threshold_public_key: PublicKey::try_from(threshold_public_key.to_bytes().as_slice()) - .unwrap(), + .expect("this should not be possible to error as the threshold_public_key was already verified on disk"), } } } @@ -91,7 +91,7 @@ impl From for dpp::core_types::validator::v0::ValidatorV0 { } = value; Self { pro_tx_hash, - public_key: public_key.map(|pk| PublicKey::try_from(pk.to_bytes().as_slice()).unwrap()), + public_key: public_key.map(|pk| PublicKey::try_from(pk.to_bytes().as_slice()).expect("this should not be possible to error as the public_key was already verified on disk")), node_ip, node_id, core_port, diff --git a/packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/mod.rs b/packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/mod.rs index 38c00188ec..a314829a7d 100644 --- a/packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/mod.rs +++ b/packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/mod.rs @@ -119,7 +119,7 @@ impl SignatureVerificationQuorumSetV0Methods for SignatureVerificationQuorumSet pub enum SignatureVerificationQuorumSetForSaving { /// Version 0 of the signature verification quorums V0(SignatureVerificationQuorumSetForSavingV0), - /// Version 0 of the signature verification quorums + /// Version 1 of the signature verification quorums V1(SignatureVerificationQuorumSetForSavingV1), } diff --git a/packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/v0/for_saving.rs b/packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/v0/for_saving.rs index 1c6a07db91..de69aff56d 100644 --- a/packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/v0/for_saving.rs +++ b/packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/v0/for_saving.rs @@ -156,10 +156,10 @@ impl From> for Quorums { } } -#[allow(clippy::from_over_into)] -impl Into> for Quorums { - fn into(self) -> Vec { - self.into_iter() +impl From> for Vec { + fn from(quorums: Quorums) -> Self { + quorums + .into_iter() .map(|(hash, quorum)| QuorumForSavingV0 { hash: Bytes32::from(hash.as_byte_array()), public_key: bls_signatures::PublicKey::from_bytes( diff --git a/packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/v0/for_saving_v1.rs b/packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/v0/for_saving_v1.rs index c7e9bbaf10..bfb596e451 100644 --- a/packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/v0/for_saving_v1.rs +++ b/packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/v0/for_saving_v1.rs @@ -78,11 +78,10 @@ impl From> for Quorums { })) } } - -#[allow(clippy::from_over_into)] -impl Into> for Quorums { - fn into(self) -> Vec { - self.into_iter() +impl From> for Vec { + fn from(quorums: Quorums) -> Self { + quorums + .into_iter() .map(|(hash, quorum)| QuorumForSavingV1 { hash: Bytes32::from(hash.as_byte_array()), public_key: quorum.public_key, diff --git a/packages/rs-drive-abci/tests/strategy_tests/masternode_list_item_helpers.rs b/packages/rs-drive-abci/tests/strategy_tests/masternode_list_item_helpers.rs index bdd1ecc911..4084e7f065 100644 --- a/packages/rs-drive-abci/tests/strategy_tests/masternode_list_item_helpers.rs +++ b/packages/rs-drive-abci/tests/strategy_tests/masternode_list_item_helpers.rs @@ -38,9 +38,9 @@ impl UpdateMasternodeListItem for MasternodeListItem { .to_bytes() .to_vec(); let private_key_operator = BlsPrivateKey::::from_be_bytes( - &private_key_operator_bytes.try_into().unwrap(), + &private_key_operator_bytes.try_into().expect("expected the secret key to be 32 bytes"), ) - .unwrap(); + .expect("expected the conversion between bls signatures library and blsful to happen without failing"); let pub_key_operator = private_key_operator.public_key().0.to_compressed().to_vec(); self.state.pub_key_operator = pub_key_operator; @@ -92,9 +92,9 @@ mod tests { .to_bytes() .to_vec(); let private_key_operator = BlsPrivateKey::::from_be_bytes( - &private_key_operator_bytes.try_into().unwrap(), + &private_key_operator_bytes.try_into().expect("expected the secret key to be 32 bytes"), ) - .unwrap(); + .expect("expected the conversion between bls signatures library and blsful to happen without failing"); let pub_key_operator = private_key_operator.public_key().0.to_compressed().to_vec(); let masternode_list_item = MasternodeListItem { node_type: MasternodeType::Regular, diff --git a/packages/rs-drive-abci/tests/strategy_tests/masternodes.rs b/packages/rs-drive-abci/tests/strategy_tests/masternodes.rs index 89a9697d44..02a647b66c 100644 --- a/packages/rs-drive-abci/tests/strategy_tests/masternodes.rs +++ b/packages/rs-drive-abci/tests/strategy_tests/masternodes.rs @@ -214,9 +214,9 @@ pub fn generate_test_masternodes( .to_bytes() .to_vec(); let private_key_operator = BlsPrivateKey::::from_be_bytes( - &private_key_operator_bytes.try_into().unwrap(), + &private_key_operator_bytes.try_into().expect("expected the secret key to be 32 bytes"), ) - .unwrap(); + .expect("expected the conversion between bls signatures library and blsful to happen without failing"); let pub_key_operator = private_key_operator.public_key().0.to_compressed().to_vec(); let pro_tx_hash = ProTxHash::from_byte_array(rng.gen::<[u8; 32]>()); let masternode_list_item = MasternodeListItem { @@ -352,9 +352,9 @@ pub fn generate_test_masternodes( .to_bytes() .to_vec(); let private_key_operator = BlsPrivateKey::::from_be_bytes( - &private_key_operator_bytes.try_into().unwrap(), + &private_key_operator_bytes.try_into().expect("expected the secret key to be 32 bytes"), ) - .unwrap(); + .expect("expected the conversion between bls signatures library and blsful to happen without failing"); let pub_key_operator = private_key_operator.public_key().0.to_compressed().to_vec(); let masternode_list_item = MasternodeListItem { node_type: MasternodeType::Evo, diff --git a/packages/simple-signer/src/signer.rs b/packages/simple-signer/src/signer.rs index 04a9b325d3..d93acb5aee 100644 --- a/packages/simple-signer/src/signer.rs +++ b/packages/simple-signer/src/signer.rs @@ -89,7 +89,7 @@ impl Signer for SimpleSigner { ))?; let signature = pk .sign(SignatureSchemes::Basic, data) - .map_err(|e| ProtocolError::Generic(format!("unable to sign {}", e)))?; + .map_err(|e| ProtocolError::Generic(format!("BLS signing failed {}", e)))?; Ok(signature.as_raw_value().to_compressed().to_vec().into()) } KeyType::EDDSA_25519_HASH160 => {