Skip to content

Commit c8e8fff

Browse files
Sera (Bartok9)Bartok9
authored andcommitted
fix(peer_store): upsert peer address when re-adding known peer
Previously PeerStore::add_peer returned Ok early when the node_id was already present, so a changed SocketAddress (e.g. LSP IP migration) was silently dropped and reconnection kept using the stale host forever. Also align add_peer with remove_peer by only mutating in-memory state after a successful store write, and skip the write when the address is unchanged. Fixes #700. Co-authored-by prior attempts: - #735 @chahat-101 (abandoned; incorporated maintainer direction from review) - #801 @ben-kaufman (closed; peer-store plot only here — no bindings/version bump) AI: assisted with Hermes/Grok (Nous). Human/agent review by Sera (agent_id=sera).
1 parent abbed3e commit c8e8fff

1 file changed

Lines changed: 105 additions & 6 deletions

File tree

src/peer_store.rs

Lines changed: 105 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -42,17 +42,27 @@ where
4242
Self { peers, mutation_lock, kv_store, logger }
4343
}
4444

45+
/// Inserts or updates a peer entry.
46+
///
47+
/// If the peer is already known with the same address, this is a no-op. If the peer is new or
48+
/// the stored address changed (e.g. an LSP migrated hosts), the entry is updated and persisted.
49+
/// In-memory state is only mutated after a successful store write, matching [`Self::remove_peer`].
4550
pub(crate) async fn add_peer(&self, peer_info: PeerInfo) -> Result<(), Error> {
4651
let _guard = self.mutation_lock.lock().await;
4752
let data = {
48-
let mut locked_peers = self.peers.write().expect("lock");
49-
if locked_peers.contains_key(&peer_info.node_id) {
50-
return Ok(());
53+
let locked_peers = self.peers.read().expect("lock");
54+
if let Some(existing) = locked_peers.get(&peer_info.node_id) {
55+
if existing.address == peer_info.address {
56+
return Ok(());
57+
}
5158
}
52-
locked_peers.insert(peer_info.node_id, peer_info);
53-
PeerStoreSerWrapper(&locked_peers).encode()
59+
let mut updated_peers = locked_peers.clone();
60+
updated_peers.insert(peer_info.node_id, peer_info.clone());
61+
PeerStoreSerWrapper(&updated_peers).encode()
5462
};
55-
self.persist_peers(data).await
63+
self.persist_peers(data).await?;
64+
self.peers.write().expect("lock").insert(peer_info.node_id, peer_info);
65+
Ok(())
5666
}
5767

5868
pub(crate) async fn remove_peer(&self, node_id: &PublicKey) -> Result<(), Error> {
@@ -277,4 +287,93 @@ mod tests {
277287
assert_eq!(Err(Error::PersistenceFailed), peer_store.remove_peer(&node_id).await);
278288
assert_eq!(Some(peer_info), peer_store.get_peer(&node_id));
279289
}
290+
291+
#[tokio::test]
292+
async fn peer_address_updated_on_readd() {
293+
let store: Arc<DynStore> = Arc::new(DynStoreWrapper(InMemoryStore::new()));
294+
let logger = Arc::new(TestLogger::new());
295+
let peer_store = PeerStore::new(Arc::clone(&store), Arc::clone(&logger));
296+
297+
let node_id = PublicKey::from_str(
298+
"0276607124ebe6a6c9338517b6f485825b27c2dcc0b9fc2aa6a4c0df91194e5993",
299+
)
300+
.unwrap();
301+
let old_address = SocketAddress::from_str("127.0.0.1:9738").unwrap();
302+
let new_address = SocketAddress::from_str("127.0.0.1:9739").unwrap();
303+
304+
peer_store.add_peer(PeerInfo { node_id, address: old_address.clone() }).await.unwrap();
305+
assert_eq!(peer_store.get_peer(&node_id), Some(PeerInfo { node_id, address: old_address }));
306+
307+
// Re-adding the same peer with a new socket address must refresh the stored entry
308+
// (regression for https://github.com/lightningdevkit/ldk-node/issues/700).
309+
let updated = PeerInfo { node_id, address: new_address.clone() };
310+
peer_store.add_peer(updated.clone()).await.unwrap();
311+
assert_eq!(peer_store.get_peer(&node_id), Some(updated.clone()));
312+
313+
let persisted_bytes = KVStore::read(
314+
&*store,
315+
PEER_INFO_PERSISTENCE_PRIMARY_NAMESPACE,
316+
PEER_INFO_PERSISTENCE_SECONDARY_NAMESPACE,
317+
PEER_INFO_PERSISTENCE_KEY,
318+
)
319+
.await
320+
.unwrap();
321+
let deser_peer_store =
322+
PeerStore::read(&mut &persisted_bytes[..], (Arc::clone(&store), logger)).unwrap();
323+
assert_eq!(deser_peer_store.get_peer(&node_id), Some(updated));
324+
}
325+
326+
#[tokio::test]
327+
async fn peer_same_address_skips_persist() {
328+
let store: Arc<DynStore> = Arc::new(DynStoreWrapper(InMemoryStore::new()));
329+
let logger = Arc::new(TestLogger::new());
330+
let peer_store = PeerStore::new(Arc::clone(&store), Arc::clone(&logger));
331+
332+
let node_id = PublicKey::from_str(
333+
"0276607124ebe6a6c9338517b6f485825b27c2dcc0b9fc2aa6a4c0df91194e5993",
334+
)
335+
.unwrap();
336+
let address = SocketAddress::from_str("127.0.0.1:9738").unwrap();
337+
let peer_info = PeerInfo { node_id, address };
338+
339+
peer_store.add_peer(peer_info.clone()).await.unwrap();
340+
let first_bytes = KVStore::read(
341+
&*store,
342+
PEER_INFO_PERSISTENCE_PRIMARY_NAMESPACE,
343+
PEER_INFO_PERSISTENCE_SECONDARY_NAMESPACE,
344+
PEER_INFO_PERSISTENCE_KEY,
345+
)
346+
.await
347+
.unwrap();
348+
349+
// Identical re-add is a no-op for the store payload.
350+
peer_store.add_peer(peer_info.clone()).await.unwrap();
351+
let second_bytes = KVStore::read(
352+
&*store,
353+
PEER_INFO_PERSISTENCE_PRIMARY_NAMESPACE,
354+
PEER_INFO_PERSISTENCE_SECONDARY_NAMESPACE,
355+
PEER_INFO_PERSISTENCE_KEY,
356+
)
357+
.await
358+
.unwrap();
359+
assert_eq!(first_bytes, second_bytes);
360+
assert_eq!(peer_store.get_peer(&node_id), Some(peer_info));
361+
}
362+
363+
#[tokio::test]
364+
async fn add_peer_does_not_mutate_memory_if_persist_fails() {
365+
let store: Arc<DynStore> = Arc::new(DynStoreWrapper(FailingStore));
366+
let logger = Arc::new(TestLogger::new());
367+
let peer_store = PeerStore::new(store, logger);
368+
369+
let node_id = PublicKey::from_str(
370+
"0276607124ebe6a6c9338517b6f485825b27c2dcc0b9fc2aa6a4c0df91194e5993",
371+
)
372+
.unwrap();
373+
let peer_info =
374+
PeerInfo { node_id, address: SocketAddress::from_str("127.0.0.1:9738").unwrap() };
375+
376+
assert_eq!(Err(Error::PersistenceFailed), peer_store.add_peer(peer_info.clone()).await);
377+
assert_eq!(None, peer_store.get_peer(&node_id));
378+
}
280379
}

0 commit comments

Comments
 (0)