PeerManager: add separate queue for gossip broadcasts - #1683

Merged
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom
valentinewallace:2022-08-multiqueue-peerman
Aug 25, 2022
Merged

PeerManager: add separate queue for gossip broadcasts#1683
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom
valentinewallace:2022-08-multiqueue-peerman

Conversation

@valentinewallace

@valentinewallacevalentinewallace commented Aug 24, 2022•
edited
Loading

Copy link
Copy Markdown
Contributor

This allows us to better prioritize channel messages over gossip broadcasts and lays groundwork for rate limiting onion messages more simply, since they won't be competing with gossip broadcasts in the channel message queue.

Also fixes a bug that was introduced in #1660, oops.

  • docs

Lays groundwork for #1604

@valentinewallace
valentinewallaceforce-pushed the 2022-08-multiqueue-peerman branch from 5f4e243 to 758561dCompareAugust 24, 2022 22:18
TheBlueMatt
TheBlueMatt reviewed Aug 24, 2022
View reviewed changes

@TheBlueMattTheBlueMatt left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nice, thanks.

Comment threadlightning/src/ln/peer_handler.rs Outdated
Comment threadlightning/src/ln/peer_handler.rs Outdated
dunxen
dunxen reviewed Aug 25, 2022
View reviewed changes
Comment threadlightning/src/ln/peer_handler.rs Outdated
@valentinewallace
valentinewallaceforce-pushed the 2022-08-multiqueue-peerman branch from 758561d to 91b113dCompareAugust 25, 2022 14:56
TheBlueMatt
TheBlueMatt reviewed Aug 25, 2022
View reviewed changes

@TheBlueMattTheBlueMatt left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM. Can we now drop the enqueue_encoded_message utility now and only use the gossip variant?

@valentinewallace
valentinewallaceforce-pushed the 2022-08-multiqueue-peerman branch from 91b113d to 50a417dCompareAugust 25, 2022 16:39
@valentinewallace
valentinewallace marked this pull request as ready for review August 25, 2022 16:40
@valentinewallace
valentinewallaceforce-pushed the 2022-08-multiqueue-peerman branch from 50a417d to 0fce80dCompareAugust 25, 2022 16:41
@codecov-commenter

codecov-commenter commented Aug 25, 2022•
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #1683 (47e818f) into main (414d8e1) will decrease coverage by 0.05%.
The diff coverage is 46.42%.

@@ Coverage Diff @@## main #1683 +/- ##
==========================================
- Coverage 90.91% 90.85% -0.06% 
==========================================
Files 85 85 Lines 45753 45763 +10 Branches 45753 45763 +10 ==========================================
- Hits 41596 41580 -16 - Misses 4157 4183 +26
Impacted FilesCoverage Δ
lightning/src/ln/peer_handler.rs56.77% <46.42%> (-0.06%)⬇️
lightning/src/chain/onchaintx.rs94.02% <0.00%> (-0.92%)⬇️
lightning/src/ln/payment_tests.rs98.57% <0.00%> (-0.32%)⬇️
lightning/src/ln/functional_tests.rs96.84% <0.00%> (-0.20%)⬇️
lightning/src/ln/monitor_tests.rs99.35% <0.00%> (-0.13%)⬇️
lightning/src/ln/channelmanager.rs85.13% <0.00%> (-0.03%)⬇️

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

@jkczyz
jkczyz self-requested a review August 25, 2022 17:32
jkczyz
jkczyz reviewed Aug 25, 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.

Looks good but should make commits atomic as per comment.

Comment threadlightning/src/ln/peer_handler.rs
@valentinewallace
valentinewallaceforce-pushed the 2022-08-multiqueue-peerman branch from 0fce80d to 9b3fe9eCompareAugust 25, 2022 17:59
@valentinewallacevalentinewallace mentioned this pull request Aug 25, 2022
Merged
@TheBlueMatt

TheBlueMatt commented Aug 25, 2022

Copy link
Copy Markdown
Collaborator

If you're referring to another PR in the git commit, please refer to the specific commit by commit hash, not to github - git history should stand on its own without context from github.

@valentinewallace
PeerMan: fix bug in drop_gossip util
1de698f
Fixes a flipped bool that was introduced in
4a1ee5f
@valentinewallace
PeerMan: rename drop_gossip util to be more accurate
ab149dc
It's more accurate to name it as dropping gossip broadcasts, as it won't drop
all gossip.
@valentinewallace
Separate gossip broadcasts into their own queue in PeerManager
47e818f
This allows us to better prioritize channel messages over gossip broadcasts and
lays groundwork for rate limiting onion messages more simply, since they won't
be competing with gossip broadcasts for space in the main message queue.
@valentinewallace
valentinewallaceforce-pushed the 2022-08-multiqueue-peerman branch from 9b3fe9e to 47e818fCompareAugust 25, 2022 18:58
@TheBlueMatt
TheBlueMatt merged commit 2358b0b into lightningdevkit:mainAug 25, 2022
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dunxendunxendunxen left review comments
@TheBlueMattTheBlueMattTheBlueMatt approved these changes
@jkczyzjkczyzjkczyz 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

