-
Notifications
You must be signed in to change notification settings - Fork 2.2k
lnd: use persisted node announcement settings across restarts #8825
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
lnd: use persisted node announcement settings across restarts #8825
Conversation
|
Important Review skippedAuto reviews are limited to specific labels. 🏷️ Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@ellemouton, this pull request replaces #8690 as per your suggestion here. Please review when you have time. Thanks |
|
@Abdulkbk - thanks! Although I think you could have just updated the commits in the original PR - no need for a new PR :) |
At first, I thought about doing that, but then I ended up creating a new PR. Thanks for the feedback; I'm really learning a lot. |
|
Hi @alexbosworth, I've opened this PR to fix the issue #7123 you reported. Could you spare some time to review it? Your input will help me improve the solution. |
alexbosworth
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utconcept ack
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work. Left some suggestions. I think the commit message does not follow guideline (i.e. length of the message):
https://github.com/lightningnetwork/lnd/blob/master/docs/code_contribution_guidelines.md#model-git-commit-messages
Here 6ebec6148ef072bd5cddc7, I do not think this is necessary:
'''
lnd: refactor code to elliminate duplicate nill check
lnd: fix comment
'''
Also I think this needs a release note? Maybe add here: https://github.com/lightningnetwork/lnd/blob/7065b6462edf48f3c86a58f2fd215cb0db2c0474/docs/release-notes/release-notes-0.18.1.md
e2a4aa2 to
b1e6571
Compare
Since this PR persists whatever was previously updated with the |
|
I think you can add a bullet point to talk about what the change actually is and maybe a short explanation. Maybe under functional updates |
I just noticed that the issue this PR is addressing is under milestone 0.19.0. Shouldn't the milestone version and release notes version be the same? |
|
I think it makes sense that it should be the same. |
b1e6571 to
5acc286
Compare
|
Hi @yyforyongyu , I noticed that you are one of the code owners of netann. This PR touches |
yyforyongyu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved CI run.
19d2ab7 to
367fbb7
Compare
61d4c75 to
e43254e
Compare
e43254e to
59462a5
Compare
|
Could you help approve the CI, @yyforyongyu? |
59462a5 to
d3fa798
Compare
0c08783 to
26b09ed
Compare
|
I have addressed the feedback @ellemouton. I've moved all the node ann logic into the |
|
!lightninglabs-deploy unmute |
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
The code changes introduce the ability for lnd to retain its previously configured node announcement settings after a restart, making it more consistent. The code is well-structured and the integration tests are thorough.
itest/lnd_channel_graph_test.go
Outdated
| return fmt.Errorf("address %v not found in "+ | ||
| "node announcement", addr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure @gemini-code-asist? I count 77.
ellemouton
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
server.go
Outdated
| return nil, err | ||
| } | ||
|
|
||
| // TODO(elle): All previously persisted node announcement fields (ie, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks - it should ideally be removed in the commit that addressed it. Ie, this commit
| * [nodeannouncement](https://github.com/lightningnetwork/lnd/pull/8825) | ||
| previously configured is now resued when a node restarts, instead of always | ||
| going back to the default configurations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isnt very clear to me tbh. "nodeannouncement previously configured" doesnt really make much sense. How about instead something to the tune of "Use persisted node announcement settings across restarts .. etc"
server.go
Outdated
| selfNode.AuthSigBytes, | ||
| // We now get the self node which represents our node on the channel | ||
| // graph. | ||
| selfNode, err := s.getSelfNode( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get* implies that this purely does a read. I suggest rather naming it something like generateSelfNode/buildSelfNode/setSelfNode to indicate that it is doing some processing given its arguments and also some persistence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will go with setSelfNode
server.go
Outdated
| selfNode.AuthSigBytes, | ||
| // We now get the self node which represents our node on the channel | ||
| // graph. | ||
| selfNode, err := s.getSelfNode( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note that the only thing used from the returned selfNode is the public key bytes which we actually know before ever calling this. So i dont think the new helper needs to return anything
server.go
Outdated
| // getSelfNode returns the self node. It handles setting the node announcement, | ||
| // signing it, and updating the source node in the graph. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the comment should speak to the fact that config values that differ from defaults are prio'd, otherwise pre-existing settings found on disk are prio'd
c4b2f91 to
bcea84d
Compare
Thanks for the review @ellemouton. I am unable to access the patch, but I've addressed your feedback and left a question. |
out of curiosity, what goes wrong with accessing it? you should be able to download it and apply it to commit 655e87c |
ellemouton
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very nice work! pretty much g2g. The only thing i'll add (which is also answering your previous question) is about the address handling. If you take a look at my patch, you can see there that i moved all the address handling into the new helper since this sort of nicely separates all the self-node stuff into a single method.
I was getting a 404, but now it has downloaded. |
3301407 to
b05ce16
Compare
ellemouton
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice work!
|
@yyforyongyu: review reminder |
This commit ensures that we start with the alias, node color, addresses, and features as advertised in the node's previous runtime. This approach maintains consistency in the node's advertised information across restarts.
This commit adds an itest that verify the behaviour of correctly reusing persisted node ann configs across restarts. It also ensures that the node ann configs are applied using the correct hierarchy.
b05ce16 to
fbac730
Compare
yyforyongyu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM👍
| // testSelfNodeAnnouncementPersistence tests that the node announcement configs | ||
| // are persisted correctly and reused when the node is restarted using the | ||
| // correct hierarchy (config > source node > defaults). | ||
| func testSelfNodeAnnouncementPersistence(ht *lntest.HarnessTest) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
| cfg.net.ResolveTCPAddr, | ||
| ) | ||
| nodePubKey := route.NewVertex(nodeKeyDesc.PubKey) | ||
| // Set the self node which represents our node in the graph. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: newline above - for future PRs tho
| ) | ||
| nodePubKey := route.NewVertex(nodeKeyDesc.PubKey) | ||
| // Set the self node which represents our node in the graph. | ||
| err = s.setSelfNode(ctx, nodePubKey, listenAddrs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For future PRs, changes like this should be split into two commits - the first commit is a pure refactor that introduces the setSelfNode method, and the second commit is used to modify the behavior.
| // the source node that are not already in our address list yet. We | ||
| // create this map for quick lookup. | ||
| addressMap := make(map[string]struct{}, len(addrs)) | ||
| // Populate the map with the existing addresses. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: newline above
closes #7123
replaces #8690
Change Description
This change allows a node to retain its previously configured node announcement settings after a restart, making it more consistent.
Issue:
As mentioned in the issue description:
This creates additional work for users; for instance, if you have a list of addresses you associated with your node using the
updatenodeannouncementRPC, you need to set them again each time your node restarts.This also applies to all fields that can be updated using the
updatenodeannouncementRPC, including:alias,color,addresses, andfeature bit.Solution:
This update checks for existing node announcement settings and reuses them during startup.
Since the node announcement settings can be set either through the node's config (specified in
lnd.confor passed as command line args) or using theupdatenodeannouncementRPC, the hierarchy of precedence for these settings is as follows:Steps to Test
peersrpctag:make install tags="peersrpc".Test for precedence:
lnd.conffile, for example,alias=alice.lncli peers updatenodeannouncement -alias=bob.lncli getinfocommand.lncli getinfoagain to confirm that the alias is now set toalice.Test for persistence
lnd.conffile.lncli peers updatenodeannouncement -color=#000000to update the node's color, and verify the change by runninglncli getinfo.lncli getinfoagain to ensure that the color value remains the same after the restart.Alternatively*
Pull Request Checklist
Testing
Code Style and Documentation
[skip ci]in the commit message for small changes.📝 Please see our Contribution Guidelines for further guidance.