mirror of
https://github.com/jmcorgan/fips.git
synced 2026-07-30 11:36:15 +00:00
Send traversal signals only to relays the client pool actually holds
A traversal signal is addressed to a merge of the peer's NIP-17 inbox relays, the relays its advert nominates for signaling, and our own DM relays. The client pool is built once at startup from our configured relays and never added to, and send_event_to rejects the entire send with "relay not found" if any single URL in the list is outside that pool, before contacting anything. So one relay we are not configured with, anywhere in that merge, killed the whole attempt -- including the sends to relays we do share and that would have carried the signal. In an open-mode window on a public node this made discovery non-functional: 309 traversal attempts, 290 explicit failures, zero successes, every failure on "relay not found". Configured peers were unaffected because they run a matching relay set. Filter the merged list down to relays the pool holds before sending. Our own DM relays are always in the merge and always in the pool, so the result is empty only when no DM relay is configured at all, which is already a total failure. Comparison is on the normalized RelayUrl rather than the raw string, because that is how the pool is keyed -- a raw comparison would discard a configured relay spelled with a trailing slash or a different host case, which is the same defect in a quieter form. Merge and filter are one synchronous function so the decision can be exercised without a relay client. The pool is read via all_relays(), which is the set send_event_to validates against; relays() is filtered by service flags and would be narrower. Two smaller fixes ride along. The responder now resolves its relays before binding a socket and running STUN, instead of spending a STUN round trip and holding an offer slot only to discover it has nowhere to answer. And it gained the empty-list guard the initiator already had, which gives BootstrapError::MissingRelays a condition it can reach for the first time -- it was unreachable, since the merge always appended our own DM relays.
This commit is contained in:
@@ -1302,6 +1302,16 @@ impl NostrDiscovery {
|
||||
|
||||
self.mark_session_seen(&offer.session_id).await?;
|
||||
|
||||
// Resolve the answer's relays before binding a socket and running STUN.
|
||||
// Nothing in the relay choice depends on what STUN observes, and an offer
|
||||
// from a peer we share no relay with cannot be answered at all — doing it
|
||||
// in this order spends a STUN round trip, and holds an offer slot for its
|
||||
// duration, only to discard the result.
|
||||
let relays = self.preferred_signal_relays(sender, None).await?;
|
||||
if relays.is_empty() {
|
||||
return Err(BootstrapError::MissingRelays(offer.sender_npub.clone()));
|
||||
}
|
||||
|
||||
let base_socket = std::net::UdpSocket::bind(("0.0.0.0", 0))?;
|
||||
base_socket.set_nonblocking(true)?;
|
||||
let (reflexive_address, local_addresses, stun_server) = observe_traversal_addresses(
|
||||
@@ -1336,7 +1346,6 @@ impl NostrDiscovery {
|
||||
(!accepted).then_some("no-usable-addresses".to_string()),
|
||||
Some(offer_received_at),
|
||||
);
|
||||
let relays = self.preferred_signal_relays(sender, None).await?;
|
||||
let answer_event = self.send_signal(&relays, sender, &answer).await?;
|
||||
debug!(
|
||||
peer = %peer_short,
|
||||
@@ -1466,22 +1475,21 @@ impl NostrDiscovery {
|
||||
target_pubkey: PublicKey,
|
||||
advert: Option<&OverlayAdvert>,
|
||||
) -> Result<Vec<String>, BootstrapError> {
|
||||
let mut merged = self.find_recipient_inbox_relays(target_pubkey).await?;
|
||||
if let Some(advert) = advert
|
||||
&& let Some(relays) = advert.signal_relays.as_ref()
|
||||
{
|
||||
for relay in relays {
|
||||
if !merged.contains(relay) {
|
||||
merged.push(relay.clone());
|
||||
}
|
||||
}
|
||||
}
|
||||
for relay in &self.config.dm_relays {
|
||||
if !merged.contains(relay) {
|
||||
merged.push(relay.clone());
|
||||
}
|
||||
}
|
||||
Ok(merged)
|
||||
let inbox = self.find_recipient_inbox_relays(target_pubkey).await?;
|
||||
let pool: HashSet<RelayUrl> = self.client.pool().all_relays().await.into_keys().collect();
|
||||
let usable = signal_relays(
|
||||
&inbox,
|
||||
advert.and_then(|advert| advert.signal_relays.as_deref()),
|
||||
&self.config.dm_relays,
|
||||
&pool,
|
||||
);
|
||||
debug!(
|
||||
peer = %target_pubkey.to_bech32().map(|npub| short_npub(&npub)).unwrap_or_default(),
|
||||
inbox = inbox.len(),
|
||||
usable = usable.len(),
|
||||
"traversal: signal relays resolved against the client pool"
|
||||
);
|
||||
Ok(usable)
|
||||
}
|
||||
|
||||
async fn find_recipient_inbox_relays(
|
||||
@@ -1732,6 +1740,56 @@ impl NostrDiscovery {
|
||||
}
|
||||
}
|
||||
|
||||
/// Retain only the candidates the client pool actually holds.
|
||||
///
|
||||
/// `send_event_to` rejects the whole send with `RelayNotFound` if any single URL
|
||||
/// is outside the pool, so a signal addressed to a peer's advertised relays fails
|
||||
/// entirely on one relay we are not configured with. Filtering first turns that
|
||||
/// into a send to the relays we share.
|
||||
///
|
||||
/// Comparison is on the normalized `RelayUrl` rather than the raw string, because
|
||||
/// the pool is keyed that way: a candidate spelled `wss://relay.example/` matches
|
||||
/// a configured `wss://relay.example`. Order is preserved, candidates that fail
|
||||
/// to parse are dropped, and duplicates that normalize alike are collapsed.
|
||||
fn retain_pooled_relays(candidates: &[String], pool: &HashSet<RelayUrl>) -> Vec<String> {
|
||||
let mut seen: HashSet<RelayUrl> = HashSet::new();
|
||||
let mut usable = Vec::with_capacity(candidates.len());
|
||||
for candidate in candidates {
|
||||
let Ok(url) = RelayUrl::parse(candidate) else {
|
||||
continue;
|
||||
};
|
||||
if pool.contains(&url) && seen.insert(url.clone()) {
|
||||
usable.push(url.to_string());
|
||||
}
|
||||
}
|
||||
usable
|
||||
}
|
||||
|
||||
/// Choose the relays a traversal signal for one peer should be sent to.
|
||||
///
|
||||
/// The candidates are the peer's NIP-17 inbox relays, then the relays its advert
|
||||
/// nominates for signaling, then our own DM relays — remote-supplied first, ours
|
||||
/// last, so a peer's preference is honored where we can act on it. The result is
|
||||
/// whatever survives [`retain_pooled_relays`].
|
||||
///
|
||||
/// This is the whole decision, kept synchronous so it can be exercised without a
|
||||
/// relay client: the caller's only job is to supply the fetched inbox list and
|
||||
/// the pool.
|
||||
pub(super) fn signal_relays(
|
||||
inbox: &[String],
|
||||
advert_signal: Option<&[String]>,
|
||||
dm_relays: &[String],
|
||||
pool: &HashSet<RelayUrl>,
|
||||
) -> Vec<String> {
|
||||
let mut merged: Vec<String> = inbox.to_vec();
|
||||
for relay in advert_signal.unwrap_or_default().iter().chain(dm_relays) {
|
||||
if !merged.contains(relay) {
|
||||
merged.push(relay.clone());
|
||||
}
|
||||
}
|
||||
retain_pooled_relays(&merged, pool)
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
impl NostrDiscovery {
|
||||
/// Build a minimal `NostrDiscovery` for unit tests. No relay client is
|
||||
|
||||
@@ -1,6 +1,8 @@
|
||||
use nostr::prelude::{EventBuilder, Kind, Tag, Timestamp};
|
||||
use std::collections::HashSet;
|
||||
|
||||
use super::runtime::{NostrDiscovery, suppress_responder_for_own_initiator};
|
||||
use nostr::prelude::{EventBuilder, Kind, RelayUrl, Tag, Timestamp};
|
||||
|
||||
use super::runtime::{NostrDiscovery, signal_relays, suppress_responder_for_own_initiator};
|
||||
use super::signal::{
|
||||
FreshnessOutcome, build_signal_event, create_traversal_answer, create_traversal_offer,
|
||||
estimate_clock_skew, validate_offer_freshness, validate_traversal_answer_for_offer,
|
||||
@@ -707,3 +709,150 @@ fn now_ms_tracks_the_wall_clock() {
|
||||
"now_ms() is ahead of the wall clock: {sampled} > {after}"
|
||||
);
|
||||
}
|
||||
|
||||
fn pool(urls: &[&str]) -> HashSet<RelayUrl> {
|
||||
urls.iter()
|
||||
.map(|url| RelayUrl::parse(url).expect("test pool url parses"))
|
||||
.collect()
|
||||
}
|
||||
|
||||
fn candidates(urls: &[&str]) -> Vec<String> {
|
||||
urls.iter().map(|url| url.to_string()).collect()
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn out_of_pool_relay_does_not_suppress_the_shared_ones() {
|
||||
let usable = signal_relays(
|
||||
&candidates(&[
|
||||
"wss://relay.damus.io",
|
||||
"wss://temp.iris.to",
|
||||
"wss://nos.lol",
|
||||
]),
|
||||
None,
|
||||
&[],
|
||||
&pool(&[
|
||||
"wss://relay.damus.io",
|
||||
"wss://nos.lol",
|
||||
"wss://offchain.pub",
|
||||
]),
|
||||
);
|
||||
assert_eq!(
|
||||
usable,
|
||||
vec![
|
||||
"wss://relay.damus.io".to_string(),
|
||||
"wss://nos.lol".to_string()
|
||||
],
|
||||
"the unknown relay must be dropped without taking the shared ones with it"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn trailing_slash_and_host_case_variants_are_retained() {
|
||||
let usable = signal_relays(
|
||||
&candidates(&["wss://Relay.Damus.io/", "wss://nos.lol"]),
|
||||
None,
|
||||
&[],
|
||||
&pool(&["wss://relay.damus.io", "wss://nos.lol"]),
|
||||
);
|
||||
assert_eq!(
|
||||
usable.len(),
|
||||
2,
|
||||
"normalized spellings of a configured relay are the same relay: {usable:?}"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn duplicates_that_normalize_alike_are_collapsed() {
|
||||
let usable = signal_relays(
|
||||
&candidates(&["wss://nos.lol", "wss://nos.lol/", "wss://NOS.LOL"]),
|
||||
None,
|
||||
&[],
|
||||
&pool(&["wss://nos.lol"]),
|
||||
);
|
||||
assert_eq!(usable, vec!["wss://nos.lol".to_string()]);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn unparseable_candidates_are_dropped_rather_than_failing_the_set() {
|
||||
let usable = signal_relays(
|
||||
&candidates(&["not a url", "wss://nos.lol"]),
|
||||
None,
|
||||
&[],
|
||||
&pool(&["wss://nos.lol"]),
|
||||
);
|
||||
assert_eq!(usable, vec!["wss://nos.lol".to_string()]);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn no_shared_relay_yields_an_empty_set_for_the_caller_to_reject() {
|
||||
let usable = signal_relays(
|
||||
&candidates(&["wss://temp.iris.to"]),
|
||||
None,
|
||||
&[],
|
||||
&pool(&["wss://nos.lol"]),
|
||||
);
|
||||
assert!(
|
||||
usable.is_empty(),
|
||||
"with no overlap the caller must see nothing to send to, not a doomed send"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn signal_relays_merges_all_three_sources_then_filters() {
|
||||
let usable = signal_relays(
|
||||
&candidates(&["wss://temp.iris.to", "wss://nos.lol"]),
|
||||
Some(&candidates(&[
|
||||
"wss://relay.damus.io",
|
||||
"wss://unknown.example",
|
||||
])),
|
||||
&candidates(&["wss://offchain.pub"]),
|
||||
&pool(&[
|
||||
"wss://nos.lol",
|
||||
"wss://relay.damus.io",
|
||||
"wss://offchain.pub",
|
||||
]),
|
||||
);
|
||||
assert_eq!(
|
||||
usable,
|
||||
vec![
|
||||
"wss://nos.lol".to_string(),
|
||||
"wss://relay.damus.io".to_string(),
|
||||
"wss://offchain.pub".to_string(),
|
||||
],
|
||||
"every source must contribute, and only the out-of-pool entries drop out"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn signal_relays_keeps_our_dm_relays_when_the_peer_shares_nothing() {
|
||||
let usable = signal_relays(
|
||||
&candidates(&["wss://temp.iris.to"]),
|
||||
Some(&candidates(&["wss://also.unknown"])),
|
||||
&candidates(&["wss://nos.lol"]),
|
||||
&pool(&["wss://nos.lol"]),
|
||||
);
|
||||
assert_eq!(
|
||||
usable,
|
||||
vec!["wss://nos.lol".to_string()],
|
||||
"our own DM relays are always in the pool, so the result is never empty \
|
||||
while any are configured"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn signal_relays_without_an_advert_still_resolves() {
|
||||
let usable = signal_relays(
|
||||
&candidates(&["wss://nos.lol", "wss://temp.iris.to"]),
|
||||
None,
|
||||
&candidates(&["wss://offchain.pub"]),
|
||||
&pool(&["wss://nos.lol", "wss://offchain.pub"]),
|
||||
);
|
||||
assert_eq!(
|
||||
usable,
|
||||
vec![
|
||||
"wss://nos.lol".to_string(),
|
||||
"wss://offchain.pub".to_string()
|
||||
],
|
||||
"the responder path passes no advert and must still produce a target set"
|
||||
);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user