Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chanfunding: create new package to abstract over funding workflows #3659

Merged

Conversation

@Roasbeef
Copy link
Member

Roasbeef commented Nov 1, 2019

In this commit, we create a new package chanfunding, that introduces a number of abstractions to our funding workflow. The base implementation of the core new abstraction (chanfunding.Assembler), ischanfunding.WalletAssembler. Construction of this concrete implementation of the new interfaces involved heavily refactoring much of the code within wallet.go. As a result, much of the channel funding code now lives outside of the lnwallet package.

The second instance of a new channel funder introduced in this PR is the chanfunding.CannedAssembler. This channel funder is used in the scenario wherein the "shape" of the funding output is agreed upon outside the normal protocol, while the construction of the commitment transaction and so forth are carried out as normal. Instances where this might be the case include variants of channel factory leaf channel construction, and external hardware wallet based channel funding.

This PR is the 3rd in a series of PRs that revamps the way channel funding works within lnd to allow funding transactions to be constructed externally, with the grand goal of a PSBT (safety first!!!) based channel funding workflow.

@Roasbeef Roasbeef requested a review from halseth as a code owner Nov 1, 2019
@Roasbeef Roasbeef requested a review from wpaulino Nov 1, 2019
@Roasbeef Roasbeef added this to the 0.9.0 milestone Nov 1, 2019
@Roasbeef Roasbeef added this to WIP in v0.9.0-beta via automation Nov 1, 2019
@Roasbeef

This comment has been minimized.

Copy link
Member Author

Roasbeef commented Nov 1, 2019

The final diff looks larger than it actually is due to the code movement that has taken place.

@wpaulino

This comment has been minimized.

Copy link
Collaborator

wpaulino commented Nov 5, 2019

Haven't started review yet, but there are a few linter failures causing Travis to fail.

@joostjager joostjager moved this from WIP to Needs Review in v0.9.0-beta Nov 5, 2019
Copy link
Collaborator

halseth left a comment

Did an initial pass, and I like the direction we are goin in with the interfaces. I still think we can extract even more of the logic behind the interfaces, such that the subsystems can have even less assumptions about the used Assembler.

Also I think several of the commits won't compile.

fundingmanager.go Show resolved Hide resolved
fundingmanager.go Outdated Show resolved Hide resolved
lnwallet/wallet.go Outdated Show resolved Hide resolved
lnwallet/chanfunding/wallet_assembler.go Show resolved Hide resolved
lnwallet/chanfunding/wallet_assembler.go Show resolved Hide resolved
lnwallet/chanfunding/wallet_assembler.go Outdated Show resolved Hide resolved
lnwallet/wallet.go Show resolved Hide resolved
lnwallet/chanfunding/external_assembler.go Outdated Show resolved Hide resolved
lnwallet/chanfunding/external_assembler.go Outdated Show resolved Hide resolved
lnwallet/reservation.go Outdated Show resolved Hide resolved
@Roasbeef Roasbeef requested a review from halseth Nov 7, 2019
@Roasbeef Roasbeef force-pushed the Roasbeef:external-funding-chanfunding branch 2 times, most recently from 498fa47 to e02289e Nov 7, 2019
lnwallet/chanfunding/assembler.go Show resolved Hide resolved
lnwallet/chanfunding/assembler.go Show resolved Hide resolved
lnwallet/chanfunding/assembler.go Outdated Show resolved Hide resolved
lnwallet/chanfunding/assembler.go Show resolved Hide resolved
lnwallet/chanfunding/assembler.go Show resolved Hide resolved
lnwallet/reservation.go Outdated Show resolved Hide resolved
lnwallet/reservation.go Show resolved Hide resolved
lnwallet/wallet.go Show resolved Hide resolved
lnwallet/wallet.go Show resolved Hide resolved
lnwallet/wallet.go Outdated Show resolved Hide resolved
Copy link
Collaborator

guggero left a comment

Very nice abstraction! It's great to see this push towards external funding of channels.
I did a high-level review, mostly to understand what the goal with the Assembler is. Left some nits along the way.

channeldb/channel.go Outdated Show resolved Hide resolved
fundingmanager.go Show resolved Hide resolved
lnwallet/chanfunding/coin_select.go Show resolved Hide resolved
lnwallet/chanfunding/assembler.go Outdated Show resolved Hide resolved
lnwallet/chanfunding/wallet_assembler.go Outdated Show resolved Hide resolved
lnwallet/chanfunding/canned_assembler.go Outdated Show resolved Hide resolved
Copy link
Collaborator

wpaulino left a comment

I found the PR a bit involved to follow and review due to the commit structure, though it may be too late to address this now.

