refactor(dht): use CipherKey new type for diffie-hellman key (#4038)

Description
---
- Document the DHT public interface
- Move forward layer to `outbound` module
- Remove unnecessary allocation from `encrypt` helper
- Return `CipherKey` new type from Diffie-Hellman helper function 
- Changes `encrypt` and `decrypt` to take in `CipherKey` newtype
- implement zeroize on drop for `CipherKey` new type

Motivation and Context
---
Documentation. 

`generate_ecdh_secret` returned a public key, which implies that it is safe to share publicly.
The `CipherKey` new type will zero it's contents before releasing it's memory.

The forward layer didn't have anything to do with SAF so shouldn't be located in that module.

How Has This Been Tested?
---
Existing tests pass
Manually, no breaking changes.

cargo doc --no-deps

base_layer/p2p/src/initialization.rs

@@ -323,8 +323,6 @@ async fn configure_comms_and_dht(
     }
 
     // Hook up DHT messaging middlewares
-    // TODO: messaging events should be optional
-    let (messaging_events_sender, _) = broadcast::channel(1);
     let messaging_pipeline = pipeline::Builder::new()
         .outbound_buffer_size(config.outbound_buffer_size)
         .with_outbound_pipeline(outbound_rx, |sink| {
@@ -339,6 +337,8 @@ async fn configure_comms_and_dht(
         )
         .build();
 
+    // TODO: messaging events should be optional
+    let (messaging_events_sender, _) = broadcast::channel(1);
     comms = comms.add_protocol_extension(MessagingProtocolExtension::new(
         messaging_events_sender,
         messaging_pipeline,

comms/dht/Cargo.toml

@@ -37,6 +37,7 @@ serde = "1.0.90"
 serde_derive = "1.0.90"
 thiserror = "1.0.26"
 tower = { version = "0.4", features = ["full"] }
+zeroize = "1.4.0"
 
 # Uncomment for tokio tracing via tokio-console (needs "tracing" features)
 #console-subscriber = "0.1.3"

comms/dht/src/actor.rs

@@ -63,6 +63,7 @@ use crate::{
 
 const LOG_TARGET: &str = "comms::dht::actor";
 
+/// Error type for the DHT actor
 #[derive(Debug, Error)]
 pub enum DhtActorError {
     #[error("MPSC channel is disconnected")]
@@ -93,6 +94,7 @@ impl<T> From<mpsc::error::SendError<T>> for DhtActorError {
     }
 }
 
+/// Request type for the DHT actor
 #[derive(Debug)]
 #[allow(clippy::large_enum_variant)]
 pub enum DhtRequest {
@@ -143,20 +145,23 @@ impl Display for DhtRequest {
     }
 }
 
+/// DHT actor requester
 #[derive(Clone)]
 pub struct DhtRequester {
     sender: mpsc::Sender<DhtRequest>,
 }
 
 impl DhtRequester {
-    pub fn new(sender: mpsc::Sender<DhtRequest>) -> Self {
+    pub(crate) fn new(sender: mpsc::Sender<DhtRequest>) -> Self {
         Self { sender }
     }
 
+    /// Send a Join message to the network
     pub async fn send_join(&mut self) -> Result<(), DhtActorError> {
         self.sender.send(DhtRequest::SendJoin).await.map_err(Into::into)
     }
 
+    /// Select peers by [BroadcastStrategy](crate::broadcast_strategy::BroadcastStrategy]
     pub async fn select_peers(&mut self, broadcast_strategy: BroadcastStrategy) -> Result<Vec<NodeId>, DhtActorError> {
         let (reply_tx, reply_rx) = oneshot::channel();
         self.sender
@@ -165,6 +170,7 @@ impl DhtRequester {
         reply_rx.await.map_err(|_| DhtActorError::ReplyCanceled)
     }
 
+    /// Adds a message hash to the dedup cache.
     pub async fn add_message_to_dedup_cache(
         &mut self,
         message_hash: Vec<u8>,
@@ -182,6 +188,7 @@ impl DhtRequester {
         reply_rx.await.map_err(|_| DhtActorError::ReplyCanceled)
     }
 
+    /// Gets the number of hits for a given message hash.
     pub async fn get_message_cache_hit_count(&mut self, message_hash: Vec<u8>) -> Result<u32, DhtActorError> {
         let (reply_tx, reply_rx) = oneshot::channel();
         self.sender
@@ -191,6 +198,7 @@ impl DhtRequester {
         reply_rx.await.map_err(|_| DhtActorError::ReplyCanceled)
     }
 
+    /// Returns the deserialized metadata value for the given key
     pub async fn get_metadata<T: MessageFormat>(&mut self, key: DhtMetadataKey) -> Result<Option<T>, DhtActorError> {
         let (reply_tx, reply_rx) = oneshot::channel();
         self.sender.send(DhtRequest::GetMetadata(key, reply_tx)).await?;
@@ -202,6 +210,7 @@ impl DhtRequester {
         }
     }
 
+    /// Sets the metadata value for the given key
     pub async fn set_metadata<T: MessageFormat>(&mut self, key: DhtMetadataKey, value: T) -> Result<(), DhtActorError> {
         let (reply_tx, reply_rx) = oneshot::channel();
         let bytes = value.to_binary().map_err(DhtActorError::FailedToSerializeValue)?;
@@ -223,6 +232,7 @@ impl DhtRequester {
     }
 }
 
+/// DHT actor. Responsible for executing DHT-related tasks.
 pub struct DhtActor {
     node_identity: Arc<NodeIdentity>,
     peer_manager: Arc<PeerManager>,
@@ -237,7 +247,8 @@ pub struct DhtActor {
 }
 
 impl DhtActor {
-    pub fn new(
+    /// Create a new DhtActor
+    pub(crate) fn new(
         config: Arc<DhtConfig>,
         conn: DbConnection,
         node_identity: Arc<NodeIdentity>,
@@ -268,6 +279,7 @@ impl DhtActor {
         }
     }
 
+    /// Spawns the DHT actor on a new task.
     pub fn spawn(self) {
         task::spawn(async move {
             if let Err(err) = self.run().await {

comms/dht/src/broadcast_strategy.rs

@@ -20,6 +20,10 @@
 // WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE
 // USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
+//! # Broadcast strategy
+//!
+//! Describes a strategy for selecting peers and active connections when sending messages.
+
 use std::{
     fmt,
     fmt::{Display, Formatter},
@@ -29,6 +33,7 @@ use tari_comms::{peer_manager::node_id::NodeId, types::CommsPublicKey};
 
 use crate::envelope::NodeDestination;
 
+/// Parameters for the [ClosestNodes](self::BroadcastStrategy::ClosestNodes) broadcast strategy.
 #[derive(Debug, Clone)]
 pub struct BroadcastClosestRequest {
     pub node_id: NodeId,
@@ -48,6 +53,7 @@ impl Display for BroadcastClosestRequest {
     }
 }
 
+/// Describes a strategy for selecting peers and active connections when sending messages.
 #[derive(Debug, Clone)]
 pub enum BroadcastStrategy {
     /// Send to a particular peer matching the given node ID
@@ -101,11 +107,14 @@ impl BroadcastStrategy {
         }
     }
 
+    /// Returns true if the strategy is to send directly to the peer, otherwise false
     pub fn is_direct(&self) -> bool {
         use BroadcastStrategy::{DirectNodeId, DirectPublicKey};
         matches!(self, DirectNodeId(_) | DirectPublicKey(_))
     }
 
+    /// Returns a reference to the NodeId used in the `DirectNodeId` strategy, otherwise None if the strategy is not
+    /// `DirectNodeId`.
     pub fn direct_node_id(&self) -> Option<&NodeId> {
         use BroadcastStrategy::DirectNodeId;
         match self {
@@ -114,6 +123,8 @@ impl BroadcastStrategy {
         }
     }
 
+    /// Returns a reference to the `CommsPublicKey` used in the `DirectPublicKey` strategy, otherwise None if the
+    /// strategy is not `DirectPublicKey`.
     pub fn direct_public_key(&self) -> Option<&CommsPublicKey> {
         use BroadcastStrategy::DirectPublicKey;
         match self {
@@ -122,6 +133,8 @@ impl BroadcastStrategy {
         }
     }
 
+    /// Returns the `CommsPublicKey` used in the `DirectPublicKey` strategy, otherwise None if the strategy is not
+    /// `DirectPublicKey`.
     pub fn into_direct_public_key(self) -> Option<Box<CommsPublicKey>> {
         use BroadcastStrategy::DirectPublicKey;
         match self {

comms/dht/src/builder.rs

@@ -20,6 +20,8 @@
 // WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE
 // USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
+//! A builder for customizing and constructing the DHT
+
 use std::{sync::Arc, time::Duration};
 
 use tari_comms::{connectivity::ConnectivityRequester, NodeIdentity, PeerManager};
@@ -35,14 +37,21 @@ use crate::{
     DhtConfig,
 };
 
+/// Builder for the DHT.
+///
+/// ```rust
+/// # use tari_comms_dht::{DbConnectionUrl, Dht};
+/// let builder = Dht::builder().mainnet().with_database_url(DbConnectionUrl::Memory);
+/// // let dht = builder.build(...).unwrap();
+/// ```
 #[derive(Debug, Clone, Default)]
 pub struct DhtBuilder {
     config: DhtConfig,
     outbound_tx: Option<mpsc::Sender<DhtOutboundRequest>>,
 }
 
 impl DhtBuilder {
-    pub fn new() -> Self {
+    pub(crate) fn new() -> Self {
         Self {
             #[cfg(test)]
             config: DhtConfig::default_local_test(),
@@ -52,87 +61,89 @@ impl DhtBuilder {
         }
     }
 
+    /// Specify a complete [DhtConfig](crate::DhtConfig).
     pub fn with_config(&mut self, config: DhtConfig) -> &mut Self {
         self.config = config;
         self
     }
 
+    /// Default configuration for local test environments.
     pub fn local_test(&mut self) -> &mut Self {
         self.config = DhtConfig::default_local_test();
         self
     }
 
+    /// Sets the DHT protocol version.
     pub fn with_protocol_version(&mut self, protocol_version: DhtProtocolVersion) -> &mut Self {
         self.config.protocol_version = protocol_version;
         self
     }
 
+    /// Sets whether SAF messages are automatically requested on every new connection to a SAF node.
     pub fn set_auto_store_and_forward_requests(&mut self, enabled: bool) -> &mut Self {
         self.config.saf.auto_request = enabled;
         self
     }
 
+    /// Sets the mpsc sender that is hooked up to the outbound messaging pipeline.
     pub fn with_outbound_sender(&mut self, outbound_tx: mpsc::Sender<DhtOutboundRequest>) -> &mut Self {
         self.outbound_tx = Some(outbound_tx);
         self
     }
 
+    /// Use the default testnet configuration.
     pub fn testnet(&mut self) -> &mut Self {
         self.config = DhtConfig::default_testnet();
         self
     }
 
+    /// Use the default mainnet configuration.
     pub fn mainnet(&mut self) -> &mut Self {
         self.config = DhtConfig::default_mainnet();
         self
     }
 
+    /// Sets the [DbConnectionUrl](crate::DbConnectionUrl).
     pub fn with_database_url(&mut self, database_url: DbConnectionUrl) -> &mut Self {
         self.config.database_url = database_url;
         self
     }
 
-    pub fn with_dedup_cache_trim_interval(&mut self, trim_interval: Duration) -> &mut Self {
-        self.config.dedup_cache_trim_interval = trim_interval;
-        self
-    }
-
-    pub fn with_dedup_cache_capacity(&mut self, capacity: usize) -> &mut Self {
-        self.config.dedup_cache_capacity = capacity;
-        self
-    }
-
-    pub fn with_dedup_discard_hit_count(&mut self, max_hit_count: usize) -> &mut Self {
-        self.config.dedup_allowed_message_occurrences = max_hit_count;
-        self
-    }
-
+    /// The number of connections to random peers that should be maintained.
+    /// Connections to random peers are reshuffled every `DhtConfig::connectivity::random_pool_refresh_interval`.
     pub fn with_num_random_nodes(&mut self, n: usize) -> &mut Self {
         self.config.num_random_nodes = n;
         self
     }
 
+    /// The number of neighbouring peers that the DHT should try maintain connections to.
     pub fn with_num_neighbouring_nodes(&mut self, n: usize) -> &mut Self {
         self.config.num_neighbouring_nodes = n;
         self.config.saf.num_neighbouring_nodes = n;
         self
     }
 
+    /// The number of peers to send a message using the
+    /// [Broadcast](crate::broadcast_strategy::BroadcastStrategy::Propagate) strategy.
     pub fn with_propagation_factor(&mut self, propagation_factor: usize) -> &mut Self {
         self.config.propagation_factor = propagation_factor;
         self
     }
 
+    /// The number of peers to send a message broadcast using the
+    /// [Broadcast](crate::broadcast_strategy::BroadcastStrategy::Broadcast) strategy.
     pub fn with_broadcast_factor(&mut self, broadcast_factor: usize) -> &mut Self {
         self.config.broadcast_factor = broadcast_factor;
         self
     }
 
+    /// The length of time to wait for a discovery reply after a discovery message has been sent.
     pub fn with_discovery_timeout(&mut self, timeout: Duration) -> &mut Self {
         self.config.discovery_request_timeout = timeout;
         self
     }
 
+    /// Enables automatically sending a join/announce message when connected to enough peers on the network.
     pub fn enable_auto_join(&mut self) -> &mut Self {
         self.config.auto_join = true;
         self

comms/dht/src/config.rs

@@ -101,14 +101,17 @@ pub struct DhtConfig {
 }
 
 impl DhtConfig {
+    /// Default testnet configuration
     pub fn default_testnet() -> Self {
         Default::default()
     }
 
+    /// Default mainnet configuration
     pub fn default_mainnet() -> Self {
         Default::default()
     }
 
+    /// Default local test configuration
     pub fn default_local_test() -> Self {
         Self {
             database_url: DbConnectionUrl::Memory,
@@ -171,7 +174,7 @@ pub struct DhtConnectivityConfig {
     pub update_interval: Duration,
     /// The interval to change the random pool peers.
     /// Default: 2 hours
-    pub random_pool_refresh: Duration,
+    pub random_pool_refresh_interval: Duration,
     /// Length of cooldown when high connection failure rates are encountered. Default: 45s
     pub high_failure_rate_cooldown: Duration,
     /// The minimum desired ratio of TCPv4 to Tor connections. TCPv4 addresses have some significant cost to create,
@@ -185,7 +188,7 @@ impl Default for DhtConnectivityConfig {
     fn default() -> Self {
         Self {
             update_interval: Duration::from_secs(2 * 60),
-            random_pool_refresh: Duration::from_secs(2 * 60 * 60),
+            random_pool_refresh_interval: Duration::from_secs(2 * 60 * 60),
             high_failure_rate_cooldown: Duration::from_secs(45),
             minimum_desired_tcpv4_node_ratio: 0.1,
         }