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.

Sign up for GitHub

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

Address 2609 follow-up comments #2624

Merged
merged 2 commits into from
Sep 29, 2023
Merged

Address 2609 follow-up comments #2624

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6cf0351
Note required levels of descendant transactions in get_spendable_outputs
wpaulino Sep 29, 2023
f267a30
Only yield DelayedPaymentOutput descriptors once their delay expires
wpaulino Sep 29, 2023
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
20 changes: 13 additions & 7 deletions lightning/src/chain/channelmonitor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1672,6 +1672,8 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitor<Signer> {

/// Returns the descriptors for relevant outputs (i.e., those that we can spend) within the
/// transaction if they exist and the transaction has at least [`ANTI_REORG_DELAY`]
/// confirmations. For [`SpendableOutputDescriptor::DelayedPaymentOutput`] descriptors to be
/// returned, the transaction must have at least `max(ANTI_REORG_DELAY, to_self_delay)`
/// confirmations.
///
/// Descriptors returned by this method are primarily exposed via [`Event::SpendableOutputs`]
Expand All @@ -1680,8 +1682,7 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitor<Signer> {
/// missed/unhandled descriptors. For the purpose of gathering historical records, if the
/// channel close has fully resolved (i.e., [`ChannelMonitor::get_claimable_balances`] returns
/// an empty set), you can retrieve all spendable outputs by providing all descendant spending
/// transactions starting from the channel's funding or closing transaction that have at least
/// [`ANTI_REORG_DELAY`] confirmations.
/// transactions starting from the channel's funding transaction and going down three levels.
Copy link

@tlulu tlulu Oct 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Just to clarify, do the three levels here mean looking at these txns?

  1. Funding tx
  2. Closing tx
  3. Tx spending the closing tx outputs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The funding transaction will never match. It's the closing transaction, all spends of the closing transaction, and all spends of those which spend the closing transaction.

Copy link

Choose a reason for hiding this comment

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

Makes sense that the funding transaction isn't needed. Why is the third level needed? Isn't the closing transaction and all spends of the closing transaction enough?

Copy link
Contributor Author

@wpaulino wpaulino Oct 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Because HTLCs are resolved in two stages when you broadcast your own commitment transaction. The first spends the HTLC output in the commitment transaction via the timeout or preimage path. The second spends the output in that timeout/preimage transaction after a CSV delay to give the counterparty a chance to claim it instead if the HTLC came from a revoked commitment.

Copy link

Choose a reason for hiding this comment

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

Oh that's right, there could be the revocation tx as well!

So this scenario would be:

  1. Commitment tx (closing tx)
  2. Revocation tx (spends the commitment tx)
  3. Tx spending the revocation tx

We need the third layer because we want to make sure that the output is spent right?

Thanks for the speedy reply btw 🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have LN-related outputs to claim within each of those transactions. Any spends after that will be from your wallet.

Copy link

@tlulu tlulu Nov 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Because HTLCs are resolved in two stages when you broadcast your own commitment transaction.

I didn't fully grok the two stage HTLC protocol until now. The way I look at it, I still only see two layers that we need to check. I.e

  1. Commitment Tx
  2. HTLC-timeout Tx (which contains the LDK spendable output)
Screenshot 2023-10-30 at 2 15 57 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the HTLC-timeout/success transaction is from a revoked commitment, then LDK will claim it with a new transaction via the revocation path, yielding another SpendableOutputDescriptor.

Copy link

Choose a reason for hiding this comment

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

Gotcha, that makes sense

///
/// `tx` is a transaction we'll scan the outputs of. Any transaction can be provided. If any
/// outputs which can be spent by us are found, at least one descriptor is returned.
Expand All @@ -1690,11 +1691,16 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitor<Signer> {
pub fn get_spendable_outputs(&self, tx: &Transaction, confirmation_height: u32) -> Vec<SpendableOutputDescriptor> {
let inner = self.inner.lock().unwrap();
let current_height = inner.best_block.height;
if current_height.saturating_sub(ANTI_REORG_DELAY) + 1 >= confirmation_height {
inner.get_spendable_outputs(tx)
} else {
Vec::new()
}
let mut spendable_outputs = inner.get_spendable_outputs(tx);
spendable_outputs.retain(|descriptor| {
let mut conf_threshold = current_height.saturating_sub(ANTI_REORG_DELAY) + 1;
if let SpendableOutputDescriptor::DelayedPaymentOutput(descriptor) = descriptor {
conf_threshold = cmp::min(conf_threshold,
current_height.saturating_sub(descriptor.to_self_delay as u32) + 1);
}
conf_threshold >= confirmation_height
});
spendable_outputs
}
}

Expand Down
13 changes: 10 additions & 3 deletions lightning/src/ln/monitor_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -637,7 +637,7 @@ fn test_balances_on_local_commitment_htlcs() {
// First confirm the commitment transaction on nodes[0], which should leave us with three
// claimable balances.
let node_a_commitment_claimable = nodes[0].best_block_info().1 + BREAKDOWN_TIMEOUT as u32;
mine_transaction(&nodes[0], &as_txn[0]);
let commitment_tx_conf_height_a = block_from_scid(&mine_transaction(&nodes[0], &as_txn[0]));
check_added_monitors!(nodes[0], 1);
check_closed_broadcast!(nodes[0], true);
check_closed_event!(nodes[0], 1, ClosureReason::CommitmentTxConfirmed, [nodes[1].node.get_our_node_id()], 1000000);
Expand Down Expand Up @@ -729,13 +729,20 @@ fn test_balances_on_local_commitment_htlcs() {

// Connect blocks until the commitment transaction's CSV expires, providing us the relevant
// `SpendableOutputs` event and removing the claimable balance entry.
connect_blocks(&nodes[0], node_a_commitment_claimable - nodes[0].best_block_info().1);
connect_blocks(&nodes[0], node_a_commitment_claimable - nodes[0].best_block_info().1 - 1);
assert!(get_monitor!(nodes[0], chan_id)
.get_spendable_outputs(&as_txn[0], commitment_tx_conf_height_a).is_empty());
connect_blocks(&nodes[0], 1);
assert_eq!(vec![Balance::ClaimableAwaitingConfirmations {
amount_satoshis: 10_000,
confirmation_height: node_a_htlc_claimable,
}],
nodes[0].chain_monitor.chain_monitor.get_monitor(funding_outpoint).unwrap().get_claimable_balances());
test_spendable_output(&nodes[0], &as_txn[0]);
let to_self_spendable_output = test_spendable_output(&nodes[0], &as_txn[0]);
assert_eq!(
get_monitor!(nodes[0], chan_id).get_spendable_outputs(&as_txn[0], commitment_tx_conf_height_a),
to_self_spendable_output
);

// Connect blocks until the HTLC-Timeout's CSV expires, providing us the relevant
// `SpendableOutputs` event and removing the claimable balance entry.
Expand Down