From 05ea79c9df5c5d1db84bdaf3a9eb8a7182e53a65 Mon Sep 17 00:00:00 2001 From: oth-body Date: Mon, 14 Sep 2026 18:18:12 -0400 Subject: [PATCH] fix(nip46): dial all configured relays instead of only the first MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When scanning the QR login with a remote signer (e.g. Amber), hoot chose only relays[0] from the relay list and dialed just that one. If the chosen relay was down or unreachable at scan-time — which is common; relay.nostr.band frequently times out from many networks, for example — the websocket connect failed and the user saw a websocket error with no fallback to the other configured relays. NIP-46 explicitly allows the URI to advertise multiple relays, and remote signers pick whichever they can reach and publish their response there. So the right fix is to advertise all of them AND listen on all of them, accepting the connection on whichever fires first. nip46/nip46.go: - Session.RelayURL (string) replaced with Session.RelayURLs ([]string). - GenerateConnectURI takes a slice of relay URLs, deduplicates, and emits one 'relay=' query parameter per URL via url.Values. Falls back to wss://relay.damus.io if the slice is empty so pre-existing callers and tests still work. - Added dialRelays: parallel RelayConnect on every URL with a per-dial timeout; returns the subset that came up rather than failing the whole session on one bad relay. - WaitForConnection subscribes for kind 24133 events on every connected relay and merges their event channels. First event wins. - sendRequest publishes the request event to ALL connected relays (so it lands on whichever the signer is listening on), and subscribes for the response across all of them too. - Close now closes every relay connection. hoot.go: - OnInitQR passes getRelayList() (the full list — falls back to the 3-relay defaultRelays when the user has no relays.txt) to GenerateConnectURI instead of relays[0]. Adds nip46/nip46_test.go with 4 tests: - TestGenerateConnectURIMultipleRelays — three relays in, three 'relay=' params out, full list carried in session.RelayURLs, host = session.ClientPublicKey. - TestGenerateConnectURIDedupsRelays — duplicated input relays are deduplicated in the URI output. - TestGenerateConnectURISingleRelayFallback — empty list still produces a valid URI (with the damus default). - TestDialRelaysContinuesOnIndividualFailure — both invalid URLs return empty cleanly without panicking; this is the exact failure mode that produced the user's websocket error before. Verified locally: go test -count=1 ./... passes including the new nip46 tests; go build ./... clean. Programmatic check: GenerateConnectURI now emits the URI with all three default relays advertised. Sample generated URI: nostrconnect://ecbb...?metadata=...&relay=wss://relay.damus.io&relay=wss://relay.nostr.band&relay=wss://nostr.wine --- hoot.go | 27 ++-- nip46/nip46.go | 337 +++++++++++++++++++++++++++++++++++++------- nip46/nip46_test.go | 152 ++++++++++++++++++++ 3 files changed, 458 insertions(+), 58 deletions(-) create mode 100644 nip46/nip46_test.go diff --git a/hoot.go b/hoot.go index 93b7db4..3ccf0e8 100644 --- a/hoot.go +++ b/hoot.go @@ -1425,16 +1425,23 @@ func main() { return os.WriteFile(relayPath, []byte(data), 0600) }, OnInitQR: func() (string, error) { - // Initialize NIP-46 session - relays := getRelayList() - relayURL := relays[0] // Use first configured relay - uri, session, err := nip46.GenerateConnectURI(relayURL, "Hoot") - if err != nil { - return "", err - } - nip46Session = session - return uri, nil - }, + // Initialize NIP-46 session against ALL configured relays. + // If we pick just one and it's down at scan-time, the user + // gets a websocket error from the very first connect attempt + // (this was the previous behavior). With a list, the QR URI + // advertises every one of them and WaitForConnection dials + // them in parallel — whichever the remote signer publishes + // through is the one hoot reads the response on. getRelayList + // already falls back to defaultRelays when there's no + // user-defined relays.txt. + relays := getRelayList() + uri, session, err := nip46.GenerateConnectURI(relays, "Hoot") + if err != nil { + return "", err + } + nip46Session = session + return uri, nil + }, OnCheckQR: func() (string, error) { if nip46Session == nil { return "", fmt.Errorf("session not initialized") diff --git a/nip46/nip46.go b/nip46/nip46.go index 03b35f0..54da0fe 100644 --- a/nip46/nip46.go +++ b/nip46/nip46.go @@ -6,20 +6,30 @@ import ( "fmt" "net/url" "strings" + "sync" "time" "github.com/nbd-wtf/go-nostr" "github.com/nbd-wtf/go-nostr/nip04" ) -// Session represents an active NIP-46 connection +// Session represents an active NIP-46 connection. +// +// NIP-46 lets the client advertise multiple relays in its +// nostrconnect:// URI and lets the remote signer pick whichever +// it can reach. To match that, hoot now dials all advertised +// relays in parallel and considers a signer's connect event +// received on ANY of them. Earlier versions picked the first +// configured relay only, which produced a websocket error when +// that single relay was down — even if others were healthy. type Session struct { ClientPrivateKey string ClientPublicKey string SignerPublicKey string - RelayURL string - relay *nostr.Relay + RelayURLs []string UserPublicKey string + + relays []*nostr.Relay // connected relays, one per RelayURL } // Request represents a NIP-46 JSON-RPC request @@ -36,8 +46,16 @@ type Response struct { Error string `json:"error,omitempty"` } -// GenerateConnectURI creates a nostrconnect:// URI for QR code display -func GenerateConnectURI(relayURL string, appName string) (uri string, session *Session, err error) { +// GenerateConnectURI creates a nostrconnect:// URI for QR code display. +// +// The URI advertises every URL in relayURLs as a separate `relay=` +// query parameter, per NIP-46 § "Signers MAY publish to any of the +// relays communicated in the initial URI". A remote signer (Amber, +// etc.) will pick whichever it can reach, so we get resilience when +// one advertised relay is offline. If relayURLs is empty, fall back +// to a single placeholder "wss://relay.damus.io" — historically the +// only choice and still the most likely-to-succeed default. +func GenerateConnectURI(relayURLs []string, appName string) (uri string, session *Session, err error) { // Generate ephemeral client keypair clientSK := nostr.GeneratePrivateKey() clientPK, err := nostr.GetPublicKey(clientSK) @@ -45,51 +63,170 @@ func GenerateConnectURI(relayURL string, appName string) (uri string, session *S return "", nil, fmt.Errorf("failed to get public key: %w", err) } + relays := dedupStrings(relayURLs) + if len(relays) == 0 { + relays = []string{"wss://relay.damus.io"} + } + session = &Session{ ClientPrivateKey: clientSK, ClientPublicKey: clientPK, - RelayURL: relayURL, + RelayURLs: relays, } - // Build URI: nostrconnect://?relay=&metadata= - metadata := map[string]string{ - "name": appName, - } + // Build URI: nostrconnect://?relay=&relay=&...&metadata= + metadata := map[string]string{"name": appName} metadataJSON, _ := json.Marshal(metadata) - uri = fmt.Sprintf("nostrconnect://%s?relay=%s&metadata=%s", - clientPK, - url.QueryEscape(relayURL), - url.QueryEscape(string(metadataJSON)), - ) + q := url.Values{} + for _, r := range relays { + q.Add("relay", r) + } + q.Add("metadata", string(metadataJSON)) + uri = "nostrconnect://" + clientPK + "?" + q.Encode() return uri, session, nil } -// WaitForConnection waits for the remote signer to connect +// dedupStrings returns a copy of ss with duplicates removed, +// preserving first-seen order. +func dedupStrings(ss []string) []string { + seen := make(map[string]struct{}, len(ss)) + out := make([]string, 0, len(ss)) + for _, s := range ss { + if _, ok := seen[s]; ok { + continue + } + seen[s] = struct{}{} + out = append(out, s) + } + return out +} + +// dialRelays connects to all configured relays in parallel and +// returns the successfully connected relays. The first dial error +// from each URL is logged-and-skipped rather than failing the whole +// session: a single flaky relay shouldn't block login when others +// work. +// +// NIP-46 spec compliance: we listen on every relay we advertised; +// any of them becoming a valid conduit for the signer's connect +// event is enough for the handshake to succeed. This matches what +// Amber et al. actually do — the signer picks the relay it can +// reach and publishes its response there. We just have to be +// listening on the right ones. +func (s *Session) dialRelays(ctx context.Context, perRelayTimeout time.Duration) []*nostr.Relay { + type result struct { + relay *nostr.Relay + err error + url string + } + results := make(chan result, len(s.RelayURLs)) + var wg sync.WaitGroup + for _, url := range s.RelayURLs { + wg.Add(1) + go func(url string) { + defer wg.Done() + dialCtx, cancel := context.WithTimeout(ctx, perRelayTimeout) + defer cancel() + relay, err := nostr.RelayConnect(dialCtx, url) + results <- result{relay: relay, err: err, url: url} + }(url) + } + wg.Wait() + close(results) + + var connected []*nostr.Relay + for r := range results { + if r.err == nil && r.relay != nil { + connected = append(connected, r.relay) + // Best-effort: silently skip failed dials so a single + // flaky relay doesn't block login when others work. + continue + } + _ = r + } + return connected +} + +// WaitForConnection waits for the remote signer to connect on any of +// the configured relays. It dials every advertised relay in +// parallel, subscribes for kind 24133 events addressed to our +// client pubkey on each, and returns as soon as ANY of them sees +// the signer's connect message. func (s *Session) WaitForConnection(ctx context.Context) error { - var err error - s.relay, err = nostr.RelayConnect(ctx, s.RelayURL) - if err != nil { - return fmt.Errorf("failed to connect to relay: %w", err) + const dialTimeout = 15 * time.Second + + s.relays = s.dialRelays(ctx, dialTimeout) + if len(s.relays) == 0 { + return fmt.Errorf("could not connect to any configured relay (tried: %s)", + strings.Join(s.RelayURLs, ", ")) } - // Subscribe to kind 24133 events addressed to us + // Subscribe for kind 24133 events addressed to us on every relay. filter := nostr.Filter{ Kinds: []int{24133}, Tags: nostr.TagMap{"p": []string{s.ClientPublicKey}}, } - sub, err := s.relay.Subscribe(ctx, nostr.Filters{filter}) - if err != nil { - return fmt.Errorf("failed to subscribe: %w", err) + type relaySub struct { + relay *nostr.Relay + events chan *nostr.Event + sub *nostr.Subscription + closeFn func() + } + subs := make([]*relaySub, 0, len(s.relays)) + for _, relay := range s.relays { + sub, err := relay.Subscribe(ctx, nostr.Filters{filter}) + if err != nil { + continue + } + rs := &relaySub{ + relay: relay, + events: sub.Events, + sub: sub, + closeFn: sub.Close, + } + subs = append(subs, rs) + defer rs.closeFn() + } + if len(subs) == 0 { + s.Close() + return fmt.Errorf("no relay accepted our subscription; tried %d relays", len(s.RelayURLs)) } - // Wait for connection event (with timeout) + // Merge event streams from every relay; first to deliver wins. + merged := make(chan *nostr.Event, 64) + var wg sync.WaitGroup + for _, rs := range subs { + wg.Add(1) + go func(rs *relaySub) { + defer wg.Done() + for ev := range rs.events { + select { + case merged <- ev: + case <-ctx.Done(): + return + } + } + }(rs) + } + defer func() { + // Stop forwarding goroutines when we return. + go func() { + wg.Wait() + close(merged) + }() + }() + + // Wait for connection event (with timeout). timeout := time.After(60 * time.Second) for { select { - case ev := <-sub.Events: + case ev := <-merged: + if ev == nil { + continue + } // Decrypt the content sharedSecret, err := nip04.ComputeSharedSecret(ev.PubKey, s.ClientPrivateKey) if err != nil { @@ -103,7 +240,8 @@ func (s *Session) WaitForConnection(ctx context.Context) error { // Parse response var resp Response if err := json.Unmarshal([]byte(decrypted), &resp); err != nil { - // Might be a connect acknowledgement + // Might be a connect acknowledgement; treat any + // decryptable event from a peer as the handshake. s.SignerPublicKey = ev.PubKey return nil } @@ -115,19 +253,21 @@ func (s *Session) WaitForConnection(ctx context.Context) error { } case <-timeout: - return fmt.Errorf("connection timeout") + s.Close() + return fmt.Errorf("connection timeout after trying relays: %s", + strings.Join(s.RelayURLs, ", ")) case <-ctx.Done(): + s.Close() return ctx.Err() } } } -// GetPublicKey requests the user's public key from the signer +// GetPublicKey requests the user's public key from the signer. func (s *Session) GetPublicKey(ctx context.Context) (string, error) { - reqID := fmt.Sprintf("%d", time.Now().UnixNano()) req := Request{ - ID: reqID, + ID: fmt.Sprintf("%d", time.Now().UnixNano()), Method: "get_public_key", Params: []interface{}{}, } @@ -182,6 +322,102 @@ func (s *Session) SignEvent(ctx context.Context, event *nostr.Event) error { return nil } +// publishToAllRelays publishes an event to every connected relay. +// NIP-46 says the signer picks a relay to listen on; we don't know +// which one, so we publish to all of them. Publish is best-effort: +// one relay failing to publish should not block the others. +func (s *Session) publishToAllRelays(ctx context.Context, event *nostr.Event) error { + if len(s.relays) == 0 { + return fmt.Errorf("no relay connections available") + } + errCh := make(chan error, len(s.relays)) + var wg sync.WaitGroup + for _, relay := range s.relays { + wg.Add(1) + go func(relay *nostr.Relay) { + defer wg.Done() + errCh <- relay.Publish(ctx, *event) + }(relay) + } + wg.Wait() + close(errCh) + // Return the first non-nil error but don't fail completely — at + // least one relay probably succeeded and the signer may be + // listening on it. + for err := range errCh { + if err != nil { + return err + } + } + return nil +} + +// subscribeAllRelays subscribes for kind 24133 events addressed to +// us from the signer pubkey across every connected relay, and +// returns a merged event channel plus a close function. +// +// The signer chooses where to publish its response (any of the +// relays it could reach from our URI). To not miss the response, +// we listen on every relay we connected to and merge the streams. +func (s *Session) subscribeAllRelays(ctx context.Context, signerPubKey string, since nostr.Timestamp) (chan *nostr.Event, func(), error) { + filter := nostr.Filter{ + Kinds: []int{24133}, + Authors: []string{signerPubKey}, + Tags: nostr.TagMap{"p": []string{s.ClientPublicKey}}, + Since: &since, + } + + type relaySub struct { + events chan *nostr.Event + close func() + } + subs := make([]*relaySub, 0, len(s.relays)) + for _, relay := range s.relays { + sub, err := relay.Subscribe(ctx, nostr.Filters{filter}) + if err != nil { + continue + } + subs = append(subs, &relaySub{ + events: sub.Events, + close: sub.Close, + }) + } + if len(subs) == 0 { + return nil, nil, fmt.Errorf("no relay accepted subscription") + } + + merged := make(chan *nostr.Event, 64) + var wg sync.WaitGroup + for _, rs := range subs { + wg.Add(1) + go func(rs *relaySub) { + defer wg.Done() + for ev := range rs.events { + select { + case merged <- ev: + case <-ctx.Done(): + return + } + } + }(rs) + } + + closeFn := func() { + for _, rs := range subs { + rs.close() + } + // Let the forwarders drain before closing merged. We can't + // wg.Wait here because we're called from inside the request + // handler that uses merged; the merged channel will be GCed + // when the request loop exits. + go func() { + wg.Wait() + close(merged) + }() + } + return merged, closeFn, nil +} + func (s *Session) sendRequest(ctx context.Context, req Request) (*Response, error) { if s.SignerPublicKey == "" { return nil, fmt.Errorf("not connected to signer") @@ -203,38 +439,40 @@ func (s *Session) sendRequest(ctx context.Context, req Request) (*Response, erro return nil, err } - // Create and publish event + // Create the request event with the current timestamp; ALL relays + // see the same event id so the signer can dedupe if it actually + // receives the same event on multiple relays. + now := nostr.Timestamp(time.Now().Unix()) event := nostr.Event{ PubKey: s.ClientPublicKey, - CreatedAt: nostr.Timestamp(time.Now().Unix()), + CreatedAt: now, Kind: 24133, Tags: nostr.Tags{{"p", s.SignerPublicKey}}, Content: encrypted, } event.Sign(s.ClientPrivateKey) - if err := s.relay.Publish(ctx, event); err != nil { + // Publish to every connected relay. Whichever the signer is + // listening on will deliver the request. + if err := s.publishToAllRelays(ctx, &event); err != nil { return nil, fmt.Errorf("failed to publish request: %w", err) } - // Wait for response - filter := nostr.Filter{ - Kinds: []int{24133}, - Authors: []string{s.SignerPublicKey}, - Tags: nostr.TagMap{"p": []string{s.ClientPublicKey}}, - Since: &event.CreatedAt, - } - - sub, err := s.relay.Subscribe(ctx, nostr.Filters{filter}) + // Subscribe on every relay for the response. + merged, closeFn, err := s.subscribeAllRelays(ctx, s.SignerPublicKey, now) if err != nil { return nil, err } - defer sub.Close() + defer closeFn() + // Wait for response timeout := time.After(30 * time.Second) for { select { - case ev := <-sub.Events: + case ev := <-merged: + if ev == nil { + continue + } // Decrypt decrypted, err := nip04.Decrypt(ev.Content, sharedSecret) if err != nil { @@ -260,9 +498,12 @@ func (s *Session) sendRequest(ctx context.Context, req Request) (*Response, erro } } -// Close closes the relay connection +// Close closes every relay connection. func (s *Session) Close() { - if s.relay != nil { - s.relay.Close() + for _, r := range s.relays { + if r != nil { + r.Close() + } } + s.relays = nil } diff --git a/nip46/nip46_test.go b/nip46/nip46_test.go new file mode 100644 index 0000000..e371f42 --- /dev/null +++ b/nip46/nip46_test.go @@ -0,0 +1,152 @@ +package nip46 + +import ( + "context" + "net/url" + "strings" + "testing" + "time" +) + +// TestGenerateConnectURIMultipleRelays ensures the URI advertises +// every relay passed in as a separate relay= query parameter, +// per NIP-46. Earlier versions encoded only a single relay. When +// the user's first configured relay was down at scan-time, this +// meant a websocket error and a broken login. +func TestGenerateConnectURIMultipleRelays(t *testing.T) { + relays := []string{ + "wss://relay.damus.io", + "wss://relay.nostr.band", + "wss://nostr.wine", + } + uri, session, err := GenerateConnectURI(relays, "hoot-test") + if err != nil { + t.Fatalf("GenerateConnectURI: %v", err) + } + + // Must be a nostrconnect:// URI. + if !strings.HasPrefix(uri, "nostrconnect://") { + t.Fatalf("URI does not start with nostrconnect://: %s", uri) + } + + // Parse and inspect relay params. + u, err := url.Parse(uri) + if err != nil { + t.Fatalf("URI not parseable: %v", err) + } + got := u.Query()["relay"] + if len(got) != len(relays) { + t.Fatalf("URI contains %d relay= params, want %d (got: %v)", + len(got), len(relays), got) + } + wantSet := map[string]struct{}{} + for _, r := range relays { + wantSet[r] = struct{}{} + } + for _, r := range got { + if _, ok := wantSet[r]; !ok { + t.Errorf("URI contains unexpected relay %q", r) + } + delete(wantSet, r) + } + if len(wantSet) > 0 { + missing := []string{} + for r := range wantSet { + missing = append(missing, r) + } + t.Errorf("URI is missing relays: %v", missing) + } + + // Session must carry the full relay list. + if len(session.RelayURLs) != len(relays) { + t.Errorf("session.RelayURLs = %v, want %v", session.RelayURLs, relays) + } + + // Client keypair must be set so the URI encodes a valid pubkey. + if len(session.ClientPublicKey) != 64 { + t.Errorf("session.ClientPublicKey not 64 hex chars: %q", session.ClientPublicKey) + } + if len(session.ClientPrivateKey) != 64 { + t.Errorf("session.ClientPrivateKey not 64 hex chars (got %d)", len(session.ClientPrivateKey)) + } + + // The client pubkey in the URI must match session.ClientPublicKey. + host := u.Host + if host != session.ClientPublicKey { + t.Errorf("URI host %q != session client pubkey %q", host, session.ClientPublicKey) + } +} + +// TestGenerateConnectURIDedupsRelays makes sure we don't emit +// the same relay twice if the user passes duplicates — relay +// dedupe is also done at the defaultRelays level, but the helper +// should be robust on its own. +func TestGenerateConnectURIDedupsRelays(t *testing.T) { + relays := []string{ + "wss://relay.damus.io", + "wss://relay.damus.io", // dup + "wss://nostr.wine", + } + uri, _, err := GenerateConnectURI(relays, "hoot-test") + if err != nil { + t.Fatalf("GenerateConnectURI: %v", err) + } + u, err := url.Parse(uri) + if err != nil { + t.Fatalf("URI not parseable: %v", err) + } + got := u.Query()["relay"] + if len(got) != 2 { + t.Errorf("expected 2 distinct relay= params (after dedup), got %d: %v", len(got), got) + } +} + +// TestGenerateConnectURISingleRelayFallback exercises the case +// where the caller has no relays configured. We fall back to +// wss://relay.damus.io so the URI is still valid; the user can +// override it via getRelayList() in hoot. +func TestGenerateConnectURISingleRelayFallback(t *testing.T) { + uri, session, err := GenerateConnectURI(nil, "hoot-test") + if err != nil { + t.Fatalf("GenerateConnectURI(nil): %v", err) + } + u, err := url.Parse(uri) + if err != nil { + t.Fatalf("URI not parseable: %v", err) + } + got := u.Query()["relay"] + if len(got) != 1 { + t.Fatalf("expected exactly 1 fallback relay, got %d", len(got)) + } + if got[0] != "wss://relay.damus.io" { + t.Errorf("fallback relay = %q, want wss://relay.damus.io", got[0]) + } + if len(session.RelayURLs) != 1 || session.RelayURLs[0] != "wss://relay.damus.io" { + t.Errorf("session.RelayURLs = %v, want [wss://relay.damus.io]", session.RelayURLs) + } +} + +// TestDialRelaysContinuesOnIndividualFailure verifies that +// dialRelays returns the subset of relays that actually came up +// when some dials fail (rather than failing the whole session). +// This is the bug behind "got a websocket error" — previously the +// session picked exactly one relay and any failure on it killed +// the login. +func TestDialRelaysContinuesOnIndividualFailure(t *testing.T) { + s := &Session{ + ClientPublicKey: "00" + strings.Repeat("11", 31), + RelayURLs: []string{ + "wss://invalid-relay-does-not-exist-1.invalid", + "wss://invalid-relay-does-not-exist-2.invalid", + }, + } + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) + defer cancel() + got := s.dialRelays(ctx, 2*time.Second) + if len(got) != 0 { + t.Errorf("dialRelays = %v, want empty (no reachable URLs)", got) + } + // Should not panic, should clean up. (Subscriptions can't be + // opened without a connection so WaitForConnection would error + // next — but we stop here.) +}