Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Custom Transactions] Define the TxBuilder trait #3606

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

tankyleo
Copy link
Contributor

@tankyleo tankyleo commented Feb 18, 2025

    [Custom Transactions] Define the `TxBuilder` trait
    
    This commit defines the `TxBuilder` trait to give users the ability to
    customize the build of the lightning commitment transactions.
    
    The `TxBuilder` trait has a single method,
    `build_commitment_transaction`, which builds the commitment transaction,
    and populates the output indices of the HTLCs passed in. Note that the
    method itself does not impose any sorting.

@arik-so
Copy link
Contributor

arik-so commented Feb 19, 2025

Do you also wanna add the cfg-gate to the CI testing configs?

@tankyleo
Copy link
Contributor Author

Do you also wanna add the cfg-gate to the CI testing configs?

Thank you done

Copy link

codecov bot commented Feb 19, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.68%. Comparing base (b05402a) to head (beec57f).
Report is 21 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3606      +/-   ##
==========================================
+ Coverage   88.67%   89.68%   +1.01%     
==========================================
  Files         149      149              
  Lines      116643   124700    +8057     
  Branches   116643   124700    +8057     
==========================================
+ Hits       103432   111843    +8411     
+ Misses      10710    10339     -371     
- Partials     2501     2518      +17     

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