@valentinewallace@codecov-commenter@TheBlueMatt@dunxen@jkczyz
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n 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;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} 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
PeerManager: add separate queue for gossip broadcasts - #1683
Merged
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
valentinewallace:2022-08-multiqueue-peerman
Aug 25, 2022
Merged
PeerManager: add separate queue for gossip broadcasts#1683
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
valentinewallace:2022-08-multiqueue-peerman
Conversation
@valentinewallace
@valentinewallacevalentinewallace
commented
Aug 24, 2022•

edited

Loading

Copy link


Copy Markdown

Contributor
This allows us to better prioritize channel messages over gossip broadcasts and lays groundwork for rate limiting onion messages more simply, since they won't be competing with gossip broadcasts in the channel message queue.
Also fixes a bug that was introduced in #1660, oops.
  •  docs
Lays groundwork for #1604
@valentinewallace
valentinewallaceforce-pushed
the
2022-08-multiqueue-peerman
branch
from
5f4e243 to
758561dCompareAugust 24, 2022 22:18
TheBlueMatt
TheBlueMatt
reviewed
Aug 24, 2022
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

Nice, thanks.
Comment threadlightning/src/ln/peer_handler.rs
Outdated

Comment threadlightning/src/ln/peer_handler.rs
Outdated

dunxen
dunxen
reviewed
Aug 25, 2022
View reviewed changes
Comment threadlightning/src/ln/peer_handler.rs
Outdated

@valentinewallace
valentinewallaceforce-pushed
the
2022-08-multiqueue-peerman
branch
from
758561d to
91b113dCompareAugust 25, 2022 14:56
TheBlueMatt
TheBlueMatt
reviewed
Aug 25, 2022
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

LGTM. Can we now drop the enqueue_encoded_message utility now and only use the gossip variant?
@valentinewallace
valentinewallaceforce-pushed
the
2022-08-multiqueue-peerman
branch
from
91b113d to
50a417dCompareAugust 25, 2022 16:39
@valentinewallace
valentinewallace
marked this pull request as ready for review
August 25, 2022 16:40
@valentinewallace
valentinewallaceforce-pushed
the
2022-08-multiqueue-peerman
branch
from
50a417d to
0fce80dCompareAugust 25, 2022 16:41
@codecov-commenter
codecov-commenter
commented
Aug 25, 2022•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #1683 (47e818f) into main (414d8e1) will decrease coverage by 0.05%.

The diff coverage is 46.42%.
@@ Coverage Diff @@## main #1683 +/- ##
==========================================
- Coverage 90.91% 90.85% -0.06% 
==========================================
Files 85 85 Lines 45753 45763 +10 Branches 45753 45763 +10 ==========================================
- Hits 41596 41580 -16 - Misses 4157 4183 +26 
Impacted FilesCoverage Δ
lightning/src/ln/peer_handler.rs56.77% <46.42%> (-0.06%)⬇️
lightning/src/chain/onchaintx.rs94.02% <0.00%> (-0.92%)⬇️
lightning/src/ln/payment_tests.rs98.57% <0.00%> (-0.32%)⬇️
lightning/src/ln/functional_tests.rs96.84% <0.00%> (-0.20%)⬇️
lightning/src/ln/monitor_tests.rs99.35% <0.00%> (-0.13%)⬇️
lightning/src/ln/channelmanager.rs85.13% <0.00%> (-0.03%)⬇️
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
@jkczyz
jkczyz
self-requested a review
August 25, 2022 17:32
jkczyz
jkczyz
reviewed
Aug 25, 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.

Looks good but should make commits atomic as per comment.
Comment threadlightning/src/ln/peer_handler.rs
@valentinewallace
valentinewallaceforce-pushed
the
2022-08-multiqueue-peerman
branch
from
0fce80d to
9b3fe9eCompareAugust 25, 2022 17:59
@valentinewallacevalentinewallace
mentioned this pull request
Aug 25, 2022
 Merged

@TheBlueMatt
TheBlueMatt
commented
Aug 25, 2022

Copy link


Copy Markdown

Collaborator
If you're referring to another PR in the git commit, please refer to the specific commit by commit hash, not to github - git history should stand on its own without context from github.
@valentinewallace
PeerMan: fix bug in drop_gossip util
1de698f
Fixes a flipped bool that was introduced in
4a1ee5f
@valentinewallace
PeerMan: rename drop_gossip util to be more accurate
ab149dc
It's more accurate to name it as dropping gossip broadcasts, as it won't drop
all gossip.
@valentinewallace
Separate gossip broadcasts into their own queue in PeerManager
47e818f
This allows us to better prioritize channel messages over gossip broadcasts and
lays groundwork for rate limiting onion messages more simply, since they won't
be competing with gossip broadcasts for space in the main message queue.
@valentinewallace
valentinewallaceforce-pushed
the
2022-08-multiqueue-peerman
branch
from
9b3fe9e to
47e818fCompareAugust 25, 2022 18:58
@TheBlueMatt
TheBlueMatt
merged commit 2358b0b
into
lightningdevkit:mainAug 25, 2022
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@dunxendunxendunxen left review comments
@TheBlueMattTheBlueMattTheBlueMatt approved these changes
@jkczyzjkczyzjkczyz 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

@valentinewallace@codecov-commenter@TheBlueMatt@dunxen@jkczyz

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

