Support opening anchor channels and test end-to-end unilateral close #1860

wpaulino · 2022-11-18T01:26:42Z

This PR introduces support for the anchors feature bit and opening/accepting anchor channels. It also includes an end-to-end test of the unilateral channel close flow, consuming the recently added anchor events and attaching fees to transactions when necessary.

Depends on #1825.

wpaulino · 2022-11-18T01:28:09Z

Currently, it doesn't expose any way to automatically reject inbound anchor channels (e.g., low onchain funds) and it doesn't force users to manually accept them either (like with zero conf). I'd imagine we'd at least want to implement the latter before considering merging this.

ariard · 2022-11-18T02:44:34Z

Note, there was a security issue affecting LND/Core Lightning anchor output support few months ago: https://lists.linuxfoundation.org/pipermail/lightning-dev/2022-April/003561.html. Briefly, anchor output relaxes the sighash flags for second-stage HTLC transactions from SIGHASH_ALL to SIGHASH_SINGLE | SIGHASH_ANYONECANPAY, allowing your counterparty to combine them in a single-batch. If your implementation was doing assumptions on the inputs/outputs size or order to parse those second-stage HTLC transactions in case of revoked state, a malicious party could extract funds.

With the current state of our parsing logic in check_spend_counterparty_htlc, I think we're encumbering the same exact security issue due to the following checks (1231907, channelmonitors, L2644) and we should adapt our logic in consequence:

		if tx.input.len() != 1 || tx.output.len() != 1 || tx.input[0].witness.len() != 5 {

wpaulino · 2022-11-22T01:28:53Z

Thanks for the note @ariard, I've addressed this in #1825 (commit aaf458d). I definitely missed this, but I plan to follow up with a series of tests that I imagine would've caught this. For now, this PR will just focus on an initial end-to-end test to make sure the happy path works.

TheBlueMatt · 2022-12-06T18:28:34Z

Let's make sure we have test coverage that would have caught #1825 (comment)

ariard · 2022-12-07T18:08:16Z

More test coverage requested from #1825: #1825 (comment)

TheBlueMatt · 2022-12-07T19:22:00Z

Needs rebase now 🎉

ariard · 2022-12-07T19:32:01Z

And more test coverage: #1825 (comment)

wpaulino · 2022-12-07T23:56:44Z

Working on the aggregated revoked HTLC test before I push another update to this. There are definitely several more cases we should be testing, but in the spirit of keeping PRs relatively small, this PR will just focus on that edge case and the happy case.

ariard

Left few reviews comments.

There are definitely several more cases we should be testing, but in the spirit of keeping PRs relatively small, this PR will just focus on that edge case and the happy case.

We can even move the aggregated revoked HTLC test in its own PR, bundled with few others from #1825. I think it's already good if we focus just on all the cases of features bit interpretation in both directions here, including unsafe option_anchor_outputs attempts.

lightning/src/ln/features.rs

lightning/src/util/config.rs

lightning/src/ln/channel.rs

wpaulino · 2022-12-16T23:18:20Z

Rebased and addressed @ariard's comments. This now depends on #1922.

lightning/src/ln/channelmanager.rs

lightning/src/ln/channel.rs

TheBlueMatt · 2022-12-19T17:32:10Z

lightning/src/util/config.rs

+	/// [`ChannelManager`]: crate::ln::channelmanager::ChannelManager
+	/// [`DecodeError::InvalidValue`]: crate::ln::msgs::DecodeError::InvalidValue
+	/// [`SIGHASH_SINGLE + update_fee Considered Harmful`]: https://lists.linuxfoundation.org/pipermail/lightning-dev/2020-September/002796.html
+	pub negotiate_anchors_zero_fee_htlc_tx: bool,


Do we want a separate flag for "dont accept anchor channels" in addition to "open outbound channels"?

Possibly. We'll also want to base our acceptance of anchor channels on our availability of fee UTXOs, which will come in a follow-up, so we could choose to do it all there.

lightning/src/ln/channel.rs

lightning/src/chain/onchaintx.rs

codecov-commenter · 2022-12-19T19:24:37Z

Codecov Report

Base: 90.69% // Head: 90.69% // Decreases project coverage by -0.01% ⚠️

Coverage data is based on head (27a74fe) compared to base (01fe9ba).
Patch coverage: 69.00% of modified lines in pull request are covered.

❗ Current head 27a74fe differs from pull request most recent head 202ff7f. Consider uploading reports for the commit 202ff7f to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1860      +/-   ##
==========================================
- Coverage   90.69%   90.69%   -0.01%     
==========================================
  Files          97       95       -2     
  Lines       50577    50074     -503     
  Branches    50577    50074     -503     
==========================================
- Hits        45870    45413     -457     
+ Misses       4707     4661      -46

Impacted Files	Coverage Δ
lightning/src/ln/monitor_tests.rs	`99.56% <ø> (ø)`
lightning/src/util/config.rs	`65.90% <0.00%> (ø)`
lightning/src/util/events.rs	`29.35% <ø> (-1.15%)`	⬇️
lightning/src/ln/functional_test_utils.rs	`93.44% <60.00%> (+1.96%)`	⬆️
lightning/src/ln/channel.rs	`88.45% <62.06%> (-0.32%)`	⬇️
lightning/src/ln/features.rs	`98.46% <80.00%> (-1.23%)`	⬇️
lightning/src/ln/channelmanager.rs	`86.72% <100.00%> (-0.39%)`	⬇️
lightning/src/ln/functional_tests.rs	`96.90% <100.00%> (-0.01%)`	⬇️
lightning/src/util/macro_logger.rs	`86.15% <100.00%> (ø)`
lightning/src/util/fairrwlock.rs	`85.71% <0.00%> (-2.53%)`	⬇️
... and 45 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.

lightning/src/ln/features.rs

TheBlueMatt · 2022-12-21T16:20:27Z

lightning/src/ln/channel.rs

@@ -911,7 +932,11 @@ impl<Signer: Sign> Channel<Signer> {
 	where K::Target: KeysInterface<Signer = Signer>,
 	      F::Target: FeeEstimator,
 	{
-		let opt_anchors = false; // TODO - should be based on features
+		#[cfg(anchors)]


This is only used for "is it even possible to build a channel with this amount". Should we just drop this and always pass anchors (since its a slightly higher commitment tx fee)? Otherwise I'm worried about making decisions based on the anchor flag which isn't really right, its just the initial state.

It's also setting the value in ChannelTransactionParameters. That of course won't be locked in until we receive an accept_channel with the same channel_type we extended. Are you also suggesting we delay setting that value in ChannelTransactionParameters until then?

lightning/src/ln/channel.rs

lightning/src/ln/channelmanager.rs

lightning/src/ln/channel.rs

lightning/src/util/config.rs

lightning/src/ln/channel.rs

lightning/src/ln/features.rs

lightning/src/ln/monitor_tests.rs

TheBlueMatt

LGTM. Needs a pretty trivial rebase to drop the first commit.

lightning/src/ln/channel.rs

ariard

Otherwise test coverage sounds good.

ariard · 2023-01-17T20:51:31Z

lightning/src/ln/channel.rs

+
+	#[cfg(anchors)]
+	#[test]
+	fn test_supports_anchors_zero_htlc_tx_fee() {


I don't know if the flow that matters, though when our negotiate_anchors_zero_fee_htlc_tx=true and their init features option_anchors_zero_fee_htlc_tx=false doesn't seem cover:

diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 8cf5e54f..b552b61b 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -892,10 +892,8 @@ impl<Signer: Sign> Channel<Signer> { // Optionally, if the user would like to negotiate the `anchors_zero_fee_htlc_tx` option, we // set it now. If they don't understand it, we'll fall back to our default of // `only_static_remotekey`. - #[cfg(anchors)] { // Attributes are not allowed on if expressions on our current MSRV of 1.41. - if config.channel_handshake_config.negotiate_anchors_zero_fee_htlc_tx && - their_features.supports_anchors_zero_fee_htlc_tx() { + if config.channel_handshake_config.negotiate_anchors_zero_fee_htlc_tx { ret.set_anchors_zero_fee_htlc_tx_required(); } }

Should be covered now.

ariard · 2023-01-17T21:04:50Z

lightning/src/ln/channel.rs

+		// We support opening a few different types of channels. Try removing our additional
+		// features one by one until we've either arrived at our default or the counterparty has
+		// accepted one.
+		if self.channel_type.supports_anchors_zero_fee_htlc_tx() {


Do we have test coverage for handle_error once we got an error message due to rejection of channel type ?

Should be covered now.

wpaulino · 2023-01-17T23:04:57Z

Would appreciate a thorough pass from reviewers as it's been a while since the previous review iteration.

TheBlueMatt

LGTM, comments can be in a followup or the comment request ignored.

TheBlueMatt · 2023-01-18T19:01:49Z

lightning/src/ln/channel.rs

+		// We support opening a few different types of channels. Try removing our additional
+		// features one by one until we've either arrived at our default or the counterparty has
+		// accepted one.
+		if self.channel_type.supports_anchors_zero_fee_htlc_tx() {


May be worth adding a comment that the ordering here would mean that we'd never negotiate anchor with a peer that supports anchor but not scid privacy, except that we check the peer's init flags above so in general this code should really only apply for a peer that is setting init flags but not actually supporting the channel type, which should be very rare.

May be worth adding a comment that the ordering here would mean that we'd never negotiate anchor with a peer that supports anchor but not scid privacy

Comment added.

this code should really only apply for a peer that is setting init flags but not actually supporting the channel type, which should be very rare

It won't be too rare with anchors if the inbound node doesn't have enough onchain funds to bump their commitments. They'll still advertise the feature, but reject the channel.

Sure, sorry, my point was if we didn't do the check initially we'd never negotiate anchors for lots of old lnd nodes - because anchors predates the scid privacy stuff we'd always end up with no features. As long as we check their features, we're safe from that.

TheBlueMatt · 2023-01-18T19:09:17Z

lightning/src/ln/channel.rs

+		if self.channel_type.supports_anchors_zero_fee_htlc_tx() {
+			self.channel_type.clear_anchors_zero_fee_htlc_tx();
+			assert!(self.channel_transaction_parameters.opt_non_zero_fee_anchors.is_none());
+			self.channel_transaction_parameters.opt_anchors = None;


Given we're only supporting anchors with channel type negotiation, we should probably just drop opt_anchors entirely and do channel_type.supports... instead? Can come in a followup though since its a new change...I really do hate having duplicate state tracking the same thing :)

I don't think we can drop ChannelTransactionParamerters.opt_anchors since signers rely on it in provide_channel_parameters.

We can regenerate it for them, though, rather than storing it explicitly in Channel and in its serialized form.

Ah, I'll look into this later then.

lightning/src/ln/monitor_tests.rs

lightning/src/ln/channel.rs

TheBlueMatt · 2023-01-19T00:32:58Z

lightning/src/util/config.rs

+	/// If set, we attempt to negotiate the `anchors_zero_fee_htlc_tx`option for outbound channels.
+	///
+	/// If this option is set, channels may be created that will not be readable by LDK versions
+	/// prior to 0.0.114, causing [`ChannelManager`]'s read method to return a


Reminder that we may need to update this when we make anchor not cfg-gated anymore. Would be nice if we didn't and that happens in 0.0.114, but if not, that's okay too.

wpaulino mentioned this pull request Nov 22, 2022

Introduce new BumpTransactionEvent variant HTLCResolution #1825

Merged

TheBlueMatt added blocked on dependent pr and removed blocked on dependent pr labels Dec 1, 2022

TheBlueMatt removed the blocked on dependent pr label Dec 7, 2022

ariard reviewed Dec 8, 2022

View reviewed changes

wpaulino force-pushed the open-channel-anchors-support branch from 1231907 to 0d7b5fa Compare December 16, 2022 23:16

wpaulino marked this pull request as ready for review December 16, 2022 23:17

TheBlueMatt added the Seeking Code Review label Dec 18, 2022

TheBlueMatt reviewed Dec 19, 2022

View reviewed changes

wpaulino force-pushed the open-channel-anchors-support branch from 0d7b5fa to 523c412 Compare December 19, 2022 19:24

wpaulino force-pushed the open-channel-anchors-support branch from 523c412 to 27a74fe Compare December 19, 2022 22:09

TheBlueMatt reviewed Dec 21, 2022

View reviewed changes

TheBlueMatt mentioned this pull request Dec 21, 2022

Pre-work for BOLT 12 invoices #1927

Merged

wpaulino force-pushed the open-channel-anchors-support branch from 27a74fe to a88d650 Compare December 22, 2022 19:51

ariard reviewed Dec 29, 2022

View reviewed changes

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

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

lightning/src/util/config.rs Show resolved Hide resolved

wpaulino force-pushed the open-channel-anchors-support branch 2 times, most recently from 9cf8351 to 404f3d1 Compare January 4, 2023 00:35

TheBlueMatt assigned TheBlueMatt and arik-so Jan 5, 2023

TheBlueMatt removed the Seeking Code Review label Jan 5, 2023

arik-so reviewed Jan 7, 2023

View reviewed changes

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

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

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

wpaulino mentioned this pull request Jan 12, 2023

Use UserConfig to determine advertised InitFeatures by ChannelManager #1946

Merged

wpaulino force-pushed the open-channel-anchors-support branch from 404f3d1 to 7e7abb5 Compare January 12, 2023 01:09

TheBlueMatt added blocked on dependent pr and removed blocked on dependent pr labels Jan 13, 2023

TheBlueMatt reviewed Jan 16, 2023

View reviewed changes

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

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

ariard reviewed Jan 17, 2023

View reviewed changes

wpaulino force-pushed the open-channel-anchors-support branch from 7e7abb5 to 6dbcde5 Compare January 17, 2023 23:03

wpaulino added 3 commits January 17, 2023 15:03

Add missing space in DebugTx logging

9cbbb58

Fix stale reference to BaseSign::ready_channel

bb617d3

Define option_anchors_zero_fee_htlc_tx feature bit

31a5962

wpaulino force-pushed the open-channel-anchors-support branch from 6dbcde5 to 202ff7f Compare January 17, 2023 23:04

TheBlueMatt previously approved these changes Jan 18, 2023

View reviewed changes

arik-so reviewed Jan 18, 2023

View reviewed changes

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

arik-so reviewed Jan 18, 2023

View reviewed changes

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

arik-so previously approved these changes Jan 18, 2023

View reviewed changes

wpaulino added 3 commits January 18, 2023 14:46

Support negotiating anchors throughout channel open

8dda117

Add mine_transactions test util

d9dd21b

Add test yielding anchor-related events

660165c

wpaulino dismissed stale reviews from arik-so and TheBlueMatt via 660165c January 18, 2023 22:49

wpaulino force-pushed the open-channel-anchors-support branch from 202ff7f to 660165c Compare January 18, 2023 22:49

TheBlueMatt approved these changes Jan 19, 2023

View reviewed changes

arik-so approved these changes Jan 19, 2023

View reviewed changes

TheBlueMatt reviewed Jan 19, 2023

View reviewed changes

TheBlueMatt mentioned this pull request Jan 19, 2023

Don't store opt_anchors explicitly in Channel #1971

Closed

TheBlueMatt merged commit 50d1260 into lightningdevkit:main Jan 19, 2023

wpaulino deleted the open-channel-anchors-support branch January 19, 2023 01:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support opening anchor channels and test end-to-end unilateral close #1860

Support opening anchor channels and test end-to-end unilateral close #1860

wpaulino commented Nov 18, 2022

wpaulino commented Nov 18, 2022

ariard commented Nov 18, 2022

wpaulino commented Nov 22, 2022

TheBlueMatt commented Dec 6, 2022

ariard commented Dec 7, 2022

TheBlueMatt commented Dec 7, 2022

ariard commented Dec 7, 2022

wpaulino commented Dec 7, 2022

ariard left a comment

wpaulino commented Dec 16, 2022

TheBlueMatt Dec 19, 2022

wpaulino Dec 19, 2022

codecov-commenter commented Dec 19, 2022 •

edited

Loading

TheBlueMatt Dec 21, 2022

wpaulino Dec 22, 2022

TheBlueMatt left a comment

ariard left a comment

ariard Jan 17, 2023

wpaulino Jan 17, 2023

ariard Jan 17, 2023

wpaulino Jan 17, 2023

wpaulino commented Jan 17, 2023

TheBlueMatt left a comment

TheBlueMatt Jan 18, 2023

wpaulino Jan 18, 2023

TheBlueMatt Jan 19, 2023

TheBlueMatt Jan 18, 2023

wpaulino Jan 18, 2023

TheBlueMatt Jan 19, 2023

wpaulino Jan 19, 2023

TheBlueMatt Jan 19, 2023

Support opening anchor channels and test end-to-end unilateral close #1860

Support opening anchor channels and test end-to-end unilateral close #1860

Conversation

wpaulino commented Nov 18, 2022

wpaulino commented Nov 18, 2022

ariard commented Nov 18, 2022

wpaulino commented Nov 22, 2022

TheBlueMatt commented Dec 6, 2022

ariard commented Dec 7, 2022

TheBlueMatt commented Dec 7, 2022

ariard commented Dec 7, 2022

wpaulino commented Dec 7, 2022

ariard left a comment

Choose a reason for hiding this comment

wpaulino commented Dec 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Dec 19, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

ariard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wpaulino commented Jan 17, 2023

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Dec 19, 2022 •

edited

Loading