Skip to content

Fix sender is the introduction node onion messages - #2951

Merged
TheBlueMatt merged 6 commits into
lightningdevkit:mainfrom
jkczyz:2024-03-fix-sender-is-intro-node
Mar 20, 2024
Merged

Fix sender is the introduction node onion messages#2951
TheBlueMatt merged 6 commits into
lightningdevkit:mainfrom
jkczyz:2024-03-fix-sender-is-intro-node

Conversation

@jkczyz

@jkczyzjkczyz commented Mar 19, 2024

Copy link
Copy Markdown
Contributor

DefaultMessageRouter will form an OnionMessagePath from a BlindedPath where the sender is the introduction node but only if the sender is announced. If the sender is unannounced, then DefaultMessageRouter will fail. While DefaultMessageRouter will only create a blinded path with an announced introduction node, it may receive one where the introduction node is unannounced. Don't return an error in this case, as the OnionMessenger can advance the blinded path by one hop.

This may occur when two nodes have an unannounced channel and one (the offer creator) wants to use it for payments without an intermediary node and without putting its node id in the offer.

@jkczyz
Re-order imports
22c9da3
@jkczyz
Include SocketAddress in test node_announcement
dc627b3
@jkczyz
Fix sender is the introduction node onion messages
f2b84e0
DefaultMessageRouter will form an OnionMessagePath from a BlindedPath
where the sender is the introduction node but only if the sender is
announced. If the sender is unannounced, then DefaultMessageRouter will
fail. While DefaultMessageRouter will only create a blinded path with an
announced introduction node, it may receive one where the introduction
node is unannounced. Don't return an error in this case, as the
OnionMessenger can advance the blinded path by one hop.
This may occur when two nodes have an unannounced channel and one (the
offer creator) wants to use it for payments without an intermediary node
and without putting its node id in the offer.
@jkczyz
pub(crate) visibility for routing/test_utils.rs
3d2a0c2
Give pub(crate) visibility to some routing test utilities to facilitate
testing DefaultMessageRouter in functional tests.
@jkczyz
Use DefaultMessageRouter in functional tests
ad3de23
This helps test cases in DefaultMessageRouter that may not be exercised
now or in the future.
@jkczyz
Use OnionMessenger::send_onion_message in tests
806fef5
Use OnionMessenger's public interface in tests whenever possible (i.e.,
when not using any intermediate_nodes in an OnionMessagePath. This
allows us to exercise DefaultMessageRouter, and, in particular that a
path can be found for an unannounced sender when its in the introduction
node.
@codecov-commenter

codecov-commenter commented Mar 19, 2024

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.62%. Comparing base (670b41a) to head (806fef5).
Report is 33 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@ Coverage Diff @@## main #2951 +/- ##
==========================================
+ Coverage 89.11% 89.62% +0.51% 
==========================================
Files 117 117 Lines 95029 97762 +2733 Branches 95029 97762 +2733 ==========================================
+ Hits 84685 87621 +2936 + Misses 7856 7692 -164 + Partials 2488 2449 -39

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

dunxen
dunxen approved these changes Mar 20, 2024
View reviewed changes

@dunxendunxen 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

CI failures unrelated and on main. Do we have an issue tracking them?

tnull
tnull reviewed Mar 20, 2024
View reviewed changes

@tnulltnull 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, although in this context I'd like to (briefly) raise the question again whether we should relax some of the announcement checks for the introduction node to also support BOLT12 between two directly-connected nodes, i.e., something along the lines of tnull@8138bec

If we're really concerned about the privacy leakage, it would be nice to at least expose a config flag allowing to do this, as otherwise it's pretty confusing for users why they could send payments via an intermediate node, but not directly.

@TheBlueMatt

TheBlueMatt commented Mar 20, 2024

Copy link
Copy Markdown
Collaborator

Can you open an issue? That seems at least marginally unrelated and I'd like to better understand the failure cases and don't want to derail this PR.

@TheBlueMatt
TheBlueMatt merged commit 0cc0858 into lightningdevkit:mainMar 20, 2024
@tnull

tnull commented Mar 20, 2024

Copy link
Copy Markdown
Contributor

Can you open an issue? That seems at least marginally unrelated and I'd like to better understand the failure cases and don't want to derail this PR.

Yes, sorry, this was definitely not my intention. Now tracking here: #2952

@jkczyzjkczyz mentioned this pull request Mar 20, 2024
Closed
9 tasks
@valentinewallacevalentinewallace mentioned this pull request May 1, 2024
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tnulltnulltnull left review comments
@TheBlueMattTheBlueMattTheBlueMatt approved these changes
@dunxendunxendunxen approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@jkczyz@codecov-commenter@TheBlueMatt@tnull@dunxen
, '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" + '
Fix sender is the introduction node onion messages by jkczyz · Pull Request #2951 · lightningdevkit/rust-lightning · GitHub
Skip to content
Fix sender is the introduction node onion messages - #2951
Merged
TheBlueMatt merged 6 commits into
lightningdevkit:mainfrom 
jkczyz:2024-03-fix-sender-is-intro-node
Mar 20, 2024
Merged
Fix sender is the introduction node onion messages#2951
TheBlueMatt merged 6 commits into
lightningdevkit:mainfrom 
jkczyz:2024-03-fix-sender-is-intro-node
Conversation
@jkczyz
@jkczyzjkczyz
commented
Mar 19, 2024

Copy link


Copy Markdown

Contributor
DefaultMessageRouter will form an OnionMessagePath from a BlindedPath where the sender is the introduction node but only if the sender is announced. If the sender is unannounced, then DefaultMessageRouter will fail. While DefaultMessageRouter will only create a blinded path with an announced introduction node, it may receive one where the introduction node is unannounced. Don't return an error in this case, as the OnionMessenger can advance the blinded path by one hop.
This may occur when two nodes have an unannounced channel and one (the offer creator) wants to use it for payments without an intermediary node and without putting its node id in the offer.
@jkczyz
Re-order imports
22c9da3
@jkczyz
Include SocketAddress in test node_announcement
dc627b3
@jkczyz
Fix sender is the introduction node onion messages
f2b84e0
DefaultMessageRouter will form an OnionMessagePath from a BlindedPath
where the sender is the introduction node but only if the sender is
announced. If the sender is unannounced, then DefaultMessageRouter will
fail. While DefaultMessageRouter will only create a blinded path with an
announced introduction node, it may receive one where the introduction
node is unannounced. Don't return an error in this case, as the
OnionMessenger can advance the blinded path by one hop.
This may occur when two nodes have an unannounced channel and one (the
offer creator) wants to use it for payments without an intermediary node
and without putting its node id in the offer.
@jkczyz
pub(crate) visibility for routing/test_utils.rs
3d2a0c2
Give pub(crate) visibility to some routing test utilities to facilitate
testing DefaultMessageRouter in functional tests.
@jkczyz
Use DefaultMessageRouter in functional tests
ad3de23
This helps test cases in DefaultMessageRouter that may not be exercised
now or in the future.
@jkczyz
Use OnionMessenger::send_onion_message in tests
806fef5
Use OnionMessenger's public interface in tests whenever possible (i.e.,
when not using any intermediate_nodes in an OnionMessagePath. This
allows us to exercise DefaultMessageRouter, and, in particular that a
path can be found for an unannounced sender when its in the introduction
node.
@codecov-commenter
codecov-commenter
commented
Mar 19, 2024

Copy link


Copy Markdown

Codecov Report
All modified and coverable lines are covered by tests ✅
Project coverage is 89.62%. Comparing base (670b41a) to head (806fef5).

Report is 33 commits behind head on main.
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files
@@ Coverage Diff @@## main #2951 +/- ##
==========================================
+ Coverage 89.11% 89.62% +0.51% 
==========================================
Files 117 117 Lines 95029 97762 +2733 Branches 95029 97762 +2733 ==========================================
+ Hits 84685 87621 +2936 + Misses 7856 7692 -164 + Partials 2488 2449 -39 
☔ View full report in Codecov by Sentry.

📢 Have feedback on the report? Share it here.
dunxen
dunxen
approved these changes
Mar 20, 2024
View reviewed changes
@dunxendunxen
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
CI failures unrelated and on main. Do we have an issue tracking them?
tnull
tnull
reviewed
Mar 20, 2024
View reviewed changes
@tnulltnull
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, although in this context I'd like to (briefly) raise the question again whether we should relax some of the announcement checks for the introduction node to also support BOLT12 between two directly-connected nodes, i.e., something along the lines of tnull@8138bec
If we're really concerned about the privacy leakage, it would be nice to at least expose a config flag allowing to do this, as otherwise it's pretty confusing for users why they could send payments via an intermediate node, but not directly.
@TheBlueMatt
TheBlueMatt
commented
Mar 20, 2024

Copy link


Copy Markdown

Collaborator
Can you open an issue? That seems at least marginally unrelated and I'd like to better understand the failure cases and don't want to derail this PR.
@TheBlueMatt
TheBlueMatt
merged commit 0cc0858
into
lightningdevkit:mainMar 20, 2024
@tnull
tnull
commented
Mar 20, 2024

Copy link


Copy Markdown

Contributor
Can you open an issue? That seems at least marginally unrelated and I'd like to better understand the failure cases and don't want to derail this PR.
Yes, sorry, this was definitely not my intention. Now tracking here: #2952
@jkczyzjkczyz
mentioned this pull request
Mar 20, 2024

Closed

9 tasks
@valentinewallacevalentinewallace
mentioned this pull request
May 1, 2024
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@tnulltnulltnull left review comments
@TheBlueMattTheBlueMattTheBlueMatt approved these changes
@dunxendunxendunxen approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@jkczyz@codecov-commenter@TheBlueMatt@tnull@dunxen

, '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('^' + ".*" + '
Fix sender is the introduction node onion messages by jkczyz · Pull Request #2951 · lightningdevkit/rust-lightning · GitHub
Skip to content
Fix sender is the introduction node onion messages - #2951
Merged
TheBlueMatt merged 6 commits into
lightningdevkit:mainfrom 
jkczyz:2024-03-fix-sender-is-intro-node
Mar 20, 2024
Merged
Fix sender is the introduction node onion messages#2951
TheBlueMatt merged 6 commits into
lightningdevkit:mainfrom 
jkczyz:2024-03-fix-sender-is-intro-node
Conversation
@jkczyz
@jkczyzjkczyz
commented
Mar 19, 2024

Copy link


Copy Markdown

Contributor
DefaultMessageRouter will form an OnionMessagePath from a BlindedPath where the sender is the introduction node but only if the sender is announced. If the sender is unannounced, then DefaultMessageRouter will fail. While DefaultMessageRouter will only create a blinded path with an announced introduction node, it may receive one where the introduction node is unannounced. Don't return an error in this case, as the OnionMessenger can advance the blinded path by one hop.
This may occur when two nodes have an unannounced channel and one (the offer creator) wants to use it for payments without an intermediary node and without putting its node id in the offer.
@jkczyz
Re-order imports
22c9da3
@jkczyz
Include SocketAddress in test node_announcement
dc627b3
@jkczyz
Fix sender is the introduction node onion messages
f2b84e0
DefaultMessageRouter will form an OnionMessagePath from a BlindedPath
where the sender is the introduction node but only if the sender is
announced. If the sender is unannounced, then DefaultMessageRouter will
fail. While DefaultMessageRouter will only create a blinded path with an
announced introduction node, it may receive one where the introduction
node is unannounced. Don't return an error in this case, as the
OnionMessenger can advance the blinded path by one hop.
This may occur when two nodes have an unannounced channel and one (the
offer creator) wants to use it for payments without an intermediary node
and without putting its node id in the offer.
@jkczyz
pub(crate) visibility for routing/test_utils.rs
3d2a0c2
Give pub(crate) visibility to some routing test utilities to facilitate
testing DefaultMessageRouter in functional tests.
@jkczyz
Use DefaultMessageRouter in functional tests
ad3de23
This helps test cases in DefaultMessageRouter that may not be exercised
now or in the future.
@jkczyz
Use OnionMessenger::send_onion_message in tests
806fef5
Use OnionMessenger's public interface in tests whenever possible (i.e.,
when not using any intermediate_nodes in an OnionMessagePath. This
allows us to exercise DefaultMessageRouter, and, in particular that a
path can be found for an unannounced sender when its in the introduction
node.
@codecov-commenter
codecov-commenter
commented
Mar 19, 2024

Copy link


Copy Markdown

Codecov Report
All modified and coverable lines are covered by tests ✅
Project coverage is 89.62%. Comparing base (670b41a) to head (806fef5).

Report is 33 commits behind head on main.
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files
@@ Coverage Diff @@## main #2951 +/- ##
==========================================
+ Coverage 89.11% 89.62% +0.51% 
==========================================
Files 117 117 Lines 95029 97762 +2733 Branches 95029 97762 +2733 ==========================================
+ Hits 84685 87621 +2936 + Misses 7856 7692 -164 + Partials 2488 2449 -39 
☔ View full report in Codecov by Sentry.

📢 Have feedback on the report? Share it here.
dunxen
dunxen
approved these changes
Mar 20, 2024
View reviewed changes
@dunxendunxen
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
CI failures unrelated and on main. Do we have an issue tracking them?
tnull
tnull
reviewed
Mar 20, 2024
View reviewed changes
@tnulltnull
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, although in this context I'd like to (briefly) raise the question again whether we should relax some of the announcement checks for the introduction node to also support BOLT12 between two directly-connected nodes, i.e., something along the lines of tnull@8138bec
If we're really concerned about the privacy leakage, it would be nice to at least expose a config flag allowing to do this, as otherwise it's pretty confusing for users why they could send payments via an intermediate node, but not directly.
@TheBlueMatt
TheBlueMatt
commented
Mar 20, 2024

Copy link


Copy Markdown

Collaborator
Can you open an issue? That seems at least marginally unrelated and I'd like to better understand the failure cases and don't want to derail this PR.
@TheBlueMatt
TheBlueMatt
merged commit 0cc0858
into
lightningdevkit:mainMar 20, 2024
@tnull
tnull
commented
Mar 20, 2024

Copy link


Copy Markdown

Contributor
Can you open an issue? That seems at least marginally unrelated and I'd like to better understand the failure cases and don't want to derail this PR.
Yes, sorry, this was definitely not my intention. Now tracking here: #2952
@jkczyzjkczyz
mentioned this pull request
Mar 20, 2024

Closed

9 tasks
@valentinewallacevalentinewallace
mentioned this pull request
May 1, 2024
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@tnulltnulltnull left review comments
@TheBlueMattTheBlueMattTheBlueMatt approved these changes
@dunxendunxendunxen approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@jkczyz@codecov-commenter@TheBlueMatt@tnull@dunxen

, '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('^' + ".*" + '
Fix sender is the introduction node onion messages by jkczyz · Pull Request #2951 · lightningdevkit/rust-lightning · GitHub
Skip to content
Fix sender is the introduction node onion messages - #2951
Merged
TheBlueMatt merged 6 commits into
lightningdevkit:mainfrom 
jkczyz:2024-03-fix-sender-is-intro-node
Mar 20, 2024
Merged
Fix sender is the introduction node onion messages#2951
TheBlueMatt merged 6 commits into
lightningdevkit:mainfrom 
jkczyz:2024-03-fix-sender-is-intro-node
Conversation
@jkczyz
@jkczyzjkczyz
commented
Mar 19, 2024

Copy link


Copy Markdown

Contributor
DefaultMessageRouter will form an OnionMessagePath from a BlindedPath where the sender is the introduction node but only if the sender is announced. If the sender is unannounced, then DefaultMessageRouter will fail. While DefaultMessageRouter will only create a blinded path with an announced introduction node, it may receive one where the introduction node is unannounced. Don't return an error in this case, as the OnionMessenger can advance the blinded path by one hop.
This may occur when two nodes have an unannounced channel and one (the offer creator) wants to use it for payments without an intermediary node and without putting its node id in the offer.
@jkczyz
Re-order imports
22c9da3
@jkczyz
Include SocketAddress in test node_announcement
dc627b3
@jkczyz
Fix sender is the introduction node onion messages
f2b84e0
DefaultMessageRouter will form an OnionMessagePath from a BlindedPath
where the sender is the introduction node but only if the sender is
announced. If the sender is unannounced, then DefaultMessageRouter will
fail. While DefaultMessageRouter will only create a blinded path with an
announced introduction node, it may receive one where the introduction
node is unannounced. Don't return an error in this case, as the
OnionMessenger can advance the blinded path by one hop.
This may occur when two nodes have an unannounced channel and one (the
offer creator) wants to use it for payments without an intermediary node
and without putting its node id in the offer.
@jkczyz
pub(crate) visibility for routing/test_utils.rs
3d2a0c2
Give pub(crate) visibility to some routing test utilities to facilitate
testing DefaultMessageRouter in functional tests.
@jkczyz
Use DefaultMessageRouter in functional tests
ad3de23
This helps test cases in DefaultMessageRouter that may not be exercised
now or in the future.
@jkczyz
Use OnionMessenger::send_onion_message in tests
806fef5
Use OnionMessenger's public interface in tests whenever possible (i.e.,
when not using any intermediate_nodes in an OnionMessagePath. This
allows us to exercise DefaultMessageRouter, and, in particular that a
path can be found for an unannounced sender when its in the introduction
node.
@codecov-commenter
codecov-commenter
commented
Mar 19, 2024

Copy link


Copy Markdown

Codecov Report
All modified and coverable lines are covered by tests ✅
Project coverage is 89.62%. Comparing base (670b41a) to head (806fef5).

Report is 33 commits behind head on main.
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files
@@ Coverage Diff @@## main #2951 +/- ##
==========================================
+ Coverage 89.11% 89.62% +0.51% 
==========================================
Files 117 117 Lines 95029 97762 +2733 Branches 95029 97762 +2733 ==========================================
+ Hits 84685 87621 +2936 + Misses 7856 7692 -164 + Partials 2488 2449 -39 
☔ View full report in Codecov by Sentry.

📢 Have feedback on the report? Share it here.
dunxen
dunxen
approved these changes
Mar 20, 2024
View reviewed changes
@dunxendunxen
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
CI failures unrelated and on main. Do we have an issue tracking them?
tnull
tnull
reviewed
Mar 20, 2024
View reviewed changes
@tnulltnull
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, although in this context I'd like to (briefly) raise the question again whether we should relax some of the announcement checks for the introduction node to also support BOLT12 between two directly-connected nodes, i.e., something along the lines of tnull@8138bec
If we're really concerned about the privacy leakage, it would be nice to at least expose a config flag allowing to do this, as otherwise it's pretty confusing for users why they could send payments via an intermediate node, but not directly.
@TheBlueMatt
TheBlueMatt
commented
Mar 20, 2024

Copy link


Copy Markdown

Collaborator
Can you open an issue? That seems at least marginally unrelated and I'd like to better understand the failure cases and don't want to derail this PR.
@TheBlueMatt
TheBlueMatt
merged commit 0cc0858
into
lightningdevkit:mainMar 20, 2024
@tnull
tnull
commented
Mar 20, 2024

Copy link


Copy Markdown

Contributor
Can you open an issue? That seems at least marginally unrelated and I'd like to better understand the failure cases and don't want to derail this PR.
Yes, sorry, this was definitely not my intention. Now tracking here: #2952
@jkczyzjkczyz
mentioned this pull request
Mar 20, 2024

Closed

9 tasks
@valentinewallacevalentinewallace
mentioned this pull request
May 1, 2024
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@tnulltnulltnull left review comments
@TheBlueMattTheBlueMattTheBlueMatt approved these changes
@dunxendunxendunxen approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@jkczyz@codecov-commenter@TheBlueMatt@tnull@dunxen

, '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" + '
Fix sender is the introduction node onion messages by jkczyz · Pull Request #2951 · lightningdevkit/rust-lightning · GitHub
Skip to content
Fix sender is the introduction node onion messages - #2951
Merged
TheBlueMatt merged 6 commits into
lightningdevkit:mainfrom 
jkczyz:2024-03-fix-sender-is-intro-node
Mar 20, 2024
Merged
Fix sender is the introduction node onion messages#2951
TheBlueMatt merged 6 commits into
lightningdevkit:mainfrom 
jkczyz:2024-03-fix-sender-is-intro-node
Conversation
@jkczyz
@jkczyzjkczyz
commented
Mar 19, 2024

Copy link


Copy Markdown

Contributor
DefaultMessageRouter will form an OnionMessagePath from a BlindedPath where the sender is the introduction node but only if the sender is announced. If the sender is unannounced, then DefaultMessageRouter will fail. While DefaultMessageRouter will only create a blinded path with an announced introduction node, it may receive one where the introduction node is unannounced. Don't return an error in this case, as the OnionMessenger can advance the blinded path by one hop.
This may occur when two nodes have an unannounced channel and one (the offer creator) wants to use it for payments without an intermediary node and without putting its node id in the offer.
@jkczyz
Re-order imports
22c9da3
@jkczyz
Include SocketAddress in test node_announcement
dc627b3
@jkczyz
Fix sender is the introduction node onion messages
f2b84e0
DefaultMessageRouter will form an OnionMessagePath from a BlindedPath
where the sender is the introduction node but only if the sender is
announced. If the sender is unannounced, then DefaultMessageRouter will
fail. While DefaultMessageRouter will only create a blinded path with an
announced introduction node, it may receive one where the introduction
node is unannounced. Don't return an error in this case, as the
OnionMessenger can advance the blinded path by one hop.
This may occur when two nodes have an unannounced channel and one (the
offer creator) wants to use it for payments without an intermediary node
and without putting its node id in the offer.
@jkczyz
pub(crate) visibility for routing/test_utils.rs
3d2a0c2
Give pub(crate) visibility to some routing test utilities to facilitate
testing DefaultMessageRouter in functional tests.
@jkczyz
Use DefaultMessageRouter in functional tests
ad3de23
This helps test cases in DefaultMessageRouter that may not be exercised
now or in the future.
@jkczyz
Use OnionMessenger::send_onion_message in tests
806fef5
Use OnionMessenger's public interface in tests whenever possible (i.e.,
when not using any intermediate_nodes in an OnionMessagePath. This
allows us to exercise DefaultMessageRouter, and, in particular that a
path can be found for an unannounced sender when its in the introduction
node.
@codecov-commenter
codecov-commenter
commented
Mar 19, 2024

Copy link


Copy Markdown

Codecov Report
All modified and coverable lines are covered by tests ✅
Project coverage is 89.62%. Comparing base (670b41a) to head (806fef5).

Report is 33 commits behind head on main.
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files
@@ Coverage Diff @@## main #2951 +/- ##
==========================================
+ Coverage 89.11% 89.62% +0.51% 
==========================================
Files 117 117 Lines 95029 97762 +2733 Branches 95029 97762 +2733 ==========================================
+ Hits 84685 87621 +2936 + Misses 7856 7692 -164 + Partials 2488 2449 -39 
☔ View full report in Codecov by Sentry.

📢 Have feedback on the report? Share it here.
dunxen
dunxen
approved these changes
Mar 20, 2024
View reviewed changes
@dunxendunxen
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
CI failures unrelated and on main. Do we have an issue tracking them?
tnull
tnull
reviewed
Mar 20, 2024
View reviewed changes
@tnulltnull
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, although in this context I'd like to (briefly) raise the question again whether we should relax some of the announcement checks for the introduction node to also support BOLT12 between two directly-connected nodes, i.e., something along the lines of tnull@8138bec
If we're really concerned about the privacy leakage, it would be nice to at least expose a config flag allowing to do this, as otherwise it's pretty confusing for users why they could send payments via an intermediate node, but not directly.
@TheBlueMatt
TheBlueMatt
commented
Mar 20, 2024

Copy link


Copy Markdown

Collaborator
Can you open an issue? That seems at least marginally unrelated and I'd like to better understand the failure cases and don't want to derail this PR.
@TheBlueMatt
TheBlueMatt
merged commit 0cc0858
into
lightningdevkit:mainMar 20, 2024
@tnull
tnull
commented
Mar 20, 2024

Copy link


Copy Markdown

Contributor
Can you open an issue? That seems at least marginally unrelated and I'd like to better understand the failure cases and don't want to derail this PR.
Yes, sorry, this was definitely not my intention. Now tracking here: #2952
@jkczyzjkczyz
mentioned this pull request
Mar 20, 2024

Closed

9 tasks
@valentinewallacevalentinewallace
mentioned this pull request
May 1, 2024
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@tnulltnulltnull left review comments
@TheBlueMattTheBlueMattTheBlueMatt approved these changes
@dunxendunxendunxen approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@jkczyz@codecov-commenter@TheBlueMatt@tnull@dunxen

, '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('^' + ".*" + '
Fix sender is the introduction node onion messages by jkczyz · Pull Request #2951 · lightningdevkit/rust-lightning · GitHub
Skip to content
Fix sender is the introduction node onion messages - #2951
Merged
TheBlueMatt merged 6 commits into
lightningdevkit:mainfrom 
jkczyz:2024-03-fix-sender-is-intro-node
Mar 20, 2024
Merged
Fix sender is the introduction node onion messages#2951
TheBlueMatt merged 6 commits into
lightningdevkit:mainfrom 
jkczyz:2024-03-fix-sender-is-intro-node
Conversation
@jkczyz
@jkczyzjkczyz
commented
Mar 19, 2024

Copy link


Copy Markdown

Contributor
DefaultMessageRouter will form an OnionMessagePath from a BlindedPath where the sender is the introduction node but only if the sender is announced. If the sender is unannounced, then DefaultMessageRouter will fail. While DefaultMessageRouter will only create a blinded path with an announced introduction node, it may receive one where the introduction node is unannounced. Don't return an error in this case, as the OnionMessenger can advance the blinded path by one hop.
This may occur when two nodes have an unannounced channel and one (the offer creator) wants to use it for payments without an intermediary node and without putting its node id in the offer.
@jkczyz
Re-order imports
22c9da3
@jkczyz
Include SocketAddress in test node_announcement
dc627b3
@jkczyz
Fix sender is the introduction node onion messages
f2b84e0
DefaultMessageRouter will form an OnionMessagePath from a BlindedPath
where the sender is the introduction node but only if the sender is
announced. If the sender is unannounced, then DefaultMessageRouter will
fail. While DefaultMessageRouter will only create a blinded path with an
announced introduction node, it may receive one where the introduction
node is unannounced. Don't return an error in this case, as the
OnionMessenger can advance the blinded path by one hop.
This may occur when two nodes have an unannounced channel and one (the
offer creator) wants to use it for payments without an intermediary node
and without putting its node id in the offer.
@jkczyz
pub(crate) visibility for routing/test_utils.rs
3d2a0c2
Give pub(crate) visibility to some routing test utilities to facilitate
testing DefaultMessageRouter in functional tests.
@jkczyz
Use DefaultMessageRouter in functional tests
ad3de23
This helps test cases in DefaultMessageRouter that may not be exercised
now or in the future.
@jkczyz
Use OnionMessenger::send_onion_message in tests
806fef5
Use OnionMessenger's public interface in tests whenever possible (i.e.,
when not using any intermediate_nodes in an OnionMessagePath. This
allows us to exercise DefaultMessageRouter, and, in particular that a
path can be found for an unannounced sender when its in the introduction
node.
@codecov-commenter
codecov-commenter
commented
Mar 19, 2024

Copy link


Copy Markdown

Codecov Report
All modified and coverable lines are covered by tests ✅
Project coverage is 89.62%. Comparing base (670b41a) to head (806fef5).

Report is 33 commits behind head on main.
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files
@@ Coverage Diff @@## main #2951 +/- ##
==========================================
+ Coverage 89.11% 89.62% +0.51% 
==========================================
Files 117 117 Lines 95029 97762 +2733 Branches 95029 97762 +2733 ==========================================
+ Hits 84685 87621 +2936 + Misses 7856 7692 -164 + Partials 2488 2449 -39 
☔ View full report in Codecov by Sentry.

📢 Have feedback on the report? Share it here.
dunxen
dunxen
approved these changes
Mar 20, 2024
View reviewed changes
@dunxendunxen
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
CI failures unrelated and on main. Do we have an issue tracking them?
tnull
tnull
reviewed
Mar 20, 2024
View reviewed changes
@tnulltnull
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, although in this context I'd like to (briefly) raise the question again whether we should relax some of the announcement checks for the introduction node to also support BOLT12 between two directly-connected nodes, i.e., something along the lines of tnull@8138bec
If we're really concerned about the privacy leakage, it would be nice to at least expose a config flag allowing to do this, as otherwise it's pretty confusing for users why they could send payments via an intermediate node, but not directly.
@TheBlueMatt
TheBlueMatt
commented
Mar 20, 2024

Copy link


Copy Markdown

Collaborator
Can you open an issue? That seems at least marginally unrelated and I'd like to better understand the failure cases and don't want to derail this PR.
@TheBlueMatt
TheBlueMatt
merged commit 0cc0858
into
lightningdevkit:mainMar 20, 2024
@tnull
tnull
commented
Mar 20, 2024

Copy link


Copy Markdown

Contributor
Can you open an issue? That seems at least marginally unrelated and I'd like to better understand the failure cases and don't want to derail this PR.
Yes, sorry, this was definitely not my intention. Now tracking here: #2952
@jkczyzjkczyz
mentioned this pull request
Mar 20, 2024

Closed

9 tasks
@valentinewallacevalentinewallace
mentioned this pull request
May 1, 2024
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@tnulltnulltnull left review comments
@TheBlueMattTheBlueMattTheBlueMatt approved these changes
@dunxendunxendunxen approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@jkczyz@codecov-commenter@TheBlueMatt@tnull@dunxen

, '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); }
})();
})();
Fix sender is the introduction node onion messages by jkczyz · Pull Request #2951 · lightningdevkit/rust-lightning · GitHub
Skip to content
Fix sender is the introduction node onion messages - #2951
Merged
TheBlueMatt merged 6 commits into
lightningdevkit:mainfrom 
jkczyz:2024-03-fix-sender-is-intro-node
Mar 20, 2024
Merged
Fix sender is the introduction node onion messages#2951
TheBlueMatt merged 6 commits into
lightningdevkit:mainfrom 
jkczyz:2024-03-fix-sender-is-intro-node
Conversation
@jkczyz
@jkczyzjkczyz
commented
Mar 19, 2024

Copy link


