Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
95 changes: 83 additions & 12 deletions lightning/src/chain/channelmonitor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -802,6 +802,18 @@ pub struct HolderCommitmentTransactionBalance {
/// The amount available to claim, in satoshis, excluding the on-chain fees which will be
/// required to do so.
pub amount_satoshis: u64,
Copy link
Contributor

Choose a reason for hiding this comment

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

Renaming to amount_onchain_satoshis might be helpful to avoid confusion. Or maybe it should be amount_unilateral_close_satoshis?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, good point. What about amount_claimable_satoshis? Initially I thought that might be confusing as there's a somewhat separate concept of "claimable" between on-chain and off-chain, but actually they're pretty related - the amount we're allowed to send off-chain is, basically, the amount we're able to claim on-chain in an FC, with the one exception being dust, but that only applies for channels with 0 reserve (which are mostly end-user channels and probably use the amount_offchain_satoshis. Its also a part of Balance::ClaimableOnChannelClose which makes it align better I think?

Copy link
Contributor

Choose a reason for hiding this comment

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

"Claimable" as the unilaterally recoverable balance does seem like an improvement. And it often remains hard to capture the full meaning in a name when dealing with lightning concepts.

/// The amount which is owed to us, excluding HTLCs, before dust limits, fees, anchor outputs,
/// and reserve values.
///
/// If the channel is (eventually) cooperatively closed, and this value is above the channel's
/// dust limit, then we will be paid this on-chain less any fees required for the closure.
///
/// This is generally roughly equal to [`Self::amount_satoshis`] +
/// [`Self::transaction_fee_satoshis`] + any anchor outputs in the current commitment
/// transaction. It might differ slightly due to differences in rounding and HTLC calculation.
Copy link
Contributor

Choose a reason for hiding this comment

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

What differences in rounding and htlc calculation cause this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The fee is calculated by just totaling the transaction fee (funding amount minus sum of all the outputs). The amount-offchain is the actual channel balance minus HTLCs pending outbound (in msats) plus inbound HTLCs we have the preimage for.

The HTLC difference is the fees on outputs for inbound HTLCs we have the preimages for, rounding is totally different as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

You could consider describing some of this in the code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, spent a minute fighting with it but not sure how to add it such that its actually useful information. It shouldn't differ by more than a few sats, so going into detail about how its all calculated seems kinda overkill?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes not critical to add. Just the question that came to mind when reading the comment.

///
/// This will be `None` for channels last updated on LDK 0.2 or prior.
pub amount_offchain_satoshis: Option<u64>,
/// The transaction fee we pay for the closing commitment transaction. This amount is not
/// included in the [`HolderCommitmentTransactionBalance::amount_satoshis`] value.
/// This amount includes the sum of dust HTLCs on the commitment transaction, any elided anchors,
Expand Down Expand Up @@ -978,24 +990,72 @@ impl Balance {
/// [`Balance::MaybePreimageClaimableHTLC`].
///
/// On-chain fees required to claim the balance are not included in this amount.
#[rustfmt::skip]
pub fn claimable_amount_satoshis(&self) -> u64 {
match self {
Balance::ClaimableOnChannelClose {
balance_candidates, confirmed_balance_candidate_index, ..
balance_candidates,
confirmed_balance_candidate_index,
..
} => {
if *confirmed_balance_candidate_index != 0 {
balance_candidates[*confirmed_balance_candidate_index].amount_satoshis
} else {
balance_candidates.last().map(|balance| balance.amount_satoshis).unwrap_or(0)
}
},
Balance::ClaimableAwaitingConfirmations { amount_satoshis, .. }|
Balance::ContentiousClaimable { amount_satoshis, .. }|
Balance::CounterpartyRevokedOutputClaimable { amount_satoshis, .. }
=> *amount_satoshis,
Balance::MaybeTimeoutClaimableHTLC { amount_satoshis, outbound_payment, .. }
=> if *outbound_payment { 0 } else { *amount_satoshis },
Balance::ClaimableAwaitingConfirmations { amount_satoshis, .. }
| Balance::ContentiousClaimable { amount_satoshis, .. }
| Balance::CounterpartyRevokedOutputClaimable { amount_satoshis, .. } => *amount_satoshis,
Balance::MaybeTimeoutClaimableHTLC { amount_satoshis, outbound_payment, .. } => {
if *outbound_payment {
0
} else {
*amount_satoshis
}
},
Balance::MaybePreimageClaimableHTLC { .. } => 0,
}
}

/// The "offchain balance", in satoshis.
///
/// When the channel has yet to close, this returns the balance we are owed, ignoring fees,
/// reserve values, anchors, and dust limits. This is the sum of our inbound and outbound
/// payments, initial channel contribution, and splices and may be more useful as the balance
/// displayed in an end-user wallet. Still, it is somewhat misleading from an
/// on-chain-funds-available perspective.
///
/// For pending payments, splice behavior, or behavior after a channel has been closed, this
/// behaves the same as [`Self::claimable_amount_satoshis`].
pub fn offchain_amount_satoshis(&self) -> u64 {
match self {
Balance::ClaimableOnChannelClose {
balance_candidates,
confirmed_balance_candidate_index,
..
} => {
if *confirmed_balance_candidate_index != 0 {
let candidate = &balance_candidates[*confirmed_balance_candidate_index];
candidate.amount_offchain_satoshis.unwrap_or(candidate.amount_satoshis)
} else {
balance_candidates
.last()
.map(|balance| {
balance.amount_offchain_satoshis.unwrap_or(balance.amount_satoshis)
})
.unwrap_or(0)
}
},
Balance::ClaimableAwaitingConfirmations { amount_satoshis, .. }
| Balance::ContentiousClaimable { amount_satoshis, .. }
| Balance::CounterpartyRevokedOutputClaimable { amount_satoshis, .. } => *amount_satoshis,
Balance::MaybeTimeoutClaimableHTLC { amount_satoshis, outbound_payment, .. } => {
if *outbound_payment {
0
} else {
*amount_satoshis
}
},
Balance::MaybePreimageClaimableHTLC { .. } => 0,
}
}
Expand Down Expand Up @@ -3117,6 +3177,8 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
.chain(us.pending_funding.iter())
.map(|funding| {
let to_self_value_sat = funding.current_holder_commitment_tx.to_broadcaster_value_sat();
let to_self_offchain_msat =
funding.current_holder_commitment_tx.to_broadcaster_value_offchain_msat();
// In addition to `commit_tx_fee_sat`, this can also include dust HTLCs, any
// elided anchors, and the total msat amount rounded down from non-dust HTLCs.
let transaction_fee_satoshis = if us.holder_pays_commitment_tx_fee.unwrap_or(true) {
Expand All @@ -3128,6 +3190,8 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
};
HolderCommitmentTransactionBalance {
amount_satoshis: to_self_value_sat + claimable_inbound_htlc_value_sat,
amount_offchain_satoshis:
to_self_offchain_msat.map(|v| v / 1_000 + claimable_inbound_htlc_value_sat),
transaction_fee_satoshis,
}
})
Expand Down Expand Up @@ -4556,16 +4620,23 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
})
}

#[rustfmt::skip]
fn build_counterparty_commitment_tx(
&self, channel_parameters: &ChannelTransactionParameters, commitment_number: u64,
their_per_commitment_point: &PublicKey, to_broadcaster_value: u64,
to_countersignatory_value: u64, feerate_per_kw: u32,
nondust_htlcs: Vec<HTLCOutputInCommitment>
nondust_htlcs: Vec<HTLCOutputInCommitment>,
) -> CommitmentTransaction {
let channel_parameters = &channel_parameters.as_counterparty_broadcastable();
CommitmentTransaction::new(commitment_number, their_per_commitment_point,
to_broadcaster_value, to_countersignatory_value, feerate_per_kw, nondust_htlcs, channel_parameters, &self.onchain_tx_handler.secp_ctx)
CommitmentTransaction::new_without_broadcaster_offchain_value(
commitment_number,
their_per_commitment_point,
to_broadcaster_value,
to_countersignatory_value,
feerate_per_kw,
nondust_htlcs,
channel_parameters,
&self.onchain_tx_handler.secp_ctx,
)
}

#[rustfmt::skip]
Expand Down
98 changes: 86 additions & 12 deletions lightning/src/ln/chan_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1375,7 +1375,9 @@ impl HolderCommitmentTransaction {
for _ in 0..nondust_htlcs.len() {
counterparty_htlc_sigs.push(dummy_sig);
}
let inner = CommitmentTransaction::new(0, &dummy_key, 0, 0, 0, nondust_htlcs, &channel_parameters.as_counterparty_broadcastable(), &secp_ctx);
let inner = CommitmentTransaction::new_without_broadcaster_offchain_value(
0, &dummy_key, 0, 0, 0, nondust_htlcs, &channel_parameters.as_counterparty_broadcastable(), &secp_ctx
);
HolderCommitmentTransaction {
inner,
counterparty_sig: dummy_sig,
Expand Down Expand Up @@ -1606,6 +1608,7 @@ impl<'a> TrustedClosingTransaction<'a> {
#[derive(Clone, Debug)]
pub struct CommitmentTransaction {
commitment_number: u64,
to_broadcaster_value_offchain_msat: Option<u64>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Were there other data structures that could have stored this value? The field looks a bit out of place between all the on-chain properties.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We could put it directly in the ChannelMonitorUpdateStep::LatestCounterpartyCommitment but it doesn't really feel like it belongs there any more. The nice thing about being in the CommitmentTransaction is it will (eventually) go through the custom commitment transaction API, which I think feels right?

Copy link
Contributor

Choose a reason for hiding this comment

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

LatestCounterpartyCommitment does have some additional non-on-chain info like htlc sources already. But not sure how those two options compare.

The nice thing about being in the CommitmentTransaction is it will (eventually) go through the custom commitment transaction API

Can you elaborate a bit more on what it brings to have to_broadcaster_value_offchain_msat go through the custom commit tx API? That the calculation of value_offchain can also be custom you mean?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That the calculation of value_offchain can also be custom you mean?

Right, this. Some "balance" might actually be locked up in some sub-protocol LDK isn't aware of, at least eventually.

Copy link
Contributor

Choose a reason for hiding this comment

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

See comment below, and PR #4026 for a potential path to customization of this offchain value.

to_broadcaster_value_sat: Amount,
to_countersignatory_value_sat: Amount,
to_broadcaster_delay: Option<u16>, // Added in 0.0.117
Expand All @@ -1626,6 +1629,7 @@ impl PartialEq for CommitmentTransaction {
#[rustfmt::skip]
fn eq(&self, o: &Self) -> bool {
let eq = self.commitment_number == o.commitment_number &&
self.to_broadcaster_value_offchain_msat == o.to_broadcaster_value_offchain_msat &&
self.to_broadcaster_value_sat == o.to_broadcaster_value_sat &&
self.to_countersignatory_value_sat == o.to_countersignatory_value_sat &&
self.feerate_per_kw == o.feerate_per_kw &&
Expand Down Expand Up @@ -1655,6 +1659,7 @@ impl Writeable for CommitmentTransaction {
(12, self.nondust_htlcs, required_vec),
(14, legacy_deserialization_prevention_marker, option),
(15, self.channel_type_features, required),
(17, self.to_broadcaster_value_offchain_msat, option),
});
Ok(())
}
Expand All @@ -1674,6 +1679,7 @@ impl Readable for CommitmentTransaction {
(12, nondust_htlcs, required_vec),
(14, _legacy_deserialization_prevention_marker, (option, explicit_type: ())),
(15, channel_type_features, option),
(17, to_broadcaster_value_offchain_msat, option),
});

let mut additional_features = ChannelTypeFeatures::empty();
Expand All @@ -1683,6 +1689,7 @@ impl Readable for CommitmentTransaction {
Ok(Self {
commitment_number: commitment_number.0.unwrap(),
to_broadcaster_value_sat: to_broadcaster_value_sat.0.unwrap(),
to_broadcaster_value_offchain_msat,
to_countersignatory_value_sat: to_countersignatory_value_sat.0.unwrap(),
to_broadcaster_delay,
feerate_per_kw: feerate_per_kw.0.unwrap(),
Expand All @@ -1700,33 +1707,94 @@ impl CommitmentTransaction {
/// All HTLCs MUST be above the dust limit for the channel.
/// The broadcaster and countersignatory amounts MUST be either 0 or above dust. If the amount
/// is 0, the corresponding output will be omitted from the transaction.
#[rustfmt::skip]
pub fn new(commitment_number: u64, per_commitment_point: &PublicKey, to_broadcaster_value_sat: u64, to_countersignatory_value_sat: u64, feerate_per_kw: u32, mut nondust_htlcs: Vec<HTLCOutputInCommitment>, channel_parameters: &DirectedChannelTransactionParameters, secp_ctx: &Secp256k1<secp256k1::All>) -> CommitmentTransaction {
pub fn new(
commitment_number: u64, per_commitment_point: &PublicKey, to_broadcaster_value_sat: u64,
to_broadcaster_value_offchain_msat: u64, to_countersignatory_value_sat: u64,
feerate_per_kw: u32, nondust_htlcs: Vec<HTLCOutputInCommitment>,
channel_parameters: &DirectedChannelTransactionParameters,
secp_ctx: &Secp256k1<secp256k1::All>,
) -> CommitmentTransaction {
debug_assert!(to_broadcaster_value_sat * 1000 <= to_broadcaster_value_offchain_msat);
Self::build(
commitment_number,
per_commitment_point,
to_broadcaster_value_sat,
Some(to_broadcaster_value_offchain_msat),
to_countersignatory_value_sat,
feerate_per_kw,
nondust_htlcs,
channel_parameters,
secp_ctx,
)
}

pub(crate) fn new_without_broadcaster_offchain_value(
commitment_number: u64, per_commitment_point: &PublicKey, to_broadcaster_value_sat: u64,
to_countersignatory_value_sat: u64, feerate_per_kw: u32,
nondust_htlcs: Vec<HTLCOutputInCommitment>,
channel_parameters: &DirectedChannelTransactionParameters,
secp_ctx: &Secp256k1<secp256k1::All>,
) -> CommitmentTransaction {
Self::build(
commitment_number,
per_commitment_point,
to_broadcaster_value_sat,
None,
to_countersignatory_value_sat,
feerate_per_kw,
nondust_htlcs,
channel_parameters,
secp_ctx,
)
}

fn build(
commitment_number: u64, per_commitment_point: &PublicKey, to_broadcaster_value_sat: u64,
to_broadcaster_value_offchain_msat: Option<u64>, to_countersignatory_value_sat: u64,
feerate_per_kw: u32, mut nondust_htlcs: Vec<HTLCOutputInCommitment>,
channel_parameters: &DirectedChannelTransactionParameters,
secp_ctx: &Secp256k1<secp256k1::All>,
) -> CommitmentTransaction {
let to_broadcaster_value_sat = Amount::from_sat(to_broadcaster_value_sat);
let to_countersignatory_value_sat = Amount::from_sat(to_countersignatory_value_sat);
let keys = TxCreationKeys::from_channel_static_keys(per_commitment_point, channel_parameters.broadcaster_pubkeys(), channel_parameters.countersignatory_pubkeys(), secp_ctx);
let keys = TxCreationKeys::from_channel_static_keys(
per_commitment_point,
channel_parameters.broadcaster_pubkeys(),
channel_parameters.countersignatory_pubkeys(),
secp_ctx,
);

// Build and sort the outputs of the transaction.
// Also sort the HTLC output data in `nondust_htlcs` in the same order, and populate the
// transaction output indices therein.
let outputs = Self::build_outputs_and_htlcs(&keys, to_broadcaster_value_sat, to_countersignatory_value_sat, &mut nondust_htlcs, channel_parameters);
let outputs = Self::build_outputs_and_htlcs(
&keys,
to_broadcaster_value_sat,
to_countersignatory_value_sat,
&mut nondust_htlcs,
channel_parameters,
);

let (obscured_commitment_transaction_number, txins) = Self::build_inputs(commitment_number, channel_parameters);
let transaction = Self::make_transaction(obscured_commitment_transaction_number, txins, outputs, channel_parameters);
let (obscured_commitment_transaction_number, txins) =
Self::build_inputs(commitment_number, channel_parameters);
let transaction = Self::make_transaction(
obscured_commitment_transaction_number,
txins,
outputs,
channel_parameters,
);
let txid = transaction.compute_txid();
CommitmentTransaction {
commitment_number,
to_broadcaster_value_sat,
to_broadcaster_value_offchain_msat,
to_countersignatory_value_sat,
to_broadcaster_delay: Some(channel_parameters.contest_delay()),
feerate_per_kw,
nondust_htlcs,
channel_type_features: channel_parameters.channel_type_features().clone(),
keys,
built: BuiltCommitmentTransaction {
transaction,
txid
},
built: BuiltCommitmentTransaction { transaction, txid },
}
}

Expand Down Expand Up @@ -2034,6 +2102,12 @@ impl CommitmentTransaction {
self.keys.per_commitment_point
}

/// The value which is owed the broadcaster (excluding HTLCs) before reductions due to dust
/// limits, being rounded down to the nearest sat, reserve value(s).
pub fn to_broadcaster_value_offchain_msat(&self) -> Option<u64> {
self.to_broadcaster_value_offchain_msat
}

/// The value to be sent to the broadcaster
pub fn to_broadcaster_value_sat(&self) -> u64 {
self.to_broadcaster_value_sat.to_sat()
Expand Down Expand Up @@ -2324,7 +2398,7 @@ mod tests {

#[rustfmt::skip]
fn build(&self, to_broadcaster_sats: u64, to_countersignatory_sats: u64, nondust_htlcs: Vec<HTLCOutputInCommitment>) -> CommitmentTransaction {
CommitmentTransaction::new(
CommitmentTransaction::new_without_broadcaster_offchain_value(
self.commitment_number, &self.per_commitment_point, to_broadcaster_sats, to_countersignatory_sats, self.feerate_per_kw,
nondust_htlcs, &self.channel_parameters.as_holder_broadcastable(), &self.secp_ctx
)
Expand Down
2 changes: 1 addition & 1 deletion lightning/src/ln/htlc_reserve_unit_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2346,7 +2346,7 @@ pub fn do_test_dust_limit_fee_accounting(can_afford: bool) {
get_channel_ref!(nodes[0], nodes[1], per_peer_lock, peer_state_lock, chan_id);
let chan_signer = channel.as_funded().unwrap().get_signer();

let commitment_tx = CommitmentTransaction::new(
let commitment_tx = CommitmentTransaction::new_without_broadcaster_offchain_value(
commitment_number,
&remote_point,
node_1_balance_sat,
Expand Down
Loading