Integrate BumpTransactionEventHandler into existing anchor tests#2403

ariard · 2023-07-14T00:49:55Z

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 ?

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.

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).

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?

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.

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 :)

ariard · 2023-07-14T00:49:55Z

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 ?

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.

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).

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?

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.

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 :)

ariard · 2023-07-14T00:49:55Z

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 ?

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.

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).

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?

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.

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 :)

ariard · 2023-07-14T00:49:55Z

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 ?

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.

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).

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?

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.

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 :)

ariard · 2023-07-14T00:49:55Z

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 ?

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.

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).

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?

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.

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 :)

ariard · 2023-07-14T00:49:55Z

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 ?

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.

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).

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?

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.

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 :)

ariard · 2023-07-14T00:49:55Z

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 ?

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.

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).

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?

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.

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 :)

-Original file 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 @@

-Original file 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 DownExpand Up @@
  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 {
+ 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 DownExpand Up @@
  ) {
  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 @@

-Original file 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 @@

-Original file 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 @@

-Original file 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 DownExpand Up @@
  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 {
+ 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 DownExpand Up @@
  ) {
  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 @@

Uh oh!

Integrate BumpTransactionEventHandler into existing anchor tests#2403

Uh oh!

Integrate BumpTransactionEventHandler into existing anchor tests #2403

Uh oh!

Uh oh!

Diff view

Diff view

There are no files selected for viewing

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Integrate BumpTransactionEventHandler into existing anchor tests#2403

Uh oh!

Integrate BumpTransactionEventHandler into existing anchor tests #2403

Uh oh!

Uh oh!

Diff view

Diff view

There are no files selected for viewing

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Integrate BumpTransactionEventHandler into existing anchor tests#2403

Uh oh!

Integrate BumpTransactionEventHandler into existing anchor tests #2403

Uh oh!

Uh oh!

Diff view

Diff view

There are no files selected for viewing

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!