-
Notifications
You must be signed in to change notification settings - Fork 58
fix(sdk): skip TLS-dead seed nodes + shielded-sync network diagnostics #4418
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
5ac9676
2694687
753ba64
4619b56
71d5c16
f0a0df0
bedb637
190362c
70465c8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -706,14 +706,43 @@ impl ContextProvider for TrustedHttpContextProvider { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ))); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // This network refetch blocks the caller (proof verification) and | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // re-runs on every retry of the outer request, so record how long | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // it takes. `Instant` is unavailable on wasm32; those builds log | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // `elapsed_ms=None` rather than a fabricated duration. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #[cfg(not(target_arch = "wasm32"))] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let started = std::time::Instant::now(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #[cfg(not(target_arch = "wasm32"))] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let elapsed_ms = move || Some(started.elapsed().as_millis() as u64); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #[cfg(target_arch = "wasm32")] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let elapsed_ms = || None::<u64>; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| tracing::info!( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| quorum_type, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| quorum_hash = %hex::encode(quorum_hash), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "quorum cache miss; blocking refetch of quorum lists" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let this = self.clone(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let quorum = | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| dash_async::block_on(async move { this.find_quorum(quorum_type, quorum_hash).await })? | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .map_err(|e| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| debug!("Error finding quorum: {}", e); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| tracing::warn!( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| quorum_type, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| quorum_hash = %hex::encode(quorum_hash), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| elapsed_ms = ?elapsed_ms(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "quorum refetch failed: {}", e | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ContextProviderError::Generic(format!("Failed to find quorum: {}", e)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| })?; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
coderabbitai[bot] marked this conversation as resolved.
Comment on lines
726
to
746
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Suggestion: Log failures from the outer block_on result The warning and elapsed time are attached only to the inner
Suggested change
source: ['codex']
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right — the outer
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Resolved in Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| tracing::info!( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| quorum_type, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| quorum_hash = %hex::encode(quorum_hash), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| elapsed_ms = ?elapsed_ms(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "quorum refetch succeeded" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Self::parse_quorum_public_key(&quorum.key) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,7 +4,7 @@ use drive_proof_verifier::types::{ | |
| ShieldedEncryptedNote, ShieldedEncryptedNotes, ShieldedEncryptedNotesQuery, | ||
| }; | ||
| use rs_dapi_client::RequestSettings; | ||
| use tracing::debug; | ||
| use tracing::{info, warn}; | ||
|
|
||
| /// Fetch a single chunk of encrypted notes from the network. | ||
| /// | ||
|
|
@@ -28,21 +28,43 @@ pub async fn fetch_chunk( | |
| count: chunk_size as u32, | ||
| }; | ||
|
|
||
| debug!(chunk_start, chunk_size, "fetching shielded notes chunk"); | ||
| info!(chunk_start, chunk_size, "fetching shielded notes chunk"); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not sure why this should be info.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair — downgraded both to |
||
|
|
||
| let (result, metadata) = | ||
| ShieldedEncryptedNotes::fetch_with_metadata(sdk, query, Some(settings)).await?; | ||
| // `Instant` is unavailable on wasm32; a chunk fetched there logs | ||
| // `elapsed_ms=None` rather than a fabricated duration. | ||
| #[cfg(not(target_arch = "wasm32"))] | ||
| let started = std::time::Instant::now(); | ||
| #[cfg(not(target_arch = "wasm32"))] | ||
| let elapsed_ms = move || Some(started.elapsed().as_millis() as u64); | ||
| #[cfg(target_arch = "wasm32")] | ||
| let elapsed_ms = || None::<u64>; | ||
|
|
||
| let fetched = ShieldedEncryptedNotes::fetch_with_metadata(sdk, query, Some(settings)).await; | ||
|
|
||
| let (result, metadata) = match fetched { | ||
| Ok(v) => v, | ||
| Err(e) => { | ||
| warn!( | ||
| chunk_start, | ||
| elapsed_ms = ?elapsed_ms(), | ||
| error = %e, | ||
| "shielded notes chunk fetch failed" | ||
| ); | ||
| return Err(e); | ||
| } | ||
| }; | ||
|
|
||
| let (notes, total_count) = match result { | ||
| Some(ShieldedEncryptedNotes { notes, total_count }) => (notes, total_count), | ||
| None => (Vec::new(), 0), | ||
| }; | ||
|
|
||
| debug!( | ||
| info!( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this should be info.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same fix — |
||
| chunk_start, | ||
| notes_returned = notes.len(), | ||
| block_height = metadata.height, | ||
| total_count, | ||
| elapsed_ms = ?elapsed_ms(), | ||
| "shielded notes chunk fetched" | ||
| ); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -95,27 +95,67 @@ const DEFAULT_REQUEST_SETTINGS: RequestSettings = RequestSettings { | |||||||||||||||||||||||||||||||||||||||||||||||||
| /// Malformed upstream entries are silently skipped rather than panicking; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| /// the DAPI client handles retry/rotation across the remaining addresses. | ||||||||||||||||||||||||||||||||||||||||||||||||||
| /// | ||||||||||||||||||||||||||||||||||||||||||||||||||
| /// Seeds whose recorded Platform TLS probe shows a certificate that this | ||||||||||||||||||||||||||||||||||||||||||||||||||
| /// client's rustls stack would deterministically reject (`Expired`, | ||||||||||||||||||||||||||||||||||||||||||||||||||
| /// `SelfSigned`, `Untrusted`, `NoHandshake`) are skipped: every connect to | ||||||||||||||||||||||||||||||||||||||||||||||||||
| /// them fails the handshake, so keeping them in rotation only costs | ||||||||||||||||||||||||||||||||||||||||||||||||||
| /// retry/ban churn. `Valid` and `Unknown` (not probed) are kept. If the | ||||||||||||||||||||||||||||||||||||||||||||||||||
| /// filter would empty the list (e.g. a seed file with all-stale probes), | ||||||||||||||||||||||||||||||||||||||||||||||||||
| /// it falls back to the unfiltered set so the client can still bootstrap | ||||||||||||||||||||||||||||||||||||||||||||||||||
| /// and let runtime banning sort it out. | ||||||||||||||||||||||||||||||||||||||||||||||||||
| /// | ||||||||||||||||||||||||||||||||||||||||||||||||||
| /// ## Panics | ||||||||||||||||||||||||||||||||||||||||||||||||||
| /// | ||||||||||||||||||||||||||||||||||||||||||||||||||
| /// Panics on networks other than `Mainnet` and `Testnet` — no upstream | ||||||||||||||||||||||||||||||||||||||||||||||||||
| /// seed list exists for devnet/regtest. | ||||||||||||||||||||||||||||||||||||||||||||||||||
| fn default_address_list_for_network(network: Network) -> AddressList { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| use dash_network_seeds::SslStatus; | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| if !matches!(network, Network::Mainnet | Network::Testnet) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| panic!("default address list is only available for mainnet and testnet"); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
| let mut list = AddressList::new(); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| for seed in dash_network_seeds::evo_seeds(network) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| let Some(port) = seed.platform_http_port else { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| continue; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| let url = format!("https://{}:{}", seed.address.ip(), port); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| if let Ok(uri) = url.parse::<Uri>() { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| if let Ok(address) = Address::try_from(uri) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| list.add(address); | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| let seeds = dash_network_seeds::evo_seeds(network); | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| let build = |skip_bad_tls: bool| -> AddressList { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| let mut list = AddressList::new(); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| for seed in &seeds { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| let Some(port) = seed.platform_http_port else { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| continue; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| if skip_bad_tls { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| let ssl = seed.platform.as_ref().map(|p| p.ssl); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| if matches!( | ||||||||||||||||||||||||||||||||||||||||||||||||||
| ssl, | ||||||||||||||||||||||||||||||||||||||||||||||||||
| Some( | ||||||||||||||||||||||||||||||||||||||||||||||||||
| SslStatus::Expired | ||||||||||||||||||||||||||||||||||||||||||||||||||
| | SslStatus::SelfSigned | ||||||||||||||||||||||||||||||||||||||||||||||||||
| | SslStatus::Untrusted | ||||||||||||||||||||||||||||||||||||||||||||||||||
| | SslStatus::NoHandshake | ||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||
| ) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| continue; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Suggestion: Do not classify every NoHandshake probe as a deterministic TLS failure The upstream probe uses
Suggested change
source: ['codex']
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch — confirmed in the prober:
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Resolved in Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread. |
||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
| let url = format!("https://{}:{}", seed.address.ip(), port); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| if let Ok(uri) = url.parse::<Uri>() { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| if let Ok(address) = Address::try_from(uri) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| list.add(address); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
| list | ||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| let filtered = build(true); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| if filtered.is_empty() { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| tracing::warn!( | ||||||||||||||||||||||||||||||||||||||||||||||||||
| ?network, | ||||||||||||||||||||||||||||||||||||||||||||||||||
| "all seed entries have failing TLS probes; falling back to unfiltered seed list" | ||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| return build(false); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
| list | ||||||||||||||||||||||||||||||||||||||||||||||||||
| filtered | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| /// Dash Platform SDK | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 Suggestion: Honor the caller-selected log level for new targets
platform_wallet_enable_file_loggingaccepts an explicit level and every existing target directive uses the resultinglog_level, but the newly addedrs_dapi_clientandrs_sdk_trusted_context_providerdirectives forcedebug. The same forced directives are repeated in the stdout filter at lines 161–162. Consequently, callers selectingerror,warn, orinfostill execute and emit potentially high-volume request, retry, ban, and provider debug events. Use{log_level}for both targets in both filters; the Swift and Kotlin APIs already default todebug, so normal diagnostic sessions retain these events while lower verbosity settings work as advertised.source: ['codex']
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point. 4619b56 adds a
diag_levelhelper: the two diagnostic targets getdebugas a floor (their useful events — request execution, ban/unban, quorum cache misses — sit at debug), but a caller selectingtracenow getstrace. Lower global levels intentionally don't mute them — these directives exist precisely so field diagnostics from the app (which logs atinfo) capture the network path.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved in
4619b56— Honor the caller-selected log level for new targets no longer present.Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.