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

adds fundmax flag to openchannel #4029

Closed
19 changes: 18 additions & 1 deletion cmd/lncli/cmd_open_channel.go
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,13 @@ var openChannelCommand = cli.Command{
Name: "connect",
Usage: "(optional) the host:port of the target node",
},
cli.BoolFlag{
Name: "fundmax",
Usage: "(optional) if set, the wallet will attempt to " +
"commit the maximum possible local amount to " +
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole commit should be squashed with the respective commits where the initial changes were introduced.

Copy link
Collaborator

@hieblmi hieblmi Sep 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done here b8e351e and here 7915d0d

"the channel. Should not be set together with " +
"local_amt",
},
cli.IntFlag{
Name: "local_amt",
Usage: "the number of satoshis the wallet should commit to the channel",
Expand Down Expand Up @@ -249,6 +256,7 @@ func openChannel(ctx *cli.Context) error {
CloseAddress: ctx.String("close_address"),
RemoteMaxValueInFlightMsat: ctx.Uint64("remote_max_value_in_flight_msat"),
MaxLocalCsv: uint32(ctx.Uint64("max_local_csv")),
FundMax: ctx.Bool("fundmax"),
}

switch {
Expand Down Expand Up @@ -302,10 +310,19 @@ func openChannel(ctx *cli.Context) error {
return fmt.Errorf("unable to decode local amt: %v", err)
}
args = args.Tail()
default:
case !ctx.Bool("fundmax"):
guggero marked this conversation as resolved.
Show resolved Hide resolved
return fmt.Errorf("local amt argument missing")
}

// Note:
// The fundmax flag is NOT allowed to be combiend with local_amt above.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo (combiend -> combined)

Copy link
Collaborator

@hieblmi hieblmi Sep 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// It is allowed to be combined with push_amt, but only if explicitly
// set.
if ctx.Bool("fundmax") && req.LocalFundingAmount != 0 {
return fmt.Errorf("local amount cannot be set if attempting to " +
"commit the maximum amount out of the wallet")
}

if ctx.IsSet("push_amt") {
req.PushSat = int64(ctx.Int("push_amt"))
} else if args.Present() {
Expand Down
7 changes: 7 additions & 0 deletions funding/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,12 @@ type InitFundingMsg struct {
// peer.
MaxLocalCsv uint16

// FundUpToMax is the maximum amount to try to commit to. If set, the
// LocalFundingAmt field denotes the acceptable minimum amount to
// commit to, while trying to commit as many coins as possible up to
// this value.
FundUpToMaxAmt btcutil.Amount

// ChanFunder is an optional channel funder that allows the caller to
// control exactly how the channel funding is carried out. If not
// specified, then the default chanfunding.WalletAssembler will be
Expand Down Expand Up @@ -3293,6 +3299,7 @@ func (f *Manager) handleInitFundingMsg(msg *InitFundingMsg) {
SubtractFees: msg.SubtractFees,
LocalFundingAmt: localAmt,
RemoteFundingAmt: 0,
FundUpToMaxAmt: msg.FundUpToMaxAmt,
CommitFeePerKw: commitFeePerKw,
FundingFeePerKw: msg.FundingFeePerKw,
PushMSat: msg.PushAmt,
Expand Down
114 changes: 111 additions & 3 deletions funding/manager_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -650,7 +650,7 @@ func openChannel(t *testing.T, alice, bob *testNode, localFundingAmt,
*wire.OutPoint, *wire.MsgTx) {

publ := fundChannel(
t, alice, bob, localFundingAmt, pushAmt, false, numConfs,
t, alice, bob, localFundingAmt, pushAmt, false, 0, numConfs,
updateChan, announceChan,
)
fundingOutPoint := &wire.OutPoint{
Expand All @@ -663,7 +663,7 @@ func openChannel(t *testing.T, alice, bob *testNode, localFundingAmt,
// fundChannel takes the funding process to the point where the funding
// transaction is confirmed on-chain. Returns the funding tx.
func fundChannel(t *testing.T, alice, bob *testNode, localFundingAmt,
pushAmt btcutil.Amount, subtractFees bool, numConfs uint32,
pushAmt btcutil.Amount, subtractFees bool, fundUpToMaxAmt btcutil.Amount, numConfs uint32, // nolint:unparam
updateChan chan *lnrpc.OpenStatusUpdate, announceChan bool) *wire.MsgTx {

// Create a funding request and start the workflow.
Expand All @@ -673,6 +673,7 @@ func fundChannel(t *testing.T, alice, bob *testNode, localFundingAmt,
TargetPubkey: bob.privKey.PubKey(),
ChainHash: *fundingNetParams.GenesisHash,
SubtractFees: subtractFees,
FundUpToMaxAmt: fundUpToMaxAmt,
LocalFundingAmt: localFundingAmt,
PushAmt: lnwire.NewMSatFromSatoshis(pushAmt),
FundingFeePerKw: 1000,
Expand Down Expand Up @@ -3249,7 +3250,7 @@ func TestFundingManagerFundAll(t *testing.T) {
// Initiate a fund channel, and inspect the funding tx.
pushAmt := btcutil.Amount(0)
fundingTx := fundChannel(
t, alice, bob, test.spendAmt, pushAmt, true, 1,
t, alice, bob, test.spendAmt, pushAmt, true, 0, 1,
updateChan, true,
)

Expand Down Expand Up @@ -3280,6 +3281,113 @@ func TestFundingManagerFundAll(t *testing.T) {
}
}

// TestFundingManagerFundMax tests that we can initiate a funding request to
// use the maximum allowed funds remaining in the wallet.
func TestFundingManagerFundMax(t *testing.T) {
t.Parallel()

tests := []struct {
coins []*lnwallet.Utxo
fundUpToMaxAmt btcutil.Amount
change bool
}{
{
// We will spend all the funds in the wallet, and
// expects no change output.
coins: []*lnwallet.Utxo{{
AddressType: lnwallet.WitnessPubKey,
Value: MaxBtcFundingAmount + 1,
PkScript: mock.CoinPkScript,
OutPoint: wire.OutPoint{
Hash: chainhash.Hash{},
Index: 0,
},
}},
fundUpToMaxAmt: MaxBtcFundingAmount,
change: false,
},
{
// We spend less than the funds in the wallet,
// so a change output should be created.
coins: []*lnwallet.Utxo{{
AddressType: lnwallet.WitnessPubKey,
Value: 2 * MaxBtcFundingAmount,
PkScript: mock.CoinPkScript,
OutPoint: wire.OutPoint{
Hash: chainhash.Hash{},
Index: 0,
},
}},
fundUpToMaxAmt: MaxBtcFundingAmount,
change: true,
},
{
// We spend less than the funds in the wallet when
// setting a smaller channel size, so a change output
// should be created.
coins: []*lnwallet.Utxo{{
AddressType: lnwallet.WitnessPubKey,
Value: MaxBtcFundingAmount,
PkScript: mock.CoinPkScript,
OutPoint: wire.OutPoint{
Hash: chainhash.Hash{},
Index: 0,
},
}},
fundUpToMaxAmt: MaxBtcFundingAmount / 2,
change: true,
},
}

for _, test := range tests {
// We set up our mock wallet to control a list of UTXOs that
// sum to more than the max channel size.
test := test
addFunds := func(fundingCfg *Config) {
wc := fundingCfg.Wallet.WalletController
wc.(*mock.WalletController).Utxos = test.coins
}
alice, bob := setupFundingManagers(t, addFunds)
defer tearDownFundingManagers(t, alice, bob)

// We will consume the channel updates as we go, so no
// buffering is needed.
updateChan := make(chan *lnrpc.OpenStatusUpdate)

// Initiate a fund channel, and inspect the funding tx.
pushAmt := btcutil.Amount(0)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we also test a pushamt greater than the MinChanFundingSize?

Copy link
Collaborator

@hieblmi hieblmi Sep 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fundingTx := fundChannel(
t, alice, bob, MinChanFundingSize, pushAmt, false,
test.fundUpToMaxAmt, 1, updateChan, true,
)

// Check whether the expected change output is present.
if test.change && len(fundingTx.TxOut) != 2 {
t.Fatalf("expected 2 outputs, had %v",
len(fundingTx.TxOut))
}

if !test.change && len(fundingTx.TxOut) != 1 {
t.Fatalf("expected 1 output, had %v",
len(fundingTx.TxOut))
}

// Inputs should be all funds in the wallet.
if len(fundingTx.TxIn) != len(test.coins) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add a test case with multiple inputs so this check actually does make sense?

Copy link
Collaborator

@hieblmi hieblmi Sep 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

t.Fatalf("Had %d inputs, expected %d",
len(fundingTx.TxIn), len(test.coins))
}

for i, txIn := range fundingTx.TxIn {
if txIn.PreviousOutPoint != test.coins[i].OutPoint {
t.Fatalf("expected outpoint to be %v, was %v",
test.coins[i].OutPoint,
txIn.PreviousOutPoint)
}
}
}
}

// TestGetUpfrontShutdown tests different combinations of inputs for getting a
// shutdown script. It varies whether the peer has the feature set, whether
// the user has provided a script and our local configuration to test that
Expand Down
Loading