graph: fix zombie chan pruning #10340
Conversation
In this commit we update `GetChannelsByPolicyLastUpdateRange` query to return channels that have no policy. This is enables us to prune staled channels that have never received a policy update. To avoid premature deletion of chans, we will add another layer of check to ensure that the chans with no policy are old ones.
Summary of ChangesHello @Abdulkbk, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves an issue where Lightning Network channels that never received an edge policy update were not being properly pruned as 'zombie' channels. The solution introduces a mechanism to determine the staleness of such channels by examining the timestamp of their funding block. This ensures that all inactive channels, regardless of their policy update status, are correctly identified and removed from the graph, leading to a more accurate and efficient network graph representation, specifically for the native SQL backend. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a fix for pruning zombie channels that have no edge policies. The approach of modifying the SQL query to include these channels and then checking their age based on the funding block's timestamp is sound. The new tests for IsZombieByAge and TestChanUpdatesInHorizonWithNoPolicies are well-written and cover the new functionality. However, I've identified a logical bug in pruneZombieChans that could lead to the incorrect pruning of valid channels that have no policies. My review includes a detailed comment and a suggested fix for this issue.
Abdulkbk
force-pushed
the
zombie-chan-pruning
branch
from
8c2bb0b to
f8399e7
Compare
ziggie1984
left a comment
ziggie1984
left a comment
There was a problem hiding this comment.
Cool, I think that works.
| (cp1.last_update >= @start_time AND cp1.last_update < @end_time) | ||
| OR | ||
| (cp2.last_update >= @start_time AND cp2.last_update < @end_time) | ||
| -- TODO(abdulkbk): see the potential of adding a created_at to channel |
There was a problem hiding this comment.
I wonder if we should limit this query since for example couple of nodes will have like >2000 of these cases, should be fine I think just thinking out loud.
There was a problem hiding this comment.
I think it makes sense, especially when this change is run for the first time and the node has many stale channels with no policy. It should be fine for the next ticks (1-hour pruning intervals) tho.
graph/db/graph_test.go
Outdated
| checkIndexTimestamps() | ||
| } | ||
|
|
||
| // TestChanUpdatesInHorizonWithNoPolicies tests that we're able to properly |
There was a problem hiding this comment.
add in the function header here as well that the kv backend does not handle the no-policy correctly or maybe creating a specific file which only runs if the sql tags on the file level are set ?
There was a problem hiding this comment.
let's do separate files, checked with elle.
In this commit we add TestChanUpdatesInHorizonWithNoPolicies to test that we can query channels with no policy attached to it. We also test that we can query channels with policies alongside those without.
In this commit, we add a method to determine if a channel is a zombie using the timestamp of the block of the funding trx that opened the channel.
Abdulkbk
force-pushed
the
zombie-chan-pruning
branch
from
f8399e7 to
c364eae
Compare
|
@ziggie1984: review reminder |
3 participants
Change Description
Fixes #10223
This PR fixes the issue where channels with no edge policies escape pruning even though they have been stale for a while.
The fix only affects native SQL backend see why.
In the PR, we modify the SQL query to return channels without an edge policy, and then we check the timestamp of the block that includes the channel's funding transaction to estimate its age and determine if it should be pruned.
Steps to Test