Send update_fee on reconnection

[t-bast]

We update transaction fees at every block (ie every 10 minutes). While this works well when the remote peer is a node that's online for more than 10 minutes, it's an issue for mobile wallets that usually come online for a few minutes and then disconnect.

We want to make sure we send these wallet peers an update_fee when one is needed, so we now check for feerate updates on reconnection.

Fixes #1381.

[codecov-io]

Codecov Report

Merging #1383 into master will increase coverage by 0.09%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1383      +/-   ##
==========================================
+ Coverage   86.56%   86.65%   +0.09%     
==========================================
  Files         123      123              
  Lines        9398     9403       +5     
  Branches      401      404       +3     
==========================================
+ Hits         8135     8148      +13     
+ Misses       1263     1255       -8     

Impacted Files	Coverage Δ
...c/main/scala/fr/acinq/eclair/channel/Channel.scala	85.27% <100.00%> (-0.39%)	⬇️
...nq/eclair/blockchain/electrum/ElectrumClient.scala	74.19% <0.00%> (+0.35%)	⬆️
.../eclair/payment/relay/PostRestartHtlcCleaner.scala	86.75% <0.00%> (+1.32%)	⬆️
...main/scala/fr/acinq/eclair/router/Validation.scala	92.57% <0.00%> (+1.56%)	⬆️
...clair/blockchain/electrum/ElectrumClientPool.scala	82.79% <0.00%> (+4.30%)	⬆️
...cinq/eclair/blockchain/bitcoind/zmq/ZMQActor.scala	94.87% <0.00%> (+5.12%)	⬆️

[araspitzu]

The approach is correct but I'm not 100% sure we should use BroadcastChannelUpdate, isn't this event fired every REFRESH_CHANNEL_UPDATE_INTERVAL = 10 days? (Recall we want to re-send the update_fee on channel reestablish)

[t-bast]

The approach is correct but I'm not 100% sure we should use BroadcastChannelUpdate, isn't this event fired every REFRESH_CHANNEL_UPDATE_INTERVAL = 10 days? (Recall we want to re-send the update_fee on channel reestablish)

Look at the handler for ChannelReestablish, that's where we create this event. We add a delay of 10 seconds after reconnection to avoid flappyness, which is something I think we want to re-use for the update_fee: this is why I put it here. It's true that this event is more general, the alternative takes more code (we'd need to define a new event for BroadcastFeeUpdate), but if you strongly prefer separating these two I can do that.

[araspitzu]

The approach is correct but I'm not 100% sure we should use BroadcastChannelUpdate, isn't this event fired every REFRESH_CHANNEL_UPDATE_INTERVAL = 10 days? (Recall we want to re-send the update_fee on channel reestablish)

Look at the handler for ChannelReestablish, that's where we create this event. We add a delay of 10 seconds after reconnection to avoid flappyness, which is something I think we want to re-use for the update_fee: this is why I put it here. It's true that this event is more general, the alternative takes more code (we'd need to define a new event for BroadcastFeeUpdate), but if you strongly prefer separating these two I can do that.

Right, this looks correct to me and i'm fine with reusing BroadcastChannelUpdate with the nice comment you put on it. One possible issues we might have is if the mobile app user tries to send the payment before the 10s (so before we send out the update_fee).

[t-bast]

One possible issues we might have is if the mobile app user tries to send the payment before the 10s (so before we send out the update_fee).

This isn't necessarily an issue. The update_fee isn't meant to be blocking, there can be concurrent payments happening. At worst the update_fee will be deferred if the additional HTLC makes the new fee unapplicable, but once the HTLC settles it means the funder has more balance on his side so it's very likely that next time it will be able to send out the update_fee, even if more HTLCs are added to the commit tx.