Skip to content
PeerManager: add separate queue for gossip broadcasts - #1683
Merged
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
valentinewallace:2022-08-multiqueue-peerman
Aug 25, 2022
Merged
PeerManager: add separate queue for gossip broadcasts#1683
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
valentinewallace:2022-08-multiqueue-peerman
Conversation
@valentinewallace
@valentinewallacevalentinewallace
commented
Aug 24, 2022•

edited

Loading

Copy link


Copy Markdown

Contributor
This allows us to better prioritize channel messages over gossip broadcasts and lays groundwork for rate limiting onion messages more simply, since they won't be competing with gossip broadcasts in the channel message queue.
Also fixes a bug that was introduced in #1660, oops.
  •  docs
Lays groundwork for #1604
@valentinewallace
valentinewallaceforce-pushed
the
2022-08-multiqueue-peerman
branch
from
5f4e243 to
758561dCompareAugust 24, 2022 22:18
TheBlueMatt
TheBlueMatt
reviewed
Aug 24, 2022
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

Nice, thanks.
Comment threadlightning/src/ln/peer_handler.rs
Outdated

Comment threadlightning/src/ln/peer_handler.rs
Outdated

dunxen
dunxen
reviewed
Aug 25, 2022
View reviewed changes
Comment threadlightning/src/ln/peer_handler.rs
Outdated

@valentinewallace
valentinewallaceforce-pushed
the
2022-08-multiqueue-peerman
branch
from
758561d to
91b113dCompareAugust 25, 2022 14:56
TheBlueMatt
TheBlueMatt
reviewed
Aug 25, 2022
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

LGTM. Can we now drop the enqueue_encoded_message utility now and only use the gossip variant?
@valentinewallace
valentinewallaceforce-pushed
the
2022-08-multiqueue-peerman
branch
from
91b113d to
50a417dCompareAugust 25, 2022 16:39
@valentinewallace
valentinewallace
marked this pull request as ready for review
August 25, 2022 16:40
@valentinewallace
valentinewallaceforce-pushed
the
2022-08-multiqueue-peerman
branch
from
50a417d to
0fce80dCompareAugust 25, 2022 16:41
@codecov-commenter
codecov-commenter
commented
Aug 25, 2022•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #1683 (47e818f) into main (414d8e1) will decrease coverage by 0.05%.

The diff coverage is 46.42%.
@@ Coverage Diff @@## main #1683 +/- ##
==========================================
- Coverage 90.91% 90.85% -0.06% 
==========================================
Files 85 85 Lines 45753 45763 +10 Branches 45753 45763 +10 ==========================================
- Hits 41596 41580 -16 - Misses 4157 4183 +26 
Impacted FilesCoverage Δ
lightning/src/ln/peer_handler.rs56.77% <46.42%> (-0.06%)⬇️
lightning/src/chain/onchaintx.rs94.02% <0.00%> (-0.92%)⬇️
lightning/src/ln/payment_tests.rs98.57% <0.00%> (-0.32%)⬇️
lightning/src/ln/functional_tests.rs96.84% <0.00%> (-0.20%)⬇️
lightning/src/ln/monitor_tests.rs99.35% <0.00%> (-0.13%)⬇️
lightning/src/ln/channelmanager.rs85.13% <0.00%> (-0.03%)⬇️
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
@jkczyz
jkczyz
self-requested a review
August 25, 2022 17:32
jkczyz
jkczyz
reviewed
Aug 25, 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.

Looks good but should make commits atomic as per comment.
Comment threadlightning/src/ln/peer_handler.rs
@valentinewallace
valentinewallaceforce-pushed
the
2022-08-multiqueue-peerman
branch
from
0fce80d to
9b3fe9eCompareAugust 25, 2022 17:59
@valentinewallacevalentinewallace
mentioned this pull request
Aug 25, 2022
 Merged

@TheBlueMatt
TheBlueMatt
commented
Aug 25, 2022

Copy link


Copy Markdown

Collaborator
If you're referring to another PR in the git commit, please refer to the specific commit by commit hash, not to github - git history should stand on its own without context from github.
@valentinewallace
PeerMan: fix bug in drop_gossip util
1de698f
Fixes a flipped bool that was introduced in
4a1ee5f
@valentinewallace
PeerMan: rename drop_gossip util to be more accurate
ab149dc
It's more accurate to name it as dropping gossip broadcasts, as it won't drop
all gossip.
@valentinewallace
Separate gossip broadcasts into their own queue in PeerManager
47e818f
This allows us to better prioritize channel messages over gossip broadcasts and
lays groundwork for rate limiting onion messages more simply, since they won't
be competing with gossip broadcasts for space in the main message queue.
@valentinewallace
valentinewallaceforce-pushed
the
2022-08-multiqueue-peerman
branch
from
9b3fe9e to
47e818fCompareAugust 25, 2022 18:58
@TheBlueMatt
TheBlueMatt
merged commit 2358b0b
into
lightningdevkit:mainAug 25, 2022
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@dunxendunxendunxen left review comments
@TheBlueMattTheBlueMattTheBlueMatt approved these changes
@jkczyzjkczyzjkczyz 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

@valentinewallace@codecov-commenter@TheBlueMatt@dunxen@jkczyz

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

