Let ChannelSigner set and spend LN scriptpubkeys

[tankyleo]

This PR allows 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.

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.
• Let the various transaction builders know of the expected weight of the witness to spend such an output - the transaction builders can then set appropriate feerates when building the transactions.
• At spend time, return a finalized witness for the specified input - previously the EcdsaChannelSigner would return just the signature.

This PR:

• Assumes that if the commit tx htlc output is customized to something outside the LN spec, that channel supports zero fee htlc tx.
• Assumes that if an output of the commit tx is customized, the output remains the same type (P2WPKH, P2WSH)
• The helper functions SpendableOutputDescriptor::to_psbt_input and SpendableOutputDescriptor::create_spendable_outputs_psbt assume scripts from the LN specification are used.

I mark it as draft. I understand this is a huge PR. I hope to show the full picture of how the current approach works to get approach ACKs, then happy to break it down into smaller PRs.

Remaining TODOs:

• Update SpendableOutputDescriptor to accomodate taproot / arbitrary scripts.
• Finalize EcdsaChannelSigner vs ChannelSigner APIs.
• Documentation, and more detailed commit msgs.
• Customize the funding output.
• Consider asking the ChannelSigner to return just the witness that finalizes the given input, instead of returning the full transaction with the given input finalized.
• Figure out a mechanism to guide people who want to implement a ChannelSigner inline with the LN spec. One possible route: have the ChannelSigner trait offer "ProvidedMethods" for implementers who want to stay in-line with the LN specification - people who want to depart from the spec would then override these methods.
• Work out how the validation in FundedChannel will work given a customized commitment transaction.
• [taproot]: Expand witness calls to include all the bitcoin::sighash::Prevouts of the transaction that the input is spending.
• [anchors, taproot]: Expand commit tx fee calculation to account for the possibility of P2TR outputs on the commit tx - these change the weight from current default of P2WSH outputs. Also need to account for the possibility of the to_remote output switching back from P2WSH to P2WPKH in a TRUC world where we drop the 1 block CSV locktime on that output.
• Expose to the custom tx interface the locktime and the sequence fields of the transactions.
• Signer now builds the commit tx - can we delete CommitmentTransaction vs TrustedCommitmentTransaction ?

Supersedes #3454

[codecov]

Codecov Report

Attention: Patch coverage is 93.55783% with 44 lines in your changes missing coverage. Please review.

Project coverage is 88.81%. Comparing base (ad462bd) to head (8a20905).
Report is 11 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/util/test_channel_signer.rs	54.16%	2 Missing and 31 partials ⚠️
lightning/src/ln/chan_utils.rs	95.06%	2 Missing and 2 partials ⚠️
lightning/src/sign/mod.rs	99.08%	2 Missing and 1 partial ⚠️
lightning/src/chain/channelmonitor.rs	96.00%	1 Missing and 1 partial ⚠️
lightning/src/chain/package.rs	98.43%	1 Missing ⚠️
lightning/src/events/bump_transaction.rs	95.65%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3512      +/-   ##
==========================================
+ Coverage   88.42%   88.81%   +0.39%     
==========================================
  Files         149      149              
  Lines      112959   115657    +2698     
  Branches   112959   115657    +2698     
==========================================
+ Hits        99883   102723    +2840     
+ Misses      10605    10497     -108     
+ Partials     2471     2437      -34     

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

[TheBlueMatt]

Just returning the scripts for each position in the commitment transaction is cool, but I don't think it would allow someone to have, for example, a dual-normal-DLC channel. Rather, it would be nice if all of build_commitment_transaction were the interface, tell the "signer" what the HTLCs are and let them build the whole transaction.

It may require a bit more work as we'll need to include additional metadata in the transaction object returned (which output is to_remote/to_local/each HTLC), but the ultimate result would be fewer API calls across the trait, a simpler API and more flexibility.

[tankyleo]

thanks for the review Matt - overall I agree, I'm researching this direction now.

[tankyleo]

@TheBlueMatt regarding this additional metadata:

• right now we already have additional metadata returned, the Vec<(HTLCOutputInCommitment, Option<Box<HTLCSource>>)> for each HTLC included in the commit tx built with build_commitment_transaction.
• this additional metadata travels to the corresponding ChannelMonitor via ChannelMonitorUpdates and gets stored in counterparty_claimable_outpoints: HashMap<Txid, Vec<(HTLCOutputInCommitment, Option<Box<HTLCSource>>)>>
• When a counterparty commit tx falls onchain, we read that hashmap, and include that metadata in the PackageTemplates built, and these PackageTemplates are given to the OnchainTxHandler.

On the other hand for the to_local output of commitment transaction

• In this PR, the ChannelMonitor does a roundtrip to the signer to ask it for the expected to_local script pubkey in that commitment transaction, then iterates over the outputs of the broadcasted transaction to identify the to_local output. The output is then passed to either a RevokedOutput PackageTemplate or DelayedPaymentOutputDescriptor depending on the case.

Question:

• do we keep a single build_commitment_transaction call in channel, and then pass the data over to the ChannelMonitor via the updates, like is done now for HTLC outputs? In that case it seems we'd need to extend the updates and the state of the ChannelMonitor to include to_local outputs.
• or do we call build_commitment_transaction again in ChannelMonitor to get the data needed to identify the to_local output.

[tankyleo]

I have a rough draft that implements the first path - send to_local output metadata via channel updates.

I am not thrilled about storing all the counterparty to_local outputs in the ChannelMonitor state. EDIT: we could store a mapping of the txid to the index of the to_local output in that commitment transaction.

But asking the signer to build the entire commitment transaction again seems repetitive (ie didn't I already give it to you?). Naively, we'd grab the HTLC metadata on the call in channel, and the to_local metadata in the second call in ChannelMonitor. We'd have to find a way to consolidate metadata collection for the ChannelMonitor in a single spot.

[tankyleo]

but the ultimate result would be fewer API calls across the trait, a simpler API and more flexibility.

• I was hoping that with the Provided Methods we don't actually increase the number of methods most users would have to implement - just get_channel_parameters and get_channel_value_satoshis. Then people can override only the methods they require.
• If we require additional outputs, with this current approach I was thinking of introducing one more call that returns a collection of these additional TxOuts. It would be a Provided Method that returns None / empty collection by default.
• I still have to figure out how to offer the ability to set the locktime, and sequence fields of various transactions - definitely asking "signers" to build entire transactions would help in that regard.

[tankyleo]

@TheBlueMatt the very last commit here 22c9a5c implements a build_commitment_transaction call, let me know what you think.

a provided method - people who want custom stuff override, and people who want to stick to LN spec leave that alone.