') + ')', '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('^' + ".*" + ', '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" + ', '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('^' + ".*" + ', '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); } })(); })(); Deduplicate `PendingHTLCsForwardable` events on generation by valentinewallace · Pull Request #2026 · lightningdevkit/rust-lightning · GitHub
Skip to content

Deduplicate PendingHTLCsForwardable events on generation - #2026

Merged
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom
valentinewallace:2023-02-dedup-pending-forwardable-evs
Feb 19, 2023
Merged

Deduplicate PendingHTLCsForwardable events on generation#2026
TheBlueMatt merged 3 commits into
lightningdevkit:mainfrom
valentinewallace:2023-02-dedup-pending-forwardable-evs

Conversation

@valentinewallace

@valentinewallacevalentinewallace commented Feb 9, 2023

Copy link
Copy Markdown
Contributor

Ensure that when we generate a PendingHTLCsForwardable event, there isn't one already queued.

Completes the follow-ups from #1916.

Partially addresses #1932.

@wpaulinowpaulino added this to the 0.0.114 milestone Feb 10, 2023
@valentinewallace

valentinewallace commented Feb 11, 2023

Copy link
Copy Markdown
ContributorAuthor

On second glance this needs more work. Will update soon.

@valentinewallace
valentinewallace marked this pull request as draft February 11, 2023 01:47
@valentinewallace
valentinewallaceforce-pushed the 2023-02-dedup-pending-forwardable-evs branch from 9644d2c to 6074e8aCompareFebruary 11, 2023 20:46
@codecov-commenter

codecov-commenter commented Feb 11, 2023•
edited
Loading

Copy link
Copy Markdown

Codecov Report

Base: 87.22% // Head: 87.20% // Decreases project coverage by -0.02%⚠️

Coverage data is based on head (a2489b1) compared to base (2f36c92).
Patch coverage: 88.31% of modified lines in pull request are covered.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@ Coverage Diff @@## main #2026 +/- ##
==========================================
- Coverage 87.22% 87.20% -0.02% 
==========================================
Files 100 101 +1 Lines 44051 44175 +124 Branches 44051 44175 +124 ==========================================
+ Hits 38424 38524 +100 - Misses 5627 5651 +24
Impacted FilesCoverage Δ
lightning/src/ln/functional_test_utils.rs88.43% <0.00%> (-0.07%)⬇️
lightning/src/sync/fairrwlock.rs75.00% <0.00%> (ø)
lightning/src/sync/mod.rs50.00% <50.00%> (ø)
lightning/src/ln/outbound_payment.rs79.96% <76.47%> (-0.33%)⬇️
lightning/src/ln/payment_tests.rs95.65% <90.90%> (-0.29%)⬇️
lightning/src/sync/debug_sync.rs80.00% <92.85%> (-0.35%)⬇️
lightning/src/ln/channelmanager.rs85.81% <94.73%> (-0.04%)⬇️
lightning/src/sync/nostd_sync.rs37.50% <100.00%> (+2.20%)⬆️
lightning/src/sync/test_lockorder_checks.rs100.00% <100.00%> (ø)
lightning/src/util/test_utils.rs66.73% <100.00%> (ø)
... and 12 more

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

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@valentinewallace
valentinewallaceforce-pushed the 2023-02-dedup-pending-forwardable-evs branch from 6074e8a to 6e7c91eCompareFebruary 11, 2023 20:50
@valentinewallace

valentinewallace commented Feb 11, 2023

Copy link
Copy Markdown
ContributorAuthor

Ended up having to pull in some changes from #2008 so I based it on that.

@valentinewallace
valentinewallace marked this pull request as ready for review February 11, 2023 20:54
TheBlueMatt
TheBlueMatt reviewed Feb 15, 2023
View reviewed changes
Comment threadlightning/src/ln/outbound_payment.rs Outdated
@valentinewallace
valentinewallaceforce-pushed the 2023-02-dedup-pending-forwardable-evs branch from 6e7c91e to 2dcb379CompareFebruary 16, 2023 16:20
dunxen
dunxen reviewed Feb 16, 2023
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.

Tentative LGTM, but I need to do more digging on the retry stuff to understand abandoning in 2dcb379

Comment threadlightning/src/ln/channelmanager.rs
fn push_pending_forwards_ev(&self) {
let mut pending_events = self.pending_events.lock().unwrap();
let forward_ev_exists = pending_events.iter()
.find(|ev| if let events::Event::PendingHTLCsForwardable { .. } = ev { true } else { false })

@dunxendunxenFeb 16, 2023

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.

Lol if our MSRV was just one minor version up (1.42) we could just

.find(|ev| matches!(ev, events::Event::PendingHTLCsForwardable))

@valentinewallacevalentinewallaceFeb 17, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Soclose 🥲

@TheBlueMatt

TheBlueMatt commented Feb 16, 2023

Copy link
Copy Markdown
Collaborator

Needs rebase.

@valentinewallace
valentinewallaceforce-pushed the 2023-02-dedup-pending-forwardable-evs branch from 2dcb379 to 91be2ccCompareFebruary 17, 2023 16:02
TheBlueMatt
TheBlueMatt reviewed Feb 17, 2023
View reviewed changes
Comment threadlightning/src/ln/channelmanager.rs
// We only want to push a PendingHTLCsForwardable event if no others are queued.
fn push_pending_forwards_ev(&self) {
let mut pending_events = self.pending_events.lock().unwrap();
let forward_ev_exists = pending_events.iter()

@TheBlueMattTheBlueMattFeb 17, 2023

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.

Oh, oops, this check isn't sufficient - I mean it reduces duplication, but the issue is we send the user a PendingHTLCsForwardable event, then they wait (during which time we don't have one lying around here) and then they call pending_htlcs_forwardable. Avoiding duplicating entirely is gonna be a bit hard, but I think we could at least only have two in-flight ones at once by filtering the outbound_payment-generated one on needs_abandon (but under the same lock as the update in fail_htlc).

@valentinewallacevalentinewallaceFeb 17, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Ok I see the case we're not covering. I don't think we'd want needs_abandon, though, seems we'd want a new needs_retry check IIUC. Will add this. (Note that needs_abandon should only apply to restart since we set all payments as non-retryable on deser.)

@valentinewallace
valentinewallaceforce-pushed the 2023-02-dedup-pending-forwardable-evs branch from 91be2cc to a2489b1CompareFebruary 17, 2023 22:43
@TheBlueMatt

TheBlueMatt commented Feb 19, 2023

Copy link
Copy Markdown
Collaborator

Thanks, LGTM, I think.

@TheBlueMatt
TheBlueMatt merged commit 558b2f2 into lightningdevkit:mainFeb 19, 2023
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TheBlueMattTheBlueMattTheBlueMatt approved these changes
@dunxendunxendunxen approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

0.0.114

Development

Successfully merging this pull request may close these issues.

5 participants

@valentinewallace@codecov-commenter@TheBlueMatt@dunxen@wpaulino