Comment on lines 39 to 42
fn build_commitment_transaction(
&self, is_holder_tx: bool, commitment_number: u64, per_commitment_point: &PublicKey,
to_broadcaster_value_sat: Amount, to_countersignatory_value_sat: Amount,
htlcs: Vec<&mut HTLCOutputInCommitment>, secp_ctx: &Secp256k1<secp256k1::All>,
Copy link
Contributor Author

@tankyleo tankyleo Feb 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

htlcs currently consists only of the included_non_dust_htlcs.

I am currently reading up on t-bast zero fee commitments PR - this would need to be modified so that the txbuilder has enough information to determine the amount of the shared anchor output.

I am researching whether we could tell txbuilder the current feerate, and the broadcaster dust limit, and let it determine whether to include an output based on the dust limit.

There is also the possibility of adding a to_shared_anchor: Amount field, but that doesn't seem to be a solution that generalizes well.

--
After thinking further, I think it's fair to ask any txbuilder to not include any outputs that are below the dust limit - so we just add one more field here that tells the txbuilder of the sum of the trimmed outputs.

@tankyleo tankyleo changed the title [Custom Transactions] Define ChannelParameters, TxBuilder traits [Custom Transactions] Define the TxBuilder trait Feb 20, 2025
@tankyleo
Copy link
Contributor Author

tankyleo commented Feb 20, 2025

Rebase:

  • Remove the ChannelParameters trait - focus on the basics for now, we can add the bells and whistles later, as needed.
  • Add the trimmed_value_sat parameter to build_commitment_transaction to cover the case where the sum of the trimmed outputs gets added to an ephemeral anchor output.
  • Add ChannelSigner::derive_tx_builder method. For now we specify the exact type to return, can be made generic later.

to_broadcaster_value_sat: Amount, to_countersignatory_value_sat: Amount,
trimmed_value_sat: Amount, htlcs: Vec<&mut HTLCOutputInCommitment>,
secp_ctx: &Secp256k1<secp256k1::All>,
) -> Transaction;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to go ahead and return something like CommitmentStats from channel.rs (at a minimum we should probably return CommitmentTransaction rather than Transaction.

fn build_commitment_transaction(
&self, is_holder_tx: bool, commitment_number: u64, per_commitment_point: &PublicKey,
to_broadcaster_value_sat: Amount, to_countersignatory_value_sat: Amount,
trimmed_value_sat: Amount, htlcs: Vec<&mut HTLCOutputInCommitment>,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, IMO we should be returning the trimmed value based on the HTLCs we have (and the dust amount as configured), rather than being told it.

/// This method will be called only after all the channel parameters have been provided via `provide_channel_parameters`.
fn build_commitment_transaction(
&self, is_holder_tx: bool, commitment_number: u64, per_commitment_point: &PublicKey,
to_broadcaster_value_sat: Amount, to_countersignatory_value_sat: Amount,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to_broadcaster_value_sat and to_countersignatory_value_sat are two sides of the same coin, no? If we know the channel value and the to_self amount and the HTLCs we should be able to calculate the to_remote amount.

@tankyleo
Copy link
Contributor Author

Notes:

  • First commit defines the API, then second commit demonstrates a full integration of this API into the codebase. This is so that we can have more context for the API discussion, happy to drop it. It does not change the public API, but does require changes to persistence.
  • ChannelContext::build_commitment_transaction sorts the htlcs based on their state,
  • then TxBuilder::build_commitment_transaction sorts the htlcs based on the dust limit.
  • Now we do not tell TxBuilder the trimmed value - TxBuilder decides this - but does it need to let us know the trimmed value? I don't see an immediate need.
  • We now return CommitmentStats, but without the preimages.

@tankyleo tankyleo requested a review from TheBlueMatt February 24, 2025 06:00
&self, local: bool, commitment_number: u64, per_commitment_point: &PublicKey,
channel_parameters: &ChannelTransactionParameters, secp_ctx: &Secp256k1<secp256k1::All>,
channel_value_satoshis: u64, value_to_self_with_offset_msat: u64,
htlcs_included: Vec<(HTLCOutputInCommitment, Option<&'a HTLCSource>)>, feerate_per_kw: u32,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems weird to pass an HTLCOutputInCommitment to the thing that's building the commitment...how is the HTLC "in commitment" if the commitment doesn't exist yet :p

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I read it as - "channel built the commitment , and has determined these htlcs should be in it (based on their state)."

trimmed htlcs are still in the commitment, they just hide a little better.

The other option would be to have InboundHTLCOutput and OutboundHTLCOutput in this signature here, which doesn't appeal to me very much.

let htlc_outputs = htlc_outputs.iter().map(|(htlc, source)| (htlc.clone(), source.as_ref().map(|s| s.as_ref()))).collect();

use crate::sign::tx_builder::{SpecTxBuilder, TxBuilder};
let stats = TxBuilder::build_commitment_transaction(&SpecTxBuilder {}, false, commitment_number, &their_per_commitment_point,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, maybe rather than passing the data required to call this we just store the commitment transaction itself here? This is just used for watchtowers to fetch the transaction and HTLC list so we don't really need to be rebuilding here, we can just give them it directly. Its a bit annoying in that it'll bloat ChannelMonitorUpdates for something that many users don't want, but the size of LatestCounterpartyCommitmentTXInfo already scales with HTLC count and LatestHolderCommitmentTXInfo already contains the TX so I'm not sure that it changes the maximum size.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should now be addressed in the very first commit of this PR.

use crate::prelude::*;

/// A trait for types that can build commitment transactions, both for the holder, and the counterparty.
pub(crate) trait TxBuilder {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this will eventually be made public to external users, so we'll have to figure out what to do with the HTLC sources below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you I've changed the type to impl Iterator<Item = &'a mut HTLCOutputInCommitment> based on your suggestion offline.

Prior to this commit, `LatestCounterpartyCommitmentTXInfo` provided
`ChannelMonitor` with the data it needed to build counterparty
commitment transactions, instead of the fully built transaction.

This was an unnecessary optimization, as the data still included all
the htlcs. This meant that the size of
`LatestCounterpartyCommitmentTXInfo` scaled with the number of
htlcs, just like a fully built transaction.

This commit switches to providing the fully built transaction. We take
care to make this change both forward and backward compatible, so we
still provide the former fields of `LatestCounterpartyCommitmentTXInfo`.
@jkczyz jkczyz added the weekly goal Someone wants to land this this week label Feb 27, 2025
Building `CommitmentTransaction` previously required a pointer to a
`Vec<(HTLCOutputInCommitment, T)>`, where each item in the vector
represented a non-dust htlc. This forced the caller to place all the
non-dust htlcs and their auxiliary data in contiguous memory prior to
building a `CommitmentTransaction`.

This requirement was not necessary, so we remove it in this commit.

Instead, we choose to ask for an iterator that yields exclusive
references to the non-dust HTLCs, so that `CommitmentTranscation` may
populate their output indices during the build process.

We decided against asking explicitly for a
`Vec<&mut HTLCOutputInCommitment>` to avoid requiring an extra memory
allocation.
The `DirectedChannelTransactionParameters` argument of the
`CommitmentTransaction` constructor already contains the broadcaster,
and the countersignatory public keys, which include the respective
funding keys.

It is therefore not necessary to ask for these funding keys in
additional arguments.
Instead of asking callers to generate the `TxCreationKeys` on every
new commitment before constructing a `CommitmentTransaction`, let
`CommitmentTransaction` derive the `TxCreationKeys` it will use to
build the raw commitment transaction that it will cache.

This allows a tighter coupling between the per-commitment keys, and the
corresponding commitment transaction.

As new states are generated, callers now only have to derive new
commitment points; `CommitmentTransaction` takes care of deriving the
per-commitment keys.

This also serves to limit the objects in LDK that derive per-commitment
keys according to the LN Specification in preparation for enabling
custom derivations in the future.
@tankyleo
Copy link
Contributor Author

Note that this PR now breaks the public API of CommitmentTransaction.

Channel objects should not impose a particular per-commitment derivation
scheme on the builders of commitment transactions. This will make it
easier to enable custom derivations in the future.

To get us started, we remove most instances of `TxCreationKeys` in
channel.

The last two instances are
1) checking a counterparty's htlc signatures
2) logging the keys we used to create htlc signatures for a counterparty

Both of these cases will be abstracted away from channel in the future.
Instead of converting operands to `i64` and checking if the subtractions
overflowed by checking if the `i64` is smaller than zero, we instead
choose to do checked and saturating subtractions on the original
unsigned integers.
There is no need for an if statement if it will always be true.
This reorganizes the logic in
`ChannelContext::build_commitment_transaction` to clearly separate the
sorting of HTLCs for a commitment transaction based on their state from
the trimming of HTLCs based on the broadcaster's dust limit.

In the future, transaction builders will decide how to handle HTLCs
below the dust limit, but they will not decide which HTLCs to include
based on their state.
The fields `feerate_per_kw` and `num_nondust_htlcs` of `CommitmentStats`
can both be accessed directly from the `tx: CommitmentTransaction`
field.
We choose to include in `CommitmentStats` only fields that will be
calculated or constructed solely by custom transaction builders.

The other fields are created by channel, and placed in a new struct
we call `CommitmentData`.
This commit defines the `TxBuilder` trait to give users the ability to
customize the build of the lightning commitment transactions.

The `TxBuilder` trait has a single method,
`build_commitment_transaction`, which builds the commitment transaction,
and populates the output indices of the HTLCs passed in.

Besides that, it is mostly a copy paste of chunks of code from
`ChannelContext::build_commitment_transaction`.
@@ -548,6 +548,7 @@ pub(crate) enum ChannelMonitorUpdateStep {
feerate_per_kw: Option<u32>,
to_broadcaster_value_sat: Option<u64>,
to_countersignatory_value_sat: Option<u64>,
commitment_tx: Option<CommitmentTransaction>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this also provides all of the fields above, would it make sense to have it be part of a new variant? And the current one can become legacy?

&mut self, txid: Txid, htlc_outputs: Vec<(HTLCOutputInCommitment, Option<Box<HTLCSource>>)>,
commitment_number: u64, their_per_commitment_point: PublicKey, feerate_per_kw: u32,
to_broadcaster_value: u64, to_countersignatory_value: u64, logger: &WithChannelMonitor<L>,
&mut self, htlc_outputs: Vec<(HTLCOutputInCommitment, Option<Box<HTLCSource>>)>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be able to get rid of the htlc_outputs parameter, right? It should always be empty for the first commitment transaction.

@@ -8469,6 +8462,8 @@ impl<SP: Deref> FundedChannel<SP> where
feerate_per_kw: Some(counterparty_commitment_tx.feerate_per_kw()),
to_broadcaster_value_sat: Some(counterparty_commitment_tx.to_broadcaster_value_sat()),
to_countersignatory_value_sat: Some(counterparty_commitment_tx.to_countersignatory_value_sat()),
// This is everything we need, but we still provide all the above fields for downgrades
commitment_tx: Some(counterparty_commitment_tx),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is doubling the size of our updates now. As long as we have code to handle when this is set, I think we don't have to start setting it until we stop setting all of the other fields above.

let commitment_tx = self.build_counterparty_commitment_tx(INITIAL_COMMITMENT_NUMBER,
&their_per_commitment_point, to_broadcaster_value, to_countersignatory_value,
feerate_per_kw, htlc_outputs);
Some(commitment_tx)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can set self.initial_counterparty_commitment_tx to this value now, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not without changing the function to take a &mut self instead of a &self as far as I see.

@@ -1483,8 +1480,8 @@ impl CommitmentTransaction {
fn internal_rebuild_transaction(&self, keys: &TxCreationKeys, channel_parameters: &DirectedChannelTransactionParameters, broadcaster_funding_key: &PublicKey, countersignatory_funding_key: &PublicKey) -> Result<BuiltCommitmentTransaction, ()> {
let (obscured_commitment_transaction_number, txins) = Self::internal_build_inputs(self.commitment_number, channel_parameters);

let mut htlcs_with_aux = self.htlcs.iter().map(|h| (h.clone(), ())).collect();
let (outputs, _) = Self::internal_build_outputs(keys, self.to_broadcaster_value_sat, self.to_countersignatory_value_sat, &mut htlcs_with_aux, channel_parameters, broadcaster_funding_key, countersignatory_funding_key)?;
let mut nondust_htlcs = self.htlcs.clone();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since CommitmentTransaction::new will already assign the output index for each HTLC, ideally we don't need to clone here and can just pass an immutable reference.

}).collect::<Vec<_>>();
let mut nondust_htlcs = htlc_outputs.iter().filter_map(|(htlc, _)| {
htlc.transaction_output_index.map(|_| htlc)
}).cloned().collect::<Vec<_>>();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the HTLCs already have their output index set, it'd be nice to avoid the extra allocation here as well.

@@ -5524,19 +5493,20 @@ impl<SP: Deref> FundedChannel<SP> where

let mut nondust_htlc_sources = Vec::with_capacity(htlcs_cloned.len());
let mut htlcs_and_sigs = Vec::with_capacity(htlcs_cloned.len());
let holder_keys = TxCreationKeys::from_channel_static_keys(&self.holder_commitment_point.current_point(), self.context.get_holder_pubkeys(), self.context.get_counterparty_pubkeys(), &self.context.secp_ctx);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be able to get these from the built commitment transaction above?

let keys = self.context.build_holder_transaction_keys(self.holder_commitment_point.current_point());

let commitment_stats = self.context.build_commitment_transaction(&self.funding, self.holder_commitment_point.transaction_number(), &keys, true, false, logger);
let commitment_stats = self.context.build_commitment_transaction(&self.funding, self.holder_commitment_point.transaction_number(), &self.holder_commitment_point.current_point(), true, false, logger);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're already touching some of these long lines, let's try to break them up as we go to reduce the changes when we rustfmt this file.

@@ -3552,6 +3526,47 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
}
}

// Trim dust htlcs
for (htlc, _) in htlcs_in_tx.iter_mut() {
if htlc.offered {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be able to clean this up further to not have the branching

@@ -875,15 +875,20 @@ struct HTLCStats {
on_holder_tx_outbound_holding_cell_htlcs_count: u32, // dust HTLCs *non*-included
}

/// An enum gathering data on a commitment, either local or remote.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: not an enum

@@ -3504,13 +3504,9 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {
} else {
log_trace!(logger, " ...not including inbound HTLC {} (hash {}) with value {} due to state ({})", htlc.htlc_id, &htlc.payment_hash, htlc.amount_msat, state_name);
match &htlc.state {
&InboundHTLCState::LocalRemoved(ref reason) => {
if generated_by_local {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this always true?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

include == !generated_by_local
include == 0
generated_by_local == 1 ?


// Sort outputs and populate output indices while keeping track of the auxiliary data
let (outputs, htlcs) = Self::internal_build_outputs(&keys, to_broadcaster_value_sat, to_countersignatory_value_sat, htlcs_with_aux, channel_parameters, &broadcaster_funding_key, &countersignatory_funding_key).unwrap();
let (outputs, nondust_htlcs) = Self::internal_build_outputs(&keys, to_broadcaster_value_sat, to_countersignatory_value_sat, nondust_htlcs, channel_parameters).unwrap();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, so now, we build a vec in Channel::build_commitment_transaction then pass a mut iterator over those entries to the tx builder, which then allocates a new vec over the (non-dust) entries which then passes them through to here which then allocates a new vec of the txout/htlc pair which then gets converted to the txout vec....

Worse, passing a mutable iterator isn't gonna fly in bindings :/.

Ultimately allocating all these vecs isn't really saving us anything, and its adding a lot of complexity. Maybe instead we just have Channel::build_commitment_transaction allocate two vecs, one with the HTLC, source pairs, another with the HTLCs (maybe a different struct that just describes the HTLCs rather than HTLCInCommitment, though it doesn't matter much), then pass the second through to the TxBuilder (owned), which can then drop dust HTLCs before passing the same vec through to here, which can just the CommitmentTransaction with the HTLCInCommitment vec and return that. Channel::build_transaction can copy the output index values into its first vec and return that and move on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
weekly goal Someone wants to land this this week
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants