Skip to content

Integrate BumpTransactionEventHandler into existing anchor tests #2403

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 8 commits into from
Jul 17, 2023
Merged

Integrate BumpTransactionEventHandler into existing anchor tests #2403

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
19de435
Move feerate helpers to chain module
wpaulino Jul 11, 2023
990c500
Avoid yielding ChannelClose bump events with sufficient feerate
wpaulino Jul 11, 2023
9088dae
Consider existing commitment transaction feerate when bumping
wpaulino Jul 11, 2023
eac1bc1
Add debug assertions for weight estimates of bump transactions
wpaulino Jul 11, 2023
c18013c
Add log_iter utility macro
wpaulino Jul 14, 2023
964b493
Improve logging in BumpTransactionEventHandler paths
wpaulino Jul 11, 2023
38f1269
Add BumpTransactionEventHandler instance to node test harness
wpaulino Jul 11, 2023
ff474ba
Integrate BumpTransactionEventHandler into existing anchor tests
wpaulino Jul 11, 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
9 changes: 9 additions & 0 deletions lightning/src/chain/chaininterface.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,18 @@
//! disconnections, transaction broadcasting, and feerate information requests.

use core::{cmp, ops::Deref};
use core::convert::TryInto;

use bitcoin::blockdata::transaction::Transaction;

// TODO: Define typed abstraction over feerates to handle their conversions.
pub(crate) fn compute_feerate_sat_per_1000_weight(fee_sat: u64, weight: u64) -> u32 {
(fee_sat * 1000 / weight).try_into().unwrap_or(u32::max_value())
}
pub(crate) const fn fee_for_weight(feerate_sat_per_1000_weight: u32, weight: u64) -> u64 {
((feerate_sat_per_1000_weight as u64 * weight) + 1000 - 1) / 1000
}

