Expose API to update a channel's ChannelConfig #1527

wpaulino · 2022-06-07T22:51:26Z

A new update_channel_config method is exposed on the ChannelManger to update the ChannelConfig for a set of channels atomically. New ChannelUpdate events are generated for each eligible channel.

Note that as currently implemented, a buggy and/or auto-policy-management client could spam the network with updates as there is no rate-limiting in place. This could already be done with broadcast_node_announcement, though users are less inclined to update that as frequently as its data is mostly static.

Fixes #216.

lightning/src/ln/onion_route_tests.rs

lightning/src/util/config.rs

lightning/src/ln/channelmanager.rs

TheBlueMatt · 2022-06-14T20:45:56Z

Needs rebase after #1529 landed.

codecov-commenter · 2022-06-14T23:43:31Z

Codecov Report

Merging #1527 (fae37b9) into main (8fce6a1) will increase coverage by 0.93%.
The diff coverage is 98.00%.

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

@@            Coverage Diff             @@
##             main    #1527      +/-   ##
==========================================
+ Coverage   90.92%   91.85%   +0.93%     
==========================================
  Files          80       80              
  Lines       43533    47946    +4413     
  Branches    43533    47946    +4413     
==========================================
+ Hits        39582    44041    +4459     
+ Misses       3951     3905      -46

Impacted Files	Coverage Δ
lightning/src/ln/functional_test_utils.rs	`95.23% <ø> (ø)`
lightning/src/ln/channelmanager.rs	`88.08% <94.91%> (+3.74%)`	⬆️
lightning/src/ln/onion_route_tests.rs	`97.68% <98.52%> (+0.16%)`	⬆️
lightning/src/ln/channel.rs	`88.72% <100.00%> (+0.13%)`	⬆️
lightning/src/ln/payment_tests.rs	`99.25% <100.00%> (+<0.01%)`	⬆️
lightning/src/routing/router.rs	`92.57% <100.00%> (+<0.01%)`	⬆️
lightning/src/util/config.rs	`67.44% <100.00%> (+5.81%)`	⬆️
lightning-net-tokio/src/lib.rs	`77.16% <0.00%> (+0.30%)`	⬆️
lightning/src/ln/functional_tests.rs	`98.46% <0.00%> (+1.49%)`	⬆️
lightning/src/routing/gossip.rs	`94.38% <0.00%> (+2.69%)`	⬆️
... and 2 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 8fce6a1...0f30d76. Read the comment docs.

lightning/src/ln/channelmanager.rs

TheBlueMatt · 2022-06-15T16:44:27Z

Its probably worth supporting forwarding with the previous config for a minute or so after the update, no need to bother serializing that part out, just keep an Optional previous config in memory.

wpaulino · 2022-06-16T00:10:13Z

Its probably worth supporting forwarding with the previous config for a minute or so after the update, no need to bother serializing that part out, just keep an Optional previous config in memory.

Pushed a new version implementing this.

tnull

Looks good! Few nits, two questions.

lightning/src/ln/channel.rs

tnull · 2022-06-16T07:22:29Z

lightning/src/ln/channelmanager.rs

+						// We attempt to forward the HTLC with the current policy, or the previous
+						// for a short period of time.
+						if let Some(prev_config) = chan.get_prev_config() {
+							if chan.get_update_time_counter() > prev_config.1 + 1 {


Mh, I see that using the update time counter is the most straight forward here, but it's probably not that well correlated with real time? Since the propagation delay of channel updates are in real time, I wonder if the grace period should be based on that rather than the counter, e.g., by clearing the prev config when a (time-delayed) event fires?

+1, relatedly, lets try to keep this kinda logic in channel.rs and have as little channel logic in channelmanager.rs as possible. This may also be an opportunity to move the "what is the fee I'm currently applying for a payment of value X" logic into channel.rs and also just ask for the current cltv-delta from the Channel vs doing the previous-update logic here.

Mh, I see that using the update time counter is the most straight forward here, but it's probably not that well correlated with real time? Since the propagation delay of channel updates are in real time, I wonder if the grace period should be based on that rather than the counter, e.g., by clearing the prev config when a (time-delayed) event fires?

The expiration of the previous config now relies on ChannelManager::timer_tick_occured. See the rationale behind how many ticks need to happen within the commit, but TLDR, it's based on the average convergence delay of updates across the network as seen in https://arxiv.org/pdf/2205.12737.pdf. We could be a bit more forgiving and bump it so that we can account for 95% of all nodes or even 100%.

This may also be an opportunity to move the "what is the fee I'm currently applying for a payment of value X" logic into channel.rs and also just ask for the current cltv-delta from the Channel vs doing the previous-update logic here.

Slightly refactored to help achieve this. I would've liked to just expose a single fee and CLTV delta from Channel, but I found it a bit awkward now that we have two possible values.

Note: As of yesterday's meeting, the spec seems to lean towards a 10 minute grace period.

lightning/src/ln/channelmanager.rs

tnull

Looks good, mod two comments and the failing benchmark. Nice solution using timer_tick_occured!

I also wonder if we need to be opinionated here and enforce a certain rate limiting on channel updates, or if we should allow the user to spam BroadcastChannelUpdate via update_channel_config().

lightning/src/ln/channel.rs

lightning/src/ln/channelmanager.rs

wpaulino · 2022-06-17T18:13:05Z

I also wonder if we need to be opinionated here and enforce a certain rate limiting on channel updates, or if we should allow the user to spam BroadcastChannelUpdate via update_channel_config().

Could be worthy of a follow-up. We'd likely want to implement a block-based rate limit as it seems we'll be moving forward with that anyway in a future gossip upgrade.

tnull

LGTM, only one small nit.

lightning/src/ln/channel.rs

TheBlueMatt

Basically looks good. Haven't reviewed the test changes in detail yet.

TheBlueMatt · 2022-06-18T15:04:11Z

lightning/src/util/config.rs

@@ -345,6 +345,14 @@ impl Default for ChannelConfig {
 	}
 }

+impl_writeable_tlv_based!(ChannelConfig, {


Ugh, yuck, so now between 108 and 109 we'll keep the ChannelConfig serialization but it'll be different but silent. Can we move the 8 on force_close_avoidance_max_fee_satoshis to 10 (and make all the fields required, not default_value) so that we'll explicitly fail to read a ChannelConfig (and old versions will fail to read new ones)?

Yeah I wasn't happy about this either, thanks for the pointer. I dug into this a bit deeper and noticed that ChannelDetails is serialized within PhantomRouteHints, which impls the writeable macro but doesn't seem to be used anywhere through the codebase. I assume this is intended as noted in #1294? Otherwise it would've been nice to remove all of this serialization. Another approach would be to reduce ChannelDetails to a new struct only containing the fields necessary for phantom invoices.

Yea, we expect users to de/serialize ChannelDetails objects, so I think we need this, but we definitely dont expect users to serialize ChannelConfig objects manually. Maybe we should have some non-public internal serialization trait, I dunno, but for now this seems fine and we can mention it in the release notes.

lightning/src/ln/channelmanager.rs

TheBlueMatt · 2022-06-18T15:16:10Z

lightning/src/ln/channelmanager.rs


-					if (msg.cltv_expiry as u64) < (*outgoing_cltv_value) as u64 + forwardee_cltv_expiry_delta as u64 { // incorrect_cltv_expiry


We should keep checking this for phantom forwards, which is why it was outside the if let Some(..) = forwarding_id_opt block. We can always just re-add this check using MIN_CLTV_EXPIRY_DELTA explicitly.

Added. Any reason to not check the fee here as well? We could use the default config and call the new htlc_satisfies_config method.

tnull

LGTM

TheBlueMatt · 2022-06-20T19:50:00Z

Feel free to squash.

As we prepare to expose an API to update a channel's ChannelConfig, we'll also want to expose this struct to consumers such that they have insights into the current ChannelConfig applied for each channel.

A new `update_channel_config` method is exposed on the `ChannelManger` to update the `ChannelConfig` for a set of channels atomically. New `ChannelUpdate` events are generated for each eligible channel. Note that as currently implemented, a buggy and/or auto-policy-management client could spam the network with updates as there is no rate-limiting in place. This could already be done with `broadcast_node_announcement`, though users are less inclined to update that as frequently as its data is mostly static.

We do this to prevent payment failures while the `ChannelUpdate` for the new `ChannelConfig` still propagates throughout the network. In a follow up commit, we'll honor forwarding HTLCs that were constructed based on either the previous or current `ChannelConfig`. To handle expiration (when we should stop allowing the previous config), we rely on the ChannelManager's `timer_tick_occurred` method. After enough ticks, the previous config is cleared from memory, and only the current config applies moving forward.

This is mostly motivated by the fact that payments may happen while the latest `ChannelUpdate` indicating our new `ChannelConfig` is still propagating throughout the network. By temporarily allowing the previous config, we can help reduce payment failures across the network.

wpaulino force-pushed the update-htlc-relay-policy branch from 206a98b to 7394b93 Compare June 7, 2022 23:17

tnull reviewed Jun 8, 2022

View reviewed changes

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

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

TheBlueMatt reviewed Jun 8, 2022

View reviewed changes

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

wpaulino mentioned this pull request Jun 8, 2022

Move ChannelConfig static fields to ChannelHandshakeConfig #1529

Merged

ViktorTigerstrom reviewed Jun 9, 2022

View reviewed changes

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

TheBlueMatt mentioned this pull request Jun 9, 2022

Store channels per-peer #1507

Merged

TheBlueMatt added blocked on dependent pr and removed blocked on dependent pr labels Jun 9, 2022

wpaulino force-pushed the update-htlc-relay-policy branch from 7394b93 to cadf33a Compare June 14, 2022 23:43

wpaulino changed the title ~~Expose method to update HTLC relay policy~~ Expose API to update a channel's ChannelConfig Jun 14, 2022

tnull reviewed Jun 15, 2022

View reviewed changes

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

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

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

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

wpaulino force-pushed the update-htlc-relay-policy branch from cadf33a to 4d0bb1b Compare June 16, 2022 00:09

tnull reviewed Jun 16, 2022

View reviewed changes

wpaulino force-pushed the update-htlc-relay-policy branch from 4d0bb1b to 00ebbbb Compare June 16, 2022 23:54

tnull reviewed Jun 17, 2022

View reviewed changes

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

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

wpaulino force-pushed the update-htlc-relay-policy branch from 00ebbbb to fae37b9 Compare June 17, 2022 18:12

tnull reviewed Jun 18, 2022

View reviewed changes

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

TheBlueMatt reviewed Jun 18, 2022

View reviewed changes

wpaulino force-pushed the update-htlc-relay-policy branch 2 times, most recently from 6943c8c to a70d922 Compare June 18, 2022 19:13

tnull previously approved these changes Jun 20, 2022

View reviewed changes

wpaulino added 3 commits June 20, 2022 13:12

Expose ChannelConfig within ChannelDetails

dfd5679

As we prepare to expose an API to update a channel's ChannelConfig, we'll also want to expose this struct to consumers such that they have insights into the current ChannelConfig applied for each channel.

wpaulino added 2 commits June 20, 2022 13:12

Use new Channel::update_config method to update base fee in test

0f30d76

wpaulino dismissed tnull’s stale review via 0f30d76 June 20, 2022 20:20

wpaulino force-pushed the update-htlc-relay-policy branch from a70d922 to 0f30d76 Compare June 20, 2022 20:20

TheBlueMatt assigned tnull and TheBlueMatt Jun 21, 2022

tnull approved these changes Jun 21, 2022

View reviewed changes

TheBlueMatt approved these changes Jun 21, 2022

View reviewed changes

TheBlueMatt merged commit 90541c2 into lightningdevkit:main Jun 21, 2022

wpaulino deleted the update-htlc-relay-policy branch June 21, 2022 16:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose API to update a channel's ChannelConfig #1527

Expose API to update a channel's ChannelConfig #1527

wpaulino commented Jun 7, 2022 •

edited

Loading

TheBlueMatt commented Jun 14, 2022

codecov-commenter commented Jun 14, 2022 •

edited

Loading

TheBlueMatt commented Jun 15, 2022

wpaulino commented Jun 16, 2022

tnull left a comment •

edited

Loading

tnull Jun 16, 2022

TheBlueMatt Jun 16, 2022

wpaulino Jun 16, 2022 •

edited

Loading

tnull Jun 21, 2022

tnull left a comment

wpaulino commented Jun 17, 2022

tnull left a comment

TheBlueMatt left a comment

TheBlueMatt Jun 18, 2022

wpaulino Jun 18, 2022

TheBlueMatt Jun 18, 2022

TheBlueMatt Jun 18, 2022

wpaulino Jun 18, 2022

tnull left a comment

TheBlueMatt commented Jun 20, 2022


		if (msg.cltv_expiry as u64) < (*outgoing_cltv_value) as u64 + forwardee_cltv_expiry_delta as u64 { // incorrect_cltv_expiry

Expose API to update a channel's ChannelConfig #1527

Expose API to update a channel's ChannelConfig #1527

Conversation

wpaulino commented Jun 7, 2022 • edited Loading

TheBlueMatt commented Jun 14, 2022

codecov-commenter commented Jun 14, 2022 • edited Loading

Codecov Report

TheBlueMatt commented Jun 15, 2022

wpaulino commented Jun 16, 2022

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

wpaulino Jun 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull left a comment

Choose a reason for hiding this comment

wpaulino commented Jun 17, 2022

tnull left a comment

Choose a reason for hiding this comment

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

tnull left a comment

Choose a reason for hiding this comment

TheBlueMatt commented Jun 20, 2022

wpaulino commented Jun 7, 2022 •

edited

Loading

codecov-commenter commented Jun 14, 2022 •

edited

Loading

tnull left a comment •

edited

Loading

wpaulino Jun 16, 2022 •

edited

Loading