-
Notifications
You must be signed in to change notification settings - Fork 385
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
base: main
Are you sure you want to change the base?
Conversation
tankyleo
commented
Feb 18, 2025
•
edited
Loading
edited
Do you also wanna add the cfg-gate to the CI testing configs? |
81b0ecb
to
647d42e
Compare
Thank you done |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3606 +/- ##
==========================================
+ Coverage 89.18% 89.25% +0.06%
==========================================
Files 155 155
Lines 119274 119206 -68
Branches 119274 119206 -68
==========================================
+ Hits 106379 106393 +14
+ Misses 10290 10223 -67
+ Partials 2605 2590 -15 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
lightning/src/sign/tx_builder.rs
Outdated
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>, |
There was a problem hiding this comment.
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.
647d42e
to
beec57f
Compare
ChannelParameters
, TxBuilder
traitsTxBuilder
trait
Rebase:
|
lightning/src/sign/tx_builder.rs
Outdated
to_broadcaster_value_sat: Amount, to_countersignatory_value_sat: Amount, | ||
trimmed_value_sat: Amount, htlcs: Vec<&mut HTLCOutputInCommitment>, | ||
secp_ctx: &Secp256k1<secp256k1::All>, | ||
) -> Transaction; |
There was a problem hiding this comment.
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
.
lightning/src/sign/tx_builder.rs
Outdated
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>, |
There was a problem hiding this comment.
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.
lightning/src/sign/tx_builder.rs
Outdated
/// 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, |
There was a problem hiding this comment.
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.
beec57f
to
0c96878
Compare
Notes:
|
lightning/src/sign/tx_builder.rs
Outdated
&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, |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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 ChannelMonitorUpdate
s 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.
There was a problem hiding this comment.
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.
0c96878
to
11f6b5f
Compare
11f6b5f
to
2f705d1
Compare
Note that this PR now breaks the public API of |
035e35f
to
42d5a79
Compare
@@ -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>, |
There was a problem hiding this comment.
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>>)>, |
There was a problem hiding this comment.
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.
lightning/src/ln/channel.rs
Outdated
@@ -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), |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turns out the switch from &mut self
to &self
was mine; this should be addressed now.
lightning/src/ln/chan_utils.rs
Outdated
@@ -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(); |
There was a problem hiding this comment.
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<_>>(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is going away soon - let me know if you'd still like me to address it; seems to me this would require adding to the API of CommitmentTransaction
.
lightning/src/ln/channel.rs
Outdated
@@ -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); |
There was a problem hiding this comment.
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?
lightning/src/ln/channel.rs
Outdated
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); |
There was a problem hiding this comment.
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.
lightning/src/ln/channel.rs
Outdated
@@ -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 { |
There was a problem hiding this comment.
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
lightning/src/ln/channel.rs
Outdated
@@ -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. |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
?
lightning/src/ln/chan_utils.rs
Outdated
|
||
// 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(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can copy the output index values into its first vec and return that and move on.
@TheBlueMatt The caveat there is that now CommitmentTransaction::htlcs
is a vector of different size, and of different order from the first vec (Vec<(HTLCOutputInCommitment, Option<&HTLCSource>)>
).
So to copy the output index values into the first, we need to map the htlcs in the second vec to the htlcs in the first vec.
So far we've been passing pointers to CommitmentTransaction
to maintain that mapping, and I am currently looking for alternatives.
d9c7ba9
to
9a1997a
Compare
@wpaulino just chatted with @TheBlueMatt offline some takeaways:
|
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`, this commit lets `CommitmentTransaction` derive the `TxCreationKeys` it will use to build the raw commitment transaction. 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 commit 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.
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. Instead of building `TxCreationKeys` explicitly, the function `TrustedCommitmentTransaction::keys` should be used when the keys of the commitment transaction are needed. We do that in this commit.
The logging for loop in `send_commitment_no_state_update` only iterates over the non-dust HTLCs. Thus we do not need to create a `Vec<HTLCOutputInCommitment>` that includes both dust and non-dust HTLCs, we can grab the `Vec<HTLCOutputInCommitment>` that holds only non-dust HTLCs directly from `CommitmentTransaction`.
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.
The fields `feerate_per_kw` and `num_nondust_htlcs` of `CommitmentStats` can both be accessed directly from the `tx: CommitmentTransaction` field.
9a1997a
to
a983014
Compare
We choose to include in `CommitmentStats` only fields that will be calculated or constructed by transaction builders. The other fields are created by channel, and placed in a new struct we call `CommitmentData`.