Skip to content

Refactor commitment broadcast to always go through OnchainTxHandler #2703

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

Merged
merged 3 commits into from
Dec 13, 2023
Merged

Refactor commitment broadcast to always go through OnchainTxHandler #2703

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7dcee4f
Cancel previous commitment claims on newly confirmed commitment
wpaulino Oct 31, 2023
90f24a6
Refactor commitment broadcast to always go through OnchainTxHandler
wpaulino Nov 3, 2023
60bb39a
Add test coverage for holder commitment rebroadcast after reorg
wpaulino Dec 5, 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
166 changes: 112 additions & 54 deletions lightning/src/chain/channelmonitor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2659,18 +2659,59 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
}
}

fn broadcast_latest_holder_commitment_txn<B: Deref, L: Deref>(&mut self, broadcaster: &B, logger: &WithChannelMonitor<L>)
where B::Target: BroadcasterInterface,
L::Target: Logger,
{
let commit_txs = self.get_latest_holder_commitment_txn(logger);
let mut txs = vec![];
for tx in commit_txs.iter() {
log_info!(logger, "Broadcasting local {}", log_tx!(tx));
txs.push(tx);
}
broadcaster.broadcast_transactions(&txs);
fn generate_claimable_outpoints_and_watch_outputs(&mut self) -> (Vec<PackageTemplate>, Vec<TransactionOutputs>) {
let funding_outp = HolderFundingOutput::build(
self.funding_redeemscript.clone(),
self.channel_value_satoshis,
self.onchain_tx_handler.channel_type_features().clone()
);
let commitment_package = PackageTemplate::build_package(
self.funding_info.0.txid.clone(), self.funding_info.0.index as u32,
PackageSolvingData::HolderFundingOutput(funding_outp),
self.best_block.height(), self.best_block.height()
);
let mut claimable_outpoints = vec![commitment_package];
self.pending_monitor_events.push(MonitorEvent::HolderForceClosed(self.funding_info.0));
// Although we aren't signing the transaction directly here, the transaction will be signed
// in the claim that is queued to OnchainTxHandler. We set holder_tx_signed here to reject
// new channel updates.
self.holder_tx_signed = true;
let mut watch_outputs = Vec::new();
// We can't broadcast our HTLC transactions while the commitment transaction is
// unconfirmed. We'll delay doing so until we detect the confirmed commitment in
// `transactions_confirmed`.
if !self.onchain_tx_handler.channel_type_features().supports_anchors_zero_fee_htlc_tx() {
// Because we're broadcasting a commitment transaction, we should construct the package
// assuming it gets confirmed in the next block. Sadly, we have code which considers
// "not yet confirmed" things as discardable, so we cannot do that here.
let (mut new_outpoints, _) = self.get_broadcasted_holder_claims(
&self.current_holder_commitment_tx, self.best_block.height()
);
let unsigned_commitment_tx = self.onchain_tx_handler.get_unsigned_holder_commitment_tx();
let new_outputs = self.get_broadcasted_holder_watch_outputs(
&self.current_holder_commitment_tx, &unsigned_commitment_tx
);
if !new_outputs.is_empty() {
watch_outputs.push((self.current_holder_commitment_tx.txid.clone(), new_outputs));
}
claimable_outpoints.append(&mut new_outpoints);
}
(claimable_outpoints, watch_outputs)
}

pub(crate) fn queue_latest_holder_commitment_txn_for_broadcast<B: Deref, F: Deref, L: Deref>(
&mut self, broadcaster: &B, fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &WithChannelMonitor<L>
)
where
B::Target: BroadcasterInterface,
F::Target: FeeEstimator,
L::Target: Logger,
{
let (claimable_outpoints, _) = self.generate_claimable_outpoints_and_watch_outputs();
self.onchain_tx_handler.update_claims_view_from_requests(
claimable_outpoints, self.best_block.height(), self.best_block.height(), broadcaster,
fee_estimator, logger
);
}

fn update_monitor<B: Deref, F: Deref, L: Deref>(
Expand Down Expand Up @@ -2760,26 +2801,7 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
log_trace!(logger, "Avoiding commitment broadcast, already detected confirmed spend onchain");
continue;
}
self.broadcast_latest_holder_commitment_txn(broadcaster, logger);
// If the channel supports anchor outputs, we'll need to emit an external
// event to be consumed such that a child transaction is broadcast with a
// high enough feerate for the parent commitment transaction to confirm.
if self.onchain_tx_handler.channel_type_features().supports_anchors_zero_fee_htlc_tx() {
let funding_output = HolderFundingOutput::build(
self.funding_redeemscript.clone(), self.channel_value_satoshis,
self.onchain_tx_handler.channel_type_features().clone(),
);
let best_block_height = self.best_block.height();
let commitment_package = PackageTemplate::build_package(
self.funding_info.0.txid.clone(), self.funding_info.0.index as u32,
PackageSolvingData::HolderFundingOutput(funding_output),
best_block_height, best_block_height
);
self.onchain_tx_handler.update_claims_view_from_requests(
vec![commitment_package], best_block_height, best_block_height,
broadcaster, &bounded_fee_estimator, logger,
);
}
self.queue_latest_holder_commitment_txn_for_broadcast(broadcaster, &bounded_fee_estimator, logger);
} else if !self.holder_tx_signed {
log_error!(logger, "WARNING: You have a potentially-unsafe holder commitment transaction available to broadcast");
log_error!(logger, " in channel monitor for channel {}!", &self.funding_info.0.to_channel_id());
Expand Down Expand Up @@ -3356,6 +3378,58 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
}
}

