-
Notifications
You must be signed in to change notification settings - Fork 2k
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
channeldb: add persist nodeannounment config in db #8690
base: master
Are you sure you want to change the base?
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 <abdoollahikbk@gmail.com>
Important Review SkippedAuto reviews are disabled on this repository. 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.
I am not sure this should be channeldb as it seems to not correlate with what is done here, perhaps a separate db?
@guggero @ellemouton what do you think?
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.
channeldb does store everything about other channel node announcements today.
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.
ohh could not find where node announcements were persisted in the code. Just saw that announcements were received and used to create lightningNode objects which were persisted in the graph database but just saw that even the graph database is created by channeldb
In that case maybe we can use the miscDB
seems in line in name for this functionality or create another field for it in the server struct instead of making it a part of the channelStateDB
Want to learn from what you think
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.
ohh could not find where node announcements were persisted in the code. Just saw that announcements were received and used to create lightningNode objects which were persisted in the graph database but just saw that even the graph database is created by channeldb
In that case maybe we can use the
miscDB
seems in line in name for this functionality or create another field for it in the server struct instead of making it a part of thechannelStateDB
Want to learn from what you think
Using the miscDB seems like a good idea since it contains all the other databases within channel db. I used it here in fact to load the persisted config.
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
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.
channeldb does store everything about other channel node announcements today.
channeldb/node.go
Outdated
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?
Thanks for reviewing @Chinwendu20. I will look at the comments, answer your questions, and address any suggestions you gave me |
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.