Skip to content

minor fixes around modules#863

Merged
mojoX911 merged 4 commits into
citadel-tech:masterfrom
0xEgao:timeout
May 12, 2026
Merged

minor fixes around modules#863
mojoX911 merged 4 commits into
citadel-tech:masterfrom
0xEgao:timeout

Conversation

@0xEgao
Copy link
Copy Markdown
Collaborator

@0xEgao 0xEgao commented May 7, 2026

Pull Request

Description

Fixes small bugs -:
1-) removed timeout from net_wait_for_confirmation api. as generally most of the blocks are not mined under 10min, so wait until the txn is confirmed.
2-) Currently while testing on signet i was getting a lot of WouldBlock spam from nostr relay response, i.e.

2026-05-04T00:04:48.302868+05:30 WARN coinswap::watch_tower::nostr_discovery - Nostr session error on wss://relay.damus.io: WebSocket(Io(Os { code: 35, kind: WouldBlock, message: "Resource temporarily unavailable" })), retrying in 5s
2026-05-04T00:04:52.673107+05:30 WARN coinswap::watch_tower::nostr_discovery - Nostr session error on wss://nos.lol: WebSocket(Io(Os { code: 35, kind: WouldBlock, message: "Resource temporarily unavailable" })), retrying in 5s

Now, In read_event_loop, WouldBlock / TimedOut / Interrupted are now treated as non-fatal (continue loop), to avoid reconnections.

3-) Currently in a typical swap, we have a lot of calls to zmq::poll && zmq::recv_event, so I added a small idle backoff in the watcher loop.
Changes in Profiling-:

Earlier
coinswap::watch_tower::zmq_backend::poll                                         114548537    1218.59 s     10.64 µs     16.05 µs    97.12%
coinswap::watch_tower::zmq_backend::recv_event                                   114548537    1168.99 s     10.21 µs     15.46 µs    93.17%

Now
Current
coinswap::watch_tower::zmq_backend::poll                                           67661       2.51 s     37.10 µs     62.11 µs     0.60%
coinswap::watch_tower::zmq_backend::recv_event                                     67662       2.14 s     31.60 µs     45.57 µs     0.51%

Related Issue(s)

Closes #

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Code refactor / performance improvement
  • Documentation update only
  • CI / Docker / Build changes
  • Other (please describe):

Protocol Version(s) Affected

  • Legacy (ECDSA — messages.rs, contract.rs, handlers.rs)
  • Taproot-Musig2 (messages2.rs, contract2.rs, handlers2.rs)
  • Both
  • Neither (infrastructure / tooling only)

Note: When modifying protocol logic, changes must be made to both versions unless the
scope is explicitly version-specific.

Affected Component(s)

  • makerd (background daemon)
  • taker (CLI client)
  • maker-cli (command-line tool)
  • Core protocol / cryptography / library
  • watch_tower (contract monitoring & recovery)
  • Docker / deployment scripts
  • Tests / test framework
  • Documentation (docs/)
  • Other (please specify):

Checklist

Code Quality

  • I ran cargo +nightly fmt --all and committed the result
  • I ran cargo +stable clippy --all-features --lib --bins --tests -- -D warnings with zero warnings
  • I ran cargo +stable clippy --examples -- -D warnings with zero warnings
  • I ran RUSTDOCFLAGS="-D warnings" cargo +nightly doc --all-features --document-private-items --no-deps with zero warnings
  • Pre-commit git hook passes (ln -s ../../git_hooks/pre-commit .git/hooks/pre-commit if not already set)

Testing

  • All unit tests pass (cargo test)
  • Integration tests pass (cargo test --features integration-test)
  • Changes were manually tested on regtest
  • End-to-end maker ↔ taker swap tested (where applicable)
  • Test-only code is gated behind #[cfg(feature = "integration-test")]

Documentation

  • Relevant files in the docs/ folder were updated
  • Complex logic is commented

Security & Privacy (Critical)

  • This change preserves trustlessness and atomic swap guarantees
  • No regression in sybil resistance or fidelity bond logic
  • Tor anonymity and P2P message flow were reviewed
  • No new attack vectors or trust assumptions introduced
  • Edge cases and error handling considered
  • ZMQ subscription integrity (raw tx/block feed) is unaffected
  • integration-test feature flag is not reachable in production code paths

