-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
channeldb: add persist nodeannounment config in db #8690
channeldb: add persist nodeannounment config in db #8690
Conversation
In this commit, we save nodeannouncement config in the database so that we will be able to retrieve the config later and use them on retart Signed-off-by: Abdullahi Yunus <[email protected]>
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 Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
In this commit, we check for nodeannouncement alias in the db before creating a disk representation of a node. If there none, we default to first 10 characters of our pubkey.
In this commit, we check for alias that are persisted in disk and use them when the node is restarting.
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 questions and doubts about the architecture
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 for the PR,
I think we want things to be more general and not just for the alias. Ie, for any of the fields you can change via UpdateNodeAnnouncement
type NodeAnnouncementDB struct { | ||
backend kvdb.Backend | ||
} | ||
|
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 already persist our node announcement in the bucket storing all the collected node announcement. Can we not just grab our latest one from there?
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 don't know where the collected node announcements are stored. Are you referring to here where all nodes within a channel graph are stored and their node announcements (if they have any)?.
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.
yes - we store our node announcement there too right? if so, then i dont think we need a whole new bucket for it. Let me know if my assumption is incorrect
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.
@ellemouton I explored the code related to graph DB, and it turns out you're right. We're indeed storing our node along with the node announcement, and we can easily retrieve the stored values from this method when lnd starts.
Since this is the case, I will close this PR and open a new one to replace it. Does that make sense?
Thanks for reviewing @Chinwendu20. I will look at the comments, answer your questions, and address any suggestions you gave me |
In this commit, we ensure new configurations are saved whenever node announcement is updated.
Here we try to determine the alias and color source for our node when starting. The hierarchy is config set in lnd.conf take precedence over persisted config and finally the default.
I started with alias and color for the first iteration. I will be working on the next fields (feature bits, and addresses). Thanks for taking the time to review. |
In this commit, we save the features and addresses assosciated with a node in the database, and use them when the node restarts.
channeldb/db.go
Outdated
// Create a top level bucket which holds information | ||
// about our node announcement. | ||
number: 32, | ||
migration: mig.CreateTLB(nodeAnnouncementBucket), |
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 dont need a migration to add a new bucket.
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 removed it in another commit.
channeldb/db.go
Outdated
@@ -520,9 +526,6 @@ type ChannelStateDB struct { | |||
// linkNodeDB separates all DB operations on LinkNodes. | |||
linkNodeDB *LinkNodeDB | |||
|
|||
// nodeAnnouncementDB seperates all DB operations on NodeAnnouncements. | |||
nodeAnnouncementDb *NodeAnnouncementDB | |||
|
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 commit is removing a bunch of the code added in the previous commit - so it makes it very hard to grok what is actually happening in the PR. Maybe take some time to structure the commits in a more story-telling fashion 🙏
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 for taking the time to review. I will restructure the commits to facilitate the review process.
channeldb/db.go
Outdated
// Create a top level bucket which holds information | ||
// about our node announcement. | ||
number: 32, | ||
migration: mig.CreateTLB(nodeAnnouncementBucket), |
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.
if we remove this here, then no need to add it in the first place :)
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 initially included this, but after @Chinwendu20 's review, I realized it was unnecessary. Therefore, I removed it here.
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.
Good job @Abdulkbk, you can delete the commit then :) You can reach out to me on slack if you need help with it
In this commit, we save the node announcement config in the database so that we can retrieve the config later and use it when a node restarts.
closes #7123
Change Description
Description of change / link to associated issue.
We start by checking the configuration from the configuration file
lnd.conf
or arguments when the node starts. If the configuration is absent, we check for the ones persisted on disk and use them; otherwise, we resolve to the default settings.Steps to Test
Steps for reviewers to follow to test the change.
lnd.conf
updatenodeannouncement
RPC call and set something as your aliaslncli getinfo
lncli getinfo
againPull Request Checklist
Testing
Code Style and Documentation
[skip ci]
in the commit message for small changes.📝 Please see our Contribution Guidelines for further guidance.