lnwallet,funding: simplify initial funding flow #7210
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
The bug manifests when a nil ChannelType is passed to the funding manager in InitFundingMsg. A default value for ChannelType is selected and sent in the OpenChannel message. However, a nil ChannelType is stored in the reservation context. This causes our ChannelType checks in handleFundingAccept to be bypassed. Usually this makes us end up in the "peer unexpectedly sent explicit ChannelType" case, where we can still recover by reselecting a default ChannelType and verifying it matches the one the peer sent. But if the peer sends a nil ChannelType, we miss it. While fixing the bug, I also tried to simplify the negotiation logic, as the complexity is likely what hid the bug in the first place. Now negotiateCommitmentType only returns the ChannelType to be used in OpenChannel/AcceptChannel and the CommitmentType to pass to the wallet. It will even return a nil ChannelType when we're supposed to use implicit negotiation, so we don't need to manually set it to nil for OpenChannel and AcceptChannel.
Move this functionality into InitChannelReservation, since we always have the shutdown script available at that point anyway.
Move this functionality into InitChannelReservation, since we can generate the SCID alias at that point anyway.
The ChanFunder is allowed modify the local funding amount and is expected to do so when SubtractFees is true. We add a comment to indicate we expect this. The ChanFunder does not modify the remote funding amount, so we don't need to request the amount from it.
isPublic and hasAnchors are only used within the following block, so move them into the block.
This is more natural than passing capacity, and makes the commitment balance logic easier to follow. Also resolves a TODO.
PushMSat is always 0 for dual funding.
Removes repetitive code and simplifies by combining checks for insufficient funds and dust balances.
nodeAddr and nodeID can be set directly in NewChannelReservation, so there's no need to pass them to initOurContribution.
The funding intent is always set when we call initOurContribution, so we don't need to check for nil again.
It is unused, and its value is the same as reservation.Capacity() anyway.
It is unused, and its value is also recorded in partialState.
The only value used from DefaultConstraints is the DustLimit, and all other fields are overwritten in CommitConstraints. So we can replace DefaultConstraints with DefaultDustLimit.
Currently we set DustLimit to a default value in the wallet, and the
wallet opaquely updates the DustLimit inside CommitConstraints if the
peer's requested channel reserve is too small. But all other channel
constraints are decided in the funding manager.
The existing behavior is confusing for two reasons:
1. It's unclear which DustLimit (local or remote) to pass into
CommitConstraints, and which limit to set for ProcessContribution.
Currently we use the remote DustLimit for both!
2. The local DustLimit is a secret return value from
CommitConstraints, which we must extract via OurContribution() to
send the correct limit to our peer.
This commit fixes both of the above by moving DustLimit selection into
the funding manager and modifying CommitConstraints to set both the
local and remote constraints.
The result is a mostly simpler, more intuitive API. The main ugly part
here is that now any constraints specified in ProcessContribution are
ignored. This ugliness will be addressed in subsequent commits, where we
redesign the ProcessContribution API.
Encapsulate pending remote constraints within a ChannelConstraints in the reservation context.
Move the validateReserveBounds call from ProcessContribution to CommitConstraints, since we now have both local and remote constraints available in CommitConstraints.
We replace CommitConstraints with ProcessChannelParams. The benefits are
twofold:
1. We more clearly document what values are expected for local and
remote constraints.
2. In future commits we can encapsulate more parameters in
ChannelParams, simplifying the state machine and allowing more code
sharing between the initiator and non-initiator flows.
Move this functionality into ProcessChannelParams for simplicity.
This commit does two things: 1. Removes the shutdown script from ChannelContribution. 2. Moves remote shutdown script validation to ProcessChannelParams. The shutdown script doesn't need to be in ChannelContribution because we eventually record it directly in partialState anyway, and because the funding manager doesn't actually read the shutdown script from OurContribution(). The remote shutdown script validation logic is identical in both the initiator and non-initiator flows, so we can share the code by moving validation to ProcessChannelParams.
Move the remaining functionality from ProcessSingleContribution into ProcessChannelParams. This allows the code to be fully shared between initiator and non-initiator flows and simplifies the non-initiator state machine.
Now that most of the ChannelContribution fields are unused by ProcessContribution, it is more intuitive to pass inputs and outputs directly.
Update the documentation comments to reflect the new funding flow.
morehouse
force-pushed
the
simplify_initial_funding_flow
branch
from
309794b to
5d6f7de
Compare
Roasbeef
added
funding
8 tasks
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.
The existing funding flow is quite old, with some code duplication and other accumulated technical debt. This PR attempts to increase code reuse, simplify the channel reservation state machine, and pay down the technical debt. Additionally, this refactoring sets the stage nicely for dual funding work.
Existing Flows
Initiator
lnwallet.InitChannelReservationreservation.SetOurUpfrontShutdownreservation.AddAliasreservation.SetNumConfsRequiredreservation.CommitConstraintsreservation.ProcessContributionreservation.CompleteReservationNon-initiator
lnwallet.InitChannelReservationreservation.SetOurUpfrontShutdownreservation.AddAliasreservation.SetNumConfsRequiredreservation.CommitConstraintsreservation.ProcessSingleContributionreservation.CompleteReservationSingleNew Flows
Initiator
lnwallet.InitChannelReservationreservation.ProcessChannelParamsreservation.ProcessContributionreservation.CompleteReservationNon-initiator
lnwallet.InitChannelReservationreservation.ProcessChannelParamsreservation.CompleteReservationSingleUpdates #6569. Depends on #7177.