mirror of
https://github.com/jmcorgan/fips.git
synced 2026-07-30 19:46:15 +00:00
Derive each peer's npub once instead of once per tick
The per-tick stats snapshot ran a bech32 encode for every tracked peer, and for the common mesh peer — one with no hosts-file entry and no configured alias — it ran a second one, because the display-name fallback chain bottoms out in the same encode. At 240 peers that was 14.1 ms per tick, a third of the tick body and its second largest cost, all of it recomputing values that cannot change. Cache the npub and the shortened npub on the peer at construction. An npub is a pure function of the peer's public key, and the identity is never mutated after construction: there is no setter, no identity_mut, and no assignment to the field anywhere in the tree, so the cache cannot go stale. The display name itself is deliberately NOT cached. Two of its inputs do mutate at runtime — the alias map and the host map, the latter reloaded on this same tick — so a resolved name stored on the peer would go stale on an alias change or a hosts reload. Only the immutable component is memoized. Tests cover both constructors, that the cached npub matches the identity, and that the display name still tracks an alias change. The memoization itself is asserted by pointer stability rather than by timing, so it is deterministic under load. Three deliberate breaks were each caught by exactly one test: re-deriving instead of memoizing, populating one constructor's cache from the wrong source, and reordering the display-name fallback so it stops honoring aliases.
This commit is contained in:
+1
-1
@@ -1223,7 +1223,7 @@ impl Node {
|
|||||||
return name.clone();
|
return name.clone();
|
||||||
}
|
}
|
||||||
if let Some(peer) = self.peers.get(addr) {
|
if let Some(peer) = self.peers.get(addr) {
|
||||||
return peer.identity().short_npub();
|
return peer.short_npub().to_string();
|
||||||
}
|
}
|
||||||
if let Some(entry) = self.sessions.get(addr) {
|
if let Some(entry) = self.sessions.get(addr) {
|
||||||
let (xonly, _) = entry.remote_pubkey().x_only_public_key();
|
let (xonly, _) = entry.remote_pubkey().x_only_public_key();
|
||||||
|
|||||||
@@ -2119,3 +2119,49 @@ fn test_transport_drop_state_steady_counter_fires_once() {
|
|||||||
assert!(!s.observe_drops(7));
|
assert!(!s.observe_drops(7));
|
||||||
assert!(!s.observe_drops(7));
|
assert!(!s.observe_drops(7));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn test_peer_display_name_uses_cached_short_npub() {
|
||||||
|
// Path 3 of `peer_display_name` (no host entry, no alias) reads the
|
||||||
|
// per-peer cached short npub; it must still equal the value derived
|
||||||
|
// from the peer's identity.
|
||||||
|
let mut node = make_node();
|
||||||
|
let peer_identity_full = Identity::generate();
|
||||||
|
let peer_addr = *peer_identity_full.node_addr();
|
||||||
|
let peer_identity = PeerIdentity::from_pubkey(peer_identity_full.pubkey());
|
||||||
|
node.peers
|
||||||
|
.insert(peer_addr, ActivePeer::new(peer_identity, LinkId::new(1), 0));
|
||||||
|
|
||||||
|
assert_eq!(
|
||||||
|
node.peer_display_name(&peer_addr),
|
||||||
|
peer_identity.short_npub()
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn test_peer_display_name_tracks_alias_change() {
|
||||||
|
// The display name is NOT cached on the peer: `peer_aliases` is a
|
||||||
|
// runtime-mutable map (`update_peers` inserts and removes entries), so
|
||||||
|
// a cached name would go stale. Caching only the immutable short npub
|
||||||
|
// must leave that tracking intact.
|
||||||
|
let mut node = make_node();
|
||||||
|
let peer_identity_full = Identity::generate();
|
||||||
|
let peer_addr = *peer_identity_full.node_addr();
|
||||||
|
let peer_identity = PeerIdentity::from_pubkey(peer_identity_full.pubkey());
|
||||||
|
node.peers
|
||||||
|
.insert(peer_addr, ActivePeer::new(peer_identity, LinkId::new(1), 0));
|
||||||
|
|
||||||
|
assert_eq!(
|
||||||
|
node.peer_display_name(&peer_addr),
|
||||||
|
peer_identity.short_npub()
|
||||||
|
);
|
||||||
|
|
||||||
|
node.peer_aliases.insert(peer_addr, "gateway".to_string());
|
||||||
|
assert_eq!(node.peer_display_name(&peer_addr), "gateway");
|
||||||
|
|
||||||
|
node.peer_aliases.remove(&peer_addr);
|
||||||
|
assert_eq!(
|
||||||
|
node.peer_display_name(&peer_addr),
|
||||||
|
peer_identity.short_npub()
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|||||||
+82
-1
@@ -81,6 +81,16 @@ pub struct ActivePeer {
|
|||||||
// === Identity (Verified) ===
|
// === Identity (Verified) ===
|
||||||
/// Cryptographic identity (verified via handshake).
|
/// Cryptographic identity (verified via handshake).
|
||||||
identity: PeerIdentity,
|
identity: PeerIdentity,
|
||||||
|
/// Bech32 npub, derived once at construction.
|
||||||
|
///
|
||||||
|
/// The npub is a pure function of `identity`'s public key, and
|
||||||
|
/// `identity` is never mutated after construction, so this can never
|
||||||
|
/// go stale. Deriving it costs a bech32 encode, which the per-tick
|
||||||
|
/// stats snapshot was paying once per peer per tick.
|
||||||
|
npub: String,
|
||||||
|
/// Shortened npub for log/UI display, derived once at construction.
|
||||||
|
/// Immutable for the same reason as [`ActivePeer::npub`].
|
||||||
|
short_npub: String,
|
||||||
|
|
||||||
// === Connection ===
|
// === Connection ===
|
||||||
/// Link used to reach this peer.
|
/// Link used to reach this peer.
|
||||||
@@ -224,6 +234,8 @@ impl ActivePeer {
|
|||||||
pub fn new(identity: PeerIdentity, link_id: LinkId, authenticated_at: u64) -> Self {
|
pub fn new(identity: PeerIdentity, link_id: LinkId, authenticated_at: u64) -> Self {
|
||||||
let now = Instant::now();
|
let now = Instant::now();
|
||||||
Self {
|
Self {
|
||||||
|
npub: identity.npub(),
|
||||||
|
short_npub: identity.short_npub(),
|
||||||
identity,
|
identity,
|
||||||
link_id,
|
link_id,
|
||||||
connectivity: ConnectivityState::Connected,
|
connectivity: ConnectivityState::Connected,
|
||||||
@@ -310,6 +322,8 @@ impl ActivePeer {
|
|||||||
) -> Self {
|
) -> Self {
|
||||||
let now = Instant::now();
|
let now = Instant::now();
|
||||||
Self {
|
Self {
|
||||||
|
npub: identity.npub(),
|
||||||
|
short_npub: identity.short_npub(),
|
||||||
identity,
|
identity,
|
||||||
link_id,
|
link_id,
|
||||||
connectivity: ConnectivityState::Connected,
|
connectivity: ConnectivityState::Connected,
|
||||||
@@ -423,8 +437,21 @@ impl ActivePeer {
|
|||||||
}
|
}
|
||||||
|
|
||||||
/// Get the peer's npub string.
|
/// Get the peer's npub string.
|
||||||
|
///
|
||||||
|
/// Returns a clone of the value cached at construction; the bech32
|
||||||
|
/// encode is not repeated.
|
||||||
pub fn npub(&self) -> String {
|
pub fn npub(&self) -> String {
|
||||||
self.identity.npub()
|
self.npub.clone()
|
||||||
|
}
|
||||||
|
|
||||||
|
/// Borrow the peer's cached npub without allocating.
|
||||||
|
pub fn npub_str(&self) -> &str {
|
||||||
|
&self.npub
|
||||||
|
}
|
||||||
|
|
||||||
|
/// Borrow the peer's cached shortened npub (e.g. `npub1abcd...wxyz`).
|
||||||
|
pub fn short_npub(&self) -> &str {
|
||||||
|
&self.short_npub
|
||||||
}
|
}
|
||||||
|
|
||||||
// === Connection Accessors ===
|
// === Connection Accessors ===
|
||||||
@@ -1219,6 +1246,60 @@ mod tests {
|
|||||||
assert!(peer.needs_filter_update()); // New peers need filter
|
assert!(peer.needs_filter_update()); // New peers need filter
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn test_npub_cache_matches_identity() {
|
||||||
|
let identity = make_peer_identity();
|
||||||
|
let peer = ActivePeer::new(identity, LinkId::new(1), 1000);
|
||||||
|
|
||||||
|
assert_eq!(peer.npub(), identity.npub());
|
||||||
|
assert_eq!(peer.npub_str(), identity.npub());
|
||||||
|
assert_eq!(peer.short_npub(), identity.short_npub());
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn test_npub_cache_matches_identity_with_session() {
|
||||||
|
// `with_session` builds its own struct literal, so it needs its
|
||||||
|
// own check that the cache is populated from the same identity.
|
||||||
|
let identity = make_peer_identity();
|
||||||
|
let (session, _peer_session) = ik_session_pair();
|
||||||
|
|
||||||
|
let peer = ActivePeer::with_session(
|
||||||
|
identity,
|
||||||
|
LinkId::new(1),
|
||||||
|
1000,
|
||||||
|
session,
|
||||||
|
SessionIndex::new(1),
|
||||||
|
SessionIndex::new(2),
|
||||||
|
TransportId::new(1),
|
||||||
|
TransportAddr::from_string("127.0.0.1:9000"),
|
||||||
|
LinkStats::new(),
|
||||||
|
true,
|
||||||
|
&MmpConfig::default(),
|
||||||
|
None,
|
||||||
|
);
|
||||||
|
|
||||||
|
assert_eq!(peer.npub(), identity.npub());
|
||||||
|
assert_eq!(peer.short_npub(), identity.short_npub());
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn test_npub_is_memoized_not_rederived() {
|
||||||
|
// The whole point of the fix: the strings are stored on the peer,
|
||||||
|
// not recomputed per call. A stored string keeps one heap buffer,
|
||||||
|
// so repeated borrows have a stable address. A per-call bech32
|
||||||
|
// encode would hand back a fresh allocation each time.
|
||||||
|
let identity = make_peer_identity();
|
||||||
|
let peer = ActivePeer::new(identity, LinkId::new(1), 1000);
|
||||||
|
|
||||||
|
let first = peer.npub_str().as_ptr();
|
||||||
|
let second = peer.npub_str().as_ptr();
|
||||||
|
assert_eq!(first, second);
|
||||||
|
|
||||||
|
let short_first = peer.short_npub().as_ptr();
|
||||||
|
let short_second = peer.short_npub().as_ptr();
|
||||||
|
assert_eq!(short_first, short_second);
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn test_connectivity_transitions() {
|
fn test_connectivity_transitions() {
|
||||||
let identity = make_peer_identity();
|
let identity = make_peer_identity();
|
||||||
|
|||||||
Reference in New Issue
Block a user