diff --git a/contractcourt/chain_arbitrator.go b/contractcourt/chain_arbitrator.go index a8be6a42..453f292b 100644 --- a/contractcourt/chain_arbitrator.go +++ b/contractcourt/chain_arbitrator.go @@ -241,7 +241,7 @@ type arbChannel struct { // commitment transactions. // // NOTE: Part of the ArbChannel interface. -func (a *arbChannel) NewAnchorResolutions() ([]*lnwallet.AnchorResolution, +func (a *arbChannel) NewAnchorResolutions() (*lnwallet.AnchorResolutions, error) { // Get a fresh copy of the database state to base the anchor resolutions diff --git a/contractcourt/channel_arbitrator.go b/contractcourt/channel_arbitrator.go index 6d58ef20..5f4cb31d 100644 --- a/contractcourt/channel_arbitrator.go +++ b/contractcourt/channel_arbitrator.go @@ -4,6 +4,7 @@ import ( "bytes" "errors" "fmt" + "math" "sync" "sync/atomic" "time" @@ -31,8 +32,10 @@ var ( const ( // anchorSweepConfTarget is the conf target used when sweeping - // commitment anchors. - anchorSweepConfTarget = 6 + // commitment anchors. This value is only used when the commitment + // transaction has no valid HTLCs for determining a confirmation + // deadline. + anchorSweepConfTarget = 144 // arbitratorBlockBufferSize is the size of the buffer we give to each // channel arbitrator. @@ -91,7 +94,7 @@ type ArbChannel interface { // NewAnchorResolutions returns the anchor resolutions for currently // valid commitment transactions. - NewAnchorResolutions() ([]*lnwallet.AnchorResolution, error) + NewAnchorResolutions() (*lnwallet.AnchorResolutions, error) } // ChannelArbitratorConfig contains all the functionality that the @@ -1087,14 +1090,24 @@ func (c *ChannelArbitrator) stateStep( // sweepAnchors offers all given anchor resolutions to the sweeper. It requests // sweeping at the minimum fee rate. This fee rate can be upped manually by the // user via the BumpFee rpc. -func (c *ChannelArbitrator) sweepAnchors(anchors []*lnwallet.AnchorResolution, +func (c *ChannelArbitrator) sweepAnchors(anchors *lnwallet.AnchorResolutions, heightHint uint32) error { // Use the chan id as the exclusive group. This prevents any of the // anchors from being batched together. exclusiveGroup := c.cfg.ShortChanID.ToUint64() - for _, anchor := range anchors { + // sweepWithDeadline is a helper closure that takes an anchor + // resolution and sweeps it with its corresponding deadline. + sweepWithDeadline := func(anchor *lnwallet.AnchorResolution, + htlcs htlcSet) error { + + // Find the deadline for this specific anchor. + deadline, err := c.findCommitmentDeadline(heightHint, htlcs) + if err != nil { + return err + } + log.Debugf("ChannelArbitrator(%v): pre-confirmation sweep of "+ "anchor of tx %v", c.cfg.ChanPoint, anchor.CommitAnchor) @@ -1118,11 +1131,11 @@ func (c *ChannelArbitrator) sweepAnchors(anchors []*lnwallet.AnchorResolution, // Also signal that this is a force sweep, so that the anchor // will be swept even if it isn't economical purely based on the // anchor value. - _, err := c.cfg.Sweeper.SweepInput( + _, err = c.cfg.Sweeper.SweepInput( &anchorInput, sweep.Params{ Fee: sweep.FeePreference{ - ConfTarget: anchorSweepConfTarget, + ConfTarget: deadline, }, Force: true, ExclusiveGroup: &exclusiveGroup, @@ -1131,11 +1144,129 @@ func (c *ChannelArbitrator) sweepAnchors(anchors []*lnwallet.AnchorResolution, if err != nil { return err } + + return nil + } + + // Sweep anchors based on different HTLC sets. Notice the HTLC sets may + // differ across commitments, thus their deadline values could vary. + for htlcSet, htlcs := range c.activeHTLCs { + switch { + case htlcSet == LocalHtlcSet && anchors.Local != nil: + err := sweepWithDeadline(anchors.Local, htlcs) + if err != nil { + return err + } + + case htlcSet == RemoteHtlcSet && anchors.Remote != nil: + err := sweepWithDeadline(anchors.Remote, htlcs) + if err != nil { + return err + } + + case htlcSet == RemotePendingHtlcSet && + anchors.RemotePending != nil: + + err := sweepWithDeadline(anchors.RemotePending, htlcs) + if err != nil { + return err + } + } } return nil } +// findCommitmentDeadline finds the deadline (relative block height) for a +// commitment transaction by extracting the minimum CLTV from its HTLCs. From +// our PoV, the deadline is defined to be the smaller of, +// - the least CLTV from outgoing HTLCs, or, +// - the least CLTV from incoming HTLCs if the preimage is available. +// +// Note: when the deadline turns out to be 0 blocks, we will replace it with 1 +// block because our fee estimator doesn't allow a 0 conf target. This also +// means we've left behind and should increase our fee to make the transaction +// confirmed asap. +func (c *ChannelArbitrator) findCommitmentDeadline(heightHint uint32, + htlcs htlcSet) (uint32, error) { + + deadlineMinHeight := uint32(math.MaxUint32) + + // First, iterate through the outgoingHTLCs to find the lowest CLTV + // value. + for _, htlc := range htlcs.outgoingHTLCs { + // Skip if the HTLC is dust. + if htlc.OutputIndex < 0 { + log.Debugf("ChannelArbitrator(%v): skipped deadline "+ + "for dust htlc=%x", + c.cfg.ChanPoint, htlc.RHash[:]) + + continue + } + + if htlc.RefundTimeout < deadlineMinHeight { + deadlineMinHeight = htlc.RefundTimeout + } + } + + // Then going through the incomingHTLCs, and update the minHeight when + // conditions met. + for _, htlc := range htlcs.incomingHTLCs { + // Skip if the HTLC is dust. + if htlc.OutputIndex < 0 { + log.Debugf("ChannelArbitrator(%v): skipped deadline "+ + "for dust htlc=%x", + c.cfg.ChanPoint, htlc.RHash[:]) + + continue + } + + // Since it's an HTLC sent to us, check if we have preimage for + // this HTLC. + preimageAvailable, err := c.isPreimageAvailable(htlc.RHash) + if err != nil { + return 0, err + } + + if !preimageAvailable { + continue + } + + if htlc.RefundTimeout < deadlineMinHeight { + deadlineMinHeight = htlc.RefundTimeout + } + } + + // Calculate the deadline. There are two cases to be handled here, + // - when the deadlineMinHeight never gets updated, which could + // happen when we have no outgoing HTLCs, and, for incoming HTLCs, + // * either we have none, or, + // * none of the HTLCs are preimageAvailable. + // - when our deadlineMinHeight is no greater than the heightHint, + // which means we are behind our schedule. + deadline := deadlineMinHeight - heightHint + switch { + // When we couldn't find a deadline height from our HTLCs, we will fall + // back to the default value. + case deadlineMinHeight == math.MaxUint32: + deadline = anchorSweepConfTarget + + // When the deadline is passed, we will fall back to the smallest conf + // target (1 block). + case deadlineMinHeight <= heightHint: + log.Warnf("ChannelArbitrator(%v): deadline is passed with "+ + "deadlineMinHeight=%d, heightHint=%d", + c.cfg.ChanPoint, deadlineMinHeight, heightHint) + deadline = 1 + } + + log.Debugf("ChannelArbitrator(%v): calculated deadline: %d, "+ + "using deadlineMinHeight=%d, heightHint=%d", + c.cfg.ChanPoint, deadline, deadlineMinHeight, heightHint) + + return deadline, nil +} + // launchResolvers updates the activeResolvers list and starts the resolvers. func (c *ChannelArbitrator) launchResolvers(resolvers []ContractResolver) { c.activeResolversLock.Lock() diff --git a/contractcourt/channel_arbitrator_test.go b/contractcourt/channel_arbitrator_test.go index fa3f04ef..34eab1e3 100644 --- a/contractcourt/channel_arbitrator_test.go +++ b/contractcourt/channel_arbitrator_test.go @@ -7,6 +7,7 @@ import ( "os" "path/filepath" "reflect" + "sort" "sync" "testing" "time" @@ -20,8 +21,10 @@ import ( "github.com/lightningnetwork/lnd/input" "github.com/lightningnetwork/lnd/kvdb" "github.com/lightningnetwork/lnd/lntest/mock" + "github.com/lightningnetwork/lnd/lntypes" "github.com/lightningnetwork/lnd/lnwallet" "github.com/lightningnetwork/lnd/lnwire" + "github.com/stretchr/testify/require" ) const ( @@ -2093,6 +2096,254 @@ func TestRemoteCloseInitiator(t *testing.T) { } } +// TestFindCommitmentDeadline tests the logic used to determine confirmation +// deadline is implemented as expected. +func TestFindCommitmentDeadline(t *testing.T) { + // Create a testing channel arbitrator. + log := &mockArbitratorLog{ + state: StateDefault, + newStates: make(chan ArbitratorState, 5), + } + chanArbCtx, err := createTestChannelArbitrator(t, log) + require.NoError(t, err, "unable to create ChannelArbitrator") + + // Add a dummy payment hash to the preimage lookup. + rHash := [lntypes.PreimageSize]byte{1, 2, 3} + mockPreimageDB := newMockWitnessBeacon() + mockPreimageDB.lookupPreimage[rHash] = rHash + + // Attack a mock PreimageDB and Registry to channel arbitrator. + chanArb := chanArbCtx.chanArb + chanArb.cfg.PreimageDB = mockPreimageDB + chanArb.cfg.Registry = &mockRegistry{} + + htlcIndexBase := uint64(99) + heightHint := uint32(1000) + htlcExpiryBase := heightHint + uint32(10) + + // Create four testing HTLCs. + htlcDust := channeldb.HTLC{ + HtlcIndex: htlcIndexBase + 1, + RefundTimeout: htlcExpiryBase + 1, + OutputIndex: -1, + } + htlcSmallExipry := channeldb.HTLC{ + HtlcIndex: htlcIndexBase + 2, + RefundTimeout: htlcExpiryBase + 2, + } + + htlcPreimage := channeldb.HTLC{ + HtlcIndex: htlcIndexBase + 3, + RefundTimeout: htlcExpiryBase + 3, + RHash: rHash, + } + htlcLargeExpiry := channeldb.HTLC{ + HtlcIndex: htlcIndexBase + 4, + RefundTimeout: htlcExpiryBase + 100, + } + htlcExpired := channeldb.HTLC{ + HtlcIndex: htlcIndexBase + 5, + RefundTimeout: heightHint, + } + + makeHTLCSet := func(incoming, outgoing channeldb.HTLC) htlcSet { + return htlcSet{ + incomingHTLCs: map[uint64]channeldb.HTLC{ + incoming.HtlcIndex: incoming, + }, + outgoingHTLCs: map[uint64]channeldb.HTLC{ + outgoing.HtlcIndex: outgoing, + }, + } + } + + testCases := []struct { + name string + htlcs htlcSet + err error + deadline uint32 + }{ + { + // When we have no HTLCs, the default value should be + // used. + name: "use default conf target", + htlcs: htlcSet{}, + err: nil, + deadline: anchorSweepConfTarget, + }, + { + // When we have a preimage available in the local HTLC + // set, its CLTV should be used. + name: "use htlc with preimage available", + htlcs: makeHTLCSet(htlcPreimage, htlcLargeExpiry), + err: nil, + deadline: htlcPreimage.RefundTimeout - heightHint, + }, + { + // When the HTLC in the local set is not preimage + // available, we should not use its CLTV even its value + // is smaller. + name: "use htlc with no preimage available", + htlcs: makeHTLCSet(htlcSmallExipry, htlcLargeExpiry), + err: nil, + deadline: htlcLargeExpiry.RefundTimeout - heightHint, + }, + { + // When we have dust HTLCs, their CLTVs should NOT be + // used even the values are smaller. + name: "ignore dust HTLCs", + htlcs: makeHTLCSet(htlcPreimage, htlcDust), + err: nil, + deadline: htlcPreimage.RefundTimeout - heightHint, + }, + { + // When we've reached our deadline, use conf target of + // 1 as our deadline. + name: "use conf target 1", + htlcs: makeHTLCSet(htlcPreimage, htlcExpired), + err: nil, + deadline: 1, + }, + } + + for _, tc := range testCases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + deadline, err := chanArb.findCommitmentDeadline( + heightHint, tc.htlcs, + ) + + require.Equal(t, tc.err, err) + require.Equal(t, tc.deadline, deadline) + }) + } + +} + +// TestSweepAnchors checks the sweep transactions are created using the +// expected deadlines for different anchor resolutions. +func TestSweepAnchors(t *testing.T) { + // Create a testing channel arbitrator. + log := &mockArbitratorLog{ + state: StateDefault, + newStates: make(chan ArbitratorState, 5), + } + chanArbCtx, err := createTestChannelArbitrator(t, log) + require.NoError(t, err, "unable to create ChannelArbitrator") + + // Add a dummy payment hash to the preimage lookup. + rHash := [lntypes.PreimageSize]byte{1, 2, 3} + mockPreimageDB := newMockWitnessBeacon() + mockPreimageDB.lookupPreimage[rHash] = rHash + + // Attack a mock PreimageDB and Registry to channel arbitrator. + chanArb := chanArbCtx.chanArb + chanArb.cfg.PreimageDB = mockPreimageDB + chanArb.cfg.Registry = &mockRegistry{} + + // Set current block height. + heightHint := uint32(1000) + chanArbCtx.chanArb.blocks <- int32(heightHint) + + htlcIndexBase := uint64(99) + htlcExpiryBase := heightHint + uint32(10) + + // Create three testing HTLCs. + htlcDust := channeldb.HTLC{ + HtlcIndex: htlcIndexBase + 1, + RefundTimeout: htlcExpiryBase + 1, + OutputIndex: -1, + } + htlcWithPreimage := channeldb.HTLC{ + HtlcIndex: htlcIndexBase + 2, + RefundTimeout: htlcExpiryBase + 2, + RHash: rHash, + } + htlcSmallExipry := channeldb.HTLC{ + HtlcIndex: htlcIndexBase + 3, + RefundTimeout: htlcExpiryBase + 3, + } + + // Setup our local HTLC set such that we will use the HTLC's CLTV from + // the incoming HTLC set. + expectedLocalDeadline := htlcWithPreimage.RefundTimeout - heightHint + chanArb.activeHTLCs[LocalHtlcSet] = htlcSet{ + incomingHTLCs: map[uint64]channeldb.HTLC{ + htlcWithPreimage.HtlcIndex: htlcWithPreimage, + }, + outgoingHTLCs: map[uint64]channeldb.HTLC{ + htlcDust.HtlcIndex: htlcDust, + }, + } + + // Setup our remote HTLC set such that no valid HTLCs can be used, thus + // we default to anchorSweepConfTarget. + expectedRemoteDeadline := anchorSweepConfTarget + chanArb.activeHTLCs[RemoteHtlcSet] = htlcSet{ + incomingHTLCs: map[uint64]channeldb.HTLC{ + htlcSmallExipry.HtlcIndex: htlcSmallExipry, + }, + outgoingHTLCs: map[uint64]channeldb.HTLC{ + htlcDust.HtlcIndex: htlcDust, + }, + } + + // Setup out pending remote HTLC set such that we will use the HTLC's + // CLTV from the outgoing HTLC set. + expectedPendingDeadline := htlcSmallExipry.RefundTimeout - heightHint + chanArb.activeHTLCs[RemotePendingHtlcSet] = htlcSet{ + incomingHTLCs: map[uint64]channeldb.HTLC{ + htlcDust.HtlcIndex: htlcDust, + }, + outgoingHTLCs: map[uint64]channeldb.HTLC{ + htlcSmallExipry.HtlcIndex: htlcSmallExipry, + }, + } + + // Create AnchorResolutions. + anchors := &lnwallet.AnchorResolutions{ + Local: &lnwallet.AnchorResolution{ + AnchorSignDescriptor: input.SignDescriptor{ + Output: &wire.TxOut{Value: 1}, + }, + }, + Remote: &lnwallet.AnchorResolution{ + AnchorSignDescriptor: input.SignDescriptor{ + Output: &wire.TxOut{Value: 1}, + }, + }, + RemotePending: &lnwallet.AnchorResolution{ + AnchorSignDescriptor: input.SignDescriptor{ + Output: &wire.TxOut{Value: 1}, + }, + }, + } + + // Sweep anchors and check there's no error. + err = chanArb.sweepAnchors(anchors, heightHint) + require.NoError(t, err) + + // Verify deadlines are used as expected. + deadlines := chanArbCtx.sweeper.deadlines + // Since there's no guarantee of the deadline orders, we sort it here + // so they can be compared. + sort.Ints(deadlines) // [12, 13, 144] + require.EqualValues( + t, expectedLocalDeadline, deadlines[0], + "local deadline not matched", + ) + require.EqualValues( + t, expectedPendingDeadline, deadlines[1], + "pending remote deadline not matched", + ) + require.EqualValues( + t, expectedRemoteDeadline, deadlines[2], + "remote deadline not matched", + ) + +} + // TestChannelArbitratorAnchors asserts that the commitment tx anchor is swept. func TestChannelArbitratorAnchors(t *testing.T) { log := &mockArbitratorLog{ @@ -2113,14 +2364,29 @@ func TestChannelArbitratorAnchors(t *testing.T) { reports, ) + // Add a dummy payment hash to the preimage lookup. + rHash := [lntypes.PreimageSize]byte{1, 2, 3} + mockPreimageDB := newMockWitnessBeacon() + mockPreimageDB.lookupPreimage[rHash] = rHash + + // Attack a mock PreimageDB and Registry to channel arbitrator. chanArb := chanArbCtx.chanArb - chanArb.cfg.PreimageDB = newMockWitnessBeacon() + chanArb.cfg.PreimageDB = mockPreimageDB chanArb.cfg.Registry = &mockRegistry{} // Setup two pre-confirmation anchor resolutions on the mock channel. chanArb.cfg.Channel.(*mockChannel).anchorResolutions = - []*lnwallet.AnchorResolution{ - {}, {}, + &lnwallet.AnchorResolutions{ + Local: &lnwallet.AnchorResolution{ + AnchorSignDescriptor: input.SignDescriptor{ + Output: &wire.TxOut{Value: 1}, + }, + }, + Remote: &lnwallet.AnchorResolution{ + AnchorSignDescriptor: input.SignDescriptor{ + Output: &wire.TxOut{Value: 1}, + }, + }, } if err := chanArb.Start(nil); err != nil { @@ -2141,6 +2407,41 @@ func TestChannelArbitratorAnchors(t *testing.T) { } chanArb.UpdateContractSignals(signals) + // Set current block height. + heightHint := uint32(1000) + chanArbCtx.chanArb.blocks <- int32(heightHint) + + // Create testing HTLCs. + htlcExpiryBase := heightHint + uint32(10) + htlcWithPreimage := channeldb.HTLC{ + HtlcIndex: 99, + RefundTimeout: htlcExpiryBase + 2, + RHash: rHash, + Incoming: true, + } + htlc := channeldb.HTLC{ + HtlcIndex: 100, + RefundTimeout: htlcExpiryBase + 3, + } + + // We now send two HTLC updates, one for local HTLC set and the other + // for remote HTLC set. + htlcUpdates <- &ContractUpdate{ + HtlcKey: LocalHtlcSet, + // This will make the deadline of the local anchor resolution + // to be htlcWithPreimage's CLTV minus heightHint since the + // incoming HTLC (toLocalHTLCs) has a lower CLTV value and is + // preimage available. + Htlcs: []channeldb.HTLC{htlc, htlcWithPreimage}, + } + htlcUpdates <- &ContractUpdate{ + HtlcKey: RemoteHtlcSet, + // This will make the deadline of the remote anchor resolution + // to be htlcWithPreimage's CLTV minus heightHint because the + // incoming HTLC (toRemoteHTLCs) has a lower CLTV. + Htlcs: []channeldb.HTLC{htlc, htlcWithPreimage}, + } + errChan := make(chan error, 1) respChan := make(chan *wire.MsgTx, 1) @@ -2256,6 +2557,20 @@ func TestChannelArbitratorAnchors(t *testing.T) { } assertResolverReport(t, reports, expectedReport) + + // We expect two anchor inputs, the local and the remote to be swept. + // Thus we should expect there are two deadlines used, both are equal + // to htlcWithPreimage's CLTV minus current block height. + require.Equal(t, 2, len(chanArbCtx.sweeper.deadlines)) + require.EqualValues(t, + htlcWithPreimage.RefundTimeout-heightHint, + chanArbCtx.sweeper.deadlines[0], + ) + require.EqualValues(t, + htlcWithPreimage.RefundTimeout-heightHint, + chanArbCtx.sweeper.deadlines[1], + ) + } // putResolverReportInChannel returns a put report function which will pipe @@ -2286,13 +2601,16 @@ func assertResolverReport(t *testing.T, reports chan *channeldb.ResolverReport, } type mockChannel struct { - anchorResolutions []*lnwallet.AnchorResolution + anchorResolutions *lnwallet.AnchorResolutions } -func (m *mockChannel) NewAnchorResolutions() ([]*lnwallet.AnchorResolution, +func (m *mockChannel) NewAnchorResolutions() (*lnwallet.AnchorResolutions, error) { + if m.anchorResolutions != nil { + return m.anchorResolutions, nil + } - return m.anchorResolutions, nil + return &lnwallet.AnchorResolutions{}, nil } func (m *mockChannel) ForceCloseChan() (*lnwallet.LocalForceCloseSummary, error) { diff --git a/contractcourt/commit_sweep_resolver_test.go b/contractcourt/commit_sweep_resolver_test.go index 37c0fe59..1321c032 100644 --- a/contractcourt/commit_sweep_resolver_test.go +++ b/contractcourt/commit_sweep_resolver_test.go @@ -108,14 +108,17 @@ type mockSweeper struct { sweepTx *wire.MsgTx sweepErr error createSweepTxChan chan *wire.MsgTx + + deadlines []int } func newMockSweeper() *mockSweeper { return &mockSweeper{ - sweptInputs: make(chan input.Input), + sweptInputs: make(chan input.Input, 3), updatedInputs: make(chan wire.OutPoint), sweepTx: &wire.MsgTx{}, createSweepTxChan: make(chan *wire.MsgTx), + deadlines: []int{}, } } @@ -124,6 +127,11 @@ func (s *mockSweeper) SweepInput(input input.Input, params sweep.Params) ( s.sweptInputs <- input + // Update the deadlines used if it's set. + if params.Fee.ConfTarget != 0 { + s.deadlines = append(s.deadlines, int(params.Fee.ConfTarget)) + } + result := make(chan sweep.Result, 1) result <- sweep.Result{ Tx: s.sweepTx, diff --git a/docs/release-notes/release-notes-0.13.1.md b/docs/release-notes/release-notes-0.13.1.md index 6720c6a6..46382cb1 100644 --- a/docs/release-notes/release-notes-0.13.1.md +++ b/docs/release-notes/release-notes-0.13.1.md @@ -37,6 +37,17 @@ The [`monitoring` build tag is now on by default](https://github.com/lightningnetwork/lnd/pull/5399) for all routine builds. +## Deadline Aware in Anchor Sweeping +Anchor sweeping is now [deadline +aware](https://github.com/lightningnetwork/lnd/pull/5148). Previously, all +anchor sweepings use a default conf target of 6, which is likely to cause +overpaying miner fees since the CLTV values of the HTLCs are far in the future. +Brought by this update, the anchor sweeping (particularly local force close) +will construct a deadline from its set of HTLCs, and use it as the conf target +when estimating miner fees. The previous default conf target 6 is now changed +to 144, and it's only used when there are no eligible HTLCs for deadline +construction. + ## Bug Fixes An optimization intended to speed up the payment critical path by diff --git a/lntest/fee_service.go b/lntest/fee_service.go index 4f61649c..f8dbac21 100644 --- a/lntest/fee_service.go +++ b/lntest/fee_service.go @@ -15,7 +15,7 @@ const ( // feeServiceTarget is the confirmation target for which a fee estimate // is returned. Requests for higher confirmation targets will fall back // to this. - feeServiceTarget = 2 + feeServiceTarget = 1 ) // feeService runs a web service that provides fee estimation information. @@ -100,3 +100,11 @@ func (f *feeService) setFee(fee chainfee.SatPerKWeight) { f.Fees[feeServiceTarget] = uint32(fee.FeePerKVByte()) } + +// setFeeWithConf sets a fee for the given confirmation target. +func (f *feeService) setFeeWithConf(fee chainfee.SatPerKWeight, conf uint32) { + f.lock.Lock() + defer f.lock.Unlock() + + f.Fees[conf] = uint32(fee.FeePerKVByte()) +} diff --git a/lntest/fee_service_test.go b/lntest/fee_service_test.go index c7ad38c4..c8c0ca89 100644 --- a/lntest/fee_service_test.go +++ b/lntest/fee_service_test.go @@ -34,6 +34,6 @@ func TestFeeService(t *testing.T) { require.NoError(t, err) require.Equal( - t, "{\"fee_by_block_target\":{\"2\":20000}}", string(body), + t, "{\"fee_by_block_target\":{\"1\":20000}}", string(body), ) } diff --git a/lntest/harness.go b/lntest/harness.go index d6885ca1..8ba49559 100644 --- a/lntest/harness.go +++ b/lntest/harness.go @@ -1512,6 +1512,12 @@ func (n *NetworkHarness) SetFeeEstimate(fee chainfee.SatPerKWeight) { n.feeService.setFee(fee) } +func (n *NetworkHarness) SetFeeEstimateWithConf( + fee chainfee.SatPerKWeight, conf uint32) { + + n.feeService.setFeeWithConf(fee, conf) +} + // CopyFile copies the file src to dest. func CopyFile(dest, src string) error { s, err := os.Open(src) diff --git a/lntest/itest/lnd_channel_force_close.go b/lntest/itest/lnd_channel_force_close.go new file mode 100644 index 00000000..aa389c18 --- /dev/null +++ b/lntest/itest/lnd_channel_force_close.go @@ -0,0 +1,222 @@ +package itest + +import ( + "context" + "testing" + + "github.com/btcsuite/btcd/blockchain" + "github.com/btcsuite/btcd/integration/rpctest" + "github.com/btcsuite/btcd/wire" + "github.com/btcsuite/btcutil" + "github.com/lightningnetwork/lnd/lnrpc" + "github.com/lightningnetwork/lnd/lnrpc/routerrpc" + "github.com/lightningnetwork/lnd/lntest" + "github.com/lightningnetwork/lnd/lntest/wait" + "github.com/lightningnetwork/lnd/lnwallet" + "github.com/lightningnetwork/lnd/lnwallet/chainfee" + "github.com/lightningnetwork/lnd/routing" + "github.com/stretchr/testify/require" +) + +// TODO(yy): move channel force closed related tests into this file. + +// testCommitmentTransactionDeadline tests that the anchor sweep transaction is +// taking account of the deadline of the commitment transaction. It tests two +// scenarios: +// 1) when the CPFP is skipped, checks that the deadline is not used. +// 2) when the CPFP is used, checks that the deadline is applied. +// Note that whether the deadline is used or not is implicitly checked by its +// corresponding fee rates. +func testCommitmentTransactionDeadline(net *lntest.NetworkHarness, + t *harnessTest) { + + // Get the default max fee rate used in sweeping the commitment + // transaction. + defaultMax := lnwallet.DefaultAnchorsCommitMaxFeeRateSatPerVByte + maxPerKw := chainfee.SatPerKVByte(defaultMax * 1000).FeePerKWeight() + + const ( + // feeRateConfDefault(sat/kw) is used when no conf target is + // set. This value will be returned by the fee estimator but + // won't be used because our commitment fee rate is capped by + // DefaultAnchorsCommitMaxFeeRateSatPerVByte. + feeRateDefault = 20000 + + // finalCTLV is used when Alice sends payment to Bob. + finalCTLV = 144 + + // deadline is used when Alice sweep the anchor. Notice there + // is a block padding of 3 added, such that the value of + // deadline is 147. + deadline = uint32(finalCTLV + routing.BlockPadding) + ) + + // feeRateSmall(sat/kw) is used when we want to skip the CPFP + // on anchor transactions. When the fee rate is smaller than + // the parent's (commitment transaction) fee rate, the CPFP + // will be skipped. Atm, the parent tx's fee rate is roughly + // 2500 sat/kw in this test. + feeRateSmall := maxPerKw / 2 + + // feeRateLarge(sat/kw) is used when we want to use the anchor + // transaction to CPFP our commitment transaction. + feeRateLarge := maxPerKw * 2 + + ctxt, cancel := context.WithTimeout( + context.Background(), defaultTimeout, + ) + defer cancel() + + // Before we start, set up the default fee rate and we will test the + // actual fee rate against it to decide whether we are using the + // deadline to perform fee estimation. + net.SetFeeEstimate(feeRateDefault) + + // setupNode creates a new node and sends 1 btc to the node. + setupNode := func(name string) *lntest.HarnessNode { + // Create the node. + args := []string{"--hodl.exit-settle"} + args = append(args, commitTypeAnchors.Args()...) + node := net.NewNode(t.t, name, args) + + // Send some coins to the node. + net.SendCoins(ctxt, t.t, btcutil.SatoshiPerBitcoin, node) + return node + } + + // calculateSweepFeeRate runs multiple steps to calculate the fee rate + // used in sweeping the transactions. + calculateSweepFeeRate := func(expectedSweepTxNum int) int64 { + // Create two nodes, Alice and Bob. + alice := setupNode("Alice") + defer shutdownAndAssert(net, t, alice) + + bob := setupNode("Bob") + defer shutdownAndAssert(net, t, bob) + + // Connect Alice to Bob. + net.ConnectNodes(ctxt, t.t, alice, bob) + + // Open a channel between Alice and Bob. + chanPoint := openChannelAndAssert( + ctxt, t, net, alice, bob, + lntest.OpenChannelParams{ + Amt: 10e6, + PushAmt: 5e6, + }, + ) + + // Send a payment with a specified finalCTLVDelta, which will + // be used as our deadline later on when Alice force closes the + // channel. + _, err := alice.RouterClient.SendPaymentV2( + ctxt, + &routerrpc.SendPaymentRequest{ + Dest: bob.PubKey[:], + Amt: 10e4, + PaymentHash: makeFakePayHash(t), + FinalCltvDelta: finalCTLV, + TimeoutSeconds: 60, + FeeLimitMsat: noFeeLimitMsat, + }, + ) + require.NoError(t.t, err, "unable to send alice htlc") + + // Once the HTLC has cleared, all the nodes in our mini network + // should show that the HTLC has been locked in. + nodes := []*lntest.HarnessNode{alice, bob} + err = wait.NoError(func() error { + return assertNumActiveHtlcs(nodes, 1) + }, defaultTimeout) + require.NoError(t.t, err, "htlc mismatch") + + // Alice force closes the channel. + _, _, err = net.CloseChannel(ctxt, alice, chanPoint, true) + require.NoError(t.t, err, "unable to force close channel") + + // Now that the channel has been force closed, it should show + // up in the PendingChannels RPC under the waiting close + // section. + pendingChansRequest := &lnrpc.PendingChannelsRequest{} + pendingChanResp, err := alice.PendingChannels( + ctxt, pendingChansRequest, + ) + require.NoError( + t.t, err, "unable to query for pending channels", + ) + require.NoError( + t.t, checkNumWaitingCloseChannels(pendingChanResp, 1), + ) + + // We should see only one sweep transaction because the anchor + // sweep is skipped. + sweepTxns, err := getNTxsFromMempool( + net.Miner.Client, + expectedSweepTxNum, minerMempoolTimeout, + ) + require.NoError( + t.t, err, "failed to find commitment tx in mempool", + ) + + // Mine a block to confirm these transactions such that they + // don't remain in the mempool for any subsequent tests. + _, err = net.Miner.Client.Generate(1) + require.NoError(t.t, err, "unable to mine blocks") + + // Calculate the fee rate used. + feeRate := calculateTxnsFeeRate(t.t, net.Miner, sweepTxns) + + return feeRate + } + + // Setup our fee estimation for the deadline. Because the fee rate is + // smaller than the parent tx's fee rate, this value won't be used and + // we should see only one sweep tx in the mempool. + net.SetFeeEstimateWithConf(feeRateSmall, deadline) + + // Calculate fee rate used. + feeRate := calculateSweepFeeRate(1) + + // We expect the default max fee rate is used. Allow some deviation + // because weight estimates during tx generation are estimates. + require.InEpsilonf( + t.t, int64(maxPerKw), feeRate, 0.01, + "expected fee rate:%d, got fee rate:%d", maxPerKw, feeRate, + ) + + // Setup our fee estimation for the deadline. Because the fee rate is + // greater than the parent tx's fee rate, this value will be used to + // sweep the anchor transaction and we should see two sweep + // transactions in the mempool. + net.SetFeeEstimateWithConf(feeRateLarge, deadline) + + // Calculate fee rate used. + feeRate = calculateSweepFeeRate(2) + + // We expect the anchor to be swept with the deadline, which has the + // fee rate of feeRateLarge. + require.InEpsilonf( + t.t, int64(feeRateLarge), feeRate, 0.01, + "expected fee rate:%d, got fee rate:%d", feeRateLarge, feeRate, + ) +} + +// calculateTxnsFeeRate takes a list of transactions and estimates the fee rate +// used to sweep them. +func calculateTxnsFeeRate(t *testing.T, + miner *rpctest.Harness, txns []*wire.MsgTx) int64 { + + var totalWeight, totalFee int64 + for _, tx := range txns { + utx := btcutil.NewTx(tx) + totalWeight += blockchain.GetTransactionWeight(utx) + + fee, err := getTxFee(miner.Client, tx) + require.NoError(t, err) + + totalFee += int64(fee) + } + feeRate := totalFee * 1000 / totalWeight + + return feeRate +} diff --git a/lntest/itest/lnd_test_list_on_test.go b/lntest/itest/lnd_test_list_on_test.go index 8dee0fa1..88d01f84 100644 --- a/lntest/itest/lnd_test_list_on_test.go +++ b/lntest/itest/lnd_test_list_on_test.go @@ -111,7 +111,6 @@ var allTestCases = []*testCase{ name: "private channel update policy", test: testUpdateChannelPolicyForPrivateChannel, }, - { name: "invoice routing hints", test: testInvoiceRoutingHints, @@ -239,6 +238,10 @@ var allTestCases = []*testCase{ name: "hold invoice force close", test: testHoldInvoiceForceClose, }, + { + name: "commitment deadline", + test: testCommitmentTransactionDeadline, + }, { name: "cpfp", test: testCPFP, diff --git a/lnwallet/chainfee/estimator.go b/lnwallet/chainfee/estimator.go index 341ee919..eee26148 100644 --- a/lnwallet/chainfee/estimator.go +++ b/lnwallet/chainfee/estimator.go @@ -19,12 +19,12 @@ const ( // a WebAPIEstimator will cache fees for. This number is chosen // because it's the highest number of confs bitcoind will return a fee // estimate for. - maxBlockTarget uint32 = 1009 + maxBlockTarget uint32 = 1008 // minBlockTarget is the lowest number of blocks confirmations that // a WebAPIEstimator will cache fees for. Requesting an estimate for // less than this will result in an error. - minBlockTarget uint32 = 2 + minBlockTarget uint32 = 1 // minFeeUpdateTimeout represents the minimum interval in which a // WebAPIEstimator will request fresh fees from its API. @@ -375,7 +375,16 @@ func (b *BitcoindEstimator) Stop() error { // confirmation and returns the estimated fee expressed in sat/kw. // // NOTE: This method is part of the Estimator interface. -func (b *BitcoindEstimator) EstimateFeePerKW(numBlocks uint32) (SatPerKWeight, error) { +func (b *BitcoindEstimator) EstimateFeePerKW( + numBlocks uint32) (SatPerKWeight, error) { + + if numBlocks > maxBlockTarget { + log.Debugf("conf target %d exceeds the max value, "+ + "use %d instead.", numBlocks, maxBlockTarget, + ) + numBlocks = maxBlockTarget + } + feeEstimate, err := b.fetchEstimate(numBlocks) switch { // If the estimator doesn't have enough data, or returns an error, then diff --git a/lnwallet/chainfee/estimator_test.go b/lnwallet/chainfee/estimator_test.go index f98ae80c..2900ff17 100644 --- a/lnwallet/chainfee/estimator_test.go +++ b/lnwallet/chainfee/estimator_test.go @@ -172,7 +172,7 @@ func TestWebAPIFeeEstimator(t *testing.T) { est uint32 err string }{ - {"target_below_min", 1, 12345, 12345, "too low, minimum"}, + {"target_below_min", 0, 12345, 12345, "too low, minimum"}, {"target_w_too-low_fee", 10, 42, feeFloor, ""}, {"API-omitted_target", 2, 0, 0, "web API does not include"}, {"valid_target", 20, 54321, 54321, ""}, diff --git a/lnwallet/channel.go b/lnwallet/channel.go index 94c01e94..1301d83f 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -27,8 +27,6 @@ import ( "github.com/lightningnetwork/lnd/lnwire" ) -var zeroHash chainhash.Hash - var ( // ErrChanClosing is returned when a caller attempts to close a channel // that has already been closed or is in the process of being closed. @@ -6406,16 +6404,33 @@ func (lc *LightningChannel) CompleteCooperativeClose( return closeTx, ourBalance, nil } -// NewAnchorResolutions returns the anchor resolutions for all currently valid -// commitment transactions. Because we have no view on the mempool, we can only -// blindly anchor all of these txes down. -func (lc *LightningChannel) NewAnchorResolutions() ([]*AnchorResolution, +// AnchorResolutions is a set of anchor resolutions that's being used when +// sweeping anchors during local channel force close. +type AnchorResolutions struct { + // Local is the anchor resolution for the local commitment tx. + Local *AnchorResolution + + // Remote is the anchor resolution for the remote commitment tx. + Remote *AnchorResolution + + // RemotePending is the anchor resolution for the remote pending + // commitment tx. The value will be non-nil iff we've created a new + // commitment tx for the remote party which they haven't ACKed yet. + RemotePending *AnchorResolution +} + +// NewAnchorResolutions returns a set of anchor resolutions wrapped in the +// struct AnchorResolutions. Because we have no view on the mempool, we can +// only blindly anchor all of these txes down. Caller needs to check the +// returned values against nil to decide whether there exists an anchor +// resolution for local/remote/pending remote commitment txes. +func (lc *LightningChannel) NewAnchorResolutions() (*AnchorResolutions, error) { lc.Lock() defer lc.Unlock() - var resolutions []*AnchorResolution + resolutions := &AnchorResolutions{} // Add anchor for local commitment tx, if any. localRes, err := NewAnchorResolution( @@ -6424,9 +6439,7 @@ func (lc *LightningChannel) NewAnchorResolutions() ([]*AnchorResolution, if err != nil { return nil, err } - if localRes != nil { - resolutions = append(resolutions, localRes) - } + resolutions.Local = localRes // Add anchor for remote commitment tx, if any. remoteRes, err := NewAnchorResolution( @@ -6435,9 +6448,7 @@ func (lc *LightningChannel) NewAnchorResolutions() ([]*AnchorResolution, if err != nil { return nil, err } - if remoteRes != nil { - resolutions = append(resolutions, remoteRes) - } + resolutions.Remote = remoteRes // Add anchor for remote pending commitment tx, if any. remotePendingCommit, err := lc.channelState.RemoteCommitChainTip() @@ -6453,10 +6464,7 @@ func (lc *LightningChannel) NewAnchorResolutions() ([]*AnchorResolution, if err != nil { return nil, err } - - if remotePendingRes != nil { - resolutions = append(resolutions, remotePendingRes) - } + resolutions.RemotePending = remotePendingRes } return resolutions, nil diff --git a/lnwallet/channel_test.go b/lnwallet/channel_test.go index 85ccc305..6de8b2ac 100644 --- a/lnwallet/channel_test.go +++ b/lnwallet/channel_test.go @@ -917,14 +917,19 @@ func testForceClose(t *testing.T, testCase *forceCloseTestCase) { } // Check the pre-confirmation resolutions. - resList, err := aliceChannel.NewAnchorResolutions() + res, err := aliceChannel.NewAnchorResolutions() if err != nil { t.Fatalf("pre-confirmation resolution error: %v", err) } - if len(resList) != 2 { - t.Fatal("expected two resolutions") - } + // Check we have the expected anchor resolutions. + require.NotNil(t, res.Local, "expected local anchor resolution") + require.NotNil(t, + res.Remote, "expected remote anchor resolution", + ) + require.Nil(t, + res.RemotePending, "expected no anchor resolution", + ) } // The SelfOutputSignDesc should be non-nil since the output to-self is