/// Cancels any existing pending claims for a commitment that previously confirmed and has now
/// been replaced by another.
pub fn cancel_prev_commitment_claims<L: Deref>(
&mut self, logger: &L, confirmed_commitment_txid: &Txid
) where L::Target: Logger {
for (counterparty_commitment_txid, _) in &self.counterparty_commitment_txn_on_chain {
// Cancel any pending claims for counterparty commitments we've seen confirm.
if counterparty_commitment_txid == confirmed_commitment_txid {
continue;
}
for (htlc, _) in self.counterparty_claimable_outpoints.get(counterparty_commitment_txid).unwrap_or(&vec![]) {
log_trace!(logger, "Canceling claims for previously confirmed counterparty commitment {}",
counterparty_commitment_txid);
let mut outpoint = BitcoinOutPoint { txid: *counterparty_commitment_txid, vout: 0 };
if let Some(vout) = htlc.transaction_output_index {
outpoint.vout = vout;
self.onchain_tx_handler.abandon_claim(&outpoint);
}
}
}
if self.holder_tx_signed {
// If we've signed, we may have broadcast either commitment (prev or current), and
// attempted to claim from it immediately without waiting for a confirmation.
if self.current_holder_commitment_tx.txid != *confirmed_commitment_txid {
log_trace!(logger, "Canceling claims for previously broadcast holder commitment {}",
self.current_holder_commitment_tx.txid);
let mut outpoint = BitcoinOutPoint { txid: self.current_holder_commitment_tx.txid, vout: 0 };
for (htlc, _, _) in &self.current_holder_commitment_tx.htlc_outputs {
if let Some(vout) = htlc.transaction_output_index {
outpoint.vout = vout;
self.onchain_tx_handler.abandon_claim(&outpoint);
}
}
}
if let Some(prev_holder_commitment_tx) = &self.prev_holder_signed_commitment_tx {
if prev_holder_commitment_tx.txid != *confirmed_commitment_txid {
log_trace!(logger, "Canceling claims for previously broadcast holder commitment {}",
prev_holder_commitment_tx.txid);
let mut outpoint = BitcoinOutPoint { txid: prev_holder_commitment_tx.txid, vout: 0 };
for (htlc, _, _) in &prev_holder_commitment_tx.htlc_outputs {
if let Some(vout) = htlc.transaction_output_index {
outpoint.vout = vout;
self.onchain_tx_handler.abandon_claim(&outpoint);
}
}
}
}
} else {
// No previous claim.
}
}

