-
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
base: master
Are you sure you want to change the base?
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. |
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 |
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
10c860f
to
472a245
Compare
Hmmm can't seem to figure out why the entire itests keep failing, will need to dig in a little deeper... |
Carol fails to start back up:
With the following error in Carol's log:
|
472a245
to
d54a487
Compare
Thanks for pointing that out. This made me realize I could download and view a particular node's logs from the CI, very helpful. I was initially updating the feature manager with the feature sets that were persisted in the previous run, something like: prevFeatures := map[feature.Set]*lnwire.RawFeatureVector{
feature.SetNodeAnn: sourceNode.Features.RawFeatureVector,
}
err = s.featureMgr.UpdateFeatureSets(prevFeatures)
if err != nil {
srvrLog.Errorf("unable to update feature sets: %v", err)
return err
} This ensures that when you add an unknown feature bit (2, for example) before restarting, you get to see it in ...
"2": {
"name": "unknown",
"is_required": true,
"is_known": false
},
... The issue arises when we start a node carol in this case with |
@saubyk could you add this to the review board? |
4d17282
to
89cb0bd
Compare
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! Will be great to get this in as it is closely tied to #9999 (where we also will need to start fetching our SourceNode from the DB on startup
server.go
Outdated
srvrLog.Debugf("Unable to get source node from graphDB to "+ | ||
"construct initial node announcement: %v", err) | ||
|
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.
we should distinguish between if this error is ErrSourceNodeNotSet
, in which case we dont need to log an error, or is it some other error in which case we should be erroring out.
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.
Done
Alias: "alice", | ||
Color: "#eeeeee", | ||
AddressUpdates: []*peersrpc.UpdateAddressAction{ | ||
{ |
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.
it would be good to test that we can still remove an address too
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.
Now testing for this behaviour
server.go
Outdated
func getNodeAnnouncementFields(ctx context.Context, cfg *Config, s *server, | ||
serializedPubKey [33]byte) (color.RGBA, string, []net.Addr, error) { |
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.
rather make a method on server
ie:
func (s *server) getNodeAnnFields(ctx context.Context, serializedPubKey route.Vertex) ...
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, looks cleaner!
server.go
Outdated
srvrLog.Errorf("unable to parse color: %v\n", err) | ||
|
||
return color, "", nil, err |
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.
rather just return a decorated error - it will get logged higher up
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.
Now returning a decorated err
server.go
Outdated
alias := cfg.Alias | ||
if alias == "" { | ||
alias = hex.EncodeToString(serializedPubKey[:10]) | ||
// Create a map to track existing addresses for quick lookup. This is |
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.
can probs move this logic into the getNodeAnnFields method and just pass it selfNodes
and then used the returned addresses
as the new selfNodes
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.
You mean selfAddrs
right? Done
89cb0bd
to
b3542ec
Compare
Thanks for the review @ellemouton I've addressed the feedback you left. |
I noticed some of the itests are failing particulary with this error from here [ERR] LTND lnd.go:169: Shutting down due to error in main method err="unable to fetch source node: unable to fetch V1 source node: source node does not exist" will investigate more... |
cool - it has been merged now @Abdulkbk |
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.
b3542ec
to
aa2d6f9
Compare
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
updatenodeannouncement
RPC, you need to set them again each time your node restarts.This also applies to all fields that can be updated using the
updatenodeannouncement
RPC, 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.conf
or passed as command line args) or using theupdatenodeannouncement
RPC, the hierarchy of precedence for these settings is as follows:Steps to Test
peersrpc
tag:make install tags="peersrpc"
.Test for precedence:
lnd.conf
file, for example,alias=alice
.lncli peers updatenodeannouncement -alias=bob
.lncli getinfo
command.lncli getinfo
again to confirm that the alias is now set toalice
.Test for persistence
lnd.conf
file.lncli peers updatenodeannouncement -color=#000000
to update the node's color, and verify the change by runninglncli getinfo
.lncli getinfo
again 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.