fix: replace deadlocking futures::executor::block_on with runtime-aware dash-async crate (#3432)#3497
Conversation
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The initial extraction only kept the multi-thread block_in_place path. Restore the 3-way handling: no runtime (create temporary), current-thread (spawn OS thread), multi-thread (block_in_place). Also restore the current-thread runtime regression test and add missing tokio sync feature for dev-dependencies. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 32 minutes and 35 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughA new Changes
Sequence DiagramsequenceDiagram
participant Caller as Sync Caller
participant BlockOn as block_on()
participant TokioRT as Tokio Runtime<br/>(Detector)
participant NewRT as New RT<br/>(No-Runtime Path)
participant Worker as OS Thread<br/>(CurrentThread Path)
participant PlaceSpawn as block_in_place<br/>(MultiThread Path)
Caller->>BlockOn: block_on(future)
BlockOn->>TokioRT: Check current handle
alt No Tokio Runtime
TokioRT-->>BlockOn: try_current() → None
BlockOn->>NewRT: Create new_current_thread
NewRT->>NewRT: Run future to completion
NewRT-->>BlockOn: Output
else Tokio CurrentThread
TokioRT-->>BlockOn: CurrentThread detected
BlockOn->>Worker: Spawn dedicated OS thread
Worker->>Worker: Create isolated<br/>current-thread RT
Worker->>Worker: Await future
Worker->>BlockOn: Send Output via MPSC
BlockOn-->>BlockOn: Join thread
else Tokio MultiThread
TokioRT-->>BlockOn: MultiThread detected
BlockOn->>PlaceSpawn: block_in_place + spawn
PlaceSpawn->>PlaceSpawn: Await spawned future
PlaceSpawn->>BlockOn: Receive Output via MPSC
BlockOn-->>BlockOn: Check task completion
end
BlockOn-->>Caller: Result<Output, AsyncError>
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
lklimek
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
packages/rs-dash-async/src/block_on.rs (2)
87-97: Minor inconsistency in channel type selection.The current-thread path uses
sync_channel::<...>(1)while the multi-thread path uses unboundedchannel(). Both work correctly since only one message is sent, but usingsync_channel(1)consistently would be slightly more explicit about the expected single-message behavior.This is a very minor style observation and doesn't affect correctness.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dash-async/src/block_on.rs` around lines 87 - 97, In block_on's multi-thread branch (the match arm using handle.spawn(worker(fut, tx))), replace the unbounded std::sync::mpsc::channel() with a bounded std::sync::mpsc::sync_channel(1) to mirror the current-thread path and make the single-message intent explicit; ensure you create tx/rx with the same generic type as the other branch (e.g., sync_channel::<...>(1)) so the spawned worker(fut, tx), rx.recv(), and the hdl handling remain type-compatible.
209-216: Test name is misleading - it tests success, not failure.The test
test_block_on_fails_on_current_thread_runtimeactually verifies thatblock_onsucceeds on a current-thread runtime (the fix for the bug). Consider renaming to something liketest_block_on_succeeds_on_current_thread_runtimeto match the assertion on line 259-263.✏️ Suggested rename
- fn test_block_on_fails_on_current_thread_runtime() { + fn test_block_on_succeeds_on_current_thread_runtime() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dash-async/src/block_on.rs` around lines 209 - 216, The test function name test_block_on_fails_on_current_thread_runtime is misleading because the body asserts success; rename the test function to test_block_on_succeeds_on_current_thread_runtime and update any references (doc comment or CI/test filters) accordingly so the name matches the assertion and comment describing the regression fix in the surrounding block_on test code.packages/rs-sdk-trusted-context-provider/src/provider.rs (2)
631-646: Consider extracting public key parsing into a helper method.The public key parsing logic (hex decode, length validation, array conversion) is duplicated three times in
get_quorum_public_key: once for the current cache hit (lines 568-584), once for the previous cache hit (lines 594-610), and here for the refetch path.♻️ Proposed helper extraction
impl TrustedHttpContextProvider { /// Parse a BLS public key from hex string fn parse_quorum_public_key(key: &str) -> Result<[u8; 48], ContextProviderError> { let pubkey_hex = key.trim_start_matches("0x"); let pubkey_bytes = hex::decode(pubkey_hex).map_err(|e| { ContextProviderError::Generic(format!("Invalid hex in public key: {}", e)) })?; if pubkey_bytes.len() != 48 { return Err(ContextProviderError::Generic(format!( "Invalid public key length: {} bytes, expected 48", pubkey_bytes.len() ))); } pubkey_bytes.try_into().map_err(|_| { ContextProviderError::Generic("Failed to convert public key to array".to_string()) }) } }Then each call site becomes:
return Self::parse_quorum_public_key(&quorum.key);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-sdk-trusted-context-provider/src/provider.rs` around lines 631 - 646, Extract the duplicated public-key parsing logic into a new helper method on TrustedHttpContextProvider (e.g., fn parse_quorum_public_key(key: &str) -> Result<[u8;48], ContextProviderError>) that performs trim_start_matches("0x"), hex::decode with ContextProviderError::Generic on decode errors, checks pubkey_bytes.len() == 48 and converts to [u8;48] returning the same ContextProviderError::Generic on conversion failure; then replace each duplicated block in get_quorum_public_key with a call to Self::parse_quorum_public_key(&quorum.key) to DRY up the code and preserve existing error behavior.
622-629: Consider using the existingFrom<AsyncError>conversion.The error from
block_onis manually mapped toContextProviderError::Generic, but there's already aFrom<dash_async::AsyncError> for ContextProviderErrorimpl inrs-context-provider/src/error.rsthat maps to the semanticAsyncErrorvariant. Using the?operator with theFromimpl would preserve the error categorization.♻️ Proposed refactor to use From impl
let this = self.clone(); let quorum = dash_async::block_on(async move { this.find_quorum(quorum_type, quorum_hash).await }) - .map_err(|e| ContextProviderError::Generic(format!("block_on failed: {}", e)))? + ? // Uses From<AsyncError> for ContextProviderError .map_err(|e| { debug!("Error finding quorum: {}", e); ContextProviderError::Generic(format!("Failed to find quorum: {}", e)) })?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-sdk-trusted-context-provider/src/provider.rs` around lines 622 - 629, The block_on error is being manually mapped to ContextProviderError::Generic, but there is a From<dash_async::AsyncError> -> ContextProviderError impl; replace the outer map_err mapping with the ? operator so the dash_async::block_on(...) error converts via From automatically. Specifically, change the block that calls dash_async::block_on(async move { this.find_quorum(quorum_type, quorum_hash).await }) .map_err(|e| ContextProviderError::Generic(...))? to simply use ? on the block_on result (i.e., dash_async::block_on(...)?), then keep the existing inner map_err that handles the find_quorum application error and debugging log; reference symbols: block_on, find_quorum, ContextProviderError, and dash_async::AsyncError.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/rs-dash-async/src/block_on.rs`:
- Around line 87-97: In block_on's multi-thread branch (the match arm using
handle.spawn(worker(fut, tx))), replace the unbounded std::sync::mpsc::channel()
with a bounded std::sync::mpsc::sync_channel(1) to mirror the current-thread
path and make the single-message intent explicit; ensure you create tx/rx with
the same generic type as the other branch (e.g., sync_channel::<...>(1)) so the
spawned worker(fut, tx), rx.recv(), and the hdl handling remain type-compatible.
- Around line 209-216: The test function name
test_block_on_fails_on_current_thread_runtime is misleading because the body
asserts success; rename the test function to
test_block_on_succeeds_on_current_thread_runtime and update any references (doc
comment or CI/test filters) accordingly so the name matches the assertion and
comment describing the regression fix in the surrounding block_on test code.
In `@packages/rs-sdk-trusted-context-provider/src/provider.rs`:
- Around line 631-646: Extract the duplicated public-key parsing logic into a
new helper method on TrustedHttpContextProvider (e.g., fn
parse_quorum_public_key(key: &str) -> Result<[u8;48], ContextProviderError>)
that performs trim_start_matches("0x"), hex::decode with
ContextProviderError::Generic on decode errors, checks pubkey_bytes.len() == 48
and converts to [u8;48] returning the same ContextProviderError::Generic on
conversion failure; then replace each duplicated block in get_quorum_public_key
with a call to Self::parse_quorum_public_key(&quorum.key) to DRY up the code and
preserve existing error behavior.
- Around line 622-629: The block_on error is being manually mapped to
ContextProviderError::Generic, but there is a From<dash_async::AsyncError> ->
ContextProviderError impl; replace the outer map_err mapping with the ? operator
so the dash_async::block_on(...) error converts via From automatically.
Specifically, change the block that calls dash_async::block_on(async move {
this.find_quorum(quorum_type, quorum_hash).await }) .map_err(|e|
ContextProviderError::Generic(...))? to simply use ? on the block_on result
(i.e., dash_async::block_on(...)?), then keep the existing inner map_err that
handles the find_quorum application error and debugging log; reference symbols:
block_on, find_quorum, ContextProviderError, and dash_async::AsyncError.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 56379006-6e11-4e56-a33c-7609591618e8
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
Cargo.tomlpackages/rs-context-provider/Cargo.tomlpackages/rs-context-provider/src/error.rspackages/rs-dash-async/Cargo.tomlpackages/rs-dash-async/src/block_on.rspackages/rs-dash-async/src/lib.rspackages/rs-sdk-trusted-context-provider/Cargo.tomlpackages/rs-sdk-trusted-context-provider/src/provider.rspackages/rs-sdk/Cargo.tomlpackages/rs-sdk/src/sync.rs
There was a problem hiding this comment.
Pull request overview
Fixes the deadlock caused by using futures::executor::block_on inside a Tokio runtime by introducing a runtime-aware async→sync bridging implementation (dash-async) and migrating the trusted context provider to use it.
Changes:
- Added new
dash-asynccrate providing a runtime-flavor-awareblock_on+AsyncError(including a WASM stub and regression tests for #3432). - Updated
TrustedHttpContextProviderto usedash_async::block_on(and made itCloneby switchingfallback_providertoArc). - Updated
dash-sdk::syncto re-exportblock_on/AsyncError, and movedFrom<AsyncError>conversion tors-context-provider.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/rs-sdk/src/sync.rs | Re-exports dash-async bridge types; removes local implementation/tests. |
| packages/rs-sdk/Cargo.toml | Adds dash-async dependency. |
| packages/rs-sdk-trusted-context-provider/src/provider.rs | Replaces futures::executor::block_on with dash_async::block_on; makes provider clonable via Arc fallback. |
| packages/rs-sdk-trusted-context-provider/Cargo.toml | Adds dash-async, removes futures. |
| packages/rs-dash-async/src/lib.rs | New crate entry point exporting block_on + AsyncError. |
| packages/rs-dash-async/src/block_on.rs | Runtime-aware block_on implementation + tests (incl. #3432 regression). |
| packages/rs-dash-async/Cargo.toml | New crate manifest and dependencies. |
| packages/rs-context-provider/src/error.rs | Adds From<dash_async::AsyncError> to map to ContextProviderError::AsyncError. |
| packages/rs-context-provider/Cargo.toml | Adds dash-async dependency. |
| Cargo.toml | Adds rs-dash-async to workspace members. |
| Cargo.lock | Records new crate and dependency graph updates. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
✅ Review complete (commit 5787e41) |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3497 +/- ##
============================================
- Coverage 84.84% 84.83% -0.01%
============================================
Files 2476 2477 +1
Lines 267915 267872 -43
============================================
- Hits 227303 227242 -61
- Misses 40612 40630 +18
🚀 New features to boost your workflow:
|
|
✅ DashSDKFFI.xcframework built for this PR.
SwiftPM (host the zip at a stable URL, then use): .binaryTarget(
name: "DashSDKFFI",
url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
checksum: "c5852c74f5dd9cc0e525ee97e185b055e059fb14b31eb9c5982464a5925a1968"
)Xcode manual integration:
|
- dash-async: always join worker thread even when rx.recv() fails, so panics aren't swallowed and we don't leak detached threads - dash-async: rename test_block_on_fails_on_current_thread_runtime to test_block_on_succeeds_on_current_thread_runtime to match its assertion - dash-async: use bounded sync_channel(1) in the multi-thread branch for consistency with the current-thread branch (only one message flows) - trusted-context-provider: rely on From<AsyncError> for ContextProviderError via ? rather than wrapping in Generic, so async-bridging failures keep their semantic AsyncError variant - trusted-context-provider: extract parse_quorum_public_key helper to DRY up three duplicate "hex-decode BLS key" blocks in get_quorum_public_key Addresses copilot-pull-request-reviewer and coderabbit nits on PR #3497. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The Rust workspace coverage job enumerates packages explicitly for `cargo llvm-cov nextest`, and the new `dash-async` crate was missing. As a result the multi-thread `block_on` branch and its `worker` fn were only reached transitively (without running the crate's own tests), dropping patch coverage on packages/rs-dash-async/src/block_on.rs to 46.77% and failing codecov/patch on PR #3497. Add `--package dash-async` so its tests run under coverage and the existing `test_block_on_nested_async_sync` / current-thread tests cover the full matrix. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The WASM stub never drives the future (always returns AsyncError::Generic), so the native `F: Future + Send + 'static` / `F::Output: Send` bounds were purely a compile-time obstacle: WASM is single-threaded, and WASM-only async types — notably `JsFuture` (which wraps `Rc<RefCell<..>>`) and reqwest's `wasm::Response` — are never `Send`. Dropping the bounds lets legitimate callers like `rs-sdk-trusted-context-provider::provider::get_quorum_public_key` compile on wasm32. The native `block_on` keeps its full bound set. Fixes "Build JS packages / Build JS" on PR #3497 where rs-sdk-trusted- context-provider failed with E0277 "Rc<RefCell<..Inner>> cannot be sent between threads safely" at provider.rs:610. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
.github/workflows/tests-rs-workspace.yml (1)
152-152: Consider mirroring--package dash-asyncin the Ubuntu backup job for parity.The macOS nextest run now includes
dash-async, but the Ubuntu fallbacktest-ubuntujob (lines 312–335) still uses an explicit package list that omits it. When the Mac runner is unavailable and the Ubuntu backup kicks in,dash-asynctests — including the new current-thread deadlock regression for issue#3432— will silently not run. Adding--package dash-asyncto the Ubuntu list would keep both paths in lockstep.🔧 Suggested change to test-ubuntu
--package dash-sdk \ + --package dash-async \ --package platform-value \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/tests-rs-workspace.yml at line 152, The Ubuntu backup job test-ubuntu is missing the dash-async package that the macOS nextest run includes; update the test command in the test-ubuntu job to include the same --package dash-async flag so the Ubuntu fallback runs the same suite (including the current-thread deadlock regression for issue `#3432`) and keep both jobs in parity with the macOS nextest invocation.packages/rs-dash-async/src/block_on.rs (1)
64-105: LGTM on the three-branch runtime-aware bridge.The flavor detection and the separate strategy per scenario (temp runtime / dedicated OS thread /
block_in_place+spawn) is sound, and the regression test for the current-thread case exercises the right path. One small observability nit in the current-thread branch: when the worker thread panics, thejoin()error payload is dropped and the caller only sees the generic"block_on worker thread panicked"string. If the root cause is, for example, a panic inside the user future, that message is lost and makes debugging harder. Consider extracting the panic message before discarding, e.g.:🔧 Preserve panic message from the worker thread
- match (join_result, recv_result) { - (Err(_), _) => Err(AsyncError::Generic( - "block_on worker thread panicked".to_string(), - )), + match (join_result, recv_result) { + (Err(payload), _) => { + let msg = payload + .downcast_ref::<&'static str>() + .map(|s| (*s).to_string()) + .or_else(|| payload.downcast_ref::<String>().cloned()) + .unwrap_or_else(|| "unknown panic payload".to_string()); + Err(AsyncError::Generic(format!( + "block_on worker thread panicked: {msg}" + ))) + } (Ok(()), Err(_)) => Err(AsyncError::Generic( "block_on worker exited without sending a result".to_string(), )), (Ok(()), Ok(result)) => result, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dash-async/src/block_on.rs` around lines 64 - 105, The current-thread branch of block_on drops the panic payload from join_handle.join() and returns a generic AsyncError; modify the join_result handling in block_on to extract the panic payload from the Err(Box<dyn Any + Send>) returned by join_handle.join(), convert it to a readable string (try downcasting to &str and String, falling back to a debug format), and include that extracted message in the AsyncError::Generic returned when (join_result, _) is (Err(_), _); keep the rest of the match logic unchanged and reference join_handle.join(), join_result, and AsyncError::Generic to locate the changes.packages/rs-sdk-trusted-context-provider/src/provider.rs (1)
608-616: block_on bridge wiring is correct; minor error-context nit.The double-
?layering (outer convertsAsyncErrorviaFrom, innermap_errwrapsTrustedContextProviderErrorintoGeneric) is correct and theself.clone()gives you the required'staticfuture cheaply via theArcfields. One small thing:debug!("Error finding quorum: {}", e)followed by wrapping intoGeneric(format!("Failed to find quorum: {}", e))loses the structured error type at the boundary — ifTrustedContextProviderErrorhas more specific variants (e.g.QuorumNotFound,NetworkError) that callers might want to distinguish, consider either mapping them to the correspondingContextProviderErrorvariants or adding a#[from]-based conversion inTrustedContextProviderError → ContextProviderError. Not blocking; current behavior matches the PR scope.🔧 Optional: preserve source chain via error-kind mapping
- 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); - ContextProviderError::Generic(format!("Failed to find quorum: {}", e)) - })?; + let this = self.clone(); + let quorum = + dash_async::block_on(async move { this.find_quorum(quorum_type, quorum_hash).await })? + .map_err(|e| match e { + TrustedContextProviderError::QuorumNotFound { quorum_hash, .. } => { + ContextProviderError::InvalidQuorum(format!( + "Quorum not found for hash: {quorum_hash}" + )) + } + other => { + debug!("Error finding quorum: {other}"); + ContextProviderError::Generic(format!("Failed to find quorum: {other}")) + } + })?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-sdk-trusted-context-provider/src/provider.rs` around lines 608 - 616, The debug log and Generic wrapping around the inner TrustedContextProviderError loses the original error variants; update the handling in the dash_async::block_on(...) call around TrustedContextProvider::find_quorum to preserve the error kind by either (A) matching/map_err on the TrustedContextProviderError returned by find_quorum and convert each variant to a corresponding ContextProviderError variant (e.g., map QuorumNotFound → ContextProviderError::QuorumNotFound, NetworkError → ContextProviderError::NetworkError), or (B) implement From<TrustedContextProviderError> for ContextProviderError (or add #[from] on ContextProviderError) and then remove the manual Generic wrapping so you can use the ? operator directly; locate the logic around self.clone(), find_quorum, and Self::parse_quorum_public_key(&quorum.key) and apply one of these two fixes so the specific error type is preserved instead of being hidden in Generic while keeping the existing debug logging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/rs-dash-async/src/block_on.rs`:
- Around line 121-133: The block_on stub on WASM (pub fn block_on) always
returns Err and needs clearer public documentation to prevent misuse; update the
block_on function doc comment to state explicitly that it is unsupported on
wasm32 (always returns AsyncError::Generic) and callers must use async APIs
(e.g., #[wasm_bindgen] async functions) instead, and add a prominent note in the
rs-dash-async crate-level docs (lib.rs or README) explaining the JSPI limitation
and the intended safety pattern (prefetchMainnet()/other prefetch factories +
refetch_if_not_found(false) leading to InvalidQuorum on cache misses) so
consumers understand why the stub exists and when they must avoid calling
block_on.
---
Nitpick comments:
In @.github/workflows/tests-rs-workspace.yml:
- Line 152: The Ubuntu backup job test-ubuntu is missing the dash-async package
that the macOS nextest run includes; update the test command in the test-ubuntu
job to include the same --package dash-async flag so the Ubuntu fallback runs
the same suite (including the current-thread deadlock regression for issue
`#3432`) and keep both jobs in parity with the macOS nextest invocation.
In `@packages/rs-dash-async/src/block_on.rs`:
- Around line 64-105: The current-thread branch of block_on drops the panic
payload from join_handle.join() and returns a generic AsyncError; modify the
join_result handling in block_on to extract the panic payload from the
Err(Box<dyn Any + Send>) returned by join_handle.join(), convert it to a
readable string (try downcasting to &str and String, falling back to a debug
format), and include that extracted message in the AsyncError::Generic returned
when (join_result, _) is (Err(_), _); keep the rest of the match logic unchanged
and reference join_handle.join(), join_result, and AsyncError::Generic to locate
the changes.
In `@packages/rs-sdk-trusted-context-provider/src/provider.rs`:
- Around line 608-616: The debug log and Generic wrapping around the inner
TrustedContextProviderError loses the original error variants; update the
handling in the dash_async::block_on(...) call around
TrustedContextProvider::find_quorum to preserve the error kind by either (A)
matching/map_err on the TrustedContextProviderError returned by find_quorum and
convert each variant to a corresponding ContextProviderError variant (e.g., map
QuorumNotFound → ContextProviderError::QuorumNotFound, NetworkError →
ContextProviderError::NetworkError), or (B) implement
From<TrustedContextProviderError> for ContextProviderError (or add #[from] on
ContextProviderError) and then remove the manual Generic wrapping so you can use
the ? operator directly; locate the logic around self.clone(), find_quorum, and
Self::parse_quorum_public_key(&quorum.key) and apply one of these two fixes so
the specific error type is preserved instead of being hidden in Generic while
keeping the existing debug logging.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d86f0442-99a9-4b3f-afec-3631279cba61
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
.github/workflows/tests-rs-workspace.ymlpackages/rs-dash-async/src/block_on.rspackages/rs-sdk-trusted-context-provider/src/provider.rs
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 12 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
lklimek
added
the
ready for final review
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The assigned detached HEAD 5787e41fad1f5ee835b138171ca06c27423d6507 changes only parse_quorum_public_key() in packages/rs-sdk-trusted-context-provider/src/provider.rs, replacing lowercase-only prefix stripping with explicit 0x/0X handling. That code change is correct, but there is no focused test covering the new uppercase 0X path, so this regression fix can be lost silently in a later refactor.
Reviewed commit: 5787e41
🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-sdk-trusted-context-provider/src/provider.rs`:
- [SUGGESTION] lines 552-556: Add a regression test for uppercase `0X` quorum key prefixes
This commit broadens `parse_quorum_public_key()` to accept uppercase `0X` as well as lowercase `0x`, but the module has no unit test that exercises the new branch. Because the helper is private and reached indirectly through cache and network code, a future cleanup could revert to lowercase-only handling without any test failure. Add a focused test that asserts both prefix casings decode to the same `[u8;48]` value.
| fn parse_quorum_public_key(key: &str) -> Result<[u8; 48], ContextProviderError> { | ||
| let pubkey_hex = key | ||
| .strip_prefix("0x") | ||
| .or_else(|| key.strip_prefix("0X")) | ||
| .unwrap_or(key); |
There was a problem hiding this comment.
🟡 Suggestion: Add a regression test for uppercase 0X quorum key prefixes
This commit broadens parse_quorum_public_key() to accept uppercase 0X as well as lowercase 0x, but the module has no unit test that exercises the new branch. Because the helper is private and reached indirectly through cache and network code, a future cleanup could revert to lowercase-only handling without any test failure. Add a focused test that asserts both prefix casings decode to the same [u8;48] value.
source: ['codex']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-sdk-trusted-context-provider/src/provider.rs`:
- [SUGGESTION] lines 552-556: Add a regression test for uppercase `0X` quorum key prefixes
This commit broadens `parse_quorum_public_key()` to accept uppercase `0X` as well as lowercase `0x`, but the module has no unit test that exercises the new branch. Because the helper is private and reached indirectly through cache and network code, a future cleanup could revert to lowercase-only handling without any test failure. Add a focused test that asserts both prefix casings decode to the same `[u8;48]` value.
Issue being fixed or feature implemented
Fixes #3432 —
futures::executor::block_oninside a tokio runtime inTrustedHttpContextProvidercauses a deadlock when quorum cache misses trigger async HTTP fetches.Root cause (from #3432)
ContextProvider::get_quorum_public_key()is synchronous, but on cache miss it callsfutures::executor::block_on(self.find_quorum(...))which makes HTTP requests viareqwest. Nestingfutures::executor::block_oninside a tokio runtime deadlocks on single-threaded runtimes or panics on multi-threaded runtimes. This affects the iOS SDK and any FFI consumer using the trusted context provider.What was done?
New crate:
packages/rs-dash-async(dash-async)Extracted a runtime-aware
block_onfunction that correctly handles all three scenarios:block_in_placepanic)block_in_place+ spawn for efficient bridgingAlso includes:
AsyncErrorerror typeErrwith JSPI reference instead ofunimplemented!()rs-sdk-trusted-context-provider— the fixfutures::executor::block_onwithdash_async::block_on— this is the direct fix for Deadlock: futures::executor::block_on inside tokio runtime in TrustedHttpContextProvider #3432TrustedHttpContextProviderClone(fallback_providerchanged fromBox<dyn ContextProvider>toArc<dyn ContextProvider>— only accessed via&self,ContextProvider: Send + Sync)#[cfg(target_arch = "wasm32")]separate code path —block_onhandles WASM internallydash-sdk::sync— backward compatibleblock_onandAsyncErrorfromdash-asyncuse dash_sdk::sync::block_onimports continue to work unchangedretryfunction and its tests remain indash-sdkrs-context-providerFrom<AsyncError> for ContextProviderErrorimpl here (orphan rule —AsyncErroris no longer local todash-sdk)How Has This Been Tested?
cargo test -p dash-async— 2 tests pass:test_block_on_nested_async_sync— multi-thread runtime with nested async→sync→async callstest_block_on_fails_on_current_thread_runtime— regression test for Deadlock: futures::executor::block_on inside tokio runtime in TrustedHttpContextProvider #3432: provesblock_onworks correctly on a current-thread runtime (the exact scenario that caused the deadlock)cargo test -p dash-sdk --lib -- sync::test— 40 retry tests passcargo test -p rs-sdk-trusted-context-provider— 8 tests passcargo checksucceeds for all affected cratesBreaking Changes
None. All public APIs are backward compatible via re-exports.
Checklist:
🤖 Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit
New Features
Improvements