fix(mempool): remove TODOs and other minor changes (#5498)

Description
---

- Update config tests to use non-deprecated api
- remove outdated TODOs
- remove dead code
- dont return internal error details back to peer (possible info leak)

Motivation and Context
---
Mempool audit

How Has This Been Tested?
---
Existing tests

What process can a PR reviewer use to test or verify this change?
---
Cosmetic changes
<!-- Checklist -->
<!-- 1. Is the title of your PR in the form that would make nice release
notes? The title, excluding the conventional commit
tag, will be included exactly as is in the CHANGELOG, so please think
about it carefully. -->


Breaking Changes
---

- [x] None
- [ ] Requires data directory on base node to be deleted
- [ ] Requires hard fork
- [ ] Other - Please specify

<!-- Does this include a breaking change? If so, include this line as a
footer -->
<!-- BREAKING CHANGE: Description what the user should do, e.g. delete a
database, resync the chain -->

base_layer/core/src/mempool/config.rs

@@ -66,9 +66,6 @@ impl Default for MempoolServiceConfig {
 
 #[cfg(test)]
 mod test {
-    // TODO: Use new Config api - seems that you need to use the builder each time you want to change a value which
-    //       isn't great, there must be a better way.
-    #![allow(deprecated)]
     use config::Config;
     use tari_common::DefaultConfigLoader;
 
@@ -77,11 +74,12 @@ mod test {
 
     #[test]
     pub fn test_mempool_config() {
-        let mut config = Config::builder().build().unwrap();
+        let config = Config::builder()
+            .set_override("mempool.unconfirmed_pool.storage_capacity", 3)
+            .unwrap()
+            .build()
+            .unwrap();
 
-        config
-            .set("mempool.unconfirmed_pool.storage_capacity", 3)
-            .expect("Could not set ''");
         let my_config = MempoolConfig::load_from(&config).expect("Could not load configuration");
         // [ ] mempool.mainnet, [X]  mempool = 3, [X] Default
         assert_eq!(my_config.unconfirmed_pool.storage_capacity, 3);
@@ -92,13 +90,15 @@ mod test {
             ReorgPoolConfig::default().expiry_height
         );
 
-        config
-            .set("mainnet.mempool.unconfirmed_pool.storage_capacity", 20)
-            .expect("Could not set ''");
+        let config = Config::builder()
+            .add_source(config)
+            .set_override("mainnet.mempool.unconfirmed_pool.storage_capacity", 20)
+            .unwrap()
+            .set_override("mempool.override_from", "mainnet")
+            .unwrap()
+            .build()
+            .unwrap();
 
-        config
-            .set("mempool.override_from", "mainnet")
-            .expect("Could not set 'override_from'");
         // use_network = mainnet
         let my_config = MempoolConfig::load_from(&config).expect("Could not load configuration");
         // [ ] mempool.mainnet, [X]  mempool = 3, [X] Default

base_layer/core/src/mempool/mempool_storage.rs

@@ -243,7 +243,6 @@ impl MempoolStorage {
     }
 
     /// Returns all unconfirmed transaction stored in the Mempool, except the transactions stored in the ReOrgPool.
-    // TODO: Investigate returning an iterator rather than a large vector of transactions
     pub fn snapshot(&self) -> Vec<Arc<Transaction>> {
         self.unconfirmed_pool.snapshot()
     }

base_layer/core/src/mempool/reorg_pool/reorg_pool.rs

@@ -77,16 +77,16 @@ impl ReorgPool {
     /// Insert a new transaction into the ReorgPool. Published transactions will be discarded once they are
     /// `config.expiry_height` blocks old.
     fn insert(&mut self, height: u64, tx: Arc<Transaction>) {
-        let excess_hex = tx
-            .first_kernel_excess_sig()
-            .map(|s| s.get_signature().to_hex())
-            .unwrap_or_else(|| "no kernel!".to_string());
         if tx
             .body
             .kernels()
             .iter()
             .all(|k| self.txs_by_signature.contains_key(k.excess_sig.get_signature()))
         {
+            let excess_hex = tx
+                .first_kernel_excess_sig()
+                .map(|s| s.get_signature().to_hex())
+                .unwrap_or_else(|| "no kernel!".to_string());
             debug!(
                 target: LOG_TARGET,
                 "Transaction {} already found in reorg pool", excess_hex

base_layer/core/src/mempool/rpc/service.rs

@@ -48,11 +48,9 @@ impl MempoolRpcService {
     }
 }
 
-// TODO: Logging the error and returning a general error to the requester is a common requirement. Figure out a clean
-//       way to provide this functionality.
 fn to_internal_error<T: std::error::Error>(err: T) -> RpcStatus {
     error!(target: LOG_TARGET, "Internal error: {}", err);
-    RpcStatus::general(&err.to_string())
+    RpcStatus::general_default()
 }
 
 #[tari_comms::async_trait]

base_layer/core/src/mempool/service/inbound_handlers.rs

@@ -45,13 +45,16 @@ pub const LOG_TARGET: &str = "c::mp::service::inbound_handlers";
 #[derive(Clone)]
 pub struct MempoolInboundHandlers {
     mempool: Mempool,
-    outbound_nmi: OutboundMempoolServiceInterface,
+    outbound_service: OutboundMempoolServiceInterface,
 }
 
 impl MempoolInboundHandlers {
     /// Construct the MempoolInboundHandlers.
-    pub fn new(mempool: Mempool, outbound_nmi: OutboundMempoolServiceInterface) -> Self {
-        Self { mempool, outbound_nmi }
+    pub fn new(mempool: Mempool, outbound_service: OutboundMempoolServiceInterface) -> Self {
+        Self {
+            mempool,
+            outbound_service,
+        }
     }
 
     /// Handle inbound Mempool service requests from remote nodes and local services.
@@ -139,7 +142,7 @@ impl MempoolInboundHandlers {
                         target: LOG_TARGET,
                         "Propagate transaction ({}) to network.", kernel_excess_sig,
                     );
-                    self.outbound_nmi
+                    self.outbound_service
                         .propagate_tx(tx, source_peer.into_iter().collect())
                         .await?;
                 }

base_layer/core/src/mempool/service/service.rs

@@ -131,7 +131,6 @@ impl MempoolService {
     }
 
     async fn handle_request(&mut self, request: MempoolRequest) -> Result<MempoolResponse, MempoolServiceError> {
-        // TODO: Move db calls into MempoolService
         self.inbound_handlers.handle_request(request).await
     }