Miscellaneous anchor outputs preparatory changes #1685

wpaulino · 2022-08-25T23:18:57Z

This PR includes a few miscellaneous changes that will be required for the integration of anchors outputs. They are standalone and should be fine to merge without the remaining changes to come to help split up the upcoming work into multiple PRs.

wpaulino · 2022-08-26T00:21:45Z

Didn't see the CI failure locally as it only came up when running without grind_signatures. The responsible commit seems to be b765be4 as the test vectors don't use the zero fee anchors variant.

lightning/src/chain/channelmonitor.rs

lightning/src/util/macro_logger.rs

codecov-commenter · 2022-08-26T20:21:25Z

Codecov Report

Base: 90.86% // Head: 90.80% // Decreases project coverage by -0.05% ⚠️

Coverage data is based on head (eddcf0f) compared to base (4ae65e8).
Patch coverage: 82.03% of modified lines in pull request are covered.

❗ Current head eddcf0f differs from pull request most recent head f3a5a72. Consider uploading reports for the commit f3a5a72 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1685      +/-   ##
==========================================
- Coverage   90.86%   90.80%   -0.06%     
==========================================
  Files          86       85       -1     
  Lines       46482    46019     -463     
  Branches    46482    46019     -463     
==========================================
- Hits        42234    41789     -445     
+ Misses       4248     4230      -18

Impacted Files	Coverage Δ
lightning/src/ln/functional_test_utils.rs	`93.59% <ø> (+0.14%)`	⬆️
lightning/src/util/config.rs	`66.66% <ø> (ø)`
lightning/src/ln/channel.rs	`88.68% <70.83%> (-0.20%)`	⬇️
lightning/src/ln/chan_utils.rs	`94.55% <72.50%> (-0.95%)`	⬇️
lightning/src/util/enforcing_trait_impls.rs	`89.07% <83.33%> (-0.40%)`	⬇️
lightning/src/chain/channelmonitor.rs	`91.08% <88.23%> (-0.15%)`	⬇️
lightning/src/ln/functional_tests.rs	`96.94% <94.28%> (-0.18%)`	⬇️
lightning/src/ln/payment_tests.rs	`98.88% <100.00%> (+0.06%)`	⬆️
lightning/src/util/macro_logger.rs	`87.14% <100.00%> (+1.65%)`	⬆️
lightning/src/onion_message/packet.rs	`64.83% <0.00%> (-7.70%)`	⬇️
... and 27 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

wpaulino · 2022-08-26T21:10:42Z

CI is green now that the test vectors are updated for the zero fee HTLC transaction anchors variant. I've also upstreamed the changes to the spec at lightning/bolts#1018.

lightning/src/ln/chan_utils.rs

TheBlueMatt · 2022-08-29T19:05:05Z

Basically LGTM.

wpaulino · 2022-08-29T21:40:28Z

Pushed a new update to the HTLC trimming logic as it's only based on the dust limit for the zero fee HTLC transaction variant.

TheBlueMatt · 2022-08-30T17:13:11Z

Can you squash the fixups and I think we can land this.

lightning/src/ln/chan_utils.rs

TheBlueMatt · 2022-08-31T23:21:58Z

LGTM, can you squash again and I think this is good to go (again lol).

lightning/src/chain/channelmonitor.rs

ariard · 2022-09-01T03:43:04Z

lightning/src/ln/chan_utils.rs

@@ -616,12 +616,17 @@ pub fn build_htlc_transaction(commitment_txid: &Txid, feerate_per_kw: u32, conte
 	} else {
 		htlc_success_tx_weight(opt_anchors)
 	};
-	let total_fee = feerate_per_kw as u64 * weight / 1000;
+	let output_value = if opt_anchors {


Actually we could a) recall opt_anchors to opt_anchors_zero_fee or document the field in ChannelTransactionParameters that we're supporting only option_anchors_zero_fee_htlc_tx variant

It's now documented under ChannelTransactionParameters. I'm not opposed to renaming it but it will be a large diff. Perhaps we can leave it as a follow-up PR?

Yes, can be done in a follow-up PR.

lightning/src/ln/chan_utils.rs

lightning/src/ln/channel.rs

TheBlueMatt

Generally LGTM, feel free to squash.

lightning/src/util/macro_logger.rs

wpaulino · 2022-09-06T20:19:39Z

Squashed with no further changes.

arik-so

Still LGTM :)

TheBlueMatt · 2022-09-07T18:07:51Z

