Skip to content
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

Signal GossipQuery support when using IgnoringMessagHandler #2959

Conversation

tnull
Copy link
Contributor

@tnull tnull commented Mar 21, 2024

With its v24.02 release CLN made GossipQueries a required feature (see ElementsProject/lightning#6864 and https://github.com/ElementsProject/lightning/blob/master/CHANGELOG.md#2402---2024-02-27-uint-needs-signature), leading to a incompatibility between LDK and CLN when using IgnoringMessagHandler as a RoutingMessageHandler, which is usually the case when a node uses RGS.

To fix this issue, we let IgnoringMessagHandler signal GossipQuery support, just to go ahead and ignore every gossip message the peer will send us. While this is nonsensical and still might result in some unnecessary bandwidth wasted, we have to do something to fix the incompatibility.

With its v24.02 release CLN made `GossipQueries` a required feature,
leading to a incompatibility between LDK and CLN when using
`IgnoringMessagHandler` as a `RoutingMessageHandler`, which is usually
the case when a node uses RGS.

To fix this issue, we let `IgnoringMessagHandler` signal `GossipQuery`
support, just to go ahead and ignore every gossip message the peer will
send us. While this is nonsensical and still might result in some
unnecessary bandwidth wasted, we have to do something to fix the
incompatibility.
@tnull tnull force-pushed the 2024-03-support-gossip-queries-in-ignoring-handler branch from 770f5bf to 843079d Compare March 21, 2024 08:05
@TheBlueMatt TheBlueMatt added this to the 0.0.122 milestone Mar 21, 2024
@G8XSU
Copy link
Contributor

G8XSU commented Mar 21, 2024

Following,
Is there an issue open on CLN for this?

@tnull
Copy link
Contributor Author

tnull commented Mar 21, 2024

Following, Is there an issue open on CLN for this?

No, but I left a comment on the corresponding BOLTs PR: lightning/bolts#1092 (comment)

@tnull
Copy link
Contributor Author

tnull commented Mar 26, 2024

@TheBlueMatt Even though the feature will be repurposed now and CLN reverted requiring it, I still wonder if we should still land this in 0.0.122 but revert it in 0.0.123 or 0.0.124 to fix the compatibility issues with current CLN versions in the meantime?

Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Yea, doing this for a release then reverting it makes sense to me. It looks like CLN will do a point release for this so hopefully it all resolves quickly.

@G8XSU G8XSU merged commit 68d5e88 into lightningdevkit:main Mar 26, 2024
13 of 16 checks passed
@tnull
Copy link
Contributor Author

tnull commented Mar 27, 2024

LGTM. Yea, doing this for a release then reverting it makes sense to me. It looks like CLN will do a point release for this so hopefully it all resolves quickly.

Now tracking in #2972 so we don't forget.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants