Re-add support for non-zero-fee-anchors to chan_utils #1828

devrandom · 2022-11-03T11:15:25Z

This would let VLS continue to use LDK past 0.0.111.

I tried to minimize the diff.

devrandom · 2022-11-03T11:16:41Z

lightning/src/ln/chan_utils.rs

 	(10, built, required),
 	(12, htlcs, vec_type),
 	(14, opt_anchors, option),
+	(16, opt_non_zero_fee_anchors, option),


alternatively, I can create a const_false "TLV" directive for the macros, so that nothing is actually serialized for this field.

I think this either needs to be an odd type or an unwrap_or(false) such that we can read it on new versions that didn't previously serialize this field.

I don't think so? I think it should be even because old versions should reject it - technically this means a non-forwards-compatible change on VLS' end, but I assume that's okay, it shouldn't matter for LDK users.

I am doing self.opt_non_zero_fee_anchors.is_some() instead of self.opt_non_zero_fee_anchors.unwrap_or(false), but they are equivalent.

if it was serialized by an old version, the field won't exist, and that will be interpreted as None.

so this should work as written

codecov-commenter · 2022-11-03T14:51:15Z

Codecov Report

Base: 90.72% // Head: 92.37% // Increases project coverage by +1.65% 🎉

Coverage data is based on head (c4bb578) compared to base (8245128).
Patch coverage: 80.95% of modified lines in pull request are covered.

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1828      +/-   ##
==========================================
+ Coverage   90.72%   92.37%   +1.65%     
==========================================
  Files          91       91              
  Lines       47896    61923   +14027     
  Branches    47896    61923   +14027     
==========================================
+ Hits        43453    57204   +13751     
- Misses       4443     4719     +276

Impacted Files	Coverage Δ
lightning/src/ln/chan_utils.rs	`93.63% <63.63%> (-0.36%)`	⬇️
lightning/src/chain/channelmonitor.rs	`92.92% <100.00%> (+2.19%)`	⬆️
lightning/src/chain/keysinterface.rs	`90.76% <100.00%> (+0.01%)`	⬆️
lightning/src/ln/channel.rs	`92.12% <100.00%> (+3.29%)`	⬆️
lightning/src/util/enforcing_trait_impls.rs	`86.17% <100.00%> (ø)`
lightning/src/chain/mod.rs	`66.66% <0.00%> (-1.52%)`	⬇️
lightning-net-tokio/src/lib.rs	`76.73% <0.00%> (-0.31%)`	⬇️
lightning/src/ln/peer_channel_encryptor.rs	`93.38% <0.00%> (-0.25%)`	⬇️
lightning/src/lib.rs	`100.00% <0.00%> (ø)`
lightning/src/ln/reorg_tests.rs	`100.00% <0.00%> (ø)`
... and 30 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.

TheBlueMatt · 2022-11-03T16:13:20Z

Hmm, okay, so if I understand correctly, we're gonna have to keep code around to support this forever - CLN may well go to prod as greenlight with VLS and channels that are non-0-htlc-fee anchors, so future versions will always have to support those channels.

Do y'all use the signer directly? Can we avoid passing anything to the signer and just let the CommitmentTransaction track if we're 0-htlc-fee?

devrandom · 2022-11-03T17:43:11Z

Do y'all use the signer directly? Can we avoid passing anything to the signer and just let the CommitmentTransaction track if we're 0-htlc-fee?

Ah, right, we do use InMemorySigner too.

TheBlueMatt · 2022-11-03T17:51:09Z

Ah, okay.

Separately, it looks like the changes to CommitmentTransaction are unused - the InMemorySigner just calls build_htlc_transaction, is there a reason to have the opt_non_zero_fee_anchors in CommitmentTransaction?

devrandom · 2022-11-15T08:47:25Z

Apologies, missed this comment.

Separately, it looks like the changes to CommitmentTransaction are unused - the InMemorySigner just calls build_htlc_transaction, is there a reason to have the opt_non_zero_fee_anchors in CommitmentTransaction?

We use CommitmentTransaction, and we would like to use it with non-zero-fee anchors.

wpaulino

The logic for dust HTLCs also changed with the zero fee variant (see commit af2ff9b), do we need to bring that back as well?

