Bump transaction event handler fixups #2393

wpaulino · 2023-07-05T17:48:15Z

This PR addresses some follow-up comments from #2089 and a few gaps in the API that were found by @tnull when adding anchors support in ldk-node.

codecov-commenter · 2023-07-05T20:00:49Z

Codecov Report

Patch coverage: 63.76% and project coverage change: -0.05 ⚠️

Comparison is base (31a0456) 90.30% compared to head (4c7883c) 90.26%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2393      +/-   ##
==========================================
- Coverage   90.30%   90.26%   -0.05%     
==========================================
  Files         106      106              
  Lines       55175    55188      +13     
  Branches    55175    55188      +13     
==========================================
- Hits        49828    49816      -12     
- Misses       5347     5372      +25

Impacted Files	Coverage Δ
lightning/src/events/bump_transaction.rs	`40.36% <45.65%> (-1.20%)`	⬇️
lightning/src/chain/channelmonitor.rs	`94.63% <100.00%> (-0.10%)`	⬇️
lightning/src/ln/monitor_tests.rs	`98.33% <100.00%> (-0.02%)`	⬇️
lightning/src/sign/mod.rs	`83.78% <100.00%> (-0.60%)`	⬇️
lightning/src/util/enforcing_trait_impls.rs	`87.90% <100.00%> (-0.10%)`	⬇️

... and 10 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

tnull

Thanks! CI break is due to doc links, this diff should fix it:

