Fetch InitFeatures from both Channel and Routing Message Handlers - #1701

Merged
TheBlueMatt merged 4 commits into
lightningdevkit:mainfrom
TheBlueMatt:2022-09-feature-or
Sep 9, 2022
Merged

Fetch InitFeatures from both Channel and Routing Message Handlers#1701
TheBlueMatt merged 4 commits into
lightningdevkit:mainfrom
TheBlueMatt:2022-09-feature-or

Conversation

@TheBlueMatt

@TheBlueMattTheBlueMatt commented Sep 7, 2022

Copy link
Copy Markdown
Collaborator

Based on #1699, this fetches the InitFeatures from both Channel and Routing Message Handlers and OR's them toegether. It then moves the relevant feature flags to our ChannelManager and P2PGossipHandlers.

It should tee us up nicely for #1688 to set the onion message features in the messenger itself.

jkczyz
jkczyz reviewed Sep 7, 2022
View reviewed changes
Comment threadlightning/src/ln/peer_handler.rs Outdated
jkczyz
jkczyz reviewed Sep 7, 2022
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs Outdated
Comment threadlightning/src/ln/features.rs Outdated
Comment threadlightning/src/routing/gossip.rs

fn provided_init_features(&self, _their_node_id: &PublicKey) -> InitFeatures {
let mut features = InitFeatures::empty();
features.set_gossip_queries_optional();

@jkczyzjkczyzSep 7, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should this also set initial_routing_sync since that is cleared in known_channel_features? Maybe it would be better to define known_channel_features as the set difference of InitFeatures::known with P2PGossipSync::provided_init_features (as a const somewhere). Then what is described in the comment on InitContext's optional features wouldn't be necessary.

@TheBlueMattTheBlueMattSep 7, 2022

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

No, the fact that we were previously setting initial_routing_sync was an oversight, basically. If our peer supports gossip sync they'll ignore it, and we shouldnt be setting it on every connection anyway as it'll waste bandwidth.

@jkczyzjkczyzSep 7, 2022•
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we call this out in the commit message and changelog since it is a behavioral change? Will / should we ever set initial_routing_sync now?

@TheBlueMattTheBlueMattSep 8, 2022

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Sure added a note. We will no longer ever set it, no.

@jkczyzjkczyzSep 8, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Any way we could still do something with set difference, though? Would need to special case removing initial_routing_sync, but I guess that is an unordinary feature anyhow.

@TheBlueMattTheBlueMattSep 8, 2022

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

I'm not sure what a set difference here gets us? If anything I'd be inclined to have the ChannelManager explicitly set all the features it wants, rather than having that logic in features.rs itself, I only didn't bother because it'd be very verbose.

I see the purpose of this change, ultimately as moving away from defining an LDK-global "known features" set, which was always a little awkward, and instead defining the "known features" set in each module that actually provides said features.

@jkczyzjkczyzSep 8, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Discussed offline. My primary concern was to avoid relying on a comment to know when to update known_channel_features. Possible alternative could be to have a unit test asserting that the intersection of ChannelManager and P2PGossipSync's provided features is empty.

@TheBlueMattTheBlueMattSep 9, 2022

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

See #1707

@TheBlueMatt
TheBlueMattforce-pushed the 2022-09-feature-or branch 2 times, most recently from eced794 to ff3c0dcCompareSeptember 7, 2022 22:08
jkczyz
jkczyz reviewed Sep 7, 2022
View reviewed changes
Comment threadlightning/src/routing/gossip.rs

fn provided_init_features(&self, _their_node_id: &PublicKey) -> InitFeatures {
let mut features = InitFeatures::empty();
features.set_gossip_queries_optional();

@jkczyzjkczyzSep 7, 2022•
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we call this out in the commit message and changelog since it is a behavioral change? Will / should we ever set initial_routing_sync now?

Comment threadlightning/src/ln/peer_handler.rs Outdated
@TheBlueMatt

TheBlueMatt commented Sep 8, 2022

Copy link
Copy Markdown
CollaboratorAuthor

Rebased on #1699.

@codecov-commenter

codecov-commenter commented Sep 8, 2022•
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #1701 (ba69536) into main (ba69536) will not change coverage.
The diff coverage is n/a.

❗ Current head ba69536 differs from pull request most recent head 1b67b0b. Consider uploading reports for the commit 1b67b0b to get more accurate results

@@ Coverage Diff @@## main #1701 +/- ##
=======================================
Coverage 90.87% 90.87% =======================================
Files 86 86 Lines 46378 46378 Branches 46378 46378 =======================================
Hits 42146 42146 Misses 4232 4232

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

jkczyz
jkczyz reviewed Sep 8, 2022
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs Outdated
/// Panics if `addresses` is absurdly large (more than 100).
///
/// [`get_and_clear_pending_msg_events`]: MessageSendEventsProvider::get_and_clear_pending_msg_events
pub fn broadcast_node_announcement(&self, rgb: [u8; 3], alias: [u8; 32], mut addresses: Vec<NetAddress>) {

@jkczyzjkczyzSep 8, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Unused import for NetAddress.

@TheBlueMattTheBlueMattSep 9, 2022

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Oops, shit, this was from the previous PR. We have another warning introduced recently, I'll fix both in a followup.

Comment threadlightning/src/ln/msgs.rs Outdated
Comment threadlightning/src/ln/msgs.rs Outdated
jkczyz
jkczyz previously approved these changes Sep 9, 2022
View reviewed changes

@jkczyzjkczyz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. Feel free to squash.

@valentinewallace

valentinewallace commented Sep 9, 2022

Copy link
Copy Markdown
Contributor

LGTM after squash

@TheBlueMatt
Fetch our InitFeatures from ChannelMessageHandler
950ccc4
Like we now do for `NodeFeatures`, this converts to asking our
registered `ChannelMessageHandler` for our `InitFeatures` instead
of hard-coding them to the global LDK known set.
This allows handlers to set different feature bits based on what
our configuration actually supports rather than what LDK supports
in aggregate.
@TheBlueMatt
OR InitFeatures from both Channel and Routing message handlers
06cb48a
When we go to send an Init message to new peers, the features we
support are really a combination of all the various features our
different handlers support. This commit captures this concept by
OR'ing our InitFeatures across both our Channel and Routing
handlers.
Note that this also disables setting the `initial_routing_sync`
flag in init messages, as was intended in
e742894, per the comment added on
`clear_initial_routing_sync`, though this should not be a behavior
change in practice as nodes which support gossip queries ignore the
initial routing sync flag.
@TheBlueMatt
Add a new InitFeatures constructor to capture the types of flags
e34a2bc
When `ChannelMessageHandler` implementations wish to return an
`InitFeatures` which contain all the known flags that are relevant
to channel handling, but not gossip handling, they currently need
to do so by manually constructing an InitFeatures with all known
flags and then clearing the ones they dont want.
Instead of spreading this logic out across the codebase, this
consolidates such construction to one place in features.rs.
@TheBlueMatt
Make clear_initial_routing_sync more consistent with other APIs
1b67b0b
@TheBlueMatt

TheBlueMatt commented Sep 9, 2022

Copy link
Copy Markdown
CollaboratorAuthor

Squashed without further change.

@TheBlueMatt
TheBlueMatt merged commit 3fb3218 into lightningdevkit:mainSep 9, 2022
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jkczyzjkczyzjkczyz approved these changes
@valentinewallacevalentinewallacevalentinewallace approved these changes

Assignees

No one assigned

Labels

blocked on dependent pr

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@TheBlueMatt@codecov-commenter@valentinewallace@jkczyz
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all 
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
Fetch InitFeatures from both Channel and Routing Message Handlers - #1701
Merged
TheBlueMatt merged 4 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2022-09-feature-or
Sep 9, 2022
Merged
Fetch InitFeatures from both Channel and Routing Message Handlers#1701
TheBlueMatt merged 4 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2022-09-feature-or
Conversation
@TheBlueMatt
@TheBlueMattTheBlueMatt
commented
Sep 7, 2022

Copy link


Copy Markdown

Collaborator
Based on #1699, this fetches the InitFeatures from both Channel and Routing Message Handlers and OR's them toegether. It then moves the relevant feature flags to our ChannelManager and P2PGossipHandlers.
It should tee us up nicely for #1688 to set the onion message features in the messenger itself.
jkczyz
jkczyz
reviewed
Sep 7, 2022
View reviewed changes
Comment threadlightning/src/ln/peer_handler.rs
Outdated

jkczyz
jkczyz
reviewed
Sep 7, 2022
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/features.rs
Outdated

Comment threadlightning/src/routing/gossip.rs

	fn provided_init_features(&self, _their_node_id: &PublicKey) -> InitFeatures {
 let mut features = InitFeatures::empty();
 features.set_gossip_queries_optional();
@jkczyzjkczyzSep 7, 2022

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Should this also set initial_routing_sync since that is cleared in known_channel_features? Maybe it would be better to define known_channel_features as the set difference of InitFeatures::known with P2PGossipSync::provided_init_features (as a const somewhere). Then what is described in the comment on InitContext's optional features wouldn't be necessary.
@TheBlueMattTheBlueMattSep 7, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

No, the fact that we were previously setting initial_routing_sync was an oversight, basically. If our peer supports gossip sync they'll ignore it, and we shouldnt be setting it on every connection anyway as it'll waste bandwidth.
@jkczyzjkczyzSep 7, 2022•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Should we call this out in the commit message and changelog since it is a behavioral change? Will / should we ever set initial_routing_sync now?
@TheBlueMattTheBlueMattSep 8, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

Sure added a note. We will no longer ever set it, no.
@jkczyzjkczyzSep 8, 2022

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Any way we could still do something with set difference, though? Would need to special case removing initial_routing_sync, but I guess that is an unordinary feature anyhow.
@TheBlueMattTheBlueMattSep 8, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

I'm not sure what a set difference here gets us? If anything I'd be inclined to have the ChannelManager explicitly set all the features it wants, rather than having that logic in features.rs itself, I only didn't bother because it'd be very verbose.
I see the purpose of this change, ultimately as moving away from defining an LDK-global "known features" set, which was always a little awkward, and instead defining the "known features" set in each module that actually provides said features.
@jkczyzjkczyzSep 8, 2022

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Discussed offline. My primary concern was to avoid relying on a comment to know when to update known_channel_features. Possible alternative could be to have a unit test asserting that the intersection of ChannelManager and P2PGossipSync's provided features is empty.
@TheBlueMattTheBlueMattSep 9, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

See #1707
@TheBlueMatt
TheBlueMattforce-pushed
the
2022-09-feature-or
branch
2 times, most recently
from
eced794 to
ff3c0dcCompareSeptember 7, 2022 22:08
jkczyz
jkczyz
reviewed
Sep 7, 2022
View reviewed changes
Comment threadlightning/src/routing/gossip.rs

	fn provided_init_features(&self, _their_node_id: &PublicKey) -> InitFeatures {
 let mut features = InitFeatures::empty();
 features.set_gossip_queries_optional();
@jkczyzjkczyzSep 7, 2022•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Should we call this out in the commit message and changelog since it is a behavioral change? Will / should we ever set initial_routing_sync now?
Comment threadlightning/src/ln/peer_handler.rs
Outdated

@TheBlueMatt
TheBlueMatt
commented
Sep 8, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Rebased on #1699.
@codecov-commenter
codecov-commenter
commented
Sep 8, 2022•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #1701 (ba69536) into main (ba69536) will not change coverage.

The diff coverage is n/a.
❗ Current head ba69536 differs from pull request most recent head 1b67b0b. Consider uploading reports for the commit 1b67b0b to get more accurate results
@@ Coverage Diff @@## main #1701 +/- ##
=======================================
Coverage 90.87% 90.87% =======================================
Files 86 86 Lines 46378 46378 Branches 46378 46378 =======================================
Hits 42146 42146 Misses 4232 4232 
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
jkczyz
jkczyz
reviewed
Sep 8, 2022
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Outdated

	/// Panics if `addresses` is absurdly large (more than 100).
	///
	/// [`get_and_clear_pending_msg_events`]: MessageSendEventsProvider::get_and_clear_pending_msg_events
	pub fn broadcast_node_announcement(&self, rgb: [u8; 3], alias: [u8; 32], mut addresses: Vec<NetAddress>) {
@jkczyzjkczyzSep 8, 2022

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Unused import for NetAddress.
@TheBlueMattTheBlueMattSep 9, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

Oops, shit, this was from the previous PR. We have another warning introduced recently, I'll fix both in a followup.
Comment threadlightning/src/ln/msgs.rs
Outdated

Comment threadlightning/src/ln/msgs.rs
Outdated

jkczyz
jkczyz
previously approved these changes
Sep 9, 2022
View reviewed changes
@jkczyzjkczyz
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM. Feel free to squash.
@valentinewallace
valentinewallace
commented
Sep 9, 2022

Copy link


Copy Markdown

Contributor
LGTM after squash
@TheBlueMatt
Fetch our InitFeatures from ChannelMessageHandler
950ccc4
Like we now do for `NodeFeatures`, this converts to asking our
registered `ChannelMessageHandler` for our `InitFeatures` instead
of hard-coding them to the global LDK known set.
This allows handlers to set different feature bits based on what
our configuration actually supports rather than what LDK supports
in aggregate.
@TheBlueMatt
OR InitFeatures from both Channel and Routing message handlers
06cb48a
When we go to send an Init message to new peers, the features we
support are really a combination of all the various features our
different handlers support. This commit captures this concept by
OR'ing our InitFeatures across both our Channel and Routing
handlers.
Note that this also disables setting the `initial_routing_sync`
flag in init messages, as was intended in
e742894, per the comment added on
`clear_initial_routing_sync`, though this should not be a behavior
change in practice as nodes which support gossip queries ignore the
initial routing sync flag.
@TheBlueMatt
Add a new InitFeatures constructor to capture the types of flags
e34a2bc
When `ChannelMessageHandler` implementations wish to return an
`InitFeatures` which contain all the known flags that are relevant
to channel handling, but not gossip handling, they currently need
to do so by manually constructing an InitFeatures with all known
flags and then clearing the ones they dont want.
Instead of spreading this logic out across the codebase, this
consolidates such construction to one place in features.rs.
@TheBlueMatt
Make clear_initial_routing_sync more consistent with other APIs
1b67b0b
@TheBlueMatt
TheBlueMatt
commented
Sep 9, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Squashed without further change.
@TheBlueMatt
TheBlueMatt
merged commit 3fb3218
into
lightningdevkit:mainSep 9, 2022
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@jkczyzjkczyzjkczyz approved these changes
@valentinewallacevalentinewallacevalentinewallace approved these changes

Assignees


No one assigned


Labels

blocked on dependent pr

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@TheBlueMatt@codecov-commenter@valentinewallace@jkczyz

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Force GitHub README to respect dark mode
(function() {
var style = document.createElement('style');
style.textContent = '
.markdown-body {
color-scheme: dark light;
}
.markdown-body pre { background: #161b22 !important; }
.markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }
.markdown-body table th, .markdown-body table td { border-color: #30363d !important; }
.markdown-body img { background: #0d1117; }
.markdown-body blockquote { border-left-color: #8b949e; }
.markdown-body hr { border-color: #30363d; }
';
document.head.appendChild(style);
})();
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Fetch InitFeatures from both Channel and Routing Message Handlers - #1701
Merged
TheBlueMatt merged 4 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2022-09-feature-or
Sep 9, 2022
Merged
Fetch InitFeatures from both Channel and Routing Message Handlers#1701
TheBlueMatt merged 4 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2022-09-feature-or
Conversation
@TheBlueMatt
@TheBlueMattTheBlueMatt
commented
Sep 7, 2022

Copy link


Copy Markdown

Collaborator
Based on #1699, this fetches the InitFeatures from both Channel and Routing Message Handlers and OR's them toegether. It then moves the relevant feature flags to our ChannelManager and P2PGossipHandlers.
It should tee us up nicely for #1688 to set the onion message features in the messenger itself.
jkczyz
jkczyz
reviewed
Sep 7, 2022
View reviewed changes
Comment threadlightning/src/ln/peer_handler.rs
Outdated

jkczyz
jkczyz
reviewed
Sep 7, 2022
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/features.rs
Outdated

Comment threadlightning/src/routing/gossip.rs

	fn provided_init_features(&self, _their_node_id: &PublicKey) -> InitFeatures {
 let mut features = InitFeatures::empty();
 features.set_gossip_queries_optional();
@jkczyzjkczyzSep 7, 2022

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Should this also set initial_routing_sync since that is cleared in known_channel_features? Maybe it would be better to define known_channel_features as the set difference of InitFeatures::known with P2PGossipSync::provided_init_features (as a const somewhere). Then what is described in the comment on InitContext's optional features wouldn't be necessary.
@TheBlueMattTheBlueMattSep 7, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

No, the fact that we were previously setting initial_routing_sync was an oversight, basically. If our peer supports gossip sync they'll ignore it, and we shouldnt be setting it on every connection anyway as it'll waste bandwidth.
@jkczyzjkczyzSep 7, 2022•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Should we call this out in the commit message and changelog since it is a behavioral change? Will / should we ever set initial_routing_sync now?
@TheBlueMattTheBlueMattSep 8, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

Sure added a note. We will no longer ever set it, no.
@jkczyzjkczyzSep 8, 2022

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Any way we could still do something with set difference, though? Would need to special case removing initial_routing_sync, but I guess that is an unordinary feature anyhow.
@TheBlueMattTheBlueMattSep 8, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

I'm not sure what a set difference here gets us? If anything I'd be inclined to have the ChannelManager explicitly set all the features it wants, rather than having that logic in features.rs itself, I only didn't bother because it'd be very verbose.
I see the purpose of this change, ultimately as moving away from defining an LDK-global "known features" set, which was always a little awkward, and instead defining the "known features" set in each module that actually provides said features.
@jkczyzjkczyzSep 8, 2022

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Discussed offline. My primary concern was to avoid relying on a comment to know when to update known_channel_features. Possible alternative could be to have a unit test asserting that the intersection of ChannelManager and P2PGossipSync's provided features is empty.
@TheBlueMattTheBlueMattSep 9, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

See #1707
@TheBlueMatt
TheBlueMattforce-pushed
the
2022-09-feature-or
branch
2 times, most recently
from
eced794 to
ff3c0dcCompareSeptember 7, 2022 22:08
jkczyz
jkczyz
reviewed
Sep 7, 2022
View reviewed changes
Comment threadlightning/src/routing/gossip.rs

	fn provided_init_features(&self, _their_node_id: &PublicKey) -> InitFeatures {
 let mut features = InitFeatures::empty();
 features.set_gossip_queries_optional();
@jkczyzjkczyzSep 7, 2022•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Should we call this out in the commit message and changelog since it is a behavioral change? Will / should we ever set initial_routing_sync now?
Comment threadlightning/src/ln/peer_handler.rs
Outdated

@TheBlueMatt
TheBlueMatt
commented
Sep 8, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Rebased on #1699.
@codecov-commenter
codecov-commenter
commented
Sep 8, 2022•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #1701 (ba69536) into main (ba69536) will not change coverage.

The diff coverage is n/a.
❗ Current head ba69536 differs from pull request most recent head 1b67b0b. Consider uploading reports for the commit 1b67b0b to get more accurate results
@@ Coverage Diff @@## main #1701 +/- ##
=======================================
Coverage 90.87% 90.87% =======================================
Files 86 86 Lines 46378 46378 Branches 46378 46378 =======================================
Hits 42146 42146 Misses 4232 4232 
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
jkczyz
jkczyz
reviewed
Sep 8, 2022
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Outdated

	/// Panics if `addresses` is absurdly large (more than 100).
	///
	/// [`get_and_clear_pending_msg_events`]: MessageSendEventsProvider::get_and_clear_pending_msg_events
	pub fn broadcast_node_announcement(&self, rgb: [u8; 3], alias: [u8; 32], mut addresses: Vec<NetAddress>) {
@jkczyzjkczyzSep 8, 2022

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Unused import for NetAddress.
@TheBlueMattTheBlueMattSep 9, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

Oops, shit, this was from the previous PR. We have another warning introduced recently, I'll fix both in a followup.
Comment threadlightning/src/ln/msgs.rs
Outdated

Comment threadlightning/src/ln/msgs.rs
Outdated

jkczyz
jkczyz
previously approved these changes
Sep 9, 2022
View reviewed changes
@jkczyzjkczyz
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM. Feel free to squash.
@valentinewallace
valentinewallace
commented
Sep 9, 2022

Copy link


Copy Markdown

Contributor
LGTM after squash
@TheBlueMatt
Fetch our InitFeatures from ChannelMessageHandler
950ccc4
Like we now do for `NodeFeatures`, this converts to asking our
registered `ChannelMessageHandler` for our `InitFeatures` instead
of hard-coding them to the global LDK known set.
This allows handlers to set different feature bits based on what
our configuration actually supports rather than what LDK supports
in aggregate.
@TheBlueMatt
OR InitFeatures from both Channel and Routing message handlers
06cb48a
When we go to send an Init message to new peers, the features we
support are really a combination of all the various features our
different handlers support. This commit captures this concept by
OR'ing our InitFeatures across both our Channel and Routing
handlers.
Note that this also disables setting the `initial_routing_sync`
flag in init messages, as was intended in
e742894, per the comment added on
`clear_initial_routing_sync`, though this should not be a behavior
change in practice as nodes which support gossip queries ignore the
initial routing sync flag.
@TheBlueMatt
Add a new InitFeatures constructor to capture the types of flags
e34a2bc
When `ChannelMessageHandler` implementations wish to return an
`InitFeatures` which contain all the known flags that are relevant
to channel handling, but not gossip handling, they currently need
to do so by manually constructing an InitFeatures with all known
flags and then clearing the ones they dont want.
Instead of spreading this logic out across the codebase, this
consolidates such construction to one place in features.rs.
@TheBlueMatt
Make clear_initial_routing_sync more consistent with other APIs
1b67b0b
@TheBlueMatt
TheBlueMatt
commented
Sep 9, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Squashed without further change.
@TheBlueMatt
TheBlueMatt
merged commit 3fb3218
into
lightningdevkit:mainSep 9, 2022
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@jkczyzjkczyzjkczyz approved these changes
@valentinewallacevalentinewallacevalentinewallace approved these changes

Assignees


No one assigned


Labels

blocked on dependent pr

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@TheBlueMatt@codecov-commenter@valentinewallace@jkczyz

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Highlight search terms from Google/DuckDuckGo/Bing referrer
(function() {
var ref = document.referrer;
var terms = [];
if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {
var url = new URL(ref);
var q = url.searchParams.get('q') || url.searchParams.get('p');
if (q) {
terms = q.split(/\s+/).filter(function(t) { return t.length > 2; });
}
}
if (terms.length === 0) return;
var style = document.createElement('style');
style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';
document.head.appendChild(style);
function highlight(node) {
if (node.nodeType === 3) { // text node
var text = node.textContent;
var found = false;
terms.forEach(function(term) {
var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi');
if (regex.test(text)) {
found = true;
var frag = document.createDocumentFragment();
var parts = text.split(regex);
parts.forEach(function(part, i) {
if (i % 2 === 0) {
frag.appendChild(document.createTextNode(part));
} else {
var span = document.createElement('span');
span.className = 'userscript-highlight';
span.textContent = part;
frag.appendChild(span);
}
});
node.parentNode.replaceChild(frag, node);
}
});
} else if (node.nodeType === 1 && node.childNodes) { // element
var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];
if (!skipTags.includes(node.tagName)) {
Array.from(node.childNodes).forEach(highlight);
}
}
}
highlight(document.body);
// Re-highlight on dynamic content
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1 || node.nodeType === 3) highlight(node);
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Fetch InitFeatures from both Channel and Routing Message Handlers - #1701
Merged
TheBlueMatt merged 4 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2022-09-feature-or
Sep 9, 2022
Merged
Fetch InitFeatures from both Channel and Routing Message Handlers#1701
TheBlueMatt merged 4 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2022-09-feature-or
Conversation
@TheBlueMatt
@TheBlueMattTheBlueMatt
commented
Sep 7, 2022

Copy link


Copy Markdown

Collaborator
Based on #1699, this fetches the InitFeatures from both Channel and Routing Message Handlers and OR's them toegether. It then moves the relevant feature flags to our ChannelManager and P2PGossipHandlers.
It should tee us up nicely for #1688 to set the onion message features in the messenger itself.
jkczyz
jkczyz
reviewed
Sep 7, 2022
View reviewed changes
Comment threadlightning/src/ln/peer_handler.rs
Outdated

jkczyz
jkczyz
reviewed
Sep 7, 2022
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/features.rs
Outdated

Comment threadlightning/src/routing/gossip.rs

	fn provided_init_features(&self, _their_node_id: &PublicKey) -> InitFeatures {
 let mut features = InitFeatures::empty();
 features.set_gossip_queries_optional();
@jkczyzjkczyzSep 7, 2022

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Should this also set initial_routing_sync since that is cleared in known_channel_features? Maybe it would be better to define known_channel_features as the set difference of InitFeatures::known with P2PGossipSync::provided_init_features (as a const somewhere). Then what is described in the comment on InitContext's optional features wouldn't be necessary.
@TheBlueMattTheBlueMattSep 7, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

No, the fact that we were previously setting initial_routing_sync was an oversight, basically. If our peer supports gossip sync they'll ignore it, and we shouldnt be setting it on every connection anyway as it'll waste bandwidth.
@jkczyzjkczyzSep 7, 2022•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Should we call this out in the commit message and changelog since it is a behavioral change? Will / should we ever set initial_routing_sync now?
@TheBlueMattTheBlueMattSep 8, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

Sure added a note. We will no longer ever set it, no.
@jkczyzjkczyzSep 8, 2022

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Any way we could still do something with set difference, though? Would need to special case removing initial_routing_sync, but I guess that is an unordinary feature anyhow.
@TheBlueMattTheBlueMattSep 8, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

I'm not sure what a set difference here gets us? If anything I'd be inclined to have the ChannelManager explicitly set all the features it wants, rather than having that logic in features.rs itself, I only didn't bother because it'd be very verbose.
I see the purpose of this change, ultimately as moving away from defining an LDK-global "known features" set, which was always a little awkward, and instead defining the "known features" set in each module that actually provides said features.
@jkczyzjkczyzSep 8, 2022

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Discussed offline. My primary concern was to avoid relying on a comment to know when to update known_channel_features. Possible alternative could be to have a unit test asserting that the intersection of ChannelManager and P2PGossipSync's provided features is empty.
@TheBlueMattTheBlueMattSep 9, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

See #1707
@TheBlueMatt
TheBlueMattforce-pushed
the
2022-09-feature-or
branch
2 times, most recently
from
eced794 to
ff3c0dcCompareSeptember 7, 2022 22:08
jkczyz
jkczyz
reviewed
Sep 7, 2022
View reviewed changes
Comment threadlightning/src/routing/gossip.rs

	fn provided_init_features(&self, _their_node_id: &PublicKey) -> InitFeatures {
 let mut features = InitFeatures::empty();
 features.set_gossip_queries_optional();
@jkczyzjkczyzSep 7, 2022•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Should we call this out in the commit message and changelog since it is a behavioral change? Will / should we ever set initial_routing_sync now?
Comment threadlightning/src/ln/peer_handler.rs
Outdated

@TheBlueMatt
TheBlueMatt
commented
Sep 8, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Rebased on #1699.
@codecov-commenter
codecov-commenter
commented
Sep 8, 2022•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #1701 (ba69536) into main (ba69536) will not change coverage.

The diff coverage is n/a.
❗ Current head ba69536 differs from pull request most recent head 1b67b0b. Consider uploading reports for the commit 1b67b0b to get more accurate results
@@ Coverage Diff @@## main #1701 +/- ##
=======================================
Coverage 90.87% 90.87% =======================================
Files 86 86 Lines 46378 46378 Branches 46378 46378 =======================================
Hits 42146 42146 Misses 4232 4232 
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
jkczyz
jkczyz
reviewed
Sep 8, 2022
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Outdated

	/// Panics if `addresses` is absurdly large (more than 100).
	///
	/// [`get_and_clear_pending_msg_events`]: MessageSendEventsProvider::get_and_clear_pending_msg_events
	pub fn broadcast_node_announcement(&self, rgb: [u8; 3], alias: [u8; 32], mut addresses: Vec<NetAddress>) {
@jkczyzjkczyzSep 8, 2022

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Unused import for NetAddress.
@TheBlueMattTheBlueMattSep 9, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

Oops, shit, this was from the previous PR. We have another warning introduced recently, I'll fix both in a followup.
Comment threadlightning/src/ln/msgs.rs
Outdated

Comment threadlightning/src/ln/msgs.rs
Outdated

jkczyz
jkczyz
previously approved these changes
Sep 9, 2022
View reviewed changes
@jkczyzjkczyz
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM. Feel free to squash.
@valentinewallace
valentinewallace
commented
Sep 9, 2022

Copy link


Copy Markdown

Contributor
LGTM after squash
@TheBlueMatt
Fetch our InitFeatures from ChannelMessageHandler
950ccc4
Like we now do for `NodeFeatures`, this converts to asking our
registered `ChannelMessageHandler` for our `InitFeatures` instead
of hard-coding them to the global LDK known set.
This allows handlers to set different feature bits based on what
our configuration actually supports rather than what LDK supports
in aggregate.
@TheBlueMatt
OR InitFeatures from both Channel and Routing message handlers
06cb48a
When we go to send an Init message to new peers, the features we
support are really a combination of all the various features our
different handlers support. This commit captures this concept by
OR'ing our InitFeatures across both our Channel and Routing
handlers.
Note that this also disables setting the `initial_routing_sync`
flag in init messages, as was intended in
e742894, per the comment added on
`clear_initial_routing_sync`, though this should not be a behavior
change in practice as nodes which support gossip queries ignore the
initial routing sync flag.
@TheBlueMatt
Add a new InitFeatures constructor to capture the types of flags
e34a2bc
When `ChannelMessageHandler` implementations wish to return an
`InitFeatures` which contain all the known flags that are relevant
to channel handling, but not gossip handling, they currently need
to do so by manually constructing an InitFeatures with all known
flags and then clearing the ones they dont want.
Instead of spreading this logic out across the codebase, this
consolidates such construction to one place in features.rs.
@TheBlueMatt
Make clear_initial_routing_sync more consistent with other APIs
1b67b0b
@TheBlueMatt
TheBlueMatt
commented
Sep 9, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Squashed without further change.
@TheBlueMatt
TheBlueMatt
merged commit 3fb3218
into
lightningdevkit:mainSep 9, 2022
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@jkczyzjkczyzjkczyz approved these changes
@valentinewallacevalentinewallacevalentinewallace approved these changes

Assignees


No one assigned


Labels

blocked on dependent pr

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@TheBlueMatt@codecov-commenter@valentinewallace@jkczyz

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Strip utm_, fbclid, gclid, etc. from all links on page
(function() {
var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',
'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',
'ref', 'ref_src', 'source', 'medium', 'campaign'];
function cleanUrl(url) {
try {
var u = new URL(url, window.location.origin);
var changed = false;
trackingParams.forEach(function(p) {
if (u.searchParams.has(p)) {
u.searchParams.delete(p);
changed = true;
}
});
return changed ? u.toString() : url;
} catch (e) {
return url;
}
}
function cleanLinks() {
document.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
cleanLinks();
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1) {
if (node.tagName === 'A') cleanLinks();
node.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
Fetch InitFeatures from both Channel and Routing Message Handlers - #1701
Merged
TheBlueMatt merged 4 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2022-09-feature-or
Sep 9, 2022
Merged
Fetch InitFeatures from both Channel and Routing Message Handlers#1701
TheBlueMatt merged 4 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2022-09-feature-or
Conversation
@TheBlueMatt
@TheBlueMattTheBlueMatt
commented
Sep 7, 2022

Copy link


Copy Markdown

Collaborator
Based on #1699, this fetches the InitFeatures from both Channel and Routing Message Handlers and OR's them toegether. It then moves the relevant feature flags to our ChannelManager and P2PGossipHandlers.
It should tee us up nicely for #1688 to set the onion message features in the messenger itself.
jkczyz
jkczyz
reviewed
Sep 7, 2022
View reviewed changes
Comment threadlightning/src/ln/peer_handler.rs
Outdated

jkczyz
jkczyz
reviewed
Sep 7, 2022
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/features.rs
Outdated

Comment threadlightning/src/routing/gossip.rs

	fn provided_init_features(&self, _their_node_id: &PublicKey) -> InitFeatures {
 let mut features = InitFeatures::empty();
 features.set_gossip_queries_optional();
@jkczyzjkczyzSep 7, 2022

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Should this also set initial_routing_sync since that is cleared in known_channel_features? Maybe it would be better to define known_channel_features as the set difference of InitFeatures::known with P2PGossipSync::provided_init_features (as a const somewhere). Then what is described in the comment on InitContext's optional features wouldn't be necessary.
@TheBlueMattTheBlueMattSep 7, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

No, the fact that we were previously setting initial_routing_sync was an oversight, basically. If our peer supports gossip sync they'll ignore it, and we shouldnt be setting it on every connection anyway as it'll waste bandwidth.
@jkczyzjkczyzSep 7, 2022•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Should we call this out in the commit message and changelog since it is a behavioral change? Will / should we ever set initial_routing_sync now?
@TheBlueMattTheBlueMattSep 8, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

Sure added a note. We will no longer ever set it, no.
@jkczyzjkczyzSep 8, 2022

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Any way we could still do something with set difference, though? Would need to special case removing initial_routing_sync, but I guess that is an unordinary feature anyhow.
@TheBlueMattTheBlueMattSep 8, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

I'm not sure what a set difference here gets us? If anything I'd be inclined to have the ChannelManager explicitly set all the features it wants, rather than having that logic in features.rs itself, I only didn't bother because it'd be very verbose.
I see the purpose of this change, ultimately as moving away from defining an LDK-global "known features" set, which was always a little awkward, and instead defining the "known features" set in each module that actually provides said features.
@jkczyzjkczyzSep 8, 2022

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Discussed offline. My primary concern was to avoid relying on a comment to know when to update known_channel_features. Possible alternative could be to have a unit test asserting that the intersection of ChannelManager and P2PGossipSync's provided features is empty.
@TheBlueMattTheBlueMattSep 9, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

See #1707
@TheBlueMatt
TheBlueMattforce-pushed
the
2022-09-feature-or
branch
2 times, most recently
from
eced794 to
ff3c0dcCompareSeptember 7, 2022 22:08
jkczyz
jkczyz
reviewed
Sep 7, 2022
View reviewed changes
Comment threadlightning/src/routing/gossip.rs

	fn provided_init_features(&self, _their_node_id: &PublicKey) -> InitFeatures {
 let mut features = InitFeatures::empty();
 features.set_gossip_queries_optional();
@jkczyzjkczyzSep 7, 2022•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Should we call this out in the commit message and changelog since it is a behavioral change? Will / should we ever set initial_routing_sync now?
Comment threadlightning/src/ln/peer_handler.rs
Outdated

@TheBlueMatt
TheBlueMatt
commented
Sep 8, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Rebased on #1699.
@codecov-commenter
codecov-commenter
commented
Sep 8, 2022•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #1701 (ba69536) into main (ba69536) will not change coverage.

The diff coverage is n/a.
❗ Current head ba69536 differs from pull request most recent head 1b67b0b. Consider uploading reports for the commit 1b67b0b to get more accurate results
@@ Coverage Diff @@## main #1701 +/- ##
=======================================
Coverage 90.87% 90.87% =======================================
Files 86 86 Lines 46378 46378 Branches 46378 46378 =======================================
Hits 42146 42146 Misses 4232 4232 
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
jkczyz
jkczyz
reviewed
Sep 8, 2022
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Outdated

	/// Panics if `addresses` is absurdly large (more than 100).
	///
	/// [`get_and_clear_pending_msg_events`]: MessageSendEventsProvider::get_and_clear_pending_msg_events
	pub fn broadcast_node_announcement(&self, rgb: [u8; 3], alias: [u8; 32], mut addresses: Vec<NetAddress>) {
@jkczyzjkczyzSep 8, 2022

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Unused import for NetAddress.
@TheBlueMattTheBlueMattSep 9, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

Oops, shit, this was from the previous PR. We have another warning introduced recently, I'll fix both in a followup.
Comment threadlightning/src/ln/msgs.rs
Outdated

Comment threadlightning/src/ln/msgs.rs
Outdated

jkczyz
jkczyz
previously approved these changes
Sep 9, 2022
View reviewed changes
@jkczyzjkczyz
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM. Feel free to squash.
@valentinewallace
valentinewallace
commented
Sep 9, 2022

Copy link


Copy Markdown

Contributor
LGTM after squash
@TheBlueMatt
Fetch our InitFeatures from ChannelMessageHandler
950ccc4
Like we now do for `NodeFeatures`, this converts to asking our
registered `ChannelMessageHandler` for our `InitFeatures` instead
of hard-coding them to the global LDK known set.
This allows handlers to set different feature bits based on what
our configuration actually supports rather than what LDK supports
in aggregate.
@TheBlueMatt
OR InitFeatures from both Channel and Routing message handlers
06cb48a
When we go to send an Init message to new peers, the features we
support are really a combination of all the various features our
different handlers support. This commit captures this concept by
OR'ing our InitFeatures across both our Channel and Routing
handlers.
Note that this also disables setting the `initial_routing_sync`
flag in init messages, as was intended in
e742894, per the comment added on
`clear_initial_routing_sync`, though this should not be a behavior
change in practice as nodes which support gossip queries ignore the
initial routing sync flag.
@TheBlueMatt
Add a new InitFeatures constructor to capture the types of flags
e34a2bc
When `ChannelMessageHandler` implementations wish to return an
`InitFeatures` which contain all the known flags that are relevant
to channel handling, but not gossip handling, they currently need
to do so by manually constructing an InitFeatures with all known
flags and then clearing the ones they dont want.
Instead of spreading this logic out across the codebase, this
consolidates such construction to one place in features.rs.
@TheBlueMatt
Make clear_initial_routing_sync more consistent with other APIs
1b67b0b
@TheBlueMatt
TheBlueMatt
commented
Sep 9, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Squashed without further change.
@TheBlueMatt
TheBlueMatt
merged commit 3fb3218
into
lightningdevkit:mainSep 9, 2022
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@jkczyzjkczyzjkczyz approved these changes
@valentinewallacevalentinewallacevalentinewallace approved these changes

Assignees


No one assigned


Labels

blocked on dependent pr

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@TheBlueMatt@codecov-commenter@valentinewallace@jkczyz

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Auto-enable theater mode on YouTube
(function() {
function tryTheater() {
var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]');
if (btn && !btn.classList.contains('activated')) {
btn.click();
}
}
// Try immediately
tryTheater();
// Try after navigation (SPA)
var lastUrl = location.href;
setInterval(function() {
if (location.href !== lastUrl) {
lastUrl = location.href;
setTimeout(tryTheater, 500);
}
}, 1000);
// Also try on player load
var observer = new MutationObserver(tryTheater);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Fetch InitFeatures from both Channel and Routing Message Handlers - #1701
Merged
TheBlueMatt merged 4 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2022-09-feature-or
Sep 9, 2022
Merged
Fetch InitFeatures from both Channel and Routing Message Handlers#1701
TheBlueMatt merged 4 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2022-09-feature-or
Conversation
@TheBlueMatt
@TheBlueMattTheBlueMatt
commented
Sep 7, 2022

Copy link


Copy Markdown

Collaborator
Based on #1699, this fetches the InitFeatures from both Channel and Routing Message Handlers and OR's them toegether. It then moves the relevant feature flags to our ChannelManager and P2PGossipHandlers.
It should tee us up nicely for #1688 to set the onion message features in the messenger itself.
jkczyz
jkczyz
reviewed
Sep 7, 2022
View reviewed changes
Comment threadlightning/src/ln/peer_handler.rs
Outdated

jkczyz
jkczyz
reviewed
Sep 7, 2022
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/features.rs
Outdated

Comment threadlightning/src/routing/gossip.rs

	fn provided_init_features(&self, _their_node_id: &PublicKey) -> InitFeatures {
 let mut features = InitFeatures::empty();
 features.set_gossip_queries_optional();
@jkczyzjkczyzSep 7, 2022

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Should this also set initial_routing_sync since that is cleared in known_channel_features? Maybe it would be better to define known_channel_features as the set difference of InitFeatures::known with P2PGossipSync::provided_init_features (as a const somewhere). Then what is described in the comment on InitContext's optional features wouldn't be necessary.
@TheBlueMattTheBlueMattSep 7, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

No, the fact that we were previously setting initial_routing_sync was an oversight, basically. If our peer supports gossip sync they'll ignore it, and we shouldnt be setting it on every connection anyway as it'll waste bandwidth.
@jkczyzjkczyzSep 7, 2022•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Should we call this out in the commit message and changelog since it is a behavioral change? Will / should we ever set initial_routing_sync now?
@TheBlueMattTheBlueMattSep 8, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

Sure added a note. We will no longer ever set it, no.
@jkczyzjkczyzSep 8, 2022

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Any way we could still do something with set difference, though? Would need to special case removing initial_routing_sync, but I guess that is an unordinary feature anyhow.
@TheBlueMattTheBlueMattSep 8, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

I'm not sure what a set difference here gets us? If anything I'd be inclined to have the ChannelManager explicitly set all the features it wants, rather than having that logic in features.rs itself, I only didn't bother because it'd be very verbose.
I see the purpose of this change, ultimately as moving away from defining an LDK-global "known features" set, which was always a little awkward, and instead defining the "known features" set in each module that actually provides said features.
@jkczyzjkczyzSep 8, 2022

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Discussed offline. My primary concern was to avoid relying on a comment to know when to update known_channel_features. Possible alternative could be to have a unit test asserting that the intersection of ChannelManager and P2PGossipSync's provided features is empty.
@TheBlueMattTheBlueMattSep 9, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

See #1707
@TheBlueMatt
TheBlueMattforce-pushed
the
2022-09-feature-or
branch
2 times, most recently
from
eced794 to
ff3c0dcCompareSeptember 7, 2022 22:08
jkczyz
jkczyz
reviewed
Sep 7, 2022
View reviewed changes
Comment threadlightning/src/routing/gossip.rs

	fn provided_init_features(&self, _their_node_id: &PublicKey) -> InitFeatures {
 let mut features = InitFeatures::empty();
 features.set_gossip_queries_optional();
@jkczyzjkczyzSep 7, 2022•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Should we call this out in the commit message and changelog since it is a behavioral change? Will / should we ever set initial_routing_sync now?
Comment threadlightning/src/ln/peer_handler.rs
Outdated

@TheBlueMatt
TheBlueMatt
commented
Sep 8, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Rebased on #1699.
@codecov-commenter
codecov-commenter
commented
Sep 8, 2022•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #1701 (ba69536) into main (ba69536) will not change coverage.

The diff coverage is n/a.
❗ Current head ba69536 differs from pull request most recent head 1b67b0b. Consider uploading reports for the commit 1b67b0b to get more accurate results
@@ Coverage Diff @@## main #1701 +/- ##
=======================================
Coverage 90.87% 90.87% =======================================
Files 86 86 Lines 46378 46378 Branches 46378 46378 =======================================
Hits 42146 42146 Misses 4232 4232 
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
jkczyz
jkczyz
reviewed
Sep 8, 2022
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Outdated

	/// Panics if `addresses` is absurdly large (more than 100).
	///
	/// [`get_and_clear_pending_msg_events`]: MessageSendEventsProvider::get_and_clear_pending_msg_events
	pub fn broadcast_node_announcement(&self, rgb: [u8; 3], alias: [u8; 32], mut addresses: Vec<NetAddress>) {
@jkczyzjkczyzSep 8, 2022

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Unused import for NetAddress.
@TheBlueMattTheBlueMattSep 9, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

Oops, shit, this was from the previous PR. We have another warning introduced recently, I'll fix both in a followup.
Comment threadlightning/src/ln/msgs.rs
Outdated

Comment threadlightning/src/ln/msgs.rs
Outdated

jkczyz
jkczyz
previously approved these changes
Sep 9, 2022
View reviewed changes
@jkczyzjkczyz
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM. Feel free to squash.
@valentinewallace
valentinewallace
commented
Sep 9, 2022

Copy link


Copy Markdown

Contributor
LGTM after squash
@TheBlueMatt
Fetch our InitFeatures from ChannelMessageHandler
950ccc4
Like we now do for `NodeFeatures`, this converts to asking our
registered `ChannelMessageHandler` for our `InitFeatures` instead
of hard-coding them to the global LDK known set.
This allows handlers to set different feature bits based on what
our configuration actually supports rather than what LDK supports
in aggregate.
@TheBlueMatt
OR InitFeatures from both Channel and Routing message handlers
06cb48a
When we go to send an Init message to new peers, the features we
support are really a combination of all the various features our
different handlers support. This commit captures this concept by
OR'ing our InitFeatures across both our Channel and Routing
handlers.
Note that this also disables setting the `initial_routing_sync`
flag in init messages, as was intended in
e742894, per the comment added on
`clear_initial_routing_sync`, though this should not be a behavior
change in practice as nodes which support gossip queries ignore the
initial routing sync flag.
@TheBlueMatt
Add a new InitFeatures constructor to capture the types of flags
e34a2bc
When `ChannelMessageHandler` implementations wish to return an
`InitFeatures` which contain all the known flags that are relevant
to channel handling, but not gossip handling, they currently need
to do so by manually constructing an InitFeatures with all known
flags and then clearing the ones they dont want.
Instead of spreading this logic out across the codebase, this
consolidates such construction to one place in features.rs.
@TheBlueMatt
Make clear_initial_routing_sync more consistent with other APIs
1b67b0b
@TheBlueMatt
TheBlueMatt
commented
Sep 9, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Squashed without further change.
@TheBlueMatt
TheBlueMatt
merged commit 3fb3218
into
lightningdevkit:mainSep 9, 2022
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@jkczyzjkczyzjkczyz approved these changes
@valentinewallacevalentinewallacevalentinewallace approved these changes

Assignees


No one assigned


Labels

blocked on dependent pr

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@TheBlueMatt@codecov-commenter@valentinewallace@jkczyz

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Remove or un-stick sticky/fixed headers that block content
(function() {
function unstick() {
document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) {
if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {
el.style.position = 'static';
el.style.top = 'auto';
el.style.zIndex = 'auto';
}
});
}
unstick();
var observer = new MutationObserver(unstick);
observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });
})();
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Fetch InitFeatures from both Channel and Routing Message Handlers - #1701
Merged
TheBlueMatt merged 4 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2022-09-feature-or
Sep 9, 2022
Merged
Fetch InitFeatures from both Channel and Routing Message Handlers#1701
TheBlueMatt merged 4 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2022-09-feature-or
Conversation
@TheBlueMatt
@TheBlueMattTheBlueMatt
commented
Sep 7, 2022

Copy link


Copy Markdown

Collaborator
Based on #1699, this fetches the InitFeatures from both Channel and Routing Message Handlers and OR's them toegether. It then moves the relevant feature flags to our ChannelManager and P2PGossipHandlers.
It should tee us up nicely for #1688 to set the onion message features in the messenger itself.
jkczyz
jkczyz
reviewed
Sep 7, 2022
View reviewed changes
Comment threadlightning/src/ln/peer_handler.rs
Outdated

jkczyz
jkczyz
reviewed
Sep 7, 2022
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/features.rs
Outdated

Comment threadlightning/src/routing/gossip.rs

	fn provided_init_features(&self, _their_node_id: &PublicKey) -> InitFeatures {
 let mut features = InitFeatures::empty();
 features.set_gossip_queries_optional();
@jkczyzjkczyzSep 7, 2022

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Should this also set initial_routing_sync since that is cleared in known_channel_features? Maybe it would be better to define known_channel_features as the set difference of InitFeatures::known with P2PGossipSync::provided_init_features (as a const somewhere). Then what is described in the comment on InitContext's optional features wouldn't be necessary.
@TheBlueMattTheBlueMattSep 7, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

No, the fact that we were previously setting initial_routing_sync was an oversight, basically. If our peer supports gossip sync they'll ignore it, and we shouldnt be setting it on every connection anyway as it'll waste bandwidth.
@jkczyzjkczyzSep 7, 2022•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Should we call this out in the commit message and changelog since it is a behavioral change? Will / should we ever set initial_routing_sync now?
@TheBlueMattTheBlueMattSep 8, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

Sure added a note. We will no longer ever set it, no.
@jkczyzjkczyzSep 8, 2022

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Any way we could still do something with set difference, though? Would need to special case removing initial_routing_sync, but I guess that is an unordinary feature anyhow.
@TheBlueMattTheBlueMattSep 8, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

I'm not sure what a set difference here gets us? If anything I'd be inclined to have the ChannelManager explicitly set all the features it wants, rather than having that logic in features.rs itself, I only didn't bother because it'd be very verbose.
I see the purpose of this change, ultimately as moving away from defining an LDK-global "known features" set, which was always a little awkward, and instead defining the "known features" set in each module that actually provides said features.
@jkczyzjkczyzSep 8, 2022

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Discussed offline. My primary concern was to avoid relying on a comment to know when to update known_channel_features. Possible alternative could be to have a unit test asserting that the intersection of ChannelManager and P2PGossipSync's provided features is empty.
@TheBlueMattTheBlueMattSep 9, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

See #1707
@TheBlueMatt
TheBlueMattforce-pushed
the
2022-09-feature-or
branch
2 times, most recently
from
eced794 to
ff3c0dcCompareSeptember 7, 2022 22:08
jkczyz
jkczyz
reviewed
Sep 7, 2022
View reviewed changes
Comment threadlightning/src/routing/gossip.rs

	fn provided_init_features(&self, _their_node_id: &PublicKey) -> InitFeatures {
 let mut features = InitFeatures::empty();
 features.set_gossip_queries_optional();
@jkczyzjkczyzSep 7, 2022•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Should we call this out in the commit message and changelog since it is a behavioral change? Will / should we ever set initial_routing_sync now?
Comment threadlightning/src/ln/peer_handler.rs
Outdated

@TheBlueMatt
TheBlueMatt
commented
Sep 8, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Rebased on #1699.
@codecov-commenter
codecov-commenter
commented
Sep 8, 2022•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #1701 (ba69536) into main (ba69536) will not change coverage.

The diff coverage is n/a.
❗ Current head ba69536 differs from pull request most recent head 1b67b0b. Consider uploading reports for the commit 1b67b0b to get more accurate results
@@ Coverage Diff @@## main #1701 +/- ##
=======================================
Coverage 90.87% 90.87% =======================================
Files 86 86 Lines 46378 46378 Branches 46378 46378 =======================================
Hits 42146 42146 Misses 4232 4232 
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
jkczyz
jkczyz
reviewed
Sep 8, 2022
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Outdated

	/// Panics if `addresses` is absurdly large (more than 100).
	///
	/// [`get_and_clear_pending_msg_events`]: MessageSendEventsProvider::get_and_clear_pending_msg_events
	pub fn broadcast_node_announcement(&self, rgb: [u8; 3], alias: [u8; 32], mut addresses: Vec<NetAddress>) {
@jkczyzjkczyzSep 8, 2022

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Unused import for NetAddress.
@TheBlueMattTheBlueMattSep 9, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

Oops, shit, this was from the previous PR. We have another warning introduced recently, I'll fix both in a followup.
Comment threadlightning/src/ln/msgs.rs
Outdated

Comment threadlightning/src/ln/msgs.rs
Outdated

jkczyz
jkczyz
previously approved these changes
Sep 9, 2022
View reviewed changes
@jkczyzjkczyz
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM. Feel free to squash.
@valentinewallace
valentinewallace
commented
Sep 9, 2022

Copy link


Copy Markdown

Contributor
LGTM after squash
@TheBlueMatt
Fetch our InitFeatures from ChannelMessageHandler
950ccc4
Like we now do for `NodeFeatures`, this converts to asking our
registered `ChannelMessageHandler` for our `InitFeatures` instead
of hard-coding them to the global LDK known set.
This allows handlers to set different feature bits based on what
our configuration actually supports rather than what LDK supports
in aggregate.
@TheBlueMatt
OR InitFeatures from both Channel and Routing message handlers
06cb48a
When we go to send an Init message to new peers, the features we
support are really a combination of all the various features our
different handlers support. This commit captures this concept by
OR'ing our InitFeatures across both our Channel and Routing
handlers.
Note that this also disables setting the `initial_routing_sync`
flag in init messages, as was intended in
e742894, per the comment added on
`clear_initial_routing_sync`, though this should not be a behavior
change in practice as nodes which support gossip queries ignore the
initial routing sync flag.
@TheBlueMatt
Add a new InitFeatures constructor to capture the types of flags
e34a2bc
When `ChannelMessageHandler` implementations wish to return an
`InitFeatures` which contain all the known flags that are relevant
to channel handling, but not gossip handling, they currently need
to do so by manually constructing an InitFeatures with all known
flags and then clearing the ones they dont want.
Instead of spreading this logic out across the codebase, this
consolidates such construction to one place in features.rs.
@TheBlueMatt
Make clear_initial_routing_sync more consistent with other APIs
1b67b0b
@TheBlueMatt
TheBlueMatt
commented
Sep 9, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Squashed without further change.
@TheBlueMatt
TheBlueMatt
merged commit 3fb3218
into
lightningdevkit:mainSep 9, 2022
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@jkczyzjkczyzjkczyz approved these changes
@valentinewallacevalentinewallacevalentinewallace approved these changes

Assignees


No one assigned


Labels

blocked on dependent pr

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@TheBlueMatt@codecov-commenter@valentinewallace@jkczyz

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Universal Dark Mode - works on any site
(function() {
var enabled = true;
function applyDarkMode() {
if (!enabled) return;
// Create style element if it doesn't exist
var style = document.getElementById('universal-dark-mode-style');
if (!style) {
style = document.createElement('style');
style.id = 'universal-dark-mode-style';
document.head.appendChild(style);
}
// Dark mode CSS - inverts colors but preserves images/video
style.textContent = '
/* Invert everything except media */
html {
filter: invert(1) hue-rotate(180deg) !important;
background: #1a1a2e !important;
}
/* Restore images, videos, iframes, canvas */
img, video, iframe, canvas, svg, picture, [style*="background-image"] {
filter: invert(1) hue-rotate(180deg) !important;
}
/* Preserve specific elements that should not be inverted */
.no-dark-mode, .no-dark-mode *,
[data-theme="light"], [data-theme="light"],
.ace_editor, .ace_editor *,
.CodeMirror, .CodeMirror *,
.monaco-editor, .monaco-editor *,
.markdown-body pre, .markdown-body pre *,
.highlight, .highlight *,
pre code, pre code * {
filter: none !important;
}
/* Fix common UI elements */
.modal, .popup, .dropdown-menu, .tooltip, .popover {
filter: invert(1) hue-rotate(180deg) !important;
background: #2d2d44 !important;
border-color: #444 !important;
}
/* Scrollbars */
::-webkit-scrollbar { background: #1a1a2e !important; }
::-webkit-scrollbar-thumb { background: #444 !important; }
::-webkit-scrollbar-thumb:hover { background: #555 !important; }
/* Selection */
::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }
::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }
';
}
function removeDarkMode() {
var style = document.getElementById('universal-dark-mode-style');
if (style) style.remove();
}
// Toggle with Alt+Shift+D
document.addEventListener('keydown', function(e) {
if (e.altKey && e.shiftKey && e.key === 'D') {
e.preventDefault();
enabled = !enabled;
if (enabled) {
applyDarkMode();
console.log('[Universal Dark Mode] Enabled');
} else {
removeDarkMode();
console.log('[Universal Dark Mode] Disabled');
}
}
});
// Apply on load
applyDarkMode();
// Re-apply on dynamic content
var observer = new MutationObserver(function(mutations) {
if (enabled && !document.getElementById('universal-dark-mode-style')) {
applyDarkMode();
}
});
observer.observe(document.head, { childList: true });
console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');
})();
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
Fetch InitFeatures from both Channel and Routing Message Handlers - #1701
Merged
TheBlueMatt merged 4 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2022-09-feature-or
Sep 9, 2022
Merged
Fetch InitFeatures from both Channel and Routing Message Handlers#1701
TheBlueMatt merged 4 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2022-09-feature-or
Conversation
@TheBlueMatt
@TheBlueMattTheBlueMatt
commented
Sep 7, 2022

Copy link


Copy Markdown

Collaborator
Based on #1699, this fetches the InitFeatures from both Channel and Routing Message Handlers and OR's them toegether. It then moves the relevant feature flags to our ChannelManager and P2PGossipHandlers.
It should tee us up nicely for #1688 to set the onion message features in the messenger itself.
jkczyz
jkczyz
reviewed
Sep 7, 2022
View reviewed changes
Comment threadlightning/src/ln/peer_handler.rs
Outdated

jkczyz
jkczyz
reviewed
Sep 7, 2022
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Outdated

Comment threadlightning/src/ln/features.rs
Outdated

Comment threadlightning/src/routing/gossip.rs

	fn provided_init_features(&self, _their_node_id: &PublicKey) -> InitFeatures {
 let mut features = InitFeatures::empty();
 features.set_gossip_queries_optional();
@jkczyzjkczyzSep 7, 2022

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Should this also set initial_routing_sync since that is cleared in known_channel_features? Maybe it would be better to define known_channel_features as the set difference of InitFeatures::known with P2PGossipSync::provided_init_features (as a const somewhere). Then what is described in the comment on InitContext's optional features wouldn't be necessary.
@TheBlueMattTheBlueMattSep 7, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

No, the fact that we were previously setting initial_routing_sync was an oversight, basically. If our peer supports gossip sync they'll ignore it, and we shouldnt be setting it on every connection anyway as it'll waste bandwidth.
@jkczyzjkczyzSep 7, 2022•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Should we call this out in the commit message and changelog since it is a behavioral change? Will / should we ever set initial_routing_sync now?
@TheBlueMattTheBlueMattSep 8, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

Sure added a note. We will no longer ever set it, no.
@jkczyzjkczyzSep 8, 2022

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Any way we could still do something with set difference, though? Would need to special case removing initial_routing_sync, but I guess that is an unordinary feature anyhow.
@TheBlueMattTheBlueMattSep 8, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

I'm not sure what a set difference here gets us? If anything I'd be inclined to have the ChannelManager explicitly set all the features it wants, rather than having that logic in features.rs itself, I only didn't bother because it'd be very verbose.
I see the purpose of this change, ultimately as moving away from defining an LDK-global "known features" set, which was always a little awkward, and instead defining the "known features" set in each module that actually provides said features.
@jkczyzjkczyzSep 8, 2022

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Discussed offline. My primary concern was to avoid relying on a comment to know when to update known_channel_features. Possible alternative could be to have a unit test asserting that the intersection of ChannelManager and P2PGossipSync's provided features is empty.
@TheBlueMattTheBlueMattSep 9, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

See #1707
@TheBlueMatt
TheBlueMattforce-pushed
the
2022-09-feature-or
branch
2 times, most recently
from
eced794 to
ff3c0dcCompareSeptember 7, 2022 22:08
jkczyz
jkczyz
reviewed
Sep 7, 2022
View reviewed changes
Comment threadlightning/src/routing/gossip.rs

	fn provided_init_features(&self, _their_node_id: &PublicKey) -> InitFeatures {
 let mut features = InitFeatures::empty();
 features.set_gossip_queries_optional();
@jkczyzjkczyzSep 7, 2022•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Should we call this out in the commit message and changelog since it is a behavioral change? Will / should we ever set initial_routing_sync now?
Comment threadlightning/src/ln/peer_handler.rs
Outdated

@TheBlueMatt
TheBlueMatt
commented
Sep 8, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Rebased on #1699.
@codecov-commenter
codecov-commenter
commented
Sep 8, 2022•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #1701 (ba69536) into main (ba69536) will not change coverage.

The diff coverage is n/a.
❗ Current head ba69536 differs from pull request most recent head 1b67b0b. Consider uploading reports for the commit 1b67b0b to get more accurate results
@@ Coverage Diff @@## main #1701 +/- ##
=======================================
Coverage 90.87% 90.87% =======================================
Files 86 86 Lines 46378 46378 Branches 46378 46378 =======================================
Hits 42146 42146 Misses 4232 4232 
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
jkczyz
jkczyz
reviewed
Sep 8, 2022
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
Outdated

	/// Panics if `addresses` is absurdly large (more than 100).
	///
	/// [`get_and_clear_pending_msg_events`]: MessageSendEventsProvider::get_and_clear_pending_msg_events
	pub fn broadcast_node_announcement(&self, rgb: [u8; 3], alias: [u8; 32], mut addresses: Vec<NetAddress>) {
@jkczyzjkczyzSep 8, 2022

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Unused import for NetAddress.
@TheBlueMattTheBlueMattSep 9, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

Oops, shit, this was from the previous PR. We have another warning introduced recently, I'll fix both in a followup.
Comment threadlightning/src/ln/msgs.rs
Outdated

Comment threadlightning/src/ln/msgs.rs
Outdated

jkczyz
jkczyz
previously approved these changes
Sep 9, 2022
View reviewed changes
@jkczyzjkczyz
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM. Feel free to squash.
@valentinewallace
valentinewallace
commented
Sep 9, 2022

Copy link


Copy Markdown

Contributor
LGTM after squash
@TheBlueMatt
Fetch our InitFeatures from ChannelMessageHandler
950ccc4
Like we now do for `NodeFeatures`, this converts to asking our
registered `ChannelMessageHandler` for our `InitFeatures` instead
of hard-coding them to the global LDK known set.
This allows handlers to set different feature bits based on what
our configuration actually supports rather than what LDK supports
in aggregate.
@TheBlueMatt
OR InitFeatures from both Channel and Routing message handlers
06cb48a
When we go to send an Init message to new peers, the features we
support are really a combination of all the various features our
different handlers support. This commit captures this concept by
OR'ing our InitFeatures across both our Channel and Routing
handlers.
Note that this also disables setting the `initial_routing_sync`
flag in init messages, as was intended in
e742894, per the comment added on
`clear_initial_routing_sync`, though this should not be a behavior
change in practice as nodes which support gossip queries ignore the
initial routing sync flag.
@TheBlueMatt
Add a new InitFeatures constructor to capture the types of flags
e34a2bc
When `ChannelMessageHandler` implementations wish to return an
`InitFeatures` which contain all the known flags that are relevant
to channel handling, but not gossip handling, they currently need
to do so by manually constructing an InitFeatures with all known
flags and then clearing the ones they dont want.
Instead of spreading this logic out across the codebase, this
consolidates such construction to one place in features.rs.
@TheBlueMatt
Make clear_initial_routing_sync more consistent with other APIs
1b67b0b
@TheBlueMatt
TheBlueMatt
commented
Sep 9, 2022

Copy link


Copy Markdown

CollaboratorAuthor
Squashed without further change.
@TheBlueMatt
TheBlueMatt
merged commit 3fb3218
into
lightningdevkit:mainSep 9, 2022
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@jkczyzjkczyzjkczyz approved these changes
@valentinewallacevalentinewallacevalentinewallace approved these changes

Assignees


No one assigned


Labels

blocked on dependent pr

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@TheBlueMatt@codecov-commenter@valentinewallace@jkczyz