multi: fix reliable re-processing+re-transmission of FundingLocked factoring in ChannelReestabilshment #457
Merged
Conversation
cfromknecht
reviewed
Dec 6, 2017
Contributor
There was a problem hiding this comment.
Why doesn't this use link.EligibleToForward()?
halseth
previously approved these changes
Dec 6, 2017
Contributor
There was a problem hiding this comment.
Should closing of the newChanReq.done be moved to the end? In case the sender is synchronising on this and expects the breachArbiter to have the updated contract and revocation to be set up when it closes.
Contributor
There was a problem hiding this comment.
Might have to close at each return/continue as well?
Member
Author
There was a problem hiding this comment.
Atm, the caller (the funding manager) has already sent a channel to the breach arbiter. We then override that by sending out this newer channel who's pointer to the next revocation point will be updated in memory.
Member
Author
|
Pushed a few follow up commits to address #458 At a high level:
I think this should address the items brought up in the issue, and also accomplish the original task set out. |
Roasbeef
force-pushed
the
funding-locked-fix
branch
from
8183d25 to
96f1c51
Compare
Member
Author
|
Pushed a rebased version addressing conner's comment. Tested this PR a good bit locally ensuring that we now properly handle the scenario pointed out in #458. This should fix a good bit of the issues people have been having when using ecliar's android app with |
…rward In this commit, we add a new method to the ChanneLink interface: EligibleToForward. This method allows a link to be added to the switch, but in an intermediate state which indicates that it isn’t yet ready to forward any incoming HTLC’s.
…ing links In this commit, when selecting a candidate link to forward a payment, we’ll ensure that it’s actually able to take on the HTLC. Otherwise, we’ll skip over the link itself. Currently, a link is only fully eligible for forwarding, *after* we’ve received and fully processed the FundingLocked message.
In this commit, we further constrain the candidacy for an “active” channel. In addition to being present within the link, it *must* also have the RemoteNextRevocation set. Otherwise, this indicates that we haven’t yet processed a FundingLocked message for this channel.
In this commit we revert a prior change which was added after FundingLocked retransmission was implemented. This prior change didn’t factor in the fact that the FundingLocked message will *only* be re-sent after both sides receive the ChannelReestablishment message. With the prior code, as we never added the channel to the link, we’d never re-send the ChannelReestablishment, meaning the other side would never send the FundingLocked message. By unconditionally adding the channel to the switch, we ensure that we’ll always properly retransmit the FundingLocked message.
…r processed
In this commit, we modify the logic within the channelManager to be
able to process any retransmitted FundingLocked messages. Before this
commit, we would simply ignore any new channels sent to us, iff, we
already had an active channel with the same channel point. With the
recent change to the loadActiveChannels method in the peer, this is now
incorrect.
When a peer retransmits the FundingLocked message, it goes through to
the fundingManager. The fundingMgr will then (if we haven’t already
processed it), send the channel to the breach arbiter and also to the
peer’s channelManager. In order to handle this case properly, if we
already have the channel, we’ll check if our current channel *doesn’t*
already have the RemoteNextRevocation field set. If it doesn’t, then
this means that we haven’t yet processed the FundingLcoked message, so
we’ll process it for the first time.
This new logic will properly:
* ensure that the breachArbiter still has the most up to date channel
* allow us to update the state of the link has been added to the
switch at this point
* this link will now be eligible for forwarding after this
sequence
…tablish This will allow users to run in trace mode again, without having an excessive amount of spam in their logs.
…h peer In this commit, we modify the logic within loadActiveChannels to *always* load a channel, even if it isn’t yet fully confirmed. With this change, we ensure that we’ll always send a channel_reestablish message upon reconnection. Fixes lightningnetwork#458.
…nc message This commit is a follow up to the prior commit: as it’s possible for the channel_reestablish message to be sent *before* the channel has been fully confirmed, we’ll now ensure that we process it to the link even if the channel isn’t yet open.
The IsPending method will allow callers to determine if a channel has been fully confirmed or not.
Roasbeef
force-pushed
the
funding-locked-fix
branch
from
96f1c51 to
669c2ee
Compare
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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 PR set out to remedy the issues that many users have reported surrounding their channels not being shown as "active" after the funding process has completed.
Existing Issue
The original logic surrounding process retransmitted
FundingLockedmessage and also re-sending the message was written without mind for the requirements around re-sending theChannelReestablishmentmessage upon reconnect. This was drawn to my attention after realizing I had a flapping connection to ac-lightningnode. The funding flow with the node didn't complete before I shutdown my local node. As a result, both nodes needed to retransmit theFundingLockedas we hadn't done any channel updates yet. However, my locallndnode was unable to do so for the following reasons:FundingLockedmessage.ChannelReestablishmentmessage, nor theFundingLockedmessage upon reconnection. This is due to the fact that we only re-send if the channel was added to the link.* Since we didn't add the channel to the link, the other peer wouldn't re-send the
FundingLockedmessage as the spec dictates to only do so after both sides have sent and received theChannelReestablishmentmessage.* The above series of actions would result in neither peer re-sending or re-processing the
FundingLockedmessage.Fix For Issue
In order to fix this, the following changes have been made:
EligibleToForwardhas been added to theChannelLinkinterface. This should return true, iff, the link is fully able to cary and forward any incoming HTLC's. The current default implementation of this only returns true if we have already processed theFundingLockedmessage.ListChannelsnow only marks a channel as "active", iff, it has fully processed theFundingLockedmessage.FundingLockedmessage for this first time, thechannelManagerwill now detect if the newly sent channel (by thefundingManager) is more "up to date". The channel is more up-to-date if the old channel didn't have the remote next revocation set. If it's newer, then it'll re-send to thebreachArbiter(to ensure it has the latest channel) and then update the revocation point in-place. After this is update, theEligibleToForwardwill now returntruewithin the switch.