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! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.