fixes for coderabbit

dashpay · Nov 20, 2024 · 9f9c123 · 9f9c123
1 parent f745857
commit 9f9c123
Show file tree

Hide file tree

Showing 12 changed files with 90 additions and 55 deletions.
diff --git 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<bool, ProtocolError> {
-        let public_key =
-            PublicKey::<Bls12381G2Impl>::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::<Bls12381G2Impl>::try_from(public_key)?;
+        let signature_96_bytes =
+            signature
+                .try_into()
+                .map_err(|_| ProtocolError::BlsSignatureSizeError {
+                    got: signature.len() as u32,
+                })?;
+        let Some(g2_element) =
             <Bls12381G2Impl as Pairing>::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<Vec<u8>, 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::<Bls12381G2Impl>::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<Vec<u8>, 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::<Bls12381G2Impl>::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
@@ -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
@@ -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
@@ -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 = <Bls12381G2Impl as Pairing>::Signature::from_compressed(&signature_bytes)
-                    .expect("G2 projective");
+                let g2 = match <Bls12381G2Impl as Pairing>::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::<Bls12381G2Impl>::Basic(g2);
 
                 if signature.verify(&public_key, signable_data).is_err() {

diff --git a/...events/core_based_updates/update_masternode_identities/update_operator_identity/v0/mod.rs b/...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::<Bls12381G2Impl>::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::<Bls12381G2Impl>::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::<Bls12381G2Impl>::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
@@ -51,7 +51,7 @@ impl From<ValidatorSetV0> 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<ValidatorV0> 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
@@ -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
@@ -156,10 +156,10 @@ impl From<Vec<QuorumForSavingV0>> for Quorums<VerificationQuorum> {
     }
 }
 
-#[allow(clippy::from_over_into)]
-impl Into<Vec<QuorumForSavingV0>> for Quorums<VerificationQuorum> {
-    fn into(self) -> Vec<QuorumForSavingV0> {
-        self.into_iter()
+impl From<Quorums<VerificationQuorum>> for Vec<QuorumForSavingV0> {
+    fn from(quorums: Quorums<VerificationQuorum>) -> 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/...es/rs-drive-abci/src/platform_types/signature_verification_quorum_set/v0/for_saving_v1.rs b/...es/rs-drive-abci/src/platform_types/signature_verification_quorum_set/v0/for_saving_v1.rs
@@ -78,11 +78,10 @@ impl From<Vec<QuorumForSavingV1>> for Quorums<VerificationQuorum> {
         }))
     }
 }
-
-#[allow(clippy::from_over_into)]
-impl Into<Vec<QuorumForSavingV1>> for Quorums<VerificationQuorum> {
-    fn into(self) -> Vec<QuorumForSavingV1> {
-        self.into_iter()
+impl From<Quorums<VerificationQuorum>> for Vec<QuorumForSavingV1> {
+    fn from(quorums: Quorums<VerificationQuorum>) -> 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
@@ -38,9 +38,9 @@ impl UpdateMasternodeListItem for MasternodeListItem {
                         .to_bytes()
                         .to_vec();
                     let private_key_operator = BlsPrivateKey::<Bls12381G2Impl>::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::<Bls12381G2Impl>::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
@@ -214,9 +214,9 @@ pub fn generate_test_masternodes(
             .to_bytes()
             .to_vec();
         let private_key_operator = BlsPrivateKey::<Bls12381G2Impl>::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::<Bls12381G2Impl>::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
@@ -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 => {