Skip to content

Commit 8680419

Browse files
grunchclaude
andcommitted
fix(pow): surface explicit PoW rejection instead of generic timeout
When mostrod requires NIP-13 PoW and the client sends an event without satisfying it, the daemon silently drops the event (logs "Not POW verified event!" on its side) and never replies. The CLI was hiding that behind a generic timeout message ("deadline has elapsed" / "Timeout waiting for DM or gift wrap event"), which can mislead users into suspecting network failures, relay issues, or daemon downtime. After timing out, wait_for_dm now consults the daemon's kind-38385 info event ("pow" tag) and, if the required difficulty exceeds the configured POW, returns the new PowRequirementUnmet error with an actionable message pointing at --pow / POW=. Genuine timeouts keep their existing WaitForDmTimeout error, so the add-bond-invoice happy-path branch (which only matches WaitForDmTimeout) is unaffected. Closes #172 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 5623f71 commit 8680419

4 files changed

Lines changed: 433 additions & 20 deletions

File tree

docs/pow_error_handling.md

Lines changed: 246 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,246 @@
1+
# Spec — Surface explicit PoW rejection error
2+
3+
Tracking issue: [MostroP2P/mostro-cli#172](https://github.com/MostroP2P/mostro-cli/issues/172)
4+
5+
## 1. Problem
6+
7+
When a user sends an event from `mostro-cli` to a `mostrod` instance that
8+
**requires NIP‑13 Proof‑of‑Work** without providing the required PoW, the
9+
daemon silently drops the event (logs `Not POW verified event!` on its side)
10+
and never replies. The CLI eventually times out and surfaces a generic
11+
timeout error to the user:
12+
13+
```text
14+
deadline has elapsed
15+
```
16+
17+
(or, in current `main`, the slightly less awful but still ambiguous
18+
`Timeout waiting for DM or gift wrap event`).
19+
20+
This is misleading. From the user perspective the message can mean almost
21+
anything: the relay is slow, mostrod is down, the network is broken, the CLI
22+
is buggy, or the command was wrong. The real cause — PoW not satisfied — is
23+
hidden.
24+
25+
## 2. Root cause
26+
27+
The flow that breaks today:
28+
29+
1. `mostro-cli` mines PoW on the outer GiftWrap based on the `POW` env var
30+
(default `0`) — see `src/util/messaging.rs::parse_pow_env` and the
31+
`WrapOptions { pow, .. }` plumbing.
32+
2. The wrapped event is published to relays via `client.send_event(...)`.
33+
3. `wait_for_dm` opens a subscription on the trade key and waits for
34+
`FETCH_EVENTS_TIMEOUT` (15 s) for an inbound GiftWrap.
35+
4. If `mostrod` requires PoW above what the client provided, mostrod
36+
silently drops the event in
37+
[`mostro/src/app.rs`](https://github.com/MostroP2P/mostro) — the relay
38+
accepted it (so there's no NIP‑01 `OK false` from the relay either) and
39+
no reply is ever produced.
40+
5. `tokio::time::timeout` elapses → `wait_for_dm` returns `WaitForDmTimeout`
41+
→ the user sees the generic timeout message.
42+
43+
The CLI has **no way to distinguish** "daemon silently dropped my event for
44+
PoW reasons" from "transient relay or network issue".
45+
46+
## 3. Signal we can use
47+
48+
`mostrod` already publishes its required PoW in its **kind‑38385 info event**
49+
under the tag `["pow", "<difficulty>"]`. See
50+
[`mostro/src/nip33.rs`](https://github.com/MostroP2P/mostro) (`new_info_event`)
51+
where the daemon writes:
52+
53+
```rust
54+
Tag::custom(
55+
TagKind::Custom(Cow::Borrowed("pow")),
56+
vec![mostro_settings.pow.to_string()],
57+
),
58+
```
59+
60+
Reading that tag on the CLI side lets us answer the only question we care
61+
about: *did this Mostro instance reject our event because we didn't provide
62+
enough PoW?*
63+
64+
We already fetch kind‑38385 elsewhere in the CLI — see
65+
`src/util/events.rs::fetch_bond_claim_window_days` — so this is a small
66+
extension of an existing pattern.
67+
68+
## 4. Design
69+
70+
### 4.1 Error variant
71+
72+
Add a typed error so callers (and tests) can match the PoW failure mode
73+
distinctly from a generic timeout:
74+
75+
```rust
76+
#[derive(Debug)]
77+
pub struct PowRequirementUnmet {
78+
pub required: u8,
79+
pub configured: u8,
80+
}
81+
```
82+
83+
The user-facing `Display` will be explicit:
84+
85+
```text
86+
This Mostro instance requires NIP-13 proof of work of N bits, but the
87+
client sent the event with M bits. Re-run with `--pow N` or set
88+
`POW=N` and try again.
89+
```
90+
91+
The error lives next to `WaitForDmTimeout` in `src/util/messaging.rs` (same
92+
module, same idiom: small zero/struct error wrapped via `anyhow`).
93+
94+
### 4.2 Helper — fetch required PoW from kind‑38385
95+
96+
Add a helper in `src/util/events.rs`:
97+
98+
```rust
99+
/// Best-effort: fetch the Mostro instance's kind-38385 info event and read
100+
/// the `pow` tag. Returns `None` when no info event is available or the tag
101+
/// is missing/unparseable. Mirrors `fetch_bond_claim_window_days`.
102+
pub async fn fetch_required_pow(ctx: &crate::cli::Context) -> Option<u8>;
103+
```
104+
105+
Implementation details:
106+
107+
- Author filter on `ctx.mostro_pubkey`, kind `NOSTR_INFO_EVENT_KIND`.
108+
- Pick the **newest** revision by `created_at` (replaceable but lagging
109+
relays can still serve old copies; same caveat as the bond-window helper).
110+
- Scan tags for `["pow", "<value>"]`, parse as `u8`.
111+
- Any error (fetch failure, missing tag, unparseable value) → `None`.
112+
113+
Returning `None` is the "I don't know" signal; the caller treats it as
114+
*"can't blame PoW"* and falls back to the generic timeout error. This keeps
115+
the helper non-fatal: older daemons that don't publish the tag, or unreachable
116+
relays, never break flows that worked before.
117+
118+
### 4.3 Where to plug it in — `wait_for_dm`
119+
120+
`wait_for_dm` is the single chokepoint that every request/reply flow goes
121+
through (`add_invoice`, `take_order`, `take_dispute`, `send_msg`, `new_order`,
122+
`rate_user`, `orders_info`, `restore`, `last_trade_index`, `add_bond_invoice`).
123+
Centralizing the fix here covers every command in one place.
124+
125+
Postflight check (chosen — see Alternatives below):
126+
127+
```rust
128+
let event = match waited {
129+
Ok(inner) => inner?,
130+
Err(_elapsed) => {
131+
// Before declaring this a generic timeout, check whether the daemon
132+
// advertises a PoW requirement we didn't meet — that's the real
133+
// cause "deadline has elapsed" was hiding.
134+
if let Some(required) = fetch_required_pow(ctx).await {
135+
let configured = parse_pow_env().unwrap_or(0);
136+
if required > configured {
137+
return Err(PowRequirementUnmet { required, configured }.into());
138+
}
139+
}
140+
return Err(WaitForDmTimeout.into());
141+
}
142+
};
143+
```
144+
145+
Add an `&Context` parameter? Look at the signature today —
146+
`wait_for_dm(ctx, order_trade_keys, sent_message)``ctx` is already
147+
passed. We just need to grant the helper access to `ctx.client` and
148+
`ctx.mostro_pubkey` (already does).
149+
150+
### 4.4 `add_bond_invoice` interplay
151+
152+
`add_bond_invoice` treats `WaitForDmTimeout` as the happy path (Mostro pays
153+
the invoice without acking over Nostr). The new variant must **not** be
154+
caught by that branch — a PoW failure on `add-bond-invoice` is *not* a
155+
successful submission. Concretely:
156+
157+
- The existing `Err(e) if e.downcast_ref::<WaitForDmTimeout>().is_some()`
158+
arm continues to match only `WaitForDmTimeout`.
159+
- A `PowRequirementUnmet` falls through to the generic `Err(e) => return
160+
Err(e)` arm and bubbles up to the user, which is the desired behavior.
161+
162+
No code change needed in `add_bond_invoice.rs` — the existing pattern match
163+
already handles the right split. We assert this with a regression note.
164+
165+
## 5. Alternatives considered
166+
167+
### 5.1 Preflight check (fail before sending)
168+
169+
Run `fetch_required_pow` *before* `send_dm`. Advantages: fail fast (no
170+
15‑second wait). Disadvantages:
171+
172+
- Adds a relay roundtrip to the happy path for every command.
173+
- Doubles "you didn't set PoW" errors for daemons that publish the tag but
174+
also accept PoW=0 (no real check on that side today).
175+
- Couples every command to the info event being reachable.
176+
177+
→ Rejected. Postflight has the right cost model: zero overhead when things
178+
work, and *only* pays for the info-event fetch in the failure path where the
179+
user is already waiting anyway.
180+
181+
### 5.2 Auto-mine the required PoW
182+
183+
Bump `WrapOptions.pow` to `max(configured, required)` instead of erroring.
184+
Tempting, but high PoW targets (e.g. 28+ bits) can take minutes to mine on
185+
a laptop, and the user wouldn't see *why* the CLI is suddenly chewing CPU.
186+
Better to fail explicitly with an actionable hint and let the user opt in
187+
by setting `POW` themselves.
188+
189+
→ Rejected. Defer to explicit user opt-in via `--pow` / `POW` env.
190+
191+
### 5.3 Read NIP‑01 `OK false` from the relay
192+
193+
Won't help: mostrod drops the event *after* the relay accepts it, so the
194+
relay returns `OK true`. There is no rejection signal on the publish path.
195+
196+
→ Rejected.
197+
198+
## 6. Acceptance criteria
199+
200+
(Reproducing the issue's checklist, with concrete bindings.)
201+
202+
- Sending an event from `mostro-cli` to a PoW‑enabled `mostrod` without
203+
satisfying PoW must surface an error that mentions PoW. Specifically the
204+
`PowRequirementUnmet { required, configured }` variant rendered as:
205+
`"This Mostro instance requires NIP-13 proof of work of N bits, but the
206+
client sent the event with M bits. Re-run with --pow N or set POW=N and
207+
try again."`
208+
- Genuine timeouts (daemon reachable, no PoW requirement, just slow / no
209+
reply) keep returning `WaitForDmTimeout` — its existing message stays.
210+
- `add_bond_invoice`'s "timeout is the happy path" arm only matches
211+
`WaitForDmTimeout`; a `PowRequirementUnmet` propagates and is shown to
212+
the user.
213+
- Older daemons that don't publish `["pow", ...]` in kind‑38385 behave
214+
exactly as before (helper returns `None` → generic timeout).
215+
216+
## 7. Test plan
217+
218+
Unit tests in `src/util/messaging.rs` and `src/util/events.rs`:
219+
220+
- `pow_requirement_unmet_display_mentions_required_and_configured`
221+
formatting check on the error message.
222+
- `fetch_required_pow_returns_none_when_tag_missing` — synthesize a
223+
kind-38385 event with no `pow` tag, assert `None`.
224+
- `fetch_required_pow_picks_newest_revision` — two events, newer with a
225+
different pow value; helper returns the newer value. Mirrors the existing
226+
`fetch_bond_claim_window_days` pattern.
227+
- `fetch_required_pow_parses_pow_tag` — single event with `["pow", "12"]`,
228+
assert `Some(12)`.
229+
230+
A manual end-to-end check (out of the unit-test scope) is to run against a
231+
PoW‑enabled local `mostrod`:
232+
233+
```sh
234+
POW=0 mostro-cli neworder ...
235+
# expected: PowRequirementUnmet error, not "deadline has elapsed"
236+
POW=<required> mostro-cli neworder ...
237+
# expected: normal flow, no error
238+
```
239+
240+
## 8. Out of scope
241+
242+
- Auto-mining the missing PoW (see 5.2).
243+
- Removing or restructuring the `POW` env var / `--pow` flag.
244+
- Surfacing the PoW requirement during `list-orders` / informational
245+
flows. The fix is scoped to the request/reply path where the missing
246+
PoW silently kills the flow.

src/util/events.rs

Lines changed: 103 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -83,15 +83,26 @@ pub fn create_filter(
8383
}
8484
}
8585

86-
/// Fetch the Mostro instance's kind-38385 info event and read the
87-
/// `bond_payout_claim_window_days` tag.
86+
/// Fetch the Mostro instance's kind-38385 info event and read a tag whose
87+
/// first slot equals `tag_name`. Returns the second slot as a string.
8888
///
89-
/// Returns `None` when the node publishes no info event, the tag is absent
90-
/// (older daemon or bonds disabled), or the value can't be parsed. Used to
91-
/// render the forfeit deadline for an `add-bond-invoice` request locally, per
92-
/// the protocol's "Bond payout invoice" / "Other events" docs. Best-effort:
93-
/// any relay error degrades to `None` rather than failing the caller.
94-
pub async fn fetch_bond_claim_window_days(ctx: &crate::cli::Context) -> Option<i64> {
89+
/// Best-effort: any relay error or missing tag degrades to `None` so callers
90+
/// keep working against older daemons that don't publish the tag.
91+
/// Pure: read the second slot of the first tag whose first slot equals
92+
/// `tag_name`. Split out so the kind-38385 tag-parsing logic stays unit
93+
/// testable without spinning up a relay.
94+
fn read_info_tag_from_event(event: &nostr_sdk::Event, tag_name: &str) -> Option<String> {
95+
event.tags.iter().find_map(|tag| {
96+
let slice = tag.as_slice();
97+
if slice.first().map(String::as_str) == Some(tag_name) {
98+
slice.get(1).cloned()
99+
} else {
100+
None
101+
}
102+
})
103+
}
104+
105+
async fn fetch_info_tag(ctx: &crate::cli::Context, tag_name: &str) -> Option<String> {
95106
let filter = Filter::new()
96107
.author(ctx.mostro_pubkey)
97108
.kind(nostr_sdk::Kind::Custom(NOSTR_INFO_EVENT_KIND));
@@ -104,16 +115,38 @@ pub async fn fetch_bond_claim_window_days(ctx: &crate::cli::Context) -> Option<i
104115

105116
// kind-38385 is replaceable, but pick the newest revision by `created_at`
106117
// explicitly: a lagging relay (or several relays at once) can still surface
107-
// an older copy, and a stale claim window would render the wrong, very
108-
// user-facing forfeit deadline.
118+
// an older copy.
109119
let event = events.iter().max_by_key(|e| e.created_at)?;
110-
for tag in event.tags.iter() {
111-
let slice = tag.as_slice();
112-
if slice.first().map(String::as_str) == Some("bond_payout_claim_window_days") {
113-
return slice.get(1).and_then(|v| v.parse::<i64>().ok());
114-
}
115-
}
116-
None
120+
read_info_tag_from_event(event, tag_name)
121+
}
122+
123+
/// Fetch the Mostro instance's kind-38385 info event and read the
124+
/// `bond_payout_claim_window_days` tag.
125+
///
126+
/// Returns `None` when the node publishes no info event, the tag is absent
127+
/// (older daemon or bonds disabled), or the value can't be parsed. Used to
128+
/// render the forfeit deadline for an `add-bond-invoice` request locally, per
129+
/// the protocol's "Bond payout invoice" / "Other events" docs. Best-effort:
130+
/// any relay error degrades to `None` rather than failing the caller.
131+
pub async fn fetch_bond_claim_window_days(ctx: &crate::cli::Context) -> Option<i64> {
132+
// A stale claim window would render the wrong, very user-facing forfeit
133+
// deadline — same newest-revision caveat as the rest of the info event.
134+
fetch_info_tag(ctx, "bond_payout_claim_window_days")
135+
.await
136+
.and_then(|v| v.parse::<i64>().ok())
137+
}
138+
139+
/// Fetch the Mostro instance's required NIP-13 proof-of-work difficulty from
140+
/// the kind-38385 info event (`["pow", "<bits>"]` tag).
141+
///
142+
/// Returns `None` when the daemon doesn't publish the tag (older versions),
143+
/// when the value is unparseable, or when the info event can't be fetched.
144+
/// Used by [`crate::util::messaging::wait_for_dm`] to distinguish a real
145+
/// timeout from a silent PoW rejection — see `docs/pow_error_handling.md`.
146+
pub async fn fetch_required_pow(ctx: &crate::cli::Context) -> Option<u8> {
147+
fetch_info_tag(ctx, "pow")
148+
.await
149+
.and_then(|v| v.parse::<u8>().ok())
117150
}
118151

119152
#[allow(clippy::too_many_arguments)]
@@ -206,3 +239,56 @@ pub async fn fetch_events_list(
206239
}
207240
}
208241
}
242+
243+
#[cfg(test)]
244+
mod tests {
245+
use super::*;
246+
247+
async fn make_info_event(keys: &Keys, tags: Vec<Tag>) -> nostr_sdk::Event {
248+
EventBuilder::new(nostr_sdk::Kind::Custom(NOSTR_INFO_EVENT_KIND), "")
249+
.tags(tags)
250+
.sign(keys)
251+
.await
252+
.expect("sign info event")
253+
}
254+
255+
fn pow_tag(value: &str) -> Tag {
256+
Tag::parse(["pow", value]).expect("build pow tag")
257+
}
258+
259+
#[tokio::test]
260+
async fn read_info_tag_finds_pow_value() {
261+
let keys = Keys::generate();
262+
let event = make_info_event(
263+
&keys,
264+
vec![
265+
Tag::parse(["fee", "0.006"]).unwrap(),
266+
pow_tag("12"),
267+
Tag::parse(["fiat_currencies_accepted", "USD,EUR"]).unwrap(),
268+
],
269+
)
270+
.await;
271+
assert_eq!(read_info_tag_from_event(&event, "pow"), Some("12".into()));
272+
}
273+
274+
#[tokio::test]
275+
async fn read_info_tag_returns_none_when_missing() {
276+
let keys = Keys::generate();
277+
let event = make_info_event(&keys, vec![Tag::parse(["fee", "0.006"]).unwrap()]).await;
278+
assert_eq!(read_info_tag_from_event(&event, "pow"), None);
279+
}
280+
281+
#[tokio::test]
282+
async fn pow_tag_parses_as_u8() {
283+
// u8 parse is what fetch_required_pow chains after the helper.
284+
// Lock in that the daemon's stringified u8 round-trips cleanly,
285+
// and that garbage values degrade to None.
286+
let parse = |s: &str| s.parse::<u8>().ok();
287+
assert_eq!(parse("12"), Some(12));
288+
assert_eq!(parse("0"), Some(0));
289+
assert_eq!(parse("nope"), None);
290+
// Out of range for u8 → None, which is the right "ignore this
291+
// weird value, fall back to generic timeout" behavior.
292+
assert_eq!(parse("999"), None);
293+
}
294+
}

0 commit comments

Comments
 (0)