Skip to content
PeerManager: add separate queue for gossip broadcasts - #1683
Merged
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
valentinewallace:2022-08-multiqueue-peerman
Aug 25, 2022
Merged
PeerManager: add separate queue for gossip broadcasts#1683
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
valentinewallace:2022-08-multiqueue-peerman
Conversation
@valentinewallace
@valentinewallacevalentinewallace
commented
Aug 24, 2022•

edited

Loading

Copy link


Copy Markdown

Contributor
This allows us to better prioritize channel messages over gossip broadcasts and lays groundwork for rate limiting onion messages more simply, since they won't be competing with gossip broadcasts in the channel message queue.
Also fixes a bug that was introduced in #1660, oops.
  •  docs
Lays groundwork for #1604
@valentinewallace
valentinewallaceforce-pushed
the
2022-08-multiqueue-peerman
branch
from
5f4e243 to
758561dCompareAugust 24, 2022 22:18
TheBlueMatt
TheBlueMatt
reviewed
Aug 24, 2022
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

Nice, thanks.
Comment threadlightning/src/ln/peer_handler.rs
Outdated

Comment threadlightning/src/ln/peer_handler.rs
Outdated

dunxen
dunxen
reviewed
Aug 25, 2022
View reviewed changes
Comment threadlightning/src/ln/peer_handler.rs
Outdated

@valentinewallace
valentinewallaceforce-pushed
the
2022-08-multiqueue-peerman
branch
from
758561d to
91b113dCompareAugust 25, 2022 14:56
TheBlueMatt
TheBlueMatt
reviewed
Aug 25, 2022
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

LGTM. Can we now drop the enqueue_encoded_message utility now and only use the gossip variant?
@valentinewallace
valentinewallaceforce-pushed
the
2022-08-multiqueue-peerman
branch
from
91b113d to
50a417dCompareAugust 25, 2022 16:39
@valentinewallace
valentinewallace
marked this pull request as ready for review
August 25, 2022 16:40
@valentinewallace
valentinewallaceforce-pushed
the
2022-08-multiqueue-peerman
branch
from
50a417d to
0fce80dCompareAugust 25, 2022 16:41
@codecov-commenter
codecov-commenter
commented
Aug 25, 2022•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #1683 (47e818f) into main (414d8e1) will decrease coverage by 0.05%.

The diff coverage is 46.42%.
@@ Coverage Diff @@## main #1683 +/- ##
==========================================
- Coverage 90.91% 90.85% -0.06% 
==========================================
Files 85 85 Lines 45753 45763 +10 Branches 45753 45763 +10 ==========================================
- Hits 41596 41580 -16 - Misses 4157 4183 +26 
Impacted FilesCoverage Δ
lightning/src/ln/peer_handler.rs56.77% <46.42%> (-0.06%)⬇️
lightning/src/chain/onchaintx.rs94.02% <0.00%> (-0.92%)⬇️
lightning/src/ln/payment_tests.rs98.57% <0.00%> (-0.32%)⬇️
lightning/src/ln/functional_tests.rs96.84% <0.00%> (-0.20%)⬇️
lightning/src/ln/monitor_tests.rs99.35% <0.00%> (-0.13%)⬇️
lightning/src/ln/channelmanager.rs85.13% <0.00%> (-0.03%)⬇️
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
@jkczyz
jkczyz
self-requested a review
August 25, 2022 17:32
jkczyz
jkczyz
reviewed
Aug 25, 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.

Looks good but should make commits atomic as per comment.
Comment threadlightning/src/ln/peer_handler.rs
@valentinewallace
valentinewallaceforce-pushed
the
2022-08-multiqueue-peerman
branch
from
0fce80d to
9b3fe9eCompareAugust 25, 2022 17:59
@valentinewallacevalentinewallace
mentioned this pull request
Aug 25, 2022
 Merged

@TheBlueMatt
TheBlueMatt
commented
Aug 25, 2022

Copy link


Copy Markdown

Collaborator
If you're referring to another PR in the git commit, please refer to the specific commit by commit hash, not to github - git history should stand on its own without context from github.
@valentinewallace
PeerMan: fix bug in drop_gossip util
1de698f
Fixes a flipped bool that was introduced in
4a1ee5f
@valentinewallace
PeerMan: rename drop_gossip util to be more accurate
ab149dc
It's more accurate to name it as dropping gossip broadcasts, as it won't drop
all gossip.
@valentinewallace
Separate gossip broadcasts into their own queue in PeerManager
47e818f
This allows us to better prioritize channel messages over gossip broadcasts and
lays groundwork for rate limiting onion messages more simply, since they won't
be competing with gossip broadcasts for space in the main message queue.
@valentinewallace
valentinewallaceforce-pushed
the
2022-08-multiqueue-peerman
branch
from
9b3fe9e to
47e818fCompareAugust 25, 2022 18:58
@TheBlueMatt
TheBlueMatt
merged commit 2358b0b
into
lightningdevkit:mainAug 25, 2022
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@dunxendunxendunxen left review comments
@TheBlueMattTheBlueMattTheBlueMatt approved these changes
@jkczyzjkczyzjkczyz 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

