Fix various issues found in `full_stack_target` fuzzing #2808

TheBlueMatt · 2023-12-29T06:31:36Z

#2804 reported a message-processing-reachable unwrap, which is concerning, but more generally it reported a serious coverage gap in our full_stack_target fuzzer. This fuzzer should be our first line of defense against such issues, but sadly was using a predefined config object which left us not testing cases that required specific config flags. This fixes #2804 (in an imo cleaner way than #2805, at least for downstream devs even if not in LDK) as well as a number of other quite minor debug and overflow issues found after I updated the full_stack_target to test different config flags.

If we get a `Feature` object which has excess zero bytes, we shouldn't consider it a different `Feature` from another with the same bits set, but no excess zero bytes. Here we fix both the `Hash` and `PartialEq` implementation for `Features` to ignore excess zero bytes.

When we or our counterparty are updating the fees on the channel, we currently check that the resulting balance is sufficient not only to meet the reserve threshold, but also not push it below dust. This isn't required in the BOLTs and may lead to spurious force-closures (which would be a bit safer, but reserve should always exceed the dust threshold). Worse, the current logic is broken - it compares the output value in *billionths of satoshis* to the dust limit in satoshis. Thus, the code is borderline dead anyway, but can overflow for channels with several million Bitcoin, causing the fuzzer to get mad (and lead to spurious force-closures for few-billion-dollar channels).

When contest delays are >= 0x8000, script pushes require an extra byte to avoid being interpreted as a negative int. Thus, for channels with CSV delays longer than ~7.5 months we may generate transactions with slightly too little fee. This isn't really a huge deal, but we should prefer to be conservative here, and slightly too high fee in the general case is better than slightly too little fee in other cases.

If we try to open a channel with a peer that is disconnected (but with which we have some other channels), we'll end up with an unfunded channel which will lead to a panic when the peer reconnects. Here we drop this debug assertion without bother to add a new test, given this behavior will change in a PR very soon.

If a peer provides a feerate which nears `u32::MAX`, we may overflow calculating the dust buffer feerate, leading to spuriously keeping non-anchor channels open when they should be force-closed.

codecov-commenter · 2023-12-29T06:38:18Z

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (4deb263) 88.49% compared to head (bceca1e) 88.73%.
Report is 14 commits behind head on main.

❗ Current head bceca1e differs from pull request most recent head 7f24e83. Consider uploading reports for the commit 7f24e83 to get more accurate results

Files	Patch %	Lines
lightning/src/ln/channel.rs	88.57%	4 Missing ⚠️
lightning/src/ln/features.rs	96.55%	0 Missing and 1 partial ⚠️

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2808      +/-   ##
==========================================
+ Coverage   88.49%   88.73%   +0.23%     
==========================================
  Files         114      114              
  Lines       91935    95699    +3764     
  Branches    91935    95699    +3764     
==========================================
+ Hits        81359    84914    +3555     
- Misses       8097     8336     +239     
+ Partials     2479     2449      -30

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

tnull

One question, otherwise LGTM.

tnull · 2024-01-01T12:37:38Z

lightning/src/ln/channel.rs

@@ -6884,7 +6894,7 @@ impl<SP: Deref> InboundV1Channel<SP> where SP::Target: SignerProvider {
 			channel_type.clone()
 		} else {
 			let channel_type = ChannelTypeFeatures::from_init(&their_features);
-			if channel_type != ChannelTypeFeatures::only_static_remote_key() {
+			if channel_type != channel_type_from_open_channel(msg) {


This change is a bit confusing to me: why make it look like we'd consider msg after all, when in fact it would always result in ChannelTypeFeatures::only_static_remote_key()? Leaving it as-is seems more readable?

Yea, I was trying to keep from making the "default channel type is only-static-remote-key" assumption copied in several places, but I agree this ended up being more awkward than not. Instead I moved the whole "what is the channel type given the message" thing into the new method and made it fallible. This no longer directly solves the issue, but rather actually handling the failures does, which is also kinda nice in that we will avoid giving users an event for a channel we can't use.

shaavan

The changes look great! I have one small question and two suggestions for the comments.

lightning/src/ln/chan_utils.rs

shaavan · 2024-01-04T12:39:20Z

lightning/src/ln/channel.rs

@@ -1876,7 +1872,8 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider  {
 		if let Some(feerate) = outbound_feerate_update {


In the description above it is written:

// may, at any point, increase _by_ at least 10 sat/vB (i.e 2530 sat/kWU) or 25%, // whichever is higher.

which implies:

cmp::max(feerate_by_kw + 2530, feerate_plus_quarter)

Whereas, the line should read:

pub fn get_dust_buffer_feerate(&self, outbound_feerate_update: Option<u32>) -> u32 { // When calculating our exposure to dust HTLCs, we assume that the channel feerate - // may, at any point, increase by at least 10 sat/vB (i.e 2530 sat/kWU) or 25%, + // may, at any point, increase to at least 10 sat/vB (i.e 2530 sat/kWU) or by 25%, // whichever is higher. This ensures that we aren't suddenly exposed to significantly

Since we are touching this function in this PR. I believe we should also address this small but significant comment change.

Oh, good catch! I'd like to actually address this by making the code match the comment, not the other way around, so I'll address this in a later PR - #2815

wpaulino

LGTM after squash

lightning/src/ln/chan_utils.rs

TheBlueMatt · 2024-01-06T07:41:50Z

Squashed.

tnull

LGTM

One nit, but feel free to land as is.

lightning/src/ln/channelmanager.rs

shaavan

ACK
The changes look great, and since the comment will be addressed in a follow-up PR, I believe this PR is good to go!

If we receive an `OpenChannel` message without a `channel_type` with `manually_accept_inbound_channels` set, we will `unwrap()` `None`. This is uncommon these days as most nodes support `channel_type`, but sadly is rather trivial for a peer to hit for those with manual channel acceptance enabled. Reported in and fixes lightningdevkit#2804. Luckily, the updated `full_stack_target` has no issue reaching this issue quickly.

TheBlueMatt · 2024-01-08T18:21:36Z

Addressed the nit:

$ git diff-tree -U1 bceca1eed7aced011150f0f6ca7f0dd6fbe71d6a 7f24e833
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 5efef2e90..8e0ac2fdf 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -6172,10 +6172,7 @@ where
 		if self.default_configuration.manually_accept_inbound_channels {
-			let channel_type_res = channel::channel_type_from_open_channel(
-				&msg, &peer_state.latest_features, &self.channel_type_features()
-			);
-			let channel_type = match channel_type_res {
-				Ok(channel_type) => channel_type,
-				Err(e) =>
-					return Err(MsgHandleErrInternal::from_chan_no_close(e, msg.temporary_channel_id)),
-			};
+			let channel_type = channel::channel_type_from_open_channel(
+					&msg, &peer_state.latest_features, &self.channel_type_features()
+				).map_err(|e|
+					MsgHandleErrInternal::from_chan_no_close(e, msg.temporary_channel_id)
+				)?;
 			let mut pending_events = self.pending_events.lock().unwrap();

TheBlueMatt added 5 commits December 24, 2023 17:34

Fix dust buffer feerate calculation overflow

3b6a361

If a peer provides a feerate which nears `u32::MAX`, we may overflow calculating the dust buffer feerate, leading to spuriously keeping non-anchor channels open when they should be force-closed.

TheBlueMatt added this to the 0.0.120 milestone Dec 29, 2023

This was referenced Dec 29, 2023

Clean Up Funding Error Handling and shutdown #2809

Merged

Update full_stack_target to take an arbitrary config object #2810

Merged

tnull reviewed Jan 1, 2024

View reviewed changes

shaavan reviewed Jan 4, 2024

View reviewed changes

wpaulino reviewed Jan 4, 2024

View reviewed changes

lightning/src/ln/chan_utils.rs Show resolved Hide resolved

TheBlueMatt force-pushed the 2023-12-fuzzing-fixes-1 branch from e2b8063 to bceca1e Compare January 6, 2024 02:07

TheBlueMatt mentioned this pull request Jan 6, 2024

Channel::get_dust_buffer_feerate comment doesn't match code #2815

Closed

tnull previously approved these changes Jan 8, 2024

View reviewed changes

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

shaavan approved these changes Jan 8, 2024

View reviewed changes

TheBlueMatt dismissed tnull’s stale review via dea37db January 8, 2024 18:17

TheBlueMatt force-pushed the 2023-12-fuzzing-fixes-1 branch from dea37db to 7f24e83 Compare January 8, 2024 18:20

tnull approved these changes Jan 8, 2024

View reviewed changes

wpaulino approved these changes Jan 8, 2024

View reviewed changes

TheBlueMatt merged commit 3fbee85 into lightningdevkit:main Jan 8, 2024
14 of 15 checks passed

TheBlueMatt mentioned this pull request Jan 8, 2024

Make channel_type optional in ChannelOpenRequest event. #2805

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix various issues found in `full_stack_target` fuzzing #2808

Fix various issues found in `full_stack_target` fuzzing #2808

TheBlueMatt commented Dec 29, 2023

codecov-commenter commented Dec 29, 2023 •

edited

tnull left a comment

tnull Jan 1, 2024

TheBlueMatt Jan 2, 2024

shaavan left a comment

shaavan Jan 4, 2024

TheBlueMatt Jan 6, 2024

shaavan Jan 8, 2024

wpaulino left a comment

TheBlueMatt commented Jan 6, 2024

tnull left a comment

shaavan left a comment

TheBlueMatt commented Jan 8, 2024

		@@ -1876,7 +1872,8 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
		if let Some(feerate) = outbound_feerate_update {

Fix various issues found in full_stack_target fuzzing #2808

Fix various issues found in full_stack_target fuzzing #2808

Conversation

TheBlueMatt commented Dec 29, 2023

codecov-commenter commented Dec 29, 2023 • edited

Codecov Report

tnull left a comment

Choose a reason for hiding this comment

tnull Jan 1, 2024

Choose a reason for hiding this comment

TheBlueMatt Jan 2, 2024

Choose a reason for hiding this comment

shaavan left a comment

Choose a reason for hiding this comment

shaavan Jan 4, 2024

Choose a reason for hiding this comment

TheBlueMatt Jan 6, 2024

Choose a reason for hiding this comment

shaavan Jan 8, 2024

Choose a reason for hiding this comment

wpaulino left a comment

Choose a reason for hiding this comment

TheBlueMatt commented Jan 6, 2024

tnull left a comment

Choose a reason for hiding this comment

shaavan left a comment

Choose a reason for hiding this comment

TheBlueMatt commented Jan 8, 2024

Fix various issues found in `full_stack_target` fuzzing #2808

Fix various issues found in `full_stack_target` fuzzing #2808

codecov-commenter commented Dec 29, 2023 •

edited