fn get_latest_holder_commitment_txn<L: Deref>(
&mut self, logger: &WithChannelMonitor<L>,
) -> Vec<Transaction> where L::Target: Logger {
Expand Down Expand Up @@ -3568,6 +3642,10 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {
commitment_tx_to_counterparty_output,
},
});
// Now that we've detected a confirmed commitment transaction, attempt to cancel
// pending claims for any commitments that were previously confirmed such that
// we don't continue claiming inputs that no longer exist.
self.cancel_prev_commitment_claims(&logger, &txid);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm honestly pretty skeptical of our test coverage of re-creating claims after a reorg, which makes me pretty skeptical of this change. If we want to delete pending claims, can we instead do it after ANTI_REORG_DELAY? I'm not quite sure I understand the motivation for this commit anyway.

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'm honestly pretty skeptical of our test coverage of re-creating claims after a reorg, which makes me pretty skeptical of this change.

We have several tests covering possible reorg scenarios, are you implying we have cases uncovered?

If we want to delete pending claims, can we instead do it after ANTI_REORG_DELAY?

The claims never confirm because their inputs are now reorged out so ANTI_REORG_DELAY doesn't help.

I'm not quite sure I understand the motivation for this commit anyway.

It's mostly a nice-to-have change -- it simplifies certain test assertions and prevents us from continuously trying to claim inputs that will never succeed as they no longer exist.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We have several tests covering possible reorg scenarios, are you implying we have cases uncovered?

Where? I glanced in reorg_tests and didn't see any that were checking that if we reorg out a commitment tx we broadcast our own (replacement) commitment tx immediately afterwards.

The claims never confirm because their inputs are now reorged out so ANTI_REORG_DELAY doesn't help.

Right, I mean if we see a conflicting commitment tx we remove the conflicts here, but we could also do this after 6 confs on the conflicting commitment tx.

It's mostly a nice-to-have change -- it simplifies certain test assertions and prevents us from continuously trying to claim inputs that will never succeed as they no longer exist.

Hmm, looks like currently only one test fails? I assume this is mostly in reference to a future patchset.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where? I glanced in reorg_tests and didn't see any that were checking that if we reorg out a commitment tx we broadcast our own (replacement) commitment tx immediately afterwards.

That specific case we don't have coverage, but it all depends on whether we needed to broadcast before the reorg. I wrote a quick test locally and it checks out, so I can push that.

Right, I mean if we see a conflicting commitment tx we remove the conflicts here, but we could also do this after 6 confs on the conflicting commitment tx.

Why wait that long though? We know the previous claims are invalid as soon as the conflict confirms. Note that this is just about removing the claims that come after the commitment, not the commitment itself. We will continue to retry the commitment until one reaches ANTI_REORG_DELAY.

Hmm, looks like currently only one test fails? I assume this is mostly in reference to a future patchset.

It's not so much about the number of tests failing, but rather simplifying assertions throughout the failing test. There is a future patch to follow, but it doesn't really concern reorgs.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why wait that long though? We know the previous claims are invalid as soon as the conflict confirms. Note that this is just about removing the claims that come after the commitment, not the commitment itself. We will continue to retry the commitment until one reaches ANTI_REORG_DELAY.