@valentinewallace@codecov-commenter@TheBlueMatt@dunxen@jkczyz

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} 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
PeerManager: add separate queue for gossip broadcasts - #1683
Merged
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
valentinewallace:2022-08-multiqueue-peerman
Aug 25, 2022
Merged
PeerManager: add separate queue for gossip broadcasts#1683
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
valentinewallace:2022-08-multiqueue-peerman
Conversation
@valentinewallace
@valentinewallacevalentinewallace
commented
Aug 24, 2022•

edited

Loading

Copy link


Copy Markdown

Contributor
This allows us to better prioritize channel messages over gossip broadcasts and lays groundwork for rate limiting onion messages more simply, since they won't be competing with gossip broadcasts in the channel message queue.
Also fixes a bug that was introduced in #1660, oops.
  •  docs
Lays groundwork for #1604
@valentinewallace
valentinewallaceforce-pushed
the
2022-08-multiqueue-peerman
branch
from
5f4e243 to
758561dCompareAugust 24, 2022 22:18
TheBlueMatt
TheBlueMatt
reviewed
Aug 24, 2022
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

Nice, thanks.
Comment threadlightning/src/ln/peer_handler.rs
Outdated

Comment threadlightning/src/ln/peer_handler.rs
Outdated

dunxen
dunxen
reviewed
Aug 25, 2022
View reviewed changes
Comment threadlightning/src/ln/peer_handler.rs
Outdated

@valentinewallace
valentinewallaceforce-pushed
the
2022-08-multiqueue-peerman
branch
from
758561d to
91b113dCompareAugust 25, 2022 14:56
TheBlueMatt
TheBlueMatt
reviewed
Aug 25, 2022
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

LGTM. Can we now drop the enqueue_encoded_message utility now and only use the gossip variant?
@valentinewallace
valentinewallaceforce-pushed
the
2022-08-multiqueue-peerman
branch
from
91b113d to
50a417dCompareAugust 25, 2022 16:39
@valentinewallace
valentinewallace
marked this pull request as ready for review
August 25, 2022 16:40
@valentinewallace
valentinewallaceforce-pushed
the
2022-08-multiqueue-peerman
branch
from
50a417d to
0fce80dCompareAugust 25, 2022 16:41
@codecov-commenter
codecov-commenter
commented
Aug 25, 2022•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #1683 (47e818f) into main (414d8e1) will decrease coverage by 0.05%.

The diff coverage is 46.42%.
@@ Coverage Diff @@## main #1683 +/- ##
==========================================
- Coverage 90.91% 90.85% -0.06% 
==========================================
Files 85 85 Lines 45753 45763 +10 Branches 45753 45763 +10 ==========================================
- Hits 41596 41580 -16 - Misses 4157 4183 +26 
Impacted FilesCoverage Δ
lightning/src/ln/peer_handler.rs56.77% <46.42%> (-0.06%)⬇️
lightning/src/chain/onchaintx.rs94.02% <0.00%> (-0.92%)⬇️
lightning/src/ln/payment_tests.rs98.57% <0.00%> (-0.32%)⬇️
lightning/src/ln/functional_tests.rs96.84% <0.00%> (-0.20%)⬇️
lightning/src/ln/monitor_tests.rs99.35% <0.00%> (-0.13%)⬇️
lightning/src/ln/channelmanager.rs85.13% <0.00%> (-0.03%)⬇️
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
@jkczyz
jkczyz
self-requested a review
August 25, 2022 17:32
jkczyz
jkczyz
reviewed
Aug 25, 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.

Looks good but should make commits atomic as per comment.
Comment threadlightning/src/ln/peer_handler.rs
@valentinewallace
valentinewallaceforce-pushed
the
2022-08-multiqueue-peerman
branch
from
0fce80d to
9b3fe9eCompareAugust 25, 2022 17:59
@valentinewallacevalentinewallace
mentioned this pull request
Aug 25, 2022
 Merged

@TheBlueMatt
TheBlueMatt
commented
Aug 25, 2022

Copy link


Copy Markdown

Collaborator
If you're referring to another PR in the git commit, please refer to the specific commit by commit hash, not to github - git history should stand on its own without context from github.
@valentinewallace
PeerMan: fix bug in drop_gossip util
1de698f
Fixes a flipped bool that was introduced in
4a1ee5f
@valentinewallace
PeerMan: rename drop_gossip util to be more accurate
ab149dc
It's more accurate to name it as dropping gossip broadcasts, as it won't drop
all gossip.
@valentinewallace
Separate gossip broadcasts into their own queue in PeerManager
47e818f
This allows us to better prioritize channel messages over gossip broadcasts and
lays groundwork for rate limiting onion messages more simply, since they won't
be competing with gossip broadcasts for space in the main message queue.
@valentinewallace
valentinewallaceforce-pushed
the
2022-08-multiqueue-peerman
branch
from
9b3fe9e to
47e818fCompareAugust 25, 2022 18:58
@TheBlueMatt
TheBlueMatt
merged commit 2358b0b
into
lightningdevkit:mainAug 25, 2022
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@dunxendunxendunxen left review comments
@TheBlueMattTheBlueMattTheBlueMatt approved these changes
@jkczyzjkczyzjkczyz 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

@valentinewallace@codecov-commenter@TheBlueMatt@dunxen@jkczyz

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

