mirror of
https://github.com/ACINQ/eclair.git
synced 2025-02-21 22:11:46 +01:00
Better logic for sending channel_updates (#888)
* don't spam with channel_updates at startup Previous logic was very simple but naive: - every time a channel_update changed we would send it out - we would always make a new channel_update with the disabled flag set at startup. In case our node was simply restarted, this resulted in us re-sending a channel_update with the disabled flag set, then a second one with the disabled flag unset a few seconds later, for each public channel. On top of that, this opened way to a bug: if reconnection is very fast, then the two successive channel_update will have the same timestamp, causing the router to not send the second one, which means that the channel would be considered disabled by the network, and excluded from payments. The new logic is as follows: - when we do NORMAL->NORMAL or NORMAL->OFFLINE or OFFLINE->NORMAL, we send out the new channel_update if it has changed - in all other case (e.g. WAIT_FOR_INIT_INTERNAL->OFFLINE) we do nothing As a side effect, if we were connected to a peer, then we shut down eclair, then the peer goes down, then we restart eclair: we will make a new channel_update with the disabled flag set but we won't broadcast it. If someone tries to make a payment to that node, we will return the new channel_update with disabled flag set (and maybe the payer will then broadcast that channel_update). So even in that corner case we are good. * quick reconnection: bump channel_update timestamp In case of a disconnection-reconnection, we first generate a channel_update with disabled bit set, then after we reconnect we generate a second channel_update with disabled bit not set. If this happens very quickly, then both channel_updates will have the same timestamp, and the second one will get ignored by the network. A simple fix is to bump the second timestamp in this case. * set channel_update refresh timer at reconnection We only care about this timer when connected anyway. We also cancel it when disconnecting. This has several advantages: - having a static task resulted in unnecessary refresh if the channel got disconnected/reconnected in between 2 weeks - better repartition of the channel_update refresh over time because at startup all channels were generated at the same time causing all refresh tasks to be synchronized - less overhead for the scheduler (because we cancel refresh task for offline channels (minor, but still)
This commit is contained in:
Pierre-Marie Padiou
GitHub
parent
5519d0aa6a
commit
cc3395a5bb
3 changed files with 59 additions and 12 deletions
|
|
@ -97,8 +97,6 @@ class Channel(val nodeParams: NodeParams, wallet: EclairWallet, remoteNodeId: Pu
|
|||
context.system.eventStream.subscribe(self, classOf[CurrentBlockCount])
|
||||
// this will be used to make sure the current commitment fee is up-to-date
|
||||
context.system.eventStream.subscribe(self, classOf[CurrentFeerates])
|
||||
// we need to periodically re-send channel updates, otherwise channel will be considered stale and get pruned by network
|
||||
setTimer(TickRefreshChannelUpdate.toString, TickRefreshChannelUpdate, timeout = REFRESH_CHANNEL_UPDATE_INTERVAL, repeat = true)
|
||||
|
||||
/*
|
||||
8888888 888b 888 8888888 88888888888
|
||||
|
|
@ -201,9 +199,7 @@ class Channel(val nodeParams: NodeParams, wallet: EclairWallet, remoteNodeId: Pu
|
|||
blockchain ! WatchSpent(self, data.commitments.commitInput.outPoint.txid, data.commitments.commitInput.outPoint.index.toInt, data.commitments.commitInput.txOut.publicKeyScript, BITCOIN_FUNDING_SPENT)
|
||||
blockchain ! WatchLost(self, data.commitments.commitInput.outPoint.txid, nodeParams.minDepthBlocks, BITCOIN_FUNDING_LOST)
|
||||
context.system.eventStream.publish(ShortChannelIdAssigned(self, normal.channelId, normal.channelUpdate.shortChannelId))
|
||||
// we rebuild a channel_update for two reasons:
|
||||
// - we want to reload values from configuration
|
||||
// - if eclair was previously killed, it might not have had time to publish a channel_update with enable=false
|
||||
// we rebuild a new channel_update with values from the configuration because they may have changed while eclair was down
|
||||
val channelUpdate = Announcements.makeChannelUpdate(nodeParams.chainHash, nodeParams.privateKey, remoteNodeId, normal.channelUpdate.shortChannelId, nodeParams.expiryDeltaBlocks,
|
||||
normal.commitments.remoteParams.htlcMinimumMsat, normal.channelUpdate.feeBaseMsat, normal.channelUpdate.feeProportionalMillionths, normal.commitments.localCommit.spec.totalFunds, enable = false)
|
||||
|
||||
|
|
@ -879,6 +875,8 @@ class Channel(val nodeParams: NodeParams, wallet: EclairWallet, remoteNodeId: Pu
|
|||
d.commitments.localChanges.proposed.collect {
|
||||
case add: UpdateAddHtlc => relayer ! Status.Failure(AddHtlcFailed(d.channelId, add.paymentHash, ChannelUnavailable(d.channelId), d.commitments.originChannels(add.id), Some(channelUpdate), None))
|
||||
}
|
||||
// disable the channel_update refresh timer
|
||||
cancelTimer(TickRefreshChannelUpdate.toString)
|
||||
goto(OFFLINE) using d.copy(channelUpdate = channelUpdate)
|
||||
|
||||
case Event(e: Error, d: DATA_NORMAL) => handleRemoteError(e, d)
|
||||
|
|
@ -1453,7 +1451,13 @@ class Channel(val nodeParams: NodeParams, wallet: EclairWallet, remoteNodeId: Pu
|
|||
}
|
||||
}
|
||||
// re-enable the channel
|
||||
val channelUpdate = Announcements.makeChannelUpdate(nodeParams.chainHash, nodeParams.privateKey, remoteNodeId, d.shortChannelId, nodeParams.expiryDeltaBlocks, d.commitments.remoteParams.htlcMinimumMsat, d.channelUpdate.feeBaseMsat, d.channelUpdate.feeProportionalMillionths, d.commitments.localCommit.spec.totalFunds, enable = Helpers.aboveReserve(d.commitments))
|
||||
val timestamp = Platform.currentTime / 1000 match {
|
||||
case ts if ts == d.channelUpdate.timestamp => ts + 1 // corner case: in case of quick reconnection, we bump the timestamp of the new channel_update, otherwise it will get ignored by the network
|
||||
case ts => ts
|
||||
}
|
||||
val channelUpdate = Announcements.makeChannelUpdate(nodeParams.chainHash, nodeParams.privateKey, remoteNodeId, d.shortChannelId, nodeParams.expiryDeltaBlocks, d.commitments.remoteParams.htlcMinimumMsat, d.channelUpdate.feeBaseMsat, d.channelUpdate.feeProportionalMillionths, d.commitments.localCommit.spec.totalFunds, enable = Helpers.aboveReserve(d.commitments), timestamp = timestamp)
|
||||
// we will refresh need to periodically re-send channel updates, otherwise channel will be considered stale and get pruned by network
|
||||
setTimer(TickRefreshChannelUpdate.toString, TickRefreshChannelUpdate, timeout = REFRESH_CHANNEL_UPDATE_INTERVAL, repeat = true)
|
||||
|
||||
goto(NORMAL) using d.copy(commitments = commitments1, channelUpdate = channelUpdate)
|
||||
}
|
||||
|
|
@ -1559,7 +1563,7 @@ class Channel(val nodeParams: NodeParams, wallet: EclairWallet, remoteNodeId: Pu
|
|||
// we only care about this event in NORMAL and SHUTDOWN state, and we never unregister to the event stream
|
||||
case Event(CurrentFeerates(_), _) => stay
|
||||
|
||||
// we only care about this event in NORMAL state, and the scheduler is never disabled
|
||||
// we only care about this event in NORMAL state, and the scheduler is not cancelled in some cases (e.g. NORMAL->CLOSING)
|
||||
case Event(TickRefreshChannelUpdate, _) => stay
|
||||
|
||||
// we receive this when we send command to ourselves
|
||||
|
|
@ -1604,14 +1608,18 @@ class Channel(val nodeParams: NodeParams, wallet: EclairWallet, remoteNodeId: Pu
|
|||
case _ => ()
|
||||
}
|
||||
|
||||
(stateData, nextStateData) match {
|
||||
case (d1: DATA_NORMAL, d2: DATA_NORMAL) if d1.channelUpdate == d2.channelUpdate && d1.channelAnnouncement == d2.channelAnnouncement =>
|
||||
(state, nextState, stateData, nextStateData) match {
|
||||
// ORDER MATTERS!
|
||||
case (_, _, d1: DATA_NORMAL, d2: DATA_NORMAL) if d1.channelUpdate == d2.channelUpdate && d1.channelAnnouncement == d2.channelAnnouncement =>
|
||||
// don't do anything if neither the channel_update nor the channel_announcement didn't change
|
||||
()
|
||||
case (_, normal: DATA_NORMAL) =>
|
||||
// whenever we go to a state with NORMAL data (can be OFFLINE or NORMAL), we send out the new channel_update (most of the time it will just be to enable/disable the channel)
|
||||
case (WAIT_FOR_FUNDING_LOCKED | NORMAL | SYNCING, NORMAL | OFFLINE, _, normal: DATA_NORMAL) =>
|
||||
// when we do WAIT_FOR_FUNDING_LOCKED->NORMAL or NORMAL->NORMAL or SYNCING->NORMAL or NORMAL->OFFLINE, we send out the new channel_update (most of the time it will just be to enable/disable the channel)
|
||||
context.system.eventStream.publish(LocalChannelUpdate(self, normal.commitments.channelId, normal.shortChannelId, normal.commitments.remoteParams.nodeId, normal.channelAnnouncement, normal.channelUpdate, normal.commitments))
|
||||
case (normal: DATA_NORMAL, _) =>
|
||||
case (_, _, _: DATA_NORMAL, _: DATA_NORMAL) =>
|
||||
// in any other case (e.g. WAIT_FOR_INIT_INTERNAL->OFFLINE) we do nothing
|
||||
()
|
||||
case (_, _, normal: DATA_NORMAL, _) =>
|
||||
// when we finally leave the NORMAL state (or OFFLINE with NORMAL data) to go to SHUTDOWN/NEGOTIATING/CLOSING/ERR*, we advertise the fact that channel can't be used for payments anymore
|
||||
// if the channel is private we don't really need to tell the counterparty because it is already aware that the channel is being closed
|
||||
context.system.eventStream.publish(LocalChannelDown(self, normal.commitments.channelId, normal.shortChannelId, normal.commitments.remoteParams.nodeId))
|
||||
|
|
|
|||
|
|
@ -366,4 +366,42 @@ class OfflineStateSpec extends TestkitBaseClass with StateTestsHelperMethods {
|
|||
channelUpdateListener.expectNoMsg(300 millis)
|
||||
}
|
||||
|
||||
test("bump timestamp in case of quick reconnection") { f =>
|
||||
import f._
|
||||
val sender = TestProbe()
|
||||
|
||||
// we simulate a disconnection
|
||||
sender.send(alice, INPUT_DISCONNECTED)
|
||||
sender.send(bob, INPUT_DISCONNECTED)
|
||||
awaitCond(alice.stateName == OFFLINE)
|
||||
awaitCond(bob.stateName == OFFLINE)
|
||||
|
||||
// alice and bob announce that their channel is OFFLINE
|
||||
val channelUpdate_alice_disabled = channelUpdateListener.expectMsgType[LocalChannelUpdate].channelUpdate
|
||||
val channelUpdate_bob_disabled = channelUpdateListener.expectMsgType[LocalChannelUpdate].channelUpdate
|
||||
assert(Announcements.isEnabled(channelUpdate_alice_disabled.channelFlags) == false)
|
||||
assert(Announcements.isEnabled(channelUpdate_bob_disabled.channelFlags) == false)
|
||||
|
||||
// we immediately reconnect them
|
||||
sender.send(alice, INPUT_RECONNECTED(alice2bob.ref, aliceInit, bobInit))
|
||||
sender.send(bob, INPUT_RECONNECTED(bob2alice.ref, bobInit, aliceInit))
|
||||
|
||||
// peers exchange channel_reestablish messages
|
||||
alice2bob.expectMsgType[ChannelReestablish]
|
||||
bob2alice.expectMsgType[ChannelReestablish]
|
||||
alice2bob.forward(bob)
|
||||
bob2alice.forward(alice)
|
||||
|
||||
// both nodes reach NORMAL state, and broadcast a new channel_update
|
||||
val channelUpdate_alice_enabled = channelUpdateListener.expectMsgType[LocalChannelUpdate].channelUpdate
|
||||
val channelUpdate_bob_enabled = channelUpdateListener.expectMsgType[LocalChannelUpdate].channelUpdate
|
||||
assert(Announcements.isEnabled(channelUpdate_alice_enabled.channelFlags) == true)
|
||||
assert(Announcements.isEnabled(channelUpdate_bob_enabled.channelFlags) == true)
|
||||
|
||||
// let's check that the two successive channel_update have a different timestamp
|
||||
assert(channelUpdate_alice_enabled.timestamp > channelUpdate_alice_disabled.timestamp)
|
||||
assert(channelUpdate_bob_enabled.timestamp > channelUpdate_bob_disabled.timestamp)
|
||||
|
||||
}
|
||||
|
||||
}
|
||||
|
|
|
|||
1
pom.xml
1
pom.xml
|
|
@ -216,6 +216,7 @@
|
|||
<systemProperties>
|
||||
<buildDirectory>${project.build.directory}</buildDirectory>
|
||||
</systemProperties>
|
||||
<argLine>-Xmx1024m</argLine>
|
||||
</configuration>
|
||||
<executions>
|
||||
<execution>
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue