rpc: anchor reserve check for `SendOutputs` #7631

hieblmi · 2023-04-23T18:09:44Z

This PR adds a check to the SendOutputs RPC to prevent it from undershooting the anchor reserve requirement.

itest/list_on_test.go

lnrpc/walletrpc/walletkit_server.go

yyforyongyu · 2023-04-26T01:44:59Z

lnrpc/walletrpc/walletkit_server.go

@@ -1666,7 +1673,9 @@ func parseAddrType(addrType AddressType,
 	switch addrType {
 	case AddressType_UNKNOWN:
 		if required {
-			return nil, errors.New("an address type must be specified")
+			return nil, errors.New(


nit: for errors we'd format it similar to fmt.Errorf.

Thanks I missed this style fix. Will add.

itest/lnd_onchain_test.go

yyforyongyu · 2023-04-26T01:48:27Z

itest/lnd_onchain_test.go

+		MinConfs: 1,
+	}
+
+	// We try to send the reserve violating transaction...


Suggested change

// We try to send the reserve violating transaction...

// We try to send the reserve violating transaction.

yyforyongyu · 2023-04-26T01:56:37Z

lnrpc/walletrpc/walletkit_server.go

+
+	// We'll get the currently required reserve amount.
+	reserve, err := w.RequiredReserve(
+		ctx, &RequiredReserveRequest{AdditionalPublicChannels: 0},


hmm what if we need more public channels?

The AdditionalPublicChannels is just an RPC parameter that allows you to plan ahead if you intend to open more channels in the near future. Not really related to this feature, as we just need to check the reserve for the current number of channels, so just remove that parameter as the default is 0 anyway?

Cool got it, yeah make sense to use the default.

lnrpc/walletrpc/walletkit_server.go

yyforyongyu

Very close! Just a new nits in the itest.

yyforyongyu · 2023-04-28T06:09:15Z

itest/lnd_onchain_test.go

+	ht.EnsureConnected(alice, bob)
+
+	// We'll get the anchor reserve that is required for a single channel.
+	reserve, _ := alice.RPC.WalletKit.RequiredReserve(


Can we add RequiredReserve to wallet_kit.go and access it via alice.RPC.RequiredReserve to avoid checking the RPC error?

yyforyongyu · 2023-04-28T06:12:33Z

itest/lnd_onchain_test.go

+	}
+
+	// We try to send the reserve violating transaction.
+	_, err := alice.RPC.WalletKit.SendOutputs(context.Background(), req)


We need to use a timeout context here to avoid it hanging, tho almost will never happen, just in case. And we can use ht.Context() here.

yyforyongyu · 2023-04-28T06:13:10Z

itest/lnd_onchain_test.go

+	ht.MineBlocksAndAssertNumTxes(1, 0)
+
+	// We expect SendOutputs to return the respective error.
+	require.ErrorContains(ht, err, "the outputs to be send would leave "+


nit: move the error check next to the rpc call

yyforyongyu · 2023-04-28T06:14:26Z

itest/lnd_onchain_test.go

+	ht.MineBlocksAndAssertNumTxes(1, 1)
+
+	// This second transaction should be published correctly.
+	require.NoError(ht, err)


same here re moving the error check, so we can skip mining and assertion if there does return an error, otherwise we won't see the NoError fail because ht.MineBlocksAndAssertNumTxes will fail first.

guggero

LGTM, only nits 🎉

guggero · 2023-04-28T07:52:17Z

lnrpc/walletrpc/walletkit_server.go

+
+	// We'll get the currently required reserve amount.
+	reserve, err := w.RequiredReserve(
+		ctx, &RequiredReserveRequest{},


nit: can now fit on a single line.

guggero · 2023-04-28T07:52:46Z

lnrpc/walletrpc/walletkit_server.go

+	}
+
+	// Then we check if our current wallet balance undershoots the required
+	// reserve if we'd send out the outputs sepcified in the request.


nit: typo in the word "specified".

guggero · 2023-04-28T07:54:02Z

lnrpc/walletrpc/walletkit_server.go

+	// Then we check if our current wallet balance undershoots the required
+	// reserve if we'd send out the outputs sepcified in the request.
+	if int64(walletBalance)-totalOutputValue < reserve.RequiredReserve {
+		return nil, fmt.Errorf("the outputs to be send would leave " +


nit: s/send/sent/. Also, should we mention anchor channels? E.g. "would leave insufficient reserves for anchor channels".

guggero · 2023-04-28T07:55:21Z

lnrpc/walletrpc/walletkit_server.go

@@ -717,7 +719,8 @@ func (w *WalletKit) SendOutputs(ctx context.Context,
 	// requirement, we can request that the wallet attempts to create this
 	// transaction.
 	tx, err := w.cfg.Wallet.SendOutputs(
-		outputsToCreate, chainfee.SatPerKWeight(req.SatPerKw), minConfs, label,
+		outputsToCreate, chainfee.SatPerKWeight(req.SatPerKw), minConfs,


itest/lnd_onchain_test.go

guggero · 2023-04-28T07:58:00Z

itest/lnd_onchain_test.go

+	// Alice opens an anchor channel and keeps twice the amount of the
+	// anchor reserve in her wallet.
+	chanAmt := fundingAmount - 2*btcutil.Amount(reserve.RequiredReserve)
+	outpoint := ht.OpenChannel(


nit: could use the compact form here, since things seem to fit on one line:

outpoint := ht.OpenChannel(alice, bob, lntest.OpenChannelParams{ Amt: chanAmt, CommitmentType: lnrpc.CommitmentType_ANCHORS, SatPerVByte: 1, })

guggero · 2023-04-28T08:00:49Z

itest/lnd_onchain_test.go

+	ht.MineBlocksAndAssertNumTxes(1, 0)
+
+	// We expect SendOutputs to return the respective error.
+	require.ErrorContains(ht, err, "the outputs to be send would leave "+


Also, we should probably define this as an exported error, then we can string match on it here to shorten the line and also avoid braking the test when the message is updated.

lnrpc/walletrpc/errors.go

yyforyongyu

LGTM⛵️

hieblmi changed the title ~~Sendoutputs reserve~~ rpc: anchor reserve check for SendOutputs Apr 23, 2023

hieblmi force-pushed the sendoutputs-reserve branch 2 times, most recently from 16f47eb to 7e1fecf Compare April 23, 2023 18:29

hieblmi commented Apr 23, 2023

View reviewed changes

itest/list_on_test.go Outdated Show resolved Hide resolved

hieblmi force-pushed the sendoutputs-reserve branch from 7e1fecf to 142f991 Compare April 23, 2023 18:57

saubyk assigned hieblmi Apr 24, 2023

saubyk added this to the v0.17.0 milestone Apr 24, 2023

guggero self-requested a review April 24, 2023 08:40

hieblmi force-pushed the sendoutputs-reserve branch from 142f991 to da6206b Compare April 24, 2023 08:57

hieblmi commented Apr 24, 2023

View reviewed changes

lnrpc/walletrpc/walletkit_server.go Show resolved Hide resolved

yyforyongyu reviewed Apr 26, 2023

View reviewed changes

hieblmi force-pushed the sendoutputs-reserve branch from da6206b to 041f6bc Compare April 27, 2023 19:09

hieblmi requested a review from yyforyongyu April 27, 2023 19:14

yyforyongyu requested changes Apr 28, 2023

View reviewed changes

guggero approved these changes Apr 28, 2023

View reviewed changes

hieblmi force-pushed the sendoutputs-reserve branch 2 times, most recently from d1c64ea to 79a06d1 Compare April 29, 2023 20:44

hieblmi added 4 commits April 29, 2023 22:44

rpc: SendOutputs considers anchor reserve

903b5dd

rpc: fix code style

8f5910b

itest: anchor reserve test for SendOutputs

77322dd

docs: update release notes

79a06d1

hieblmi commented Apr 30, 2023

View reviewed changes

lnrpc/walletrpc/errors.go Show resolved Hide resolved

hieblmi requested a review from yyforyongyu April 30, 2023 09:33

yyforyongyu approved these changes May 4, 2023

View reviewed changes

guggero merged commit c6b736b into lightningnetwork:master May 4, 2023

guggero mentioned this pull request Aug 2, 2023

Account open allows for undercutting the lnd anchor reserve requirement. lightninglabs/pool#461

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rpc: anchor reserve check for `SendOutputs` #7631

rpc: anchor reserve check for `SendOutputs` #7631

hieblmi commented Apr 23, 2023

yyforyongyu Apr 26, 2023

hieblmi Apr 26, 2023

yyforyongyu Apr 26, 2023

yyforyongyu Apr 26, 2023

guggero Apr 26, 2023

yyforyongyu Apr 26, 2023

yyforyongyu left a comment

yyforyongyu Apr 28, 2023

yyforyongyu Apr 28, 2023

yyforyongyu Apr 28, 2023

yyforyongyu Apr 28, 2023

guggero left a comment

guggero Apr 28, 2023

guggero Apr 28, 2023

guggero Apr 28, 2023

guggero Apr 28, 2023

guggero Apr 28, 2023

guggero Apr 28, 2023

yyforyongyu left a comment

	// We try to send the reserve violating transaction...
	// We try to send the reserve violating transaction.

rpc: anchor reserve check for SendOutputs #7631

rpc: anchor reserve check for SendOutputs #7631

Conversation

hieblmi commented Apr 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yyforyongyu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guggero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yyforyongyu left a comment

Choose a reason for hiding this comment

rpc: anchor reserve check for `SendOutputs` #7631

rpc: anchor reserve check for `SendOutputs` #7631