Skip to content
PeerManager: add separate queue for gossip broadcasts - #1683
Merged
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
valentinewallace:2022-08-multiqueue-peerman
Aug 25, 2022
Merged
PeerManager: add separate queue for gossip broadcasts#1683
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
valentinewallace:2022-08-multiqueue-peerman
Conversation
@valentinewallace
@valentinewallacevalentinewallace
commented
Aug 24, 2022•

edited

Loading

Copy link


Copy Markdown

Contributor
This allows us to better prioritize channel messages over gossip broadcasts and lays groundwork for rate limiting onion messages more simply, since they won't be competing with gossip broadcasts in the channel message queue.
Also fixes a bug that was introduced in #1660, oops.
  •  docs
Lays groundwork for #1604
@valentinewallace
valentinewallaceforce-pushed
the
2022-08-multiqueue-peerman
branch
from
5f4e243 to
758561dCompareAugust 24, 2022 22:18
TheBlueMatt
TheBlueMatt
reviewed
Aug 24, 2022
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

Nice, thanks.
Comment threadlightning/src/ln/peer_handler.rs
Outdated

Comment threadlightning/src/ln/peer_handler.rs
Outdated

dunxen
dunxen
reviewed
Aug 25, 2022
View reviewed changes
Comment threadlightning/src/ln/peer_handler.rs
Outdated

@valentinewallace
valentinewallaceforce-pushed
the
2022-08-multiqueue-peerman
branch
from
758561d to
91b113dCompareAugust 25, 2022 14:56
TheBlueMatt
TheBlueMatt
reviewed
Aug 25, 2022
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

LGTM. Can we now drop the enqueue_encoded_message utility now and only use the gossip variant?
@valentinewallace
valentinewallaceforce-pushed
the
2022-08-multiqueue-peerman
branch
from
91b113d to
50a417dCompareAugust 25, 2022 16:39
@valentinewallace
valentinewallace
marked this pull request as ready for review
August 25, 2022 16:40
@valentinewallace
valentinewallaceforce-pushed
the
2022-08-multiqueue-peerman
branch
from
50a417d to
0fce80dCompareAugust 25, 2022 16:41
@codecov-commenter
codecov-commenter
commented
Aug 25, 2022•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #1683 (47e818f) into main (414d8e1) will decrease coverage by 0.05%.

The diff coverage is 46.42%.
@@ Coverage Diff @@## main #1683 +/- ##
==========================================
- Coverage 90.91% 90.85% -0.06% 
==========================================
Files 85 85 Lines 45753 45763 +10 Branches 45753 45763 +10 ==========================================
- Hits 41596 41580 -16 - Misses 4157 4183 +26 
Impacted FilesCoverage Δ
lightning/src/ln/peer_handler.rs56.77% <46.42%> (-0.06%)⬇️
lightning/src/chain/onchaintx.rs94.02% <0.00%> (-0.92%)⬇️
lightning/src/ln/payment_tests.rs98.57% <0.00%> (-0.32%)⬇️
lightning/src/ln/functional_tests.rs96.84% <0.00%> (-0.20%)⬇️
lightning/src/ln/monitor_tests.rs99.35% <0.00%> (-0.13%)⬇️
lightning/src/ln/channelmanager.rs85.13% <0.00%> (-0.03%)⬇️
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
@jkczyz
jkczyz
self-requested a review
August 25, 2022 17:32
jkczyz
jkczyz
reviewed
Aug 25, 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.

Looks good but should make commits atomic as per comment.
Comment threadlightning/src/ln/peer_handler.rs
@valentinewallace
valentinewallaceforce-pushed
the
2022-08-multiqueue-peerman
branch
from
0fce80d to
9b3fe9eCompareAugust 25, 2022 17:59
@valentinewallacevalentinewallace
mentioned this pull request
Aug 25, 2022
 Merged

@TheBlueMatt
TheBlueMatt
commented
Aug 25, 2022

Copy link


Copy Markdown

Collaborator
If you're referring to another PR in the git commit, please refer to the specific commit by commit hash, not to github - git history should stand on its own without context from github.
@valentinewallace
PeerMan: fix bug in drop_gossip util
1de698f
Fixes a flipped bool that was introduced in
4a1ee5f
@valentinewallace
PeerMan: rename drop_gossip util to be more accurate
ab149dc
It's more accurate to name it as dropping gossip broadcasts, as it won't drop
all gossip.
@valentinewallace
Separate gossip broadcasts into their own queue in PeerManager
47e818f
This allows us to better prioritize channel messages over gossip broadcasts and
lays groundwork for rate limiting onion messages more simply, since they won't
be competing with gossip broadcasts for space in the main message queue.
@valentinewallace
valentinewallaceforce-pushed
the
2022-08-multiqueue-peerman
branch
from
9b3fe9e to
47e818fCompareAugust 25, 2022 18:58
@TheBlueMatt
TheBlueMatt
merged commit 2358b0b
into
lightningdevkit:mainAug 25, 2022
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@dunxendunxendunxen left review comments
@TheBlueMattTheBlueMattTheBlueMatt approved these changes
@jkczyzjkczyzjkczyz 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

@valentinewallace@codecov-commenter@TheBlueMatt@dunxen@jkczyz

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

