Let `ChannelSigner` set `to_remote`, `to_local`, htlc tx scriptpubkeys by tankyleo · Pull Request #3454 · lightningdevkit/rust-lightning

tankyleo · 2024-12-10T18:05:29Z

See commit message for a description of this specific commit.

This begins work on allowing the customization of different outputs of the commitment transaction, in preparation for taproot channels and also to allow people to set the outputs to arbitrary scripts if they don't require compatibility with the formal LN spec.

This PR begins with the to_remote output, with the goal of illustrating the approach taken with a simple example.

If this approach is ok, I will next work on StaticPaymentOutputDescriptor, as it would need to be updated to handle the taproot / arbitrary scripts used in this output. I can do this work in this PR, or in a follow-up depending on your preference.

My approach is to ask the channel signers to do more work (but not more than that of most hardware wallets):

Set the appropriate SPK's for the different outputs of the commitment transaction.
Return the full witness to spend such an output (not just the signature); this is not shown here, but is the current plan for the to_local outputs in a justice scenario.

By putting scriptpubkey and witness construction behind the signer trait, we can have some parts of LDK be implemented in terms of scriptpubkeys and witnesses, and these parts can then remain the same across segwit, taproot, and arbitrary scripts.

Let me know what you think, thank you for your input.

cc @arik-so @TheBlueMatt

~~EDIT 2024-12-13~~

~~In cbac0e7 I apply the same approach to the to_local output of the commit tx, and in 72ca0a9 same approach to the htlc tx output.~~

EDIT 2024-12-15

~~In the last two commits, I apply the same approach to the to_local output of the commit tx, and to the htlc tx output.~~

I also apply the same approach to the to_local output of the commit tx, and to the htlc tx output later in the same patchset.

Instead of returning only the witness when punishing a revokeable output, I instead choose to ask the ChannelSigner to return a full transaction with the specified input finalized to punish the corresponding previous output. This is primarily to leave the possibility of a signer to customize how the sequence field of the input is set.