lnwallet/reservation.go Show resolved Hide resolved
// OutpointLocker allows a caller to lock/unlock an outpoint. When locked, the
// outpoints shouldn't be used for any sort of channel funding of coin
// selection. Locked outpoints are not expect to be persisted between restarts.
type OutpointLocker interface {

This comment has been minimized.

Copy link
@wpaulino

wpaulino Nov 12, 2019

Collaborator

Not a blocker for this PR, but some of these interfaces are already defined within sweep/walletsweep.go. Do we plan to import these there?

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Nov 12, 2019

Author Member

They were purposefully copied over as they're pretty small interfaces, so would rather not create additional dependancies. It's also the case that if the interfaces in the sweeper are used, then a circular dependency cycle is created.

This comment has been minimized.

Copy link
@wpaulino

wpaulino Nov 22, 2019

Collaborator

There's no circular dependency here since this is the chanfunding package?

This comment has been minimized.

Copy link
@halseth

halseth Nov 27, 2019

Collaborator

I also agree that it is nice for a package to define its own set of interfaces, since it has it own set of requirements separate from the other package that might diverge in the future.

lnwallet/chanfunding/assembler.go Outdated Show resolved Hide resolved
lnwallet/chanfunding/wallet_assembler.go Outdated Show resolved Hide resolved
lnwallet/interface_test.go Show resolved Hide resolved
lnwallet/wallet.go Show resolved Hide resolved
lnwallet/chanfunding/canned_assembler.go Show resolved Hide resolved
lnwallet/wallet.go Outdated Show resolved Hide resolved
lnwallet/wallet.go Outdated Show resolved Hide resolved
)
if err != nil {
selected.unlockCoins()
if fundingIntent != nil {
fundingIntent.Cancel()

This comment has been minimized.

Copy link
@wpaulino

wpaulino Nov 12, 2019

Collaborator

Shouldn't we also do these for the other steps, e.g., handleContributionMsg?

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Nov 14, 2019

Author Member

To free up the coins? If a reservation is failed, and doesn't proceed, then they'll be reclaimed via the zombie reservation sweeper within the fundingManager.

This comment has been minimized.

Copy link
@wpaulino

wpaulino Nov 22, 2019

Collaborator

Following that logic, why bother doing it here then? Seems inconsistent.

This comment has been minimized.

Copy link
@halseth

halseth Nov 27, 2019

Collaborator

could do it in all FailFundingFlow cases perhaps

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Nov 28, 2019

Author Member

It's done here optimistically to immediately clean up any coins for a funding request that hasn't even advanced passed the reservation phase. failFundingFlow already does this (in response to another error, or an invalid funding workflow message by the remote peer) since it calls Cancel on the reservation object, which will cancel and remove the intent.

Copy link
Collaborator

guggero left a comment

Nice, commit structure improved! Was able to follow the changes more easily.
Some commits are probably not in the right order, some of them don't compile.
Also the unit tests fail:
interface_test.go:491: unable to consume alice's sigs: input to funding tx does not exist: target output was not found

lnwallet/reservation.go Outdated Show resolved Hide resolved
lnwallet/wallet.go Show resolved Hide resolved
lnwallet/wallet.go Show resolved Hide resolved
channeldb/channel.go Show resolved Hide resolved
fundingmanager.go Show resolved Hide resolved
@@ -166,6 +171,12 @@ func (c ChannelType) IsTweakless() bool {
return c&SingleFunderTweaklessBit == SingleFunderTweaklessBit
}

// HasFundingTx returns true if this channel type is one that has a funding
// transaction stored locally.
func (c ChannelType) HasFundingTx() bool {

This comment has been minimized.

Copy link
@halseth

halseth Nov 19, 2019

Collaborator

This might actually be a bit dangerous, as old channels which we are not initiator will not have the funding tx on disk, but also the bit won't be set.

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Nov 21, 2019

Author Member

If the bit isn't set, then that means we have the funding transaction. If it's set, then it means we don't. As written, if we're about to attempt to handle the funding transaction, we also check if we're the initiator or not.

This comment has been minimized.

Copy link
@wpaulino

wpaulino Nov 22, 2019

Collaborator

I think the confusion arises from the method name. HasFundingTx makes it sound like it's the only check needed to retrieve the transaction, perhaps something like CraftedExternally is better?

This comment has been minimized.

Copy link
@halseth

halseth Nov 26, 2019

Collaborator

... we also check if we're the initiator or not.

Yeah, this needs to be done which is why I think HasFundingTx is a dangerous name, as there is no guarantee we have it even though it returns true.

This comment has been minimized.

Copy link
@halseth

halseth Nov 27, 2019

Collaborator

I guess it makes sense since it is the channel type that has the funding tx, not the channel. I'm okay with keeping the name, but adding to the godoc that it is only the initiator that has the tx strored locally, but I agree with @wpaulino that a more descriptive name could lessen the confusion.

lnwallet/chanfunding/assembler.go Show resolved Hide resolved
lnwallet/chanfunding/canned_assembler.go Outdated Show resolved Hide resolved
lnwallet/chanfunding/wallet_assembler.go Outdated Show resolved Hide resolved
lnwallet/wallet.go Show resolved Hide resolved
lnwallet/wallet.go Show resolved Hide resolved
lnwallet/wallet.go Show resolved Hide resolved
lnwallet/interface_test.go Outdated Show resolved Hide resolved
@Roasbeef Roasbeef force-pushed the Roasbeef:external-funding-chanfunding branch from abfa7f0 to 3711cf5 Nov 21, 2019
@Roasbeef

This comment has been minimized.

Copy link
Member Author

Roasbeef commented Nov 21, 2019

The issue in the tests should be fixed now, see the last commit w.r.t the exact situation (GetUtxo for neutrino treats the height hint as a precise birth height rather than a lower bound on the birth height of the UTXO).

@Roasbeef Roasbeef requested review from halseth and guggero Nov 21, 2019
@Roasbeef Roasbeef force-pushed the Roasbeef:external-funding-chanfunding branch 2 times, most recently from 0811963 to d2dc3e0 Nov 21, 2019
Copy link
Collaborator

guggero left a comment

I did another pass and the changes look solid to me 🔥

Copy link
Collaborator

wpaulino left a comment

Code LGTM, left a few minor comments.

channeldb/channel.go Outdated Show resolved Hide resolved
@@ -166,6 +171,12 @@ func (c ChannelType) IsTweakless() bool {
return c&SingleFunderTweaklessBit == SingleFunderTweaklessBit
}

// HasFundingTx returns true if this channel type is one that has a funding
// transaction stored locally.
func (c ChannelType) HasFundingTx() bool {

This comment has been minimized.

Copy link
@wpaulino

wpaulino Nov 22, 2019

Collaborator

I think the confusion arises from the method name. HasFundingTx makes it sound like it's the only check needed to retrieve the transaction, perhaps something like CraftedExternally is better?

fundingmanager.go Show resolved Hide resolved
// OutpointLocker allows a caller to lock/unlock an outpoint. When locked, the
// outpoints shouldn't be used for any sort of channel funding of coin
// selection. Locked outpoints are not expect to be persisted between restarts.
type OutpointLocker interface {

This comment has been minimized.

Copy link
@wpaulino

wpaulino Nov 22, 2019

Collaborator

There's no circular dependency here since this is the chanfunding package?

lnwallet/chanfunding/wallet_assembler.go Outdated Show resolved Hide resolved
)
if err != nil {
selected.unlockCoins()
if fundingIntent != nil {
fundingIntent.Cancel()

This comment has been minimized.

Copy link
@wpaulino

wpaulino Nov 22, 2019

Collaborator

Following that logic, why bother doing it here then? Seems inconsistent.

testName := fmt.Sprintf("%v/%v:%v", walletType, backEnd,
walletTest.name)
success := t.Run(testName, func(t *testing.T) {
if backEnd == "neutrino" &&
strings.Contains(walletTest.name, "dual funder") {

This comment has been minimized.

Copy link
@wpaulino

wpaulino Nov 22, 2019

Collaborator

Nit: just add backend check in the test case itself?

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Nov 28, 2019

Author Member

AFAICT, the test doesn't know which backend is being used. The config for btcwallet is a private variable so the test can't access that to perform a type assertion, I tried that route before this one.

Copy link
Collaborator

halseth left a comment

I think this is coming together now, and after also looking at the follow-up PR I see the big picture of this change.

However, I still think there are still significant cleanup that can be made, by removing reservation logic from the wallet that is now all handled by the ChanFunder. Since the the funding transaction crafting now is handled by the ChanFunder, keys, signatures, funding inputs etc can be opaque to the reservation logic, as it should be abstracted away by the assembler interface (it already is for the external funding case). That would get us on a much needed path towards simplifying the reservation logic :)

lnwallet/chanfunding/coin_select.go Show resolved Hide resolved
lnwallet/chanfunding/assembler.go Show resolved Hide resolved
// OutpointLocker allows a caller to lock/unlock an outpoint. When locked, the
// outpoints shouldn't be used for any sort of channel funding of coin
// selection. Locked outpoints are not expect to be persisted between restarts.
type OutpointLocker interface {

This comment has been minimized.

Copy link
@halseth

halseth Nov 27, 2019

Collaborator

I also agree that it is nice for a package to define its own set of interfaces, since it has it own set of requirements separate from the other package that might diverge in the future.

lnwallet/chanfunding/assembler.go Show resolved Hide resolved
lnwallet/chanfunding/assembler.go Show resolved Hide resolved
lnwallet/wallet.go Outdated Show resolved Hide resolved
lnwallet/wallet.go Show resolved Hide resolved
lnwallet/wallet.go Show resolved Hide resolved
lnwallet/wallet.go Outdated Show resolved Hide resolved
lnwallet/wallet.go Show resolved Hide resolved
@Roasbeef

This comment has been minimized.

Copy link
Member Author

Roasbeef commented Nov 28, 2019

However, I still think there are still significant cleanup that can be made, by removing reservation logic from the wallet that is now all handled by the ChanFunder

IMO this is out of scope for this PR, I purposefully kept that logic in place in order to not have the PR be too large. The reservation logic is still the bridge between our internal representatio+provisioning of the various channel parmaters (keys, csv delay, sigs, etc) and how things are manifested on the p2p layer (openchannel, acceptchannel, etc). This PR is only about abstracting away how the funding transaction and channel point are obtained by the wallet, and nothing more.

@Roasbeef Roasbeef force-pushed the Roasbeef:external-funding-chanfunding branch 2 times, most recently from 664af73 to 1227a57 Nov 28, 2019
lnwallet/chanfunding/canned_assembler.go Outdated Show resolved Hide resolved
lnwallet/wallet.go Show resolved Hide resolved
lnwallet/wallet.go Outdated Show resolved Hide resolved
lnwallet/wallet.go Show resolved Hide resolved
Roasbeef added 12 commits Nov 1, 2019
In this commit, we add a new bit to the existing ChannelType bitfield.
If this bit is set, then it signals that we have the funding transaction
stored on disk. A future change will enable lnd to have the funding
transaction be constructed externally, allowing for things like funding
from a hardware wallet, or a channel created as a sub-branch within an
existing channel factory.
…ackage

In this commit, we make an incremental change to move the existing coin
selection code into a new chanfunding package. In later commits, this
package will grow to serve all the lower level channel funding needs in
the daemon.
We also remove some related and also unused attributes as well along the
way.
In this commit, we introduce a series of new abstractions for channel
funding. The end goal is to enable uses cases that construct the funding
transaction externally, eventually handing the funding outpoint to lnd.
An example of such a use case includes channel factories and external
channel funding using a hardware wallet.

We also add a new chanfunding.Assembler meant to allow external channel
funding in contexts similar to how channel factories
can be constructed. With this channel funder, we'll only obtain the
channel point and funding output from it, as this alone is enough to
carry out a funding flow as normal.
…ackage

In this commit, we begin to integrate the new channel funding package
into the existing codebase. With this set of changes, we'll no longer
construct and sign the funding transaction within this package, instead
delegating it to the new chanfunding package. We use the new
chanfunding.WalletAssembler to carry out all channel funding, providing
it with an implementation of all its interfaces backed by the wallet.
In this commit, we make the wallet aware of the second type of funding
intent: the ShimIntent. If we have one of these, then we don't need to
construct the funding transaction, and can instead just obtain the
outpoint directly from it.
…ntext

In this commit, we start to thread the pending channel ID from wire
protocol all the way down into the reservation context. This change will
allow negotiation to take place _outside_ the protocol that may result
in a particular chanfunding.Assembler being dispatched.
In this commit, we add a new method `RegisterFundingIntent` that allows
a caller to "inject" a pre-populated chanfunding.Intent into a funding
workflow. As an example, if we've already agreed upon the "shape" of the
funding output _outside_ the protocol, then we can use this to pass down
the details of the output, then leverage the normal wire protocol to
carry out the remainder of the funding flow.
…eutrino

In this commit, we fix a long standing bug within the newly created
`verifyFundingInputs` method. Before this commit, the method would
attempt to derive the pkScript by looking at the last items on the
witness stack, and making a p2wsh output script from that. This is
incorrect as typically non of these scripts will actually be p2wsh, and
instead will be p2wkh. We fix this by using the newly available
`txscript.ComputePkScript` method to derive the proper pkScript.

This resolves an issue w.r.t passing incorrect arguments for all
backends, but an issue still stands for the neutrino backend. As is, we
pass a height hint of zero into the `GetUtxo` method call. With the way
the current utxo scanner is set up for neutrino, this'll cause it to
never find the UTXO, as it takes the height hint as a UTXO birth height,
rather than a lower bound of the birth of the UTXO.
@Roasbeef Roasbeef force-pushed the Roasbeef:external-funding-chanfunding branch from 1227a57 to b1940d6 Dec 2, 2019
@Roasbeef Roasbeef merged commit f2f79d3 into lightningnetwork:master Dec 3, 2019
1 of 2 checks passed
1 of 2 checks passed
coverage/coveralls Coverage decreased (-0.6%) to 62.099%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
v0.9.0-beta automation moved this from Needs Review to Done Dec 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
v0.9.0-beta
  
Done
4 participants
You can’t perform that action at this time.