Skip to content

Conversation

@pm47
Copy link
Member

@pm47 pm47 commented Sep 2, 2021

This is very similar to #1918 (ef31de1), but we use the internal
actor state instead of the ChannelData.

Pros:

  • conceptually simple, low risk of regression
  • ChannelData stays untouched

Cons:

  • now there is a var in the channel

Those are the minimal changes to have the simplest diff, but we can
include some improvements made in ef31de1.

This is very similar to #1918 (ef31de1), but we use the internal
actor state instead of the `ChannelData`.

Pros:
- conceptually simple, low risk of regression
- `ChannelData` stays untouched
Cons:
- now there is a `var` in the channel

Those are the minimal changes to have the simplest diff, but we can
include some improvements made in ef31de1.
@pm47 pm47 requested review from t-bast and thomash-acinq September 2, 2021 09:29
@codecov-commenter
Copy link

Codecov Report

Merging #1934 (6a1fc84) into master (54fa208) will increase coverage by 0.07%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1934      +/-   ##
==========================================
+ Coverage   87.53%   87.61%   +0.07%     
==========================================
  Files         158      158              
  Lines       12231    12268      +37     
  Branches      505      520      +15     
==========================================
+ Hits        10707    10749      +42     
+ Misses       1524     1519       -5     
Impacted Files Coverage Δ
...c/main/scala/fr/acinq/eclair/channel/Channel.scala 88.20% <100.00%> (+2.06%) ⬆️
...main/scala/fr/acinq/eclair/db/DbEventHandler.scala 92.53% <100.00%> (-0.43%) ⬇️
...n/scala/fr/acinq/eclair/router/Announcements.scala 100.00% <100.00%> (ø)
...cinq/eclair/blockchain/watchdogs/ExplorerApi.scala 44.89% <0.00%> (-9.39%) ⬇️
...cala/fr/acinq/eclair/payment/relay/NodeRelay.scala 96.06% <0.00%> (-1.58%) ⬇️
...main/scala/fr/acinq/eclair/router/Validation.scala 90.76% <0.00%> (-1.54%) ⬇️
.../acinq/eclair/blockchain/bitcoind/ZmqWatcher.scala 90.00% <0.00%> (ø)
...re/src/main/scala/fr/acinq/eclair/NodeParams.scala 93.26% <0.00%> (+0.03%) ⬆️
...n/scala/fr/acinq/eclair/tor/Socks5Connection.scala 8.66% <0.00%> (+5.32%) ⬆️
... and 1 more

@pm47
Copy link
Member Author

pm47 commented Sep 2, 2021

And.... still doesn't work 😒. For the same reasons as #1922 (comment).

If we send multiple CMD_UPDATE_RELAY_FEE in OFFLINE, the previousChannelUpdate_opt will be overwritten.

The root cause of all our problems is that we should not reuse LocalChannelUpdate. The semantics of LocalChannelUpdate revolves around channel availability (connections/disconnections) and trying to minimize events in order to save bandwidth, by deferring some events. It's very different from tracking changes in parameters.

@pm47 pm47 closed this Sep 2, 2021
@pm47 pm47 deleted the keep-previous-channel-update branch November 4, 2021 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants