[bug]: `locked_balance` in `WalletBalanceResponse` only counts "leased outputs", not "locked outpoints"

[aakselrod]

trafficstars

Background

OpenChannel and BatchOpenChannel (via FundPsbt) lock coins differently. OpenChannel uses LockOutpoint and FundPsbt uses LeaseOutput.

While outputs reserved by LeaseOutput are surfaced in the ListLeases walletrpc endpoint and counted as locked_balance in the WalletBalance result, ones reserved by LockOutpoint aren't surfaced anywhere in the gRPC API.

This means that when using BatchOpenChannel and constructing a funding transaction, if a potential counterparty delays sending a signed commitment to open the channel, it's possible to see the total amount reserved to the channel opening attempt and which outputs are part of that reservation. However, when using OpenChannel, this data is invisible: it doesn't show up in the response to WalletBalance or ListUnspent or even ListLeases. It looks like the balance has disappeared until the attempt times out or succeeds, after which the balance reappears.

In our environment, I've built a temporary workaround by adding a volatile_locked_balance field in WalletBalanceResponse to count the coins locked by LockOutpoint (and therefore by OpenChannel). I'm happy to upstream this if desired, or you may want to choose a different approach, like including these in the regular locked_balance field.

We are also planning on (soon-ish) adding a workaround ListLockedOutpoints RPC endpoint, which I'll also be happy to upstream if you don't choose a different path, such as including these outpoints in ListLeases and somehow signaling that they're volatile/have no lazy expiration.

Your environment

• version of lnd: any recent
• which operating system (uname -a on *Nix): any
• version of btcd, bitcoind, or other backend: any
• any other relevant environment details: none

Steps to reproduce

An integration test similar to:

func testVolatileLockedBalance(ht *lntest.HarnessTest) {
	// Before we start the test, we'll ensure both sides are connected so
	// the funding flow can be properly executed.
	alice := ht.Alice
	bob := ht.Bob
	ht.EnsureConnected(alice, bob)

	// We first make sure Alice has no locked wallet balance.
	balance := alice.RPC.WalletBalance()
	require.Equal(ht, int64(0), balance.LockedBalance)

	// Next, we register a ChannelAcceptor on Bob. This way, we can get
	// Alice's wallet balance after coin selection is done and outpoints
	// are locked.
	stream, cancel := bob.RPC.ChannelAcceptor()
	defer cancel()

	// Then, we request creation of a channel from Alice to Bob. We don't
	// do it synchronously since we want to receive Bob's message in the
	// same goroutine.
	openChannelReq := &lnrpc.OpenChannelRequest{
		NodePubkey:         bob.PubKey[:],
		LocalFundingAmount: int64(funding.MaxBtcFundingAmount),
	}
	_ = alice.RPC.OpenChannel(openChannelReq)

	// After that, we receive the request on Bob's side, get the wallet
	// balance from Alice, and ensure it's non-zero.
	req, err := stream.Recv()
	require.NoError(ht, err)

	balance = alice.RPC.WalletBalance()
	require.NotEqual(ht, int64(0), balance.LockedBalance) // This fails because balance locked by LockOutpoint isn't counted

	// Next, we let Bob deny the request.
	resp := &lnrpc.ChannelAcceptResponse{
		Accept:        false,
		PendingChanId: req.PendingChanId,
	}
	err = stream.Send(resp)
	require.NoError(ht, err)

	// Finally, we check to make sure the balance is unlocked again.
	balance = alice.RPC.WalletBalance()
	require.Equal(ht, int64(0), balance.LockedBalance)
}

For our workaround, the balance.LockedBalance is replaced by balance.VolatileLockedBalance and the test is made to pass by populating that field with data from (*Wallet) LockedOutpoints().

Expected behaviour

Test should pass, or there should be another way to access this information

Actual behaviour

Test fails

[guggero]

Oh wow, I wasn't even aware there were two mechanisms for locking. Since LockOutpoint only does in-memory locking, perhaps we should instead refactor things to not use that anymore at all? @Roasbeef I assume this is a legacy thing where LockOutpoint was built first and then the persistent LeaseOutput mechanism was added that survives restarts. Any reason not to move all code over from LockOutpoint to LeaseOutput?

[Roasbeef]

LockOutpoint was built first and then the persistent LeaseOutput mechanism was added that survives restarts. Any reason not to move all code over from LockOutpoint to LeaseOutput?

Hmm yeah, LockOutpoint was added originally just to avoid double spends within the wallet itself. Later on once we started to open things up to using lnd as a general wallet, we added leasing, since outside programs may be constructing and broadcasting their own transactions.

This is also related to: https://github.com/lightningnetwork/lnd/issues/7868

[aakselrod]

Moving from LockOutpoint to LeaseOutput everywhere would definitely make things easier from our side. The balances and UTXOs would already be included everywhere we'd expect them.