/// An interface to send a transaction to the Bitcoin network.
pub trait BroadcasterInterface {
/// Sends a list of transactions out to (hopefully) be mined.
Expand Down
24 changes: 20 additions & 4 deletions lightning/src/chain/onchaintx.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ use bitcoin::hash_types::{Txid, BlockHash};
use bitcoin::secp256k1::{Secp256k1, ecdsa::Signature};
use bitcoin::secp256k1;

use crate::chain::chaininterface::compute_feerate_sat_per_1000_weight;
use crate::sign::{ChannelSigner, EntropySource, SignerProvider};
use crate::ln::msgs::DecodeError;
use crate::ln::PaymentPreimage;
Expand Down Expand Up @@ -623,19 +624,31 @@ impl<ChannelSigner: WriteableEcdsaChannelSigner> OnchainTxHandler<ChannelSigner>
return inputs.find_map(|input| match input {
// Commitment inputs with anchors support are the only untractable inputs supported
// thus far that require external funding.
PackageSolvingData::HolderFundingOutput(..) => {
PackageSolvingData::HolderFundingOutput(output) => {
debug_assert_eq!(tx.txid(), self.holder_commitment.trust().txid(),
"Holder commitment transaction mismatch");

let conf_target = ConfirmationTarget::HighPriority;
let package_target_feerate_sat_per_1000_weight = cached_request
.compute_package_feerate(fee_estimator, conf_target, force_feerate_bump);
if let Some(input_amount_sat) = output.funding_amount {
Copy link

Choose a reason for hiding this comment

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

Hmmm, I think we should keep yielding commitment transaction even if they meet the sufficient feerate. A commitment transaction attached with a ConfirmationTarget::HighPriority can be a hint you’re under pinning attacks or even that your transaction-relay eclipsed.

This information could be consumed by an anomalie detection module (Eclair has one for block-relay though I don’t know a Lightning implem which has one for transaction-relay, as it has been discussed in the past).

I would rather handle it back to the user and instead extend our documentation, or what do you think ? Is yielding a lot of ChannelClose raising issues in term of higher-level API ?

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 not sure how generating the events helps with that, though - we could have the same issue with anchors or non-anchors, and if you want to detect it you can do it at the transaction-broadcaster level. Most likely, the user has the events hooked into the BumpTransactionEventHandler anyway, which doesn't have any such handling and we're back to square one.

If we want some kind of big warning that something isn't confirming when we expected it to, it needs to be a separate piece of logic that handles both anchor and non-anchor channels and exists independently.

Copy link

Choose a reason for hiding this comment

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

If the following comment can be added above ChannelClose (alternatively a earliest_cltv_expiry can be added in ChannelClose generation in ChannelMonitor::get_repeated_events though won’t be used for now).

diff --git a/lightning/src/events/bump_transaction.rs b/lightning/src/events/bump_transaction.rs
index cbd5becd..097e868b 100644
--- a/lightning/src/events/bump_transaction.rs
+++ b/lightning/src/events/bump_transaction.rs
@@ -262,6 +262,12 @@ pub enum BumpTransactionEvent {
        /// for users to override LDK's behavior. On commitments with no HTLCs (indicated by those with
        /// an empty `pending_htlcs`), confirmation of the commitment transaction can be considered to
        /// be not urgent.
+       /// 
+       /// Note if the commitment transaction has pending HTLC outputs that we can satisfies with valid
+       /// witnesses, the module responsible for broadcast should be ready to take corrective actions
+       /// if the local mempool is partitioned, or full-node transaction-relay capabilities tampered
+       /// by an eclipse attack. Those corrective actions should be take on the earliest `cltv_expiry`
+       /// of the set of pending htlcs.
        ///
        /// [`EcdsaChannelSigner`]: crate::sign::EcdsaChannelSigner
        /// [`EcdsaChannelSigner::sign_holder_anchor_input`]: crate::sign::EcdsaChannelSigner::sign_holder_anchor_input

If we want some kind of big warning that something isn't confirming when we expected it to, it needs to be a separate piece of logic that handles both anchor and non-anchor channels and exists independently.

Yeah I thought above that type of automatic anomalie detection logic in the past, see bitcoin/bitcoin#18987. Good if we start to document or indicate where can be the hooks points on our side, as probably you won’t have the same logic for mobile clients vs servers (or at the very least not the same reaction policy).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Again, this isn't specific to anchor, and I don't see how its related to this PR. If you want to add a similar comment to the broadcaster (or, better, a much longer-form discussion of risks of transaction relay and the lightning security model relying on it) that would be very welcome, but I'm not sure why it has to be on the anchor-specific bumper?

Copy link

Choose a reason for hiding this comment

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

but I'm not sure why it has to be on the anchor-specific bumper?

The anomalie detection module should be based on the frequency of the anchor-specific bumper, as the “ideal" frequency should be the one of a confirmation in normal mempols propagation and the anomalie detection works compared from discrepancies of its “ideal”, so yeah to me changing the ChannelClose announcement is a loss of relevant information.

All that said, we can move it elsewhere, let’s not invalidate the review so far.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why? It doesn't matter how many times we bump, we only get useful information once per block, so if you're trying to figure out if a transaction isn't getting confirmed, you only need to look once per block, since that's the only time your transaction can get confirmed :)

let fee_sat = input_amount_sat - tx.output.iter().map(|output| output.value).sum::<u64>();
if compute_feerate_sat_per_1000_weight(fee_sat, tx.weight() as u64) >=
package_target_feerate_sat_per_1000_weight
{
log_debug!(logger, "Commitment transaction {} already meets required feerate {} sat/kW",
tx.txid(), package_target_feerate_sat_per_1000_weight);
return Some((new_timer, 0, OnchainClaim::Tx(tx.clone())));
}
}

// We'll locate an anchor output we can spend within the commitment transaction.
let funding_pubkey = &self.channel_transaction_parameters.holder_pubkeys.funding_pubkey;
match chan_utils::get_anchor_output(&tx, funding_pubkey) {
// An anchor output was found, so we should yield a funding event externally.
Some((idx, _)) => {
// TODO: Use a lower confirmation target when both our and the
// counterparty's latest commitment don't have any HTLCs present.
let conf_target = ConfirmationTarget::HighPriority;
let package_target_feerate_sat_per_1000_weight = cached_request
.compute_package_feerate(fee_estimator, conf_target, force_feerate_bump);
Some((
new_timer,
package_target_feerate_sat_per_1000_weight as u64,
Expand Down Expand Up @@ -739,6 +752,9 @@ impl<ChannelSigner: WriteableEcdsaChannelSigner> OnchainTxHandler<ChannelSigner>
) {
req.set_timer(new_timer);
req.set_feerate(new_feerate);
// Once a pending claim has an id assigned, it remains fixed until the claim is
// satisfied, regardless of whether the claim switches between different variants of
// `OnchainClaim`.
let claim_id = match claim {
OnchainClaim::Tx(tx) => {
log_info!(logger, "Broadcasting onchain {}", log_tx!(tx));
Expand Down
2 changes: 1 addition & 1 deletion lightning/src/chain/package.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -429,7 +429,7 @@ impl Readable for HolderHTLCOutput {
#[derive(Clone, PartialEq, Eq)]
pub(crate) struct HolderFundingOutput {
funding_redeemscript: Script,
funding_amount: Option<u64>,
pub(crate) funding_amount: Option<u64>,
channel_type_features: ChannelTypeFeatures,
}

Expand Down
Loading