-
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
discovery: fix bug that can lead to sending invalid chan_ann msgs #9002
discovery: fix bug that can lead to sending invalid chan_ann msgs #9002
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 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 Configuration File (
|
Initially in lnd, we didn't store the extra TLV data that could be dangling off of gossip messages. This was fixed initially in lnd v0.5 with this PR: lightningnetwork#1825. Within the PR, we incorrect set the `ExtraOpaqueData` (extra TLV blob) of the `ChannelAnnouncement` to the value stored in `edge`, which is actually our channel update. As 6-ish years ago we didn't yet have anything that used the TLV gossip fields, this went unnoticed. Fast forward to 2024, we shipped an experimental version of inbounbd fees. This starts to store additional data in the `ExtraOpaqueData` field, the TLV for the inbound fee. Initially, everything is valid when the first `ChannelAnnouncement` is sent, but as soon as a user attempts to set an inbound fee policy, we'd incorrectly swap in that new serialized TLV for the _channel announcement_: lightningnetwork@841e243#diff-1eda595bbebe495bd74a6a0431c46b66cb4e8b53beb311067c010feac2665dcbR2560. Since we're just trying to generate a new `channel_update`, we don't also regenerate the signature for the `channel_announcement` message. As a result, we end up storing a `channel_announcement` with an invalid sig on disk, continuing to broadcast that to peers.
be6640c
to
f7daa30
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 🎉
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🙏 Wonder what happens to the existing nodes - I guess a restart should be enough to fix the issue for them.
@@ -60,6 +60,10 @@ commitment when the channel was force closed. | |||
* We'll now always send [channel updates to our remote peer for open | |||
channels](https://github.com/lightningnetwork/lnd/pull/8963). | |||
|
|||
* [A bug has been fixed that could cause invalid channel |
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.
Cool think we need to do a new rc?
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.
Yeah, I think we have another PR or two incoming to trigger a new rc.
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 🌮
Reading the code diff again, we don't update the chan ann on disk to be invalid, just when we were making the wire message, we swapped in the wrong TLV data. So the chan ann's on disk are valid, and now with this PR as soon as we go to update the chan upds (zombie prevention or |
Initially in lnd, we didn't store the extra TLV data that could be dangling off of gossip messages. This was fixed initially in lnd v0.5 with this PR: #1825.
Within the PR, we incorrectly set the
ExtraOpaqueData
(extra TLV blob) of theChannelAnnouncement
to the value stored inedge
, which is actually our channel update. As 6-ish years ago we didn't yet have anything that used the TLV gossip fields, this went unnoticed.Fast forward to 2024, we shipped an experimental version of inbound fee discounts. This starts to store additional data in the
ExtraOpaqueData
field: the TLV for the inbound fee discount. Initially, everything is valid when the firstChannelAnnouncement
is sent, but as soon as a user attempts to set an inbound fee policy, we'd incorrectly swap in that new serialized TLV for the channel announcement:841e243#diff-1eda595bbebe495bd74a6a0431c46b66cb4e8b53beb311067c010feac2665dcbR2560.
Since we're just trying to generate a new
channel_update
, we don't also regenerate the signature for thechannel_announcement
message. As a result, we end up storing achannel_announcement
with an invalid sig on disk, continuing to broadcast that to peers.Fixes #9000