Merge pull request #4006 from ProvableHQ/fix/sha-length

[Fix] Use array for handshake commit hashes

Author: kaimast

node/bft/events/src/challenge_request.rs

@@ -15,27 +15,23 @@
 
 use super::*;
 
-use snarkos_node_network::built_info;
+use snarkos_node_network::get_repo_commit_hash;
 
 #[derive(Clone, Debug, PartialEq, Eq)]
 pub struct ChallengeRequest<N: Network> {
     pub version: u32,
     pub listener_port: u16,
     pub address: Address<N>,
     pub nonce: u64,
-    pub snarkos_sha: String,
+    pub snarkos_sha: Option<[u8; 40]>,
 }
 
 impl<N: Network> ChallengeRequest<N> {
-    /// Creates a new `ChallengeRequest` event.
+    /// Constant for an unknown commit hash.
+    const UNKNOWN_COMMIT_HASH: [u8; 40] = [b'?'; 40];
+
     pub fn new(listener_port: u16, address: Address<N>, nonce: u64) -> Self {
-        Self {
-            version: Event::<N>::VERSION,
-            listener_port,
-            address,
-            nonce,
-            snarkos_sha: built_info::GIT_COMMIT_HASH.unwrap_or_default().into(),
-        }
+        Self { version: Event::<N>::VERSION, listener_port, address, nonce, snarkos_sha: get_repo_commit_hash() }
     }
 }
 
