db: Retrieve peer ID if it exists or create the peer if not #3801
Conversation
This file contains 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
rustyrussell
approved these changes
Jun 29, 2020
We were assuming `wallet_channel_insert` that there cannot be a matching peer if our in-memory representation isn't bound to it (`dbid == 0`). If we then attempt to create the peer, and we already had one it'd cause a unique constraint violation. As far as I can tell this could end up happening if we have an uncommitted channel, and then exited without cleanup (`tal_destructor` on the uncommitted channel not running). This could then leave the peer in the DB. This is because the constraint that every peer has at least one channel is not enforce at DB level, but rather in destructors that may or may not run. Changelog-Fixed: Fixed a failing assertion if we reconnect to a peer that we had a channel with before, and then attempt to insert the peer into the DB twice.
cdecker
force-pushed
the
peer-unique-constraint
branch
from
ed57b5d
to
5782596
Compare
niftynei
approved these changes
Jun 30, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
We were assuming
wallet_channel_insertthat there cannot be a matching peerif our in-memory representation isn't bound to it (
dbid == 0). If we thenattempt to create the peer, and we already had one it'd cause a unique
constraint violation. As far as I can tell this could end up happening if we
have an uncommitted channel, and then exited without cleanup (
tal_destructoron the uncommitted channel not running). This could then leave the peer in the
DB. This is because the constraint that every peer has at least one channel is
not enforce at DB level, but rather in destructors that may or may not run.
I tried to reproduce the issue in pytest, but was unsuccessful, suggesting I don't
fully capture the causes yet.
Notice that an alternative to this would be adding yet another cleanup-on-start
but that feels even more heavy handed, and if implemented wrongly might
itself result in inconsistencies, which is why I went for the load-or-create method.
Changelog-Fixed: Fixed a failing assertion if we reconnect to a peer that we had a channel with before, and then attempt to insert the peer into the DB twice.