Summary by CodeRabbit

  • Bug Fixes

    • Longer non-test transaction confirmation timeout to reduce false timeouts.
    • Treat transient WebSocket I/O errors as recoverable to keep discovery/monitoring running.
    • Add brief idle sleep in the watcher loop to lower CPU usage when idle.
  • Reliability / UX

    • Confirmation waiting now uses a wallet-backed, multi-transaction waiter with interrupt/abort support and clearer interruption reporting.
  • Tests

    • Integration tests updated to match the new confirmation-wait behavior.

Review Change Stack

Copilot AI review requested due to automatic review settings May 7, 2026 07:50
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 7, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Centralized confirmation waiting in the wallet with a multi-tx, threshold-based waiter (with interruption/abort hooks), updated call sites (maker/taker/tests), increased a taker non-test confirmation timeout (600s→900s), refined websocket read error handling, and added a 5ms idle sleep in the watcher loop.

Changes

Reliability and Performance Improvements

Layer / File(s) Summary
Error Types
src/taker/error.rs, src/wallet/error.rs
Removed FundingTxWaitTimeOut from TakerError; added WalletError::Interrupted(&'static str) and Display formatting.
Wallet Confirmation API & Impl
src/wallet/api.rs
Replaced single-tx waiter with wait_for_tx_confirmation(&[Txid], required_confirms, shutdown, abort_check) that polls multiple txids, returns max confirmation height, supports shutdown and abort predicate; updated sweep call to use new API.
Call Sites: Maker & Taker
src/maker/api.rs, src/taker/legacy_swap.rs, src/taker/taproot_swap.rs, src/taker/api.rs
Replaced network-based confirmation waits with the wallet waiter; updated maker/taker call sites and adjusted taker non-integration timeout from 600s to 900s.
Watch Tower I/O & Idle Loop
src/watch_tower/nostr_discovery.rs, src/watch_tower/watcher.rs
read_event_loop retries non-fatal IO errors (WouldBlock/TimedOut/Interrupted) and treats closed sockets as graceful when shutting down; watcher sleeps 5ms when idle to avoid busy spin.
Tests
tests/integration/fidelity.rs
Integration tests updated to call new slice-based wait_for_tx_confirmation(&[txid], 1, None, None) signature.

Sequence Diagram(s)

sequenceDiagram
  participant Caller as Taker/Maker
  participant Wallet
  participant Network
  Caller->>Wallet: wait_for_tx_confirmation(&[txid], required_confirms, shutdown, abort_check)
  Wallet->>Network: get_raw_transaction_info(txid) (loop)
  Network-->>Wallet: txinfo (confirmations, height)
  Wallet-->>Caller: confirmation_height or WalletError::Interrupted / other error
Loading

Estimated Code Review Effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested Reviewers

  • mojoX911
  • Godzilla-oss

Poem

"I nibble code and stretch my paws,
Timeouts lengthen for steadier claws,
Soft errors pass while sockets sleep,
The wallet counts confirmations deep,
Bunny hops—these fixes earn applause." 🐰✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Title check ❓ Inconclusive The title "minor fixes around modules" is vague and generic, using non-descriptive terms that don't convey meaningful information about the specific changes (timeout removal, error handling, performance optimization). Consider a more descriptive title like "Improve wallet confirmation API and watch-tower performance" to clearly reflect the main changes.
✅ Passed checks (4 passed)
Check name Status Explanation
Description check ✅ Passed The description provides clear details of three specific bug fixes/improvements with examples and profiling data, but incomplete testing checklists and missing related issue reference.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Tip

💬 Introducing Slack Agent: The best way for teams to turn conversations into code.

Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.

  • Generate code and open pull requests
  • Plan features and break down work
  • Investigate incidents and troubleshoot customer tickets together
  • Automate recurring tasks and respond to alerts with triggers
  • Summarize progress and report instantly

Built for teams:

  • Shared memory across your entire org—no repeating context
  • Per-thread sandboxes to safely plan and execute work
  • Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@src/watch_tower/nostr_discovery.rs`:
- Around line 205-207: The match arm that converts
tungstenite::Error::ConnectionClosed and tungstenite::Error::AlreadyClosed into
Ok(()) causes run_nostr_session_for_relay to treat relay-initiated disconnects
as graceful shutdowns and stop reconnecting; change the behavior to return an
Err (or a distinct error) from that arm instead of Ok(()) so the outer retry
loop in run_nostr_session_for_relay will log and retry, or alternatively check
the shared shutdown flag inside that arm and only return Ok(()) when shutdown is
set while returning Err otherwise; locate the match handling those tungstenite
errors and update it accordingly.
🪄 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: ASSERTIVE

Plan: Pro

Run ID: b9268273-162c-491a-bde4-26fef54a2a54

📥 Commits

Reviewing files that changed from the base of the PR and between 836c2eb and 0a83994.

📒 Files selected for processing (3)
  • src/taker/api.rs
  • src/watch_tower/nostr_discovery.rs
  • src/watch_tower/watcher.rs

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR makes small operational/performance tweaks to the watch tower and taker by reducing busy-looping in the watcher, adjusting Nostr relay read error handling to avoid unnecessary reconnects, and increasing the taker confirmation wait timeout.

Changes:

  • Add a small idle sleep in the watch tower watcher loop when there are no commands and no ZMQ events.
  • Treat WouldBlock/TimedOut/Interrupted socket read errors as non-fatal in Nostr discovery to prevent reconnect spam.
  • Increase taker confirmation wait timeout from 600s to 900s outside integration tests.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
src/watch_tower/watcher.rs Adds a short sleep when both command queue and ZMQ backend are idle to prevent CPU busy-looping.
src/watch_tower/nostr_discovery.rs Adjusts websocket read error handling to continue on transient I/O errors rather than reconnecting.
src/taker/api.rs Increases the non-test confirmation wait timeout to better match typical block intervals.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/watch_tower/nostr_discovery.rs Outdated
Comment thread src/watch_tower/watcher.rs
@codecov
Copy link
Copy Markdown

codecov Bot commented May 7, 2026

Codecov Report

❌ Patch coverage is 86.17021% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.47%. Comparing base (776b75d) to head (d0264e1).
⚠️ Report is 162 commits behind head on master.

Files with missing lines Patch % Lines
src/watch_tower/nostr_discovery.rs 27.27% 8 Missing ⚠️
src/wallet/api.rs 94.11% 3 Missing ⚠️
src/taker/legacy_swap.rs 95.83% 1 Missing ⚠️
src/wallet/error.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #863      +/-   ##
==========================================
+ Coverage   68.87%   78.47%   +9.60%     
==========================================
  Files          35       57      +22     
  Lines        4932    15762   +10830     
==========================================
+ Hits         3397    12370    +8973     
- Misses       1535     3392    +1857     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/watch_tower/watcher.rs (1)

179-188: 🧹 Nitpick | 🔵 Trivial | 💤 Low value

Sleep fires after every command when no ZMQ event is pending, not only when fully idle.

After handle_command returns (any command other than Shutdown), execution falls through to self.backend.poll(); if poll() returns None we sleep 5 ms before the next try_recv(). The comment says "no commands and no ZMQ events" but the sleep is conditioned only on the ZMQ side. A burst of incoming commands (e.g. multiple RegisterWatchRequest at startup) would each incur an extra 5 ms round-trip.

♻️ Proposed fix — track whether work was done this tick
 loop {
+    let mut did_work = false;
     match self.rx_requests.try_recv() {
         Ok(cmd) => {
+            did_work = true;
             if !self.handle_command(cmd, &rest_backend_2) {
                 discovery_clone.store(true, Ordering::SeqCst);
                 break;
             }
         }
         Err(TryRecvError::Disconnected) => break,
         Err(TryRecvError::Empty) => {}
     }

     if let Some(event) = self.backend.poll() {
+        did_work = true;
         self.handle_event(event);
-    } else {
-        // Avoid busy-looping when there are no commands and no ZMQ events.
-        std::thread::sleep(Duration::from_millis(5));
+    }
+    if !did_work {
+        // Avoid busy-looping when there are no commands and no ZMQ events.
+        std::thread::sleep(Duration::from_millis(5));
     }
 }
🤖 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 `@src/watch_tower/watcher.rs` around lines 179 - 188, The loop currently sleeps
whenever backend.poll() returns None even if a command was just processed; add a
local boolean (e.g., work_done) at the start of the loop, set it true whenever a
command is successfully handled (from try_recv/handle_command) or when
self.backend.poll() yields an event handled by handle_event, then only call
std::thread::sleep(Duration::from_millis(5)) when work_done is false (i.e., no
command and no event this tick); update references to try_recv(),
handle_command/handle_event, and backend.poll() in watcher.rs to mark work_done
appropriately and preserve existing shutdown/control flow.
🤖 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 `@src/watch_tower/nostr_discovery.rs`:
- Around line 205-210: The match arm handling
tungstenite::Error::ConnectionClosed | tungstenite::Error::AlreadyClosed should
return the actual matched error when shutdown.load(Ordering::SeqCst) is false so
reconnection logic in run_nostr_session_for_relay triggers; split that arm into
two patterns or bind the error (e.g., err) so you can return err.into() for both
ConnectionClosed and AlreadyClosed, and only return Ok(()) when shutdown is
true, preserving the original error value for logging/fidelity.

---

Outside diff comments:
In `@src/watch_tower/watcher.rs`:
- Around line 179-188: The loop currently sleeps whenever backend.poll() returns
None even if a command was just processed; add a local boolean (e.g., work_done)
at the start of the loop, set it true whenever a command is successfully handled
(from try_recv/handle_command) or when self.backend.poll() yields an event
handled by handle_event, then only call
std::thread::sleep(Duration::from_millis(5)) when work_done is false (i.e., no
command and no event this tick); update references to try_recv(),
handle_command/handle_event, and backend.poll() in watcher.rs to mark work_done
appropriately and preserve existing shutdown/control flow.
🪄 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: ASSERTIVE

Plan: Pro

Run ID: 2ce94873-3352-4c29-8f81-b80b37d670da

📥 Commits

Reviewing files that changed from the base of the PR and between 0a83994 and c8e5c54.

📒 Files selected for processing (2)
  • src/watch_tower/nostr_discovery.rs
  • src/watch_tower/watcher.rs

Comment thread src/watch_tower/nostr_discovery.rs
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/watch_tower/watcher.rs (1)

171-187: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Idle backoff is applied even when a command was just processed.

std::thread::sleep(Duration::from_millis(5)) currently runs whenever backend.poll() is None, including iterations where try_recv() returned Ok(cmd). That adds avoidable command latency and reduces throughput.

Proposed fix
             loop {
+                let mut idle = true;
                 match self.rx_requests.try_recv() {
                     Ok(cmd) => {
+                        idle = false;
                         if !self.handle_command(cmd, &rest_backend_2) {
                             discovery_clone.store(true, Ordering::SeqCst);
                             break;
                         }
                     }
                     Err(TryRecvError::Disconnected) => break,
                     Err(TryRecvError::Empty) => {}
                 }

                 if let Some(event) = self.backend.poll() {
+                    idle = false;
                     self.handle_event(event);
-                } else {
+                }
+
+                if idle {
                     // Avoid busy-looping when there are no commands and no ZMQ events.
                     std::thread::sleep(Duration::from_millis(5));
                 }
             }
🤖 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 `@src/watch_tower/watcher.rs` around lines 171 - 187, The loop currently always
sleeps when backend.poll() returns None, even if rx_requests.try_recv() returned
Ok(cmd) and was handled, causing added latency; change the loop to track whether
any work was done this iteration (e.g., introduce a local bool like did_work),
set it to true when try_recv() yields Ok and handle_command(...) runs (or when
handle_event(...) runs), and only call
std::thread::sleep(Duration::from_millis(5)) when did_work is false; update the
block that matches on rx_requests.try_recv(), the call to handle_command, and
the backend.poll() branch to set that flag accordingly so sleep is skipped after
processing a command or event.
🤖 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.

Outside diff comments:
In `@src/watch_tower/watcher.rs`:
- Around line 171-187: The loop currently always sleeps when backend.poll()
returns None, even if rx_requests.try_recv() returned Ok(cmd) and was handled,
causing added latency; change the loop to track whether any work was done this
iteration (e.g., introduce a local bool like did_work), set it to true when
try_recv() yields Ok and handle_command(...) runs (or when handle_event(...)
runs), and only call std::thread::sleep(Duration::from_millis(5)) when did_work
is false; update the block that matches on rx_requests.try_recv(), the call to
handle_command, and the backend.poll() branch to set that flag accordingly so
sleep is skipped after processing a command or event.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: ff412efb-07e8-4fd3-a48c-7754b722c793

📥 Commits

Reviewing files that changed from the base of the PR and between c8e5c54 and 2aa07b1.

📒 Files selected for processing (4)
  • src/taker/api.rs
  • src/taker/error.rs
  • src/watch_tower/nostr_discovery.rs
  • src/watch_tower/watcher.rs
💤 Files with no reviewable changes (2)
  • src/taker/error.rs
  • src/taker/api.rs

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

♻️ Duplicate comments (1)
src/watch_tower/nostr_discovery.rs (1)

205-210: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win

Preserve the matched error variant for better diagnostics.

The past review comment remains valid: when shutdown is false, line 209 always returns tungstenite::Error::ConnectionClosed.into(), discarding the distinction between ConnectionClosed and AlreadyClosed. Binding the matched error preserves logging fidelity.

♻️ Bind the error to preserve the variant
-            Err(tungstenite::Error::ConnectionClosed | tungstenite::Error::AlreadyClosed) => {
-                if shutdown.load(Ordering::SeqCst) {
-                    return Ok(());
-                }
-                return Err(tungstenite::Error::ConnectionClosed.into());
-            }
+            Err(e @ (tungstenite::Error::ConnectionClosed | tungstenite::Error::AlreadyClosed)) => {
+                if shutdown.load(Ordering::SeqCst) {
+                    return Ok(());
+                }
+                return Err(e.into());
+            }
🤖 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 `@src/watch_tower/nostr_discovery.rs` around lines 205 - 210, The match arm
that currently patterns Err(tungstenite::Error::ConnectionClosed |
tungstenite::Error::AlreadyClosed) should bind the matched error (e.g., using a
variable like `e`) so that when shutdown.load(...) is false you return the
original variant via `e.into()` instead of always returning `ConnectionClosed`;
update the arm in the function handling the websocket read/error (the arm
referencing shutdown and tungstenite::Error::ConnectionClosed |
tungstenite::Error::AlreadyClosed) to bind the error and return that bound error
when not shutting down to preserve diagnostic fidelity.
🤖 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 `@src/taker/legacy_swap.rs`:
- Around line 313-322: The confirmation wait currently uses the `?` operator on
`wallet.wait_for_tx_confirmation(...)` which propagates interruption directly;
change this to mirror the maker path by matching the result and mapping an
interruption error to `TakerError::ContractsBroadcasted(...)` (include the
funding txids as in the maker path) while otherwise returning the original error
or success. Locate the block around `prev_confirm_height = { let wallet =
self.read_wallet()?; wallet.wait_for_tx_confirmation(&funding_txids,
required_confirms, None, None,)? }` in legacy_swap.rs and replace the `?`
propagation with a match that detects the wallet interruption error and returns
`Err(TakerError::ContractsBroadcasted(funding_txids.clone()))`, returning the
normal Ok height for success and propagating other errors unchanged.

In `@src/taker/taproot_swap.rs`:
- Around line 566-570: The current wait_for_tx_confirmation call may propagate
WalletError::Interrupted via ? and lose the recovery-specific
ContractsBroadcasted control flow; change the block that calls
wallet.wait_for_tx_confirmation(&contract_txids, required_confirms, None, None)
to explicitly match its Result and convert an Err(WalletError::Interrupted(_))
into the taker-side recovery error (e.g., return
Err(TakerError::ContractsBroadcasted) or the equivalent variant used in this
module) while still propagating other wallet errors; reference the symbols
wallet.read_wallet(), wait_for_tx_confirmation, WalletError::Interrupted,
contract_txids, required_confirms, and the ContractsBroadcasted taker error when
implementing the match/convert.

In `@src/wallet/api.rs`:
- Around line 2702-2714: The linear backoff computed as total_sleep =
sleep_increment_secs.saturating_mul(attempt) can grow unbounded; change it to
cap growth by introducing a max backoff (e.g., max_backoff_secs) and compute
total_sleep = std::cmp::min(sleep_increment_secs.saturating_mul(attempt),
max_backoff_secs); keep the 1-second loop and shutdown/abort checks as-is and
update the log message to reflect the capped value; reference the variables
total_sleep, sleep_increment_secs, attempt and add a nearby constant like
MAX_BACKOFF_SECS or max_backoff_secs.
- Around line 2671-2690: Replace the wallet-scoped use of getrawtransaction by
calling the wallet RPC gettransaction instead: change the call site using
self.rpc.get_raw_transaction_info(txid, ...) to use the wallet method (e.g.
self.rpc.get_transaction(txid) or the crate's equivalent) and extract
confirmations from the returned wallet transaction struct (e.g.
tx.confirmations.unwrap_or(0)) just like before; keep the existing error
handling/loop logic but rely on the wallet RPC so nodes without txindex won't
permanently fail to find confirmed wallet txs. Ensure you update any type
matches/usages around the returned value to the wallet-transaction type and keep
the same variables (txid, confirms, required_confirms, max_confirm_height,
all_confirmed) referenced in the surrounding code.

---

Duplicate comments:
In `@src/watch_tower/nostr_discovery.rs`:
- Around line 205-210: The match arm that currently patterns
Err(tungstenite::Error::ConnectionClosed | tungstenite::Error::AlreadyClosed)
should bind the matched error (e.g., using a variable like `e`) so that when
shutdown.load(...) is false you return the original variant via `e.into()`
instead of always returning `ConnectionClosed`; update the arm in the function
handling the websocket read/error (the arm referencing shutdown and
tungstenite::Error::ConnectionClosed | tungstenite::Error::AlreadyClosed) to
bind the error and return that bound error when not shutting down to preserve
diagnostic fidelity.
🪄 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: ASSERTIVE

Plan: Pro

Run ID: b0affb52-8cfa-4fcd-9473-f72d4696350f

📥 Commits

Reviewing files that changed from the base of the PR and between 2aa07b1 and 45f0d14.

📒 Files selected for processing (10)
  • src/maker/api.rs
  • src/taker/api.rs
  • src/taker/error.rs
  • src/taker/legacy_swap.rs
  • src/taker/taproot_swap.rs
  • src/wallet/api.rs
  • src/wallet/error.rs
  • src/watch_tower/nostr_discovery.rs
  • src/watch_tower/watcher.rs
  • tests/integration/fidelity.rs
💤 Files with no reviewable changes (1)
  • src/taker/error.rs

Comment thread src/taker/legacy_swap.rs
Comment thread src/taker/taproot_swap.rs
Comment thread src/wallet/api.rs
Comment thread src/wallet/api.rs Outdated
Copy link
Copy Markdown

@mojoX911 mojoX911 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack

@mojoX911
Copy link
Copy Markdown

Looks good to me. Feel free to take any of the other feedbacks given by coderabbit.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (2)
src/taker/taproot_swap.rs (1)

571-575: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Map interruption to a recovery error after broadcast.

At Line 574, ? can propagate WalletError::Interrupted(_) as a generic error even though contract txs are already broadcast (point-of-no-return). Handle interruption explicitly and return TakerError::ContractsBroadcasted(...).

Proposed fix
         let required_confirms = self.swap_state()?.params.required_confirms;
         {
             let wallet = self.read_wallet()?;
-            wallet.wait_for_tx_confirmation(&contract_txids, required_confirms, None, None)?;
+            match wallet.wait_for_tx_confirmation(&contract_txids, required_confirms, None, None) {
+                Ok(_) => {}
+                Err(crate::wallet::WalletError::Interrupted(_)) => {
+                    return Err(TakerError::ContractsBroadcasted(contract_txids.clone()));
+                }
+                Err(e) => return Err(e.into()),
+            }
         }
🤖 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 `@src/taker/taproot_swap.rs` around lines 571 - 575, The call to
wallet.wait_for_tx_confirmation(...) can return WalletError::Interrupted which
currently bubbles up via `?` even after contract_txids were broadcast; update
the error handling around the block that reads the wallet and calls
`wait_for_tx_confirmation(&contract_txids, required_confirms, None, None)` to
catch an interrupted/wallet-interruption error and map it to
`TakerError::ContractsBroadcasted(contract_txids.clone())` (or appropriate
variant) instead of letting the generic error propagate; keep all other errors
propagated as before and ensure you reference `self.swap_state()?` to obtain
`required_confirms` and use `self.read_wallet()?` as before.
src/taker/legacy_swap.rs (1)

314-323: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Preserve ContractsBroadcasted flow for taker funding confirmation interruption.

At Line 317, ? still hides WalletError::Interrupted(_) after funding is already broadcast. Mirror the explicit mapping used elsewhere and return TakerError::ContractsBroadcasted(funding_txids.clone()).

Proposed fix
                 let required_confirms = self.swap_state()?.params.required_confirms;
                 prev_confirm_height = {
                     let wallet = self.read_wallet()?;
-                    wallet.wait_for_tx_confirmation(
-                        &funding_txids,
-                        required_confirms,
-                        None,
-                        None,
-                    )?
+                    match wallet.wait_for_tx_confirmation(
+                        &funding_txids,
+                        required_confirms,
+                        None,
+                        None,
+                    ) {
+                        Ok(h) => h,
+                        Err(crate::wallet::WalletError::Interrupted(_)) => {
+                            return Err(TakerError::ContractsBroadcasted(funding_txids.clone()));
+                        }
+                        Err(e) => return Err(e.into()),
+                    }
                 };
🤖 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 `@src/taker/legacy_swap.rs` around lines 314 - 323, The current call to
wallet.wait_for_tx_confirmation in legacy_swap.rs uses the ? operator which will
propagate WalletError::Interrupted(_) instead of preserving the
ContractsBroadcasted flow; change the error handling around
wait_for_tx_confirmation (called via self.read_wallet()?) to explicitly map
WalletError::Interrupted(_) to return
Err(TakerError::ContractsBroadcasted(funding_txids.clone())) (e.g., match or
map_err on the result of wait_for_tx_confirmation) while letting other errors
propagate as before; locate usages of swap_state(), read_wallet(),
wait_for_tx_confirmation and replace the `?` on that call with explicit mapping
to TakerError::ContractsBroadcasted(funding_txids.clone()) when Interrupted is
encountered.
🤖 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 `@src/watch_tower/watcher.rs`:
- Around line 186-189: Add a short rationale to the idle backoff comment next to
the sleep in watcher.rs (the else branch that calls
std::thread::sleep(Duration::from_millis(5))) explaining why 5ms was
chosen—e.g., it gives ~200Hz polling for reasonable responsiveness while
eliminating busy-loop CPU use—so future readers see the tradeoff without
changing behavior.

---

Duplicate comments:
In `@src/taker/legacy_swap.rs`:
- Around line 314-323: The current call to wallet.wait_for_tx_confirmation in
legacy_swap.rs uses the ? operator which will propagate
WalletError::Interrupted(_) instead of preserving the ContractsBroadcasted flow;
change the error handling around wait_for_tx_confirmation (called via
self.read_wallet()?) to explicitly map WalletError::Interrupted(_) to return
Err(TakerError::ContractsBroadcasted(funding_txids.clone())) (e.g., match or
map_err on the result of wait_for_tx_confirmation) while letting other errors
propagate as before; locate usages of swap_state(), read_wallet(),
wait_for_tx_confirmation and replace the `?` on that call with explicit mapping
to TakerError::ContractsBroadcasted(funding_txids.clone()) when Interrupted is
encountered.

In `@src/taker/taproot_swap.rs`:
- Around line 571-575: The call to wallet.wait_for_tx_confirmation(...) can
return WalletError::Interrupted which currently bubbles up via `?` even after
contract_txids were broadcast; update the error handling around the block that
reads the wallet and calls `wait_for_tx_confirmation(&contract_txids,
required_confirms, None, None)` to catch an interrupted/wallet-interruption
error and map it to `TakerError::ContractsBroadcasted(contract_txids.clone())`
(or appropriate variant) instead of letting the generic error propagate; keep
all other errors propagated as before and ensure you reference
`self.swap_state()?` to obtain `required_confirms` and use `self.read_wallet()?`
as before.
🪄 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: ASSERTIVE

Plan: Pro

Run ID: a6252741-cf8e-493b-bc03-c4956f77be03

📥 Commits

Reviewing files that changed from the base of the PR and between 45f0d14 and d0264e1.

📒 Files selected for processing (10)
  • src/maker/api.rs
  • src/taker/api.rs
  • src/taker/error.rs
  • src/taker/legacy_swap.rs
  • src/taker/taproot_swap.rs
  • src/wallet/api.rs
  • src/wallet/error.rs
  • src/watch_tower/nostr_discovery.rs
  • src/watch_tower/watcher.rs
  • tests/integration/fidelity.rs
💤 Files with no reviewable changes (1)
  • src/taker/error.rs

Comment thread src/watch_tower/watcher.rs
@mojoX911 mojoX911 merged commit 0d5d362 into citadel-tech:master May 12, 2026
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants