Skip to content

fix(pow): filter wait_for_dm notifications so PoW detection actually fires #174

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
grunch merged 1 commit into from
May 28, 2026
Merged

fix(pow): filter wait_for_dm notifications so PoW detection actually fires #174

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions docs/pow_error_handling.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,26 @@ Add an `&Context` parameter? Look at the signature today —
passed. We just need to grant the helper access to `ctx.client` and
`ctx.mostro_pubkey` (already does).

#### 4.3.1 Notification leak from the concurrent probe

`client.notifications()` is a **global** broadcast: every event seen by
any active subscription or fetch lands on the same channel
(`nostr-relay-pool::relay::inner::send_notification`). The spawned PoW
probe issues its own subscription for kind‑38385 events to read the
required difficulty — those info events show up on the same broadcast
the wait loop is consuming. Without an application‑side filter, the
loop returns the info event as the "first event" and short‑circuits the
wait before mostrod has even had a chance to (silently) drop the
request, surfacing further downstream as `"No response received from
Mostro"`.

Fix: the notification loop mirrors the subscription filter explicitly —
only `Kind::GiftWrap` events whose `p` tags contain `trade_keys.public_key()`
escape the loop. Anything else (kind‑38385 info, replaceable
status events, gift wraps for other trade keys) is dropped on the floor
so the wait properly reaches its timeout, where the PoW probe result
escalates to `PowRequirementUnmet`.

### 4.4 `add_bond_invoice` interplay

`add_bond_invoice` treats `WaitForDmTimeout` as the happy path (Mostro pays
Expand Down
23 changes: 20 additions & 3 deletions src/util/messaging.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,11 +317,12 @@ where
F: std::future::Future<Output = Result<()>> + Send,
{
let trade_keys = order_trade_keys.unwrap_or(&ctx.trade_keys);
let trade_pubkey = trade_keys.public_key();
let mut notifications = ctx.client.notifications();
let opts =
SubscribeAutoCloseOptions::default().exit_policy(ReqExitPolicy::WaitForEventsAfterEOSE(1));
let subscription = Filter::new()
.pubkey(trade_keys.public_key())
.pubkey(trade_pubkey)
.kind(nostr_sdk::Kind::GiftWrap)
.limit(0);
Comment on lines 324 to 327
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 28, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 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.

Copy link
Copy Markdown
Member Author

@grunch grunch May 28, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 28, 2026

Choose a reason for hiding this comment

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

@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.

ctx.client.subscribe(subscription, Some(opts)).await?;
Expand All @@ -340,11 +341,27 @@ where
ctx.mostro_pubkey,
));

// Wait for the DM or gift wrap event
// Wait for the DM or gift wrap event.
//
// `client.notifications()` is the **global** broadcast for every event
// any active subscription / fetch sees — including the kind-38385 info
// event coming back from the spawned PoW probe above. Without an
// application-side filter, that info event would race ahead of the real
// reply and short-circuit the wait, surfacing as "No response received
// from Mostro" further downstream. So mirror the subscription filter
// here and only accept GiftWraps tagged to our trade key.
let waited = tokio::time::timeout(super::events::FETCH_EVENTS_TIMEOUT, async move {
loop {
match notifications.recv().await {
Ok(RelayPoolNotification::Event { event, .. }) => return Ok(*event),
Ok(RelayPoolNotification::Event { event, .. }) => {
if event.kind != nostr_sdk::Kind::GiftWrap {
continue;
}
if !event.tags.public_keys().any(|pk| *pk == trade_pubkey) {
continue;
}
return Ok(*event);
}
Ok(_) => continue,
Err(e) => {
return Err(anyhow::anyhow!("Error receiving notification: {:?}", e));
Expand Down
Loading