Skip to content
PeerManager: add separate queue for gossip broadcasts - #1683
Merged
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
valentinewallace:2022-08-multiqueue-peerman
Aug 25, 2022
Merged
PeerManager: add separate queue for gossip broadcasts#1683
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
valentinewallace:2022-08-multiqueue-peerman
Conversation
@valentinewallace
@valentinewallacevalentinewallace
commented
Aug 24, 2022•

edited

Loading

Copy link


Copy Markdown

Contributor
This allows us to better prioritize channel messages over gossip broadcasts and lays groundwork for rate limiting onion messages more simply, since they won't be competing with gossip broadcasts in the channel message queue.
Also fixes a bug that was introduced in #1660, oops.
  •  docs
Lays groundwork for #1604
@valentinewallace
valentinewallaceforce-pushed
the
2022-08-multiqueue-peerman
branch
from
5f4e243 to
758561dCompareAugust 24, 2022 22:18
TheBlueMatt
TheBlueMatt
reviewed
Aug 24, 2022
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

Nice, thanks.
Comment threadlightning/src/ln/peer_handler.rs
Outdated

Comment threadlightning/src/ln/peer_handler.rs
Outdated

dunxen
dunxen
reviewed
Aug 25, 2022
View reviewed changes
Comment threadlightning/src/ln/peer_handler.rs
Outdated

@valentinewallace
valentinewallaceforce-pushed
the
2022-08-multiqueue-peerman
branch
from
758561d to
91b113dCompareAugust 25, 2022 14:56
TheBlueMatt
TheBlueMatt
reviewed
Aug 25, 2022
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

LGTM. Can we now drop the enqueue_encoded_message utility now and only use the gossip variant?
@valentinewallace
valentinewallaceforce-pushed
the
2022-08-multiqueue-peerman
branch
from
91b113d to
50a417dCompareAugust 25, 2022 16:39
@valentinewallace
valentinewallace
marked this pull request as ready for review
August 25, 2022 16:40
@valentinewallace
valentinewallaceforce-pushed
the
2022-08-multiqueue-peerman
branch
from
50a417d to
0fce80dCompareAugust 25, 2022 16:41
@codecov-commenter
codecov-commenter
commented
Aug 25, 2022•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #1683 (47e818f) into main (414d8e1) will decrease coverage by 0.05%.

The diff coverage is 46.42%.
@@ Coverage Diff @@## main #1683 +/- ##
==========================================
- Coverage 90.91% 90.85% -0.06% 
==========================================
Files 85 85 Lines 45753 45763 +10 Branches 45753 45763 +10 ==========================================
- Hits 41596 41580 -16 - Misses 4157 4183 +26 
Impacted FilesCoverage Δ
lightning/src/ln/peer_handler.rs56.77% <46.42%> (-0.06%)⬇️
lightning/src/chain/onchaintx.rs94.02% <0.00%> (-0.92%)⬇️
lightning/src/ln/payment_tests.rs98.57% <0.00%> (-0.32%)⬇️
lightning/src/ln/functional_tests.rs96.84% <0.00%> (-0.20%)⬇️
lightning/src/ln/monitor_tests.rs99.35% <0.00%> (-0.13%)⬇️
lightning/src/ln/channelmanager.rs85.13% <0.00%> (-0.03%)⬇️
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
@jkczyz
jkczyz
self-requested a review
August 25, 2022 17:32
jkczyz
jkczyz
reviewed
Aug 25, 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.

Looks good but should make commits atomic as per comment.
Comment threadlightning/src/ln/peer_handler.rs
@valentinewallace
valentinewallaceforce-pushed
the
2022-08-multiqueue-peerman
branch
from
0fce80d to
9b3fe9eCompareAugust 25, 2022 17:59
@valentinewallacevalentinewallace
mentioned this pull request
Aug 25, 2022
 Merged

@TheBlueMatt
TheBlueMatt
commented
Aug 25, 2022

Copy link


Copy Markdown

Collaborator
If you're referring to another PR in the git commit, please refer to the specific commit by commit hash, not to github - git history should stand on its own without context from github.
@valentinewallace
PeerMan: fix bug in drop_gossip util
1de698f
Fixes a flipped bool that was introduced in
4a1ee5f
@valentinewallace
PeerMan: rename drop_gossip util to be more accurate
ab149dc
It's more accurate to name it as dropping gossip broadcasts, as it won't drop
all gossip.
@valentinewallace
Separate gossip broadcasts into their own queue in PeerManager
47e818f
This allows us to better prioritize channel messages over gossip broadcasts and
lays groundwork for rate limiting onion messages more simply, since they won't
be competing with gossip broadcasts for space in the main message queue.
@valentinewallace
valentinewallaceforce-pushed
the
2022-08-multiqueue-peerman
branch
from
9b3fe9e to
47e818fCompareAugust 25, 2022 18:58
@TheBlueMatt
TheBlueMatt
merged commit 2358b0b
into
lightningdevkit:mainAug 25, 2022
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@dunxendunxendunxen left review comments
@TheBlueMattTheBlueMattTheBlueMatt approved these changes
@jkczyzjkczyzjkczyz 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

@valentinewallace@codecov-commenter@TheBlueMatt@dunxen@jkczyz

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

