-
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
graph/db: correctly handle de(ser)ialisation of models.LightningNode
opaque addresses
#9474
graph/db: correctly handle de(ser)ialisation of models.LightningNode
opaque addresses
#9474
Conversation
This test started out demonstrating a bug. But that bug has since been fixed. Fix the comment to reflect.
And use it in the gossiper. This helps ensure that we do this conversion consistently.
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
4e38424
to
58d1890
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.
Thank you for this quick fix, LGTM (only question regarding migrations)
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 fix! I guess #9455 builds on top of this PR?
return nil, err | ||
} | ||
|
||
address = &lnwire.OpaqueAddrs{ |
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.
Feels weird to have this in the lnwire
package, while lnwire
stores all the messages defined in BOLTs, not sub fields, guess it's pre-existing. Also didn't know we support more addr types in addition to the five defined in bolt7.
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's a bit of a strange one but i actually think it might make sense to keep it in lnwire
given that currently, this is what the node_announcement
address field is:
[u16:addrlen]
[addrlen*byte:addresses]
and so then this addresses
field includes any of those 5 types along with any extras that we dont know yet. So to me it's like lnwire.ExtraOpaqueData
but specifically for address fields.
Also didn't know we support more addr types in addition to the five defined in bolt7.
We (as in LND) don't. but we should: SHOULD ignore the first address descriptor that does NOT match the types defined above.
ie, we should still persist & propagate these. Here is an example of CLN adding new Websocket addresses as type 6.
In this commit, we fix the bug demonstrated in the prior commit. We correctly handle the persistence of lnwire.OpaqueAddrs.
58d1890
to
16e2a48
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.
LGTM🚢
Fixes #9473
In #6435, we started to correctly handle the serialisation/deserialisation of unknown addresses in node announcement wire messages but we were still not persisting these node announcements correctly.
In this PR, the bug is first demonstrated and then fixed by properly handing the
OpaqueAddrs
case when persisting themodels.LightningNode
type.