Send CMD_UPDATE_RELAY_FEES to self at restore
#1922
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is an alternative to #1918 and #1920. It's very close to the latter, except that we do check the conf only once in the `WAIT_FOR_INIT_INTERNAL` state, as opposed to at each reconnection in `SYNCING`. We do not change the `channel_update` in `WAIT_FOR_INIT_INTERNAL`, which allows us to set `previousChannelUpdate_opt=Some(normal.channelUpdate)` in the transition and fix the duplicate bug in the audit db. If there is a change in conf, there will be an additional `LocalChannelUpdate` emitted, but only at reconnection, and following the regular update flow, which should protect us against regressions. We do handle `CMD_UPDATE_RELAY_FEES` in both `OFFLINE` and `SYNCING`, because there may be a race between `CMD_UPDATE_RELAY_FEES` and `ChannelRestablish` if the conf change at restore. And there was no good reason to behave differently in those states anyway.
pm47
commented
Aug 25, 2021
| } | ||
|
|
||
| private def handleUpdateRelayFeeDisconnected(c: CMD_UPDATE_RELAY_FEE, d: DATA_NORMAL) = { | ||
| val channelUpdate1 = Announcements.makeChannelUpdate(nodeParams.chainHash, nodeParams.privateKey, remoteNodeId, d.shortChannelId, c.cltvExpiryDelta_opt.getOrElse(d.channelUpdate.cltvExpiryDelta), d.channelUpdate.htlcMinimumMsat, c.feeBase, c.feeProportionalMillionths, d.commitments.capacity.toMilliSatoshi, enable = false) |
Member
Author
There was a problem hiding this comment.
It probably doesn't matter a lot, but I believe this is a bug:
Suggested change
| val channelUpdate1 = Announcements.makeChannelUpdate(nodeParams.chainHash, nodeParams.privateKey, remoteNodeId, d.shortChannelId, c.cltvExpiryDelta_opt.getOrElse(d.channelUpdate.cltvExpiryDelta), d.channelUpdate.htlcMinimumMsat, c.feeBase, c.feeProportionalMillionths, d.commitments.capacity.toMilliSatoshi, enable = false) | |
| val channelUpdate1 = Announcements.makeChannelUpdate(nodeParams.chainHash, nodeParams.privateKey, remoteNodeId, d.shortChannelId, c.cltvExpiryDelta_opt.getOrElse(d.channelUpdate.cltvExpiryDelta), d.channelUpdate.htlcMinimumMsat, c.feeBase, c.feeProportionalMillionths, d.commitments.capacity.toMilliSatoshi, enable = Announcements.isEnabled(d.channelUpdate.channelFlags)) |
pm47
changed the title
CMD_UPDATE_RELAY_FEES to self at restore
| log.debug("re-emitting channel_update={} enabled={} ", normal.channelUpdate, Announcements.isEnabled(normal.channelUpdate.channelFlags)) | ||
| } | ||
| context.system.eventStream.publish(LocalChannelUpdate(self, normal.commitments.channelId, normal.shortChannelId, normal.commitments.remoteParams.nodeId, normal.channelAnnouncement, normal.channelUpdate, previousChannelUpdate_opt, normal.commitments)) | ||
| context.system.eventStream.publish(LocalChannelUpdate(self, normal.commitments.channelId, normal.shortChannelId, normal.commitments.remoteParams.nodeId, normal.channelAnnouncement, normal.channelUpdate, Some(normal.channelUpdate), normal.commitments)) |
Contributor
There was a problem hiding this comment.
Here we will emit an additional outdated LocalChannelUpdate in case the config has changed.
Contributor
thomash-acinq
left a comment
thomash-acinq
left a comment
There was a problem hiding this comment.
Overall it looks like a clean solution.
The only downside is that it can behave differently than the current code by emitting an outdated LocalChannelUpdate. If you think it's fine, I'm OK with it. I'm not sure how this LocalChannelUpdate is used later, maybe it's not a problem to emit a bad one followed not too long after by a good one.
There should be some tests that the correct LocalChannelUpdate are emitted when we expect and won't produce duplicate rows in the channel_updates table.
pm47
commented
Sep 2, 2021
Comment on lines
+2137
to
+2138
| // we're in OFFLINE state, we don't broadcast the new update right away, we will do that when next time we go to NORMAL state | ||
| stay() using d.copy(channelUpdate = channelUpdate1) storing() |
Member
Author
There was a problem hiding this comment.
So... this doesn't work, as evidenced by 2c8db6e.
By using a stay() to defer the sending of the event, we lose the reference to the previous channel_update 🤦♂️
Member
Author
|
Closed in favor of #1934 |
This was referenced Sep 2, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is an alternative to #1918 and #1920. It's very close to the
latter, except that we do check the conf only once in the
WAIT_FOR_INIT_INTERNALstate, as opposed to at each reconnection inSYNCING.We do not change the
channel_updateinWAIT_FOR_INIT_INTERNAL, whichallows us to set
previousChannelUpdate_opt=Some(normal.channelUpdate)in the transition and fix the duplicate bug in the audit db.
If there is a change in conf, there will be an additional
LocalChannelUpdateemitted, but only at reconnection, and followingthe regular update flow, which should protect us against regressions.
We do handle
CMD_UPDATE_RELAY_FEESin bothOFFLINEandSYNCING,because there may be a race between
CMD_UPDATE_RELAY_FEESandChannelRestablishif the conf change at restore. And there was no goodreason to behave differently in those states anyway.