Move ChannelConfig static fields to ChannelHandshakeConfig by wpaulino · Pull Request #1529 · lightningdevkit/rust-lightning

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

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

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

 	///
 	/// [`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

+/// [`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 3bbd4f2Compare 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 ec7ccf0Compare 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:mainJun 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

Uh oh!

Conversation

wpaulino commented Jun 8, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tnull left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

tnullJun 9, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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

Uh oh!

Codecov Report

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

tnullJun 10, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

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

wpaulino commented Jun 8, 2022•
edited
Loading

tnull left a comment •
edited
Loading

tnullJun 9, 2022•
edited
Loading

codecov-commenter commented Jun 9, 2022•
edited
Loading

tnullJun 10, 2022•
edited
Loading