diff --git a/lightning/src/events/bump_transaction.rs b/lightning/src/events/bump_transaction.rs
index fd2e9b93..efeec55c 100644
--- a/lightning/src/events/bump_transaction.rs
+++ b/lightning/src/events/bump_transaction.rs
@@ -238,8 +238,8 @@ pub enum BumpTransactionEvent {
        /// child anchor transaction. To sign its anchor input, an [`InMemorySigner`] should be
        /// re-derived through [`KeysManager::derive_channel_keys`] with the help of
-       /// [`AnchorDescriptor::channel_keys_id`] and [`AnchorDescriptor::channel_value_satoshis`]. The
-       /// anchor input signature can be computed with [`EcdsaChannelSigner::sign_holder_anchor_input`],
-       /// which can then be provided to [`build_anchor_input_witness`] along with the `funding_pubkey`
-       /// to obtain the full witness required to spend.
+       /// [`AnchorDescriptor::channel_derivation_parameters`]. The anchor input signature can be
+       /// computed with [`EcdsaChannelSigner::sign_holder_anchor_input`], which can then be provided
+       /// to [`build_anchor_input_witness`] along with the `funding_pubkey` to obtain the full
+       /// witness required to spend.
        ///
        /// It is possible to receive more than one instance of this event if a valid child anchor
@@ -614,4 +614,6 @@ impl<W: Deref> CoinSelectionSource for Wallet<W> where W::Target: WalletSource {
 /// [`CoinSelectionSource`] to fee bump transactions via Child-Pays-For-Parent (CPFP) or
 /// Replace-By-Fee (RBF).
+///
+/// [`Event::BumpTransaction`]: crate::events::Event::BumpTransaction
 pub struct BumpTransactionEventHandler<B: Deref, C: Deref, SP: Deref, L: Deref>
 where
@@ -636,4 +638,6 @@ where
 {
        /// Returns a new instance capable of handling [`Event::BumpTransaction`] events.
+       ///
+       /// [`Event::BumpTransaction`]: crate::events::Event::BumpTransaction
        pub fn new(broadcaster: B, utxo_source: C, signer_provider: SP, logger: L) -> Self {
                Self {

lightning/src/events/bump_transaction.rs

tnull

I'm unfortunately still struggling to use BDK's TxBuilder to implement CoinSelectionSource::select_confirmed_utxos.

If I'm not overlooking something BDK's interface doesn't allow me to filter the selected UTXOs to only include confirmed outputs, see Expose way to select only (n-)confirmed UTXOs for TX creation bitcoindevkit/bdk#1029.
I'm trying to add the must_spend: &[Input] given by CoinSelectionSource::select_confirmed_utxos via TxBuilder::add_foreign_utxo, which however takes a psbt::Input that IIUC in turn requires me to at least supply the previous TxOut the input is spending from, possibly also the full Transaction if we want to include the non_witness_utxo, which BDK's signer requires by default. Would be nice to have them included in bump_transaction::Input.

For now, I'll probably have to stick with using WalletSource (cf. lightningdevkit/ldk-node#105)

wpaulino · 2023-07-06T17:44:51Z

I'm trying to add the must_spend: &[Input] given by CoinSelectionSource::select_confirmed_utxos via TxBuilder::add_foreign_utxo, which however takes a psbt::Input that IIUC in turn requires me to at least supply the previous TxOut the input is spending from, possibly also the full Transaction if we want to include the non_witness_utxo, which BDK's signer requires by default. Would be nice to have them included in bump_transaction::Input.

Providing the full transaction will be a bit tricky for HTLCs, better to leave it as a follow-up and not block the release on it. For now, at least the spending TxOut is included.

lightning/src/events/bump_transaction.rs

ariard

I wonder if a smarter move in term of codebase interfaces wouldn’t be to have the AnchorDescriptor here specified as real output descriptors, and therefore from the LDK-side we put the burden on downstream projects like ldk-node for the compatibility.

Specifying the descriptors make it easier to have them compatible with other LDK downstream projects like VLS, or LDK-based server nodes. Descriptors are nice as they start to be integrated with hardware wallets descriptors.

ariard · 2023-07-08T02:49:59Z

lightning/src/events/bump_transaction.rs

 	/// The transaction input's outpoint corresponding to the commitment transaction's anchor
 	/// output.
 	pub outpoint: OutPoint,
 }

+impl AnchorDescriptor {


If we can update the AnchorDescriptor document to say its a “custom LDK” descriptor and not an output descriptors as documented in https://github.com/bitcoin/bitcoin/blob/master/doc/descriptors.md. All the *Descriptor in lightning/src/sign/mod.rs are coming in an inspiration from the Core’s descriptor. When we introduced *Descriptor in rust-lightning, I think I discussed with the Core’s side to have the lightning scripts standardized as such real descriptors. They would have to modify the base descriptor language at that time to include the preimage operation iirc.

TheBlueMatt · 2023-07-08T19:16:40Z

I don't see how we could build Bitcoin Core-style descriptors out of lightning anchor outputs given the requirements/format, I'm not sure how that's relevant.

TheBlueMatt

All of these commit messages need to be longer and explain why we're exposing things.

TheBlueMatt · 2023-07-08T19:39:24Z

lightning/src/events/bump_transaction.rs

+	/// [`ChannelSigner::provide_channel_parameters`].
+	///
+	/// [`ChannelSigner::provide_channel_parameters`]: crate::sign::ChannelSigner::provide_channel_parameters
+	pub transaction_parameters: ChannelTransactionParameters,


Why do these need to be exposed? Signers doing enforcement should have this cached, if not they shouldn't need it.

Hm, that's a fair point, but from the current documentation users may expect the parameters to be provided shortly after derivation instead of storing them.

Hmmm, then I'd rather update the docs (on the anchor handling structs and the trait) and mention this rather than storing the channel parameters in all our events and shuffling them around just to copy them into the signer and then ignore them...

We still need them to obtain the per-commitment keys and re-derive the descriptor's witness script. Unless you'd rather just provide each party's ChannelPublicKeys directly, which ChannelTransactionParameters essentially is a small wrapper over, so it doesn't seem like a big deal to keep things as is.

Ah, grrrrrrrrrrrrrrrr, man that's a lot of crap to schlep around for one pubkey. I guess I'm fine with it the way it is, just annoyed.

ariard · 2023-07-09T19:57:10Z

I don't see how we could build Bitcoin Core-style descriptors out of lightning anchor outputs given the requirements/format, I'm not sure how that's relevant.

That's the thing, I had the discussion with sipa back in the days and there would be a need to upgrade the descriptor format/requirements. The idea is good given integration with hardware wallets and "cold" signers, though given if it does happen it will take a bunch of time, let's not take it as a bottleneck here. Already have a lot of more priority things on my todo on the LN-side (and I think same on folks on this PR thread), so I'll see if I can find a summer-of-bitcoin enthusiast to champion the idea.

TheBlueMatt · 2023-07-11T20:12:26Z

Needs rebase.

There's no reason to accept the general `Event` enum.

These are meant to be provided by the user, so they need to be exposed in the API.

lightning/src/events/bump_transaction.rs

TheBlueMatt · 2023-07-11T23:46:32Z

lightning/src/events/bump_transaction.rs

+	/// Returns the UTXO to be spent by the anchor input, which can be obtained via
+	/// [`Self::unsigned_tx_input`].
+	pub fn previous_utxo(&self) -> TxOut {
+		TxOut {


Would very much like to see some test coverage of this so we can check that this and the HTLC variant's scripts are right.

See #2403. Each transaction produced by the event handler should have a check_spends assertion to verify script execution.

This allows us to obtain the HTLC input and output from its descriptor without needing to derive the `per_commitment_point` through the signer.

Users may expect these to be provided manually after derivation in the event they need to perform any enforcement prior to signing.

This provides a similar interface as `HTLCDescriptor` for users which choose to implement their own bump transaction event handler.

This may be required by some wallets that rely on PSBTs internally to create/sign transactions.

wpaulino requested a review from tnull July 5, 2023 17:48

wpaulino added the Anchor Support label Jul 5, 2023

wpaulino added this to the 0.0.116 milestone Jul 5, 2023

tnull reviewed Jul 6, 2023

View reviewed changes

lightning/src/events/bump_transaction.rs Outdated Show resolved Hide resolved

tnull reviewed Jul 6, 2023

View reviewed changes

TheBlueMatt assigned tnull and TheBlueMatt Jul 6, 2023

wpaulino force-pushed the bump-transaction-event-handler-fixups branch from 04e916f to e413165 Compare July 6, 2023 17:41

tnull reviewed Jul 7, 2023

View reviewed changes

lightning/src/events/bump_transaction.rs Outdated Show resolved Hide resolved

ariard reviewed Jul 8, 2023

View reviewed changes

TheBlueMatt reviewed Jul 8, 2023

View reviewed changes

wpaulino force-pushed the bump-transaction-event-handler-fixups branch from e413165 to f17c05a Compare July 10, 2023 17:12

wpaulino added 2 commits July 11, 2023 13:34

Accept BumpTransactionEvent in handle_event

a100ed0

There's no reason to accept the general `Event` enum.

Expose CoinSelection struct members

ae701a0

These are meant to be provided by the user, so they need to be exposed in the API.

wpaulino force-pushed the bump-transaction-event-handler-fixups branch from f17c05a to ceacdbf Compare July 11, 2023 20:34

TheBlueMatt previously approved these changes Jul 11, 2023

View reviewed changes

wpaulino added 4 commits July 11, 2023 16:53

Cache HTLC per_commitment_point in descriptor

72c42ee

This allows us to obtain the HTLC input and output from its descriptor without needing to derive the `per_commitment_point` through the signer.

Provide missing post-derivation signer parameters

690ad18

Users may expect these to be provided manually after derivation in the event they need to perform any enforcement prior to signing.

Add transaction-related helpers to AnchorDescriptor

0dbfe24

This provides a similar interface as `HTLCDescriptor` for users which choose to implement their own bump transaction event handler.

Expose previous UTXO for anchor and HTLC inputs

4c7883c

This may be required by some wallets that rely on PSBTs internally to create/sign transactions.

wpaulino dismissed TheBlueMatt’s stale review via 4c7883c July 12, 2023 00:02

wpaulino force-pushed the bump-transaction-event-handler-fixups branch from ceacdbf to 4c7883c Compare July 12, 2023 00:02

wpaulino mentioned this pull request Jul 12, 2023

Integrate BumpTransactionEventHandler into existing anchor tests #2403

Merged

TheBlueMatt approved these changes Jul 12, 2023

View reviewed changes

tnull approved these changes Jul 12, 2023

View reviewed changes

tnull merged commit 07606c1 into lightningdevkit:main Jul 12, 2023
11 of 14 checks passed

wpaulino deleted the bump-transaction-event-handler-fixups branch July 12, 2023 19:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bump transaction event handler fixups #2393

Bump transaction event handler fixups #2393

wpaulino commented Jul 5, 2023

codecov-commenter commented Jul 5, 2023 •

edited

Loading

tnull left a comment

tnull left a comment •

edited

Loading

wpaulino commented Jul 6, 2023

ariard left a comment

ariard Jul 8, 2023

TheBlueMatt commented Jul 8, 2023

TheBlueMatt left a comment

TheBlueMatt Jul 8, 2023

wpaulino Jul 10, 2023

TheBlueMatt Jul 10, 2023

wpaulino Jul 11, 2023

TheBlueMatt Jul 11, 2023

ariard commented Jul 9, 2023

TheBlueMatt commented Jul 11, 2023

TheBlueMatt Jul 11, 2023

wpaulino Jul 12, 2023

Bump transaction event handler fixups #2393

Bump transaction event handler fixups #2393

Conversation

wpaulino commented Jul 5, 2023

codecov-commenter commented Jul 5, 2023 • edited Loading

Codecov Report

tnull left a comment

Choose a reason for hiding this comment

tnull left a comment • edited Loading

Choose a reason for hiding this comment

wpaulino commented Jul 6, 2023

ariard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Jul 8, 2023

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ariard commented Jul 9, 2023

TheBlueMatt commented Jul 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jul 5, 2023 •

edited

Loading

tnull left a comment •

edited

Loading