Copy Markdown

Contributor
DefaultMessageRouter will form an OnionMessagePath from a BlindedPath where the sender is the introduction node but only if the sender is announced. If the sender is unannounced, then DefaultMessageRouter will fail. While DefaultMessageRouter will only create a blinded path with an announced introduction node, it may receive one where the introduction node is unannounced. Don't return an error in this case, as the OnionMessenger can advance the blinded path by one hop.
This may occur when two nodes have an unannounced channel and one (the offer creator) wants to use it for payments without an intermediary node and without putting its node id in the offer.
@jkczyz
Re-order imports
22c9da3
@jkczyz
Include SocketAddress in test node_announcement
dc627b3
@jkczyz
Fix sender is the introduction node onion messages
f2b84e0
DefaultMessageRouter will form an OnionMessagePath from a BlindedPath
where the sender is the introduction node but only if the sender is
announced. If the sender is unannounced, then DefaultMessageRouter will
fail. While DefaultMessageRouter will only create a blinded path with an
announced introduction node, it may receive one where the introduction
node is unannounced. Don't return an error in this case, as the
OnionMessenger can advance the blinded path by one hop.
This may occur when two nodes have an unannounced channel and one (the
offer creator) wants to use it for payments without an intermediary node
and without putting its node id in the offer.
@jkczyz
pub(crate) visibility for routing/test_utils.rs
3d2a0c2
Give pub(crate) visibility to some routing test utilities to facilitate
testing DefaultMessageRouter in functional tests.
@jkczyz
Use DefaultMessageRouter in functional tests
ad3de23
This helps test cases in DefaultMessageRouter that may not be exercised
now or in the future.
@jkczyz
Use OnionMessenger::send_onion_message in tests
806fef5
Use OnionMessenger's public interface in tests whenever possible (i.e.,
when not using any intermediate_nodes in an OnionMessagePath. This
allows us to exercise DefaultMessageRouter, and, in particular that a
path can be found for an unannounced sender when its in the introduction
node.
@codecov-commenter
codecov-commenter
commented
Mar 19, 2024

Copy link


Copy Markdown

Codecov Report
All modified and coverable lines are covered by tests ✅
Project coverage is 89.62%. Comparing base (670b41a) to head (806fef5).

Report is 33 commits behind head on main.
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files
@@ Coverage Diff @@## main #2951 +/- ##
==========================================
+ Coverage 89.11% 89.62% +0.51% 
==========================================
Files 117 117 Lines 95029 97762 +2733 Branches 95029 97762 +2733 ==========================================
+ Hits 84685 87621 +2936 + Misses 7856 7692 -164 + Partials 2488 2449 -39 
☔ View full report in Codecov by Sentry.

📢 Have feedback on the report? Share it here.
dunxen
dunxen
approved these changes
Mar 20, 2024
View reviewed changes
@dunxendunxen
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
CI failures unrelated and on main. Do we have an issue tracking them?
tnull
tnull
reviewed
Mar 20, 2024
View reviewed changes
@tnulltnull
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, although in this context I'd like to (briefly) raise the question again whether we should relax some of the announcement checks for the introduction node to also support BOLT12 between two directly-connected nodes, i.e., something along the lines of tnull@8138bec
If we're really concerned about the privacy leakage, it would be nice to at least expose a config flag allowing to do this, as otherwise it's pretty confusing for users why they could send payments via an intermediate node, but not directly.
@TheBlueMatt
TheBlueMatt
commented
Mar 20, 2024

Copy link


Copy Markdown

Collaborator
Can you open an issue? That seems at least marginally unrelated and I'd like to better understand the failure cases and don't want to derail this PR.
@TheBlueMatt
TheBlueMatt
merged commit 0cc0858
into
lightningdevkit:mainMar 20, 2024
@tnull
tnull
commented
Mar 20, 2024

Copy link


Copy Markdown

Contributor
Can you open an issue? That seems at least marginally unrelated and I'd like to better understand the failure cases and don't want to derail this PR.
Yes, sorry, this was definitely not my intention. Now tracking here: #2952
@jkczyzjkczyz
mentioned this pull request
Mar 20, 2024

Closed

9 tasks
@valentinewallacevalentinewallace
mentioned this pull request
May 1, 2024
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@tnulltnulltnull left review comments
@TheBlueMattTheBlueMattTheBlueMatt approved these changes
@dunxendunxendunxen approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@jkczyz@codecov-commenter@TheBlueMatt@tnull@dunxen