@@ -48,35 +44,38 @@ impl<N: Network> EventTrait for ChallengeRequest<N> {
 }
 
 impl<N: Network> ToBytes for ChallengeRequest<N> {
-    fn write_le<W: Write>(&self, mut writer: W) -> IoResult<()> {
+    fn write_le<W: Write>(&self, mut writer: W) -> io::Result<()> {
         self.version.write_le(&mut writer)?;
         self.listener_port.write_le(&mut writer)?;
         self.address.write_le(&mut writer)?;
         self.nonce.write_le(&mut writer)?;
-        self.snarkos_sha.as_bytes().write_le(&mut writer)?;
+        // Serialize `None` as a constant.
+        self.snarkos_sha.unwrap_or(Self::UNKNOWN_COMMIT_HASH).write_le(&mut writer)?;
 
         Ok(())
     }
 }
 
 impl<N: Network> FromBytes for ChallengeRequest<N> {
-    fn read_le<R: Read>(mut reader: R) -> IoResult<Self> {
+    fn read_le<R: Read>(mut reader: R) -> io::Result<Self> {
         let version = u32::read_le(&mut reader)?;
         let listener_port = u16::read_le(&mut reader)?;
         let address = Address::<N>::read_le(&mut reader)?;
         let nonce = u64::read_le(&mut reader)?;
-        let snarkos_sha = str::from_utf8(&<[u8; 40]>::read_le(&mut reader).unwrap_or([b'?'; 40])[..])
-            .map(|str| if str.starts_with('?') { "unknown" } else { str })
-            .map_err(|_| error("Invalid snarkOS SHA"))?
-            .to_owned();
+        let snarkos_sha = {
+            let bytes =
+                <[u8; 40]>::read_le(&mut reader).map_err(|err| io_error(format!("Invalid snarkOS SHA - {err}")))?;
+            if bytes == Self::UNKNOWN_COMMIT_HASH { None } else { Some(bytes) }
+        };
 
         Ok(Self { version, listener_port, address, nonce, snarkos_sha })
     }
 }
 
 #[cfg(test)]
 pub mod prop_tests {
-    use crate::ChallengeRequest;
+    use super::*;
+
     use snarkvm::{
         console::prelude::{FromBytes, ToBytes},
         prelude::{Address, TestRng, Uniform},
@@ -97,12 +96,11 @@ pub mod prop_tests {
 
     pub fn any_challenge_request() -> BoxedStrategy<ChallengeRequest<CurrentNetwork>> {
         (any_valid_address(), any::<u64>(), any::<u32>(), any::<u16>(), collection::vec(0u8..=127, 40))
-            .prop_map(|(address, nonce, version, listener_port, sha)| ChallengeRequest {
-                address,
-                nonce,
-                version,
-                listener_port,
-                snarkos_sha: sha.into_iter().map(|b| b as char).collect(),
+            .prop_map(|(address, nonce, version, listener_port, sha)| {
+                let sha: [u8; 40] = sha.try_into().unwrap();
+                let snarkos_sha =
+                    if sha == ChallengeRequest::<CurrentNetwork>::UNKNOWN_COMMIT_HASH { None } else { Some(sha) };
+                ChallengeRequest { address, nonce, version, listener_port, snarkos_sha }
             })
             .boxed()
     }
@@ -112,12 +110,8 @@ pub mod prop_tests {
         let mut buf = BytesMut::default().writer();
         ChallengeRequest::write_le(&original, &mut buf).unwrap();
 
-        let mut deserialized: ChallengeRequest<CurrentNetwork> =
+        let deserialized: ChallengeRequest<CurrentNetwork> =
             ChallengeRequest::read_le(buf.into_inner().reader()).unwrap();
-        // Upon deserialization, unsupplied SHA is registered as "unknown".
-        if deserialized.snarkos_sha == "unknown" {
-            deserialized.snarkos_sha = original.snarkos_sha.clone();
-        }
         assert_eq!(original, deserialized);
     }
 }

node/bft/events/src/lib.rs

@@ -68,7 +68,7 @@ pub use worker_ping::WorkerPing;
 
 use snarkos_node_sync_locators::BlockLocators;
 use snarkvm::{
-    console::prelude::{FromBytes, Network, Read, ToBytes, Write, error},
+    console::prelude::{FromBytes, Network, Read, ToBytes, Write, error, io_error},
     ledger::{
         block::Block,
         narwhal::{BatchCertificate, BatchHeader, Data, Transmission, TransmissionID},

node/bft/tests/gateway_e2e.rs

@@ -110,7 +110,7 @@ async fn handshake_responder_side_invalid_challenge_request() {
     let listener_port = test_peer.listening_addr().port();
     let address = accounts.get(1).unwrap().address();
     let nonce = rng.r#gen();
-    let snarkos_sha = String::new();
+    let snarkos_sha = None;
     // Set the wrong version so the challenge request is invalid.
     let challenge_request = ChallengeRequest { version: 0, listener_port, address, nonce, snarkos_sha };
 
@@ -150,7 +150,7 @@ async fn handshake_responder_side_invalid_challenge_response() {
     let listener_port = test_peer.listening_addr().port();
     let address = accounts.get(1).unwrap().address();
     let our_nonce = rng.r#gen();
-    let snarkos_sha = String::new();
+    let snarkos_sha = None;
     let version = Event::<CurrentNetwork>::VERSION;
     let challenge_request = ChallengeRequest { version, listener_port, address, nonce: our_nonce, snarkos_sha };
 

node/network/src/lib.rs

@@ -79,17 +79,31 @@ pub fn bootstrap_peers<N: Network>(is_dev: bool) -> Vec<SocketAddr> {
     }
 }
 
+/// Get our SHA from the build information (or None if it is not set or does not 40 bytes long).
+pub fn get_repo_commit_hash() -> Option<[u8; 40]> {
+    built_info::GIT_COMMIT_HASH.and_then(|sha| sha.as_bytes().try_into().ok())
+}
+
 /// Logs the peer's snarkOS repo SHA and how it compares to ours.
-pub fn log_repo_sha_comparison(peer_addr: SocketAddr, peer_sha: &str, ctx: &str) {
-    let our_sha = built_info::GIT_COMMIT_HASH.unwrap_or_default();
-    let sha_cmp = if peer_sha == "unknown" {
-        " with an unknown repo SHA".to_owned()
-    } else if peer_sha == our_sha {
-        format!("@{peer_sha} (same as us)")
-    } else if our_sha.is_empty() {
-        format!("@{peer_sha} (potentially different than us)")
-    } else {
-        format!("@{peer_sha} (different than us)")
+pub fn log_repo_sha_comparison(peer_addr: SocketAddr, peer_sha: &Option<[u8; 40]>, ctx: &str) {
+    let our_sha = get_repo_commit_hash();
+
+    // Generate a string representation for the peers hash.
+    let peer_sha_str: Option<&str> = peer_sha.as_ref().and_then(|h| str::from_utf8(h).ok());
+
+    let sha_cmp = match (&our_sha, peer_sha, peer_sha_str) {
+        // They sent no hash, or an invalid string.
+        (_, _, None) | (_, None, _) => " with an unknown repo SHA".to_owned(),
+        // Our hash cannot be retrieved.
+        (None, _, Some(theirs_str)) => format!("@{theirs_str} (potentially different than us)"),
+        // Both hashes are valid. Compare.
+        (Some(ours), Some(theirs), Some(theirs_str)) => {
+            if ours == theirs {
+                format!("@{theirs_str} (same as us)")
+            } else {
+                format!("@{theirs_str} (different than us)")
+            }
+        }
     };
 
     debug!("{ctx} Peer '{peer_addr}' uses snarkOS{sha_cmp}");

node/router/messages/src/challenge_request.rs

@@ -15,8 +15,8 @@
 
 use super::*;
 
-use snarkos_node_network::{NodeType, built_info};
-use snarkvm::prelude::{FromBytes, ToBytes};
+use snarkos_node_network::{NodeType, get_repo_commit_hash};
+use snarkvm::prelude::{FromBytes, ToBytes, io_error};
 
 use std::borrow::Cow;
 
@@ -27,7 +27,7 @@ pub struct ChallengeRequest<N: Network> {
     pub node_type: NodeType,
     pub address: Address<N>,
     pub nonce: u64,
-    pub snarkos_sha: String,
+    pub snarkos_sha: Option<[u8; 40]>,
 }
 
 impl<N: Network> MessageTrait for ChallengeRequest<N> {
@@ -45,7 +45,7 @@ impl<N: Network> ToBytes for ChallengeRequest<N> {
         self.node_type.write_le(&mut writer)?;
         self.address.write_le(&mut writer)?;
         self.nonce.write_le(&mut writer)?;
-        self.snarkos_sha.as_bytes().write_le(&mut writer)?;
+        self.snarkos_sha.unwrap_or(Self::UNKNOWN_COMMIT_HASH).write_le(&mut writer)?;
 
         Ok(())
     }
@@ -58,32 +58,37 @@ impl<N: Network> FromBytes for ChallengeRequest<N> {
         let node_type = NodeType::read_le(&mut reader)?;
         let address = Address::<N>::read_le(&mut reader)?;
         let nonce = u64::read_le(&mut reader)?;
-        let snarkos_sha = str::from_utf8(&<[u8; 40]>::read_le(&mut reader).unwrap_or([b'?'; 40])[..])
-            .map(|str| if str.starts_with('?') { "unknown" } else { str })
-            .map_err(|_| error("Invalid snarkOS SHA"))?
-            .to_owned();
+
+        let snarkos_sha = {
+            let bytes =
+                <[u8; 40]>::read_le(&mut reader).map_err(|err| io_error(format!("Invalid snarkOS SHA - {err}")))?;
+            if bytes == Self::UNKNOWN_COMMIT_HASH { None } else { Some(bytes) }
+        };
 
         Ok(Self { version, listener_port, node_type, address, nonce, snarkos_sha })
     }
 }
 
 impl<N: Network> ChallengeRequest<N> {
+    /// Constant for an unknown commit hash.
+    const UNKNOWN_COMMIT_HASH: [u8; 40] = [b'?'; 40];
+
     pub fn new(listener_port: u16, node_type: NodeType, address: Address<N>, nonce: u64) -> Self {
         Self {
             version: Message::<N>::latest_message_version(),
             listener_port,
             node_type,
             address,
             nonce,
-            snarkos_sha: built_info::GIT_COMMIT_HASH.unwrap_or_default().into(),
+            snarkos_sha: get_repo_commit_hash(),
         }
     }
 }
 
 #[cfg(test)]
 pub mod prop_tests {
-    use crate::ChallengeRequest;
-    use snarkos_node_network::NodeType;
+    use super::*;
+
     use snarkvm::{
         console::prelude::{FromBytes, ToBytes},
         prelude::{Address, TestRng, Uniform},
@@ -115,13 +120,11 @@ pub mod prop_tests {
 
     pub fn any_challenge_request() -> BoxedStrategy<ChallengeRequest<CurrentNetwork>> {
         (any_valid_address(), any::<u64>(), any::<u32>(), any::<u16>(), any_node_type(), collection::vec(0u8..=127, 40))
-            .prop_map(|(address, nonce, version, listener_port, node_type, sha)| ChallengeRequest {
-                address,
-                nonce,
-                version,
-                listener_port,
-                node_type,
-                snarkos_sha: sha.into_iter().map(|b| b as char).collect(),
+            .prop_map(|(address, nonce, version, listener_port, node_type, sha)| {
+                let sha: [u8; 40] = sha.try_into().unwrap();
+                let snarkos_sha =
+                    if sha == ChallengeRequest::<CurrentNetwork>::UNKNOWN_COMMIT_HASH { None } else { Some(sha) };
+                ChallengeRequest { address, nonce, version, listener_port, node_type, snarkos_sha }
             })
             .boxed()
     }
@@ -131,12 +134,8 @@ pub mod prop_tests {
         let mut buf = BytesMut::default().writer();
         ChallengeRequest::write_le(&original, &mut buf).unwrap();
 
-        let mut deserialized: ChallengeRequest<CurrentNetwork> =
+        let deserialized: ChallengeRequest<CurrentNetwork> =
             ChallengeRequest::read_le(buf.into_inner().reader()).unwrap();
-        // Upon deserialization, unsupplied SHA is registered as "unknown".
-        if deserialized.snarkos_sha == "unknown" {
-            deserialized.snarkos_sha = original.snarkos_sha.clone();
-        }
         assert_eq!(original, deserialized);
     }
 }