tankyleo · 2024-12-10T18:18:08Z

 best_block: BestBlock, counterparty_node_id: PublicKey, channel_id: ChannelId,
 	) -> ChannelMonitor<Signer> {
+
+ keys.provide_channel_parameters(channel_parameters);


I move the provide_channel_parameters call to here because there were some tests that built a ChannelMonitor without first calling provide_channel_parameters on the keys: Signer.
Further below in initial_commitment_signed, I delete the provide_channel_parameters call, as it is now called here.

We now need to call provide_channel_parameters before ChannelMonitor::new because ChannelSigner::get_counterparty_payment_script assumes that provide_channel_parameters has already been called - see the doc for ChannelSigner::get_counterparty_payment_script.

tankyleo · 2024-12-10T18:20:05Z

 let shutdown_script = context.shutdown_scriptpubkey.clone().map(|script| script.into_inner());
- let mut monitor_signer = signer_provider.derive_channel_signer(context.channel_value_satoshis, context.channel_keys_id);
- monitor_signer.provide_channel_parameters(&context.channel_transaction_parameters);
+ let monitor_signer = signer_provider.derive_channel_signer(context.channel_value_satoshis, context.channel_keys_id);


provide_channel_parameters is now called in ChannelMonitor::new, see reasoning above.

codecov · 2024-12-11T02:36:46Z

Codecov Report

Attention: Patch coverage is 97.61905% with 7 lines in your changes missing coverage. Please review.

Project coverage is 89.73%. Comparing base (1a8bf62) to head (0f0560a).
Report is 68 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/chan_utils.rs	95.45%	2 Missing and 2 partials ⚠️
lightning/src/sign/mod.rs	97.40%	0 Missing and 2 partials ⚠️
lightning/src/chain/channelmonitor.rs	97.22%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3454 +/- ##
==========================================
- Coverage 89.74% 89.73% -0.02% 
==========================================
Files 130 130 Lines 107793 107909 +116 Branches 107793 107909 +116 ==========================================
+ Hits 96743 96836 +93 - Misses 8651 8672 +21 - Partials 2399 2401 +2

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

tnull · 2024-12-11T09:03:04Z

I'll have to review this more closely, but want to note that this will very likely conflict with (the currently stale) #3391, which we're in the process of rewriting. We should coordinate to see how to resolve this, i.e., which approach we should take (first) so you can lean on it for follow-up work.

tankyleo · 2024-12-11T17:37:14Z

I'll have to review this more closely, but want to note that this will very likely conflict with (the currently stale) #3391, which we're in the process of rewriting. We should coordinate to see how to resolve this, i.e., which approach we should take (first) so you can lean on it for follow-up work.

Thank you for taking a look - I've made a first pass on 3391, and currently do not see any conflicts.

This allows the `to_remote` output to easily be changed according to the features of the channel, or the evolution of the LN specification. `to_remote` could even be set to completely arbitrary scripts if compatibility with the formal LN spec is not required. Builders of `CommitmentTransaction` now ask a `ChannelSigner` for the appropriate `to_remote` script to use, and then pass the script to the `CommitmentTransaction` constructor. External signers now provide the expected `to_remote` script to the `verify` call of `CommitmentTransaction`.

Builds of `CommitmentTransaction` now query a `ChannelSigner` for the script pubkey to use in the `to_remote` output of the commitment transaction. So we need to overwrite the `ChannelTransactionParameters` of a `ChannelSigner` anytime we want to build a new commitment transaction with a different set of features. This is feature is only used in tests, so we cfg-gate it behind the test flag.

This allows the `to_local` output to easily be changed according to the features of the channel, or the evolution of the LN specification. `to_local` could even be set to completely arbitrary scripts if compatibility with the formal LN spec is not required. Builders of `CommitmentTransaction` now ask a `ChannelSigner` for the appropriate `to_local` script pubkey to use, and then pass it to the `CommitmentTransaction` constructor. External signers now provide the expected `to_local` script pubkey to the `verify` call of `CommitmentTransaction`.

This allows the htlc tx output to easily be changed according to the features of the channel, or the evolution of the LN specification. The output could even be set to completely arbitrary scripts if compatibility with the formal LN spec is not required. Builders of htlc transactions now ask a `ChannelSigner` for the appropriate revokeable script pubkey to use, and then pass it to the htlc transaction constructors.

All LN-Penalty channel signers need to be able to punish the counterparty in case they broadcast an old state. In this commit, we ask implementers of `ChannelSigner` to produce the full transaction with the given input finalized to punish the corresponding previous output. Consumers of the `ChannelSigner` trait can now be agnostic to the specific scripts used in revokeable outputs. We leave passing to the `ChannelSigner` all the previous `TxOut`'s needed to produce valid schnorr signatures under BIP 341 spending rules to a later patch.

tankyleo · 2025-01-08T19:48:34Z

Close in favor of #3512

tankyleo · 2024-12-10T18:05:29Z

See commit message for a description of this specific commit.

This begins work on allowing the customization of different outputs of the commitment transaction, in preparation for taproot channels and also to allow people to set the outputs to arbitrary scripts if they don't require compatibility with the formal LN spec.

This PR begins with the to_remote output, with the goal of illustrating the approach taken with a simple example.

If this approach is ok, I will next work on StaticPaymentOutputDescriptor, as it would need to be updated to handle the taproot / arbitrary scripts used in this output. I can do this work in this PR, or in a follow-up depending on your preference.

My approach is to ask the channel signers to do more work (but not more than that of most hardware wallets):

Set the appropriate SPK's for the different outputs of the commitment transaction.
Return the full witness to spend such an output (not just the signature); this is not shown here, but is the current plan for the to_local outputs in a justice scenario.

By putting scriptpubkey and witness construction behind the signer trait, we can have some parts of LDK be implemented in terms of scriptpubkeys and witnesses, and these parts can then remain the same across segwit, taproot, and arbitrary scripts.

Let me know what you think, thank you for your input.

cc @arik-so @TheBlueMatt

~~EDIT 2024-12-13~~

~~In cbac0e7 I apply the same approach to the to_local output of the commit tx, and in 72ca0a9 same approach to the htlc tx output.~~

EDIT 2024-12-15

~~In the last two commits, I apply the same approach to the to_local output of the commit tx, and to the htlc tx output.~~

I also apply the same approach to the to_local output of the commit tx, and to the htlc tx output later in the same patchset.

Instead of returning only the witness when punishing a revokeable output, I instead choose to ask the ChannelSigner to return a full transaction with the specified input finalized to punish the corresponding previous output. This is primarily to leave the possibility of a signer to customize how the sequence field of the input is set.

tankyleo · 2024-12-10T18:18:08Z

 best_block: BestBlock, counterparty_node_id: PublicKey, channel_id: ChannelId,
 	) -> ChannelMonitor<Signer> {
+
+ keys.provide_channel_parameters(channel_parameters);


I move the provide_channel_parameters call to here because there were some tests that built a ChannelMonitor without first calling provide_channel_parameters on the keys: Signer.
Further below in initial_commitment_signed, I delete the provide_channel_parameters call, as it is now called here.

We now need to call provide_channel_parameters before ChannelMonitor::new because ChannelSigner::get_counterparty_payment_script assumes that provide_channel_parameters has already been called - see the doc for ChannelSigner::get_counterparty_payment_script.

tankyleo · 2024-12-10T18:20:05Z

 let shutdown_script = context.shutdown_scriptpubkey.clone().map(|script| script.into_inner());
- let mut monitor_signer = signer_provider.derive_channel_signer(context.channel_value_satoshis, context.channel_keys_id);
- monitor_signer.provide_channel_parameters(&context.channel_transaction_parameters);
+ let monitor_signer = signer_provider.derive_channel_signer(context.channel_value_satoshis, context.channel_keys_id);


provide_channel_parameters is now called in ChannelMonitor::new, see reasoning above.

codecov · 2024-12-11T02:36:46Z

Codecov Report

Attention: Patch coverage is 97.61905% with 7 lines in your changes missing coverage. Please review.

Project coverage is 89.73%. Comparing base (1a8bf62) to head (0f0560a).
Report is 68 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/chan_utils.rs	95.45%	2 Missing and 2 partials ⚠️
lightning/src/sign/mod.rs	97.40%	0 Missing and 2 partials ⚠️
lightning/src/chain/channelmonitor.rs	97.22%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3454 +/- ##
==========================================
- Coverage 89.74% 89.73% -0.02% 
==========================================
Files 130 130 Lines 107793 107909 +116 Branches 107793 107909 +116 ==========================================
+ Hits 96743 96836 +93 - Misses 8651 8672 +21 - Partials 2399 2401 +2

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

tnull · 2024-12-11T09:03:04Z

I'll have to review this more closely, but want to note that this will very likely conflict with (the currently stale) #3391, which we're in the process of rewriting. We should coordinate to see how to resolve this, i.e., which approach we should take (first) so you can lean on it for follow-up work.

tankyleo · 2024-12-11T17:37:14Z

I'll have to review this more closely, but want to note that this will very likely conflict with (the currently stale) #3391, which we're in the process of rewriting. We should coordinate to see how to resolve this, i.e., which approach we should take (first) so you can lean on it for follow-up work.

Thank you for taking a look - I've made a first pass on 3391, and currently do not see any conflicts.

This allows the `to_remote` output to easily be changed according to the features of the channel, or the evolution of the LN specification. `to_remote` could even be set to completely arbitrary scripts if compatibility with the formal LN spec is not required. Builders of `CommitmentTransaction` now ask a `ChannelSigner` for the appropriate `to_remote` script to use, and then pass the script to the `CommitmentTransaction` constructor. External signers now provide the expected `to_remote` script to the `verify` call of `CommitmentTransaction`.

Builds of `CommitmentTransaction` now query a `ChannelSigner` for the script pubkey to use in the `to_remote` output of the commitment transaction. So we need to overwrite the `ChannelTransactionParameters` of a `ChannelSigner` anytime we want to build a new commitment transaction with a different set of features. This is feature is only used in tests, so we cfg-gate it behind the test flag.

This allows the `to_local` output to easily be changed according to the features of the channel, or the evolution of the LN specification. `to_local` could even be set to completely arbitrary scripts if compatibility with the formal LN spec is not required. Builders of `CommitmentTransaction` now ask a `ChannelSigner` for the appropriate `to_local` script pubkey to use, and then pass it to the `CommitmentTransaction` constructor. External signers now provide the expected `to_local` script pubkey to the `verify` call of `CommitmentTransaction`.

This allows the htlc tx output to easily be changed according to the features of the channel, or the evolution of the LN specification. The output could even be set to completely arbitrary scripts if compatibility with the formal LN spec is not required. Builders of htlc transactions now ask a `ChannelSigner` for the appropriate revokeable script pubkey to use, and then pass it to the htlc transaction constructors.

All LN-Penalty channel signers need to be able to punish the counterparty in case they broadcast an old state. In this commit, we ask implementers of `ChannelSigner` to produce the full transaction with the given input finalized to punish the corresponding previous output. Consumers of the `ChannelSigner` trait can now be agnostic to the specific scripts used in revokeable outputs. We leave passing to the `ChannelSigner` all the previous `TxOut`'s needed to produce valid schnorr signatures under BIP 341 spending rules to a later patch.

tankyleo · 2025-01-08T19:48:34Z

Close in favor of #3512

tankyleo · 2024-12-10T18:05:29Z

See commit message for a description of this specific commit.

This begins work on allowing the customization of different outputs of the commitment transaction, in preparation for taproot channels and also to allow people to set the outputs to arbitrary scripts if they don't require compatibility with the formal LN spec.

This PR begins with the to_remote output, with the goal of illustrating the approach taken with a simple example.

If this approach is ok, I will next work on StaticPaymentOutputDescriptor, as it would need to be updated to handle the taproot / arbitrary scripts used in this output. I can do this work in this PR, or in a follow-up depending on your preference.

My approach is to ask the channel signers to do more work (but not more than that of most hardware wallets):

Set the appropriate SPK's for the different outputs of the commitment transaction.
Return the full witness to spend such an output (not just the signature); this is not shown here, but is the current plan for the to_local outputs in a justice scenario.

By putting scriptpubkey and witness construction behind the signer trait, we can have some parts of LDK be implemented in terms of scriptpubkeys and witnesses, and these parts can then remain the same across segwit, taproot, and arbitrary scripts.

Let me know what you think, thank you for your input.

cc @arik-so @TheBlueMatt

~~EDIT 2024-12-13~~

~~In cbac0e7 I apply the same approach to the to_local output of the commit tx, and in 72ca0a9 same approach to the htlc tx output.~~

EDIT 2024-12-15

~~In the last two commits, I apply the same approach to the to_local output of the commit tx, and to the htlc tx output.~~

I also apply the same approach to the to_local output of the commit tx, and to the htlc tx output later in the same patchset.

Instead of returning only the witness when punishing a revokeable output, I instead choose to ask the ChannelSigner to return a full transaction with the specified input finalized to punish the corresponding previous output. This is primarily to leave the possibility of a signer to customize how the sequence field of the input is set.

tankyleo · 2024-12-10T18:18:08Z

 best_block: BestBlock, counterparty_node_id: PublicKey, channel_id: ChannelId,
 	) -> ChannelMonitor<Signer> {
+
+ keys.provide_channel_parameters(channel_parameters);


I move the provide_channel_parameters call to here because there were some tests that built a ChannelMonitor without first calling provide_channel_parameters on the keys: Signer.
Further below in initial_commitment_signed, I delete the provide_channel_parameters call, as it is now called here.

We now need to call provide_channel_parameters before ChannelMonitor::new because ChannelSigner::get_counterparty_payment_script assumes that provide_channel_parameters has already been called - see the doc for ChannelSigner::get_counterparty_payment_script.

tankyleo · 2024-12-10T18:20:05Z

 let shutdown_script = context.shutdown_scriptpubkey.clone().map(|script| script.into_inner());
- let mut monitor_signer = signer_provider.derive_channel_signer(context.channel_value_satoshis, context.channel_keys_id);
- monitor_signer.provide_channel_parameters(&context.channel_transaction_parameters);
+ let monitor_signer = signer_provider.derive_channel_signer(context.channel_value_satoshis, context.channel_keys_id);


provide_channel_parameters is now called in ChannelMonitor::new, see reasoning above.

codecov · 2024-12-11T02:36:46Z

Codecov Report

Attention: Patch coverage is 97.61905% with 7 lines in your changes missing coverage. Please review.

Project coverage is 89.73%. Comparing base (1a8bf62) to head (0f0560a).
Report is 68 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/chan_utils.rs	95.45%	2 Missing and 2 partials ⚠️
lightning/src/sign/mod.rs	97.40%	0 Missing and 2 partials ⚠️
lightning/src/chain/channelmonitor.rs	97.22%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3454 +/- ##
==========================================
- Coverage 89.74% 89.73% -0.02% 
==========================================
Files 130 130 Lines 107793 107909 +116 Branches 107793 107909 +116 ==========================================
+ Hits 96743 96836 +93 - Misses 8651 8672 +21 - Partials 2399 2401 +2

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

tnull · 2024-12-11T09:03:04Z

I'll have to review this more closely, but want to note that this will very likely conflict with (the currently stale) #3391, which we're in the process of rewriting. We should coordinate to see how to resolve this, i.e., which approach we should take (first) so you can lean on it for follow-up work.

tankyleo · 2024-12-11T17:37:14Z

I'll have to review this more closely, but want to note that this will very likely conflict with (the currently stale) #3391, which we're in the process of rewriting. We should coordinate to see how to resolve this, i.e., which approach we should take (first) so you can lean on it for follow-up work.

Thank you for taking a look - I've made a first pass on 3391, and currently do not see any conflicts.

This allows the `to_remote` output to easily be changed according to the features of the channel, or the evolution of the LN specification. `to_remote` could even be set to completely arbitrary scripts if compatibility with the formal LN spec is not required. Builders of `CommitmentTransaction` now ask a `ChannelSigner` for the appropriate `to_remote` script to use, and then pass the script to the `CommitmentTransaction` constructor. External signers now provide the expected `to_remote` script to the `verify` call of `CommitmentTransaction`.

Builds of `CommitmentTransaction` now query a `ChannelSigner` for the script pubkey to use in the `to_remote` output of the commitment transaction. So we need to overwrite the `ChannelTransactionParameters` of a `ChannelSigner` anytime we want to build a new commitment transaction with a different set of features. This is feature is only used in tests, so we cfg-gate it behind the test flag.

This allows the `to_local` output to easily be changed according to the features of the channel, or the evolution of the LN specification. `to_local` could even be set to completely arbitrary scripts if compatibility with the formal LN spec is not required. Builders of `CommitmentTransaction` now ask a `ChannelSigner` for the appropriate `to_local` script pubkey to use, and then pass it to the `CommitmentTransaction` constructor. External signers now provide the expected `to_local` script pubkey to the `verify` call of `CommitmentTransaction`.

This allows the htlc tx output to easily be changed according to the features of the channel, or the evolution of the LN specification. The output could even be set to completely arbitrary scripts if compatibility with the formal LN spec is not required. Builders of htlc transactions now ask a `ChannelSigner` for the appropriate revokeable script pubkey to use, and then pass it to the htlc transaction constructors.

All LN-Penalty channel signers need to be able to punish the counterparty in case they broadcast an old state. In this commit, we ask implementers of `ChannelSigner` to produce the full transaction with the given input finalized to punish the corresponding previous output. Consumers of the `ChannelSigner` trait can now be agnostic to the specific scripts used in revokeable outputs. We leave passing to the `ChannelSigner` all the previous `TxOut`'s needed to produce valid schnorr signatures under BIP 341 spending rules to a later patch.

tankyleo · 2025-01-08T19:48:34Z

Close in favor of #3512

tankyleo · 2024-12-10T18:05:29Z

See commit message for a description of this specific commit.

This begins work on allowing the customization of different outputs of the commitment transaction, in preparation for taproot channels and also to allow people to set the outputs to arbitrary scripts if they don't require compatibility with the formal LN spec.

This PR begins with the to_remote output, with the goal of illustrating the approach taken with a simple example.

If this approach is ok, I will next work on StaticPaymentOutputDescriptor, as it would need to be updated to handle the taproot / arbitrary scripts used in this output. I can do this work in this PR, or in a follow-up depending on your preference.

My approach is to ask the channel signers to do more work (but not more than that of most hardware wallets):

Set the appropriate SPK's for the different outputs of the commitment transaction.
Return the full witness to spend such an output (not just the signature); this is not shown here, but is the current plan for the to_local outputs in a justice scenario.

By putting scriptpubkey and witness construction behind the signer trait, we can have some parts of LDK be implemented in terms of scriptpubkeys and witnesses, and these parts can then remain the same across segwit, taproot, and arbitrary scripts.

Let me know what you think, thank you for your input.

cc @arik-so @TheBlueMatt

~~EDIT 2024-12-13~~

~~In cbac0e7 I apply the same approach to the to_local output of the commit tx, and in 72ca0a9 same approach to the htlc tx output.~~

EDIT 2024-12-15

~~In the last two commits, I apply the same approach to the to_local output of the commit tx, and to the htlc tx output.~~

I also apply the same approach to the to_local output of the commit tx, and to the htlc tx output later in the same patchset.

Instead of returning only the witness when punishing a revokeable output, I instead choose to ask the ChannelSigner to return a full transaction with the specified input finalized to punish the corresponding previous output. This is primarily to leave the possibility of a signer to customize how the sequence field of the input is set.

tankyleo · 2024-12-10T18:18:08Z

 best_block: BestBlock, counterparty_node_id: PublicKey, channel_id: ChannelId,
 	) -> ChannelMonitor<Signer> {
+
+ keys.provide_channel_parameters(channel_parameters);


I move the provide_channel_parameters call to here because there were some tests that built a ChannelMonitor without first calling provide_channel_parameters on the keys: Signer.
Further below in initial_commitment_signed, I delete the provide_channel_parameters call, as it is now called here.

We now need to call provide_channel_parameters before ChannelMonitor::new because ChannelSigner::get_counterparty_payment_script assumes that provide_channel_parameters has already been called - see the doc for ChannelSigner::get_counterparty_payment_script.

tankyleo · 2024-12-10T18:20:05Z

 let shutdown_script = context.shutdown_scriptpubkey.clone().map(|script| script.into_inner());
- let mut monitor_signer = signer_provider.derive_channel_signer(context.channel_value_satoshis, context.channel_keys_id);
- monitor_signer.provide_channel_parameters(&context.channel_transaction_parameters);
+ let monitor_signer = signer_provider.derive_channel_signer(context.channel_value_satoshis, context.channel_keys_id);


provide_channel_parameters is now called in ChannelMonitor::new, see reasoning above.

codecov · 2024-12-11T02:36:46Z

Codecov Report

Attention: Patch coverage is 97.61905% with 7 lines in your changes missing coverage. Please review.

Project coverage is 89.73%. Comparing base (1a8bf62) to head (0f0560a).
Report is 68 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/chan_utils.rs	95.45%	2 Missing and 2 partials ⚠️
lightning/src/sign/mod.rs	97.40%	0 Missing and 2 partials ⚠️
lightning/src/chain/channelmonitor.rs	97.22%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3454 +/- ##
==========================================
- Coverage 89.74% 89.73% -0.02% 
==========================================
Files 130 130 Lines 107793 107909 +116 Branches 107793 107909 +116 ==========================================
+ Hits 96743 96836 +93 - Misses 8651 8672 +21 - Partials 2399 2401 +2

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

tnull · 2024-12-11T09:03:04Z

I'll have to review this more closely, but want to note that this will very likely conflict with (the currently stale) #3391, which we're in the process of rewriting. We should coordinate to see how to resolve this, i.e., which approach we should take (first) so you can lean on it for follow-up work.

tankyleo · 2024-12-11T17:37:14Z

I'll have to review this more closely, but want to note that this will very likely conflict with (the currently stale) #3391, which we're in the process of rewriting. We should coordinate to see how to resolve this, i.e., which approach we should take (first) so you can lean on it for follow-up work.

Thank you for taking a look - I've made a first pass on 3391, and currently do not see any conflicts.

This allows the `to_remote` output to easily be changed according to the features of the channel, or the evolution of the LN specification. `to_remote` could even be set to completely arbitrary scripts if compatibility with the formal LN spec is not required. Builders of `CommitmentTransaction` now ask a `ChannelSigner` for the appropriate `to_remote` script to use, and then pass the script to the `CommitmentTransaction` constructor. External signers now provide the expected `to_remote` script to the `verify` call of `CommitmentTransaction`.

Builds of `CommitmentTransaction` now query a `ChannelSigner` for the script pubkey to use in the `to_remote` output of the commitment transaction. So we need to overwrite the `ChannelTransactionParameters` of a `ChannelSigner` anytime we want to build a new commitment transaction with a different set of features. This is feature is only used in tests, so we cfg-gate it behind the test flag.

This allows the `to_local` output to easily be changed according to the features of the channel, or the evolution of the LN specification. `to_local` could even be set to completely arbitrary scripts if compatibility with the formal LN spec is not required. Builders of `CommitmentTransaction` now ask a `ChannelSigner` for the appropriate `to_local` script pubkey to use, and then pass it to the `CommitmentTransaction` constructor. External signers now provide the expected `to_local` script pubkey to the `verify` call of `CommitmentTransaction`.

This allows the htlc tx output to easily be changed according to the features of the channel, or the evolution of the LN specification. The output could even be set to completely arbitrary scripts if compatibility with the formal LN spec is not required. Builders of htlc transactions now ask a `ChannelSigner` for the appropriate revokeable script pubkey to use, and then pass it to the htlc transaction constructors.

All LN-Penalty channel signers need to be able to punish the counterparty in case they broadcast an old state. In this commit, we ask implementers of `ChannelSigner` to produce the full transaction with the given input finalized to punish the corresponding previous output. Consumers of the `ChannelSigner` trait can now be agnostic to the specific scripts used in revokeable outputs. We leave passing to the `ChannelSigner` all the previous `TxOut`'s needed to produce valid schnorr signatures under BIP 341 spending rules to a later patch.

tankyleo · 2025-01-08T19:48:34Z

Close in favor of #3512

tankyleo · 2024-12-10T18:05:29Z

See commit message for a description of this specific commit.

This begins work on allowing the customization of different outputs of the commitment transaction, in preparation for taproot channels and also to allow people to set the outputs to arbitrary scripts if they don't require compatibility with the formal LN spec.

This PR begins with the to_remote output, with the goal of illustrating the approach taken with a simple example.

If this approach is ok, I will next work on StaticPaymentOutputDescriptor, as it would need to be updated to handle the taproot / arbitrary scripts used in this output. I can do this work in this PR, or in a follow-up depending on your preference.

My approach is to ask the channel signers to do more work (but not more than that of most hardware wallets):

Set the appropriate SPK's for the different outputs of the commitment transaction.
Return the full witness to spend such an output (not just the signature); this is not shown here, but is the current plan for the to_local outputs in a justice scenario.

By putting scriptpubkey and witness construction behind the signer trait, we can have some parts of LDK be implemented in terms of scriptpubkeys and witnesses, and these parts can then remain the same across segwit, taproot, and arbitrary scripts.

Let me know what you think, thank you for your input.

cc @arik-so @TheBlueMatt

~~EDIT 2024-12-13~~

~~In cbac0e7 I apply the same approach to the to_local output of the commit tx, and in 72ca0a9 same approach to the htlc tx output.~~

EDIT 2024-12-15

~~In the last two commits, I apply the same approach to the to_local output of the commit tx, and to the htlc tx output.~~

I also apply the same approach to the to_local output of the commit tx, and to the htlc tx output later in the same patchset.

Instead of returning only the witness when punishing a revokeable output, I instead choose to ask the ChannelSigner to return a full transaction with the specified input finalized to punish the corresponding previous output. This is primarily to leave the possibility of a signer to customize how the sequence field of the input is set.

tankyleo · 2024-12-10T18:18:08Z

 best_block: BestBlock, counterparty_node_id: PublicKey, channel_id: ChannelId,
 	) -> ChannelMonitor<Signer> {
+
+ keys.provide_channel_parameters(channel_parameters);


I move the provide_channel_parameters call to here because there were some tests that built a ChannelMonitor without first calling provide_channel_parameters on the keys: Signer.
Further below in initial_commitment_signed, I delete the provide_channel_parameters call, as it is now called here.

We now need to call provide_channel_parameters before ChannelMonitor::new because ChannelSigner::get_counterparty_payment_script assumes that provide_channel_parameters has already been called - see the doc for ChannelSigner::get_counterparty_payment_script.

tankyleo · 2024-12-10T18:20:05Z

 let shutdown_script = context.shutdown_scriptpubkey.clone().map(|script| script.into_inner());
- let mut monitor_signer = signer_provider.derive_channel_signer(context.channel_value_satoshis, context.channel_keys_id);
- monitor_signer.provide_channel_parameters(&context.channel_transaction_parameters);
+ let monitor_signer = signer_provider.derive_channel_signer(context.channel_value_satoshis, context.channel_keys_id);


provide_channel_parameters is now called in ChannelMonitor::new, see reasoning above.

codecov · 2024-12-11T02:36:46Z

Codecov Report

Attention: Patch coverage is 97.61905% with 7 lines in your changes missing coverage. Please review.

Project coverage is 89.73%. Comparing base (1a8bf62) to head (0f0560a).
Report is 68 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/chan_utils.rs	95.45%	2 Missing and 2 partials ⚠️
lightning/src/sign/mod.rs	97.40%	0 Missing and 2 partials ⚠️
lightning/src/chain/channelmonitor.rs	97.22%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3454 +/- ##
==========================================
- Coverage 89.74% 89.73% -0.02% 
==========================================
Files 130 130 Lines 107793 107909 +116 Branches 107793 107909 +116 ==========================================
+ Hits 96743 96836 +93 - Misses 8651 8672 +21 - Partials 2399 2401 +2

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

tnull · 2024-12-11T09:03:04Z

I'll have to review this more closely, but want to note that this will very likely conflict with (the currently stale) #3391, which we're in the process of rewriting. We should coordinate to see how to resolve this, i.e., which approach we should take (first) so you can lean on it for follow-up work.

tankyleo · 2024-12-11T17:37:14Z

I'll have to review this more closely, but want to note that this will very likely conflict with (the currently stale) #3391, which we're in the process of rewriting. We should coordinate to see how to resolve this, i.e., which approach we should take (first) so you can lean on it for follow-up work.

Thank you for taking a look - I've made a first pass on 3391, and currently do not see any conflicts.

This allows the `to_remote` output to easily be changed according to the features of the channel, or the evolution of the LN specification. `to_remote` could even be set to completely arbitrary scripts if compatibility with the formal LN spec is not required. Builders of `CommitmentTransaction` now ask a `ChannelSigner` for the appropriate `to_remote` script to use, and then pass the script to the `CommitmentTransaction` constructor. External signers now provide the expected `to_remote` script to the `verify` call of `CommitmentTransaction`.

Builds of `CommitmentTransaction` now query a `ChannelSigner` for the script pubkey to use in the `to_remote` output of the commitment transaction. So we need to overwrite the `ChannelTransactionParameters` of a `ChannelSigner` anytime we want to build a new commitment transaction with a different set of features. This is feature is only used in tests, so we cfg-gate it behind the test flag.

This allows the `to_local` output to easily be changed according to the features of the channel, or the evolution of the LN specification. `to_local` could even be set to completely arbitrary scripts if compatibility with the formal LN spec is not required. Builders of `CommitmentTransaction` now ask a `ChannelSigner` for the appropriate `to_local` script pubkey to use, and then pass it to the `CommitmentTransaction` constructor. External signers now provide the expected `to_local` script pubkey to the `verify` call of `CommitmentTransaction`.

This allows the htlc tx output to easily be changed according to the features of the channel, or the evolution of the LN specification. The output could even be set to completely arbitrary scripts if compatibility with the formal LN spec is not required. Builders of htlc transactions now ask a `ChannelSigner` for the appropriate revokeable script pubkey to use, and then pass it to the htlc transaction constructors.

All LN-Penalty channel signers need to be able to punish the counterparty in case they broadcast an old state. In this commit, we ask implementers of `ChannelSigner` to produce the full transaction with the given input finalized to punish the corresponding previous output. Consumers of the `ChannelSigner` trait can now be agnostic to the specific scripts used in revokeable outputs. We leave passing to the `ChannelSigner` all the previous `TxOut`'s needed to produce valid schnorr signatures under BIP 341 spending rules to a later patch.

tankyleo · 2025-01-08T19:48:34Z

Close in favor of #3512

tankyleo · 2024-12-10T18:05:29Z

See commit message for a description of this specific commit.

This begins work on allowing the customization of different outputs of the commitment transaction, in preparation for taproot channels and also to allow people to set the outputs to arbitrary scripts if they don't require compatibility with the formal LN spec.

This PR begins with the to_remote output, with the goal of illustrating the approach taken with a simple example.

If this approach is ok, I will next work on StaticPaymentOutputDescriptor, as it would need to be updated to handle the taproot / arbitrary scripts used in this output. I can do this work in this PR, or in a follow-up depending on your preference.

My approach is to ask the channel signers to do more work (but not more than that of most hardware wallets):

Set the appropriate SPK's for the different outputs of the commitment transaction.
Return the full witness to spend such an output (not just the signature); this is not shown here, but is the current plan for the to_local outputs in a justice scenario.

By putting scriptpubkey and witness construction behind the signer trait, we can have some parts of LDK be implemented in terms of scriptpubkeys and witnesses, and these parts can then remain the same across segwit, taproot, and arbitrary scripts.

Let me know what you think, thank you for your input.

cc @arik-so @TheBlueMatt

~~EDIT 2024-12-13~~

~~In cbac0e7 I apply the same approach to the to_local output of the commit tx, and in 72ca0a9 same approach to the htlc tx output.~~

EDIT 2024-12-15

~~In the last two commits, I apply the same approach to the to_local output of the commit tx, and to the htlc tx output.~~

I also apply the same approach to the to_local output of the commit tx, and to the htlc tx output later in the same patchset.

Instead of returning only the witness when punishing a revokeable output, I instead choose to ask the ChannelSigner to return a full transaction with the specified input finalized to punish the corresponding previous output. This is primarily to leave the possibility of a signer to customize how the sequence field of the input is set.

tankyleo · 2024-12-10T18:18:08Z

 best_block: BestBlock, counterparty_node_id: PublicKey, channel_id: ChannelId,
 	) -> ChannelMonitor<Signer> {
+
+ keys.provide_channel_parameters(channel_parameters);


I move the provide_channel_parameters call to here because there were some tests that built a ChannelMonitor without first calling provide_channel_parameters on the keys: Signer.
Further below in initial_commitment_signed, I delete the provide_channel_parameters call, as it is now called here.

We now need to call provide_channel_parameters before ChannelMonitor::new because ChannelSigner::get_counterparty_payment_script assumes that provide_channel_parameters has already been called - see the doc for ChannelSigner::get_counterparty_payment_script.

tankyleo · 2024-12-10T18:20:05Z

 let shutdown_script = context.shutdown_scriptpubkey.clone().map(|script| script.into_inner());
- let mut monitor_signer = signer_provider.derive_channel_signer(context.channel_value_satoshis, context.channel_keys_id);
- monitor_signer.provide_channel_parameters(&context.channel_transaction_parameters);
+ let monitor_signer = signer_provider.derive_channel_signer(context.channel_value_satoshis, context.channel_keys_id);


provide_channel_parameters is now called in ChannelMonitor::new, see reasoning above.

codecov · 2024-12-11T02:36:46Z

Codecov Report

Attention: Patch coverage is 97.61905% with 7 lines in your changes missing coverage. Please review.

Project coverage is 89.73%. Comparing base (1a8bf62) to head (0f0560a).
Report is 68 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/chan_utils.rs	95.45%	2 Missing and 2 partials ⚠️
lightning/src/sign/mod.rs	97.40%	0 Missing and 2 partials ⚠️
lightning/src/chain/channelmonitor.rs	97.22%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3454 +/- ##
==========================================
- Coverage 89.74% 89.73% -0.02% 
==========================================
Files 130 130 Lines 107793 107909 +116 Branches 107793 107909 +116 ==========================================
+ Hits 96743 96836 +93 - Misses 8651 8672 +21 - Partials 2399 2401 +2

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

tnull · 2024-12-11T09:03:04Z

I'll have to review this more closely, but want to note that this will very likely conflict with (the currently stale) #3391, which we're in the process of rewriting. We should coordinate to see how to resolve this, i.e., which approach we should take (first) so you can lean on it for follow-up work.

tankyleo · 2024-12-11T17:37:14Z

I'll have to review this more closely, but want to note that this will very likely conflict with (the currently stale) #3391, which we're in the process of rewriting. We should coordinate to see how to resolve this, i.e., which approach we should take (first) so you can lean on it for follow-up work.

Thank you for taking a look - I've made a first pass on 3391, and currently do not see any conflicts.

This allows the `to_remote` output to easily be changed according to the features of the channel, or the evolution of the LN specification. `to_remote` could even be set to completely arbitrary scripts if compatibility with the formal LN spec is not required. Builders of `CommitmentTransaction` now ask a `ChannelSigner` for the appropriate `to_remote` script to use, and then pass the script to the `CommitmentTransaction` constructor. External signers now provide the expected `to_remote` script to the `verify` call of `CommitmentTransaction`.

Builds of `CommitmentTransaction` now query a `ChannelSigner` for the script pubkey to use in the `to_remote` output of the commitment transaction. So we need to overwrite the `ChannelTransactionParameters` of a `ChannelSigner` anytime we want to build a new commitment transaction with a different set of features. This is feature is only used in tests, so we cfg-gate it behind the test flag.

This allows the `to_local` output to easily be changed according to the features of the channel, or the evolution of the LN specification. `to_local` could even be set to completely arbitrary scripts if compatibility with the formal LN spec is not required. Builders of `CommitmentTransaction` now ask a `ChannelSigner` for the appropriate `to_local` script pubkey to use, and then pass it to the `CommitmentTransaction` constructor. External signers now provide the expected `to_local` script pubkey to the `verify` call of `CommitmentTransaction`.

This allows the htlc tx output to easily be changed according to the features of the channel, or the evolution of the LN specification. The output could even be set to completely arbitrary scripts if compatibility with the formal LN spec is not required. Builders of htlc transactions now ask a `ChannelSigner` for the appropriate revokeable script pubkey to use, and then pass it to the htlc transaction constructors.

All LN-Penalty channel signers need to be able to punish the counterparty in case they broadcast an old state. In this commit, we ask implementers of `ChannelSigner` to produce the full transaction with the given input finalized to punish the corresponding previous output. Consumers of the `ChannelSigner` trait can now be agnostic to the specific scripts used in revokeable outputs. We leave passing to the `ChannelSigner` all the previous `TxOut`'s needed to produce valid schnorr signatures under BIP 341 spending rules to a later patch.

tankyleo · 2025-01-08T19:48:34Z

Close in favor of #3512

tankyleo · 2024-12-10T18:05:29Z

See commit message for a description of this specific commit.

This begins work on allowing the customization of different outputs of the commitment transaction, in preparation for taproot channels and also to allow people to set the outputs to arbitrary scripts if they don't require compatibility with the formal LN spec.

This PR begins with the to_remote output, with the goal of illustrating the approach taken with a simple example.

If this approach is ok, I will next work on StaticPaymentOutputDescriptor, as it would need to be updated to handle the taproot / arbitrary scripts used in this output. I can do this work in this PR, or in a follow-up depending on your preference.

My approach is to ask the channel signers to do more work (but not more than that of most hardware wallets):

Set the appropriate SPK's for the different outputs of the commitment transaction.
Return the full witness to spend such an output (not just the signature); this is not shown here, but is the current plan for the to_local outputs in a justice scenario.

By putting scriptpubkey and witness construction behind the signer trait, we can have some parts of LDK be implemented in terms of scriptpubkeys and witnesses, and these parts can then remain the same across segwit, taproot, and arbitrary scripts.

Let me know what you think, thank you for your input.

cc @arik-so @TheBlueMatt

~~EDIT 2024-12-13~~

~~In cbac0e7 I apply the same approach to the to_local output of the commit tx, and in 72ca0a9 same approach to the htlc tx output.~~

EDIT 2024-12-15

~~In the last two commits, I apply the same approach to the to_local output of the commit tx, and to the htlc tx output.~~

I also apply the same approach to the to_local output of the commit tx, and to the htlc tx output later in the same patchset.

Instead of returning only the witness when punishing a revokeable output, I instead choose to ask the ChannelSigner to return a full transaction with the specified input finalized to punish the corresponding previous output. This is primarily to leave the possibility of a signer to customize how the sequence field of the input is set.

tankyleo · 2024-12-10T18:18:08Z

 best_block: BestBlock, counterparty_node_id: PublicKey, channel_id: ChannelId,
 	) -> ChannelMonitor<Signer> {
+
+ keys.provide_channel_parameters(channel_parameters);


I move the provide_channel_parameters call to here because there were some tests that built a ChannelMonitor without first calling provide_channel_parameters on the keys: Signer.
Further below in initial_commitment_signed, I delete the provide_channel_parameters call, as it is now called here.

We now need to call provide_channel_parameters before ChannelMonitor::new because ChannelSigner::get_counterparty_payment_script assumes that provide_channel_parameters has already been called - see the doc for ChannelSigner::get_counterparty_payment_script.

tankyleo · 2024-12-10T18:20:05Z

 let shutdown_script = context.shutdown_scriptpubkey.clone().map(|script| script.into_inner());
- let mut monitor_signer = signer_provider.derive_channel_signer(context.channel_value_satoshis, context.channel_keys_id);
- monitor_signer.provide_channel_parameters(&context.channel_transaction_parameters);
+ let monitor_signer = signer_provider.derive_channel_signer(context.channel_value_satoshis, context.channel_keys_id);


provide_channel_parameters is now called in ChannelMonitor::new, see reasoning above.

codecov · 2024-12-11T02:36:46Z

Codecov Report

Attention: Patch coverage is 97.61905% with 7 lines in your changes missing coverage. Please review.

Project coverage is 89.73%. Comparing base (1a8bf62) to head (0f0560a).
Report is 68 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/chan_utils.rs	95.45%	2 Missing and 2 partials ⚠️
lightning/src/sign/mod.rs	97.40%	0 Missing and 2 partials ⚠️
lightning/src/chain/channelmonitor.rs	97.22%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3454 +/- ##
==========================================
- Coverage 89.74% 89.73% -0.02% 
==========================================
Files 130 130 Lines 107793 107909 +116 Branches 107793 107909 +116 ==========================================
+ Hits 96743 96836 +93 - Misses 8651 8672 +21 - Partials 2399 2401 +2

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

tnull · 2024-12-11T09:03:04Z

I'll have to review this more closely, but want to note that this will very likely conflict with (the currently stale) #3391, which we're in the process of rewriting. We should coordinate to see how to resolve this, i.e., which approach we should take (first) so you can lean on it for follow-up work.

tankyleo · 2024-12-11T17:37:14Z

I'll have to review this more closely, but want to note that this will very likely conflict with (the currently stale) #3391, which we're in the process of rewriting. We should coordinate to see how to resolve this, i.e., which approach we should take (first) so you can lean on it for follow-up work.

Thank you for taking a look - I've made a first pass on 3391, and currently do not see any conflicts.

This allows the `to_remote` output to easily be changed according to the features of the channel, or the evolution of the LN specification. `to_remote` could even be set to completely arbitrary scripts if compatibility with the formal LN spec is not required. Builders of `CommitmentTransaction` now ask a `ChannelSigner` for the appropriate `to_remote` script to use, and then pass the script to the `CommitmentTransaction` constructor. External signers now provide the expected `to_remote` script to the `verify` call of `CommitmentTransaction`.

Builds of `CommitmentTransaction` now query a `ChannelSigner` for the script pubkey to use in the `to_remote` output of the commitment transaction. So we need to overwrite the `ChannelTransactionParameters` of a `ChannelSigner` anytime we want to build a new commitment transaction with a different set of features. This is feature is only used in tests, so we cfg-gate it behind the test flag.

This allows the `to_local` output to easily be changed according to the features of the channel, or the evolution of the LN specification. `to_local` could even be set to completely arbitrary scripts if compatibility with the formal LN spec is not required. Builders of `CommitmentTransaction` now ask a `ChannelSigner` for the appropriate `to_local` script pubkey to use, and then pass it to the `CommitmentTransaction` constructor. External signers now provide the expected `to_local` script pubkey to the `verify` call of `CommitmentTransaction`.

This allows the htlc tx output to easily be changed according to the features of the channel, or the evolution of the LN specification. The output could even be set to completely arbitrary scripts if compatibility with the formal LN spec is not required. Builders of htlc transactions now ask a `ChannelSigner` for the appropriate revokeable script pubkey to use, and then pass it to the htlc transaction constructors.

All LN-Penalty channel signers need to be able to punish the counterparty in case they broadcast an old state. In this commit, we ask implementers of `ChannelSigner` to produce the full transaction with the given input finalized to punish the corresponding previous output. Consumers of the `ChannelSigner` trait can now be agnostic to the specific scripts used in revokeable outputs. We leave passing to the `ChannelSigner` all the previous `TxOut`'s needed to produce valid schnorr signatures under BIP 341 spending rules to a later patch.

tankyleo · 2025-01-08T19:48:34Z

Close in favor of #3512

tankyleo · 2024-12-10T18:05:29Z

See commit message for a description of this specific commit.

This begins work on allowing the customization of different outputs of the commitment transaction, in preparation for taproot channels and also to allow people to set the outputs to arbitrary scripts if they don't require compatibility with the formal LN spec.

This PR begins with the to_remote output, with the goal of illustrating the approach taken with a simple example.

If this approach is ok, I will next work on StaticPaymentOutputDescriptor, as it would need to be updated to handle the taproot / arbitrary scripts used in this output. I can do this work in this PR, or in a follow-up depending on your preference.

My approach is to ask the channel signers to do more work (but not more than that of most hardware wallets):

Set the appropriate SPK's for the different outputs of the commitment transaction.
Return the full witness to spend such an output (not just the signature); this is not shown here, but is the current plan for the to_local outputs in a justice scenario.

By putting scriptpubkey and witness construction behind the signer trait, we can have some parts of LDK be implemented in terms of scriptpubkeys and witnesses, and these parts can then remain the same across segwit, taproot, and arbitrary scripts.

Let me know what you think, thank you for your input.

cc @arik-so @TheBlueMatt

~~EDIT 2024-12-13~~

~~In cbac0e7 I apply the same approach to the to_local output of the commit tx, and in 72ca0a9 same approach to the htlc tx output.~~

EDIT 2024-12-15

~~In the last two commits, I apply the same approach to the to_local output of the commit tx, and to the htlc tx output.~~

I also apply the same approach to the to_local output of the commit tx, and to the htlc tx output later in the same patchset.

Instead of returning only the witness when punishing a revokeable output, I instead choose to ask the ChannelSigner to return a full transaction with the specified input finalized to punish the corresponding previous output. This is primarily to leave the possibility of a signer to customize how the sequence field of the input is set.

tankyleo · 2024-12-10T18:18:08Z

 best_block: BestBlock, counterparty_node_id: PublicKey, channel_id: ChannelId,
 	) -> ChannelMonitor<Signer> {
+
+ keys.provide_channel_parameters(channel_parameters);


I move the provide_channel_parameters call to here because there were some tests that built a ChannelMonitor without first calling provide_channel_parameters on the keys: Signer.
Further below in initial_commitment_signed, I delete the provide_channel_parameters call, as it is now called here.

We now need to call provide_channel_parameters before ChannelMonitor::new because ChannelSigner::get_counterparty_payment_script assumes that provide_channel_parameters has already been called - see the doc for ChannelSigner::get_counterparty_payment_script.

tankyleo · 2024-12-10T18:20:05Z

 let shutdown_script = context.shutdown_scriptpubkey.clone().map(|script| script.into_inner());
- let mut monitor_signer = signer_provider.derive_channel_signer(context.channel_value_satoshis, context.channel_keys_id);
- monitor_signer.provide_channel_parameters(&context.channel_transaction_parameters);
+ let monitor_signer = signer_provider.derive_channel_signer(context.channel_value_satoshis, context.channel_keys_id);


provide_channel_parameters is now called in ChannelMonitor::new, see reasoning above.

codecov · 2024-12-11T02:36:46Z

Codecov Report

Attention: Patch coverage is 97.61905% with 7 lines in your changes missing coverage. Please review.

Project coverage is 89.73%. Comparing base (1a8bf62) to head (0f0560a).
Report is 68 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/chan_utils.rs	95.45%	2 Missing and 2 partials ⚠️
lightning/src/sign/mod.rs	97.40%	0 Missing and 2 partials ⚠️
lightning/src/chain/channelmonitor.rs	97.22%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3454 +/- ##
==========================================
- Coverage 89.74% 89.73% -0.02% 
==========================================
Files 130 130 Lines 107793 107909 +116 Branches 107793 107909 +116 ==========================================
+ Hits 96743 96836 +93 - Misses 8651 8672 +21 - Partials 2399 2401 +2

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

tnull · 2024-12-11T09:03:04Z

I'll have to review this more closely, but want to note that this will very likely conflict with (the currently stale) #3391, which we're in the process of rewriting. We should coordinate to see how to resolve this, i.e., which approach we should take (first) so you can lean on it for follow-up work.

tankyleo · 2024-12-11T17:37:14Z

I'll have to review this more closely, but want to note that this will very likely conflict with (the currently stale) #3391, which we're in the process of rewriting. We should coordinate to see how to resolve this, i.e., which approach we should take (first) so you can lean on it for follow-up work.

Thank you for taking a look - I've made a first pass on 3391, and currently do not see any conflicts.

This allows the `to_remote` output to easily be changed according to the features of the channel, or the evolution of the LN specification. `to_remote` could even be set to completely arbitrary scripts if compatibility with the formal LN spec is not required. Builders of `CommitmentTransaction` now ask a `ChannelSigner` for the appropriate `to_remote` script to use, and then pass the script to the `CommitmentTransaction` constructor. External signers now provide the expected `to_remote` script to the `verify` call of `CommitmentTransaction`.

Builds of `CommitmentTransaction` now query a `ChannelSigner` for the script pubkey to use in the `to_remote` output of the commitment transaction. So we need to overwrite the `ChannelTransactionParameters` of a `ChannelSigner` anytime we want to build a new commitment transaction with a different set of features. This is feature is only used in tests, so we cfg-gate it behind the test flag.

This allows the `to_local` output to easily be changed according to the features of the channel, or the evolution of the LN specification. `to_local` could even be set to completely arbitrary scripts if compatibility with the formal LN spec is not required. Builders of `CommitmentTransaction` now ask a `ChannelSigner` for the appropriate `to_local` script pubkey to use, and then pass it to the `CommitmentTransaction` constructor. External signers now provide the expected `to_local` script pubkey to the `verify` call of `CommitmentTransaction`.

This allows the htlc tx output to easily be changed according to the features of the channel, or the evolution of the LN specification. The output could even be set to completely arbitrary scripts if compatibility with the formal LN spec is not required. Builders of htlc transactions now ask a `ChannelSigner` for the appropriate revokeable script pubkey to use, and then pass it to the htlc transaction constructors.

All LN-Penalty channel signers need to be able to punish the counterparty in case they broadcast an old state. In this commit, we ask implementers of `ChannelSigner` to produce the full transaction with the given input finalized to punish the corresponding previous output. Consumers of the `ChannelSigner` trait can now be agnostic to the specific scripts used in revokeable outputs. We leave passing to the `ChannelSigner` all the previous `TxOut`'s needed to produce valid schnorr signatures under BIP 341 spending rules to a later patch.

tankyleo · 2025-01-08T19:48:34Z

Close in favor of #3512

tankyleo force-pushed the 2024-12-signer-spks branch from 06f5930 to 4397759Compare December 10, 2024 18:11

tankyleo commented Dec 10, 2024
View reviewed changes

Comment threadlightning/src/ln/channel.rs Outdated

tankyleo force-pushed the 2024-12-signer-spks branch 2 times, most recently from 8dd569b to 9633310Compare December 11, 2024 02:28

tankyleo force-pushed the 2024-12-signer-spks branch 2 times, most recently from ccbcf98 to e12c3e8Compare December 11, 2024 03:22

tankyleo changed the title ~~Let ChannelSigner set to_remote scriptpubkey~~Let ChannelSigner set to_remote, to_local, htlc tx scriptpubkeysDec 14, 2024

tankyleo added 5 commits December 15, 2024 07:55

Use new ChannelSigner API in HolderCommitmentTransaction::dummy
ab43c1a

tankyleo force-pushed the 2024-12-signer-spks branch from b4a8307 to d3181c5Compare December 15, 2024 08:53

tankyleo force-pushed the 2024-12-signer-spks branch from f48bf32 to 0f0560aCompare December 15, 2024 20:48

tankyleo mentioned this pull request Jan 8, 2025
Let ChannelSigner set and spend LN scriptpubkeys #3512
Closed
9 tasks

tankyleo closed this Jan 8, 2025

tankyleo force-pushed the 2024-12-signer-spks branch from 06f5930 to 4397759Compare December 10, 2024 18:11

tankyleo commented Dec 10, 2024
View reviewed changes

Comment threadlightning/src/ln/channel.rs Outdated

tankyleo force-pushed the 2024-12-signer-spks branch 2 times, most recently from 8dd569b to 9633310Compare December 11, 2024 02:28

tankyleo force-pushed the 2024-12-signer-spks branch 2 times, most recently from ccbcf98 to e12c3e8Compare December 11, 2024 03:22

tankyleo changed the title ~~Let ChannelSigner set to_remote scriptpubkey~~Let ChannelSigner set to_remote, to_local, htlc tx scriptpubkeysDec 14, 2024

tankyleo added 5 commits December 15, 2024 07:55

Use new ChannelSigner API in HolderCommitmentTransaction::dummy
ab43c1a

tankyleo force-pushed the 2024-12-signer-spks branch from b4a8307 to d3181c5Compare December 15, 2024 08:53

tankyleo force-pushed the 2024-12-signer-spks branch from f48bf32 to 0f0560aCompare December 15, 2024 20:48

tankyleo mentioned this pull request Jan 8, 2025
Let ChannelSigner set and spend LN scriptpubkeys #3512
Closed
9 tasks

tankyleo closed this Jan 8, 2025

tankyleo force-pushed the 2024-12-signer-spks branch from 06f5930 to 4397759Compare December 10, 2024 18:11

tankyleo commented Dec 10, 2024
View reviewed changes

Comment threadlightning/src/ln/channel.rs Outdated

tankyleo force-pushed the 2024-12-signer-spks branch 2 times, most recently from 8dd569b to 9633310Compare December 11, 2024 02:28

tankyleo force-pushed the 2024-12-signer-spks branch 2 times, most recently from ccbcf98 to e12c3e8Compare December 11, 2024 03:22

tankyleo changed the title ~~Let ChannelSigner set to_remote scriptpubkey~~Let ChannelSigner set to_remote, to_local, htlc tx scriptpubkeysDec 14, 2024

tankyleo added 5 commits December 15, 2024 07:55

Use new ChannelSigner API in HolderCommitmentTransaction::dummy
ab43c1a

tankyleo force-pushed the 2024-12-signer-spks branch from b4a8307 to d3181c5Compare December 15, 2024 08:53

tankyleo force-pushed the 2024-12-signer-spks branch from f48bf32 to 0f0560aCompare December 15, 2024 20:48

tankyleo mentioned this pull request Jan 8, 2025
Let ChannelSigner set and spend LN scriptpubkeys #3512
Closed
9 tasks

tankyleo closed this Jan 8, 2025

tankyleo force-pushed the 2024-12-signer-spks branch from 06f5930 to 4397759Compare December 10, 2024 18:11

tankyleo commented Dec 10, 2024
View reviewed changes

Comment threadlightning/src/ln/channel.rs Outdated

tankyleo force-pushed the 2024-12-signer-spks branch 2 times, most recently from 8dd569b to 9633310Compare December 11, 2024 02:28

tankyleo force-pushed the 2024-12-signer-spks branch 2 times, most recently from ccbcf98 to e12c3e8Compare December 11, 2024 03:22

tankyleo changed the title ~~Let ChannelSigner set to_remote scriptpubkey~~Let ChannelSigner set to_remote, to_local, htlc tx scriptpubkeysDec 14, 2024

tankyleo added 5 commits December 15, 2024 07:55

Use new ChannelSigner API in HolderCommitmentTransaction::dummy
ab43c1a

tankyleo force-pushed the 2024-12-signer-spks branch from b4a8307 to d3181c5Compare December 15, 2024 08:53

tankyleo force-pushed the 2024-12-signer-spks branch from f48bf32 to 0f0560aCompare December 15, 2024 20:48

tankyleo mentioned this pull request Jan 8, 2025
Let ChannelSigner set and spend LN scriptpubkeys #3512
Closed
9 tasks

tankyleo closed this Jan 8, 2025

tankyleo force-pushed the 2024-12-signer-spks branch from 06f5930 to 4397759Compare December 10, 2024 18:11

tankyleo commented Dec 10, 2024
View reviewed changes

Comment threadlightning/src/ln/channel.rs Outdated

tankyleo force-pushed the 2024-12-signer-spks branch 2 times, most recently from 8dd569b to 9633310Compare December 11, 2024 02:28

tankyleo force-pushed the 2024-12-signer-spks branch 2 times, most recently from ccbcf98 to e12c3e8Compare December 11, 2024 03:22

tankyleo changed the title ~~Let ChannelSigner set to_remote scriptpubkey~~Let ChannelSigner set to_remote, to_local, htlc tx scriptpubkeysDec 14, 2024

tankyleo added 5 commits December 15, 2024 07:55

Use new ChannelSigner API in HolderCommitmentTransaction::dummy
ab43c1a

tankyleo force-pushed the 2024-12-signer-spks branch from b4a8307 to d3181c5Compare December 15, 2024 08:53

tankyleo force-pushed the 2024-12-signer-spks branch from f48bf32 to 0f0560aCompare December 15, 2024 20:48

tankyleo mentioned this pull request Jan 8, 2025
Let ChannelSigner set and spend LN scriptpubkeys #3512
Closed
9 tasks

tankyleo closed this Jan 8, 2025

tankyleo force-pushed the 2024-12-signer-spks branch from 06f5930 to 4397759Compare December 10, 2024 18:11

tankyleo commented Dec 10, 2024
View reviewed changes

Comment threadlightning/src/ln/channel.rs Outdated

tankyleo force-pushed the 2024-12-signer-spks branch 2 times, most recently from 8dd569b to 9633310Compare December 11, 2024 02:28

tankyleo force-pushed the 2024-12-signer-spks branch 2 times, most recently from ccbcf98 to e12c3e8Compare December 11, 2024 03:22

tankyleo changed the title ~~Let ChannelSigner set to_remote scriptpubkey~~Let ChannelSigner set to_remote, to_local, htlc tx scriptpubkeysDec 14, 2024

tankyleo added 5 commits December 15, 2024 07:55

Use new ChannelSigner API in HolderCommitmentTransaction::dummy
ab43c1a

tankyleo force-pushed the 2024-12-signer-spks branch from b4a8307 to d3181c5Compare December 15, 2024 08:53

tankyleo force-pushed the 2024-12-signer-spks branch from f48bf32 to 0f0560aCompare December 15, 2024 20:48

tankyleo mentioned this pull request Jan 8, 2025
Let ChannelSigner set and spend LN scriptpubkeys #3512
Closed
9 tasks

tankyleo closed this Jan 8, 2025

tankyleo force-pushed the 2024-12-signer-spks branch from 06f5930 to 4397759Compare December 10, 2024 18:11

tankyleo commented Dec 10, 2024
View reviewed changes

Comment threadlightning/src/ln/channel.rs Outdated

tankyleo force-pushed the 2024-12-signer-spks branch 2 times, most recently from 8dd569b to 9633310Compare December 11, 2024 02:28

tankyleo force-pushed the 2024-12-signer-spks branch 2 times, most recently from ccbcf98 to e12c3e8Compare December 11, 2024 03:22

tankyleo changed the title ~~Let ChannelSigner set to_remote scriptpubkey~~Let ChannelSigner set to_remote, to_local, htlc tx scriptpubkeysDec 14, 2024

tankyleo added 5 commits December 15, 2024 07:55

Use new ChannelSigner API in HolderCommitmentTransaction::dummy
ab43c1a

tankyleo force-pushed the 2024-12-signer-spks branch from b4a8307 to d3181c5Compare December 15, 2024 08:53

tankyleo force-pushed the 2024-12-signer-spks branch from f48bf32 to 0f0560aCompare December 15, 2024 20:48

tankyleo mentioned this pull request Jan 8, 2025
Let ChannelSigner set and spend LN scriptpubkeys #3512
Closed
9 tasks

tankyleo closed this Jan 8, 2025

tankyleo force-pushed the 2024-12-signer-spks branch from 06f5930 to 4397759Compare December 10, 2024 18:11

tankyleo commented Dec 10, 2024
View reviewed changes

Comment threadlightning/src/ln/channel.rs Outdated

tankyleo force-pushed the 2024-12-signer-spks branch 2 times, most recently from 8dd569b to 9633310Compare December 11, 2024 02:28

tankyleo force-pushed the 2024-12-signer-spks branch 2 times, most recently from ccbcf98 to e12c3e8Compare December 11, 2024 03:22

tankyleo changed the title ~~Let ChannelSigner set to_remote scriptpubkey~~Let ChannelSigner set to_remote, to_local, htlc tx scriptpubkeysDec 14, 2024

tankyleo added 5 commits December 15, 2024 07:55

Use new ChannelSigner API in HolderCommitmentTransaction::dummy
ab43c1a

tankyleo force-pushed the 2024-12-signer-spks branch from b4a8307 to d3181c5Compare December 15, 2024 08:53

tankyleo force-pushed the 2024-12-signer-spks branch from f48bf32 to 0f0560aCompare December 15, 2024 20:48

tankyleo mentioned this pull request Jan 8, 2025
Let ChannelSigner set and spend LN scriptpubkeys #3512
Closed
9 tasks

tankyleo closed this Jan 8, 2025

Uh oh!

Conversation

tankyleo commented Dec 10, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecovBot commented Dec 11, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tnull commented Dec 11, 2024

Uh oh!

tankyleo commented Dec 11, 2024

Uh oh!

tankyleo commented Jan 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

tankyleo commented Dec 10, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecovBot commented Dec 11, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tnull commented Dec 11, 2024

Uh oh!

tankyleo commented Dec 11, 2024

Uh oh!

tankyleo commented Jan 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

tankyleo commented Dec 10, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecovBot commented Dec 11, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tnull commented Dec 11, 2024

Uh oh!

tankyleo commented Dec 11, 2024

Uh oh!

tankyleo commented Dec 10, 2024•
edited
Loading

codecovBot commented Dec 11, 2024•
edited
Loading

tankyleo commented Dec 10, 2024•
edited
Loading

codecovBot commented Dec 11, 2024•
edited
Loading

tankyleo commented Dec 10, 2024•
edited
Loading

codecovBot commented Dec 11, 2024•
edited
Loading

tankyleo commented Dec 10, 2024•
edited
Loading

codecovBot commented Dec 11, 2024•
edited
Loading