wpaulino · 2022-11-15T19:39:56Z

lightning/src/chain/keysinterface.rs

 	/// Key derivation parameters
 	channel_keys_id: [u8; 32],
+	/// Whether non-zero-fee anchors are enabled (used in conjuction with channel_parameters.opt_anchors)
+	use_non_zero_fee_anchors: bool,


Let's include this within ChannelTransactionParameters instead.

I would normally do that, but since there's no plan to support this in actual LDK channel operation, it seems it would just touch additional code for not much gain?

We want to keep signers derivable, except for the parameters that need to be provided through ready_channel, so I'd prefer all of them be under the channel_parameters option.

wpaulino · 2022-11-15T19:52:34Z

lightning/src/ln/chan_utils.rs

 	(10, built, required),
 	(12, htlcs, vec_type),
 	(14, opt_anchors, option),
+	(16, opt_non_zero_fee_anchors, option),


I think this either needs to be an odd type or an unwrap_or(false) such that we can read it on new versions that didn't previously serialize this field.

TheBlueMatt · 2022-11-15T20:09:43Z

The logic for dust HTLCs also changed with the zero fee variant (see commit af2ff9b), do we need to bring that back as well?

I don't think so - we don't care about supporting on the Channel end, just in the utils that VLS uses to support CLN channels.

wpaulino · 2022-11-21T22:32:36Z

lightning/src/ln/chan_utils.rs

 	}

+	/// use non-zero fee anchors
+	pub fn with_non_zero_fee_anchors(mut self) -> Self {


I think this needs: (C-not exported) since bindings don't support move semantics
cc @TheBlueMatt

wpaulino · 2022-11-21T22:36:00Z

lightning/src/chain/keysinterface.rs

 	/// Key derivation parameters
 	channel_keys_id: [u8; 32],
+	/// Whether non-zero-fee anchors are enabled (used in conjuction with channel_parameters.opt_anchors)
+	use_non_zero_fee_anchors: bool,


We want to keep signers derivable, except for the parameters that need to be provided through ready_channel, so I'd prefer all of them be under the channel_parameters option.

devrandom requested review from TheBlueMatt and wpaulino November 3, 2022 11:15

devrandom commented Nov 3, 2022

View reviewed changes

devrandom force-pushed the 2022-11-non-zero-fee-anchors branch 2 times, most recently from 1f98f65 to b67d958 Compare November 3, 2022 14:48

wpaulino reviewed Nov 15, 2022

View reviewed changes

devrandom changed the title ~~Draft: Re-add support for non-zero-fee-anchors to chan_utils~~ Re-add support for non-zero-fee-anchors to chan_utils Nov 21, 2022

TheBlueMatt assigned TheBlueMatt and wpaulino Nov 21, 2022

wpaulino reviewed Nov 21, 2022

View reviewed changes

devrandom force-pushed the 2022-11-non-zero-fee-anchors branch 2 times, most recently from c4bb578 to ef2914d Compare November 22, 2022 11:20

Re-add support for non-zero-fee-anchors to chan_utils and InMemorySigner

e6b9694

devrandom force-pushed the 2022-11-non-zero-fee-anchors branch from ef2914d to e6b9694 Compare November 22, 2022 11:28

TheBlueMatt added this to the 0.0.113 milestone Nov 22, 2022

TheBlueMatt approved these changes Nov 22, 2022

View reviewed changes

wpaulino approved these changes Nov 22, 2022

View reviewed changes

TheBlueMatt merged commit af7c292 into lightningdevkit:main Nov 22, 2022

Re-add support for non-zero-fee-anchors to chan_utils #1828

Re-add support for non-zero-fee-anchors to chan_utils #1828

Uh oh!

Conversation

devrandom commented Nov 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

codecov-commenter commented Nov 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

TheBlueMatt commented Nov 3, 2022

Uh oh!

devrandom commented Nov 3, 2022

Uh oh!

TheBlueMatt commented Nov 3, 2022

Uh oh!

devrandom commented Nov 15, 2022

Uh oh!

wpaulino left a comment

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!

TheBlueMatt commented Nov 15, 2022

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

devrandom commented Nov 3, 2022 •

edited

Loading

codecov-commenter commented Nov 3, 2022 •

edited

Loading