Mostly because there's no new test in the first commit, and I know we have some level of missing test coverage here, and I'm not sure we can enumerate all the cases very easily so I'm just trying to be pretty cautious. Doubly so since we dont hit many reorg cases in prod so we won't discover these bugs unless its in tests.

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 don't really see the risk here. As long as we can guarantee we'll broadcast our own commitment after reorg (new test shows this), there's no chance we'll miss claiming anything from it, as once it confirms, the monitor will pick up the outputs to claim per usual.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess my concern is that we somehow forget to re-add claims for our own transactions, but you're right, your test should be pretty good for that. Can you make the test into a matrix, though, with anchors and use of B broadcasting a revoked transaction rather than a normal one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, done.

}
}
if tx.input.len() >= 1 {
Expand Down Expand Up @@ -3633,29 +3711,9 @@ impl<Signer: WriteableEcdsaChannelSigner> ChannelMonitorImpl<Signer> {

let should_broadcast = self.should_broadcast_holder_commitment_txn(logger);
if should_broadcast {
let funding_outp = HolderFundingOutput::build(self.funding_redeemscript.clone(), self.channel_value_satoshis, self.onchain_tx_handler.channel_type_features().clone());
let commitment_package = PackageTemplate::build_package(self.funding_info.0.txid.clone(), self.funding_info.0.index as u32, PackageSolvingData::HolderFundingOutput(funding_outp), self.best_block.height(), self.best_block.height());
claimable_outpoints.push(commitment_package);
self.pending_monitor_events.push(MonitorEvent::HolderForceClosed(self.funding_info.0));
// Although we aren't signing the transaction directly here, the transaction will be signed
// in the claim that is queued to OnchainTxHandler. We set holder_tx_signed here to reject
// new channel updates.
self.holder_tx_signed = true;
// We can't broadcast our HTLC transactions while the commitment transaction is
// unconfirmed. We'll delay doing so until we detect the confirmed commitment in
// `transactions_confirmed`.
if !self.onchain_tx_handler.channel_type_features().supports_anchors_zero_fee_htlc_tx() {
// Because we're broadcasting a commitment transaction, we should construct the package
// assuming it gets confirmed in the next block. Sadly, we have code which considers
// "not yet confirmed" things as discardable, so we cannot do that here.
let (mut new_outpoints, _) = self.get_broadcasted_holder_claims(&self.current_holder_commitment_tx, self.best_block.height());
let unsigned_commitment_tx = self.onchain_tx_handler.get_unsigned_holder_commitment_tx();
let new_outputs = self.get_broadcasted_holder_watch_outputs(&self.current_holder_commitment_tx, &unsigned_commitment_tx);
if !new_outputs.is_empty() {
watch_outputs.push((self.current_holder_commitment_tx.txid.clone(), new_outputs));
}
claimable_outpoints.append(&mut new_outpoints);
}
let (mut new_outpoints, mut new_outputs) = self.generate_claimable_outpoints_and_watch_outputs();
claimable_outpoints.append(&mut new_outpoints);
watch_outputs.append(&mut new_outputs);
}

// Find which on-chain events have reached their confirmation threshold.
Expand Down
19 changes: 19 additions & 0 deletions lightning/src/chain/onchaintx.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -676,6 +676,25 @@ impl<ChannelSigner: WriteableEcdsaChannelSigner> OnchainTxHandler<ChannelSigner>
None
}

pub fn abandon_claim(&mut self, outpoint: &BitcoinOutPoint) {
let claim_id = self.claimable_outpoints.get(outpoint).map(|(claim_id, _)| *claim_id)
.or_else(|| {
self.pending_claim_requests.iter()
.find(|(_, claim)| claim.outpoints().iter().any(|claim_outpoint| *claim_outpoint == outpoint))
.map(|(claim_id, _)| *claim_id)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be unreachable, right? It looks like no tests hit it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should be yes, but I included here just to be safe in the event we are tracking a pending request in pending_claim_requests that we have yet to generate a claim for.

});
if let Some(claim_id) = claim_id {
if let Some(claim) = self.pending_claim_requests.remove(&claim_id) {
for outpoint in claim.outpoints() {
self.claimable_outpoints.remove(&outpoint);
}
}
} else {
self.locktimed_packages.values_mut().for_each(|claims|
claims.retain(|claim| !claim.outpoints().iter().any(|claim_outpoint| *claim_outpoint == outpoint)));
}
}

/// Upon channelmonitor.block_connected(..) or upon provision of a preimage on the forward link
/// for this channel, provide new relevant on-chain transactions and/or new claim requests.
/// Together with `update_claims_view_from_matched_txn` this used to be named
Expand Down
Loading