Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't delete channels once they are resolved, just mark them closed. #2768

Merged
merged 3 commits into from Jun 30, 2019

Conversation

Projects
None yet
2 participants
@cdecker
Copy link
Member

commented Jun 22, 2019

Since we are starting to have some tables that reference the channels table using foreign keys and these survive the channel I think it's a good idea to keep a minimal version of the channels around. This ensures that foreign key relationships aren't broken once we fully resolve a channel, including access to channel-related fields, and it can allow us to recover some information about old channels for debugging or display purposes.

Fixes #2028

cdecker added some commits Jun 22, 2019

wallet: Add a final `CLOSED` state to channels
Instead of deleting the channels we will simple mark them as `CLOSED` from now
on. This is needed for some of the other tables not to end up with dangling
references that would otherwise survive the channel lifetime, e.g., forwards
and transactions.

Signed-off-by: Christian Decker <decker.christian@gmail.com>
wallet: Don't delete channels from DB, mark them closed.
Since we now have a couple of long-lived dependents it is time we stop
removing channels from the table once they are fully closed, and instead just
mark them as closed. This allows us to keep forwards and transactions foreign
keys intact, and it may help us debug things after the fact.

Fixes #2028

Signed-off-by: Christian Decker <decker.christian@gmail.com>

@cdecker cdecker requested a review from rustyrussell Jun 22, 2019

@cdecker

This comment has been minimized.

Copy link
Member Author

commented Jun 22, 2019

For reference #2766 was dealing with some channel-related fields becoming unavailable.

I'm also not adding a foreign key constraint to the forwarded_payments table at this time, since that might break with older wallets that have forwards pointing to non-existent entries. We can add them later or just run with it like it is now.

@rustyrussell rustyrussell added this to the 0.7.2 milestone Jun 23, 2019

@rustyrussell
Copy link
Contributor

left a comment

Ack e420c94

"SET state=?, peer_id=?"
"WHERE channels.id=?");
sqlite3_bind_int64(stmt, 1, CLOSED);
sqlite3_bind_null(stmt, 2);

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Jun 24, 2019

Contributor

Might be nice to keep peer_id around, but that's a separate change. Then we'd have some historic information about who our peer was.

This comment has been minimized.

Copy link
@cdecker

cdecker Jun 24, 2019

Author Member

peer_id is a foreign key into the peers table, so we'd need to keep the peer around in that table, which then conflicts with the unique constraint when inserting on channel open. We could either stub the peer out (dropping unique fields or replacing them with uniquified versions), or we could query for an existing entry first and insert only if it doesn't exist yet.

@rustyrussell rustyrussell merged commit fa1c8b3 into ElementsProject:master Jun 30, 2019

2 checks passed

ackbot PR ack'd by rustyrussell
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.