Move ChannelConfig static fields to ChannelHandshakeConfig #1529

wpaulino · 2022-06-08T22:27:09Z

In the near future, we plan to allow users to update their ChannelConfig after the initial channel handshake. In order to reuse the same struct and expose it to users, we opt to move out all static fields that cannot be updated after the initial channel handshake.

Motivated by #1527 (comment).

I also tested with ldk-sample to make sure upgrading v0.0.106 -> v0.0.107 and downgrading v0.0.107 -> v0.0.106 are allowed and the intended state is reflected properly.

tnull

Thanks! Just left some comments and questions.

Moreover: Not sure if its only me, but I find the current naming of variables in channel.rs vs. their type unnecessarily confusing: there are many instances of variables simply named config, which may refer to ChannelConfig, UserConfig, etc. depending on the context. Then there is the channel_options field and corresponding variables, which is however a ChannelConfig. Moreover, there are fields like own_channel_config, which would make you think it would be a ChannelConfig, but no, it's actually a ChannelHandshakeConfig.

TLDR: Not sure if it needs to happen in this PR, but I personally would appreciate a refactoring that brings the naming of variables more in alignment with their types to improve readability.

tnull · 2022-06-09T07:08:37Z

lightning/src/util/config.rs

-	///
-	/// Default value: false.
-	pub announced_channel: bool,
+	/// Deprecated, use [`ChannelHandshakeConfig::announced_channel`] instead.


Hm, if you deprecate this here, could you update the rest of the codebase to directly use the new location?

Also, instead of a comment, you probably want to mark this as deprecated:

#[deprecated( since = "0.0.107", note = "ChannelHandshakeConfig::announced_channel should be used instead.", )]

(Yes, if you cannot update all instances where it is used immediately, this will produce annoying warning messages, which is however exactly the idea.)

Hmm, I'd rather not have a ton of build warnings for a while, that's generally an easy way to build a culture of ignoring build warnings, which is pretty bad. Using the #[deprecated] tag is important for public API stuff, but for internal stuff it's hopefully a bit less, though I agree still useful.

In any case, I think we can do better here. Instead of making these fields pub(crate), because we don't really care about users being able to serialize ChannelConfig objects, I think, we can move the serialization to some struct LegacyChannelConfig { config: ChannelConfig, announced_channel: bool, ... } object instead. That way, we can read it all the same, but the public interface remains clean and you can construct a full ChannelConfig without using default() and then modifying. Its a good bit more work cause we can't use the impl_writeable_tlv_based!() macro anymore and have to write out the deserialization manually, but it should be doable.

Hmm, I'd rather not have a ton of build warnings for a while, that's generally an easy way to build a culture of ignoring build warnings

Well, this is probably true if there is some baseline acceptance to warnings overall. I'd however argue that feeling the pain of 'deprecated' warnings on a regular basis can actually ensure the parts in question are not forgotten and get actually removed ASAP.

Yea, I don't disagree, just noting that in this case, because its a serialization backwards-compat thing, it'll stick around at least a year or two, so it'd kinda suck to have those warnings for that long :/

Mh, I agree that in that timeframe it may be come a nuisance without much benefit. Maybe we could create something like a TODO.md in which we keep track of TODOs that cannot be completed right away and where we define until when they are deferred? Or immediately create issues for them with a 'defer' tag and assign an estimated milestone some time in the future?

In any case, I think we can do better here. Instead of making these fields pub(crate), because we don't really care about users being able to serialize ChannelConfig objects, I think, we can move the serialization to some struct LegacyChannelConfig { config: ChannelConfig, announced_channel: bool, ... } object instead. That way, we can read it all the same, but the public interface remains clean and you can construct a full ChannelConfig without using default() and then modifying. Its a good bit more work cause we can't use the impl_writeable_tlv_based!() macro anymore and have to write out the deserialization manually, but it should be doable.

Sounds good, I assume this means we can reset the serialization schema for the new ChannelConfig then? Otherwise, we would want to document that the types 4 (announced_channel) & 6 (commit_upfront_shutdown_key) cannot be reused as it would break LegacyChannelConfig serialization.

Maybe we could create something like a TODO.md in which we keep track of TODOs that cannot be completed right away and where we define until when they are deferred? Or immediately create issues for them with a 'defer' tag and assign an estimated milestone some time in the future?

👍 I would prefer the latter.

Sounds good, I assume this means we can reset the serialization schema for the new ChannelConfig then?

Yea, actually I think just drop serialization support for ChannelConfig entirely, only bother ever serializing the superset.

Force pushed with the new changes since a good bit of the diff changed, PTAL. We now have a LegacyChannelConfig which we still use within Channel, but the actual values for announced_channel and commit_upfront_shutdown_key are set through ChannelHandshakeConfig.

lightning/src/util/config.rs

lightning/src/ln/channel.rs

codecov-commenter · 2022-06-09T21:11:13Z

Codecov Report

Merging #1529 (ec7ccf0) into main (a551a62) will decrease coverage by 0.02%.
The diff coverage is 93.68%.

@@            Coverage Diff             @@
##             main    #1529      +/-   ##
==========================================
- Coverage   90.94%   90.91%   -0.03%     
==========================================
  Files          80       80              
  Lines       43426    43509      +83     
  Branches    43426    43509      +83     
==========================================
+ Hits        39492    39557      +65     
- Misses       3934     3952      +18

Impacted Files	Coverage Δ
lightning/src/util/config.rs	`61.62% <84.61%> (+18.49%)`	⬆️
lightning-invoice/src/utils.rs	`96.63% <100.00%> (ø)`
lightning/src/ln/chanmon_update_fail_tests.rs	`97.71% <100.00%> (ø)`
lightning/src/ln/channel.rs	`88.58% <100.00%> (-0.02%)`	⬇️
lightning/src/ln/functional_test_utils.rs	`95.23% <100.00%> (-0.19%)`	⬇️
lightning/src/ln/functional_tests.rs	`96.97% <100.00%> (-0.10%)`	⬇️
lightning/src/ln/onion_route_tests.rs	`97.51% <100.00%> (ø)`
lightning/src/ln/payment_tests.rs	`99.24% <100.00%> (-0.01%)`	⬇️
lightning/src/ln/priv_short_conf_tests.rs	`96.60% <100.00%> (ø)`
lightning/src/ln/shutdown_tests.rs	`96.51% <100.00%> (ø)`
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a551a62...ec7ccf0. Read the comment docs.

In the near future, we plan to allow users to update their `ChannelConfig` after the initial channel handshake. In order to reuse the same struct and expose it to users, we opt to move out all static fields that cannot be updated after the initial channel handshake.

As like the previous commit, `commit_upfront_shutdown_pubkey` is another static field that cannot change after the initial channel handshake. We therefore move it out from its existing place in `ChannelConfig`.

ChannelConfig now has its static fields removed. We introduce a new LegacyChannelConfig struct that maintains the serialization as previously defined by ChannelConfig to remain backwards compatible with clients running 0.0.107 and earlier.

tnull

LGTM, just two remarks, feel free to ignore.

tnull · 2022-06-10T11:21:04Z

lightning/src/util/config.rs

@@ -265,30 +291,6 @@ pub struct ChannelConfig {
 	///
 	/// [`MIN_CLTV_EXPIRY_DELTA`]: crate::ln::channelmanager::MIN_CLTV_EXPIRY_DELTA
 	pub cltv_expiry_delta: u16,
-	/// Set to announce the channel publicly and notify all nodes that they can route via this


nit: If you split up the commits, you may want to move the removal of the moved parts to the 'move' commits. (Nevermind if you end up squashing it all.)

tnull · 2022-06-10T11:24:46Z

lightning/src/util/config.rs

+/// [`ChannelHandshakeConfig::commit_upfront_shutdown_pubkey`] fields.
+#[derive(Copy, Clone, Debug)]
+pub(crate) struct LegacyChannelConfig {
+	pub(crate) mutable: ChannelConfig,


Not too sure about this field name. Is mutability its core characteristic/function?

Yea, this is pretty awkward. Lets land this to keep things moving but I'd appreciate a followup to rename this. While we're at it, we can probably rename a few more things here - eg UserConfig::own_channel_config makes no sense, its really the handshake config.

wpaulino mentioned this pull request Jun 8, 2022

Expose API to update a channel's ChannelConfig #1527

Merged

wpaulino force-pushed the move-channel-config-static-fields branch from 9c277b7 to 3bbd4f2 Compare June 8, 2022 22:42

tnull reviewed Jun 9, 2022

View reviewed changes

wpaulino added 3 commits June 9, 2022 16:11

Move commit_upfront_shutdown_pubkey to ChannelHandshakeConfig

8027c2f

As like the previous commit, `commit_upfront_shutdown_pubkey` is another static field that cannot change after the initial channel handshake. We therefore move it out from its existing place in `ChannelConfig`.

wpaulino force-pushed the move-channel-config-static-fields branch from 5ea51d2 to ec7ccf0 Compare June 9, 2022 23:32

tnull approved these changes Jun 10, 2022

View reviewed changes

tnull mentioned this pull request Jun 13, 2022

Make htlc_maximum_msat a required field. #1519

Merged

TheBlueMatt approved these changes Jun 13, 2022

View reviewed changes

TheBlueMatt merged commit 5421e1a into lightningdevkit:main Jun 13, 2022

This was referenced Jun 13, 2022

Add commit_upfront_shutdown_pubkey to ChannelHandshakeConfig #1270

Closed

Move forwarding_fee_proportional_millionths, forwarding_fee_base_msat to ChannelHandshakeConfig #1247

Closed

wpaulino deleted the move-channel-config-static-fields branch June 13, 2022 17:42

wpaulino mentioned this pull request Jun 13, 2022

Rename UserConfig and LegacyChannelConfig fields #1540

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move ChannelConfig static fields to ChannelHandshakeConfig #1529

Move ChannelConfig static fields to ChannelHandshakeConfig #1529

wpaulino commented Jun 8, 2022 •

edited

Loading

tnull left a comment •

edited

Loading

tnull Jun 9, 2022

TheBlueMatt Jun 9, 2022

tnull Jun 9, 2022 •

edited

Loading

TheBlueMatt Jun 9, 2022

tnull Jun 9, 2022

wpaulino Jun 9, 2022

TheBlueMatt Jun 9, 2022

wpaulino Jun 9, 2022

codecov-commenter commented Jun 9, 2022 •

edited

Loading

tnull left a comment

tnull Jun 10, 2022 •

edited

Loading

tnull Jun 10, 2022

TheBlueMatt Jun 13, 2022

Move ChannelConfig static fields to ChannelHandshakeConfig #1529

Move ChannelConfig static fields to ChannelHandshakeConfig #1529

Conversation

wpaulino commented Jun 8, 2022 • edited Loading

tnull left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull Jun 9, 2022 • edited Loading

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

codecov-commenter commented Jun 9, 2022 • edited Loading

Codecov Report

tnull left a comment

Choose a reason for hiding this comment

tnull Jun 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wpaulino commented Jun 8, 2022 •

edited

Loading

tnull left a comment •

edited

Loading

tnull Jun 9, 2022 •

edited

Loading

codecov-commenter commented Jun 9, 2022 •

edited

Loading

tnull Jun 10, 2022 •

edited

Loading