lnwire: add missing Gossip 1.75 fields and message#10232
lnwire: add missing Gossip 1.75 fields and message#10232ellemouton merged 7 commits intolightningnetwork:masterfrom
Conversation
Summary of ChangesHello @ellemouton, 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 lays foundational work for the Gossip 1.75 protocol by updating existing message definitions and introducing new ones. It primarily focuses on enhancing the Highlights
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 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 counter productive. 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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](proxy.php?url=https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This PR updates the gossip protocol to version 1.75. The changes include renaming NodeAnnouncement to NodeAnnouncement1, adding an outpoint field to ChannelAnnouncement2, and defining the new NodeAnnouncement2 message. The implementation looks solid and includes relevant test updates.
I've found a few minor issues:
- An incorrect static size for the
OutPointTLV record. - A typo in an error message in the Tor address encoder for
NodeAnnouncement2. - Several new unexported functions are missing comments, which is required by the style guide. I've pointed out a few examples.
Overall, great work on moving the gossip implementation forward.
ellemouton
force-pushed
the
g175MessageFollowup
branch
2 times, most recently
from
3d6fdbd to
fc770dd
Compare
ellemouton
force-pushed
the
g175MessageFollowup
branch
from
fc770dd to
019dc79
Compare
yyforyongyu
left a comment
yyforyongyu
left a comment
There was a problem hiding this comment.
Pretty close - left a few comments about some improvements we can make.
ellemouton
force-pushed
the
g175MessageFollowup
branch
from
019dc79 to
3676d59
Compare
ellemouton
left a comment
ellemouton
left a comment
There was a problem hiding this comment.
thanks @yyforyongyu 🙏
ellemouton
force-pushed
the
g175MessageFollowup
branch
2 times, most recently
from
72d41e1 to
1ccb590
Compare
bitromortac
left a comment
bitromortac
left a comment
There was a problem hiding this comment.
Very cool to see the work on v2 ⚡.
yyforyongyu
left a comment
yyforyongyu
left a comment
There was a problem hiding this comment.
Pending @bitromortac 's comments, otherwise LGTM!
Add a new OutPoint type that wraps wire.OutPoint and provides TLV encoding/decoding capabilities through the tlv.RecordProducer interface. This enables OutPoint to be used in TLV streams of messages.
The latest version of the spec has the outpoint included in the `channel_announcement2` message.
In preparation for adding a NodeAnnouncement2 struct along with a NodeAnnouncement interface, this commit renames the existing NodeAnnouncment struct to NodeAnnouncement1.
We leave a TODO that should be addressed after a discussion at the spec meeting. For now, having the incorrect TLV type is not a problem since this ChannelUpdate2 type is not used in production.
In preparation for using this type as a TLV record, we let it implement the RecordProducer interface.
ellemouton
force-pushed
the
g175MessageFollowup
branch
from
1ccb590 to
c9b8085
Compare
ellemouton
left a comment
ellemouton
left a comment
There was a problem hiding this comment.
Thanks @bitromortac 🙏
ellemouton
force-pushed
the
g175MessageFollowup
branch
from
c9b8085 to
16f5f48
Compare
bitromortac
left a comment
bitromortac
left a comment
There was a problem hiding this comment.
LGTM (with final nit), really great commits and tests 🎉
In this commit, the lnwire.NodeAnnouncement2 type is defined. This will be used to represent the `node_announcement_2` message used in the Gossip 2 (1.75) protocol.
ellemouton
force-pushed
the
g175MessageFollowup
branch
from
16f5f48 to
b8ff9fb
Compare
4 participants
This PR does the following main things:
outpointfield to theChannelAnnouncement2message.NodeAnnouncement2message which will be used to represent the newnode_announcement_1message.This is part of the gossip 1.75 project. None of these types are used in the projection code yet.