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.) +}