Misc refactors and cleanups in advance of zero-fee commitments by TheBlueMatt · Pull Request #3656 · lightningdevkit/rust-lightning

TheBlueMatt · 2025-03-09T22:37:35Z

A few renames, some doc cleanups, and the feature bit itself.

We have a handful of methods to clear features from `*Features` objects, but have ended up with two different API semantics for them. In the next commit we'll make them all consistent, opting against the builder pattern as it turns out all of these lines utilizing it are too long for rustfmt to be happy, so best to clean them up now so that rustfmt doesn't make a mockery of our code later.

We've ad-hoc exposed feature-clearing methods for various feature flags over the years, but there's not a lot of reason to not just do it for all the flags, so we go ahead and do that here.

These aren't really used anywhere and were only live very briefly, so there's not really any point in informing our users that we don't support them. If anything, it will lead to confusion as the zero-HTLC-fee channel type is generally referred to simply as "anchor channels".

ldk-reviews-bot · 2025-03-09T22:37:38Z

👋 Thanks for assigning @wpaulino as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

codecov · 2025-03-10T02:05:34Z

Codecov Report

Attention: Patch coverage is 82.24299% with 19 lines in your changes missing coverage. Please review.

Project coverage is 89.23%. Comparing base (4c43a5b) to head (c15b29d).

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	56.52%	10 Missing ⚠️
lightning/src/events/bump_transaction.rs	78.94%	3 Missing and 1 partial ⚠️
lightning/src/ln/chan_utils.rs	82.35%	3 Missing ⚠️
lightning/src/sign/mod.rs	94.11%	0 Missing and 1 partial ⚠️
lightning/src/util/test_channel_signer.rs	75.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3656 +/- ##
==========================================
- Coverage 89.24% 89.23% -0.01% 
==========================================
Files 155 155 Lines 119280 119281 +1 Branches 119280 119281 +1 ==========================================
Hits 106446 106446 - Misses 10240 10241 +1 
Partials 2594 2594

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

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

tnull · 2025-03-10T10:33:58Z

CI is unhappy:

error[E0034]: multiple applicable items in scope
--> lightning/src/util/test_channel_signer.rs:463:14
|
463 | self.inner.sign_holder_keyed_anchor_input(chan_params, anchor_tx, input, secp_ctx)
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ multiple `sign_holder_keyed_anchor_input` found
|
note: candidate #1 is defined in an impl of the trait `TaprootChannelSigner` for the type `DynSigner`
--> lightning/src/util/dyn_signer.rs:121:2
|
121 | / fn sign_holder_keyed_anchor_input(
122 | | &self, anchor_tx: &Transaction, input: usize, secp_ctx: &Secp256k1<All>,
123 | | ) -> Result<secp256k1::schnorr::Signature, ()> {
| |__________________________________________________^
note: candidate #2 is defined in an impl of the trait `sign::ecdsa::EcdsaChannelSigner` for the type `DynSigner`
--> lightning/src/util/mod.rs:101:17
|
101 | fn $f(&$($mu)? self, $($n: $t),*) -> $r {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
::: lightning/src/util/dyn_signer.rs:134:1
|
134 | / delegate!(DynSigner, EcdsaChannelSigner, inner,
135 | | fn sign_holder_commitment(, channel_parameters: &ChannelTransactionParameters,
136 | | commitment_tx: &HolderCommitmentTransaction,
137 | | secp_ctx: &Secp256k1<secp256k1::All>) -> Result<Signature, ()>,
... |
168 | | secp_ctx: &Secp256k1<All>) -> Result<Signature, ()>
169 | | );
| |_- in this macro invocation
= note: this error originates in the macro `delegate` (in Nightly builds, run with -Z macro-backtrace for more info)
help: disambiguate the method for candidate #1
|
463 | TaprootChannelSigner::sign_holder_keyed_anchor_input(&self.inner, chan_params, anchor_tx, input, secp_ctx)
|
help: disambiguate the method for candidate #2
|
463 | sign::ecdsa::EcdsaChannelSigner::sign_holder_keyed_anchor_input(&self.inner, chan_params, anchor_tx, input, secp_ctx)
|

TheBlueMatt · 2025-03-10T12:37:22Z

Just removed the method on the taproot signer.

valentinewallace · 2025-03-10T18:14:46Z

full_stack fuzz is failing

valentinewallace

LGTM after fuzz is fixed

ldk-reviews-bot · 2025-03-10T18:22:00Z

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

wpaulino

LGTM

wpaulino · 2025-03-11T19:37:19Z

Good to squash, CI is still sad

...as it is ambiguous now that we have multiple types of anchors. Its clearer to check the specific features at the callsites now.

As we move towards zero-fee commitment transactions, we need to differentiate between the now-two-types of "anchor channels". We do so here by renaming a number of methods which refer to anchors as "keyed anchors" as the zero-fee commitment transaction anchors do not have a public key associated with them. We also drop `TaprootChannelSigner::sign_holder_anchor_input` as we are unlikely to support keyed anchors for taproot channels.

Once we support zero-fee commitment transactions, we will no longer have a constant number of maximum HTLCs in-flight per channel but rather it will depend on the channel type. Here we prepare for this by removing the `MAX_HTLCS` constant and replacing it with a function.

TheBlueMatt · 2025-03-12T15:21:09Z

Squashed:

$ git diff-tree -U1 02ce69e59 68b16e24c
diff --git a/lightning/src/events/bump_transaction.rs b/lightning/src/events/bump_transaction.rs
index 77943845c..a9550a672 100644
--- a/lightning/src/events/bump_transaction.rs+++ b/lightning/src/events/bump_transaction.rs@@ -98,3 +98,3 @@ impl AnchorDescriptor {
} else {
- debug_assert!(tx_params.channel_type_features.supports_zero_fee_commitments());+ debug_assert!(tx_params.channel_type_features.supports_anchor_zero_fee_commitments());
Witness::from_slice(&[&[]])

TheBlueMatt · 2025-03-09T22:37:35Z

A few renames, some doc cleanups, and the feature bit itself.

We have a handful of methods to clear features from `*Features` objects, but have ended up with two different API semantics for them. In the next commit we'll make them all consistent, opting against the builder pattern as it turns out all of these lines utilizing it are too long for rustfmt to be happy, so best to clean them up now so that rustfmt doesn't make a mockery of our code later.

We've ad-hoc exposed feature-clearing methods for various feature flags over the years, but there's not a lot of reason to not just do it for all the flags, so we go ahead and do that here.

These aren't really used anywhere and were only live very briefly, so there's not really any point in informing our users that we don't support them. If anything, it will lead to confusion as the zero-HTLC-fee channel type is generally referred to simply as "anchor channels".

ldk-reviews-bot · 2025-03-09T22:37:38Z

👋 Thanks for assigning @wpaulino as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

codecov · 2025-03-10T02:05:34Z

Codecov Report

Attention: Patch coverage is 82.24299% with 19 lines in your changes missing coverage. Please review.

Project coverage is 89.23%. Comparing base (4c43a5b) to head (c15b29d).

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	56.52%	10 Missing ⚠️
lightning/src/events/bump_transaction.rs	78.94%	3 Missing and 1 partial ⚠️
lightning/src/ln/chan_utils.rs	82.35%	3 Missing ⚠️
lightning/src/sign/mod.rs	94.11%	0 Missing and 1 partial ⚠️
lightning/src/util/test_channel_signer.rs	75.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3656 +/- ##
==========================================
- Coverage 89.24% 89.23% -0.01% 
==========================================
Files 155 155 Lines 119280 119281 +1 Branches 119280 119281 +1 ==========================================
Hits 106446 106446 - Misses 10240 10241 +1 
Partials 2594 2594

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

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

tnull · 2025-03-10T10:33:58Z

CI is unhappy:

error[E0034]: multiple applicable items in scope
--> lightning/src/util/test_channel_signer.rs:463:14
|
463 | self.inner.sign_holder_keyed_anchor_input(chan_params, anchor_tx, input, secp_ctx)
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ multiple `sign_holder_keyed_anchor_input` found
|
note: candidate #1 is defined in an impl of the trait `TaprootChannelSigner` for the type `DynSigner`
--> lightning/src/util/dyn_signer.rs:121:2
|
121 | / fn sign_holder_keyed_anchor_input(
122 | | &self, anchor_tx: &Transaction, input: usize, secp_ctx: &Secp256k1<All>,
123 | | ) -> Result<secp256k1::schnorr::Signature, ()> {
| |__________________________________________________^
note: candidate #2 is defined in an impl of the trait `sign::ecdsa::EcdsaChannelSigner` for the type `DynSigner`
--> lightning/src/util/mod.rs:101:17
|
101 | fn $f(&$($mu)? self, $($n: $t),*) -> $r {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
::: lightning/src/util/dyn_signer.rs:134:1
|
134 | / delegate!(DynSigner, EcdsaChannelSigner, inner,
135 | | fn sign_holder_commitment(, channel_parameters: &ChannelTransactionParameters,
136 | | commitment_tx: &HolderCommitmentTransaction,
137 | | secp_ctx: &Secp256k1<secp256k1::All>) -> Result<Signature, ()>,
... |
168 | | secp_ctx: &Secp256k1<All>) -> Result<Signature, ()>
169 | | );
| |_- in this macro invocation
= note: this error originates in the macro `delegate` (in Nightly builds, run with -Z macro-backtrace for more info)
help: disambiguate the method for candidate #1
|
463 | TaprootChannelSigner::sign_holder_keyed_anchor_input(&self.inner, chan_params, anchor_tx, input, secp_ctx)
|
help: disambiguate the method for candidate #2
|
463 | sign::ecdsa::EcdsaChannelSigner::sign_holder_keyed_anchor_input(&self.inner, chan_params, anchor_tx, input, secp_ctx)
|

TheBlueMatt · 2025-03-10T12:37:22Z

Just removed the method on the taproot signer.

valentinewallace · 2025-03-10T18:14:46Z

full_stack fuzz is failing

valentinewallace

LGTM after fuzz is fixed

ldk-reviews-bot · 2025-03-10T18:22:00Z

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

wpaulino

LGTM

wpaulino · 2025-03-11T19:37:19Z

Good to squash, CI is still sad

...as it is ambiguous now that we have multiple types of anchors. Its clearer to check the specific features at the callsites now.

As we move towards zero-fee commitment transactions, we need to differentiate between the now-two-types of "anchor channels". We do so here by renaming a number of methods which refer to anchors as "keyed anchors" as the zero-fee commitment transaction anchors do not have a public key associated with them. We also drop `TaprootChannelSigner::sign_holder_anchor_input` as we are unlikely to support keyed anchors for taproot channels.

Once we support zero-fee commitment transactions, we will no longer have a constant number of maximum HTLCs in-flight per channel but rather it will depend on the channel type. Here we prepare for this by removing the `MAX_HTLCS` constant and replacing it with a function.

TheBlueMatt · 2025-03-12T15:21:09Z

Squashed:

$ git diff-tree -U1 02ce69e59 68b16e24c
diff --git a/lightning/src/events/bump_transaction.rs b/lightning/src/events/bump_transaction.rs
index 77943845c..a9550a672 100644
--- a/lightning/src/events/bump_transaction.rs+++ b/lightning/src/events/bump_transaction.rs@@ -98,3 +98,3 @@ impl AnchorDescriptor {
} else {
- debug_assert!(tx_params.channel_type_features.supports_zero_fee_commitments());+ debug_assert!(tx_params.channel_type_features.supports_anchor_zero_fee_commitments());
Witness::from_slice(&[&[]])

TheBlueMatt · 2025-03-09T22:37:35Z

A few renames, some doc cleanups, and the feature bit itself.

We have a handful of methods to clear features from `*Features` objects, but have ended up with two different API semantics for them. In the next commit we'll make them all consistent, opting against the builder pattern as it turns out all of these lines utilizing it are too long for rustfmt to be happy, so best to clean them up now so that rustfmt doesn't make a mockery of our code later.

We've ad-hoc exposed feature-clearing methods for various feature flags over the years, but there's not a lot of reason to not just do it for all the flags, so we go ahead and do that here.

These aren't really used anywhere and were only live very briefly, so there's not really any point in informing our users that we don't support them. If anything, it will lead to confusion as the zero-HTLC-fee channel type is generally referred to simply as "anchor channels".

ldk-reviews-bot · 2025-03-09T22:37:38Z

👋 Thanks for assigning @wpaulino as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

codecov · 2025-03-10T02:05:34Z

Codecov Report

Attention: Patch coverage is 82.24299% with 19 lines in your changes missing coverage. Please review.

Project coverage is 89.23%. Comparing base (4c43a5b) to head (c15b29d).

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	56.52%	10 Missing ⚠️
lightning/src/events/bump_transaction.rs	78.94%	3 Missing and 1 partial ⚠️
lightning/src/ln/chan_utils.rs	82.35%	3 Missing ⚠️
lightning/src/sign/mod.rs	94.11%	0 Missing and 1 partial ⚠️
lightning/src/util/test_channel_signer.rs	75.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3656 +/- ##
==========================================
- Coverage 89.24% 89.23% -0.01% 
==========================================
Files 155 155 Lines 119280 119281 +1 Branches 119280 119281 +1 ==========================================
Hits 106446 106446 - Misses 10240 10241 +1 
Partials 2594 2594

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

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

tnull · 2025-03-10T10:33:58Z

CI is unhappy:

error[E0034]: multiple applicable items in scope
--> lightning/src/util/test_channel_signer.rs:463:14
|
463 | self.inner.sign_holder_keyed_anchor_input(chan_params, anchor_tx, input, secp_ctx)
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ multiple `sign_holder_keyed_anchor_input` found
|
note: candidate #1 is defined in an impl of the trait `TaprootChannelSigner` for the type `DynSigner`
--> lightning/src/util/dyn_signer.rs:121:2
|
121 | / fn sign_holder_keyed_anchor_input(
122 | | &self, anchor_tx: &Transaction, input: usize, secp_ctx: &Secp256k1<All>,
123 | | ) -> Result<secp256k1::schnorr::Signature, ()> {
| |__________________________________________________^
note: candidate #2 is defined in an impl of the trait `sign::ecdsa::EcdsaChannelSigner` for the type `DynSigner`
--> lightning/src/util/mod.rs:101:17
|
101 | fn $f(&$($mu)? self, $($n: $t),*) -> $r {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
::: lightning/src/util/dyn_signer.rs:134:1
|
134 | / delegate!(DynSigner, EcdsaChannelSigner, inner,
135 | | fn sign_holder_commitment(, channel_parameters: &ChannelTransactionParameters,
136 | | commitment_tx: &HolderCommitmentTransaction,
137 | | secp_ctx: &Secp256k1<secp256k1::All>) -> Result<Signature, ()>,
... |
168 | | secp_ctx: &Secp256k1<All>) -> Result<Signature, ()>
169 | | );
| |_- in this macro invocation
= note: this error originates in the macro `delegate` (in Nightly builds, run with -Z macro-backtrace for more info)
help: disambiguate the method for candidate #1
|
463 | TaprootChannelSigner::sign_holder_keyed_anchor_input(&self.inner, chan_params, anchor_tx, input, secp_ctx)
|
help: disambiguate the method for candidate #2
|
463 | sign::ecdsa::EcdsaChannelSigner::sign_holder_keyed_anchor_input(&self.inner, chan_params, anchor_tx, input, secp_ctx)
|

TheBlueMatt · 2025-03-10T12:37:22Z

Just removed the method on the taproot signer.

valentinewallace · 2025-03-10T18:14:46Z

full_stack fuzz is failing

valentinewallace

LGTM after fuzz is fixed

ldk-reviews-bot · 2025-03-10T18:22:00Z

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

wpaulino

LGTM

wpaulino · 2025-03-11T19:37:19Z

Good to squash, CI is still sad

...as it is ambiguous now that we have multiple types of anchors. Its clearer to check the specific features at the callsites now.

As we move towards zero-fee commitment transactions, we need to differentiate between the now-two-types of "anchor channels". We do so here by renaming a number of methods which refer to anchors as "keyed anchors" as the zero-fee commitment transaction anchors do not have a public key associated with them. We also drop `TaprootChannelSigner::sign_holder_anchor_input` as we are unlikely to support keyed anchors for taproot channels.

Once we support zero-fee commitment transactions, we will no longer have a constant number of maximum HTLCs in-flight per channel but rather it will depend on the channel type. Here we prepare for this by removing the `MAX_HTLCS` constant and replacing it with a function.

TheBlueMatt · 2025-03-12T15:21:09Z

Squashed:

$ git diff-tree -U1 02ce69e59 68b16e24c
diff --git a/lightning/src/events/bump_transaction.rs b/lightning/src/events/bump_transaction.rs
index 77943845c..a9550a672 100644
--- a/lightning/src/events/bump_transaction.rs+++ b/lightning/src/events/bump_transaction.rs@@ -98,3 +98,3 @@ impl AnchorDescriptor {
} else {
- debug_assert!(tx_params.channel_type_features.supports_zero_fee_commitments());+ debug_assert!(tx_params.channel_type_features.supports_anchor_zero_fee_commitments());
Witness::from_slice(&[&[]])

TheBlueMatt · 2025-03-09T22:37:35Z

A few renames, some doc cleanups, and the feature bit itself.

We have a handful of methods to clear features from `*Features` objects, but have ended up with two different API semantics for them. In the next commit we'll make them all consistent, opting against the builder pattern as it turns out all of these lines utilizing it are too long for rustfmt to be happy, so best to clean them up now so that rustfmt doesn't make a mockery of our code later.

We've ad-hoc exposed feature-clearing methods for various feature flags over the years, but there's not a lot of reason to not just do it for all the flags, so we go ahead and do that here.

These aren't really used anywhere and were only live very briefly, so there's not really any point in informing our users that we don't support them. If anything, it will lead to confusion as the zero-HTLC-fee channel type is generally referred to simply as "anchor channels".

ldk-reviews-bot · 2025-03-09T22:37:38Z

👋 Thanks for assigning @wpaulino as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

codecov · 2025-03-10T02:05:34Z

Codecov Report

Attention: Patch coverage is 82.24299% with 19 lines in your changes missing coverage. Please review.

Project coverage is 89.23%. Comparing base (4c43a5b) to head (c15b29d).

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	56.52%	10 Missing ⚠️
lightning/src/events/bump_transaction.rs	78.94%	3 Missing and 1 partial ⚠️
lightning/src/ln/chan_utils.rs	82.35%	3 Missing ⚠️
lightning/src/sign/mod.rs	94.11%	0 Missing and 1 partial ⚠️
lightning/src/util/test_channel_signer.rs	75.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3656 +/- ##
==========================================
- Coverage 89.24% 89.23% -0.01% 
==========================================
Files 155 155 Lines 119280 119281 +1 Branches 119280 119281 +1 ==========================================
Hits 106446 106446 - Misses 10240 10241 +1 
Partials 2594 2594

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

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

tnull · 2025-03-10T10:33:58Z

CI is unhappy:

error[E0034]: multiple applicable items in scope
--> lightning/src/util/test_channel_signer.rs:463:14
|
463 | self.inner.sign_holder_keyed_anchor_input(chan_params, anchor_tx, input, secp_ctx)
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ multiple `sign_holder_keyed_anchor_input` found
|
note: candidate #1 is defined in an impl of the trait `TaprootChannelSigner` for the type `DynSigner`
--> lightning/src/util/dyn_signer.rs:121:2
|
121 | / fn sign_holder_keyed_anchor_input(
122 | | &self, anchor_tx: &Transaction, input: usize, secp_ctx: &Secp256k1<All>,
123 | | ) -> Result<secp256k1::schnorr::Signature, ()> {
| |__________________________________________________^
note: candidate #2 is defined in an impl of the trait `sign::ecdsa::EcdsaChannelSigner` for the type `DynSigner`
--> lightning/src/util/mod.rs:101:17
|
101 | fn $f(&$($mu)? self, $($n: $t),*) -> $r {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
::: lightning/src/util/dyn_signer.rs:134:1
|
134 | / delegate!(DynSigner, EcdsaChannelSigner, inner,
135 | | fn sign_holder_commitment(, channel_parameters: &ChannelTransactionParameters,
136 | | commitment_tx: &HolderCommitmentTransaction,
137 | | secp_ctx: &Secp256k1<secp256k1::All>) -> Result<Signature, ()>,
... |
168 | | secp_ctx: &Secp256k1<All>) -> Result<Signature, ()>
169 | | );
| |_- in this macro invocation
= note: this error originates in the macro `delegate` (in Nightly builds, run with -Z macro-backtrace for more info)
help: disambiguate the method for candidate #1
|
463 | TaprootChannelSigner::sign_holder_keyed_anchor_input(&self.inner, chan_params, anchor_tx, input, secp_ctx)
|
help: disambiguate the method for candidate #2
|
463 | sign::ecdsa::EcdsaChannelSigner::sign_holder_keyed_anchor_input(&self.inner, chan_params, anchor_tx, input, secp_ctx)
|

TheBlueMatt · 2025-03-10T12:37:22Z

Just removed the method on the taproot signer.

valentinewallace · 2025-03-10T18:14:46Z

full_stack fuzz is failing

valentinewallace

LGTM after fuzz is fixed

ldk-reviews-bot · 2025-03-10T18:22:00Z

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

wpaulino

LGTM

wpaulino · 2025-03-11T19:37:19Z

Good to squash, CI is still sad

...as it is ambiguous now that we have multiple types of anchors. Its clearer to check the specific features at the callsites now.

As we move towards zero-fee commitment transactions, we need to differentiate between the now-two-types of "anchor channels". We do so here by renaming a number of methods which refer to anchors as "keyed anchors" as the zero-fee commitment transaction anchors do not have a public key associated with them. We also drop `TaprootChannelSigner::sign_holder_anchor_input` as we are unlikely to support keyed anchors for taproot channels.

Once we support zero-fee commitment transactions, we will no longer have a constant number of maximum HTLCs in-flight per channel but rather it will depend on the channel type. Here we prepare for this by removing the `MAX_HTLCS` constant and replacing it with a function.

TheBlueMatt · 2025-03-12T15:21:09Z

Squashed:

$ git diff-tree -U1 02ce69e59 68b16e24c
diff --git a/lightning/src/events/bump_transaction.rs b/lightning/src/events/bump_transaction.rs
index 77943845c..a9550a672 100644
--- a/lightning/src/events/bump_transaction.rs+++ b/lightning/src/events/bump_transaction.rs@@ -98,3 +98,3 @@ impl AnchorDescriptor {
} else {
- debug_assert!(tx_params.channel_type_features.supports_zero_fee_commitments());+ debug_assert!(tx_params.channel_type_features.supports_anchor_zero_fee_commitments());
Witness::from_slice(&[&[]])

TheBlueMatt · 2025-03-09T22:37:35Z

A few renames, some doc cleanups, and the feature bit itself.

We have a handful of methods to clear features from `*Features` objects, but have ended up with two different API semantics for them. In the next commit we'll make them all consistent, opting against the builder pattern as it turns out all of these lines utilizing it are too long for rustfmt to be happy, so best to clean them up now so that rustfmt doesn't make a mockery of our code later.

We've ad-hoc exposed feature-clearing methods for various feature flags over the years, but there's not a lot of reason to not just do it for all the flags, so we go ahead and do that here.

These aren't really used anywhere and were only live very briefly, so there's not really any point in informing our users that we don't support them. If anything, it will lead to confusion as the zero-HTLC-fee channel type is generally referred to simply as "anchor channels".

ldk-reviews-bot · 2025-03-09T22:37:38Z

👋 Thanks for assigning @wpaulino as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

codecov · 2025-03-10T02:05:34Z

Codecov Report

Attention: Patch coverage is 82.24299% with 19 lines in your changes missing coverage. Please review.

Project coverage is 89.23%. Comparing base (4c43a5b) to head (c15b29d).

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	56.52%	10 Missing ⚠️
lightning/src/events/bump_transaction.rs	78.94%	3 Missing and 1 partial ⚠️
lightning/src/ln/chan_utils.rs	82.35%	3 Missing ⚠️
lightning/src/sign/mod.rs	94.11%	0 Missing and 1 partial ⚠️
lightning/src/util/test_channel_signer.rs	75.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3656 +/- ##
==========================================
- Coverage 89.24% 89.23% -0.01% 
==========================================
Files 155 155 Lines 119280 119281 +1 Branches 119280 119281 +1 ==========================================
Hits 106446 106446 - Misses 10240 10241 +1 
Partials 2594 2594

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

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

tnull · 2025-03-10T10:33:58Z

CI is unhappy:

error[E0034]: multiple applicable items in scope
--> lightning/src/util/test_channel_signer.rs:463:14
|
463 | self.inner.sign_holder_keyed_anchor_input(chan_params, anchor_tx, input, secp_ctx)
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ multiple `sign_holder_keyed_anchor_input` found
|
note: candidate #1 is defined in an impl of the trait `TaprootChannelSigner` for the type `DynSigner`
--> lightning/src/util/dyn_signer.rs:121:2
|
121 | / fn sign_holder_keyed_anchor_input(
122 | | &self, anchor_tx: &Transaction, input: usize, secp_ctx: &Secp256k1<All>,
123 | | ) -> Result<secp256k1::schnorr::Signature, ()> {
| |__________________________________________________^
note: candidate #2 is defined in an impl of the trait `sign::ecdsa::EcdsaChannelSigner` for the type `DynSigner`
--> lightning/src/util/mod.rs:101:17
|
101 | fn $f(&$($mu)? self, $($n: $t),*) -> $r {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
::: lightning/src/util/dyn_signer.rs:134:1
|
134 | / delegate!(DynSigner, EcdsaChannelSigner, inner,
135 | | fn sign_holder_commitment(, channel_parameters: &ChannelTransactionParameters,
136 | | commitment_tx: &HolderCommitmentTransaction,
137 | | secp_ctx: &Secp256k1<secp256k1::All>) -> Result<Signature, ()>,
... |
168 | | secp_ctx: &Secp256k1<All>) -> Result<Signature, ()>
169 | | );
| |_- in this macro invocation
= note: this error originates in the macro `delegate` (in Nightly builds, run with -Z macro-backtrace for more info)
help: disambiguate the method for candidate #1
|
463 | TaprootChannelSigner::sign_holder_keyed_anchor_input(&self.inner, chan_params, anchor_tx, input, secp_ctx)
|
help: disambiguate the method for candidate #2
|
463 | sign::ecdsa::EcdsaChannelSigner::sign_holder_keyed_anchor_input(&self.inner, chan_params, anchor_tx, input, secp_ctx)
|

TheBlueMatt · 2025-03-10T12:37:22Z

Just removed the method on the taproot signer.

valentinewallace · 2025-03-10T18:14:46Z

full_stack fuzz is failing

valentinewallace

LGTM after fuzz is fixed

ldk-reviews-bot · 2025-03-10T18:22:00Z

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

wpaulino

LGTM

wpaulino · 2025-03-11T19:37:19Z

Good to squash, CI is still sad

...as it is ambiguous now that we have multiple types of anchors. Its clearer to check the specific features at the callsites now.

As we move towards zero-fee commitment transactions, we need to differentiate between the now-two-types of "anchor channels". We do so here by renaming a number of methods which refer to anchors as "keyed anchors" as the zero-fee commitment transaction anchors do not have a public key associated with them. We also drop `TaprootChannelSigner::sign_holder_anchor_input` as we are unlikely to support keyed anchors for taproot channels.

Once we support zero-fee commitment transactions, we will no longer have a constant number of maximum HTLCs in-flight per channel but rather it will depend on the channel type. Here we prepare for this by removing the `MAX_HTLCS` constant and replacing it with a function.

TheBlueMatt · 2025-03-12T15:21:09Z

Squashed:

$ git diff-tree -U1 02ce69e59 68b16e24c
diff --git a/lightning/src/events/bump_transaction.rs b/lightning/src/events/bump_transaction.rs
index 77943845c..a9550a672 100644
--- a/lightning/src/events/bump_transaction.rs+++ b/lightning/src/events/bump_transaction.rs@@ -98,3 +98,3 @@ impl AnchorDescriptor {
} else {
- debug_assert!(tx_params.channel_type_features.supports_zero_fee_commitments());+ debug_assert!(tx_params.channel_type_features.supports_anchor_zero_fee_commitments());
Witness::from_slice(&[&[]])

TheBlueMatt · 2025-03-09T22:37:35Z

A few renames, some doc cleanups, and the feature bit itself.

We have a handful of methods to clear features from `*Features` objects, but have ended up with two different API semantics for them. In the next commit we'll make them all consistent, opting against the builder pattern as it turns out all of these lines utilizing it are too long for rustfmt to be happy, so best to clean them up now so that rustfmt doesn't make a mockery of our code later.

We've ad-hoc exposed feature-clearing methods for various feature flags over the years, but there's not a lot of reason to not just do it for all the flags, so we go ahead and do that here.

These aren't really used anywhere and were only live very briefly, so there's not really any point in informing our users that we don't support them. If anything, it will lead to confusion as the zero-HTLC-fee channel type is generally referred to simply as "anchor channels".

ldk-reviews-bot · 2025-03-09T22:37:38Z

👋 Thanks for assigning @wpaulino as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

codecov · 2025-03-10T02:05:34Z

Codecov Report

Attention: Patch coverage is 82.24299% with 19 lines in your changes missing coverage. Please review.

Project coverage is 89.23%. Comparing base (4c43a5b) to head (c15b29d).

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	56.52%	10 Missing ⚠️
lightning/src/events/bump_transaction.rs	78.94%	3 Missing and 1 partial ⚠️
lightning/src/ln/chan_utils.rs	82.35%	3 Missing ⚠️
lightning/src/sign/mod.rs	94.11%	0 Missing and 1 partial ⚠️
lightning/src/util/test_channel_signer.rs	75.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3656 +/- ##
==========================================
- Coverage 89.24% 89.23% -0.01% 
==========================================
Files 155 155 Lines 119280 119281 +1 Branches 119280 119281 +1 ==========================================
Hits 106446 106446 - Misses 10240 10241 +1 
Partials 2594 2594

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

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

tnull · 2025-03-10T10:33:58Z

CI is unhappy:

error[E0034]: multiple applicable items in scope
--> lightning/src/util/test_channel_signer.rs:463:14
|
463 | self.inner.sign_holder_keyed_anchor_input(chan_params, anchor_tx, input, secp_ctx)
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ multiple `sign_holder_keyed_anchor_input` found
|
note: candidate #1 is defined in an impl of the trait `TaprootChannelSigner` for the type `DynSigner`
--> lightning/src/util/dyn_signer.rs:121:2
|
121 | / fn sign_holder_keyed_anchor_input(
122 | | &self, anchor_tx: &Transaction, input: usize, secp_ctx: &Secp256k1<All>,
123 | | ) -> Result<secp256k1::schnorr::Signature, ()> {
| |__________________________________________________^
note: candidate #2 is defined in an impl of the trait `sign::ecdsa::EcdsaChannelSigner` for the type `DynSigner`
--> lightning/src/util/mod.rs:101:17
|
101 | fn $f(&$($mu)? self, $($n: $t),*) -> $r {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
::: lightning/src/util/dyn_signer.rs:134:1
|
134 | / delegate!(DynSigner, EcdsaChannelSigner, inner,
135 | | fn sign_holder_commitment(, channel_parameters: &ChannelTransactionParameters,
136 | | commitment_tx: &HolderCommitmentTransaction,
137 | | secp_ctx: &Secp256k1<secp256k1::All>) -> Result<Signature, ()>,
... |
168 | | secp_ctx: &Secp256k1<All>) -> Result<Signature, ()>
169 | | );
| |_- in this macro invocation
= note: this error originates in the macro `delegate` (in Nightly builds, run with -Z macro-backtrace for more info)
help: disambiguate the method for candidate #1
|
463 | TaprootChannelSigner::sign_holder_keyed_anchor_input(&self.inner, chan_params, anchor_tx, input, secp_ctx)
|
help: disambiguate the method for candidate #2
|
463 | sign::ecdsa::EcdsaChannelSigner::sign_holder_keyed_anchor_input(&self.inner, chan_params, anchor_tx, input, secp_ctx)
|

TheBlueMatt · 2025-03-10T12:37:22Z

Just removed the method on the taproot signer.

valentinewallace · 2025-03-10T18:14:46Z

full_stack fuzz is failing

valentinewallace

LGTM after fuzz is fixed

ldk-reviews-bot · 2025-03-10T18:22:00Z

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

wpaulino

LGTM

wpaulino · 2025-03-11T19:37:19Z

Good to squash, CI is still sad

...as it is ambiguous now that we have multiple types of anchors. Its clearer to check the specific features at the callsites now.

As we move towards zero-fee commitment transactions, we need to differentiate between the now-two-types of "anchor channels". We do so here by renaming a number of methods which refer to anchors as "keyed anchors" as the zero-fee commitment transaction anchors do not have a public key associated with them. We also drop `TaprootChannelSigner::sign_holder_anchor_input` as we are unlikely to support keyed anchors for taproot channels.

Once we support zero-fee commitment transactions, we will no longer have a constant number of maximum HTLCs in-flight per channel but rather it will depend on the channel type. Here we prepare for this by removing the `MAX_HTLCS` constant and replacing it with a function.

TheBlueMatt · 2025-03-12T15:21:09Z

Squashed:

$ git diff-tree -U1 02ce69e59 68b16e24c
diff --git a/lightning/src/events/bump_transaction.rs b/lightning/src/events/bump_transaction.rs
index 77943845c..a9550a672 100644
--- a/lightning/src/events/bump_transaction.rs+++ b/lightning/src/events/bump_transaction.rs@@ -98,3 +98,3 @@ impl AnchorDescriptor {
} else {
- debug_assert!(tx_params.channel_type_features.supports_zero_fee_commitments());+ debug_assert!(tx_params.channel_type_features.supports_anchor_zero_fee_commitments());
Witness::from_slice(&[&[]])

TheBlueMatt · 2025-03-09T22:37:35Z

A few renames, some doc cleanups, and the feature bit itself.

We have a handful of methods to clear features from `*Features` objects, but have ended up with two different API semantics for them. In the next commit we'll make them all consistent, opting against the builder pattern as it turns out all of these lines utilizing it are too long for rustfmt to be happy, so best to clean them up now so that rustfmt doesn't make a mockery of our code later.

We've ad-hoc exposed feature-clearing methods for various feature flags over the years, but there's not a lot of reason to not just do it for all the flags, so we go ahead and do that here.

These aren't really used anywhere and were only live very briefly, so there's not really any point in informing our users that we don't support them. If anything, it will lead to confusion as the zero-HTLC-fee channel type is generally referred to simply as "anchor channels".

ldk-reviews-bot · 2025-03-09T22:37:38Z

👋 Thanks for assigning @wpaulino as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

codecov · 2025-03-10T02:05:34Z

Codecov Report

Attention: Patch coverage is 82.24299% with 19 lines in your changes missing coverage. Please review.

Project coverage is 89.23%. Comparing base (4c43a5b) to head (c15b29d).

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	56.52%	10 Missing ⚠️
lightning/src/events/bump_transaction.rs	78.94%	3 Missing and 1 partial ⚠️
lightning/src/ln/chan_utils.rs	82.35%	3 Missing ⚠️
lightning/src/sign/mod.rs	94.11%	0 Missing and 1 partial ⚠️
lightning/src/util/test_channel_signer.rs	75.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3656 +/- ##
==========================================
- Coverage 89.24% 89.23% -0.01% 
==========================================
Files 155 155 Lines 119280 119281 +1 Branches 119280 119281 +1 ==========================================
Hits 106446 106446 - Misses 10240 10241 +1 
Partials 2594 2594

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

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

tnull · 2025-03-10T10:33:58Z

CI is unhappy:

error[E0034]: multiple applicable items in scope
--> lightning/src/util/test_channel_signer.rs:463:14
|
463 | self.inner.sign_holder_keyed_anchor_input(chan_params, anchor_tx, input, secp_ctx)
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ multiple `sign_holder_keyed_anchor_input` found
|
note: candidate #1 is defined in an impl of the trait `TaprootChannelSigner` for the type `DynSigner`
--> lightning/src/util/dyn_signer.rs:121:2
|
121 | / fn sign_holder_keyed_anchor_input(
122 | | &self, anchor_tx: &Transaction, input: usize, secp_ctx: &Secp256k1<All>,
123 | | ) -> Result<secp256k1::schnorr::Signature, ()> {
| |__________________________________________________^
note: candidate #2 is defined in an impl of the trait `sign::ecdsa::EcdsaChannelSigner` for the type `DynSigner`
--> lightning/src/util/mod.rs:101:17
|
101 | fn $f(&$($mu)? self, $($n: $t),*) -> $r {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
::: lightning/src/util/dyn_signer.rs:134:1
|
134 | / delegate!(DynSigner, EcdsaChannelSigner, inner,
135 | | fn sign_holder_commitment(, channel_parameters: &ChannelTransactionParameters,
136 | | commitment_tx: &HolderCommitmentTransaction,
137 | | secp_ctx: &Secp256k1<secp256k1::All>) -> Result<Signature, ()>,
... |
168 | | secp_ctx: &Secp256k1<All>) -> Result<Signature, ()>
169 | | );
| |_- in this macro invocation
= note: this error originates in the macro `delegate` (in Nightly builds, run with -Z macro-backtrace for more info)
help: disambiguate the method for candidate #1
|
463 | TaprootChannelSigner::sign_holder_keyed_anchor_input(&self.inner, chan_params, anchor_tx, input, secp_ctx)
|
help: disambiguate the method for candidate #2
|
463 | sign::ecdsa::EcdsaChannelSigner::sign_holder_keyed_anchor_input(&self.inner, chan_params, anchor_tx, input, secp_ctx)
|

TheBlueMatt · 2025-03-10T12:37:22Z

Just removed the method on the taproot signer.

valentinewallace · 2025-03-10T18:14:46Z

full_stack fuzz is failing

valentinewallace

LGTM after fuzz is fixed

ldk-reviews-bot · 2025-03-10T18:22:00Z

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

wpaulino

LGTM

wpaulino · 2025-03-11T19:37:19Z

Good to squash, CI is still sad

...as it is ambiguous now that we have multiple types of anchors. Its clearer to check the specific features at the callsites now.

As we move towards zero-fee commitment transactions, we need to differentiate between the now-two-types of "anchor channels". We do so here by renaming a number of methods which refer to anchors as "keyed anchors" as the zero-fee commitment transaction anchors do not have a public key associated with them. We also drop `TaprootChannelSigner::sign_holder_anchor_input` as we are unlikely to support keyed anchors for taproot channels.

Once we support zero-fee commitment transactions, we will no longer have a constant number of maximum HTLCs in-flight per channel but rather it will depend on the channel type. Here we prepare for this by removing the `MAX_HTLCS` constant and replacing it with a function.

TheBlueMatt · 2025-03-12T15:21:09Z

Squashed:

$ git diff-tree -U1 02ce69e59 68b16e24c
diff --git a/lightning/src/events/bump_transaction.rs b/lightning/src/events/bump_transaction.rs
index 77943845c..a9550a672 100644
--- a/lightning/src/events/bump_transaction.rs+++ b/lightning/src/events/bump_transaction.rs@@ -98,3 +98,3 @@ impl AnchorDescriptor {
} else {
- debug_assert!(tx_params.channel_type_features.supports_zero_fee_commitments());+ debug_assert!(tx_params.channel_type_features.supports_anchor_zero_fee_commitments());
Witness::from_slice(&[&[]])

TheBlueMatt · 2025-03-09T22:37:35Z

A few renames, some doc cleanups, and the feature bit itself.

We have a handful of methods to clear features from `*Features` objects, but have ended up with two different API semantics for them. In the next commit we'll make them all consistent, opting against the builder pattern as it turns out all of these lines utilizing it are too long for rustfmt to be happy, so best to clean them up now so that rustfmt doesn't make a mockery of our code later.

We've ad-hoc exposed feature-clearing methods for various feature flags over the years, but there's not a lot of reason to not just do it for all the flags, so we go ahead and do that here.

These aren't really used anywhere and were only live very briefly, so there's not really any point in informing our users that we don't support them. If anything, it will lead to confusion as the zero-HTLC-fee channel type is generally referred to simply as "anchor channels".

ldk-reviews-bot · 2025-03-09T22:37:38Z

👋 Thanks for assigning @wpaulino as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

codecov · 2025-03-10T02:05:34Z

Codecov Report

Attention: Patch coverage is 82.24299% with 19 lines in your changes missing coverage. Please review.

Project coverage is 89.23%. Comparing base (4c43a5b) to head (c15b29d).

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	56.52%	10 Missing ⚠️
lightning/src/events/bump_transaction.rs	78.94%	3 Missing and 1 partial ⚠️
lightning/src/ln/chan_utils.rs	82.35%	3 Missing ⚠️
lightning/src/sign/mod.rs	94.11%	0 Missing and 1 partial ⚠️
lightning/src/util/test_channel_signer.rs	75.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3656 +/- ##
==========================================
- Coverage 89.24% 89.23% -0.01% 
==========================================
Files 155 155 Lines 119280 119281 +1 Branches 119280 119281 +1 ==========================================
Hits 106446 106446 - Misses 10240 10241 +1 
Partials 2594 2594

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

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

tnull · 2025-03-10T10:33:58Z

CI is unhappy:

error[E0034]: multiple applicable items in scope
--> lightning/src/util/test_channel_signer.rs:463:14
|
463 | self.inner.sign_holder_keyed_anchor_input(chan_params, anchor_tx, input, secp_ctx)
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ multiple `sign_holder_keyed_anchor_input` found
|
note: candidate #1 is defined in an impl of the trait `TaprootChannelSigner` for the type `DynSigner`
--> lightning/src/util/dyn_signer.rs:121:2
|
121 | / fn sign_holder_keyed_anchor_input(
122 | | &self, anchor_tx: &Transaction, input: usize, secp_ctx: &Secp256k1<All>,
123 | | ) -> Result<secp256k1::schnorr::Signature, ()> {
| |__________________________________________________^
note: candidate #2 is defined in an impl of the trait `sign::ecdsa::EcdsaChannelSigner` for the type `DynSigner`
--> lightning/src/util/mod.rs:101:17
|
101 | fn $f(&$($mu)? self, $($n: $t),*) -> $r {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
::: lightning/src/util/dyn_signer.rs:134:1
|
134 | / delegate!(DynSigner, EcdsaChannelSigner, inner,
135 | | fn sign_holder_commitment(, channel_parameters: &ChannelTransactionParameters,
136 | | commitment_tx: &HolderCommitmentTransaction,
137 | | secp_ctx: &Secp256k1<secp256k1::All>) -> Result<Signature, ()>,
... |
168 | | secp_ctx: &Secp256k1<All>) -> Result<Signature, ()>
169 | | );
| |_- in this macro invocation
= note: this error originates in the macro `delegate` (in Nightly builds, run with -Z macro-backtrace for more info)
help: disambiguate the method for candidate #1
|
463 | TaprootChannelSigner::sign_holder_keyed_anchor_input(&self.inner, chan_params, anchor_tx, input, secp_ctx)
|
help: disambiguate the method for candidate #2
|
463 | sign::ecdsa::EcdsaChannelSigner::sign_holder_keyed_anchor_input(&self.inner, chan_params, anchor_tx, input, secp_ctx)
|

TheBlueMatt · 2025-03-10T12:37:22Z

Just removed the method on the taproot signer.

valentinewallace · 2025-03-10T18:14:46Z

full_stack fuzz is failing

valentinewallace

LGTM after fuzz is fixed

ldk-reviews-bot · 2025-03-10T18:22:00Z

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.

wpaulino

LGTM

wpaulino · 2025-03-11T19:37:19Z

Good to squash, CI is still sad

...as it is ambiguous now that we have multiple types of anchors. Its clearer to check the specific features at the callsites now.

As we move towards zero-fee commitment transactions, we need to differentiate between the now-two-types of "anchor channels". We do so here by renaming a number of methods which refer to anchors as "keyed anchors" as the zero-fee commitment transaction anchors do not have a public key associated with them. We also drop `TaprootChannelSigner::sign_holder_anchor_input` as we are unlikely to support keyed anchors for taproot channels.

Once we support zero-fee commitment transactions, we will no longer have a constant number of maximum HTLCs in-flight per channel but rather it will depend on the channel type. Here we prepare for this by removing the `MAX_HTLCS` constant and replacing it with a function.

TheBlueMatt · 2025-03-12T15:21:09Z

Squashed:

$ git diff-tree -U1 02ce69e59 68b16e24c
diff --git a/lightning/src/events/bump_transaction.rs b/lightning/src/events/bump_transaction.rs
index 77943845c..a9550a672 100644
--- a/lightning/src/events/bump_transaction.rs+++ b/lightning/src/events/bump_transaction.rs@@ -98,3 +98,3 @@ impl AnchorDescriptor {
} else {
- debug_assert!(tx_params.channel_type_features.supports_zero_fee_commitments());+ debug_assert!(tx_params.channel_type_features.supports_anchor_zero_fee_commitments());
Witness::from_slice(&[&[]])

TheBlueMatt added 3 commits March 9, 2025 22:33

Expose a feature-clearing method for all feature flags
f4c2a01
We've ad-hoc exposed feature-clearing methods for various feature flags over the years, but there's not a lot of reason to not just do it for all the flags, so we go ahead and do that here.

ldk-reviews-bot requested a review from valentinewallace March 9, 2025 22:48

TheBlueMatt force-pushed the 2025-03-zfc-part-1 branch from fe0a6c7 to 67c5993Compare March 10, 2025 01:56

TheBlueMatt force-pushed the 2025-03-zfc-part-1 branch from 67c5993 to c15b29dCompare March 10, 2025 12:36

valentinewallace reviewed Mar 10, 2025
View reviewed changes

wpaulino self-requested a review March 10, 2025 18:24

wpaulino reviewed Mar 10, 2025
View reviewed changes

Comment threadlightning/src/events/bump_transaction.rs
Comment threadlightning/src/events/bump_transaction.rs

TheBlueMatt force-pushed the 2025-03-zfc-part-1 branch from c15b29d to 02ce69eCompare March 11, 2025 19:19

TheBlueMatt added 4 commits March 12, 2025 15:07

Add feature flags for option_zero_fee_commitments
3e7b861

Drop ChannelTransactionParameters::supports_anchors
0faf17b
...as it is ambiguous now that we have multiple types of anchors. Its clearer to check the specific features at the callsites now.

TheBlueMatt force-pushed the 2025-03-zfc-part-1 branch from 02ce69e to 68b16e2Compare March 12, 2025 15:20

wpaulino approved these changes Mar 12, 2025
View reviewed changes

valentinewallace approved these changes Mar 12, 2025
View reviewed changes

wpaulino merged commit aaef672 into lightningdevkit:mainMar 12, 2025

carlaKC mentioned this pull request May 21, 2025
Zero Fee Commitments #3789
Closed
43 tasks

TheBlueMatt added 3 commits March 9, 2025 22:33

Expose a feature-clearing method for all feature flags
f4c2a01
We've ad-hoc exposed feature-clearing methods for various feature flags over the years, but there's not a lot of reason to not just do it for all the flags, so we go ahead and do that here.

ldk-reviews-bot requested a review from valentinewallace March 9, 2025 22:48

TheBlueMatt force-pushed the 2025-03-zfc-part-1 branch from fe0a6c7 to 67c5993Compare March 10, 2025 01:56

TheBlueMatt force-pushed the 2025-03-zfc-part-1 branch from 67c5993 to c15b29dCompare March 10, 2025 12:36

valentinewallace reviewed Mar 10, 2025
View reviewed changes

wpaulino self-requested a review March 10, 2025 18:24

wpaulino reviewed Mar 10, 2025
View reviewed changes

Comment threadlightning/src/events/bump_transaction.rs
Comment threadlightning/src/events/bump_transaction.rs

TheBlueMatt force-pushed the 2025-03-zfc-part-1 branch from c15b29d to 02ce69eCompare March 11, 2025 19:19

TheBlueMatt added 4 commits March 12, 2025 15:07

Add feature flags for option_zero_fee_commitments
3e7b861

Drop ChannelTransactionParameters::supports_anchors
0faf17b
...as it is ambiguous now that we have multiple types of anchors. Its clearer to check the specific features at the callsites now.

TheBlueMatt force-pushed the 2025-03-zfc-part-1 branch from 02ce69e to 68b16e2Compare March 12, 2025 15:20

wpaulino approved these changes Mar 12, 2025
View reviewed changes

valentinewallace approved these changes Mar 12, 2025
View reviewed changes

wpaulino merged commit aaef672 into lightningdevkit:mainMar 12, 2025

carlaKC mentioned this pull request May 21, 2025
Zero Fee Commitments #3789
Closed
43 tasks

TheBlueMatt added 3 commits March 9, 2025 22:33

Expose a feature-clearing method for all feature flags
f4c2a01
We've ad-hoc exposed feature-clearing methods for various feature flags over the years, but there's not a lot of reason to not just do it for all the flags, so we go ahead and do that here.

ldk-reviews-bot requested a review from valentinewallace March 9, 2025 22:48

TheBlueMatt force-pushed the 2025-03-zfc-part-1 branch from fe0a6c7 to 67c5993Compare March 10, 2025 01:56

TheBlueMatt force-pushed the 2025-03-zfc-part-1 branch from 67c5993 to c15b29dCompare March 10, 2025 12:36

valentinewallace reviewed Mar 10, 2025
View reviewed changes

wpaulino self-requested a review March 10, 2025 18:24

wpaulino reviewed Mar 10, 2025
View reviewed changes

Comment threadlightning/src/events/bump_transaction.rs
Comment threadlightning/src/events/bump_transaction.rs

TheBlueMatt force-pushed the 2025-03-zfc-part-1 branch from c15b29d to 02ce69eCompare March 11, 2025 19:19

TheBlueMatt added 4 commits March 12, 2025 15:07

Add feature flags for option_zero_fee_commitments
3e7b861

Drop ChannelTransactionParameters::supports_anchors
0faf17b
...as it is ambiguous now that we have multiple types of anchors. Its clearer to check the specific features at the callsites now.

TheBlueMatt force-pushed the 2025-03-zfc-part-1 branch from 02ce69e to 68b16e2Compare March 12, 2025 15:20

wpaulino approved these changes Mar 12, 2025
View reviewed changes

valentinewallace approved these changes Mar 12, 2025
View reviewed changes

wpaulino merged commit aaef672 into lightningdevkit:mainMar 12, 2025

carlaKC mentioned this pull request May 21, 2025
Zero Fee Commitments #3789
Closed
43 tasks

TheBlueMatt added 3 commits March 9, 2025 22:33

Expose a feature-clearing method for all feature flags
f4c2a01
We've ad-hoc exposed feature-clearing methods for various feature flags over the years, but there's not a lot of reason to not just do it for all the flags, so we go ahead and do that here.

ldk-reviews-bot requested a review from valentinewallace March 9, 2025 22:48

TheBlueMatt force-pushed the 2025-03-zfc-part-1 branch from fe0a6c7 to 67c5993Compare March 10, 2025 01:56

TheBlueMatt force-pushed the 2025-03-zfc-part-1 branch from 67c5993 to c15b29dCompare March 10, 2025 12:36

valentinewallace reviewed Mar 10, 2025
View reviewed changes

wpaulino self-requested a review March 10, 2025 18:24

wpaulino reviewed Mar 10, 2025
View reviewed changes

Comment threadlightning/src/events/bump_transaction.rs
Comment threadlightning/src/events/bump_transaction.rs

TheBlueMatt force-pushed the 2025-03-zfc-part-1 branch from c15b29d to 02ce69eCompare March 11, 2025 19:19

TheBlueMatt added 4 commits March 12, 2025 15:07

Add feature flags for option_zero_fee_commitments
3e7b861

Drop ChannelTransactionParameters::supports_anchors
0faf17b
...as it is ambiguous now that we have multiple types of anchors. Its clearer to check the specific features at the callsites now.

TheBlueMatt force-pushed the 2025-03-zfc-part-1 branch from 02ce69e to 68b16e2Compare March 12, 2025 15:20

wpaulino approved these changes Mar 12, 2025
View reviewed changes

valentinewallace approved these changes Mar 12, 2025
View reviewed changes

wpaulino merged commit aaef672 into lightningdevkit:mainMar 12, 2025

carlaKC mentioned this pull request May 21, 2025
Zero Fee Commitments #3789
Closed
43 tasks

TheBlueMatt added 3 commits March 9, 2025 22:33

Expose a feature-clearing method for all feature flags
f4c2a01
We've ad-hoc exposed feature-clearing methods for various feature flags over the years, but there's not a lot of reason to not just do it for all the flags, so we go ahead and do that here.

ldk-reviews-bot requested a review from valentinewallace March 9, 2025 22:48

TheBlueMatt force-pushed the 2025-03-zfc-part-1 branch from fe0a6c7 to 67c5993Compare March 10, 2025 01:56

TheBlueMatt force-pushed the 2025-03-zfc-part-1 branch from 67c5993 to c15b29dCompare March 10, 2025 12:36

valentinewallace reviewed Mar 10, 2025
View reviewed changes

wpaulino self-requested a review March 10, 2025 18:24

wpaulino reviewed Mar 10, 2025
View reviewed changes

Comment threadlightning/src/events/bump_transaction.rs
Comment threadlightning/src/events/bump_transaction.rs

TheBlueMatt force-pushed the 2025-03-zfc-part-1 branch from c15b29d to 02ce69eCompare March 11, 2025 19:19

TheBlueMatt added 4 commits March 12, 2025 15:07

Add feature flags for option_zero_fee_commitments
3e7b861

Drop ChannelTransactionParameters::supports_anchors
0faf17b
...as it is ambiguous now that we have multiple types of anchors. Its clearer to check the specific features at the callsites now.

TheBlueMatt force-pushed the 2025-03-zfc-part-1 branch from 02ce69e to 68b16e2Compare March 12, 2025 15:20

wpaulino approved these changes Mar 12, 2025
View reviewed changes

valentinewallace approved these changes Mar 12, 2025
View reviewed changes

wpaulino merged commit aaef672 into lightningdevkit:mainMar 12, 2025

carlaKC mentioned this pull request May 21, 2025
Zero Fee Commitments #3789
Closed
43 tasks

TheBlueMatt added 3 commits March 9, 2025 22:33

Expose a feature-clearing method for all feature flags
f4c2a01
We've ad-hoc exposed feature-clearing methods for various feature flags over the years, but there's not a lot of reason to not just do it for all the flags, so we go ahead and do that here.

ldk-reviews-bot requested a review from valentinewallace March 9, 2025 22:48

TheBlueMatt force-pushed the 2025-03-zfc-part-1 branch from fe0a6c7 to 67c5993Compare March 10, 2025 01:56

TheBlueMatt force-pushed the 2025-03-zfc-part-1 branch from 67c5993 to c15b29dCompare March 10, 2025 12:36

valentinewallace reviewed Mar 10, 2025
View reviewed changes

wpaulino self-requested a review March 10, 2025 18:24

wpaulino reviewed Mar 10, 2025
View reviewed changes

Comment threadlightning/src/events/bump_transaction.rs
Comment threadlightning/src/events/bump_transaction.rs

TheBlueMatt force-pushed the 2025-03-zfc-part-1 branch from c15b29d to 02ce69eCompare March 11, 2025 19:19

TheBlueMatt added 4 commits March 12, 2025 15:07

Add feature flags for option_zero_fee_commitments
3e7b861

Drop ChannelTransactionParameters::supports_anchors
0faf17b
...as it is ambiguous now that we have multiple types of anchors. Its clearer to check the specific features at the callsites now.

TheBlueMatt force-pushed the 2025-03-zfc-part-1 branch from 02ce69e to 68b16e2Compare March 12, 2025 15:20

wpaulino approved these changes Mar 12, 2025
View reviewed changes

valentinewallace approved these changes Mar 12, 2025
View reviewed changes

wpaulino merged commit aaef672 into lightningdevkit:mainMar 12, 2025

carlaKC mentioned this pull request May 21, 2025
Zero Fee Commitments #3789
Closed
43 tasks

TheBlueMatt added 3 commits March 9, 2025 22:33

Expose a feature-clearing method for all feature flags
f4c2a01
We've ad-hoc exposed feature-clearing methods for various feature flags over the years, but there's not a lot of reason to not just do it for all the flags, so we go ahead and do that here.

ldk-reviews-bot requested a review from valentinewallace March 9, 2025 22:48

TheBlueMatt force-pushed the 2025-03-zfc-part-1 branch from fe0a6c7 to 67c5993Compare March 10, 2025 01:56

TheBlueMatt force-pushed the 2025-03-zfc-part-1 branch from 67c5993 to c15b29dCompare March 10, 2025 12:36

valentinewallace reviewed Mar 10, 2025
View reviewed changes

wpaulino self-requested a review March 10, 2025 18:24

wpaulino reviewed Mar 10, 2025
View reviewed changes

Comment threadlightning/src/events/bump_transaction.rs
Comment threadlightning/src/events/bump_transaction.rs

TheBlueMatt force-pushed the 2025-03-zfc-part-1 branch from c15b29d to 02ce69eCompare March 11, 2025 19:19

TheBlueMatt added 4 commits March 12, 2025 15:07

Add feature flags for option_zero_fee_commitments
3e7b861

Drop ChannelTransactionParameters::supports_anchors
0faf17b
...as it is ambiguous now that we have multiple types of anchors. Its clearer to check the specific features at the callsites now.

TheBlueMatt force-pushed the 2025-03-zfc-part-1 branch from 02ce69e to 68b16e2Compare March 12, 2025 15:20

wpaulino approved these changes Mar 12, 2025
View reviewed changes

valentinewallace approved these changes Mar 12, 2025
View reviewed changes

wpaulino merged commit aaef672 into lightningdevkit:mainMar 12, 2025

carlaKC mentioned this pull request May 21, 2025
Zero Fee Commitments #3789
Closed
43 tasks

TheBlueMatt added 3 commits March 9, 2025 22:33

Expose a feature-clearing method for all feature flags
f4c2a01
We've ad-hoc exposed feature-clearing methods for various feature flags over the years, but there's not a lot of reason to not just do it for all the flags, so we go ahead and do that here.

ldk-reviews-bot requested a review from valentinewallace March 9, 2025 22:48

TheBlueMatt force-pushed the 2025-03-zfc-part-1 branch from fe0a6c7 to 67c5993Compare March 10, 2025 01:56

TheBlueMatt force-pushed the 2025-03-zfc-part-1 branch from 67c5993 to c15b29dCompare March 10, 2025 12:36

valentinewallace reviewed Mar 10, 2025
View reviewed changes

wpaulino self-requested a review March 10, 2025 18:24

wpaulino reviewed Mar 10, 2025
View reviewed changes

Comment threadlightning/src/events/bump_transaction.rs
Comment threadlightning/src/events/bump_transaction.rs

TheBlueMatt force-pushed the 2025-03-zfc-part-1 branch from c15b29d to 02ce69eCompare March 11, 2025 19:19

TheBlueMatt added 4 commits March 12, 2025 15:07

Add feature flags for option_zero_fee_commitments
3e7b861

Drop ChannelTransactionParameters::supports_anchors
0faf17b
...as it is ambiguous now that we have multiple types of anchors. Its clearer to check the specific features at the callsites now.

TheBlueMatt force-pushed the 2025-03-zfc-part-1 branch from 02ce69e to 68b16e2Compare March 12, 2025 15:20

wpaulino approved these changes Mar 12, 2025
View reviewed changes

valentinewallace approved these changes Mar 12, 2025
View reviewed changes

wpaulino merged commit aaef672 into lightningdevkit:mainMar 12, 2025

carlaKC mentioned this pull request May 21, 2025
Zero Fee Commitments #3789
Closed
43 tasks

Uh oh!

Conversation

TheBlueMatt commented Mar 9, 2025

Uh oh!

ldk-reviews-bot commented Mar 9, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecovBot commented Mar 10, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tnull commented Mar 10, 2025

Uh oh!

TheBlueMatt commented Mar 10, 2025

Uh oh!

valentinewallace commented Mar 10, 2025

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

ldk-reviews-bot commented Mar 10, 2025

Uh oh!

wpaulino left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

wpaulino commented Mar 11, 2025

Uh oh!

TheBlueMatt commented Mar 12, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

TheBlueMatt commented Mar 9, 2025

Uh oh!

ldk-reviews-bot commented Mar 9, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecovBot commented Mar 10, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tnull commented Mar 10, 2025

Uh oh!

TheBlueMatt commented Mar 10, 2025

Uh oh!

valentinewallace commented Mar 10, 2025

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

ldk-reviews-bot commented Mar 10, 2025

Uh oh!

wpaulino left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

wpaulino commented Mar 11, 2025

Uh oh!

TheBlueMatt commented Mar 12, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

ldk-reviews-bot commented Mar 9, 2025•
edited
Loading

codecovBot commented Mar 10, 2025•
edited
Loading

ldk-reviews-bot commented Mar 9, 2025•
edited
Loading

codecovBot commented Mar 10, 2025•
edited
Loading

ldk-reviews-bot commented Mar 9, 2025•
edited
Loading

codecovBot commented Mar 10, 2025•
edited
Loading

ldk-reviews-bot commented Mar 9, 2025•
edited
Loading

codecovBot commented Mar 10, 2025•
edited
Loading