') + ')', '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); } })(); })(); Fix `ChannelManager::accept_inbound_channel` error handling by dunxen · Pull Request #2953 · lightningdevkit/rust-lightning · GitHub
Skip to content

Fix ChannelManager::accept_inbound_channel error handling - #2953

Merged
G8XSU merged 2 commits into
lightningdevkit:mainfrom
dunxen:2024-03-fix2941
Mar 21, 2024
Merged

Fix ChannelManager::accept_inbound_channel error handling#2953
G8XSU merged 2 commits into
lightningdevkit:mainfrom
dunxen:2024-03-fix2941

Conversation

@dunxen

Copy link
Copy Markdown
Contributor

Fixes#2941.

@TheBlueMattTheBlueMatt added this to the 0.0.122 milestone Mar 20, 2024

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

Thanks, LGTM.

Comment threadlightning/src/ln/channelmanager.rs Outdated

@G8XSUG8XSU 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!

Comment threadlightning/src/ln/channelmanager.rs
@dunxen

Copy link
Copy Markdown
ContributorAuthor

I know it's probably not ideal but I chose functional_tests.rs for now to move the test to as none of the other files made sense to pollute, but created an issue for moving tests out of channelmanager.rs (#2958). We could probably also review functional_tests.rs at some stage to thin that out too.

Change diff
$ git diff-tree -U1 7dbe57e 3206d1f
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index b2b4e76d..825d4507 100644
--- a/lightning/src/ln/channelmanager.rs+++ b/lightning/src/ln/channelmanager.rs@@ -6129,3 +6129,3 @@ where
match handle_error!(self, Result::<(), MsgHandleErrInternal>::Err(err), *counterparty_node_id) {
- Ok(_) => unreachable!("`handle_error` only returns Err"),+ Ok(_) => unreachable!("`handle_error` only returns Err as we've passed in an Err"),
Err(e) => {
@@ -6135,3 +6135,3 @@ where
}
- Ok(mut channel) => {+ Ok(mut channel) => {
if accept_0conf {
@@ -6182,3 +6182,3 @@ where
Ok(())
- },+ },
}
@@ -12559,35 +12559,2 @@ mod tests {
- #[test]- fn test_accept_inbound_channel_errors_queued() {- // For manually accepted inbound channels, tests that a close error is correctly handled- // and the channel fails for the initiator.- let mut config0 = test_default_channel_config();- let mut config1 = config0.clone();- config1.channel_handshake_limits.their_to_self_delay = 1000;- config1.manually_accept_inbound_channels = true;- config0.channel_handshake_config.our_to_self_delay = 2000;-- let chanmon_cfgs = create_chanmon_cfgs(2);- let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);- let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[Some(config0), Some(config1)]);- let nodes = create_network(2, &node_cfgs, &node_chanmgrs);-- nodes[0].node.create_channel(nodes[1].node.get_our_node_id(), 100_000, 0, 42, None, None).unwrap();- let open_channel_msg = get_event_msg!(nodes[0], MessageSendEvent::SendOpenChannel, nodes[1].node.get_our_node_id());-- nodes[1].node.handle_open_channel(&nodes[0].node.get_our_node_id(), &open_channel_msg);- let events = nodes[1].node.get_and_clear_pending_events();- match events[0] {- Event::OpenChannelRequest { temporary_channel_id, .. } => {- match nodes[1].node.accept_inbound_channel(&temporary_channel_id, &nodes[0].node.get_our_node_id(), 23) {- Err(APIError::ChannelUnavailable { err: _ }) => (),- _ => panic!(),- }- }- _ => panic!("Unexpected event"),- }- assert_eq!(get_err_msg(&nodes[1], &nodes[0].node.get_our_node_id()).channel_id,- open_channel_msg.common_fields.temporary_channel_id);- }-
#[test]
diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs
index c5ea582b..92ddc01d 100644
--- a/lightning/src/ln/functional_tests.rs+++ b/lightning/src/ln/functional_tests.rs@@ -10985 +10985,34 @@ fn test_funding_and_commitment_tx_confirm_same_block() {
}
++#[test]+fn test_accept_inbound_channel_errors_queued() {+ // For manually accepted inbound channels, tests that a close error is correctly handled+ // and the channel fails for the initiator.+ let mut config0 = test_default_channel_config();+ let mut config1 = config0.clone();+ config1.channel_handshake_limits.their_to_self_delay = 1000;+ config1.manually_accept_inbound_channels = true;+ config0.channel_handshake_config.our_to_self_delay = 2000;++ let chanmon_cfgs = create_chanmon_cfgs(2);+ let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);+ let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[Some(config0), Some(config1)]);+ let nodes = create_network(2, &node_cfgs, &node_chanmgrs);++ nodes[0].node.create_channel(nodes[1].node.get_our_node_id(), 100_000, 0, 42, None, None).unwrap();+ let open_channel_msg = get_event_msg!(nodes[0], MessageSendEvent::SendOpenChannel, nodes[1].node.get_our_node_id());++ nodes[1].node.handle_open_channel(&nodes[0].node.get_our_node_id(), &open_channel_msg);+ let events = nodes[1].node.get_and_clear_pending_events();+ match events[0] {+ Event::OpenChannelRequest { temporary_channel_id, .. } => {+ match nodes[1].node.accept_inbound_channel(&temporary_channel_id, &nodes[0].node.get_our_node_id(), 23) {+ Err(APIError::ChannelUnavailable { err: _ }) => (),+ _ => panic!(),+ }+ }+ _ => panic!("Unexpected event"),+ }+ assert_eq!(get_err_msg(&nodes[1], &nodes[0].node.get_our_node_id()).channel_id,+ open_channel_msg.common_fields.temporary_channel_id);+}

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 96.47059% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 89.62%. Comparing base (0cc0858) to head (3206d1f).
Report is 8 commits behind head on main.

FilesPatch %Lines
lightning/src/ln/functional_tests.rs92.85%2 Missing ⚠️
lightning/src/ln/channelmanager.rs98.24%1 Missing ⚠️

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

Additional details and impacted files
@@ Coverage Diff @@## main #2953 +/- ##
==========================================
+ Coverage 89.23% 89.62% +0.38% 
==========================================
Files 117 118 +1 Lines 95545 100078 +4533 Branches 95545 100078 +4533 ==========================================
+ Hits 85260 89695 +4435 - Misses 7799 7872 +73 - Partials 2486 2511 +25 

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

@G8XSU
G8XSU merged commit 9ca2280 into lightningdevkit:mainMar 21, 2024
PXplod pushed a commit to bitlightlabs/rust-lightning that referenced this pull request Sep 30, 2024
v0.0.123 - May 08, 2024 - "BOLT12 Dust Sweeping"
API Updates
===========
* To reduce risk of force-closures and improve HTLC reliability the default
dust exposure limit has been increased to
`MaxDustHTLCExposure::FeeRateMultiplier(10_000)`. Users with existing
channels might want to consider using
`ChannelManager::update_channel_config` to apply the new default (lightningdevkit#3045).
* `ChainMonitor::archive_fully_resolved_channel_monitors` is now provided to
remove from memory `ChannelMonitor`s that have been fully resolved on-chain
and are now not needed. It uses the new `Persist::archive_persisted_channel`
to inform the storage layer that such a monitor should be archived (lightningdevkit#2964).
* An `OutputSweeper` is now provided which will automatically sweep
`SpendableOutputDescriptor`s, retrying until the sweep confirms (lightningdevkit#2825).
* After initiating an outbound channel, a peer disconnection no longer results
in immediate channel closure. Rather, if the peer is reconnected before the
channel times out LDK will automatically retry opening it (lightningdevkit#2725).
* `PaymentPurpose` now has separate variants for BOLT12 payments, which
include fields from the `invoice_request` as well as the `OfferId` (lightningdevkit#2970).
* `ChannelDetails` now includes a list of in-flight HTLCs (lightningdevkit#2442).
* `Event::PaymentForwarded` now includes `skimmed_fee_msat` (lightningdevkit#2858).
* The `hashbrown` dependency has been upgraded and the use of `ahash` as the
no-std hash table hash function has been removed. As a consequence, LDK's
`Hash{Map,Set}`s no longer feature several constructors when LDK is built
with no-std; see the `util::hash_tables` module instead. On platforms that
`getrandom` supports, setting the `possiblyrandom/getrandom` feature flag
will ensure hash tables are resistant to HashDoS attacks, though the
`possiblyrandom` crate should detect most common platforms (lightningdevkit#2810, lightningdevkit#2891).
* `ChannelMonitor`-originated requests to the `ChannelSigner` can now fail and
be retried using `ChannelMonitor::signer_unblocked` (lightningdevkit#2816).
* `SpendableOutputDescriptor::to_psbt_input` now includes the `witness_script`
where available as well as new proprietary data which can be used to
re-derive some spending keys from the base key (lightningdevkit#2761, lightningdevkit#3004).
* `OutPoint::to_channel_id` has been removed in favor of
`ChannelId::v1_from_funding_outpoint` in preparation for v2 channels with a
different `ChannelId` derivation scheme (lightningdevkit#2797).
* `PeerManager::get_peer_node_ids` has been replaced with `list_peers` and
`peer_by_node_id`, which provide more details (lightningdevkit#2905).
* `Bolt11Invoice::get_payee_pub_key` is now provided (lightningdevkit#2909).
* `Default[Message]Router` now take an `entropy_source` argument (lightningdevkit#2847).
* `ClosureReason::HTLCsTimedOut` has been separated out from
`ClosureReason::HolderForceClosed` as it is the most common case (lightningdevkit#2887).
* `ClosureReason::CooperativeClosure` is now split into
`{Counterparty,Locally}Initiated` variants (lightningdevkit#2863).
* `Event::ChannelPending::channel_type` is now provided (lightningdevkit#2872).
* `PaymentForwarded::{prev,next}_user_channel_id` are now provided (lightningdevkit#2924).
* Channel init messages have been refactored towards V2 channels (lightningdevkit#2871).
* `BumpTransactionEvent` now contains the channel and counterparty (lightningdevkit#2873).
* `util::scid_utils` is now public, with some trivial utilities to examine
short channel ids (lightningdevkit#2694).
* `DirectedChannelInfo::{source,target}` are now public (lightningdevkit#2870).
* Bounds in `lightning-background-processor` were simplified by using
`AChannelManager` (lightningdevkit#2963).
* The `Persist` impl for `KVStore` no longer requires `Sized`, allowing for
the use of `dyn KVStore` as `Persist` (lightningdevkit#2883, lightningdevkit#2976).
* `From<PaymentPreimage>` is now implemented for `PaymentHash` (lightningdevkit#2918).
* `NodeId::from_slice` is now provided (lightningdevkit#2942).
* `ChannelManager` deserialization may now fail with `DangerousValue` when
LDK's persistence API was violated (lightningdevkit#2974).
Bug Fixes
=========
* Excess fees on counterparty commitment transactions are now included in the
dust exposure calculation. This lines behavior up with some cases where
transaction fees can be burnt, making them effectively dust exposure (lightningdevkit#3045).
* `Future`s used as an `std::...::Future` could grow in size unbounded if it
was never woken. For those not using async persistence and using the async
`lightning-background-processor`, this could cause a memory leak in the
`ChainMonitor` (lightningdevkit#2894).
* Inbound channel requests that fail in
`ChannelManager::accept_inbound_channel` would previously have stalled from
the peer's perspective as no `error` message was sent (lightningdevkit#2953).
* Blinded path construction has been tuned to select paths more likely to
succeed, improving BOLT12 payment reliability (lightningdevkit#2911, lightningdevkit#2912).
* After a reorg, `lightning-transaction-sync` could have failed to follow a
transaction that LDK needed information about (lightningdevkit#2946).
* `RecipientOnionFields`' `custom_tlvs` are now propagated to recipients when
paying with blinded paths (lightningdevkit#2975).
* `Event::ChannelClosed` is now properly generated and peers are properly
notified for all channels that as a part of a batch channel open fail to be
funded (lightningdevkit#3029).
* In cases where user event processing is substantially delayed such that we
complete multiple round-trips with our peers before a `PaymentSent` event is
handled and then restart without persisting the `ChannelManager` after having
persisted a `ChannelMonitor[Update]`, on startup we may have `Err`d trying to
deserialize the `ChannelManager` (lightningdevkit#3021).
* If a peer has relatively high latency, `PeerManager` may have failed to
establish a connection (lightningdevkit#2993).
* `ChannelUpdate` messages broadcasted for our own channel closures are now
slightly more robust (lightningdevkit#2731).
* Deserializing malformed BOLT11 invoices may have resulted in an integer
overflow panic in debug builds (lightningdevkit#3032).
* In exceedingly rare cases (no cases of this are known), LDK may have created
an invalid serialization for a `ChannelManager` (lightningdevkit#2998).
* Message processing latency handling BOLT12 payments has been reduced (lightningdevkit#2881).
* Latency in processing `Event::SpendableOutputs` may be reduced (lightningdevkit#3033).
Node Compatibility
==================
* LDK's blinded paths were inconsistent with other implementations in several
ways, which have been addressed (lightningdevkit#2856, lightningdevkit#2936, lightningdevkit#2945).
* LDK's messaging blinded paths now support the latest features which some
nodes may begin relying on soon (lightningdevkit#2961).
* LDK's BOLT12 structs have been updated to support some last-minute changes to
the spec (lightningdevkit#3017, lightningdevkit#3018).
* CLN v24.02 requires the `gossip_queries` feature for all peers, however LDK
by default does not set it for those not using a `P2PGossipSync` (e.g. those
using RGS). This change was reverted in CLN v24.02.2 however for now LDK
always sets the `gossip_queries` feature. This change is expected to be
reverted in a future LDK release (lightningdevkit#2959).
Security
========
0.0.123 fixes a denial-of-service vulnerability which we believe to be reachable
from untrusted input when parsing invalid BOLT11 invoices containing non-ASCII
characters.
* BOLT11 invoices with non-ASCII characters in the human-readable-part may
cause an out-of-bounds read attempt leading to a panic (lightningdevkit#3054). Note that all
BOLT11 invoices containing non-ASCII characters are invalid.
In total, this release features 150 files changed, 19307 insertions, 6306
deletions in 360 commits since 0.0.121 from 17 authors, in alphabetical order:
* Arik Sosman
* Duncan Dean
* Elias Rohrer
* Evan Feenstra
* Jeffrey Czyz
* Keyue Bao
* Matt Corallo
* Orbital
* Sergi Delgado Segura
* Valentine Wallace
* Willem Van Lint
* Wilmer Paulino
* benthecarman
* jbesraa
* olegkubrakov
* optout
* shaavan
@dunxen
dunxen deleted the 2024-03-fix2941 branch January 20, 2025 08:31
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Errors not being handled during ChannelManager::accept_inbound_channel

4 participants

@dunxen@codecov-commenter@TheBlueMatt@G8XSU