fix(nostr): advance discovery cursor only after successful event processing#835
fix(nostr): advance discovery cursor only after successful event processing#835keshav0479 wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughControl flow refactoring in Nostr transaction discovery processing: cursor persistence is deferred to occur conditionally after transaction handling rather than unconditionally before cache checks. Error handling for transaction fetches uses explicit Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 the current code and only fix it if needed.
Inline comments:
In `@src/watch_tower/nostr_discovery.rs`:
- Around line 231-234: The log message "Received invalid txid" is misleading
because bitcoin_rpc.get_raw_tx can fail transiently; update the error handling
around bitcoin_rpc.get_raw_tx(&txid) (the Ok(tx) else branch) to log a
descriptive message indicating an RPC/fetch failure rather than an invalid txid,
include the captured error details (error variable from the Err arm) and the
txid in the message (e.g., "Failed to fetch raw tx for {txid}: {error}"), and
keep the same control flow (returning Ok(())) after logging.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: aa1a0861-fb8b-486e-95b8-dfc0a9726d8f
📒 Files selected for processing (2)
src/watch_tower/nostr_discovery.rssrc/watch_tower/utils.rs
There was a problem hiding this comment.
Pull request overview
This PR targets a correctness issue in the watchtower’s Nostr-based discovery flow by changing when the per-relay discovery cursor and txid dedup cache are advanced, aiming to avoid permanently skipping events when Bitcoin REST lookups fail.
Changes:
- Added a read-only
SeenTxids::containshelper to support pre-fetch dedup checks. - Reordered
handle_relay_messageto run dedup and transaction fetch/processing before committingseen_txidand the persisted Nostr cursor.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/watch_tower/utils.rs |
Adds SeenTxids::contains to allow dedup checks without mutating the seen set. |
src/watch_tower/nostr_discovery.rs |
Reorders Nostr event handling to defer committing cursor/seen-txid until after successful processing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
keshav0479
force-pushed
the
feat/nostr-cursor-ordering-fix
branch
from
33288c2 to
0598da2
Compare
|
This section of the codebase is going to be heavily refactored soon. Looping in @stark-3k . Pls check if theres anything in this PR you can find useful. |
yes this part will be refactored but we can keep the PR on hold for now |
| if seen_txid.lock()?.insert(txid) { | ||
| log::debug!("add new cache {}", txid); | ||
| let Ok(tx) = bitcoin_rpc.get_raw_tx(&txid) else { | ||
| log::debug!("Received invalid txid: {txid:?}"); |
There was a problem hiding this comment.
You can just save the nostr cursor within this Ok() branch to get the same effect. That would be a much smaller change set. No need to restructure the whole logic.
There was a problem hiding this comment.
@stark-3k do you think we can get this a separate PR or you wanna handle this in your rewrite?
keshav0479
force-pushed
the
feat/nostr-cursor-ordering-fix
branch
from
0598da2 to
0f3d2b4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/watch_tower/nostr_discovery.rs`:
- Around line 225-233: The code currently inserts txid into the SeenTxids set
before fetching/processing the transaction (seen_txid.lock()?.insert(txid)
happens before bitcoin_rpc.get_raw_tx), which causes txids to be marked seen on
fetch failure and later silently skipped; fix by deferring the insertion into
SeenTxids until after bitcoin_rpc.get_raw_tx succeeds and after any processing
that must not be retried, i.e., move the call to seen_txid.lock()?.insert(txid)
to immediately after successful retrieval/processing of tx (and do the same
adjustment for the duplicate-handling branch around the code at lines handling
the duplicate path), ensuring the cursor-advancement logic only runs for txids
actually processed successfully.
🪄 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: 02d6b409-2701-4ff7-9b51-4352dfa13177
📒 Files selected for processing (1)
src/watch_tower/nostr_discovery.rs
keshav0479
force-pushed
the
feat/nostr-cursor-ordering-fix
branch
from
0f3d2b4 to
05cfc53
Compare
|
Updated this after rechecking retry path, raw tx fetch now happens before touching the seen cache or cursor, so a fetch failure leaves both untouched. Kept it to the small one file change, no extra helper, and changed the logs in this routine to info/warn. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #835 +/- ##
==========================================
+ Coverage 68.87% 78.80% +9.93%
==========================================
Files 35 57 +22
Lines 4932 15628 +10696
==========================================
+ Hits 3397 12316 +8919
- Misses 1535 3312 +1777 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Description
Small correctness fix for the watchtower's Nostr discovery path. Not responding to a reported incident, found while re-reading code around #829.
Before this PR,
handle_relay_messageinsrc/watch_tower/nostr_discovery.rsdid things in this order:registry.save_nostr_cursor(relay_url, event.created_at)persists cursor to diskseen_txid.insert(txid)marks txid as processedbitcoin_rpc.get_raw_tx(&txid)fetches the transaction and can fail transientlyIssue: if step 3 fails, progress was already committed. The cursor moved past the event and the txid was in the seen set. On reconnect or replay, that fidelity announcement could be skipped instead of retried.
When can step 3 realistically fail:
Makers do re-broadcast fidelity events before expiry, so the protocol can self-heal over time. But relying on that leaves a blind spot where the watcher has stale bond state. The watchtower's job is reliable fidelity-bond tracking, so it should not commit progress for an event it did not actually fetch.
What the fix does
This keeps the existing routine small, but moves the state commits later:
info/warninstead ofdebugPost-fix behavior
get_raw_txtransient failprocess_fidelityreturnsNoneKnown tradeoff
Already-seen txids now fetch the raw tx before hitting the seen-cache skip. That can mean an extra RPC call for duplicate relay delivery, but it keeps the patch small and avoids adding a new helper or rollback logic in code that is already queued for refactor.
Related Issue(s)
None. Found while auditing code around #829.
Type of Change
Protocol Version(s) Affected
messages.rs,contract.rs,handlers.rs)messages2.rs,contract2.rs,handlers2.rs)Affected Component(s)
docs/)Testing
cargo checkcargo test --lib watch_tower::cargo +stable clippy --all-features --lib --bins --tests -- -D warnings