From d7299802d4b23e6a8d3008df7e524705888b0628 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Wed, 23 Jun 2021 18:03:34 -0700 Subject: [PATCH 1/2] itest: extend testAnchorReservedValue to open additional channels We do this to trigger a bug that will be resolved in a follow-up commit. This bug prevents inbound legacy channels from being rejected by the recipient if they have at least one anchor channel already opened without an on-chain balance. --- lntest/itest/lnd_onchain_test.go | 54 ++++++++++++++++++++++++-------- 1 file changed, 41 insertions(+), 13 deletions(-) diff --git a/lntest/itest/lnd_onchain_test.go b/lntest/itest/lnd_onchain_test.go index b29ca3c4..5d5a7faf 100644 --- a/lntest/itest/lnd_onchain_test.go +++ b/lntest/itest/lnd_onchain_test.go @@ -180,7 +180,8 @@ func testAnchorReservedValue(net *lntest.NetworkHarness, t *harnessTest) { err := net.ConnectNodes(ctxt, alice, bob) require.NoError(t.t, err) - // Send just enough coins for Alice to open a channel without a change output. + // Send just enough coins for Alice to open a channel without a change + // output. const ( chanAmt = 1000000 feeEst = 8000 @@ -206,20 +207,45 @@ func testAnchorReservedValue(net *lntest.NetworkHarness, t *harnessTest) { // Alice opens a smaller channel. This works since it will have a // change output. ctxt, _ = context.WithTimeout(context.Background(), defaultTimeout) - aliceChanPoint := openChannelAndAssert( + aliceChanPoint1 := openChannelAndAssert( ctxt, t, net, alice, bob, lntest.OpenChannelParams{ - Amt: chanAmt / 2, + Amt: chanAmt / 4, }, ) - ctxt, _ = context.WithTimeout(ctxb, defaultTimeout) - err = alice.WaitForNetworkChannelOpen(ctxt, aliceChanPoint) - require.NoError(t.t, err) + // If Alice tries to open another anchor channel to Bob, Bob should not + // reject it as he is not contributing any funds. + aliceChanPoint2 := openChannelAndAssert( + ctxt, t, net, alice, bob, lntest.OpenChannelParams{ + Amt: chanAmt / 4, + }, + ) - ctxt, _ = context.WithTimeout(ctxb, defaultTimeout) - err = bob.WaitForNetworkChannelOpen(ctxt, aliceChanPoint) + // Similarly, if Alice tries to open a legacy channel to Bob, Bob should + // not reject it as he is not contributing any funds. We'll restart Bob + // to remove his support for anchors. + err = net.RestartNode(bob, nil) require.NoError(t.t, err) + ctxt, _ = context.WithTimeout(ctxb, defaultTimeout) + aliceChanPoint3 := openChannelAndAssert( + ctxt, t, net, alice, bob, lntest.OpenChannelParams{ + Amt: chanAmt / 4, + }, + ) + + chanPoints := []*lnrpc.ChannelPoint{ + aliceChanPoint1, aliceChanPoint2, aliceChanPoint3, + } + for _, chanPoint := range chanPoints { + ctxt, _ = context.WithTimeout(ctxb, defaultTimeout) + err = alice.WaitForNetworkChannelOpen(ctxt, chanPoint) + require.NoError(t.t, err) + + ctxt, _ = context.WithTimeout(ctxb, defaultTimeout) + err = bob.WaitForNetworkChannelOpen(ctxt, chanPoint) + require.NoError(t.t, err) + } // Alice tries to send all coins to an internal address. This is // allowed, since the final wallet balance will still be above the @@ -317,7 +343,9 @@ func testAnchorReservedValue(net *lntest.NetworkHarness, t *harnessTest) { // Alice closes channel, should now be allowed to send everything to an // external address. - closeChannelAndAssert(ctxt, t, net, alice, aliceChanPoint, false) + for _, chanPoint := range chanPoints { + closeChannelAndAssert(ctxt, t, net, alice, chanPoint, false) + } newBalance := waitForConfirmedBalance() if newBalance <= aliceBalance { @@ -339,11 +367,11 @@ func testAnchorReservedValue(net *lntest.NetworkHarness, t *harnessTest) { // generated above. block = mineBlocks(t, net, 1, 1)[0] - // The sweep transaction should have two inputs, the change output from - // the previous sweep, and the output from the coop closed channel. + // The sweep transaction should have four inputs, the change output from + // the previous sweep, and the outputs from the coop closed channels. sweepTx = block.Transactions[1] - if len(sweepTx.TxIn) != 2 { - t.Fatalf("expected 2 inputs instead have %v", len(sweepTx.TxIn)) + if len(sweepTx.TxIn) != 4 { + t.Fatalf("expected 4 inputs instead have %v", len(sweepTx.TxIn)) } // It should have a single output. From 6bbe79053535c39508e8aef396a33d0c68d14eed Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Wed, 23 Jun 2021 17:40:36 -0700 Subject: [PATCH 2/2] lnwallet: prevent anchor reserve enforcement on legacy inbound channel This commit aims to address a flaw in our anchor reserve enforcement logic in which an inbound "legacy" channel (i.e. a channel with a commitment type that precedes anchors) would be rejected by the recipient if they have at least one opened channel using the anchors commitment type and do not have enough on-chain funds to meet the anchors reserve. --- lnwallet/wallet.go | 103 +++++++++++++++++++++------------------------ 1 file changed, 47 insertions(+), 56 deletions(-) diff --git a/lnwallet/wallet.go b/lnwallet/wallet.go index 14318615..1b1b40b6 100644 --- a/lnwallet/wallet.go +++ b/lnwallet/wallet.go @@ -595,33 +595,11 @@ func (l *LightningWallet) PsbtFundingVerify(pendingChanID [32]byte, "reservation ID %v", pid) } - // Now the the PSBT has been verified, we can again check whether the - // value reserved for anchor fee bumping is respected. - numAnchors, err := l.currentNumAnchorChans() - if err != nil { - return err - } - - // If this commit type is an anchor channel we add that to our counter, - // but only if we are contributing funds to the channel. This is done - // to still allow incoming channels even though we have no UTXOs - // available, as in bootstrapping phases. We only count public - // channels. + // Now the the PSBT has been populated and verified, we can again check + // whether the value reserved for anchor fee bumping is respected. isPublic := pendingReservation.partialState.ChannelFlags&lnwire.FFAnnounceChannel != 0 - if pendingReservation.partialState.ChanType.HasAnchors() && - intent.LocalFundingAmt() > 0 && isPublic { - numAnchors++ - } - - // We check the reserve value again, this should already have been - // checked for regular FullIntents, but now the PSBT intent is also - // populated. - return l.WithCoinSelectLock(func() error { - _, err := l.CheckReservedValue( - intent.Inputs(), intent.Outputs(), numAnchors, - ) - return err - }) + hasAnchors := pendingReservation.partialState.ChanType.HasAnchors() + return l.enforceNewReservedValue(intent, isPublic, hasAnchors) } // PsbtFundingFinalize looks up a previously registered funding intent by its @@ -809,38 +787,15 @@ func (l *LightningWallet) handleFundingReserveRequest(req *InitFundingReserveMsg // Now that we have a funding intent, we'll check whether funding a // channel using it would violate our reserved value for anchor channel - // fee bumping. We first get our current number of anchor channels. - numAnchors, err := l.currentNumAnchorChans() - if err != nil { - fundingIntent.Cancel() - - req.err <- err - req.resp <- nil - return - } - - // If this commit type is an anchor channel we add that to our counter, - // but only if we are contributing funds to the channel. This is done - // to still allow incoming channels even though we have no UTXOs - // available, as in bootstrapping phases. We only count public - // channels. - isPublic := req.Flags&lnwire.FFAnnounceChannel != 0 - if req.CommitType == CommitmentTypeAnchorsZeroFeeHtlcTx && - fundingIntent.LocalFundingAmt() > 0 && isPublic { - numAnchors++ - } - + // fee bumping. + // // Check the reserved value using the inputs and outputs given by the - // intent. Not that for the PSBT intent type we don't yet have the - // funding tx ready, so this will always pass. We'll do another check + // intent. Note that for the PSBT intent type we don't yet have the + // funding tx ready, so this will always pass. We'll do another check // when the PSBT has been verified. - err = l.WithCoinSelectLock(func() error { - _, err := l.CheckReservedValue( - fundingIntent.Inputs(), fundingIntent.Outputs(), - numAnchors, - ) - return err - }) + isPublic := req.Flags&lnwire.FFAnnounceChannel != 0 + hasAnchors := req.CommitType == CommitmentTypeAnchorsZeroFeeHtlcTx + err = l.enforceNewReservedValue(fundingIntent, isPublic, hasAnchors) if err != nil { fundingIntent.Cancel() @@ -893,6 +848,42 @@ func (l *LightningWallet) handleFundingReserveRequest(req *InitFundingReserveMsg req.err <- nil } +// enforceReservedValue enforces that the wallet, upon a new channel being +// opened, meets the minimum amount of funds required for each advertised anchor +// channel. +// +// We only enforce the reserve if we are contributing funds to the channel. This +// is done to still allow incoming channels even though we have no UTXOs +// available, as in bootstrapping phases. +func (l *LightningWallet) enforceNewReservedValue(fundingIntent chanfunding.Intent, + isPublic, hasAnchors bool) error { + + // Only enforce the reserve when an advertised channel is being opened + // in which we are contributing funds to. This ensures we never dip + // below the reserve. + if !isPublic || fundingIntent.LocalFundingAmt() == 0 { + return nil + } + + numAnchors, err := l.currentNumAnchorChans() + if err != nil { + return err + } + + // Add the to-be-opened channel. + if hasAnchors { + numAnchors++ + } + + return l.WithCoinSelectLock(func() error { + _, err := l.CheckReservedValue( + fundingIntent.Inputs(), fundingIntent.Outputs(), + numAnchors, + ) + return err + }) +} + // currentNumAnchorChans returns the current number of non-private anchor // channels the wallet should be ready to fee bump if needed. func (l *LightningWallet) currentNumAnchorChans() (int, error) {