diff --git a/packages/rs-sdk/src/error.rs b/packages/rs-sdk/src/error.rs index 2254324cc62..cd599322a48 100644 --- a/packages/rs-sdk/src/error.rs +++ b/packages/rs-sdk/src/error.rs @@ -357,10 +357,14 @@ where impl CanRetry for Error { fn can_retry(&self) -> bool { - matches!( - self, - Error::StaleNode(..) | Error::TimeoutReached(_, _) | Error::Proof(_) - ) + match self { + // Not a node failure: the Sdk's own chain id may be the outdated one. + // `sync::retry` still fails over to another node, with a short flat exclusion + // instead of a health ban. + Error::StaleNode(StaleNodeError::ChainIdMismatch { .. }) => false, + Error::StaleNode(..) | Error::TimeoutReached(_, _) | Error::Proof(_) => true, + _ => false, + } } fn is_no_available_addresses(&self) -> bool { @@ -397,6 +401,23 @@ pub enum StaleNodeError { /// Tolerance in milliseconds tolerance_ms: u64, }, + /// Server returned metadata of another chain than the one the Sdk expects + /// + /// The chain id is covered by the quorum signature, so this means the server + /// proved state of another Platform chain, e.g. one from before a network reset. + /// + /// Unlike the other variants, this error is not retryable: the Sdk fails over to + /// other servers with a short flat exclusion instead of the exponential health + /// ban, and returns this error if none of them serves the expected chain. + /// + /// See [`SdkBuilder::with_expected_chain_id`](crate::SdkBuilder::with_expected_chain_id). + #[error("received chain id is different: expected {expected}, received {received}; try another server")] + ChainIdMismatch { + /// Chain id the Sdk was configured with + expected: String, + /// Chain id received from the server + received: String, + }, /// Server kept reporting a current epoch that its own proofs contradict /// /// The epoch index in response metadata is not covered by the quorum diff --git a/packages/rs-sdk/src/mock/sdk.rs b/packages/rs-sdk/src/mock/sdk.rs index 7f13aeb991c..ca2013f2173 100644 --- a/packages/rs-sdk/src/mock/sdk.rs +++ b/packages/rs-sdk/src/mock/sdk.rs @@ -552,11 +552,18 @@ impl MockDashPlatformSdk { let data = match self.from_proof_expectations.get(&key) { // Report the latest protocol version so the proof path's ratchet // (`maybe_update_protocol_version`) fires as it would against a real - // network; `default()` reports 0, which the ratchet ignores. + // network; `default()` reports 0, which the ratchet ignores. Report the + // expected chain id, if any, so that the chain id check passes. Some(d) => ( Option::::mock_deserialize(self, d), ResponseMetadata { protocol_version: dpp::version::LATEST_VERSION, + chain_id: self + .sdk + .load() + .as_ref() + .and_then(|sdk| sdk.expected_chain_id.clone()) + .unwrap_or_default(), ..Default::default() }, Proof::default(), diff --git a/packages/rs-sdk/src/platform/transition/broadcast.rs b/packages/rs-sdk/src/platform/transition/broadcast.rs index 73d1a107e98..8782fc74c36 100644 --- a/packages/rs-sdk/src/platform/transition/broadcast.rs +++ b/packages/rs-sdk/src/platform/transition/broadcast.rs @@ -534,8 +534,9 @@ impl WaitForOutcome for StateTransition { .inner; // `metadata` is quorum-authenticated only after the verification above, so the - // protocol-version ratchet must run here, never before. A `StaleNode` error is - // retryable and prompts another server. + // protocol-version ratchet must run here, never before. A `StaleNode` error + // prompts another server: most variants are retryable, and `sync::retry` fails + // over on `ChainIdMismatch` too. let _: () = sdk .verify_response_metadata("wait_for_state_transition_result", &metadata) .wrap_to_execution_result(&response)? diff --git a/packages/rs-sdk/src/sdk.rs b/packages/rs-sdk/src/sdk.rs index cd8769b136a..238e140e53a 100644 --- a/packages/rs-sdk/src/sdk.rs +++ b/packages/rs-sdk/src/sdk.rs @@ -252,6 +252,11 @@ pub struct Sdk { /// See [SdkBuilder::with_time_tolerance] for more information. metadata_time_tolerance_ms: Option, + /// Chain id that response metadata must carry. + /// + /// See [SdkBuilder::with_expected_chain_id] for more information. + pub(crate) expected_chain_id: Option, + /// Cancellation token; once cancelled, all pending requests should be aborted. pub(crate) cancel_token: CancellationToken, @@ -276,6 +281,7 @@ impl Clone for Sdk { metadata_last_seen_height: Arc::clone(&self.metadata_last_seen_height), metadata_height_tolerance: self.metadata_height_tolerance, metadata_time_tolerance_ms: self.metadata_time_tolerance_ms, + expected_chain_id: self.expected_chain_id.clone(), dapi_client_settings: self.dapi_client_settings, #[cfg(feature = "mocks")] dump_dir: self.dump_dir.clone(), @@ -368,6 +374,16 @@ impl Sdk { method_name: &str, metadata: &ResponseMetadata, ) -> Result<(), Error> { + // A response from another chain must not advance any state below. + if let Some(expected) = &self.expected_chain_id { + if metadata.chain_id != *expected { + return Err(StaleNodeError::ChainIdMismatch { + expected: expected.clone(), + received: metadata.chain_id.clone(), + } + .into()); + } + } let (metadata_height_tolerance, metadata_time_tolerance_ms) = self.freshness_criteria(method_name); // Check the independent local-clock anchor before mutating the @@ -863,6 +879,11 @@ pub struct SdkBuilder { /// monotonic freshness high-water mark. trusted_initial_height: Option, + /// Chain id that response metadata must carry. + /// + /// See [SdkBuilder::with_expected_chain_id] for more information. + expected_chain_id: Option, + /// directory where dump files will be stored #[cfg(feature = "mocks")] dump_dir: Option, @@ -891,6 +912,7 @@ impl Default for SdkBuilder { metadata_height_tolerance: Some(1), metadata_time_tolerance_ms: None, trusted_initial_height: None, + expected_chain_id: None, #[cfg(feature = "mocks")] data_contract_cache_size: NonZeroUsize::new(DEFAULT_CONTRACT_CACHE_SIZE) @@ -1168,6 +1190,23 @@ impl SdkBuilder { self } + /// Require response metadata to carry the given Tenderdash chain id. + /// + /// The chain id is covered by the quorum signature, but the quorums that sign it can be + /// the same on two Platform chains, e.g. before and after a testnet reset. A response + /// with another chain id fails with [`StaleNodeError::ChainIdMismatch`] before it can + /// advance the height high-water mark or the protocol version. The Sdk then tries + /// other servers, excluding the one that answered for a short flat 2 s rather than + /// putting it on the exponential health-ban ladder, since the configured chain id + /// may be the outdated one. + /// + /// If not set, the chain id is not checked. An empty chain id is rejected by + /// [SdkBuilder::build]. + pub fn with_expected_chain_id(mut self, chain_id: impl Into) -> Self { + self.expected_chain_id = Some(chain_id.into()); + self + } + /// Configure directory where dumps of all requests and responses will be saved. /// Useful for debugging. /// @@ -1209,6 +1248,11 @@ impl SdkBuilder { .to_string(), )); } + if self.expected_chain_id.as_deref() == Some("") { + return Err(Error::Config( + "expected chain id must not be empty".to_string(), + )); + } let dapi_client_settings = match self.settings { Some(settings) => DEFAULT_REQUEST_SETTINGS.override_by(settings), @@ -1252,6 +1296,7 @@ impl SdkBuilder { )), metadata_height_tolerance: self.metadata_height_tolerance, metadata_time_tolerance_ms: self.metadata_time_tolerance_ms, + expected_chain_id: self.expected_chain_id, #[cfg(feature = "mocks")] dump_dir: self.dump_dir, }; @@ -1324,6 +1369,7 @@ impl SdkBuilder { )), metadata_height_tolerance: self.metadata_height_tolerance, metadata_time_tolerance_ms: self.metadata_time_tolerance_ms, + expected_chain_id: self.expected_chain_id, }; let mut guard = mock_sdk.try_lock().expect("mock sdk is in use by another thread and cannot be reconfigured"); guard.set_sdk(sdk.clone()); @@ -1646,6 +1692,106 @@ mod test { ); } + fn chain_id_metadata(chain_id: &str) -> ResponseMetadata { + ResponseMetadata { + chain_id: chain_id.to_string(), + height: 10, + protocol_version: 2, + ..Default::default() + } + } + + fn mock_sdk_with_chain_id(chain_id: &str) -> super::Sdk { + let sdk = SdkBuilder::new_mock() + .with_expected_chain_id(chain_id) + .build() + .expect("mock Sdk should be created"); + sdk.protocol_version + .store(1, std::sync::atomic::Ordering::Relaxed); + sdk + } + + #[test] + fn chain_id_mismatch_is_rejected_before_any_state_advances() { + use rs_dapi_client::CanRetry; + + let sdk = mock_sdk_with_chain_id("dash-testnet-51"); + + let error = sdk + .verify_response_metadata("test", &chain_id_metadata("dash-testnet-50")) + .expect_err("a response from another chain must be rejected"); + + assert!(matches!( + &error, + crate::Error::StaleNode(super::StaleNodeError::ChainIdMismatch { expected, received }) + if expected == "dash-testnet-51" && received == "dash-testnet-50" + )); + assert!( + !error.can_retry(), + "the server must not be health-banned for serving another chain" + ); + assert_eq!(sdk.protocol_version_number(), 1); + assert_eq!( + sdk.metadata_last_seen_height + .load(std::sync::atomic::Ordering::Acquire), + 0 + ); + } + + #[test] + fn matching_chain_id_is_accepted() { + let sdk = mock_sdk_with_chain_id("dash-testnet-51"); + + sdk.verify_response_metadata("test", &chain_id_metadata("dash-testnet-51")) + .expect("metadata should be valid"); + + assert_eq!(sdk.protocol_version_number(), 2); + assert_eq!( + sdk.metadata_last_seen_height + .load(std::sync::atomic::Ordering::Acquire), + 10 + ); + } + + #[test] + fn chain_id_is_not_checked_when_not_configured() { + let sdk = SdkBuilder::new_mock() + .build() + .expect("mock Sdk should be created"); + + sdk.verify_response_metadata("test", &chain_id_metadata("dash-testnet-50")) + .expect("metadata should be valid"); + } + + #[test] + fn chain_id_is_checked_before_time() { + let sdk = SdkBuilder::new_mock() + .with_expected_chain_id("dash-testnet-51") + .with_time_tolerance(Some(1000)) + .build() + .expect("mock Sdk should be created"); + + // `chain_id_metadata` carries time 0, far outside the tolerance. + let error = sdk + .verify_response_metadata("test", &chain_id_metadata("dash-testnet-50")) + .expect_err("a response from another chain must be rejected"); + + assert!(matches!( + error, + crate::Error::StaleNode(super::StaleNodeError::ChainIdMismatch { .. }) + )); + } + + #[test] + fn empty_expected_chain_id_is_rejected() { + let error = SdkBuilder::new_mock() + .with_expected_chain_id("") + .build() + .expect_err("an empty chain id must be rejected"); + + assert!(matches!(error, crate::Error::Config(_))); + } + #[test_matrix(97..102, 100, 2, false; "valid height")] #[test_case(103, 100, 2, true; "invalid height")] fn test_verify_metadata_height( diff --git a/packages/rs-sdk/src/sync.rs b/packages/rs-sdk/src/sync.rs index 969ed9a4fb3..15c4cdc0049 100644 --- a/packages/rs-sdk/src/sync.rs +++ b/packages/rs-sdk/src/sync.rs @@ -1,6 +1,6 @@ pub use dash_async::{block_on, AsyncError}; -use crate::error::Error; +use crate::error::{Error, StaleNodeError}; use rs_dapi_client::{ transport::sleep, update_address_ban_status, AddressList, CanRetry, ExecutionResult, RequestSettings, @@ -80,7 +80,16 @@ where FutureFactoryFn: FnMut(RequestSettings) -> Fut, R: Send, { - retry_with_additional_error(address_list, settings, future_factory_fn, |_| false).await + // A server on another chain is not unhealthy, and the Sdk's own chain id may be + // the outdated one: fail over without the health-ban ladder, so that the caller + // keeps getting the mismatch instead of running out of addresses. + retry_with_additional_error(address_list, settings, future_factory_fn, |error| { + matches!( + error, + Error::StaleNode(StaleNodeError::ChainIdMismatch { .. }) + ) + }) + .await } /// Retry an operation-specific rejection only when its responding node can be @@ -181,7 +190,7 @@ where ) }); if !excluded || address_list.get_live_addresses().is_empty() { - tracing::debug!(node = ?error.address, "DPNS failover stopped: no safely excluded alternative"); + tracing::debug!(node = ?error.address, "failover stopped: no safely excluded alternative"); let mut final_error = error; final_error.retries = total_retries; return Err(final_error); @@ -216,7 +225,7 @@ where #[cfg(test)] mod test { use super::*; - use rs_dapi_client::ExecutionError; + use rs_dapi_client::{ExecutionError, ExecutionResponse}; use std::sync::{ atomic::{AtomicUsize, Ordering}, Arc, @@ -338,6 +347,69 @@ mod test { ); } + /// A chain id mismatch fails over to another node with a short flat exclusion + /// rather than an exponential health ban, so a Sdk whose own chain id is outdated + /// keeps getting the mismatch. + #[tokio::test] + async fn test_retry_chain_id_mismatch_uses_short_exclusion() { + let address_list: AddressList = ["http://localhost:1", "http://localhost:2"] + .into_iter() + .map(|address| address.parse().expect("valid address")) + .collect(); + let call_count = Arc::new(AtomicUsize::new(0)); + + let mut settings = RequestSettings::default(); + settings.retries = Some(5); + + // The second call runs while the first node is still excluded, so it has no + // other node to fail over to. + for expected_calls in [2, 1] { + call_count.store(0, Ordering::Relaxed); + let closure = |_settings: RequestSettings| { + call_count.fetch_add(1, Ordering::Relaxed); + let address = address_list.get_live_address(); + async move { + Err::, _>(ExecutionError { + inner: match address { + Some(_) => Error::StaleNode(StaleNodeError::ChainIdMismatch { + expected: "dash-testnet-51".to_string(), + received: "dash-testnet-50".to_string(), + }), + None => Error::DapiClientError(DapiClientError::NoAvailableAddresses), + }, + retries: 0, + address, + }) + } + }; + + let error = retry(&address_list, settings, closure) + .await + .expect_err("should fail"); + + assert!( + matches!( + error.inner, + Error::StaleNode(StaleNodeError::ChainIdMismatch { .. }) + ), + "expected ChainIdMismatch, got: {:?}", + error.inner + ); + assert_eq!(call_count.load(Ordering::Relaxed), expected_calls); + } + + let max_banned_until = chrono::Utc::now() + Duration::from_secs(2); + for info in address_list.ban_info() { + assert!( + info.banned_until + .is_none_or(|until| until <= max_banned_until), + "{} must not be on the health-ban ladder: {:?}", + info.uri, + info.banned_until + ); + } + } + /// Test that if we get "no available addresses" on the first call (no previous error), /// we still return it as-is (not wrapped). #[tokio::test] diff --git a/packages/rs-sdk/tests/fetch/mock_fetch.rs b/packages/rs-sdk/tests/fetch/mock_fetch.rs index 769fd7cdb97..a465864b7ab 100644 --- a/packages/rs-sdk/tests/fetch/mock_fetch.rs +++ b/packages/rs-sdk/tests/fetch/mock_fetch.rs @@ -39,6 +39,31 @@ async fn test_mock_fetch_identity() { assert_eq!(retrieved, expected); } +#[tokio::test] +/// Given a mock SDK that expects a chain id, when I fetch an expected identity, then the +/// mocked response passes the chain id check +async fn test_mock_fetch_identity_with_expected_chain_id() { + let mut sdk = SdkBuilder::new_mock() + .with_expected_chain_id("dash-testnet-51") + .build() + .expect("mock Sdk should be created"); + + let expected: Identity = Identity::from(IdentityV0::default()); + let query = expected.id(); + + sdk.mock() + .expect_fetch(query, Some(expected.clone())) + .await + .unwrap(); + + let retrieved = Identity::fetch(&sdk, query) + .await + .unwrap() + .expect("object should exist"); + + assert_eq!(retrieved, expected); +} + #[tokio::test] /// When I define mock expectation twice for the same request, second call ends with error async fn test_mock_fetch_duplicate_expectation() {