Let's hold this until 0.0.112. I'm not 100% sure how the 0fee HTLC change will impact VLS (and the CLN folks aren't responding), but given the CLN folks haven't responded I'm fine with just shipping it.

TheBlueMatt · 2022-09-13T01:52:55Z

I believe #1685 (comment) still needs addressing.

lightning/src/util/macro_logger.rs

ariard

SGTM 168e76b

Thanks for documenting f43a7e8, make it easier to detect wrong script detection.

lightning/src/ln/chan_utils.rs

wpaulino · 2022-09-13T16:34:38Z

Force pushed to address final comments:

diff --git a/lightning/src/ln/chan_utils.rs b/lightning/src/ln/chan_utils.rs
index 57496058..df037893 100644
--- a/lightning/src/ln/chan_utils.rs
+++ b/lightning/src/ln/chan_utils.rs
@@ -44,7 +44,8 @@ use util::crypto::sign;
 pub(crate) const MAX_HTLCS: u16 = 483;
 pub(crate) const OFFERED_HTLC_SCRIPT_WEIGHT: usize = 133;
 pub(crate) const OFFERED_HTLC_SCRIPT_WEIGHT_ANCHORS: usize = 136;
-// The weight range for `accepted_htlc_script` encompasses both the non-anchors and anchors variants.
+// The weight of `accepted_htlc_script` can vary in function of its CLTV argument value. We define a
+// range that encompasses both its non-anchors and anchors variants.
 pub(crate) const MIN_ACCEPTED_HTLC_SCRIPT_WEIGHT: usize = 136;
 pub(crate) const MAX_ACCEPTED_HTLC_SCRIPT_WEIGHT: usize = 143;
 
diff --git a/lightning/src/util/macro_logger.rs b/lightning/src/util/macro_logger.rs
index 45c02583..01a0d3ff 100644
--- a/lightning/src/util/macro_logger.rs
+++ b/lightning/src/util/macro_logger.rs
@@ -91,7 +91,7 @@ impl<'a> core::fmt::Display for DebugTx<'a> {
 	fn fmt(&self, f: &mut core::fmt::Formatter) -> Result<(), core::fmt::Error> {
 		if self.0.input.len() >= 1 && self.0.input.iter().any(|i| !i.witness.is_empty()) {
 			let first_input = &self.0.input[0];
-			let witness_script_len = first_input.witness.last().unwrap().len();
+			let witness_script_len = first_input.witness.last().unwrap_or(&[]).len();
 			if self.0.input.len() == 1 && witness_script_len == 71 &&
 					(first_input.sequence.0 >> 8*3) as u8 == 0x80 {
 				write!(f, "commitment tx ")?;
@@ -115,7 +115,7 @@ impl<'a> core::fmt::Display for DebugTx<'a> {
 					}
 				}
 				if num_preimage > 0 || num_timeout > 0 || num_revoked > 0 {
-					write!(f, "HTLC claim tx ({} preimage, {} timeout, {} 2 revoked)",
+					write!(f, "HTLC claim tx ({} preimage, {} timeout, {} revoked)",
 						num_preimage, num_timeout, num_revoked)?;
 				}
 			}

TheBlueMatt · 2022-09-13T17:50:14Z

Oops, ugh, needs rebase :(

There's no need to broadcast our local commitment transaction if we've already seen a confirmed one as it'll be immediately rejected as a duplicate/conflict. This will also help prevent dispatching spurious events for bumping commitment and HTLC transactions through anchor outputs (once implemented in future work) and the dispatch for said events follows the same flow as our usual commitment broadcast.

HTLC transactions from anchor channels are constrained by a CSV of 1 block, so broadcasting them along with the unconfirmed commitment tranasction will result in them being immediately rejected as premature.

This is based on the assumption that we only support the zero HTLC transaction fee variant of anchor channels.

With the zero fee HTLC transaction anchors variant, HTLCs can no longer be trimmed due to their amount being too low to have a mempool valid HTLC transaction. Now they can only be trimmed based on the dust limit of each party within the channel.

Each test featuring HTLCs had a minimum and maximum feerate case. This is no longer necessary for the zero HTLC transaction anchors variant as the commitment feerate does not impact whether HTLCs can be trimmed or not, only the dust limit does.

non-zero-fee anchor support was broken by LDK in lightningdevkit/rust-lightning#1685 and fix was later submitted by us

TheBlueMatt reviewed Aug 26, 2022

View reviewed changes

lightning/src/chain/channelmonitor.rs Outdated Show resolved Hide resolved

lightning/src/util/macro_logger.rs Outdated Show resolved Hide resolved

wpaulino force-pushed the anchors-prep branch from a63811d to f672aa3 Compare August 26, 2022 19:59

dunxen reviewed Aug 29, 2022

View reviewed changes

lightning/src/ln/chan_utils.rs Show resolved Hide resolved

TheBlueMatt added the Seeking Code Review label Aug 29, 2022

wpaulino force-pushed the anchors-prep branch from f672aa3 to b12ac73 Compare August 29, 2022 21:37

arik-so previously approved these changes Aug 30, 2022

View reviewed changes

wpaulino dismissed arik-so’s stale review via 62e952b August 30, 2022 17:19

wpaulino force-pushed the anchors-prep branch from b12ac73 to 62e952b Compare August 30, 2022 17:19

dunxen previously approved these changes Aug 30, 2022

View reviewed changes

TheBlueMatt removed the Seeking Code Review label Aug 30, 2022

TheBlueMatt reviewed Aug 30, 2022

View reviewed changes

lightning/src/ln/chan_utils.rs Outdated Show resolved Hide resolved

wpaulino dismissed dunxen’s stale review via c9f1568 August 30, 2022 23:29

wpaulino force-pushed the anchors-prep branch from 62e952b to c9f1568 Compare August 30, 2022 23:29

wpaulino mentioned this pull request Aug 31, 2022

Expose new BumpChannelClose event for channels with anchor outputs #1689

Merged

wpaulino force-pushed the anchors-prep branch from c9f1568 to 5c0e1c2 Compare August 31, 2022 23:33

ariard reviewed Sep 1, 2022

View reviewed changes

wpaulino force-pushed the anchors-prep branch from 5c0e1c2 to eddcf0f Compare September 1, 2022 18:17

TheBlueMatt reviewed Sep 2, 2022

View reviewed changes

lightning/src/util/macro_logger.rs Outdated Show resolved Hide resolved

wpaulino force-pushed the anchors-prep branch from eddcf0f to 168e76b Compare September 6, 2022 20:19

arik-so previously approved these changes Sep 7, 2022

View reviewed changes

TheBlueMatt reviewed Sep 13, 2022

View reviewed changes

lightning/src/util/macro_logger.rs Outdated Show resolved Hide resolved

ariard reviewed Sep 13, 2022

View reviewed changes

lightning/src/ln/chan_utils.rs Show resolved Hide resolved

wpaulino dismissed arik-so’s stale review via ccae963 September 13, 2022 16:32

wpaulino force-pushed the anchors-prep branch from 168e76b to ccae963 Compare September 13, 2022 16:32

TheBlueMatt previously approved these changes Sep 13, 2022

View reviewed changes

wpaulino added 7 commits September 13, 2022 10:58

Use proper sighash flag for remote HTLCs with anchor outputs

2f4a1f7

Exclude HTLC transactions from broadcast on anchor channels

5aae0ab

HTLC transactions from anchor channels are constrained by a CSV of 1 block, so broadcasting them along with the unconfirmed commitment tranasction will result in them being immediately rejected as premature.

Use zero fee HTLC transactions for anchor channels

a447965

This is based on the assumption that we only support the zero HTLC transaction fee variant of anchor channels.

Update HTLC script detection to check for anchor output variants

cd0d19c

wpaulino dismissed TheBlueMatt’s stale review via f3a5a72 September 13, 2022 17:58

wpaulino force-pushed the anchors-prep branch from ccae963 to f3a5a72 Compare September 13, 2022 17:58

TheBlueMatt approved these changes Sep 13, 2022

View reviewed changes

dunxen approved these changes Sep 13, 2022

View reviewed changes

arik-so approved these changes Sep 13, 2022

View reviewed changes

TheBlueMatt merged commit 71f4749 into lightningdevkit:main Sep 13, 2022

wpaulino deleted the anchors-prep branch September 13, 2022 21:35

devrandom mentioned this pull request Nov 2, 2022

Non-zero-fee anchors support was removed #1822

Closed

devrandom added a commit to lightning-signer/vls that referenced this pull request Mar 22, 2023

update LDK to post 0.0.112

316a966

non-zero-fee anchor support was broken by LDK in lightningdevkit/rust-lightning#1685 and fix was later submitted by us

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Miscellaneous anchor outputs preparatory changes #1685

Miscellaneous anchor outputs preparatory changes #1685

wpaulino commented Aug 25, 2022

wpaulino commented Aug 26, 2022

codecov-commenter commented Aug 26, 2022 •

edited

Loading

wpaulino commented Aug 26, 2022

TheBlueMatt commented Aug 29, 2022

wpaulino commented Aug 29, 2022

TheBlueMatt commented Aug 30, 2022

TheBlueMatt commented Aug 31, 2022

ariard Sep 1, 2022

wpaulino Sep 1, 2022

ariard Sep 13, 2022

TheBlueMatt left a comment

wpaulino commented Sep 6, 2022

arik-so left a comment

TheBlueMatt commented Sep 7, 2022

TheBlueMatt commented Sep 13, 2022

ariard left a comment

wpaulino commented Sep 13, 2022

TheBlueMatt commented Sep 13, 2022

Miscellaneous anchor outputs preparatory changes #1685

Miscellaneous anchor outputs preparatory changes #1685

Conversation

wpaulino commented Aug 25, 2022

wpaulino commented Aug 26, 2022

codecov-commenter commented Aug 26, 2022 • edited Loading

Codecov Report

wpaulino commented Aug 26, 2022

TheBlueMatt commented Aug 29, 2022

wpaulino commented Aug 29, 2022

TheBlueMatt commented Aug 30, 2022

TheBlueMatt commented Aug 31, 2022

ariard Sep 1, 2022

Choose a reason for hiding this comment

wpaulino Sep 1, 2022

Choose a reason for hiding this comment

ariard Sep 13, 2022

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

wpaulino commented Sep 6, 2022

arik-so left a comment

Choose a reason for hiding this comment

TheBlueMatt commented Sep 7, 2022

TheBlueMatt commented Sep 13, 2022

ariard left a comment

Choose a reason for hiding this comment

wpaulino commented Sep 13, 2022

TheBlueMatt commented Sep 13, 2022

codecov-commenter commented Aug 26, 2022 •

edited

Loading