Repository navigation
Conversation
open_channel_inner created the channel and only then wrote the peer to the peer store. If that write failed, the call returned PersistenceFailed for a channel that already existed, and a caller retrying would open a second one. Persist the peer first and only then create the channel, removing the peer again if channel creation fails and it was only stored for this attempt. PeerStore::add_peer also inserted the peer into memory before persisting it, so after a failed write the next add_peer saw the peer and skipped the write. Update the in-memory set only once the write succeeds, as remove_peer already does. Fixes lightningdevkit#1143. This change was prepared with the help of an AI assistant (Claude) and reviewed and tested before submission. Co-authored-by: Claude <noreply@anthropic.com>
Inject a failure for the peer-store write on node A and check that open_channel returns PersistenceFailed with no channel created and the peer not persisted. Once the store recovers, a retry opens exactly one channel and persists the peer. This change was prepared with the help of an AI assistant (Claude) and reviewed before submission. Co-authored-by: Claude <noreply@anthropic.com>
|
I've assigned @tnull as a reviewer! |
| let _guard = self.mutation_lock.lock().await; | ||
| let data = { | ||
| let mut locked_peers = self.peers.write().expect("lock"); | ||
| let locked_peers = self.peers.read().expect("lock"); |
There was a problem hiding this comment.
Changes made here overlap with #1150, so whichever lands second will conflict. Could we keep this PR focused on the open_channel_inner ordering instead?
| if !peer_was_stored { | ||
| if let Err(e) = | ||
| self.runtime.block_on(self.peer_store.remove_peer(&peer_info.node_id)) | ||
| { |
There was a problem hiding this comment.
Since get_peer and add_peer are separate steps, a concurrent operation on the same peer could slip in between. For example, if two open_channel calls to the same node overlap and one of them fails, the failing one could remove the peer even though the other one opened a channel. It's rare, but cheap to guard against. Could we also check list_channels_with_counterparty(&node_id) before removing, like remove_peer_after_reconnect does? That way, we never drop a peer we still have a channel with.
3 participants
Fixes #1143.
open_channel_innercreated the channel and only then wrote the peer to the peer store. If that write failed, the API returnedPersistenceFailedfor a channel that was already live, and a caller retrying would open a second channel (and push the amount again).This takes the first option from the issue: the peer is persisted before
create_channel, so a persistence failure is returned before anything is initiated. If channel creation then fails and the peer was only stored for this attempt, it is removed again so we don't keep reconnecting to it.While testing the retry path I hit the cache problem the issue mentions:
PeerStore::add_peerinserted the peer into memory before writing, so after a failed write the nextadd_peerfound it and returnedOkwithout persisting.add_peernow updates the in-memory set only once the write succeeds, asremove_peeralready does. Without this, the retry below would open the channel but leave the peer unpersisted.Tests
peer_store::tests::add_peer_does_not_mutate_memory_if_persist_fails(unit): fails onmain, passes with this change.channel_open_fails_cleanly_when_peer_persistence_fails(integration): fails only the peer-store write on node A and checks thatopen_channelreturnsPersistenceFailedwith no channel and the peer not persisted; then, with the store recovered, a retry opens exactly one channel and persists the peer.cargo test --libpasses (202 tests) andcargo fmtis clean. I could not run the integration test locally (bitcoind/electrs can't be downloaded in my environment), so it is only compile-checked (cargo test --test integration_tests_rust --no-run); CI is authoritative for it.This change was prepared with the help of an AI assistant (Claude); I reviewed the code and ran the tests above.