Skip to content
PeerManager: add separate queue for gossip broadcasts - #1683
Merged
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
valentinewallace:2022-08-multiqueue-peerman
Aug 25, 2022
Merged
PeerManager: add separate queue for gossip broadcasts#1683
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom 
valentinewallace:2022-08-multiqueue-peerman
Conversation
@valentinewallace
@valentinewallacevalentinewallace
commented
Aug 24, 2022•

edited

Loading

Copy link


Copy Markdown

Contributor
This allows us to better prioritize channel messages over gossip broadcasts and lays groundwork for rate limiting onion messages more simply, since they won't be competing with gossip broadcasts in the channel message queue.
Also fixes a bug that was introduced in #1660, oops.
  •  docs
Lays groundwork for #1604
@valentinewallace
valentinewallaceforce-pushed
the
2022-08-multiqueue-peerman
branch
from
5f4e243 to
758561dCompareAugust 24, 2022 22:18
TheBlueMatt
TheBlueMatt
reviewed
Aug 24, 2022
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

Nice, thanks.
Comment threadlightning/src/ln/peer_handler.rs
Outdated

Comment threadlightning/src/ln/peer_handler.rs
Outdated

dunxen
dunxen
reviewed
Aug 25, 2022
View reviewed changes
Comment threadlightning/src/ln/peer_handler.rs
Outdated

@valentinewallace
valentinewallaceforce-pushed
the
2022-08-multiqueue-peerman
branch
from
758561d to
91b113dCompareAugust 25, 2022 14:56
TheBlueMatt
TheBlueMatt
reviewed
Aug 25, 2022
View reviewed changes
@TheBlueMattTheBlueMatt
left a comment


Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

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

LGTM. Can we now drop the enqueue_encoded_message utility now and only use the gossip variant?
@valentinewallace
valentinewallaceforce-pushed
the
2022-08-multiqueue-peerman
branch
from
91b113d to
50a417dCompareAugust 25, 2022 16:39
@valentinewallace
valentinewallace
marked this pull request as ready for review
August 25, 2022 16:40
@valentinewallace
valentinewallaceforce-pushed
the
2022-08-multiqueue-peerman
branch
from
50a417d to
0fce80dCompareAugust 25, 2022 16:41
@codecov-commenter
codecov-commenter
commented
Aug 25, 2022•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #1683 (47e818f) into main (414d8e1) will decrease coverage by 0.05%.

The diff coverage is 46.42%.
@@ Coverage Diff @@## main #1683 +/- ##
==========================================
- Coverage 90.91% 90.85% -0.06% 
==========================================
Files 85 85 Lines 45753 45763 +10 Branches 45753 45763 +10 ==========================================
- Hits 41596 41580 -16 - Misses 4157 4183 +26 
Impacted FilesCoverage Δ
lightning/src/ln/peer_handler.rs56.77% <46.42%> (-0.06%)⬇️
lightning/src/chain/onchaintx.rs94.02% <0.00%> (-0.92%)⬇️
lightning/src/ln/payment_tests.rs98.57% <0.00%> (-0.32%)⬇️
lightning/src/ln/functional_tests.rs96.84% <0.00%> (-0.20%)⬇️
lightning/src/ln/monitor_tests.rs99.35% <0.00%> (-0.13%)⬇️
lightning/src/ln/channelmanager.rs85.13% <0.00%> (-0.03%)⬇️
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
@jkczyz
jkczyz
self-requested a review
August 25, 2022 17:32
jkczyz
jkczyz
reviewed
Aug 25, 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.

Looks good but should make commits atomic as per comment.
Comment threadlightning/src/ln/peer_handler.rs
@valentinewallace
valentinewallaceforce-pushed
the
2022-08-multiqueue-peerman
branch
from
0fce80d to
9b3fe9eCompareAugust 25, 2022 17:59
@valentinewallacevalentinewallace
mentioned this pull request
Aug 25, 2022
 Merged

@TheBlueMatt
TheBlueMatt
commented
Aug 25, 2022

Copy link


Copy Markdown

Collaborator
If you're referring to another PR in the git commit, please refer to the specific commit by commit hash, not to github - git history should stand on its own without context from github.
@valentinewallace
PeerMan: fix bug in drop_gossip util
1de698f
Fixes a flipped bool that was introduced in
4a1ee5f
@valentinewallace
PeerMan: rename drop_gossip util to be more accurate
ab149dc
It's more accurate to name it as dropping gossip broadcasts, as it won't drop
all gossip.
@valentinewallace
Separate gossip broadcasts into their own queue in PeerManager
47e818f
This allows us to better prioritize channel messages over gossip broadcasts and
lays groundwork for rate limiting onion messages more simply, since they won't
be competing with gossip broadcasts for space in the main message queue.
@valentinewallace
valentinewallaceforce-pushed
the
2022-08-multiqueue-peerman
branch
from
9b3fe9e to
47e818fCompareAugust 25, 2022 18:58
@TheBlueMatt
TheBlueMatt
merged commit 2358b0b
into
lightningdevkit:mainAug 25, 2022
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@dunxendunxendunxen left review comments
@TheBlueMattTheBlueMattTheBlueMatt approved these changes
@jkczyzjkczyzjkczyz 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

@valentinewallace@codecov-commenter@TheBlueMatt@dunxen@jkczyz