alerting: webhook watch-sets (scripts, outpoints, txids, silent payments) - #483
alerting: webhook watch-sets (scripts, outpoints, txids, silent payments)#483bkeroack wants to merge 6 commits into
Conversation
eb6b6d2 to
a2d97b0
Compare
cecaf09 to
47d0811
Compare
|
Round-1 review fix (High — data loss under receiver-side dedupe). Confirmed and fixed. Watch-match deliveries minted Watch matches now draw from their own process-wide counter and render through Worth flagging about the test: my first version asserted uniqueness over a single watched payment and passed against the buggy code, because each of its two matches happened to land after a different bus publish. The test now pays the watched script twice in one transaction, which puts two matches between one pair of bus publishes. Against the old scheme it fails with the duplicate pair visible in the output ( Also corrected the watch-durability wording here and in the manual, spec, and release notes — see the updated PR body. It claimed a match occurring while the daemon was down is not re-delivered; the watch-set is in fact re-registered before P2P starts, so a restart is not a gap. |
|
Documented, per the follow-up question about A watch-set is not filtered by The trap this prevents is adding The manual also gained the table of what each category delivers and at what rate — it never had one — plus a note that The e2e deposit-watch test already configured |
d74fd4c to
e8994da
Compare
|
Round-2 blocker — confirmed and fixed. My error, and worth stating exactly how it happened. When I added the Fixed by amending the doc commit to drop them ( PR file counts: #483 16, #484 8, #485 8, #486 15. I also deleted the build directory from the worktree so it cannot be swept up again, and will point Separately: #483's previous CI "Tests" job reported failure with no failed step and no retrievable log, which reads like the runner dying rather than a real failure — plausibly on the 2119-file checkout. The re-run on the purged tip will settle that; I'll report it. |
a2d97b0 to
fb24c8c
Compare
e8994da to
a54e20b
Compare
fb24c8c to
8c4e12f
Compare
d416d3c to
bd0c28b
Compare
d29f5f9 to
aa6573d
Compare
bd0c28b to
68d95af
Compare
aa6573d to
6addf3e
Compare
949965c to
f00cf2a
Compare
…nts) PR 4 of the A3 alerting stack (SATD_ALERTING_DESIGN.md §4.5). A hook can now carry a `[webhook.watch]` table and receive the same match events the streaming `Watch` stream delivers. The shared-rendering point is the substance here: `watch_match_json` moves out of the WebSocket carrier into `node::events`, so the WS firehose and the webhook dispatcher emit *the same bytes* for the same match. A receiver written against the streaming spec parses webhook matches unchanged, and the two surfaces cannot drift apart. Design points: - One union registry subscriber for all hooks, not one per hook. The matcher's cost is per watched entry per transaction, so registering the same script twice would double it. Routing a match back to the hooks that actually asked for it is a cheap membership test on the delivery side — and getting it wrong would leak one operator's deposit activity into an unrelated endpoint, so it is unit-tested in both directions. - The alertfile is the durable source of truth for the set. Watch state is per-subscriber and lives only in memory, so each dispatcher generation re-registers from the file — the same "rebuild from durable truth on reconnect" shape `ResilientWatch` uses, with the config file as the loader. A reload therefore re-registers automatically. - Silent-payment scan keys in an alertfile are a deliberate exception to "satd never persists a scan key". Over the streaming API the client and the operator are different parties, so a key is held for one connection and never written. A webhook consumer IS the operator, alerting on their own wallet on their own node: the key is watch-only (payment detection, no spend authority) and the file already holds signing secrets at 0600. Keys are zeroized in memory, `WatchSet`'s Debug renders counts only, and a malformed-key error deliberately does not echo the value — config errors end up in bug reports. Entries require silentpaymentindex=1. - Validation stays recognize-and-reject: a short scripthash, an outpoint without a vout, a bad txid, or an unknown key under `[webhook.watch]` is an error rather than a watch that silently matches nothing. Left out, deliberately and stated in the docs: historical catch-up for watch *matches* after a restart. A hook's confirmed chain events are replayed from its cursor (PR 3), but a match that occurred while the daemon was down is not re-delivered — that needs the bounded-rescan driver currently embedded in the gRPC carrier to become shared machinery, which is a refactor of a hot, carefully-tuned path and belongs in its own change. Live matching is unaffected.
Watch-match deliveries minted X-Satd-Delivery from publisher.published() — a *read* of the bus sequence counter, not an allocation. Bus events take their id from stamp.seq, which fetch_adds. Matches never advanced it, so every match emitted between two bus publishes shared one id, and that id also collided with the bus event that last moved the counter. The contract makes this header the receiver's idempotency key, and the reference push relay (and any receiver following the spec) dedupes on it. Two watched outputs in one transaction produced two alerts carrying the same id: a correct receiver ACKs the second as a duplicate and drops it. Silent data loss in the feature this branch exists to ship. Watch matches now draw from their own process-wide counter and render through watch_delivery_id(), which `w`-prefixes the sequence so the two id spaces cannot overlap at any value. Process-wide rather than per-generation because a SIGHUP reload keeps the same instance_id — a per-generation counter would restart at 1 and re-mint ids the receiver has already seen. One id per match, shared by every hook that wants it, matching how the bus path already works. The e2e test now pays the watched script twice in a single transaction, which is what puts two matches between one pair of bus publishes. It fails against the old scheme with the duplicate pair visible in the assertion output; an earlier single-output version of the same test did not, because each match happened to land after a different bus publish. Also corrects the watch-set durability wording in the manual, release notes, and changelog. It said a match occurring while the daemon was down is not re-delivered; the watch-set is in fact re-registered before P2P starts, so a restart is not a gap and -reindex does not re-fire history. Forward-only from registration is the intended semantic. The one real hole is a crash between a block connecting and the receiver acknowledging. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018AfAm3VVN8gBeYcTL5BJqH
…nts) Round-3 review fixes folded in: - **A silent-payment scan key may now belong to only one hook.** The watch registry keys SP targets by `scan_pubkey` alone, so a second hook claiming the same key *replaced* the first — the replacement's labels and spend pubkey won — while routing (also scan-pubkey-only) delivered the survivor's matches to both hooks. Two silent failures: the losing hook stopped receiving the labels it asked for (change outputs, say) with no error and no log, and with differing spend pubkeys one hook's endpoint received payment details — txid, vout, amount, tweak — for a wallet it does not own. That is the one match shape where the per-hook re-filter was weaker than what was registered. The configuration is ambiguous, so it is refused at parse. - **Silent-payment targets are capped per hook**, matching the cap the streaming surface already enforces per connection. Scripts, outpoints, and txids are inverted-index lookups; SP matching has no index and costs one ECDH per target per eligible transaction, so an unbounded alertfile list put thousands of EC operations on the matcher for every taproot-bearing transaction in a block. - `SpWatchTarget` gained an identity `PartialEq` over its public fields only. `scan_pubkey` is `b_scan·G`, so comparing it covers the secret without comparing secret bytes; spend pubkey and labels are part of the identity too, since two targets sharing a scan key but differing in either derive different outputs. - The `watch_rx` closed arm warned and fell through, where the sibling bus arm breaks. A closed mpsc returns `None` immediately and forever, so that would spin at 100% CPU with a log flood. Unreachable today; the asymmetry was a trap. Known and deliberately not changed here, documented in the spec (#484): the reload hand-off between dispatcher generations is not atomic with respect to the watch registry, so a match landing in the millisecond-scale window can be missed or delivered twice. Fixing it properly means registering synchronously in `apply()` and threading the handle into `fan_in`; that is a larger change than this review round should carry. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018AfAm3VVN8gBeYcTL5BJqH
Round 4 of external review, scoped to the ~185 lines this branch added during round 3. The IBD gate was missing from the watch arm — three reviewers found it independently. `run_watch_matcher` is spawned unconditionally and scans every connected block, so a fresh node with a watched busy address POSTed one signed delivery per historical matching transaction for the whole sync. The bus arm's own comment names this as half of what it prevents, and the manual promises watch-sets are forward-only from registration. A reload registered the new generation's watch-set before retiring the old one, so both were live registry subscribers for the overlap and each minted its own `WATCH_DELIVERY_SEQ`. Two POSTs, same body, same hook, *different* `X-Satd-Delivery` — a receiver deduplicating on that header, as the contract instructs, credits the deposit twice. The window is sub-millisecond normally but stretches to seconds while the new generation's catch-up replays, i.e. right after a restart. Retiring now precedes spawning: a momentary gap instead of an undedupable duplicate, which is the better trade because a chain-event gap is recovered by the cursor and watch matches are already documented as best-effort. `break` on a closed watch channel exited `fan_in` entirely, so status, chain, and heartbeat deliveries would all stop on the strength of the watch channel closing — one warn line, no metric moving. Unreachable today, but reachable the moment registry-side subscriber pruning lands, which the original comment anticipates. The arm is now guarded out of the `select!` instead, which fixes the 100% CPU spin it was written for without the blast radius. `MAX_SP_TARGETS_PER_HOOK` bounded one stanza while the cost is per union subscriber: matching is one ECDH per registered target per eligible transaction whether the targets came from one hook or two hundred, and nothing capped hook count. Added `MAX_SP_TARGETS_TOTAL`. The collision error named only the losing hook (a `BTreeSet` discarded the first claimant) and described a duplicate *within* one hook as "already claimed by another hook", sending the operator to look for a stanza that does not exist. The cross-hook guard lived only inside `parse`, while `AlertFile` has public fields and derives `Default`. Extracted as `AlertFile::validate`, so a future runtime "add a webhook" surface or an SDK consumer cannot rebuild the cross-tenant leak by constructing one directly. Docs: both new limits are hard startup failures and were documented nowhere — an operator with two teams' hooks on a shared corporate wallet would have hit an `exit(1)` explained by no changelog, manual chapter, or release note. Also documented that a `lagged` covering watch matches is weaker than one covering chain events: its resume cursor is a chain position, so replaying from it re-delivers blocks, not the lost matches. Each new guard verified to fail against the code it guards. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018AfAm3VVN8gBeYcTL5BJqH
The watch half of the handover. The fan-in registered the union watch-set for itself and held the handle, so deregistration happened whenever the retired task next got scheduled — which meant the incoming generation could register while the outgoing one was still live. Both then receive every match in the window, and each mints its own WATCH_DELIVERY_SEQ: two POSTs, same body, same hook, different X-Satd-Delivery. A receiver deduplicating on that header, as the contract instructs, credits the deposit twice. The previous fix for this was to retire the old generation before spawning the new one, which bought watch correctness at the cost of a window with no bus subscriber at all — the P0 the parent commit fixes. The two resources want opposite orderings, so they now get them: the bus is overlapped (its ids are derived from the event's own seq, so duplicates collapse), the watch registration is gapped (its ids come from a process counter, so duplicates do not). apply() owns both ends and does the swap itself. register_watch_sets takes stanzas rather than live channels and returns the handle to the caller. The fan-in's "watch channel closed" error is now reserved for a channel that closed on its own: deregistration is a handover, and logging it as a total alerting outage on every reload would train operators to ignore the line that means it really happened. WatchRegistry::watched_items counts entries across all subscribers, which is what makes the swap observable. The test registers different numbers of scripts across the two applies so a deferred swap reads 1 where a completed one reads 2; verified to fail against a fan-in that registers for itself.
The IBD gate is scoped to confirmed matches — an unconfirmed one comes from the mempool and cannot be historical, and the predicate is the tip's age, which reads "syncing" on a stalled node. The manual still said watch alerts were suppressed outright. The lagged-notice and unclean-shutdown paragraphs also still described a cursor that re-delivers the span. It announces it.
f00cf2a to
f0d9935
Compare
|
Closing: webhook watch-sets are cut from the design. Webhooks had grown into a second integration API. The streaming API is the canonical way to do sophisticated integration with satd, and its Design D8 now reads "rejected" rather than "SP scan keys included", and a #484 has been retargeted onto #482, so the rest of the stack is unaffected. The branch is left in place if any of this is wanted later. |
PR 4 of the A3 alerting stack (
SATD_ALERTING_DESIGN.md§4.5). Stacked on #482 → #481 → #480 — merge bottom-up.A hook can now carry a watch-set and receive the same match events the streaming
Watchstream delivers:Decisions worth reviewing
watch_match_jsonmoves out of the WebSocket carrier intonode::events. The WS firehose and the webhook dispatcher now render matches with the same code, so a receiver written against the streaming spec parses webhook matches unchanged and the two surfaces cannot drift. This is the substance of the PR; the rest is plumbing.ResilientWatchuses, with the config file as the loader. A SIGHUP reload therefore re-registers for free.WatchSet'sDebugrenders counts only, and a malformed-key error deliberately does not echo the value (config errors end up in bug reports). Both are asserted by tests. Entries requiresilentpaymentindex=1.:vout, a bad txid, or an unknown key under[webhook.watch]is an error rather than a watch that silently matches nothing.Semantics worth stating
A watch-set is forward-only from the moment it is registered. Adding an entry does not replay history for it: you are told about payments from now on, not about the ones you already reconciled. That is the intended behavior for an alerting surface — a backfill would fire one notification per historical transaction touching the entry, which is not what "tell me when my wallet gets a deposit" means.
getaddresshistoryand the streaming API'sRescanBlocksare how you ask for history.A restart is not a gap: the watch-set is re-registered before P2P starts, so blocks arriving during catch-up are matched normally, and a
-reindexreplays before the dispatcher exists so it does not re-fire years of alerts. The one case that does lose a match is a crash between a block connecting and the receiver acknowledging — the block's chain event comes back from the hook's cursor, but the match does not, because the block is already connected and is not rescanned. Normally milliseconds; wider if the receiver is down and the queue has backed up. Documented in the manual, the spec's durability table, and the release notes.Acceptance
script_matchedat mempool → confirmed re-emit after mining.Debugoutput and from error messages; malformed targets rejected.silentpaymentindex=1.main.rs; the remaining crash-between-connect-and-ack window is documented).X-Satd-Delivery, including two matches produced by a single transaction — the case where a shared bus counter collided.--workspace --all-targets --all-featuresclean; suite green except the 10 pre-existingtest_address_index_backfill_*disk-guard failures on this machine.🤖 Generated with Claude Code
https://claude.ai/code/session_018AfAm3VVN8gBeYcTL5BJqH