fix(pow): filter wait_for_dm notifications so PoW detection actually fires#174
Conversation
…fires The concurrent PoW probe added in #173 spawned a fetch_required_pow_with task whose kind-38385 events flow through the same global notification broadcast wait_for_dm consumes (client.notifications() is global — nostr-relay-pool::relay::inner::send_notification fans every event from every subscription out to it). The notification loop returned the info event as the "first event" and short-circuited the wait before mostrod had a chance to (silently) drop the request, so the wait never timed out, the PoW probe result was never consulted, and downstream print_dm_events surfaced "No response received from Mostro" — exactly the misleading UX the original fix was meant to eliminate. Mirror the subscription filter inside the notification loop: only accept Kind::GiftWrap events whose `p` tags include our trade key. Everything else (kind-38385 info, unrelated gift wraps, status events) is ignored, so the wait reaches its timeout cleanly and the existing PowRequirementUnmet path fires as designed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
WalkthroughThis PR fixes a notification-leak bug in ChangesNotification Filtering Fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 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/util/messaging.rs`:
- Around line 324-327: The subscription uses Filter::new().pubkey(trade_pubkey)
(variable subscription with .kind(nostr_sdk::Kind::GiftWrap)) but GiftWrap
NIP-59 recipients are in the event's "p" tags, and your notification logic
checks event.tags.public_keys(); replace the author-based .pubkey(...) filter
with a tag-based recipient filter for the "p" tag (e.g., use the Filter tag API
to filter "`#p`"/"p" for trade_pubkey) so the subscription matches the same
recipient pubkey the later event.tags.public_keys() check expects.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6c56e85a-1c29-47ca-bd0d-6dc03956c77a
📒 Files selected for processing (2)
docs/pow_error_handling.mdsrc/util/messaging.rs
| let subscription = Filter::new() | ||
| .pubkey(trade_keys.public_key()) | ||
| .pubkey(trade_pubkey) | ||
| .kind(nostr_sdk::Kind::GiftWrap) | ||
| .limit(0); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that Filter.pubkey() filters GiftWrap by p-tag recipient, not author
# Check if nostr-sdk Filter.pubkey() has special GiftWrap handling
rg -n -C5 "impl.*Filter" --type rust -g "!target/**" | rg -A10 -B5 "pubkey|GiftWrap"
# Search for Filter API usage patterns with GiftWrap in the codebase
rg -n "Filter.*pubkey.*GiftWrap|GiftWrap.*pubkey" --type rust -g "!target/**"
# Look for any comments or docs about GiftWrap subscription filtering
rg -n -C3 "GiftWrap.*subscribe|subscribe.*GiftWrap|p.*tag.*GiftWrap" --type rust -g "!target/**"Repository: MostroP2P/mostro-cli
Length of output: 168
🌐 Web query:
Does nostr-sdk 0.44.1 Filter.pubkey() method filter GiftWrap events by the p-tag recipient or by the event author pubkey field?
💡 Result:
nostr-sdk 0.44.1’s Filter.pubkey filters Gift Wrap (kind 1059) events by the event’s author pubkey (the event field pubkey / “authors” filter), not by the recipient in the Gift Wrap p tag. Evidence: - In Nostr filters, the pubkey/authors attribute selects events by the author pubkey (i.e., the event pubkey field), while tag filters are separate fields such as #p for filtering by values in the event’s p tags [1]. - The Gift Wrap spec describes recipient identity as the recipient’s pubkey stored in the event’s p tag [2]. So, if you want Gift Wrap events for a specific recipient, you should use the filter’s p/#p capability (often exposed as something like p(...)/p_tag(...) / add_p_tag_public_key), not Filter.pubkey. I wasn’t able to retrieve the exact rust source for nostr-sdk v0.44.1’s Filter.pubkey method body in the provided results, but the mapping between “author pubkey filters” vs “tag-based filters” is dictated by the Nostr filter semantics [1] and Gift Wrap’s p tag recipient convention [2].
Citations:
Fix GiftWrap subscription filtering: .pubkey(trade_pubkey) matches author pubkey, not recipient p tags
In nostr filters, pubkey/authors filters by the event’s pubkey field, while GiftWrap (NIP-59) uses the recipient’s pubkey in the p tag (#p). Your subscription uses .pubkey(trade_pubkey) (src/util/messaging.rs ~324-327), but the notification matching checks trade_pubkey against event.tags.public_keys() (~356-364), so the two filters aren’t mirrored. Use the filter’s p/#p (tag-based) recipient filter for trade_pubkey in the subscription instead of .pubkey(...).
🤖 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/util/messaging.rs` around lines 324 - 327, The subscription uses
Filter::new().pubkey(trade_pubkey) (variable subscription with
.kind(nostr_sdk::Kind::GiftWrap)) but GiftWrap NIP-59 recipients are in the
event's "p" tags, and your notification logic checks event.tags.public_keys();
replace the author-based .pubkey(...) filter with a tag-based recipient filter
for the "p" tag (e.g., use the Filter tag API to filter "`#p`"/"p" for
trade_pubkey) so the subscription matches the same recipient pubkey the later
event.tags.public_keys() check expects.
There was a problem hiding this comment.
Verified against the actual nostr-sdk 0.44.1 source — finding is incorrect, no change needed.
Filter::pubkey() in nostr-0.44.2 (the matching version pulled in by nostr-sdk 0.44.1) is not an author/authors filter. From nostr-0.44.2/src/filter.rs:585-589:
/// Add pubkey
#[inline]
pub fn pubkey(self, pubkey: PublicKey) -> Self {
self.custom_tag(SingleLetterTag::lowercase(Alphabet::P), pubkey)
}It delegates to custom_tag with Alphabet::P — i.e., it is exactly the tag-based #p filter the comment recommends switching to. The author filter is a separate method, Filter::author() (same file, line 511).
So the subscription Filter::new().pubkey(trade_pubkey).kind(GiftWrap) and the application-side check event.tags.public_keys().any(|pk| *pk == trade_pubkey) are already mirrored — both look at the p tag.
Cross-check: the same idiom is already in use elsewhere in the repo for the same purpose — src/util/messaging.rs:171 (fetch_gift_wraps_for_shared_key) and src/util/events.rs:63,80 (create_filter for direct-message list kinds). If .pubkey() were author-based, none of those paths would have worked.
The web-query result in the CodeRabbit analysis was speculative (the model itself flagged "I wasn't able to retrieve the exact rust source for nostr-sdk v0.44.1's Filter.pubkey method body") — the actual source from the registry contradicts it.
There was a problem hiding this comment.
@grunch, you're absolutely right — thank you for digging into the actual source. My comment was based on a web search result that the model itself flagged as speculative ("I wasn't able to retrieve the exact rust source"), and it turns out to be wrong. Filter::pubkey() in nostr-0.44.2 is indeed a #p tag filter (custom_tag with Alphabet::P), not an author/authors filter — those are separate methods. The subscription and the notification-loop check are already correctly mirrored on the recipient p tag. I apologize for the noise.
(ᴗ_ ᴗ。)
✏️ Learnings added
Learnt from: grunch
Repo: MostroP2P/mostro-cli PR: 174
File: src/util/messaging.rs:324-327
Timestamp: 2026-05-28T17:26:11.052Z
Learning: In nostr-sdk (nostr-0.44.2, used by nostr-sdk 0.44.1), `Filter::pubkey()` is NOT an author filter — it is a `#p` tag filter. It delegates to `self.custom_tag(SingleLetterTag::lowercase(Alphabet::P), pubkey)`. The author filter is a separate method: `Filter::author()`. So `Filter::new().pubkey(trade_pubkey).kind(Kind::GiftWrap)` correctly filters GiftWrap events by the recipient `p` tag, which mirrors `event.tags.public_keys().any(|pk| *pk == trade_pubkey)` on the application side. Do not flag this pattern as incorrect in future reviews of MostroP2P/mostro-cli or any other Rust project using nostr-sdk.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
1 participant
Summary
Follow-up to #173 — fixes a regression I introduced with the concurrent PoW probe.
When the user runs e.g.
mostro-cli neworder ...against a PoW-enabledmostrodwithout settingPOW, they seeNo response received from Mostroinstead of thePowRequirementUnmeterror #173 was supposed to surface. Root cause:client.notifications()is a global broadcast —nostr-relay-pool::relay::inner::send_notificationfans every event from every active subscription/fetch onto it.tokio::spawn(fetch_required_pow_with(...))runs a kind-38385 fetch concurrently with the wait. Those info events flow through the same broadcast thatwait_for_dm's loop is consuming.notifications.recv().awaitreturns the kind-38385 info event as the "first event" — long beforemostrodhas had a chance to (silently) drop our request. The wait never times out, the PoW probe result is never consulted, andprint_dm_eventslater errors withNo response received from Mostro.Fix: mirror the subscription filter inside the notification loop. Only accept
Kind::GiftWrapevents whoseptags include our trade key. Everything else (kind-38385 info, unrelated gift wraps, status events) is dropped on the floor, so the wait reaches itsFETCH_EVENTS_TIMEOUTcleanly and the existingPowRequirementUnmetescalation in the timeout branch fires as designed.Reported by @grunch after merging #173.
Test plan
cargo fmt --all -- --checkcargo clippy --all-targets --all-features -- -D warningscargo test(10/10 unit + integration tests green)mostrod:POW=0 mostro-cli neworder …→PowRequirementUnmet(notNo response received from Mostro)POW=<required> mostro-cli neworder …→ normal flow🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation