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

[wpaulino]

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]

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]

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]

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]

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

[ariard]

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

[TheBlueMatt]

Needs rebase now 🎉

[ariard]

And more test coverage: #1825 (comment)

[wpaulino]

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.

[wpaulino]

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

[TheBlueMatt]

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

[wpaulino]

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.

[codecov-commenter]

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.

[TheBlueMatt]

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.

[wpaulino]

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?

[TheBlueMatt]

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

[ariard]

Otherwise test coverage sounds good.

[ariard]

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();
}
}

[wpaulino]

Should be covered now.

[ariard]

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

[wpaulino]

Should be covered now.

[wpaulino]

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]

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.

[wpaulino]

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.

[TheBlueMatt]

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]

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 :)

[wpaulino]

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

[TheBlueMatt]

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

[wpaulino]

Ah, I'll look into this later then.

[TheBlueMatt]

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]

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]

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]

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]

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]

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

[ariard]

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

[TheBlueMatt]

Needs rebase now 🎉

[ariard]

And more test coverage: #1825 (comment)

[wpaulino]

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.

[wpaulino]

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

[TheBlueMatt]

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

[wpaulino]

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.

[codecov-commenter]

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.

[TheBlueMatt]

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.

[wpaulino]

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?

[TheBlueMatt]

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

[ariard]

Otherwise test coverage sounds good.

[ariard]

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();
}
}

[wpaulino]

Should be covered now.

[ariard]

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

[wpaulino]

Should be covered now.

[wpaulino]

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]

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.

[wpaulino]

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.

[TheBlueMatt]

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]

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 :)

[wpaulino]

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

[TheBlueMatt]

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

[wpaulino]

Ah, I'll look into this later then.

[TheBlueMatt]

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]

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]

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]

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]

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]

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

[ariard]

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

[TheBlueMatt]

Needs rebase now 🎉

[ariard]

And more test coverage: #1825 (comment)

[wpaulino]

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.

[wpaulino]

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

[TheBlueMatt]

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

[wpaulino]

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.

[codecov-commenter]

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.

[TheBlueMatt]

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.

[wpaulino]

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?

[TheBlueMatt]

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

[ariard]

Otherwise test coverage sounds good.

[ariard]

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();
}
}

[wpaulino]

Should be covered now.

[ariard]

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

[wpaulino]

Should be covered now.

[wpaulino]

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]

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.

[wpaulino]

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.

[TheBlueMatt]

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]

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 :)

[wpaulino]

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

[TheBlueMatt]

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

[wpaulino]

Ah, I'll look into this later then.

[TheBlueMatt]

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]

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]

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]

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]

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]

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

[ariard]

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

[TheBlueMatt]

Needs rebase now 🎉

[ariard]

And more test coverage: #1825 (comment)

[wpaulino]

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.

[wpaulino]

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

[TheBlueMatt]

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

[wpaulino]

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.

[codecov-commenter]

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.

[TheBlueMatt]

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.

[wpaulino]

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?

[TheBlueMatt]

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

[ariard]

Otherwise test coverage sounds good.

[ariard]

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();
}
}

[wpaulino]

Should be covered now.

[ariard]

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

[wpaulino]

Should be covered now.

[wpaulino]

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]

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.

[wpaulino]

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.

[TheBlueMatt]

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]

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 :)

[wpaulino]

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

[TheBlueMatt]

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

[wpaulino]

Ah, I'll look into this later then.

[TheBlueMatt]

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]

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]

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]

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]

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]

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

[ariard]

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

[TheBlueMatt]

Needs rebase now 🎉

[ariard]

And more test coverage: #1825 (comment)

[wpaulino]

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.

[wpaulino]

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

[TheBlueMatt]

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

[wpaulino]

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.

[codecov-commenter]

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.

[TheBlueMatt]

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.

[wpaulino]

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?

[TheBlueMatt]

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

[ariard]

Otherwise test coverage sounds good.

[ariard]

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();
}
}

[wpaulino]

Should be covered now.

[ariard]

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

[wpaulino]

Should be covered now.

[wpaulino]

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]

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.

[wpaulino]

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.

[TheBlueMatt]

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]

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 :)

[wpaulino]

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

[TheBlueMatt]

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

[wpaulino]

Ah, I'll look into this later then.

[TheBlueMatt]

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]

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]

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]

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]

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]

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

[ariard]

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

[TheBlueMatt]

Needs rebase now 🎉

[ariard]

And more test coverage: #1825 (comment)

[wpaulino]

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.

[wpaulino]

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

[TheBlueMatt]

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

[wpaulino]

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.

[codecov-commenter]

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.

[TheBlueMatt]

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.

[wpaulino]

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?

[TheBlueMatt]

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

[ariard]

Otherwise test coverage sounds good.

[ariard]

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();
}
}

[wpaulino]

Should be covered now.

[ariard]

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

[wpaulino]

Should be covered now.

[wpaulino]

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]

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.

[wpaulino]

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.

[TheBlueMatt]

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]

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 :)

[wpaulino]

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

[TheBlueMatt]

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

[wpaulino]

Ah, I'll look into this later then.

[TheBlueMatt]

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]

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]

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]

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]

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]

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

[ariard]

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

[TheBlueMatt]

Needs rebase now 🎉

[ariard]

And more test coverage: #1825 (comment)

[wpaulino]

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.

[wpaulino]

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

[TheBlueMatt]

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

[wpaulino]

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.

[codecov-commenter]

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.

[TheBlueMatt]

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.

[wpaulino]

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?

[TheBlueMatt]

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

[ariard]

Otherwise test coverage sounds good.

[ariard]

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();
}
}

[wpaulino]

Should be covered now.

[ariard]

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

[wpaulino]

Should be covered now.

[wpaulino]

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]

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.

[wpaulino]

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.

[TheBlueMatt]

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]

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 :)

[wpaulino]

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

[TheBlueMatt]

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

[wpaulino]

Ah, I'll look into this later then.

[TheBlueMatt]

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]

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]

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]

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]

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]

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

[ariard]

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

[TheBlueMatt]

Needs rebase now 🎉

[ariard]

And more test coverage: #1825 (comment)

[wpaulino]

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.

[wpaulino]

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

[TheBlueMatt]

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

[wpaulino]

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.

[codecov-commenter]

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.

[TheBlueMatt]

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.

[wpaulino]

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?

[TheBlueMatt]

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

[ariard]

Otherwise test coverage sounds good.

[ariard]

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();
}
}

[wpaulino]

Should be covered now.

[ariard]

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

[wpaulino]

Should be covered now.

[wpaulino]

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]

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.

[wpaulino]

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.

[TheBlueMatt]

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]

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 :)

[wpaulino]

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

[TheBlueMatt]

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

[wpaulino]

Ah, I'll look into this later then.

[TheBlueMatt]

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.