Fix multiple bugs - #40
Conversation
list_coins swallowed every per-address query error, so a failing scan (eg electrum unreachable over tor) looked identical to an empty wallet. Track the first error and return it when the scan finds nothing, so the caller sees the real cause. Add a BIP84 testnet derivation vector test proving the hot signer derives the same addresses a BIP84 wallet funds.
list_coins issued two electrum requests per address (get_history then get_tx per hit), so a 100-index scan was ~200+ sequential round trips. Over tor that hangs for minutes on the create-pool screen. Batch listunspent across the whole range in chunks: a handful of round trips, and listunspent already returns only unspent outputs so spent coins are no longer offered. Covered with a regtest test that funds addresses and asserts the batched scan finds exactly them.
blockstream's electrs-esplora rejects a request batch above ~20 items, returning EOF, which made every chunk of the 50-wide scan fail and the wallet look empty. Verified live against blockstream: batches of 10 and 20 succeed, 25+ fail. Drop the batch to 10 for margin across servers. Add regtest coverage for a coin on a change address and a coin past the first batch boundary.
The scan walked the whole 100-index range every time, ~20 batched round trips over tor, and one dropped circuit failed it outright. Stop after a 20-index gap of empty addresses (like a wallet stop gap) so a typical wallet finishes in a batch or two, and retry a batch that errors before giving up. Verified live against blockstream: the change-address coin now scans in ~3s instead of minutes. Covered with a regtest test asserting a coin past the gap is skipped.
A coinjoin waits minutes over tor while peers register, long enough for the electrum connection to go idle and drop. The next request (input verification, broadcast) then failed the whole round with WouldBlock or connection-closed, because nothing reconnected. The electrum client now reconnects once and retries on a transport error, and clone/reconnect remember whether the url was ssl so a rebuilt connection keeps speaking TLS (previously a clone reconnected in plaintext to an ssl port). Also clear the leftover SOCKS handshake read/write timeout before returning the socket, so a slow coinjoin read blocks instead of surfacing WouldBlock. Verified live: get_tx over ssl still works after reconnect; regtest covers recovery after reconnect.
A peer registered its output and its input over the same relay connection, so the same tor exit IP carried both. The relay could then link a peer's input to its output and undo the coinjoin. Rotate to a fresh NostrClient (new SOCKS isolation token, new circuit) between output and input registration, keeping the pool keys and re-subscribing to the pool DMs with no since bound so no already-posted message is missed. Full regtest coinjoin round still completes.
Rotating the nostr connection between output and input registration dropped the connection while the output event could still be buffered (post is fire-and-forget, no relay OK is awaited), so over a real relay the output never reached it and the joiner deadlocked waiting for it. Reference clients keep delivery reliable by awaiting the relay OK; restore the single-connection flow that works, then re-add circuit isolation the reference way (post each registration over its own circuit and wait for the OK).
Post each output and input registration over its own fresh connection (new SOCKS isolation token, new tor circuit) and wait for the relay OK before proceeding, matching the reference clients. This makes a peer's input unlinkable from its output by exit IP and stops a fire-and-forget event from being silently dropped (the cause of the joiner deadlock). The relay OK also yields the event id: current_progress now carries the output/input event ids and the finalized psbt, streamed to the caller (FfiCoinjoinUpdate gains output_event_id/input_event_id/psbt) so the timeline can show them. Full regtest coinjoin round still completes; bindings regenerated.
Measured against a real tor instance: SOCKS isolation tokens reliably force a new circuit (0/12 failed, output and input always landed on different exit IPs across 3 trials), so the per-registration circuit design is right. The failure was the 10s connect budget: building a fresh circuit occasionally took 12s+ in testing, worse on a phone's embedded tor, and surfaced as 'socks5 connect failed' mid-coinjoin. Widen the connect/handshake timeouts and retry an isolated send up to 3 times, each attempt drawing a new circuit so a slow or dead one is routed around instead of failing the round.
📝 WalkthroughWalkthroughThe PR adds coinjoin progress streaming from Rust through Dart FFI, records relay-confirmed registration details, introduces Electrum batching and reconnect retries, expands scanning tests, and centralizes connection timeout handling. ChangesCoinjoin progress streaming
Wallet scanning and network resilience
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant Dart
participant RustApi
participant CoinjoinWorker
participant Joinstr
participant Relay
Dart->>RustApi: start coinjoin with progress sink
RustApi->>CoinjoinWorker: run coinjoin with callback
CoinjoinWorker->>Joinstr: execute registration and coinjoin steps
Joinstr->>Relay: send isolated pool message
Relay-->>Joinstr: confirmed event ID
Joinstr-->>RustApi: CoinjoinProgress
RustApi-->>Dart: progress, done, or failed update
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 7
🤖 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 `@rust/joinstr/src/electrum.rs`:
- Around line 174-192: Preserve the certificate-verification setting across
client reconstruction: add a Client field for the setting, initialize it to
false in new_local and true in other constructors, and apply it when rebuilding
RawClient. Update Clone::clone and reconnect to call verif_certificate using the
stored value, while retaining the existing URL, port, proxy, and SSL behavior.
- Around line 174-192: Update Client::clone to avoid constructing or
reconnecting a network client, keeping cloning cheap and infallible while
preserving the client’s connection configuration and state as appropriate. Add a
fallible try_clone or reconnect method for callers that need a fresh connection,
moving the current RawClient::new_ssl_maybe, proxy setup, and try_connect
behavior there, and update fresh-connection call sites to use it.
- Around line 292-376: Update list_unspent_batch_inner to handle Response::Error
alongside Response::SHListUnspent: when its request ID exists in position,
remove the ID from position and self.index, decrement pending, and propagate the
contained error immediately. Preserve cleanup of any remaining pending IDs
before returning.
- Around line 54-61: Update the error flow used by broadcast_inner() to
represent server-side broadcast rejections with a distinct non-retryable Error
variant instead of Error::Electrum(...). Adjust is_retryable() so only
transport/framing Electrum errors and WrongResponse remain retryable, and update
any matching or propagation sites required for the new variant.
In `@rust/joinstr/src/interface.rs`:
- Around line 188-242: The scan currently suppresses failed-batch errors
whenever other batches return coins, allowing incomplete results to appear
successful. Update the error handling after signer.list_coins() so any recorded
first_error is propagated regardless of whether coins is empty or non-empty,
preserving successful results only when every batch completes successfully.
In `@rust/joinstr/src/joinstr/mod.rs`:
- Around line 882-901: Update the registration flows centered on
register_output, register_input, register_outputs, and start_coinjoin_blocking
so no inner mutex guard remains held during send_pool_message_isolated or other
network waits: capture required state while locked, release the guard, perform
the call, then briefly re-acquire it to store event IDs and registration data.
Also make current_step and current_progress recover from a poisoned mutex by
reading the inner value instead of panicking, preserving progress polling and
controlled worker-panic handling.
In `@rust/joinstr/src/nostr/sync/mod.rs`:
- Around line 198-232: The retry loop in send_pool_message_isolated can resend a
registration after a local confirmation timeout, creating duplicate Output
events. Make retries idempotent by verifying the prior event was not accepted
before resending, or by preserving the sender public key through
try_receive_pool_msg and deduplicating registrations in
Joinstr::receive_outputs; ensure duplicate outputs do not increase
coinjoin.outputs_len() or invalidate register_outputs participant checks.
🪄 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: ee3b6cdf-0216-497a-ae12-4e151f572ab6
⛔ Files ignored due to path filters (4)
dart/lib/src/generated/api/joinstr.dartis excluded by!**/generated/**dart/lib/src/generated/api/types.dartis excluded by!**/generated/**dart/lib/src/generated/frb_generated.dartis excluded by!**/generated/**dart/lib/src/generated/frb_generated.io.dartis excluded by!**/generated/**
📒 Files selected for processing (12)
dart/ios/Classes/frb_generated.hdart/rust/src/api/joinstr.rsdart/rust/src/api/types.rsdart/rust/src/frb_generated.rsrust/joinstr/src/electrum.rsrust/joinstr/src/interface.rsrust/joinstr/src/joinstr/mod.rsrust/joinstr/src/nostr/sync/mod.rsrust/joinstr/src/signer/mod.rsrust/joinstr/tests/scan.rsrust/simple_nostr_client/src/lib.rsrust/socks5/src/lib.rs
| pub fn send_pool_message_isolated( | ||
| &self, | ||
| npub: &PublicKey, | ||
| msg: PoolMessage, | ||
| proxy: Option<String>, | ||
| ) -> Result<EventId, Error> { | ||
| let relay = self.get_relay().ok_or(Error::NotConnected)?; | ||
| let keys = self.get_keys()?.clone(); | ||
| let content = msg.to_string()?; | ||
|
|
||
| // Each attempt opens a new connection, which draws a new SOCKS isolation | ||
| // token and therefore a different tor circuit. Building a fresh circuit | ||
| // occasionally overshoots the connect budget or picks a dead relay; a | ||
| // retry routes around it on another circuit instead of failing the round. | ||
| let mut last_err = None; | ||
| for attempt in 0..ISOLATED_SEND_ATTEMPTS { | ||
| let result = WsClient::new() | ||
| .relay(relay.clone()) | ||
| .proxy(proxy.clone()) | ||
| .keys(keys.clone()) | ||
| .connect() | ||
| .and_then(|mut c| c.send_dm_confirmed(content.clone(), npub, CONFIRM_TIMEOUT)); | ||
| match result { | ||
| Ok(id) => return Ok(id), | ||
| Err(e) => { | ||
| log::warn!("send_pool_message_isolated attempt {attempt} failed: {e:?}"); | ||
| last_err = Some(e); | ||
| if attempt + 1 < ISOLATED_SEND_ATTEMPTS { | ||
| std::thread::sleep(Duration::from_secs(2)); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| Err(last_err.expect("at least one attempt").into()) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Retry-on-timeout can duplicate output/input registration; receiving side has no dedup.
If an attempt's send_dm_confirmed fails only because the local wait for OK timed out (not because the relay actually rejected the event), the retry sends the same registration again from a new connection — but with the same identity keys. Both events can land on the relay. On the receiving side, Joinstr::receive_outputs has an existing // FIXME: should we check if the output have been added? and try_receive_pool_msg doesn't even retain the sender pubkey for PoolMessage::Output, so there's no way to detect or drop the duplicate. A spurious retry can therefore inflate coinjoin.outputs_len() with a phantom extra output, breaking the payload.peers participant-count check in register_outputs.
This retry design meaningfully increases how often that pre-existing gap gets exercised. Consider making retries idempotent (e.g. verify the previous event actually wasn't accepted before resending, or dedupe registrations by sender pubkey on the receiving side).
🤖 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 `@rust/joinstr/src/nostr/sync/mod.rs` around lines 198 - 232, The retry loop in
send_pool_message_isolated can resend a registration after a local confirmation
timeout, creating duplicate Output events. Make retries idempotent by verifying
the prior event was not accepted before resending, or by preserving the sender
public key through try_receive_pool_msg and deduplicating registrations in
Joinstr::receive_outputs; ensure duplicate outputs do not increase
coinjoin.outputs_len() or invalidate register_outputs participant checks.
kwsantiago
left a comment
kwsantiago
left a comment
There was a problem hiding this comment.
Review of the electrum batching rewrite and the confirmed-send path. Tor plumbing itself looks right: no clearnet fallback on any of the touched connect sites, remote DNS is genuine, and the reverted mid-round rotation is superseded by a fresh isolated client per registration. Blockers are concentrated in the batch scan and the new send_pool_message_isolated retry.
| } | ||
| }; | ||
| for response in responses { | ||
| if let Response::SHListUnspent(r) = response { |
There was a problem hiding this comment.
pending only decrements on Response::SHListUnspent with a matching id. An electrum error for one batch element parses as Response::Error, so pending never reaches 0 and the loop re-enters inner.recv(), which blocks in read_line with no read timeout (the client never sets one, and socks5::connect clears the handshake deadline before returning the socket).
A server that answers one of the ten ids with a JSON-RPC error and keeps the socket open hangs the scan or coinjoin thread forever. with_reconnect can't help because no error is ever returned. The replaced get_coins_at did a single recv and returned WrongResponse, so this is a regression.
Decrement on any response whose id is in position, surface Response::Error, and put a deadline on the while pending > 0 loop.
| /// Whether this error is worth reconnecting and retrying once. Transport and | ||
| /// framing failures (dropped/stale tor circuit, `WouldBlock`, a desynced | ||
| /// response) are; logical outcomes (tx absent, unparseable) are not. | ||
| fn is_retryable(&self) -> bool { |
There was a problem hiding this comment.
broadcast_inner returns Error::Electrum(..) for server rejections (bad fee, conflicting spend), not just transport failures. Those are classified retryable here, so with_reconnect tears down the circuit and rebroadcasts a tx the server already definitively rejected, then reports the second attempt's error instead of the first.
Separate transport errors (the From<raw_client::Error> path) from application-level rejections, or keep broadcast out of with_reconnect.
| // Preserve ssl too, or the clone would reconnect in plaintext. | ||
| let mut inner = | ||
| RawClient::new_ssl_maybe(&self.url, self.port, self.ssl).proxy(self.proxy.clone()); | ||
| inner.try_connect().expect("electrum reconnect on clone"); |
There was a problem hiding this comment.
Clone now performs a full connect and panics on failure. WpkhHotSigner derives Clone, so this is a network-triggerable panic reachable from start_coinjoin. Over tor it is likely, since the electrum connect timeout is still 10s while nostr's went to 60s (see simple_nostr_client/src/lib.rs).
Worse, the panic can fire while inner is held, poisoning the mutex so the progress poller's .expect("poisoned") panics too instead of yielding CoinjoinThreadPanicked.
Also: new_local builds the inner client with .verif_certificate(false) but Client doesn't record that, so both this and reconnect() rebuild with the verifying config. A regtest/self-signed client fails TLS on its first reconnect, and here that failure is a panic. Store the flag next to ssl.
| } | ||
|
|
||
| let client = self.client.as_mut().ok_or(Error::NoElectrumClient)?; | ||
| let batch = client.list_unspent_batch(&spks)?; |
There was a problem hiding this comment.
The batch path builds TxOut straight from the server's listunspent reply. The replaced get_coins_at fetched the raw tx and asserted txout.script_pubkey == *script, so both value and outpoint were verified.
A hostile electrum server can now inject phantom outpoints or inflated values into the coin picker. The BIP143 sighash commits to the false amount and the coinjoin tx is invalid at broadcast, after this peer has already published its input outpoint and a fresh output address to the pool. Consensus bounds the fund loss; the anonymity-set burn is real and cheap to trigger.
Also, coin_paths.iter().zip(batch) correlates positionally. list_unspent_batch does return in request order, but its recv error path breaks with a partial out, which would misattribute coins to the wrong derivation path. Key by request id instead.
| // than the output) and wait for the relay OK. | ||
| let event_id = | ||
| self.client | ||
| .send_pool_message_isolated(&npub, msg, self.proxy.clone())?; |
There was a problem hiding this comment.
register_input does self.input.take() at :1555, before this send. The send is now confirmed and retried (3 attempts x 60s connect + 30s handshake + 30s OK wait).
A relay that OKs the output registration and then withholds OK on the input leaves the peer, after ~6 minutes, with its fresh output address already published and pool-associated, and self.input consumed so state() serializes input: None and restart() can never re-register it. Under the old fire-and-forget send_pool_message this path essentially never failed; Error::OkTimeout makes it reachable.
Take the input only after a confirmed send, or restore it on error.
| // occasionally overshoots the connect budget or picks a dead relay; a | ||
| // retry routes around it on another circuit instead of failing the round. | ||
| let mut last_err = None; | ||
| for attempt in 0..ISOLATED_SEND_ATTEMPTS { |
There was a problem hiding this comment.
Worst case per call is 3 x (60s connect + 30s confirm + 2s sleep) ~= 4.6 min, and this runs twice per round (output then input). Both callers hold the JoinstrInner lock across it (joinstr/mod.rs:707, :1040), so ~9 min can be spent against a pool timeout that is usually shorter: the retries make the round die rather than survive.
The held lock also means the new progress poller blocks during the longest step, so nothing is streamed exactly where the feature is most useful. And a stalling relay keeps the instance pinned while simple_nostr_client's unbounded mpsc queue accumulates 512 KiB messages with no count cap.
Budget the total against end_timeline instead of a fixed attempt count, and drop the lock around the send.
| /// returns once a fresh circuit is built, which was measured to spike past 12s; | ||
| /// on a phone's embedded tor it is slower still. A 10s cap surfaced as | ||
| /// "socks5 connect failed" mid-coinjoin. | ||
| const CONNECT_TIMEOUT: Duration = Duration::from_secs(60); |
There was a problem hiding this comment.
This widening is correct, but the electrum side was left behind: simple_electrum_client keeps CONNECT_TIMEOUT = 10s in both tcp_client.rs and ssl_client.rs. The reasoning in this comment applies identically there, so Client::reconnect() over tor (the point of 5fd842c) will keep failing with "socks5 connect failed", and via Clone that failure is a panic.
Worth hoisting one constant into the socks5 crate so all three paths move together.
| let coins = signer.list_coins().into_iter().map(|c| c.1).collect(); | ||
| let coins: Vec<Coin> = signer.list_coins().into_iter().map(|c| c.1).collect(); | ||
|
|
||
| if coins.is_empty() { |
There was a problem hiding this comment.
A batch that fails all BATCH_RETRIES sets first_error, but it is only surfaced when the coin list is empty. If any other batch found a coin, the failure is dropped and the caller gets a silently partial coin set, then picks an input from that partial view. That is the opposite of "surface electrum scan failures".
Compounding it: a failed batch contributes nothing to consecutive_empty and nothing to the results, so the caller can't distinguish "no coins" from "half the scan failed". Return the error whenever any batch failed, or signal partiality explicitly.
| Some(n) if n > 0 => consecutive_empty = 0, | ||
| Some(_) => { | ||
| consecutive_empty += end - index; | ||
| if consecutive_empty >= SCAN_STOP_GAP { |
There was a problem hiding this comment.
The gap limit stops the scan regardless of the range the caller asked for, so list_coins(.., (0, 100), ..) reports "no coins" for a wallet whose funds sit past index 20, with no signal. tests/scan.rs:104 encodes this as intended, but from the FFI it is a silent behavior change from the previous full-range scan. At minimum surface "stopped at gap" to the caller.
Minor, same block: let end = (index + INDEXES_PER_BATCH).min(range.1) at :197 overflows for a range near u32::MAX (check_scan_range only bounds the span), and consecutive_empty += end - index then underflows. saturating_add/saturating_sub.
| let _ = progress.add(FfiCoinjoinUpdate::failed(e.to_string())); | ||
| } | ||
| } | ||
| Ok(()) |
There was a problem hiding this comment.
A failed coinjoin is reported only through the stream, and the function still returns Ok(()). If the Dart subscription is cancelled the sink add fails, let _ = discards it, and the call reads as success. Same in initiate_coinjoin. Return Err(e.into()) in addition to pushing the update.
Unrelated but adjacent: FfiCoinjoinUpdate::done/failed null out output_event_id, input_event_id and psbt, so a UI rendering the latest update loses them exactly at the terminal step.
list_unspent_batch only decremented its pending count on SHListUnspent, so an electrum error for one id in the batch left that id pending forever. recv() then blocked in read_line with no read timeout, hanging the scan or coinjoin thread with no error for with_reconnect to act on. The replaced get_coins_at returned WrongResponse after a single recv, so this was a regression. Account for any response carrying one of our ids, surface Response::Error, and bound the collect loop with a deadline.
The batch path built each coin straight from the server's listunspent reply, so a hostile electrum server could inject outpoints that do not exist or inflate their value. Signing commits to the amount, so a lie produced an invalid signature and killed the coinjoin only after the peer had published its input outpoint and a fresh output address to the pool. The replaced per-address path fetched the transaction and matched its script_pubkey; do the same for every candidate and keep the chain's value, fetching only coins that were actually found. Also assert the batch length so positional results cannot be attributed to the wrong derivation path.
The connect timeout was raised for the nostr relay but both electrum clients kept a 10s cap, so Client::reconnect() over tor kept failing with the same 'socks5 connect failed' the widening was meant to fix, and via Clone that failure is a panic. Hoist one CONNECT_TIMEOUT/HANDSHAKE_TIMEOUT pair into the socks5 crate that every dialer shares, so the paths cannot drift apart again.
register_input took self.input before publishing, which was harmless while the send was fire-and-forget. Waiting for the relay OK makes failure reachable (OkTimeout, rejection), and the input was then gone: state() serialized input: None so restart() could never re-register it, stranding a peer whose fresh output address was already published and pool-associated. Restore the input when the send fails.
register_output and register_input ran the isolated send while their caller held the inner lock. Publishing opens a fresh connection per attempt and can take minutes, so current_step and current_progress blocked on the same mutex for that whole window: the poller saw one coalesced update instead of the streaming progress the FFI exposes, exactly during the longest step. A panic there also poisoned the mutex, so the poller's own lock panicked instead of the worker's join surfacing CoinjoinThreadPanicked. Split each registration into prepare (locked), send (unlocked) and commit (locked), and let the pollers recover from poisoning. Committing only after a confirmed send also means the input is cleared only once it is really registered.
A batch that exhausted its retries only surfaced when the whole scan came back empty, so if any other batch found a coin the failure was dropped and the caller received a silently partial set, then picked an input from that incomplete view. Return the error whenever a batch failed, so an incomplete scan cannot pass for a complete one.
…econnect is_retryable matched every Error::Electrum, but broadcast_inner used that variant for server rejections too (bad fee, conflicting spend). A rejected broadcast was therefore torn down and resent, and the caller saw the second attempt's error instead of the verdict. Give rejections their own non-retryable variant. Separately, Client never recorded new_local's verif_certificate(false), so a self-signed client silently rebuilt with strict verification on clone or reconnect and failed TLS; store the flag alongside ssl and reapply it.
check_scan_range bounds the span but not the start, so a range near u32::MAX overflowed computing the batch end and then underflowed accumulating the empty-index gap.
initiate_coinjoin and join_coinjoin reported failures only through the progress sink and still returned Ok, so a cancelled Dart subscription (where add is dropped) turned a failed coinjoin into a success. Return the error as well. The terminal done/failed updates also nulled out output_event_id, input_event_id and psbt, losing them for any consumer that renders just the latest update; carry the last observed detail into them.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
rust/joinstr/src/electrum.rs (3)
236-285: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
reconnect()(and by the same defect,Clone) drop thenew_localcertificate-verification override.
reconnect()rebuilds viaRawClient::new_ssl_maybe(&self.url, self.port, self.ssl).proxy(self.proxy.clone())— no.verif_certificate(false)call — andClienthas no field recording that it was built withnew_local()(line 274 sets.verif_certificate(false)on the initial connect only; the struct only storesssl, not verification mode). Sincewith_reconnectnow wraps every single operation (list_unspent_batch,get_tx,get_coins_tx_at,broadcast), a regtest/self-signed client will fail its TLS handshake on the very first retryable failure of any call, not just on an explicitreconnect()/clone()invocation as before — this is a regression in blast radius versus the prior state.🔧 Suggested fix
pub struct Client { inner: RawClient, index: HashMap<usize, Request>, last_id: usize, url: String, port: u16, proxy: Option<String>, ssl: bool, + verify_certificate: bool, }Set
trueinnew/new_with_proxy,falseinnew_local, and apply.verif_certificate(self.verify_certificate)in bothreconnect()andClone.🤖 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 `@rust/joinstr/src/electrum.rs` around lines 236 - 285, Track the certificate-verification mode on Client: initialize it to true in new and new_with_proxy, and false in new_local. Update reconnect() and the Clone implementation to apply verif_certificate using this stored verify_certificate value when rebuilding RawClient, preserving new_local’s self-signed certificate behavior across retries and clones.
358-399: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winNo early exit once a batch item errors; loop keeps waiting up to
BATCH_TIMEOUT.Once
resultbecomesErr(390 or 395), the outerwhile pending > 0loop keeps running until every remaining id resolves or the full 120s deadline elapses, even though the batch is already known to fail. Breaking as soon asresult.is_err()avoids an unnecessary multi-minute wait on the caller thread.🔧 Suggested fix
- let responses = match self.inner.recv(&self.index) { + let responses = match self.inner.recv(&self.index) { Ok(r) => r, Err(e) => { result = Err(e.into()); break; } }; for response in responses { ... } + if result.is_err() { + break; + } }🤖 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 `@rust/joinstr/src/electrum.rs` around lines 358 - 399, Update the response-processing flow around the `result` assignment and outer `while pending > 0` loop to stop waiting once any batch item sets `result` to `Err`. Preserve the existing first-error behavior and response handling, but exit the batch wait promptly instead of continuing until remaining IDs resolve or `BATCH_TIMEOUT` expires.
341-357: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBound each
recv()instead of only checking the deadline before it.
BATCH_TIMEOUTis checked only when the batch loop reaches a newrecv()call. SinceRawClientdefaults to blocked reads andread_line_cappedcallsstream.read()one byte at a time, the first unread line inpending > 0can still block indefinitely if the server keeps the socket open but sends no further plaintext. Apply or preserve per-call socket/TLS read timeouts in the underlying client rather than relying on the between-call deadline.🤖 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 `@rust/joinstr/src/electrum.rs` around lines 341 - 357, Bound each blocking recv operation in the batch response loop to BATCH_TIMEOUT by configuring or preserving the underlying RawClient socket/TLS read timeout, rather than checking the deadline only before recv. Update the client setup used by self.inner.recv so a stalled read returns an error or timeout and the existing Error::BatchTimeout handling remains effective.
♻️ Duplicate comments (2)
rust/joinstr/src/nostr/sync/mod.rs (1)
226-244: 🗄️ Data Integrity & Integration | 🟠 MajorThe isolated registration retry is still non-idempotent and not deadline-aware.
post_event_confirmedsends the event before waiting forOK; if that wait returnsOkTimeout, this loop publishes a second registration. The configured budgets alone allow roughly 3 × (60s + 30s) + 2 × 2s ≈ 274 seconds, independent of the pool deadline. Retry only failures known to occur before publishing, or add idempotency/deduplication and cap retries by the remaining round deadline.🤖 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 `@rust/joinstr/src/nostr/sync/mod.rs` around lines 226 - 244, The retry loop around WsClient::connect and send_dm_confirmed must avoid republishing registrations after an event may already have been sent and must respect the pool’s remaining deadline. Restrict retries to failures definitively occurring before publication, or introduce idempotency/deduplication, and cap each attempt, wait, and retry delay using the remaining round deadline before returning the last error.rust/joinstr/src/electrum.rs (1)
62-68: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
is_retryable()still over-matches rejections and now under-matchesBatchTimeout.Two issues in the retryable classification:
Error::Electrum(_)still coversbroadcast_inner's server-rejection path (bad fee, conflicting spend — line 876), sowith_reconnecttears down the circuit and resends a transaction the server already definitively rejected, reporting the second attempt's error instead of the original rejection. Flagged in prior reviews and unchanged here.- The new
Error::BatchTimeout(line 40) is not included, so a batched request that hits the deadline never gets the one-shot reconnect-and-retry thatwith_reconnectexists for — exactly the scenario (a possibly-stale/hung connection) this mechanism was designed to recover from.🔧 Suggested fix
fn is_retryable(&self) -> bool { - matches!(self, Error::Electrum(_) | Error::WrongResponse) + matches!(self, Error::WrongResponse | Error::BatchTimeout) }Give broadcast/batch rejections (and any other definitive server-side error) their own non-retryable variant instead of overloading
Error::Electrum(_), so transport failures stay retryable while application rejections don't.🤖 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 `@rust/joinstr/src/electrum.rs` around lines 62 - 68, Update Error::is_retryable to include Error::BatchTimeout while excluding definitive broadcast/batch server rejections from the retryable classification. Introduce or reuse a distinct non-retryable error variant at the broadcast_inner rejection path instead of wrapping those outcomes in Error::Electrum(_), preserving retry behavior for transport/framing failures and preventing resubmission of rejected transactions.
🧹 Nitpick comments (1)
rust/joinstr/src/electrum.rs (1)
406-430: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftPer-coin verification defeats part of the batching win.
get_txis called once per candidate outpoint (417), sequentially, each going through its ownwith_reconnect/round trip. This is scoped to only scripts with actual unspent outputs (not the full scanned set), so the primary batching win (avoiding one round trip per address) is preserved, but for wallets with many funded UTXOs this still serializes a round trip per coin over Tor — the exact latency profile batching was meant to avoid. A simple mitigation: cache fetched transactions bytxid(a single funding tx can pay multiple scanned addresses/coins) to avoid duplicate fetches; a fuller fix would need a batched tx-fetch API.🤖 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 `@rust/joinstr/src/electrum.rs` around lines 406 - 430, Update the per-candidate verification loop in the outpoint-processing flow to cache fetched transactions by txid and reuse the cached transaction for subsequent outpoints, while preserving the existing output-index and script_pubkey validation. Keep the cache scoped to this batch operation and retain the current error behavior for missing transactions or invalid outpoints.
🤖 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 `@rust/joinstr/src/electrum.rs`:
- Around line 236-285: Track the certificate-verification mode on Client:
initialize it to true in new and new_with_proxy, and false in new_local. Update
reconnect() and the Clone implementation to apply verif_certificate using this
stored verify_certificate value when rebuilding RawClient, preserving
new_local’s self-signed certificate behavior across retries and clones.
- Around line 358-399: Update the response-processing flow around the `result`
assignment and outer `while pending > 0` loop to stop waiting once any batch
item sets `result` to `Err`. Preserve the existing first-error behavior and
response handling, but exit the batch wait promptly instead of continuing until
remaining IDs resolve or `BATCH_TIMEOUT` expires.
- Around line 341-357: Bound each blocking recv operation in the batch response
loop to BATCH_TIMEOUT by configuring or preserving the underlying RawClient
socket/TLS read timeout, rather than checking the deadline only before recv.
Update the client setup used by self.inner.recv so a stalled read returns an
error or timeout and the existing Error::BatchTimeout handling remains
effective.
---
Duplicate comments:
In `@rust/joinstr/src/electrum.rs`:
- Around line 62-68: Update Error::is_retryable to include Error::BatchTimeout
while excluding definitive broadcast/batch server rejections from the retryable
classification. Introduce or reuse a distinct non-retryable error variant at the
broadcast_inner rejection path instead of wrapping those outcomes in
Error::Electrum(_), preserving retry behavior for transport/framing failures and
preventing resubmission of rejected transactions.
In `@rust/joinstr/src/nostr/sync/mod.rs`:
- Around line 226-244: The retry loop around WsClient::connect and
send_dm_confirmed must avoid republishing registrations after an event may
already have been sent and must respect the pool’s remaining deadline. Restrict
retries to failures definitively occurring before publication, or introduce
idempotency/deduplication, and cap each attempt, wait, and retry delay using the
remaining round deadline before returning the last error.
---
Nitpick comments:
In `@rust/joinstr/src/electrum.rs`:
- Around line 406-430: Update the per-candidate verification loop in the
outpoint-processing flow to cache fetched transactions by txid and reuse the
cached transaction for subsequent outpoints, while preserving the existing
output-index and script_pubkey validation. Keep the cache scoped to this batch
operation and retain the current error behavior for missing transactions or
invalid outpoints.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d7916be7-2b85-447d-918b-b8be9c67c5fd
📒 Files selected for processing (8)
rust/joinstr/src/electrum.rsrust/joinstr/src/joinstr/mod.rsrust/joinstr/src/nostr/sync/mod.rsrust/joinstr/src/signer/mod.rsrust/simple_electrum_client/src/raw_client/ssl_client.rsrust/simple_electrum_client/src/raw_client/tcp_client.rsrust/simple_nostr_client/src/lib.rsrust/socks5/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- rust/joinstr/src/signer/mod.rs
- rust/simple_nostr_client/src/lib.rs
- rust/joinstr/src/joinstr/mod.rs
|
Reviewed this while wiring the bindings into the wallet (both a general correctness pass and a security-focused pass). The privacy/consensus-critical parts look solid: SOCKS5 has no direct-connection fallback and still refuses no-auth (isolation preserved), the signer/SIGHASH One blocker and two smaller items: Blocker:
|
Nothing set a read timeout on the electrum socket: the ssl client restores the default None after its handshake and socks5::connect clears the handshake deadline before returning. BATCH_TIMEOUT is only evaluated between completed reads, so a server that answers some batch ids and then holds the socket open without a trailing newline parked read_line in a blocking read forever; the same applied to get_tx and broadcast. The coinjoin worker wedged and no terminal update ever reached the caller. Set an actual socket read timeout on every connect path; hitting it surfaces as a retryable error so with_reconnect rebuilds the circuit.
broadcast runs through with_reconnect, so a transaction that reached the server and was accepted but whose reply desynced gets rebroadcast; the server then answers 'already known', which mapped to BroadcastRejected. The round was reported failed with final_tx unset even though its transaction was on the network. Recognize the already-known wordings from Core, electrs and ElectrumX and return success instead, with tests pinning both directions.
No. It cannot link anything when everything is registered in the channel with the same key. |
There was a problem hiding this comment.
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 (2)
rust/joinstr/src/electrum.rs (1)
386-389: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMake
BatchTimeoutretryable.The batch path returns
Error::BatchTimeout, butError::is_retryable()only matchesElectrumandWrongResponse. Thereforewith_reconnect()returns this timeout without reconnecting, so a partially stalled connection can still fail scanning instead of recovering.🔧 Suggested fix
- matches!(self, Error::Electrum(_) | Error::WrongResponse) + matches!( + self, + Error::Electrum(_) | Error::WrongResponse | Error::BatchTimeout + )Add a regression test covering the timeout/reconnect path.
🤖 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 `@rust/joinstr/src/electrum.rs` around lines 386 - 389, Update Error::is_retryable() to classify Error::BatchTimeout as retryable alongside Electrum and WrongResponse, ensuring with_reconnect() reconnects after batch timeouts. Add a regression test covering a BatchTimeout that triggers reconnect and retry behavior.dart/rust/src/api/types.rs (1)
298-306: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve
Step::Failedin the FFI mapping.Line 306 converts
Step::FailedtoOther, soFfiCoinjoinUpdate::progressemits an ambiguous update even though the FFI contract has an explicitFailedstate. Map onlyUnconfiguredandConfiguredtoOther.Proposed fix
- Step::Unconfigured | Step::Configured | Step::Failed => FfiCoinjoinStep::Other, + Step::Unconfigured | Step::Configured => FfiCoinjoinStep::Other, + Step::Failed => FfiCoinjoinStep::Failed,🤖 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 `@dart/rust/src/api/types.rs` around lines 298 - 306, Update the From<Step> implementation for FfiCoinjoinStep so Step::Failed maps to the explicit FfiCoinjoinStep::Failed variant, while only Step::Unconfigured and Step::Configured map to Other.
🤖 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 `@rust/joinstr/src/electrum.rs`:
- Around line 80-85: Update is_already_known to remove the broad
m.contains("duplicate") condition and recognize only specific already-known
transaction phrases or error codes, while preserving the existing already/known,
mempool, and chain matching. Add negative coverage for messages such as
"duplicate inputs" to ensure rejected transactions are not reported as
successful broadcasts.
---
Outside diff comments:
In `@dart/rust/src/api/types.rs`:
- Around line 298-306: Update the From<Step> implementation for FfiCoinjoinStep
so Step::Failed maps to the explicit FfiCoinjoinStep::Failed variant, while only
Step::Unconfigured and Step::Configured map to Other.
In `@rust/joinstr/src/electrum.rs`:
- Around line 386-389: Update Error::is_retryable() to classify
Error::BatchTimeout as retryable alongside Electrum and WrongResponse, ensuring
with_reconnect() reconnects after batch timeouts. Add a regression test covering
a BatchTimeout that triggers reconnect and retry behavior.
🪄 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 Plus
Run ID: 86325dc6-f3fd-4e1c-9e87-44de26c30166
📒 Files selected for processing (4)
dart/rust/src/api/joinstr.rsdart/rust/src/api/types.rsrust/joinstr/src/electrum.rsrust/joinstr/src/interface.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- rust/joinstr/src/interface.rs
- dart/rust/src/api/joinstr.rs
| fn is_already_known(message: &str) -> bool { | ||
| let m = message.to_ascii_lowercase(); | ||
| m.contains("already") && (m.contains("known") || m.contains("mempool") || m.contains("chain")) | ||
| || m.contains("duplicate") | ||
| || m.contains("txn-already-in-mempool") | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not treat every duplicate rejection as an already-known transaction.
The bare m.contains("duplicate") branch also classifies unrelated validation failures such as "duplicate inputs" as successful broadcasts. That can report coinjoin completion even though the transaction was rejected. Match specific already-known phrases/codes and add negative coverage.
🔧 Suggested fix
- || m.contains("duplicate")
+ || m.contains("duplicate transaction") for msg in [
"min relay fee not met",
+ "duplicate inputs",
"bad-txns-inputs-missingorspent",Also applies to: 980-1014
🤖 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 `@rust/joinstr/src/electrum.rs` around lines 80 - 85, Update is_already_known
to remove the broad m.contains("duplicate") condition and recognize only
specific already-known transaction phrases or error codes, while preserving the
existing already/known, mempool, and chain matching. Add negative coverage for
messages such as "duplicate inputs" to ensure rejected transactions are not
reported as successful broadcasts.
|
You're right, and I'll retract the linkability note. I traced it against the NIP and the code: The socket-read-timeout blocker and the "successful broadcast reported as failure" item are independent of the key model and still stand. |
2 participants
These bugs were discovered while working on bull wallet implementation in SatoshiPortal/bullbitcoin-mobile#2441
Summary by CodeRabbit