autopilot+graph/db: drop unused address loading from cached node iteration#10796
autopilot+graph/db: drop unused address loading from cached node iteration#10796ellemouton wants to merge 3 commits intolightningnetwork:masterfrom
Conversation
ellemouton
force-pushed
the
for-each-node-cached-autopilot
branch
from
3351d43 to
f263c0d
Compare
ForEachNodeCached is now only used for topology-oriented traversal, so the address-loading option forced one autopilot scoring path to bypass the in-memory graph cache for data it did not consume. Remove the withAddrs parameter and the associated SQL/KV address plumbing so cached node iteration can consistently use the graph cache when it is loaded. Autopilot still requires peer addresses before opening channels. That filtering remains in Agent.openChans via ForEachNode, where the selected candidates' addresses are collected for ConnectToPeer. The trade-off is that ForEachNodesChannels no longer excludes addressless nodes from graph-wide scoring inputs such as median channel size or centrality, which also feed lncli getnetworkinfo statistics like graph diameter. In practice the only addressless nodes our local view tends to know about are nodes with no public channels (e.g. our own node or peers we share only private channels with), so the impact on the reported stats should be negligible. Active channel candidates remain address-filtered before dialing.
ellemouton
force-pushed
the
for-each-node-cached-autopilot
branch
from
f263c0d to
49740f7
Compare
github-actions
Bot
added
the
severity-critical
🔴 PR Severity: CRITICAL
🔴 Critical (1 file)
🟠 High (4 files)
🟡 Medium (4 files)
🟢 Low (1 file, excluded from count)
AnalysisThis PR is classified as CRITICAL because it touches The PR appears to be a refactoring/cleanup of graph-related interfaces — the change volumes are modest (9 non-test files, ~168 lines), so no severity bump was triggered. The To override, add a |
ellemouton
force-pushed
the
for-each-node-cached-autopilot
branch
from
49740f7 to
0190889
Compare
Summary of ChangesHello, 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 significantly optimizes Autopilot's graph-wide channel scoring by removing the unnecessary loading of node addresses during graph iteration. By streamlining the Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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 the 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 counterproductive. 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. 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 refactors graph traversal methods to remove the withAddrs parameter, enabling Autopilot's scoring traversal to use the in-memory graph cache and avoid unnecessary database lookups for node addresses. This change improves performance but results in network statistics including nodes without advertised addresses. Review feedback identifies a consistency issue where one implementation of ForEachNodesChannels should be updated to skip nodes with no channels, matching the behavior of the cached implementation.
ForEachNodesChannels is a topology traversal: its callers only need the node identity plus channel edges. After removing address loading from ForEachNodeCached, constructing a Node for this callback is misleading because Addrs is either unused or empty. Pass NodeID directly through the interface and update the scoring and simple graph callers to use that pubkey. This keeps the address-bearing Node interface on ForEachNode, where autopilot gathers connectable candidates and their addresses before dialing.
Note the performance improvement and the observable side-effect on GetNetworkInfo stats so operators are not surprised by small shifts in median channel size / graph diameter for nodes without advertised addresses.
ellemouton
force-pushed
the
for-each-node-cached-autopilot
branch
from
0190889 to
462c78c
Compare
3 participants
Change Description
Autopilot's graph-wide channel-scoring traversal calls
ForEachNodesChannels, which previously askedgraph/dbto load each node's addresses alongside its cached channel data. The scoring code does not consume those addresses — they were only used to skip nodes with no advertised addresses. That bookkeeping had two costs:batchLoadNodeAddressesHelperround-trip to load data that scoring threw away.ChannelGraph.ForEachNodeCachedalways took the slow database path because thewithAddrs=truebranch couldn't be served from the in-memory graph cache.This PR removes the
withAddrsparameter fromForEachNodeCachedso cached node iteration consistently uses the in-memory graph cache when it is loaded, and tightens the autopilot callback signature accordingly:graph/db— dropwithAddrsfromStore.ForEachNodeCachedand the SQL/kvdb implementations; remove the now-unneeded address batch-load on the SQL path.autopilot— changeChannelGraph.ForEachNodesChannelsto hand the callback aNodeID(route.Vertex) instead of aNodeinterface, since it never needed an address-bearing wrapper. The address-bearingNodeinterface is still used byForEachNode, whereAgent.openChansdoes collect addresses forConnectToPeer.docs— release note describing the perf change and a small observable side-effect onlncli getnetworkinfo.Behaviour change worth flagging
ForEachNodesChannelspreviously skipped nodes with zero advertised addresses. After this PR it does not, so graph-wide stats derived from that traversal (median channel size, graph diameter inGetNetworkInfo) now include those nodes. In practice the only addressless nodes our local view knows about are nodes with no public channels — typically our own node or peers we share only private channels with — so the impact on the reported stats should be negligible. Address-based filtering is still applied at the dialing path (Agent.openChans→ForEachNode) before autopilot opens any channel.Steps to Test
go test ./autopilot/... ./graph/db/...make itest icase=graph_migrationto exercise the SQLForEachNodeCachedpath that was updated.lncli getnetworkinfoagainst a populated graph and confirmgraph_diameter,median_channel_size_sat, etc. are returned and within the expected order of magnitude.