Integrate BumpTransactionEventHandler into existing anchor tests #2403

wpaulino · 2023-07-12T00:09:31Z

This PR integrates the BumpTransactionEventHandler into existing anchor tests to get some free coverage. Along the way, we address a few inefficiencies around bumping commitment transactions, add debug assertions for weight estimates, and improve logging within the event handler.

Depends on #2393.

codecov-commenter · 2023-07-12T01:25:47Z

Codecov Report

Patch coverage: 89.23% and project coverage change: +0.02 🎉

Comparison is base (07606c1) 90.26% compared to head (ff474ba) 90.28%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2403      +/-   ##
==========================================
+ Coverage   90.26%   90.28%   +0.02%     
==========================================
  Files         106      106              
  Lines       55188    55657     +469     
  Branches    55188    55657     +469     
==========================================
+ Hits        49815    50251     +436     
- Misses       5373     5406      +33

Impacted Files	Coverage Δ
lightning/src/util/macro_logger.rs	`89.58% <ø> (-2.09%)`	⬇️
lightning/src/chain/onchaintx.rs	`93.05% <62.50%> (-0.02%)`	⬇️
lightning/src/util/logger.rs	`83.96% <80.00%> (-0.42%)`	⬇️
lightning/src/ln/functional_test_utils.rs	`88.73% <82.35%> (+0.55%)`	⬆️
lightning/src/util/test_utils.rs	`72.58% <83.33%> (+1.02%)`	⬆️
lightning/src/events/bump_transaction.rs	`79.93% <94.23%> (+39.56%)`	⬆️
lightning/src/ln/monitor_tests.rs	`98.52% <94.91%> (+0.18%)`	⬆️
lightning/src/chain/chaininterface.rs	`96.29% <100.00%> (+1.05%)`	⬆️
lightning/src/chain/package.rs	`91.55% <100.00%> (+0.12%)`	⬆️

... and 60 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

tnull · 2023-07-12T19:19:07Z

Needs rebase now that #2393 landed.

lightning/src/chain/onchaintx.rs

lightning/src/chain/chaininterface.rs

lightning/src/events/bump_transaction.rs

TheBlueMatt

LGTM, mod @tnull's comments.

lightning/src/events/bump_transaction.rs

lightning/src/chain/chaininterface.rs

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

lightning/src/chain/onchaintx.rs

+					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 {


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-14T01:07:09Z

lightning/src/events/bump_transaction.rs

+			commitment_tx_fee_sat, commitment_tx.weight() as u64,
+		);
+		let anchor_target_feerate_sat_per_1000_weight = core::cmp::max(
+			package_target_feerate_sat_per_1000_weight - commitment_tx_sat_per_1000_weight,


IIUC here we’re subtracting the commitment transaction feerate sat per 1000 weight from the anchor feerate sat per 1000 weight. If the commitment transaction is 10 times bigger than the child, the anchor feerate might not be sufficient to meet efficient propagation and selection in network mempools.

I think this should rather go to 1) compute the commitment transaction absolute fee with its current feerate, 2) compute the commitment transaction absolute fee with ConfirmationTarget::HighPriority, 3) subtract result one from result two, 4) select enough UTXO value to meet the gap assuming some anchor transaction number of inputs (as it’s the variable, all other transaction fields are invariant).

So I understand the concern laid about “we don’t yet know the anchor transaction size” though I would rather go the direction of modifying select_confirmed_utxos to take a satoshis_value_to_complete and parameterize with per_additional_feerate_sat_per_1000_weight.

Current state of code sounds to me we might propagate time-sensitive commitment transaction that broadcast feerate inefficient txn, like FEERATE_FLOOR_SATS_PER_KW is only a belt-and-suspender for min_relay_tx_fee iirc.

The must_spend set that we pass to the user has both the commitment transaction and the input as its satisfaction_weight - thus even if the commitment transaction is huge, its still considered in the counting of the anchor-spending transaction. We'll always overshoot using this math, but we won't overshoot by as much.

thus even if the commitment transaction is huge,

In a world where an adversary can route update_add_htlc over your node to inflate the size of your commitment transaction, I think we would be better to be careful to have precise bounds on how much we overshoot (and a drain vector on our fee-bumping reserves).

I think this would benefit to have a functional test to get in sync on the feerate math here, and the overshoot bounds.

The total effective fee is the weight-weighted average of the feerates across the commitment and anchor transactions. Currently we simply ask the user to match the sought feerate, which may result in substantially under-shooting the sought feerate if the commitment transaction is large. You make a good point that overshooting may exhaust our available funds for anchoring, but I'd much rather that than substantially undershoot our fee estimator and end up not getting our transactions confirmed.

Luckily we don't overshoot by that much - we overshoot the desired feerate by commitment_fee / commitment_weight - commitment_fee / (commitment_weight + anchor_weight) sat/vB [1]. Thus, if the commitment transaction is huge, we don't really overshoot at all. If the commitment transaction is tiny and it has a pretty high fee relative to how much we want to pay, we may overshoot by more, but I think at least in that scenario we're okay paying some more.

[1] math:

base definitions - (_fr = fee rate, _f = fee, _w = weight) - (com_ = commitment tx, anc_ = anchor tx) - (actual = what we paid, sought = what we wanted to pay) # the definition of actual feerate across the package actual_fr = (com_f + anc_f) / (com_w + anc_w) # the tx anchor fee, i.e. what we told the user to put on the anchor tx anc_f = (sought_fr - (com_f / com_w)) * (com_w + anc_w) # now sub second equasion into the first and simplify: actual = (com_f + (sought_fr - (com_f / com_w)) * (com_w + anc_w)) / (com_w + anc_w) actual_fr = com_f / (com_w + anc_w) + (sought_fr - (com_f / com_w)) * (com_w + anc_w)) / (com_w + anc_w) actual_fr = com_f / (com_w + anc_w) + (sought_fr - (com_f / com_w)) actual_fr = com_f / (com_w + anc_w) + sought_fr - (com_f / com_w) actual_fr - sought_fr = com_f / (com_w + anc_w) - (com_f / com_w) sought_fr - actual_fr (i.e. how much we overpay, as a *rate*) = com_f / com_w - com_f / (com_w + anc_w)

Currently we simply ask the user to match the sought feerate, which may result in substantially under-shooting the sought feerate if the commitment transaction is large.

This is correct from looking at code at (8e2b70d9) neither current CoinSelectionSource::select_confirmed_utxos’s target_feerate_sat_per_1000_weight or BumpTransactionEventHandler::build_anchor_tx document clearly what is included in their respective target_feerate_sat_per_1000_weight from what I can grep. Note how it can be confusing as you have HTLCResolution::target_feerate_sat_per_1000_weight and ChannelClose::package_target_feerate_per_1000_weight.

So yes if you don’t include the commitment weight in your weight computation I agree you will undershoot and your transaction won’t propagate (fuck with all my respect for Wilmer hard work during the past year here, this API gently sucks in its current state for something which is safety critical).

You make a good point that overshooting may exhaust our available funds for anchoring, but I'd much rather that than substantially undershoot our fee estimator and end up not getting our transactions confirmed.

Well I observe again than fee-bumping reserves are supposed to be under your local LN node control though note the commitment weight is not due to how forwarding works (or at least unless in the bounds of your channel policy parameters…). I don’t know what is worst, this is depend the deployment.

Thanks for the math, I’ll argue the API suggested above with code-enforced bounds on the commitment weight is better, so now I'm thinking we might have to leak the channel policy parameters there in CoinSelectionSource so I think conversation can be moved in its own issue / PR.

This is correct from looking at code at (8e2b70d)

Oops, I'm sorry, I misspoke, we definitely don't undershoot today, but we substantially overshoot, and this change is making it so that we barely overshoot :).

fuck with all my respect for Wilmer hard work during the past year here, this API gently sucks in its current state for something which is safety critical

I'm sorry, what? The API is quite clear that you have to consider the satisfaction rate when you're building the transaction to meet the given weight. The naive user implementation works exactly correctly. More generally, please leave the insults elsewhere, there's no need for hostility here.

lightning/src/events/bump_transaction.rs

TheBlueMatt · 2023-07-14T21:01:21Z

Looks like the current code doesn't build.

We plan to use these outside of the `bump_transaction` module in the next commit, and they really should belong in the same module as `FeeEstimator`.

There's no need to yield such an event when the commitment transaction already meets the target feerate on its own, so we can simply broadcast it without an anchor child transaction. This may be a common occurrence until we are less aggressive about feerate updates.

With anchors, we've yet to change the frequency or aggressiveness of feerate updates, so it's likely that commitment transactions have a good enough feerate to confirm on its own. In any case, when producing a child anchor transaction, we should already take into account the fees paid by the commitment transaction itself, allowing the user to save some satoshis. Unfortunately, in its current form, this will still result in overpaying by a small margin at the expense of making the coin selection API more complex.

This ensures our estimates are correct by never underestimating and only allowing overestimations by a margin of 1%.

This is a useful primitive to have that formats the contents of the iterator as a comma-separated list.

wpaulino · 2023-07-14T21:48:39Z

Squashed and switched the logger macro to use iterators instead:

$ git diff-tree -U1 922db51 ff474ba3
diff --git a/lightning/src/events/bump_transaction.rs b/lightning/src/events/bump_transaction.rs
index bc92a95c..bf56a84c 100644
--- a/lightning/src/events/bump_transaction.rs
+++ b/lightning/src/events/bump_transaction.rs
@@ -859,3 +859,3 @@ where
 				log_info!(self.logger, "Handling HTLC bump (claim_id = {}, htlcs_to_claim = {})",
-					log_bytes!(claim_id.0), log_vec!(htlc_descriptors.iter().map(|d| d.outpoint()).collect()));
+					log_bytes!(claim_id.0), log_iter!(htlc_descriptors.iter().map(|d| d.outpoint())));
 				if let Err(_) = self.handle_htlc_resolution(
diff --git a/lightning/src/util/logger.rs b/lightning/src/util/logger.rs
index b0068f4f..722a81f7 100644
--- a/lightning/src/util/logger.rs
+++ b/lightning/src/util/logger.rs
@@ -171,3 +171,3 @@ impl<'a> core::fmt::Display for DebugBytes<'a> {
 
-/// Wrapper for logging `Vec`s.
+/// Wrapper for logging `Iterator`s.
 ///
@@ -175,11 +175,15 @@ impl<'a> core::fmt::Display for DebugBytes<'a> {
 #[doc(hidden)]
-pub struct DebugVec<'a, T: core::fmt::Display>(pub &'a Vec<T>);
-impl<'a, T: core::fmt::Display> core::fmt::Display for DebugVec<'a, T> {
-	fn fmt(&self, f: &mut core::fmt::Formatter) -> Result<(), core::fmt::Error> {
+pub struct DebugIter<T: fmt::Display, I: core::iter::Iterator<Item = T> + Clone>(pub core::cell::RefCell<I>);
+impl<T: fmt::Display, I: core::iter::Iterator<Item = T> + Clone> fmt::Display for DebugIter<T, I> {
+	fn fmt(&self, f: &mut fmt::Formatter) -> Result<(), fmt::Error> {
+		use core::ops::DerefMut;
 		write!(f, "[")?;
-		if let Some(item) = self.0.first() {
+		let iter_ref = self.0.clone();
+		let mut iter = iter_ref.borrow_mut();
+		for item in iter.deref_mut() {
 			write!(f, "{}", item)?;
+			break;
 		}
-		for i in self.0.iter().skip(1) {
-			write!(f, ", {}", i)?;
+		for item in iter.deref_mut() {
+			write!(f, ", {}", item)?;
 		}
diff --git a/lightning/src/util/macro_logger.rs b/lightning/src/util/macro_logger.rs
index fa37cb05..4703bcdb 100644
--- a/lightning/src/util/macro_logger.rs
+++ b/lightning/src/util/macro_logger.rs
@@ -19,5 +19,5 @@ use crate::util::logger::DebugBytes;
 
-macro_rules! log_vec {
+macro_rules! log_iter {
 	($obj: expr) => {
-		$crate::util::logger::DebugVec(&$obj)
+		$crate::util::logger::DebugIter(core::cell::RefCell::new($obj))
 	}

ariard · 2023-07-15T00:03:20Z

See comment here #2403 (comment) it sounds to me a bit as a safety regression, at the very least it would be good to know our “worst-case overshoot bound” or see if my proposed method of computing feerate isn’t a better belt-and-suspender

If the changes here are blocker for advance in ldk-node, I think we can split 9088dae in its own PR and weigh the feerate consumption trade-off there.

TheBlueMatt · 2023-07-15T01:14:44Z

See comment here #2403 (comment) it sounds to me a bit as a safety regression

Its not a regression if its better than the current code :).

ariard · 2023-07-15T02:37:40Z

Its not a regression if its better than the current code :).

See my answer comment, this could be legendary bike-shedding in function of the deployment / channel policy parameters considered and so in function of those criterias, it might be a regression.

Anyway anchor output is still beta for 0.0.116 and fee-bumping reserves management can be more focus in 0.0.117 to finally have a default sane anchor output API so I’m fine with it :)

Reviewing the code again.

lightning/src/util/logger.rs

ariard

Code Review ACK 2c6ac88b, comments are not blocking. Have not exercised the modified code paths to check accurate test coverage.

lightning/src/chain/onchaintx.rs

TheBlueMatt · 2023-07-17T17:18:17Z

Feel free to address remaining comments in a followup.

PR #2403 fixups

wpaulino added this to the 0.0.116 milestone Jul 12, 2023

wpaulino mentioned this pull request Jul 12, 2023

Bump transaction event handler fixups #2393

Merged

wpaulino force-pushed the bump-transaction-event-handler-tests branch from 6e932b6 to 2c6ac88 Compare July 12, 2023 19:28

TheBlueMatt reviewed Jul 12, 2023

View reviewed changes

lightning/src/chain/onchaintx.rs Show resolved Hide resolved

TheBlueMatt assigned tnull and TheBlueMatt Jul 12, 2023

tnull reviewed Jul 13, 2023

View reviewed changes

lightning/src/chain/chaininterface.rs Show resolved Hide resolved

lightning/src/events/bump_transaction.rs Show resolved Hide resolved

lightning/src/events/bump_transaction.rs Outdated Show resolved Hide resolved

lightning/src/events/bump_transaction.rs Show resolved Hide resolved

TheBlueMatt reviewed Jul 13, 2023

View reviewed changes

lightning/src/events/bump_transaction.rs Outdated Show resolved Hide resolved

ariard reviewed Jul 14, 2023

View reviewed changes

tnull reviewed Jul 14, 2023

View reviewed changes

lightning/src/events/bump_transaction.rs Show resolved Hide resolved

wpaulino force-pushed the bump-transaction-event-handler-tests branch from 2c6ac88 to 922db51 Compare July 14, 2023 18:36

wpaulino added the Anchor Support label Jul 14, 2023

wpaulino added 8 commits July 14, 2023 14:45

Move feerate helpers to chain module

19de435

We plan to use these outside of the `bump_transaction` module in the next commit, and they really should belong in the same module as `FeeEstimator`.

Add debug assertions for weight estimates of bump transactions

eac1bc1

This ensures our estimates are correct by never underestimating and only allowing overestimations by a margin of 1%.

Add log_iter utility macro

c18013c

This is a useful primitive to have that formats the contents of the iterator as a comma-separated list.

Improve logging in BumpTransactionEventHandler paths

964b493

Add BumpTransactionEventHandler instance to node test harness

38f1269

Integrate BumpTransactionEventHandler into existing anchor tests

ff474ba

wpaulino force-pushed the bump-transaction-event-handler-tests branch from 922db51 to ff474ba Compare July 14, 2023 21:46

TheBlueMatt approved these changes Jul 15, 2023

View reviewed changes

lightning/src/util/logger.rs Show resolved Hide resolved

lightning/src/util/logger.rs Show resolved Hide resolved

ariard approved these changes Jul 15, 2023

View reviewed changes

lightning/src/chain/onchaintx.rs Show resolved Hide resolved

lightning/src/chain/onchaintx.rs Show resolved Hide resolved

tnull approved these changes Jul 17, 2023

View reviewed changes

TheBlueMatt merged commit ba8af22 into lightningdevkit:main Jul 17, 2023
14 checks passed

wpaulino deleted the bump-transaction-event-handler-tests branch July 17, 2023 17:40

TheBlueMatt added a commit that referenced this pull request Jul 19, 2023

Merge pull request #2423 from wpaulino/2403-fixups

8a8f29a

PR #2403 fixups

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate BumpTransactionEventHandler into existing anchor tests #2403

Integrate BumpTransactionEventHandler into existing anchor tests #2403

wpaulino commented Jul 12, 2023

codecov-commenter commented Jul 12, 2023 •

edited

tnull commented Jul 12, 2023

TheBlueMatt left a comment

ariard Jul 14, 2023

TheBlueMatt Jul 14, 2023

ariard Jul 14, 2023

TheBlueMatt Jul 15, 2023

ariard Jul 15, 2023

TheBlueMatt Jul 15, 2023

ariard Jul 14, 2023

TheBlueMatt Jul 14, 2023

ariard Jul 14, 2023

TheBlueMatt Jul 15, 2023

ariard Jul 15, 2023

TheBlueMatt Jul 15, 2023

TheBlueMatt commented Jul 14, 2023

wpaulino commented Jul 14, 2023

ariard commented Jul 15, 2023

TheBlueMatt commented Jul 15, 2023

ariard commented Jul 15, 2023

ariard left a comment

TheBlueMatt commented Jul 17, 2023

Integrate BumpTransactionEventHandler into existing anchor tests #2403

Integrate BumpTransactionEventHandler into existing anchor tests #2403

Conversation

wpaulino commented Jul 12, 2023

codecov-commenter commented Jul 12, 2023 • edited

Codecov Report

tnull commented Jul 12, 2023

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Jul 14, 2023

wpaulino commented Jul 14, 2023

ariard commented Jul 15, 2023

TheBlueMatt commented Jul 15, 2023

ariard commented Jul 15, 2023

ariard left a comment

Choose a reason for hiding this comment

TheBlueMatt commented Jul 17, 2023

codecov-commenter commented Jul 12, 2023 •

edited