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 see @tnull was un-assigned. |
| 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?
There was a problem hiding this comment.
@Camillarhi agreed, and sorry for the slow turnaround. I've narrowed the PR to the open_channel_inner ordering:
- The
PeerStore::add_peerchange and its unit test are gone.src/peer_store.rsis back to main, and that part is left to Only publish added peers after they were persisted #1150. - I merged main and resolved the conflicts with the fee-rate override that landed since. The peer is now persisted before the override is registered, so a failed peer write can't leave a pending override behind.
- The integration test is updated for the new
open_channelsignature. I dropped its check that the peer isn't persisted after the failed write, since that depends on Only publish added peers after they were persisted #1150. It still checks that no channel is created and that a retry opens exactly one.
Against main the diff is now src/lib.rs (+19/−1) plus the test. cargo test --lib passes (377), cargo fmt --check is clean and the integration test compiles. I couldn't run it here because bitcoind/electrs aren't available in my environment.
| 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.
Brings src/lib.rs up to date with main (79e4d92) and keeps only the open_channel_inner change: the peer is persisted before create_channel, and removed again if channel creation fails and it was only stored for this attempt. The peer is now persisted before the fee-rate override is registered, so a failed peer write leaves no pending override behind. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
src/peer_store.rs now matches main. Making add_peer publish to memory only after a successful write is what lightningdevkit#1150 does, so this PR no longer touches it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Brings the test file up to date with main and adapts the test to the current open_channel signature (channel_config argument). The assertion that the peer is not persisted after the failed write is dropped, since that depends on the add_peer change now left to lightningdevkit#1150; the test still checks that no channel is created and that a retry opens exactly one. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Feel free to rebase this on #1234, or wait until that landed. |
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.