Let ChannelSigner set to_remote, to_local, htlc tx scriptpubkeys
#3454
Conversation
tankyleo
force-pushed
the
2024-12-signer-spks
branch
from
06f5930 to
4397759
Compare
| on_counterparty_tx_csv: u16, destination_script: &Script, funding_info: (OutPoint, ScriptBuf), | ||
| channel_parameters: &ChannelTransactionParameters, holder_pays_commitment_tx_fee: bool, | ||
| funding_redeemscript: ScriptBuf, channel_value_satoshis: u64, | ||
| commitment_transaction_number_obscure_factor: u64, | ||
| initial_holder_commitment_tx: HolderCommitmentTransaction, | ||
| best_block: BestBlock, counterparty_node_id: PublicKey, channel_id: ChannelId, | ||
| ) -> ChannelMonitor<Signer> { | ||
|
|
||
| keys.provide_channel_parameters(channel_parameters); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| @@ -1609,8 +1609,7 @@ trait InitialRemoteCommitmentReceiver<SP: Deref> where SP::Target: SignerProvide | |||
| let funding_txo_script = funding_redeemscript.to_p2wsh(); | |||
| let obscure_factor = get_commitment_transaction_number_obscure_factor(&context.get_holder_pubkeys().payment_point, &context.get_counterparty_pubkeys().payment_point, context.is_outbound()); | |||
| 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); | |||
There was a problem hiding this comment.
provide_channel_parameters is now called in ChannelMonitor::new, see reasoning above.
tankyleo
force-pushed
the
2024-12-signer-spks
branch
2 times, most recently
from
8dd569b to
9633310
Compare
Codecov ReportAttention: Patch coverage is
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. |
tankyleo
force-pushed
the
2024-12-signer-spks
branch
2 times, most recently
from
ccbcf98 to
e12c3e8
Compare
|
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. |
tankyleo
changed the title
ChannelSigner set to_remote scriptpubkeyChannelSigner set to_remote, to_local, htlc tx scriptpubkeys
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.
tankyleo
force-pushed
the
2024-12-signer-spks
branch
from
b4a8307 to
d3181c5
Compare
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
force-pushed
the
2024-12-signer-spks
branch
from
f48bf32 to
0f0560a
Compare
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_remoteoutput, 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):
to_localoutputs 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-13In cbac0e7 I apply the same approach to theto_localoutput 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 theto_localoutput of the commit tx, and to the htlc tx output.I also apply the same approach to the
to_localoutput 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
ChannelSignerto 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.