GH-600: Address review feedback

node/src/blockchain/blockchain_bridge.rs

@@ -336,7 +336,7 @@ impl BlockchainBridge {
             Err (e) => panic! ("Cannot retrieve start block from database; payments to you may not be processed: {:?}", e)
         };
         let max_block_count = match self.persistent_config.max_block_count() {
-            Ok(mbc) => mbc,
+            Ok(Some(mbc)) => mbc,
             _ => u64::MAX,
         };
         let end_block = match self.blockchain_interface.get_block_number() {
@@ -393,7 +393,7 @@ impl BlockchainBridge {
                 if let Some(max_block_count) = self.extract_max_block_count(e.clone()) {
                     debug!(self.logger, "Writing max_block_count({})", max_block_count);
                     self.persistent_config
-                        .set_max_block_count(max_block_count)
+                        .set_max_block_count(Some(max_block_count))
                         .map_or_else(
                             |_| {
                                 warning!(self.logger, "{} update max_block_count to {}. Scheduling next scan with that limit.", e, max_block_count);
@@ -1325,7 +1325,7 @@ mod tests {
             )))
             .get_block_number_result(LatestBlockNumber::Ok(U64::from(1234u64)));
         let persistent_config = PersistentConfigurationMock::new()
-            .max_block_count_result(Ok(100_000))
+            .max_block_count_result(Ok(Some(100_000)))
             .start_block_result(Ok(5)); // no set_start_block_result: set_start_block() must not be called
         let mut subject = BlockchainBridge::new(
             Box::new(blockchain_interface),
@@ -1620,7 +1620,7 @@ mod tests {
             )));
         let set_start_block_params_arc = Arc::new(Mutex::new(vec![]));
         let persistent_config = PersistentConfigurationMock::new()
-            .max_block_count_result(Ok(10000u64))
+            .max_block_count_result(Ok(Some(10000u64)))
             .start_block_result(Ok(6))
             .set_start_block_params(&set_start_block_params_arc)
             .set_start_block_result(Ok(()));
@@ -1711,7 +1711,7 @@ mod tests {
             .get_block_number_result(latest_block_number);
         let set_start_block_params_arc = Arc::new(Mutex::new(vec![]));
         let persistent_config = PersistentConfigurationMock::new()
-            .max_block_count_result(Ok(10000u64))
+            .max_block_count_result(Ok(Some(10000u64)))
             .start_block_result(Ok(6))
             .set_start_block_params(&set_start_block_params_arc)
             .set_start_block_result(Ok(()));
@@ -1778,7 +1778,7 @@ mod tests {
             .get_block_number_result(Ok(0u64.into()));
         let set_start_block_params_arc = Arc::new(Mutex::new(vec![]));
         let persistent_config = PersistentConfigurationMock::new()
-            .max_block_count_result(Ok(10000u64))
+            .max_block_count_result(Ok(Some(10000u64)))
             .start_block_result(Ok(6))
             .set_start_block_params(&set_start_block_params_arc)
             .set_start_block_result(Ok(()));
@@ -1861,7 +1861,7 @@ mod tests {
         let persistent_config = PersistentConfigurationMock::new()
             .start_block_result(Ok(1234))
             .set_start_block_result(Err(PersistentConfigError::TransactionError))
-            .max_block_count_result(Ok(10000u64));
+            .max_block_count_result(Ok(Some(10000u64)));
         let blockchain_interface = BlockchainInterfaceMock::default()
             .get_block_number_result(Ok(0u64.into()))
             .retrieve_transactions_result(Ok(RetrievedBlockchainTransactions {
@@ -2045,8 +2045,6 @@ mod tests {
 
     #[test]
     fn extract_max_block_range_from_pokt_error_response() {
-        // [{"jsonrpc":"2.0","id":4,"result":"0x2400ee1","error":{}},{"jsonrpc":"2.0","id":5,"error":{"code":-32001,"message":"Relay request failed validation: invalid relay request: eth_getLogs block range limit (100000 blocks) exceeded"}}]
-        // BlockchainError::QueryFailed("Rpc(Error { code: ServerError(-32001), message: \"Relay request failed validation: invalid relay request: eth_getLogs block range limit (100000 blocks) exceeded\", data: None })"
         let result = BlockchainError::QueryFailed("Rpc(Error { code: ServerError(-32001), message: \"Relay request failed validation: invalid relay request: eth_getLogs block range limit (100000 blocks) exceeded\", data: None })".to_string());
         let subject = BlockchainBridge::new(
             Box::new(BlockchainInterfaceMock::default()),

node/src/database/db_migrations/migrations/migration_8_to_9.rs

@@ -10,7 +10,7 @@ impl DatabaseMigration for Migrate_8_to_9 {
         declaration_utils: Box<dyn DBMigDeclarator + 'a>,
     ) -> rusqlite::Result<()> {
         declaration_utils.execute_upon_transaction(&[
-            &"INSERT INTO config (name, value, encrypted) VALUES ('max_block_count', null, 0)",
+            &"INSERT INTO config (name, value, encrypted) VALUES ('max_block_count', '', 0)",
         ])
     }
 
@@ -51,7 +51,7 @@ mod tests {
         let connection = result.unwrap();
         let (mp_value, mp_encrypted) = retrieve_config_row(connection.as_ref(), "max_block_count");
         let (cs_value, cs_encrypted) = retrieve_config_row(connection.as_ref(), "schema_version");
-        assert_eq!(mp_value, None);
+        assert_eq!(mp_value, Some("".to_string()));
         assert_eq!(mp_encrypted, false);
         assert_eq!(cs_value, Some("9".to_string()));
         assert_eq!(cs_encrypted, false);

node/src/db_config/persistent_configuration.rs

@@ -136,8 +136,8 @@ pub trait PersistentConfiguration {
     ) -> Result<(), PersistentConfigError>;
     fn start_block(&self) -> Result<u64, PersistentConfigError>;
     fn set_start_block(&mut self, value: u64) -> Result<(), PersistentConfigError>;
-    fn max_block_count(&self) -> Result<u64, PersistentConfigError>;
-    fn set_max_block_count(&mut self, value: u64) -> Result<(), PersistentConfigError>;
+    fn max_block_count(&self) -> Result<Option<u64>, PersistentConfigError>;
+    fn set_max_block_count(&mut self, value: Option<u64>) -> Result<(), PersistentConfigError>;
     fn set_wallet_info(
         &mut self,
         consuming_wallet_private_key: &str,
@@ -415,19 +415,25 @@ impl PersistentConfiguration for PersistentConfigurationReal {
         self.simple_set_method("start_block", value)
     }
 
-    fn max_block_count(&self) -> Result<u64, PersistentConfigError> {
+    fn max_block_count(&self) -> Result<Option<u64>, PersistentConfigError> {
         match self.get("max_block_count") {
             Ok(max_block_count) => match decode_u64(max_block_count) {
-                Ok(Some(mbc)) => Ok(mbc),
+                Ok(mbc_opt) => Ok(mbc_opt),
+                Err(TypedConfigLayerError::BadNumberFormat(value)) if value.is_empty() => Ok(None),
                 Err(e) => Err(PersistentConfigError::from(e)),
-                Ok(None) => Err(PersistentConfigError::NotPresent),
             },
             Err(e) => Err(PersistentConfigError::from(e)),
         }
     }
 
-    fn set_max_block_count(&mut self, value: u64) -> Result<(), PersistentConfigError> {
-        self.simple_set_method("max_block_count", value)
+    fn set_max_block_count(&mut self, value: Option<u64>) -> Result<(), PersistentConfigError> {
+        self.simple_set_method(
+            "max_block_count",
+            match encode_u64(value) {
+                Ok(Some(mbc)) => mbc,
+                _ => "".to_string(),
+            },
+        )
     }
 
     fn set_wallet_info(
@@ -1953,10 +1959,38 @@ mod tests {
     }
 
     #[test]
-    fn max_block_count_set_method_works() {
-        persistent_config_plain_data_assertions_for_simple_set_method!(
-            "max_block_count",
-            100000u64
+    fn max_block_count_set_method_works_with_some() {
+        let set_params_arc = Arc::new(Mutex::new(Vec::new()));
+        let config_dao = ConfigDaoMock::new()
+            .set_params(&set_params_arc)
+            .set_result(Ok(()));
+        let mut subject = PersistentConfigurationReal::new(Box::new(config_dao));
+
+        let result = subject.set_max_block_count(Some(100_000u64));
+
+        assert!(result.is_ok());
+        let set_params = set_params_arc.lock().unwrap();
+        assert_eq!(
+            *set_params,
+            vec![("max_block_count".to_string(), Some(100_000u64.to_string()))]
+        );
+    }
+
+    #[test]
+    fn max_block_count_set_method_works_with_none() {
+        let set_params_arc = Arc::new(Mutex::new(Vec::new()));
+        let config_dao = ConfigDaoMock::new()
+            .set_params(&set_params_arc)
+            .set_result(Ok(()));
+        let mut subject = PersistentConfigurationReal::new(Box::new(config_dao));
+
+        let result = subject.set_max_block_count(None);
+
+        assert!(result.is_ok());
+        let set_params = set_params_arc.lock().unwrap();
+        assert_eq!(
+            *set_params,
+            vec![("max_block_count".to_string(), Some("".to_string()))]
         );
     }
 

node/src/node_configurator/configurator.rs

@@ -554,11 +554,13 @@ impl Configurator {
             "earningWalletAddressOpt",
         )?;
         let start_block = Self::value_required(persistent_config.start_block(), "startBlock")?;
-        let max_block_count_opt =
-            match Self::value_required(persistent_config.max_block_count(), "maxBlockCount") {
-                Ok(value) => Some(value),
-                _ => None,
-            };
+        let max_block_count_opt = match persistent_config.max_block_count() {
+            Ok(value) => value,
+            Err(e) => panic!(
+                "Database corruption: Could not read max block count: {:?}",
+                e
+            ),
+        };
         let neighborhood_mode =
             Self::value_required(persistent_config.neighborhood_mode(), "neighborhoodMode")?
                 .to_string();
@@ -2423,7 +2425,7 @@ mod tests {
             .gas_price_result(Ok(2345))
             .consuming_wallet_private_key_result(Ok(Some(consuming_wallet_private_key)))
             .mapping_protocol_result(Ok(Some(AutomapProtocol::Igdp)))
-            .max_block_count_result(Ok(100000))
+            .max_block_count_result(Ok(Some(100000)))
             .neighborhood_mode_result(Ok(NeighborhoodModeLight::Standard))
             .past_neighbors_result(Ok(Some(vec![node_descriptor.clone()])))
             .earning_wallet_address_result(Ok(Some(earning_wallet_address.clone())))
@@ -2553,7 +2555,7 @@ mod tests {
             .consuming_wallet_private_key_params(&consuming_wallet_private_key_params_arc)
             .consuming_wallet_private_key_result(Ok(Some(consuming_wallet_private_key.clone())))
             .mapping_protocol_result(Ok(Some(AutomapProtocol::Igdp)))
-            .max_block_count_result(Ok(100000))
+            .max_block_count_result(Ok(Some(100000)))
             .neighborhood_mode_result(Ok(NeighborhoodModeLight::ConsumeOnly))
             .past_neighbors_params(&past_neighbors_params_arc)
             .past_neighbors_result(Ok(Some(vec![node_descriptor.clone()])))
@@ -2620,6 +2622,119 @@ mod tests {
         assert_eq!(*past_neighbors_params, vec!["password".to_string()])
     }
 
+    #[test]
+    fn configuration_handles_retrieving_max_block_count_none() {
+        let persistent_config = PersistentConfigurationMock::new()
+            .check_password_result(Ok(true))
+            .blockchain_service_url_result(Ok(None))
+            .current_schema_version_result("3")
+            .clandestine_port_result(Ok(1234))
+            .chain_name_result("ropsten".to_string())
+            .gas_price_result(Ok(2345))
+            .earning_wallet_address_result(Ok(None))
+            .start_block_result(Ok(3456))
+            .max_block_count_result(Ok(None))
+            .neighborhood_mode_result(Ok(NeighborhoodModeLight::ZeroHop))
+            .mapping_protocol_result(Ok(None))
+            .consuming_wallet_private_key_result(Ok(None))
+            .past_neighbors_result(Ok(None))
+            .rate_pack_result(Ok(RatePack {
+                routing_byte_rate: 0,
+                routing_service_rate: 0,
+                exit_byte_rate: 0,
+                exit_service_rate: 0,
+            }))
+            .scan_intervals_result(Ok(ScanIntervals {
+                pending_payable_scan_interval: Default::default(),
+                payable_scan_interval: Default::default(),
+                receivable_scan_interval: Default::default(),
+            }))
+            .payment_thresholds_result(Ok(PaymentThresholds {
+                debt_threshold_gwei: 0,
+                maturity_threshold_sec: 0,
+                payment_grace_period_sec: 0,
+                permanent_debt_allowed_gwei: 0,
+                threshold_interval_sec: 0,
+                unban_below_gwei: 0,
+            }));
+        let mut subject = make_subject(Some(persistent_config));
+
+        let (configuration, context_id) =
+            UiConfigurationResponse::fmb(subject.handle_configuration(
+                UiConfigurationRequest {
+                    db_password_opt: Some("password".to_string()),
+                },
+                4321,
+            ))
+            .unwrap();
+
+        assert_eq!(context_id, 4321);
+        assert_eq!(
+            configuration,
+            UiConfigurationResponse {
+                blockchain_service_url_opt: None,
+                current_schema_version: "3".to_string(),
+                clandestine_port: 1234,
+                chain_name: "ropsten".to_string(),
+                gas_price: 2345,
+                max_block_count_opt: None,
+                neighborhood_mode: String::from("zero-hop"),
+                consuming_wallet_private_key_opt: None,
+                consuming_wallet_address_opt: None,
+                earning_wallet_address_opt: None,
+                port_mapping_protocol_opt: None,
+                past_neighbors: vec![],
+                payment_thresholds: UiPaymentThresholds {
+                    threshold_interval_sec: 0,
+                    debt_threshold_gwei: 0,
+                    maturity_threshold_sec: 0,
+                    payment_grace_period_sec: 0,
+                    permanent_debt_allowed_gwei: 0,
+                    unban_below_gwei: 0
+                },
+                rate_pack: UiRatePack {
+                    routing_byte_rate: 0,
+                    routing_service_rate: 0,
+                    exit_byte_rate: 0,
+                    exit_service_rate: 0
+                },
+                start_block: 3456,
+                scan_intervals: UiScanIntervals {
+                    pending_payable_sec: 0,
+                    payable_sec: 0,
+                    receivable_sec: 0
+                }
+            }
+        );
+    }
+
+    #[test]
+    #[should_panic(
+        expected = "Database corruption: Could not read max block count: DatabaseError(\"Corruption\")"
+    )]
+    fn configuration_panic_on_error_retrieving_max_block_count() {
+        let persistent_config = PersistentConfigurationMock::new()
+            .check_password_result(Ok(true))
+            .blockchain_service_url_result(Ok(None))
+            .current_schema_version_result("3")
+            .clandestine_port_result(Ok(1234))
+            .chain_name_result("ropsten".to_string())
+            .gas_price_result(Ok(2345))
+            .earning_wallet_address_result(Ok(Some("4a5e43b54c6C56Ebf7".to_string())))
+            .start_block_result(Ok(3456))
+            .max_block_count_result(Err(PersistentConfigError::DatabaseError(
+                "Corruption".to_string(),
+            )));
+        let mut subject = make_subject(Some(persistent_config));
+
+        let _result = subject.handle_configuration(
+            UiConfigurationRequest {
+                db_password_opt: Some("password".to_string()),
+            },
+            4321,
+        );
+    }
+
     #[test]
     fn configuration_handles_check_password_error() {
         let persistent_config = PersistentConfigurationMock::new()
@@ -2657,7 +2772,7 @@ mod tests {
                 "0x0123456789012345678901234567890123456789".to_string(),
             )))
             .start_block_result(Ok(3456))
-            .max_block_count_result(Ok(100000))
+            .max_block_count_result(Ok(Some(100000)))
             .neighborhood_mode_result(Ok(NeighborhoodModeLight::ConsumeOnly))
             .mapping_protocol_result(Ok(Some(AutomapProtocol::Igdp)))
             .consuming_wallet_private_key_result(cwpk);

node/src/test_utils/persistent_configuration_mock.rs

@@ -61,8 +61,8 @@ pub struct PersistentConfigurationMock {
     set_start_block_params: Arc<Mutex<Vec<u64>>>,
     set_start_block_results: RefCell<Vec<Result<(), PersistentConfigError>>>,
     max_block_count_params: Arc<Mutex<Vec<()>>>,
-    max_block_count_results: RefCell<Vec<Result<u64, PersistentConfigError>>>,
-    set_max_block_count_params: Arc<Mutex<Vec<u64>>>,
+    max_block_count_results: RefCell<Vec<Result<Option<u64>, PersistentConfigError>>>,
+    set_max_block_count_params: Arc<Mutex<Vec<Option<u64>>>>,
     set_max_block_count_results: RefCell<Vec<Result<(), PersistentConfigError>>>,
     payment_thresholds_results: RefCell<Vec<Result<PaymentThresholds, PersistentConfigError>>>,
     set_payment_thresholds_params: Arc<Mutex<Vec<String>>>,
@@ -237,12 +237,12 @@ impl PersistentConfiguration for PersistentConfigurationMock {
         Self::result_from(&self.set_start_block_results)
     }
 
-    fn max_block_count(&self) -> Result<u64, PersistentConfigError> {
+    fn max_block_count(&self) -> Result<Option<u64>, PersistentConfigError> {
         self.max_block_count_params.lock().unwrap().push(());
         Self::result_from(&self.max_block_count_results)
     }
 
-    fn set_max_block_count(&mut self, value: u64) -> Result<(), PersistentConfigError> {
+    fn set_max_block_count(&mut self, value: Option<u64>) -> Result<(), PersistentConfigError> {
         self.set_max_block_count_params.lock().unwrap().push(value);
         Self::result_from(&self.set_max_block_count_results)
     }
@@ -552,12 +552,15 @@ impl PersistentConfigurationMock {
         self
     }
 
-    pub fn max_block_count_result(self, result: Result<u64, PersistentConfigError>) -> Self {
+    pub fn max_block_count_result(
+        self,
+        result: Result<Option<u64>, PersistentConfigError>,
+    ) -> Self {
         self.max_block_count_results.borrow_mut().push(result);
         self
     }
 
-    pub fn set_max_block_count_params(mut self, params: &Arc<Mutex<Vec<u64>>>) -> Self {
+    pub fn set_max_block_count_params(mut self, params: &Arc<Mutex<Vec<Option<u64>>>>) -> Self {
         self.set_max_block_count_params = params.clone();
         self
     }