diff --git a/docker/sbtc/signer/signer-config.toml b/docker/sbtc/signer/signer-config.toml index 0dfada5a1..3ff11d197 100644 --- a/docker/sbtc/signer/signer-config.toml +++ b/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 # !! ============================================================================== diff --git a/signer/src/blocklist_client.rs b/signer/src/blocklist_client.rs index 4e9165fd4..11de6f446 100644 --- a/signer/src/blocklist_client.rs +++ b/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), +} /// 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>> + Send; + fn can_accept(&self, address: &str) -> impl Future> + 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> { - 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 { + 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 { - 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 { + // 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"); } diff --git a/signer/src/config/default.toml b/signer/src/config/default.toml index 1b82d7f01..ab340c966 100644 --- a/signer/src/config/default.toml +++ b/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 # !! ============================================================================== diff --git a/signer/src/config/mod.rs b/signer/src/config/mod.rs index cb16d1ed4..7afda7f90 100644 --- a/signer/src/config/mod.rs +++ b/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(()) } diff --git a/signer/src/config/serialization.rs b/signer/src/config/serialization.rs index 0cdd2143b..b213368ff 100644 --- a/signer/src/config/serialization.rs +++ b/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 +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, D::Error> where D: Deserializer<'de>, diff --git a/signer/src/error.rs b/signer/src/error.rs index 09153f2be..d45248fab 100644 --- a/signer/src/error.rs +++ b/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}")] diff --git a/signer/src/main.rs b/signer/src/main.rs index ad27bec3d..47446e2f7 100644 --- a/signer/src/main.rs +++ b/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, }; diff --git a/signer/src/request_decider.rs b/signer/src/request_decider.rs index 9ca608cae..83aba67f3 100644 --- a/signer/src/request_decider.rs +++ b/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::>() - .await; + .await + .into_iter() + .collect::, _>>()?; // 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) } diff --git a/signer/src/testing/transaction_signer.rs b/signer/src/testing/transaction_signer.rs index 9e4d8a860..6dcd53c63 100644 --- a/signer/src/testing/transaction_signer.rs +++ b/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 = transaction_signer::TxSignerEventLoop; impl blocklist_client::BlocklistChecker for () { - async fn can_accept( - &self, - _address: &str, - ) -> Result> - { + async fn can_accept(&self, _address: &str) -> Result { Ok(true) } } diff --git a/signer/tests/integration/request_decider.rs b/signer/tests/integration/request_decider.rs index 79334b68e..8d91a917d 100644 --- a/signer/tests/integration/request_decider.rs +++ b/signer/tests/integration/request_decider.rs @@ -1,11 +1,18 @@ +use std::sync::atomic::AtomicU8; +use std::sync::atomic::Ordering; +use std::sync::Arc; + use emily_client::apis::deposit_api; use emily_client::apis::testing_api; use emily_client::models::CreateDepositRequestBody; use fake::Fake; use fake::Faker; +use mockito::Server; use rand::SeedableRng as _; +use serde_json::json; use signer::bitcoin::MockBitcoinInteract; +use signer::blocklist_client::BlocklistClient; use signer::context::Context; use signer::emily_client::EmilyClient; use signer::emily_client::MockEmilyInteract; @@ -398,3 +405,129 @@ async fn persist_received_deposit_decision_fetches_missing_deposit_requests() { testing::storage::drop_db(db).await; } + +/// Test `RequestDeciderEventLoop` behaviour in case of blocklist client +/// failures. It should try to contact the blocklist client twice per bitcoin +/// block, and in case of errors it should try again at the next block without +/// voting. +#[test_case::test_case(0, 0; "0 failures")] +#[test_case::test_case(1, 0; "1 failure")] +#[test_case::test_case(2, 1; "2 failures")] +#[test_case::test_case(3, 1; "3 failures")] +#[test_case::test_case(4, 2; "4 failures")] +#[tokio::test] +async fn blocklist_client_retry(num_failures: u8, failing_iters: u8) { + let db = testing::storage::new_test_database().await; + let network = InMemoryNetwork::new(); + + let mut rng = rand::rngs::StdRng::seed_from_u64(51); + + let ctx = TestContext::builder() + .with_storage(db.clone()) + .with_mocked_clients() + .build(); + + let (rpc, faucet) = sbtc::testing::regtest::initialize_blockchain(); + + // This confirms a deposit transaction, and has a nice helper function + // for storing a real deposit. + let setup = TestSweepSetup::new_setup(rpc, faucet, 10000, &mut rng); + + // Let's get the blockchain data into the database. + let chain_tip: BitcoinBlockHash = setup.sweep_block_hash.into(); + backfill_bitcoin_blocks(&db, rpc, &chain_tip).await; + + // We need to store the deposit request because of the foreign key + // constraint on the deposit_signers table. + setup.store_deposit_request(&db).await; + + // In order to fetch the deposit request that we just stored, we need to + // store the deposit transaction. + setup.store_deposit_tx(&db).await; + + // We check if the current signer is in the signing set. For this check we + // need a row in the dkg_shares table. + setup.store_dkg_shares(&db).await; + + let signer_public_key = setup.aggregated_signer.keypair.public_key().into(); + let mut requests = db + .get_pending_deposit_requests(&chain_tip, 100, &signer_public_key) + .await + .unwrap(); + // There should only be the one deposit request that we just fetched. + assert_eq!(requests.len(), 1); + let request = requests.pop().unwrap(); + let outpoint = setup.deposit_request.outpoint; + + let bitcoin_network = bitcoin::Network::from(ctx.config().signer.network); + let sender_address = + bitcoin::Address::from_script(&request.sender_script_pub_keys[0], bitcoin_network.params()) + .unwrap(); + + // Now we mock the blocklist client: we want it to fail for the first + // `num_failures` calls, then succeed (with the following mock) + let mut blocklist_server = Server::new_async().await; + let mock_json = json!({ + "is_blocklisted": false, + "severity": "Low", + "accept": true, + "reason": null + }) + .to_string(); + + let counter = Arc::new(AtomicU8::new(0)); + blocklist_server + .mock("GET", format!("/screen/{sender_address}").as_str()) + .match_request(move |_| counter.fetch_add(1, Ordering::SeqCst) >= num_failures) + .with_status(200) + .with_header("content-type", "application/json") + .with_body(&mock_json) + .create_async() + .await; + + let blocklist_client = BlocklistClient::with_base_url(blocklist_server.url()); + + let mut request_decider = RequestDeciderEventLoop { + network: network.connect(), + context: ctx.clone(), + context_window: 10000, + blocklist_checker: Some(blocklist_client), + signer_private_key: setup.aggregated_signer.keypair.secret_key().into(), + deposit_decisions_retry_window: 1, + }; + + // We need this so that there is a live "network". Otherwise we will error + // when trying to send a message at the end. + let _rec = ctx.get_signal_receiver(); + + // We shouldn't have any decision at the beginning + let votes = db + .get_deposit_signers(&outpoint.txid.into(), outpoint.vout) + .await + .unwrap(); + assert!(votes.is_empty()); + + // Iterations with failing blocklist client + for _ in 0..failing_iters { + request_decider.handle_new_requests().await.unwrap(); + + // We shouldn't have any decision yet + let votes = db + .get_deposit_signers(&outpoint.txid.into(), outpoint.vout) + .await + .unwrap(); + assert!(votes.is_empty()); + } + + // Final iteration with (at least one) blocklist success + request_decider.handle_new_requests().await.unwrap(); + + // A decision should get stored and there should only be one + let votes = db + .get_deposit_signers(&outpoint.txid.into(), outpoint.vout) + .await + .unwrap(); + assert_eq!(votes.len(), 1); + + testing::storage::drop_db(db).await; +}