fix: don't vote on deposit requests in case of blocklist client errors (

#1270)

* fix: don't vote on deposit requests in case of blocklist client errors

* chore: move retry to config

* chore: move delay to the blocklist client, small refactor

* chore: fmt

* chore: fix merge

* chore: bump default, nit

docker/sbtc/signer/signer-config.toml

@@ -11,6 +11,12 @@
 # [blocklist_client]
 # endpoint = "http://127.0.0.1:8080"
 
+# The delay, in milliseconds, for the retry after a blocklist client failure 
+#
+# Required: false
+# Environment: SIGNER_BLOCKLIST_CLIENT__RETRY_DELAY
+# retry_delay = 1000
+
 # !! ==============================================================================
 # !! Emily API Configuration
 # !! ==============================================================================

signer/src/blocklist_client.rs

@@ -8,44 +8,52 @@
 use blocklist_api::apis::address_api::{check_address, CheckAddressError};
 use blocklist_api::apis::configuration::Configuration;
 use blocklist_api::apis::Error as ClientError;
-use blocklist_api::models::BlocklistStatus;
 use std::future::Future;
+use std::time::Duration;
 
-use crate::context::Context;
+use crate::config::BlocklistClientConfig;
+use crate::error::Error;
+
+/// Blocklist client error variants.
+#[derive(Debug, thiserror::Error)]
+pub enum BlocklistClientError {
+    /// An error occurred while checking an address
+    #[error("error checking an address: {0}")]
+    CheckAddress(ClientError<CheckAddressError>),
+}
 
 /// A trait for checking if an address is blocklisted.
 pub trait BlocklistChecker {
     /// Checks if the given address is blocklisted.
     /// Returns `true` if the address is blocklisted, otherwise `false`.
-    fn can_accept(
-        &self,
-        address: &str,
-    ) -> impl Future<Output = Result<bool, ClientError<CheckAddressError>>> + Send;
+    fn can_accept(&self, address: &str) -> impl Future<Output = Result<bool, Error>> + Send;
 }
 
 /// A client for interacting with the blocklist service.
 #[derive(Clone, Debug)]
 pub struct BlocklistClient {
     config: Configuration,
+    retry_delay: Duration,
 }
 
 impl BlocklistChecker for BlocklistClient {
-    async fn can_accept(&self, address: &str) -> Result<bool, ClientError<CheckAddressError>> {
-        let config = self.config.clone();
-
-        // Call the generated function from blocklist-api
-        let resp: BlocklistStatus = check_address(&config, address).await?;
-        Ok(resp.accept)
+    async fn can_accept(&self, address: &str) -> Result<bool, Error> {
+        let response = self.check_address(address).await;
+        if let Err(error) = response {
+            tracing::error!(%error, "blocklist client error, sleeping and retrying once");
+            tokio::time::sleep(self.retry_delay).await;
+            self.check_address(address).await
+        } else {
+            response
+        }
     }
 }
 
 impl BlocklistClient {
     /// Construct a new [`BlocklistClient`]
-    pub fn new(ctx: &impl Context) -> Option<Self> {
-        let config = ctx.config().blocklist_client.as_ref()?;
-
+    pub fn new(client_config: &BlocklistClientConfig) -> Self {
         let mut config = Configuration {
-            base_path: config.endpoint.to_string(),
+            base_path: client_config.endpoint.to_string(),
             ..Default::default()
         };
 
@@ -57,29 +65,39 @@ impl BlocklistClient {
             .trim_end_matches("/")
             .to_string();
 
-        Some(BlocklistClient { config })
+        BlocklistClient {
+            config,
+            retry_delay: client_config.retry_delay,
+        }
     }
 
-    #[cfg(test)]
-    fn with_base_url(base_url: String) -> Self {
+    /// Construct a new [`BlocklistClient`] from a base url
+    #[cfg(any(test, feature = "testing"))]
+    pub fn with_base_url(base_url: String) -> Self {
         let config = Configuration {
             base_path: base_url.clone(),
             ..Default::default()
         };
 
-        BlocklistClient { config }
+        BlocklistClient {
+            config,
+            retry_delay: Duration::ZERO,
+        }
+    }
+
+    async fn check_address(&self, address: &str) -> Result<bool, Error> {
+        // Call the generated function from blocklist-api
+        check_address(&self.config, address)
+            .await
+            .map_err(BlocklistClientError::CheckAddress)
+            .map_err(Error::BlocklistClient)
+            .map(|resp| resp.accept)
     }
 }
 
 #[cfg(test)]
 mod tests {
-    use crate::{
-        config::BlocklistClientConfig,
-        testing::context::{
-            self, BuildContext as _, ConfigureMockedClients, ConfigureSettings,
-            ConfigureStorage as _,
-        },
-    };
+    use crate::config::BlocklistClientConfig;
 
     use super::*;
     use mockito::{Server, ServerGuard};
@@ -179,15 +197,10 @@ mod tests {
     fn try_from_url_with_slash() {
         let endpoint = Url::parse("http://localhost:8080/").unwrap();
 
-        let ctx = context::TestContext::builder()
-            .modify_settings(|config| {
-                config.blocklist_client = Some(BlocklistClientConfig { endpoint })
-            })
-            .with_in_memory_storage()
-            .with_mocked_clients()
-            .build();
-
-        let client = BlocklistClient::new(&ctx).unwrap();
+        let client = BlocklistClient::new(&BlocklistClientConfig {
+            endpoint,
+            retry_delay: Duration::ZERO,
+        });
 
         assert_eq!(client.config.base_path, "http://localhost:8080");
     }
@@ -196,15 +209,10 @@ mod tests {
     fn try_from_url_without_slash() {
         let endpoint = Url::parse("http://localhost:8080").unwrap();
 
-        let ctx = context::TestContext::builder()
-            .modify_settings(|config| {
-                config.blocklist_client = Some(BlocklistClientConfig { endpoint })
-            })
-            .with_in_memory_storage()
-            .with_mocked_clients()
-            .build();
-
-        let client = BlocklistClient::new(&ctx).unwrap();
+        let client = BlocklistClient::new(&BlocklistClientConfig {
+            endpoint,
+            retry_delay: Duration::ZERO,
+        });
 
         assert_eq!(client.config.base_path, "http://localhost:8080");
     }

signer/src/config/default.toml

@@ -14,6 +14,12 @@
 # [blocklist_client]
 # endpoint = "http://127.0.0.1:8080"
 
+# The delay, in milliseconds, for the retry after a blocklist client failure 
+#
+# Required: false
+# Environment: SIGNER_BLOCKLIST_CLIENT__RETRY_DELAY
+# retry_delay = 1000
+
 # !! ==============================================================================
 # !! Emily API Configuration
 # !! ==============================================================================

signer/src/config/mod.rs

@@ -15,6 +15,7 @@ use std::path::Path;
 use url::Url;
 
 use crate::config::error::SignerConfigError;
+use crate::config::serialization::duration_milliseconds_deserializer;
 use crate::config::serialization::duration_seconds_deserializer;
 use crate::config::serialization::p2p_multiaddr_deserializer_vec;
 use crate::config::serialization::parse_stacks_address;
@@ -212,8 +213,21 @@ pub struct BlocklistClientConfig {
     /// the url for the blocklist client
     #[serde(deserialize_with = "url_deserializer_single")]
     pub endpoint: Url,
+
+    /// The delay, in milliseconds, for the second retry after a blocklist
+    /// client failure
+    #[serde(
+        default = "BlocklistClientConfig::retry_delay_default",
+        deserialize_with = "duration_milliseconds_deserializer"
+    )]
+    pub retry_delay: std::time::Duration,
 }
 
+impl BlocklistClientConfig {
+    fn retry_delay_default() -> std::time::Duration {
+        std::time::Duration::from_secs(1)
+    }
+}
 /// Emily API configuration.
 #[derive(Deserialize, Clone, Debug)]
 pub struct EmilyClientConfig {
@@ -498,6 +512,8 @@ impl Settings {
     /// Perform validation on the configuration.
     fn validate(&self) -> Result<(), ConfigError> {
         self.signer.validate(self)?;
+        self.stacks.validate(self)?;
+        self.emily.validate(self)?;
 
         Ok(())
     }

signer/src/config/serialization.rs

@@ -46,6 +46,19 @@ where
     ))
 }
 
+/// A deserializer for the std::time::Duration type.
+/// Serde includes a default deserializer, but it expects a struct.
+pub fn duration_milliseconds_deserializer<'de, D>(
+    deserializer: D,
+) -> Result<std::time::Duration, D::Error>
+where
+    D: Deserializer<'de>,
+{
+    Ok(std::time::Duration::from_millis(
+        u64::deserialize(deserializer).map_err(serde::de::Error::custom)?,
+    ))
+}
+
 pub fn p2p_multiaddr_deserializer_vec<'de, D>(deserializer: D) -> Result<Vec<Multiaddr>, D::Error>
 where
     D: Deserializer<'de>,

signer/src/error.rs

@@ -3,6 +3,7 @@ use std::borrow::Cow;
 
 use blockstack_lib::types::chainstate::StacksBlockId;
 
+use crate::blocklist_client::BlocklistClientError;
 use crate::codec;
 use crate::emily_client::EmilyClientError;
 use crate::keys::PublicKey;
@@ -71,6 +72,10 @@ pub enum Error {
     #[error("emily API error: {0}")]
     EmilyApi(#[from] EmilyClientError),
 
+    /// An error occurred while communicating with the blocklist client
+    #[error("blocklist client error: {0}")]
+    BlocklistClient(#[from] BlocklistClientError),
+
     /// Attempt to fetch a bitcoin blockhash ended in an unexpected error.
     /// This is not triggered if the block is missing.
     #[error("bitcoin-core getblock RPC error for hash {1}: {0}")]

signer/src/main.rs

@@ -371,7 +371,7 @@ async fn run_request_decider(ctx: impl Context) -> Result<(), Error> {
         context: ctx.clone(),
         context_window: config.signer.context_window,
         deposit_decisions_retry_window: config.signer.deposit_decisions_retry_window,
-        blocklist_checker: BlocklistClient::new(&ctx),
+        blocklist_checker: config.blocklist_client.as_ref().map(BlocklistClient::new),
         signer_private_key: config.signer.private_key,
     };
 

signer/src/request_decider.rs

@@ -121,8 +121,9 @@ where
         Ok(())
     }
 
+    /// Vote on pending deposit requests
     #[tracing::instrument(skip_all, fields(chain_tip = tracing::field::Empty))]
-    async fn handle_new_requests(&mut self) -> Result<(), Error> {
+    pub async fn handle_new_requests(&mut self) -> Result<(), Error> {
         let requests_processing_delay = self.context.config().signer.requests_processing_delay;
         if requests_processing_delay > Duration::ZERO {
             tracing::debug!("sleeping before processing new requests");
@@ -374,11 +375,13 @@ where
             .then(|address| async { client.can_accept(&address.to_string()).await })
             .inspect_err(|error| tracing::error!(%error, "blocklist client issue"))
             .collect::<Vec<_>>()
-            .await;
+            .await
+            .into_iter()
+            .collect::<Result<Vec<_>, _>>()?;
 
         // If all of the inputs addresses are fine then we pass the deposit
         // request.
-        let can_accept = responses.into_iter().all(|res| res.unwrap_or(false));
+        let can_accept = responses.into_iter().all(|res| res);
         Ok(can_accept)
     }
 

signer/src/testing/transaction_signer.rs

@@ -9,6 +9,7 @@ use crate::context::Context;
 use crate::context::SignerEvent;
 use crate::context::SignerSignal;
 use crate::context::TxSignerEvent;
+use crate::error::Error;
 use crate::keys::PrivateKey;
 use crate::keys::PublicKey;
 use crate::network;
@@ -116,11 +117,7 @@ where
 type EventLoop<Context, M, Rng> = transaction_signer::TxSignerEventLoop<Context, M, Rng>;
 
 impl blocklist_client::BlocklistChecker for () {
-    async fn can_accept(
-        &self,
-        _address: &str,
-    ) -> Result<bool, blocklist_api::apis::Error<blocklist_api::apis::address_api::CheckAddressError>>
-    {
+    async fn can_accept(&self, _address: &str) -> Result<bool, Error> {
         Ok(true)
     }
 }