Adopt the oneshot VTXO subscription pattern - #265
Conversation
# Conflicts: # ark-client-sample/src/main.rs # ark-client/src/vtxo_watcher.rs # ark-grpc/proto/ark/v1/indexer.proto # ark-grpc/src/client.rs # ark-grpc/src/generated/ark.v1.rs
|
Warning Review limit reached
Next review available in: 29 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughThe SDK adds a combined scripts subscription API that returns the subscription ID and event stream. It deprecates older helpers and updates watcher handshake handling, script filters, retries, and sample usage. ChangesVTXO subscription flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant VTXOWatcher
participant ArkClient
participant ArkGrpcClient
participant SubscriptionStream
VTXOWatcher->>ArkClient: subscribe_to_scripts_stream(scripts)
ArkClient->>ArkGrpcClient: Open script subscription
ArkGrpcClient->>SubscriptionStream: Read startup frames
SubscriptionStream-->>ArkGrpcClient: SubscriptionStarted and event stream
ArkGrpcClient-->>ArkClient: Subscription ID and remaining stream
ArkClient-->>VTXOWatcher: Return subscription result
VTXOWatcher->>ArkClient: update_subscription(subscription_id, filter)
ArkClient->>ArkGrpcClient: Apply additional script filter
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ark-grpc/src/client.rs`:
- Around line 896-915: Bound the subscription-start handshake in the loop
receiving `stream.next()` from `get_subscription`: apply the existing
timeout/deadline mechanism to each wait so a missing `SubscriptionStarted` or
heartbeat-only stream expires and returns an error, allowing the watcher’s
retry/backoff path to run. Preserve the current handling for successful startup,
heartbeats, events, stream errors, and closure.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: af3f8991-b408-47fa-ac08-336cf99cd915
📒 Files selected for processing (4)
ark-client-sample/src/main.rsark-client/src/lib.rsark-client/src/vtxo_watcher.rsark-grpc/src/client.rs
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
PROTOCOL-CRITICAL: human review required.
Summary
The oneshot-subscription pattern is the right direction — collapsing the two-step subscribe+connect into a single call removes a race window. The switch to update_subscription in refresh_subscription_scripts is also cleaner. However there are two correctness issues that need fixing before merge.
[CRITICAL] Handshake loop is not cancellable — watcher cannot be stopped while blocked on SubscriptionStarted
ark-grpc/src/client.rs (new subscribe_to_scripts_stream, lines added after get_subscription call):
let subscription_id = loop {
match stream.next().await {
Some(Ok(SubscriptionResponse::SubscriptionStarted { .. })) => { break subscription_id; }
Some(Ok(SubscriptionResponse::Heartbeat)) => continue, // ← unbounded
...
}
};This loop drains heartbeats indefinitely while waiting for the SubscriptionStarted frame. It is awaited as a plain Future inside run_watcher_loop (vtxo_watcher.rs, replacing old lines ~161-183):
let (subscription_id, mut stream) =
match client.subscribe_to_scripts_stream(addresses.clone()).await { // ← no select!
...
};stop_rx is never select!-ed against during this await. Consequences:
- A server that emits only heartbeats (slow path, degraded node) will stall the watcher task indefinitely.
VtxoWatcherHandle::stop()/Dropsignalsstop_rxbut the task never wakes to observe it — graceful shutdown silently fails.- The backoff reconnection logic cannot fire.
The pre-existing two-step code had the same gap for the unary subscribe_to_scripts RPC, but a unary call is bounded by gRPC's per-call deadline. The new streaming handshake loop is unbounded.
Fix: wrap the handshake future with tokio::time::timeout (e.g. 30 s) and propagate Err so callers can retry with backoff. Example:
let subscription_id = tokio::time::timeout(Duration::from_secs(30), async {
loop {
match stream.next().await {
Some(Ok(SubscriptionResponse::SubscriptionStarted { subscription_id })) => {
return Ok(subscription_id);
}
Some(Ok(SubscriptionResponse::Heartbeat)) => continue,
Some(Ok(SubscriptionResponse::Event(_))) => {
return Err(Error::conversion("event before subscription_started"));
}
Some(Err(e)) => return Err(e),
None => return Err(Error::conversion("stream closed before subscription_started")),
}
}
})
.await
.map_err(|_| Error::conversion("timed out waiting for subscription_started"))??;The watcher call site still needs a select! on stop_rx around the whole subscribe_to_scripts_stream call for clean shutdown, or the timeout alone is enough to bound the hang (pick one, document the choice).
[SIGNIFICANT] expressions: Vec::new() in refresh_subscription_scripts silently clears subscription expressions
ark-client/src/vtxo_watcher.rs, refresh_subscription_scripts (new code replacing old line ~462):
let filter = SubscriptionFilter {
expressions: Vec::new(), // ← clears all expressions on the subscription
add_scripts: new_addrs.iter().map(|addr| addr.to_p2tr_script_pubkey()).collect(),
remove_scripts: Vec::new(),
};
client.update_subscription(subscription_id.to_string(), filter).await?;The update_subscription contract (ark-client/src/lib.rs:2211-2214 and ark-grpc/src/client.rs:797-799) explicitly states: "The filter's expressions are always overwritten as a whole (an empty list clears them)." Passing expressions: Vec::new() destroys any expressions already on the subscription. This is safe today because no expressions are ever set in this flow, but it is a latent footgun: if a future caller opens the stream with expressions, refresh_subscription_scripts will silently wipe them on the first 10-second discovery tick.
Fix: either document the deliberate clearing in a comment, or if the intent is truly "only add scripts, touch nothing else," submit a follow-up to expose a dedicated add_scripts-only RPC variant (or pass None for expressions once the filter type supports it).
[SIGNIFICANT] No tests for any new code path
Danger flagged this correctly. The new subscribe_to_scripts_stream method (ark-grpc/src/client.rs) and the updated refresh_subscription_scripts (vtxo_watcher.rs) have zero test coverage. At minimum needed:
- Happy path: heartbeats followed by
SubscriptionStarted— ID extracted, remaining stream items delivered. - Error path:
Eventarrives beforeSubscriptionStarted—Errreturned. - Error path: stream closes before
SubscriptionStarted—Errreturned. - Timeout path (once added): server sends only heartbeats past the deadline —
Errreturned and caller can retry.
The existing test module in vtxo_watcher.rs covers group_by_expiry_day, select_vtxos_for_self_renewal, etc. but nothing exercises the subscription machinery. A mock get_subscription via ark_grpc::test_utils (already imported in lib.rs tests) would work.
Minor
ark-client/src/lib.rs subscribe_to_scripts and unsubscribe_from_scripts: the #[allow(deprecated)] attributes are correct (the wrappers call the now-deprecated underlying gRPC methods) but benefit from an inline comment — // calls the deprecated grpc method; suppress the warning on this intentional wrapper — otherwise the next reader will wonder if it is an oversight.
No cross-repo breakage found
Checked ts-sdk, go-sdk, dotnet-sdk — none consume subscribe_to_scripts/unsubscribe_from_scripts from this SDK's crate directly. No external breakage. The #[deprecated] attributes correctly guide internal SDK consumers.
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
PROTOCOL-CRITICAL: human review required.
Since 8aff6b7: 4 commits, 4 prior issues closed, 1 new moderate finding.
Prior findings — closure accounting
[CRITICAL] Handshake loop not cancellable → Fixed (5444d37a)
subscribe_within_deadline correctly races stop_rx.changed() against tokio::time::timeout(SUBSCRIBE_TIMEOUT, handshake) via tokio::select!. The outer watcher loop also has the *stop_rx.borrow() guard (vtxo_watcher.rs:155) before entering the call, so a stop signal that arrives during a successful handshake is caught on the very next loop iteration. Backoff on timeout and backoff on error are both wired correctly. Four unit tests cover all exit paths (Ready, Stopped, Retry-on-timeout, Retry-on-error). ✅
[SIGNIFICANT] expressions: Vec::new() silently clears subscription expressions → Fixed (3e031ebf)
additional_scripts_filter was extracted as a standalone function and carries a clear invariant comment at vtxo_watcher.rs:524-527 warning future callers that expressions opened with the stream must be carried forward here instead of silently wiped. The latent footgun is documented at its origin. ✅
[SIGNIFICANT] No test coverage → Fixed (f5bbe331)
ark-grpc/src/client.rs tests for read_subscription_started cover: happy path with leading heartbeats, event-before-started, close-before-started (both empty and heartbeats-only), and error propagation. vtxo_watcher.rs tests cover additional_scripts_filter (selects unsubscribed, returns None when all subscribed) and all four subscribe_within_deadline outcomes. Coverage is solid for all flagged paths. ✅
Minor: #[allow(deprecated)] unexplained → Fixed (f0e080fb)
Both ark-client/src/lib.rs:2157-2160 and ark-grpc/src/client.rs:735-737 now carry #[deprecated(note = ...)] alongside inline prose explaining that the #[allow(deprecated)] is an intentional self-referential suppression. ✅
New finding
[MODERATE] subscribe_to_scripts_stream / read_subscription_started public API has no internal timeout — external callers can hang indefinitely
read_subscription_started at ark-grpc/src/client.rs:1625-1641 is an unbounded loop over stream.next().await. It is called from subscribe_to_scripts_stream in both ark-grpc/src/client.rs:897-901 and ark-client/src/lib.rs:2260-2263. Neither call site applies a deadline.
The 30-second SUBSCRIBE_TIMEOUT exists only inside subscribe_within_deadline in the watcher (vtxo_watcher.rs:468). The public API surface has no self-defense. Any caller that invokes subscribe_to_scripts_stream directly will block indefinitely if the server emits only heartbeats before subscription_started.
The sample program is directly affected:
// ark-client-sample/src/main.rs:777-783
let (subscription_id, mut subscription_stream) = client
.subscribe_to_scripts_stream(vec![address.0]) // ← no timeout; can hang forever
.await
.map_err(|e| anyhow!(e))?;
An SDK consumer integrating this into a payment flow with a misbehaving or degraded node has no recourse except wrapping the call externally, which is easy to miss.
Options (pick one):
Option A — move the timeout inside read_subscription_started (preferred, makes the API self-defending):
async fn read_subscription_started<S>(mut stream: S) -> Result<(String, S), Error>
where
S: Stream<Item = Result<SubscriptionResponse, Error>> + Unpin,
{
tokio::time::timeout(Duration::from_secs(30), async {
loop {
match stream.next().await {
...
}
}
})
.await
.map_err(|_| Error::conversion("timed out waiting for subscription_started"))?
}Option B — document the missing deadline in the public subscribe_to_scripts_stream doc comment:
/// # Deadlines
///
/// This method blocks until the server sends `subscription_started`. If the server
/// sends only heartbeats indefinitely (degraded node) the call will not return.
/// Wrap with `tokio::time::timeout` if you need a bounded wait.
Option A is preferable because it makes the safe path the default and prevents the footgun in the sample and in downstream SDK consumers.
No new cross-repo breakage
The old subscribe_to_scripts and unsubscribe_from_scripts are deprecated (not removed). Checked ts-sdk, go-sdk, dotnet-sdk: none import these methods. No external breakage.
Remaining blocker
The one open item is the unbounded public API (read_subscription_started / subscribe_to_scripts_stream). Everything else in this diff is correctly implemented and well tested.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
ark-grpc/src/client.rs (1)
1768-1844: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a deterministic regression test for the timeout branch.
The added tests cover startup parsing and stream preservation, but do not clearly exercise the
subscribe_to_scripts_streamtimeout error path at Lines [907]-[912]. Add a short/injectable timeout or paused-clock test for a stream that remains open while emitting only heartbeats.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ark-grpc/src/client.rs` around lines 1768 - 1844, Add a deterministic test covering the timeout branch in subscribe_to_scripts_stream: use an injectable or paused clock with an open stream that emits only SubscriptionResponse::Heartbeat values, advance the clock beyond the startup timeout, and assert the expected timeout error. Keep the test focused on timeout behavior without changing the existing startup parsing tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ark-grpc/src/client.rs`:
- Around line 879-881: Update the documentation comment for the public
subscribe_to_scripts_stream API to remove the intra-doc link to the private
SUBSCRIPTION_START_TIMEOUT constant, replacing it with plain-text timeout
wording such as the duration; keep the described bounded-wait and heartbeat
behavior unchanged.
- Around line 901-913: Wrap both the get_subscription call and
read_subscription_started in the same tokio::time::timeout so
SUBSCRIPTION_START_TIMEOUT begins before opening the subscription RPC. Update
the surrounding subscribe_to_scripts_stream flow to propagate the inner result
and convert timeout expiration to the existing “timed out waiting for
subscription_started” error, preserving current success and error behavior.
---
Nitpick comments:
In `@ark-grpc/src/client.rs`:
- Around line 1768-1844: Add a deterministic test covering the timeout branch in
subscribe_to_scripts_stream: use an injectable or paused clock with an open
stream that emits only SubscriptionResponse::Heartbeat values, advance the clock
beyond the startup timeout, and assert the expected timeout error. Keep the test
focused on timeout behavior without changing the existing startup parsing tests.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 324bd28c-653c-4f58-807e-97715313b5e5
📒 Files selected for processing (4)
ark-client/src/lib.rsark-client/src/vtxo_watcher.rsark-grpc/Cargo.tomlark-grpc/src/client.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- ark-client/src/lib.rs
- ark-client/src/vtxo_watcher.rs
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
PROTOCOL-CRITICAL: human review required.
2 commits since last pass (66c6c2c, 74df25e). 1 prior finding fixed. 0 still open. 1 new minor finding.
Prior findings — closure accounting
[MODERATE] Public API has no internal timeout (subscribe_to_scripts_stream / read_subscription_started hang indefinitely on a heartbeat-only server) → Fixed (74df25e)
subscribe_to_scripts_stream in ark-grpc/src/client.rs now wraps read_subscription_started(stream) with tokio::time::timeout(SUBSCRIPTION_START_TIMEOUT, …) (30 s). The watcher's own subscribe_within_deadline still provides a second layer at the call site for cooperative shutdown. The timeout constant is documented at ark-grpc/src/client.rs:1634 and the doc comment on the public function at line ~878 explains the bounded-wait guarantee. ark-grpc/Cargo.toml correctly adds tokio (with time feature, no extras) to [dependencies]. The sample program path (ark-client-sample/src/main.rs:777-783) is now also protected. ✅
All findings from the review before this one (handshake cancellability, expressions: Vec::new() footgun, test coverage, #[allow(deprecated)] prose) were carried forward unchanged — no regressions observed.
New finding
[MINOR] Intra-doc link to a private constant in a public function doc comment
ark-grpc/src/client.rs:878:
/// The wait for `subscription_started` is bounded by [`SUBSCRIPTION_START_TIMEOUT`]. A
SUBSCRIPTION_START_TIMEOUT is declared const (no pub), so this is a private item. Rustdoc emits a rustdoc::private_intra_doc_links warning and the rendered HTML docs show a broken anchor. No #![deny(rustdoc::private_intra_doc_links)] exists in this crate, so it won't currently break CI, but it degrades the public docs.
Two fixes, either works:
- Replace the link with plain text:
"bounded by a 30-second timeout"— one-liner, zero visibility change. - Make the constant
pub(crate)so the link resolves:pub(crate) const SUBSCRIPTION_START_TIMEOUT: Duration = …;at line 1634.
The parallel doc comment in ark-client/src/lib.rs:2250-2253 ("The wait for the subscription to start is internally bounded") deliberately omits the duration, which is acceptable for a facade, but appending (by default 30 seconds) would be more useful to callers who need to size their own outer timeouts.
No new cross-repo breakage
subscribe_to_scripts and unsubscribe_from_scripts remain deprecated-not-removed. ts-sdk, go-sdk, dotnet-sdk confirmed clear as before.
Status
Substantive protocol issues are all resolved. The remaining item is a documentation quality nit that has no correctness or safety impact. Safe to merge once the doc link is addressed (or documented as a known warning to clear later).
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
PROTOCOL-CRITICAL: human review required.
1 commit relevant to subscription code since 74df25e (7e36fc45); 1 prior finding fixed; 0 remain open. Ready to merge.
The bulk of the incremental diff (37 of 39 commits) is unrelated Boltz / contract-manager work brought in by merging master (5efdb3f0). Scope of this pass is scoped to the subscription changes only.
Prior finding — closure accounting
[MINOR] Intra-doc link to private constant in public doc comment → Fixed (7e36fc45)
ark-grpc/src/client.rs:880–881 now reads:
/// Startup is bounded by a 30-second timeout, covering both opening the streaming
/// RPC and waiting for `subscription_started`.
The broken [SUBSCRIPTION_START_TIMEOUT] link is gone. Rustdoc will no longer emit private_intra_doc_links. ✅
Incidental improvement in the same commit: get_subscription (the gRPC stream open) is now inside the startup async block and therefore also raced against SUBSCRIPTION_START_TIMEOUT. Previously only read_subscription_started was under the timeout; a transport stall on stream open would have escaped the deadline. Now it cannot.
New findings
None. No new issues introduced by the subscription-scoped changes.
Open CodeRabbit nitpick (not mine, not blocking)
CodeRabbit flagged that subscribe_to_scripts_stream itself has no test for the timeout branch — only read_subscription_started is exercised by the current unit tests (ark-grpc/src/client.rs:1768–1844). A stalled get_subscription open hitting the deadline is now possible to test (the startup async block is a plain Future) and would close the gap cleanly with a paused-clock or short-duration test. Not a blocker, but worth a follow-up if the test suite ever targets line coverage.
Status
All findings from every prior pass are resolved. No correctness, security, or protocol issues remain open in the subscription code.
|
Requesting for manual review @luckysori, @bonomat. I also need help verifying that the timeout constants I have used are suitable. |
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
PROTOCOL-CRITICAL: human review required.
1 commit since last pass (5efdb3f → 5c711df); 1 prior issue closed; 2 new findings, 1 moderate.
Prior finding closure
[MODERATE] / has no internal timeout → Fixed (already present at 5efdb3f)
The tokio::time::timeout(SUBSCRIPTION_START_TIMEOUT, startup) wrapper at ark-grpc/src/client.rs:907-912 was already in place at the base commit. The doc comment at lines 880-882 explicitly documents the 30-second bounded wait. My second review incorrectly identified this as absent — the fix predates this incremental commit. Retraction acknowledged.
New findings (5c711df: ark-client/src/boltz.rs only)
[MODERATE] Boltz watcher handshake is not cancellable — stop_rx ignored for up to 30 s
boltz.rs:4427-4438
let (subscription_id, mut stream) =
match client.subscribe_to_scripts_stream(addresses.clone()).await { // ← no select!The vtxo_watcher equivalent wraps this call in subscribe_within_deadline (vtxo_watcher.rs:168-180), which tokio::select!s against stop_rx:
let handshake = client.subscribe_to_scripts_stream(addresses.clone());
let (subscription_id, mut stream) =
match subscribe_within_deadline(&mut stop_rx, SUBSCRIBE_TIMEOUT, handshake).await {In the boltz watcher, stop_rx is not observed during the handshake. If the server sends only heartbeats, a stop() / Drop signal on the handle cannot wake the task — it is stranded for the full 30 s SUBSCRIPTION_START_TIMEOUT. For a VHTLC watcher handling time-locked submarine-swap HTLCs, a 30 s shutdown lag is a meaningful operational hazard and also breaks the behavioral contract established by the existing subscribe_within_deadline pattern.
Fix: reuse subscribe_within_deadline (it is a free function in vtxo_watcher.rs — consider moving it to a shared module, or duplicate it in boltz.rs for now):
let handshake = client.subscribe_to_scripts_stream(addresses.clone());
let (subscription_id, mut stream) =
match subscribe_within_deadline(&mut stop_rx, SUBSCRIBE_TIMEOUT, handshake).await {
SubscribeOutcome::Ready(sub) => sub,
SubscribeOutcome::Stopped => return,
SubscribeOutcome::Retry => {
backoff = (backoff * 2).min(VHTLC_WATCHER_MAX_BACKOFF);
continue;
}
};[MINOR] Missing invariant comment on expressions: Vec::new() in refresh_boltz_vhtlc_subscription
boltz.rs:4544
let filter = SubscriptionFilter {
expressions: Vec::new(), // ← no comment
add_scripts: ...,
remove_scripts: Vec::new(),
};This is exactly the pattern fixed in vtxo_watcher.rs with the protective comment at vtxo_watcher.rs:522-525:
// `update_subscription` overwrites expressions as a whole but treats scripts as additive. This
// flow adds scripts and never sets expressions, so the empty list has nothing to clear. A
// future caller that opens the stream with expressions must carry them here instead of wiping
// them on the first discovery tick.Copy that comment verbatim to boltz.rs:4544. A VHTLC maintainer adding expression filtering will hit the same silent-wipe footgun without it.
[MINOR] No test coverage for the new boltz subscription path
boltz.rs:4427-4438 and 4524-4560
run_boltz_vhtlc_watcher_loop's new subscribe_to_scripts_stream call and refresh_boltz_vhtlc_subscription have zero test coverage. The vtxo_watcher established the baseline: additional_scripts_filter unit tests and four subscribe_within_deadline outcome tests. The boltz path should have equivalent coverage for refresh_boltz_vhtlc_subscription (happy path: only new addrs submitted; skip when all already subscribed) and for the subscription outcome dispatch (stop, retry-on-timeout, retry-on-error). Danger flagged this correctly.
No cross-repo impact
This commit is confined to ark-client/src/boltz.rs. The new API surface (subscribe_to_scripts_stream, update_subscription) is unchanged. No breakage to ts-sdk, go-sdk, or dotnet-sdk consumers.
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
PROTOCOL-CRITICAL: human review required.
1 commit since last pass (5efdb3f → 5c711df); 1 prior issue closed; 2 new findings, 1 moderate.
Prior finding closure
[MODERATE] subscribe_to_scripts_stream / read_subscription_started has no internal timeout → Fixed (already present at 5efdb3f)
The tokio::time::timeout(SUBSCRIPTION_START_TIMEOUT, startup) wrapper at ark-grpc/src/client.rs:907-912 was already in place at the base commit. The doc comment at lines 880-882 explicitly documents the 30-second bounded wait. My second review incorrectly identified this as absent — the fix predates this incremental commit. Retraction acknowledged.
New findings (5c711df: ark-client/src/boltz.rs only)
[MODERATE] Boltz watcher handshake is not cancellable — stop_rx ignored for up to 30 s
boltz.rs:4427-4438
let (subscription_id, mut stream) =
match client.subscribe_to_scripts_stream(addresses.clone()).await { // ← no select!The vtxo_watcher equivalent wraps this call in subscribe_within_deadline (vtxo_watcher.rs:168-180), which tokio::select!s against stop_rx:
let handshake = client.subscribe_to_scripts_stream(addresses.clone());
let (subscription_id, mut stream) =
match subscribe_within_deadline(&mut stop_rx, SUBSCRIBE_TIMEOUT, handshake).await {In the boltz watcher, stop_rx is not observed during the handshake. If the server sends only heartbeats, a stop() / Drop signal on the handle cannot wake the task — it is stranded for the full 30 s SUBSCRIPTION_START_TIMEOUT. For a VHTLC watcher handling time-locked submarine-swap HTLCs, a 30 s shutdown lag is a meaningful operational hazard and also breaks the behavioral contract established by the existing subscribe_within_deadline pattern.
Fix: reuse subscribe_within_deadline (it is a free function in vtxo_watcher.rs — consider moving it to a shared module, or duplicate it in boltz.rs for now):
let handshake = client.subscribe_to_scripts_stream(addresses.clone());
let (subscription_id, mut stream) =
match subscribe_within_deadline(&mut stop_rx, SUBSCRIBE_TIMEOUT, handshake).await {
SubscribeOutcome::Ready(sub) => sub,
SubscribeOutcome::Stopped => return,
SubscribeOutcome::Retry => {
backoff = (backoff * 2).min(VHTLC_WATCHER_MAX_BACKOFF);
continue;
}
};[MINOR] Missing invariant comment on expressions: Vec::new() in refresh_boltz_vhtlc_subscription
boltz.rs:4544
let filter = SubscriptionFilter {
expressions: Vec::new(), // ← no comment
add_scripts: ...,
remove_scripts: Vec::new(),
};This is exactly the pattern fixed in vtxo_watcher.rs with the protective comment at vtxo_watcher.rs:522-525:
// `update_subscription` overwrites expressions as a whole but treats scripts as additive. This
// flow adds scripts and never sets expressions, so the empty list has nothing to clear. A
// future caller that opens the stream with expressions must carry them here instead of wiping
// them on the first discovery tick.
Copy that comment verbatim to boltz.rs:4544. A VHTLC maintainer adding expression filtering will hit the same silent-wipe footgun without it.
[MINOR] No test coverage for the new boltz subscription path
boltz.rs:4427-4438 and 4524-4560
run_boltz_vhtlc_watcher_loop's new subscribe_to_scripts_stream call and refresh_boltz_vhtlc_subscription have zero test coverage. The vtxo_watcher established the baseline: additional_scripts_filter unit tests and four subscribe_within_deadline outcome tests. The boltz path should have equivalent coverage for refresh_boltz_vhtlc_subscription (happy path: only new addrs submitted; skip when all already subscribed) and for the subscription outcome dispatch (stop, retry-on-timeout, retry-on-error). Danger flagged this correctly.
No cross-repo impact
This commit is confined to ark-client/src/boltz.rs. The new API surface (subscribe_to_scripts_stream, update_subscription) is unchanged. No breakage to ts-sdk, go-sdk, or dotnet-sdk consumers.
2 participants
Closes #254.
subscribe_to_scripts_streamthat opens a subscription and returns the stream together.Summary by CodeRabbit
New Features
Bug Fixes
Deprecations