diff --git a/contractcourt/channel_arbitrator.go b/contractcourt/channel_arbitrator.go index e1cbb117304..3176cf01289 100644 --- a/contractcourt/channel_arbitrator.go +++ b/contractcourt/channel_arbitrator.go @@ -1309,9 +1309,23 @@ func (c *ChannelArbitrator) sweepAnchors(anchors *lnwallet.AnchorResolutions, return err } + // Create a force flag that's used to indicate whether we + // should force sweeping this anchor. + var force bool + + // Check the deadline against the default value. If it's less + // than the default value of 144, it means there is a deadline + // and we will perform a CPFP for this commitment tx. + if deadline < anchorSweepConfTarget { + // 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. + force = true + } + log.Debugf("ChannelArbitrator(%v): pre-confirmation sweep of "+ - "anchor of %s commit tx %v", c.cfg.ChanPoint, - anchorPath, anchor.CommitAnchor) + "anchor of %s commit tx %v, force=%v", c.cfg.ChanPoint, + anchorPath, anchor.CommitAnchor, force) witnessType := input.CommitmentAnchor @@ -1337,20 +1351,17 @@ func (c *ChannelArbitrator) sweepAnchors(anchors *lnwallet.AnchorResolutions, ) // Sweep anchor output with a confirmation target fee - // preference. Because this is a cpfp-operation, the anchor will - // only be attempted to sweep when the current fee estimate for - // the confirmation target exceeds the commit fee rate. - // - // 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. + // preference. Because this is a cpfp-operation, the anchor + // will only be attempted to sweep when the current fee + // estimate for the confirmation target exceeds the commit fee + // rate. _, err = c.cfg.Sweeper.SweepInput( &anchorInput, sweep.Params{ Fee: sweep.FeePreference{ ConfTarget: deadline, }, - Force: true, + Force: force, ExclusiveGroup: &exclusiveGroup, }, ) @@ -1427,6 +1438,9 @@ func (c *ChannelArbitrator) findCommitmentDeadline(heightHint uint32, if htlc.RefundTimeout < deadlineMinHeight { deadlineMinHeight = htlc.RefundTimeout + log.Tracef("ChannelArbitrator(%v): outgoing HTLC has "+ + "deadline: %v", c.cfg.ChanPoint, + deadlineMinHeight) } } @@ -1455,6 +1469,9 @@ func (c *ChannelArbitrator) findCommitmentDeadline(heightHint uint32, if htlc.RefundTimeout < deadlineMinHeight { deadlineMinHeight = htlc.RefundTimeout + log.Tracef("ChannelArbitrator(%v): incoming HTLC has "+ + "deadline: %v", c.cfg.ChanPoint, + deadlineMinHeight) } } diff --git a/docs/release-notes/release-notes-0.17.1.md b/docs/release-notes/release-notes-0.17.1.md index 7af034b1955..78260acbf12 100644 --- a/docs/release-notes/release-notes-0.17.1.md +++ b/docs/release-notes/release-notes-0.17.1.md @@ -25,6 +25,16 @@ # New Features ## Functional Enhancements + +- Previously, when a channel was force closed locally, its anchor was always + force-swept when the CPFP requirements are met. This is now + [changed](https://github.com/lightningnetwork/lnd/pull/7965) to only attempt + CPFP based on the deadline of the commitment transaction. The anchor output + will still be offered to the sweeper during channel force close, while the + actual sweeping won't be forced(CPFP) unless a relevant HTLC will timeout in + 144 blocks. If CPFP before this deadline is needed, user can use `BumpFee` + instead. + ## RPC Additions ## lncli Additions @@ -44,5 +54,5 @@ ## Tooling and Documentation # Contributors (Alphabetical Order) - -* Eugene Siegel \ No newline at end of file +* Eugene Siegel +* Yong Yu diff --git a/itest/lnd_channel_backup_test.go b/itest/lnd_channel_backup_test.go index f73b2f22bea..e4c046b3df7 100644 --- a/itest/lnd_channel_backup_test.go +++ b/itest/lnd_channel_backup_test.go @@ -783,16 +783,15 @@ func runChanRestoreScenarioForceClose(ht *lntest.HarnessTest, zeroConf bool) { ht, crs.password, crs.mnemonic, multi, dave, ) - // We now wait until both Dave's closing tx and sweep tx have shown in - // mempool. - ht.Miner.AssertNumTxsInMempool(2) + // We now wait until both Dave's closing tx. + ht.Miner.AssertNumTxsInMempool(1) // Now that we're able to make our restored now, we'll shutdown the old // Dave node as we'll be storing it shortly below. ht.Shutdown(dave) // Mine a block to confirm the closing tx from Dave. - ht.MineBlocksAndAssertNumTxes(1, 2) + ht.MineBlocksAndAssertNumTxes(1, 1) // To make sure the channel state is advanced correctly if the channel // peer is not online at first, we also shutdown Carol. @@ -1412,8 +1411,9 @@ func chanRestoreViaRPC(ht *lntest.HarnessTest, password []byte, func assertTimeLockSwept(ht *lntest.HarnessTest, carol, dave *node.HarnessNode, carolStartingBalance, daveStartingBalance int64) { - // We expect Carol to sweep her funds and also the anchor tx. - expectedTxes := 2 + // We expect Carol to sweep her funds and also the anchor tx. In + // addition, Dave will also sweep his anchor output. + expectedTxes := 3 // Carol should sweep her funds immediately, as they are not // timelocked. @@ -1495,11 +1495,7 @@ func assertDLPExecuted(ht *lntest.HarnessTest, // Upon reconnection, the nodes should detect that Dave is out of sync. // Carol should force close the channel using her latest commitment. - expectedTxes := 1 - if lntest.CommitTypeHasAnchors(commitType) { - expectedTxes = 2 - } - ht.Miner.AssertNumTxsInMempool(expectedTxes) + ht.Miner.AssertNumTxsInMempool(1) // Channel should be in the state "waiting close" for Carol since she // broadcasted the force close tx. @@ -1515,7 +1511,8 @@ func assertDLPExecuted(ht *lntest.HarnessTest, ht.RestartNode(dave) // Generate a single block, which should confirm the closing tx. - ht.MineBlocksAndAssertNumTxes(1, expectedTxes) + ht.MineBlocksAndAssertNumTxes(1, 1) + blocksMined := uint32(1) // Dave should consider the channel pending force close (since he is // waiting for his sweep to confirm). @@ -1532,6 +1529,14 @@ func assertDLPExecuted(ht *lntest.HarnessTest, // Mine Dave's anchor sweep tx. ht.MineBlocksAndAssertNumTxes(1, 1) + blocksMined++ + + // The above block will trigger Carol's sweeper to reconsider + // the anchor sweeping. Because we are now sweeping at the fee + // rate floor, the sweeper will consider this input has + // positive yield thus attempts the sweeping. + ht.MineBlocksAndAssertNumTxes(1, 1) + blocksMined++ // After Carol's output matures, she should also reclaim her // funds. @@ -1539,7 +1544,7 @@ func assertDLPExecuted(ht *lntest.HarnessTest, // The commit sweep resolver publishes the sweep tx at // defaultCSV-1 and we already mined one block after the // commitmment was published, so take that into account. - ht.MineBlocks(defaultCSV - 1 - 1) + ht.MineBlocks(defaultCSV - blocksMined) ht.MineBlocksAndAssertNumTxes(1, 1) // Now the channel should be fully closed also from Carol's POV. @@ -1561,21 +1566,33 @@ func assertDLPExecuted(ht *lntest.HarnessTest, // Dave should sweep his funds immediately, as they are not // timelocked. We also expect Dave to sweep his anchor, if // present. - ht.Miner.AssertNumTxsInMempool(expectedTxes) + if lntest.CommitTypeHasAnchors(commitType) { + ht.MineBlocksAndAssertNumTxes(1, 2) + } else { + ht.MineBlocksAndAssertNumTxes(1, 1) + } - // Mine the sweep tx. - ht.MineBlocksAndAssertNumTxes(1, expectedTxes) + blocksMined++ // Now Dave should consider the channel fully closed. ht.AssertNumPendingForceClose(dave, 0) + // The above block will trigger Carol's sweeper to reconsider + // the anchor sweeping. Because we are now sweeping at the fee + // rate floor, the sweeper will consider this input has + // positive yield thus attempts the sweeping. + if lntest.CommitTypeHasAnchors(commitType) { + ht.MineBlocksAndAssertNumTxes(1, 1) + blocksMined++ + } + // After Carol's output matures, she should also reclaim her // funds. // // The commit sweep resolver publishes the sweep tx at - // defaultCSV-1 and we already mined one block after the + // defaultCSV-1 and we already have blocks mined after the // commitmment was published, so take that into account. - ht.MineBlocks(defaultCSV - 1 - 1) + ht.MineBlocks(defaultCSV - blocksMined) ht.MineBlocksAndAssertNumTxes(1, 1) // Now the channel should be fully closed also from Carol's diff --git a/itest/lnd_channel_force_close_test.go b/itest/lnd_channel_force_close_test.go index 1bb2ff08041..e640ea58399 100644 --- a/itest/lnd_channel_force_close_test.go +++ b/itest/lnd_channel_force_close_test.go @@ -23,10 +23,13 @@ import ( ) // testCommitmentTransactionDeadline tests that the anchor sweep transaction is -// taking account of the deadline of the commitment transaction. It tests two +// taking account of the deadline of the commitment transaction. It tests three // 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. +// 2. when the CPFP is used, checks that the deadline is NOT applied when it's +// larger than 144. +// 3. when the CPFP is used, checks that the deadline is applied when it's +// less than 144. // // Note that whether the deadline is used or not is implicitly checked by its // corresponding fee rates. @@ -43,13 +46,13 @@ func testCommitmentTransactionDeadline(ht *lntest.HarnessTest) { // DefaultAnchorsCommitMaxFeeRateSatPerVByte. feeRateDefault = 20000 - // finalCTLV is used when Alice sends payment to Bob. - finalCTLV = 144 + // defaultDeadline is the anchorSweepConfTarget, which is used + // when the commitment has no deadline pressure. + defaultDeadline = 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) + // deadline is one block below the default deadline. A forced + // anchor sweep will be performed when seeing this value. + deadline = defaultDeadline - 1 ) // feeRateSmall(sat/kw) is used when we want to skip the CPFP @@ -91,7 +94,7 @@ func testCommitmentTransactionDeadline(ht *lntest.HarnessTest) { // calculateSweepFeeRate runs multiple steps to calculate the fee rate // used in sweeping the transactions. - calculateSweepFeeRate := func(expectedSweepTxNum int) int64 { + calculateSweepFeeRate := func(expectedSweepTxNum, deadline int) int64 { // Create two nodes, Alice and Bob. alice := setupNode("Alice") defer ht.Shutdown(alice) @@ -110,6 +113,10 @@ func testCommitmentTransactionDeadline(ht *lntest.HarnessTest) { }, ) + // Calculate the final ctlv delta based on the expected + // deadline. + finalCltvDelta := int32(deadline - int(routing.BlockPadding)) + // Send a payment with a specified finalCTLVDelta, which will // be used as our deadline later on when Alice force closes the // channel. @@ -117,7 +124,7 @@ func testCommitmentTransactionDeadline(ht *lntest.HarnessTest) { Dest: bob.PubKey[:], Amt: 10e4, PaymentHash: ht.Random32Bytes(), - FinalCltvDelta: finalCTLV, + FinalCltvDelta: finalCltvDelta, TimeoutSeconds: 60, FeeLimitMsat: noFeeLimitMsat, } @@ -154,8 +161,27 @@ func testCommitmentTransactionDeadline(ht *lntest.HarnessTest) { // we should see only one sweep tx in the mempool. ht.SetFeeEstimateWithConf(feeRateSmall, deadline) - // Calculate fee rate used. - feeRate := calculateSweepFeeRate(1) + // Calculate fee rate used and assert only the force close tx is + // broadcast. + feeRate := calculateSweepFeeRate(1, deadline) + + // We expect the default max fee rate is used. Allow some deviation + // because weight estimates during tx generation are estimates. + require.InEpsilonf( + ht, 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. However, due to the default value + // being used, we should not attempt CPFP here because we are not force + // sweeping the anchor output. + ht.SetFeeEstimateWithConf(feeRateLarge, defaultDeadline) + + // Calculate fee rate used and assert only the force close tx is + // broadcast. + feeRate = calculateSweepFeeRate(1, defaultDeadline) // We expect the default max fee rate is used. Allow some deviation // because weight estimates during tx generation are estimates. @@ -170,8 +196,9 @@ func testCommitmentTransactionDeadline(ht *lntest.HarnessTest) { // transactions in the mempool. ht.SetFeeEstimateWithConf(feeRateLarge, deadline) - // Calculate fee rate used. - feeRate = calculateSweepFeeRate(2) + // Calculate fee rate used and assert both the force close tx and the + // anchor sweeping tx are broadcast. + feeRate = calculateSweepFeeRate(2, deadline) // We expect the anchor to be swept with the deadline, which has the // fee rate of feeRateLarge. diff --git a/itest/lnd_multi-hop_test.go b/itest/lnd_multi-hop_test.go index 129fde9104a..2630bbde6af 100644 --- a/itest/lnd_multi-hop_test.go +++ b/itest/lnd_multi-hop_test.go @@ -784,20 +784,21 @@ func runMultiHopRemoteForceCloseOnChainHtlcTimeout(ht *lntest.HarnessTest, ht.AssertActiveHtlcs(bob, payHash[:]) ht.AssertActiveHtlcs(carol, payHash[:]) - // Increase the fee estimate so that the following force close tx will - // be cpfp'ed. - ht.SetFeeEstimate(30000) - // At this point, we'll now instruct Carol to force close the // transaction. This will let us exercise that Bob is able to sweep the - // expired HTLC on Carol's version of the commitment transaction. If - // Carol has an anchor, it will be swept too. - hasAnchors := lntest.CommitTypeHasAnchors(c) + // expired HTLC on Carol's version of the commitment transaction. closeStream, _ := ht.CloseChannelAssertPending( carol, bobChanPoint, true, ) + + // For anchor channels, the anchor won't be used for CPFP because + // channel arbitrator thinks Carol doesn't have preimage for her + // incoming HTLC on the commitment transaction Bob->Carol. Although + // Carol created this invoice, because it's a hold invoice, the + // preimage won't be generated automatically. + hasAnchorSweep := false closeTx := ht.AssertStreamChannelForceClosed( - carol, bobChanPoint, hasAnchors, closeStream, + carol, bobChanPoint, hasAnchorSweep, closeStream, ) // Increase the blocks mined. At this step @@ -828,7 +829,18 @@ func runMultiHopRemoteForceCloseOnChainHtlcTimeout(ht *lntest.HarnessTest, ht.Fatalf("unhandled commitment type %v", c) } - ht.Miner.AssertNumTxsInMempool(expectedTxes) + // We now mine a block to clear up the mempool. + ht.MineBlocksAndAssertNumTxes(1, expectedTxes) + blocksMined++ + + // The above block will trigger Carol's sweeper to reconsider the + // anchor sweeping. Because we are now sweeping at the fee rate floor, + // the sweeper will consider this input has positive yield thus + // attempts the sweeping. + if lntest.CommitTypeHasAnchors(c) { + ht.MineBlocksAndAssertNumTxes(1, 1) + blocksMined++ + } // Next, we'll mine enough blocks for the HTLC to expire. At this // point, Bob should hand off the output to his internal utxo nursery, @@ -962,18 +974,20 @@ func runMultiHopHtlcLocalChainClaim(ht *lntest.HarnessTest, // to be mined to trigger a force close later on. var blocksMined uint32 - // Increase the fee estimate so that the following force close tx will - // be cpfp'ed. - ht.SetFeeEstimate(30000) - // At this point, Bob decides that he wants to exit the channel // immediately, so he force closes his commitment transaction. - hasAnchors := lntest.CommitTypeHasAnchors(c) closeStream, _ := ht.CloseChannelAssertPending( bob, aliceChanPoint, true, ) + + // For anchor channels, the anchor won't be used for CPFP as there's no + // deadline pressure for Bob on the channel Alice->Bob at the moment. + // For Bob's local commitment tx, there's only one incoming HTLC which + // he doesn't have the preimage yet. Thus this anchor won't be + // force-swept. + hasAnchorSweep := false bobForceClose := ht.AssertStreamChannelForceClosed( - bob, aliceChanPoint, hasAnchors, closeStream, + bob, aliceChanPoint, hasAnchorSweep, closeStream, ) // Increase the blocks mined. At this step @@ -1050,7 +1064,12 @@ func runMultiHopHtlcLocalChainClaim(ht *lntest.HarnessTest, expectedTxes = 2 // Carol will broadcast her second level HTLC transaction and Bob will - // sweep his commitment and anchor output. + // sweep his commitment and anchor outputs. + // For anchor channels, we'd expect to see three transactions, + // - Carol's second level HTLC transaction + // - Bob's sweep tx spending his commitment output + // - Bob's sweep tx spending two anchor outputs, one from channel Alice + // to Bob and the other from channel Bob to Carol. case lnrpc.CommitmentType_ANCHORS, lnrpc.CommitmentType_SIMPLE_TAPROOT: expectedTxes = 3 @@ -1065,19 +1084,15 @@ func runMultiHopHtlcLocalChainClaim(ht *lntest.HarnessTest, ht.Fatalf("unhandled commitment type %v", c) } - txes := ht.Miner.GetNumTxsFromMempool(expectedTxes) - - // Both Carol's second level transaction and Bob's sweep should be - // spending from the commitment transaction. - ht.AssertAllTxesSpendFrom(txes, closingTxid) + // Assert transactions can be found in the mempool. + ht.Miner.AssertNumTxsInMempool(expectedTxes) // At this point we suspend Alice to make sure she'll handle the // on-chain settle after a restart. restartAlice := ht.SuspendNode(alice) // Mine a block to confirm the expected transactions (+ the coinbase). - block = ht.MineBlocksAndAssertNumTxes(1, expectedTxes)[0] - require.Len(ht, block.Transactions, expectedTxes+1) + ht.MineBlocksAndAssertNumTxes(1, expectedTxes) // For non-anchor channel types, the nursery will handle sweeping the // second level output, and it will wait one extra block before @@ -1087,7 +1102,7 @@ func runMultiHopHtlcLocalChainClaim(ht *lntest.HarnessTest, // If this is a channel of the anchor type, we will subtract one block // from the default CSV, as the Sweeper will handle the input, and the // Sweeper sweeps the input as soon as the lock expires. - if hasAnchors { + if lntest.CommitTypeHasAnchors(c) { secondLevelMaturity = defaultCSV - 1 } @@ -1098,6 +1113,7 @@ func runMultiHopHtlcLocalChainClaim(ht *lntest.HarnessTest, // will extract the preimage and broadcast a second level tx to claim // the HTLC in his (already closed) channel with Alice. bobSecondLvlTx := ht.Miner.GetNumTxsFromMempool(1)[0] + bobSecondLvlTxid := bobSecondLvlTx.TxHash() // It should spend from the commitment in the channel with Alice. ht.AssertTxSpendFrom(bobSecondLvlTx, *bobForceClose) @@ -1119,7 +1135,6 @@ func runMultiHopHtlcLocalChainClaim(ht *lntest.HarnessTest, // We'll now mine a block which should confirm Bob's second layer // transaction. block = ht.MineBlocksAndAssertNumTxes(1, 1)[0] - bobSecondLvlTxid := bobSecondLvlTx.TxHash() ht.Miner.AssertTxInBlock(block, &bobSecondLvlTxid) // Keep track of Bob's second level maturity, and decrement our track diff --git a/itest/lnd_onchain_test.go b/itest/lnd_onchain_test.go index 4c7a34d4847..4cfe47d3761 100644 --- a/itest/lnd_onchain_test.go +++ b/itest/lnd_onchain_test.go @@ -521,18 +521,20 @@ func testAnchorThirdPartySpend(ht *lntest.HarnessTest) { require.Equal(ht, testMemo, pendingChannelsResp.WaitingCloseChannels[0].Channel.Memo) - // At this point, the channel is waiting close, and we have both the - // commitment transaction and anchor sweep in the mempool. - const expectedTxns = 2 - sweepTxns := ht.Miner.GetNumTxsFromMempool(expectedTxns) + // At this point, the channel is waiting close so we have the + // commitment transaction in the mempool. Alice's anchor, however, + // because there's no deadline pressure, it won't be swept. aliceCloseTx := waitingClose.Commitments.LocalTxid - _, aliceAnchor := ht.FindCommitAndAnchor(sweepTxns, aliceCloseTx) - - // We'll now mine _only_ the commitment force close transaction, as we - // want the anchor sweep to stay unconfirmed. + ht.MineBlocksAndAssertNumTxes(1, 1) forceCloseTxID, _ := chainhash.NewHashFromStr(aliceCloseTx) - commitTxn := ht.Miner.GetRawTransaction(forceCloseTxID) - ht.Miner.MineBlockWithTxes([]*btcutil.Tx{commitTxn}) + + // Mine one block to trigger Alice's sweeper to reconsider the anchor + // sweeping. Because we are now sweeping at the fee rate floor, the + // sweeper will consider this input has positive yield thus attempts + // the sweeping. + ht.MineEmptyBlocks(1) + sweepTxns := ht.Miner.GetNumTxsFromMempool(1) + _, aliceAnchor := ht.FindCommitAndAnchor(sweepTxns, aliceCloseTx) // Assert that the channel is now in PendingForceClose. // @@ -573,7 +575,7 @@ func testAnchorThirdPartySpend(ht *lntest.HarnessTest) { // call, we'll generate a series of _empty_ blocks here. aliceRestart := ht.SuspendNode(alice) const anchorCsv = 16 - ht.MineEmptyBlocks(anchorCsv) + ht.MineEmptyBlocks(anchorCsv - 1) // Before we sweep the anchor, we'll restart Alice. require.NoErrorf(ht, aliceRestart(), "unable to restart alice") diff --git a/sweep/sweeper.go b/sweep/sweeper.go index c118951980c..46bb1f5bb78 100644 --- a/sweep/sweeper.go +++ b/sweep/sweeper.go @@ -1229,6 +1229,12 @@ func (s *UtxoSweeper) getInputLists(cluster inputCluster, // contain inputs that failed before. Therefore we also add sets // consisting of only new inputs to the list, to make sure that new // inputs are given a good, isolated chance of being published. + // + // TODO(yy): this would lead to conflict transactions as the same input + // can be used in two sweeping transactions, and our rebroadcaster will + // retry the failed one. We should instead understand why the input is + // failed in the first place, and start tracking input states in + // sweeper to avoid this. var newInputs, retryInputs []txInput for _, input := range cluster.inputs { // Skip inputs that have a minimum publish height that is not diff --git a/sweep/tx_input_set.go b/sweep/tx_input_set.go index c42d35f77da..70004a80267 100644 --- a/sweep/tx_input_set.go +++ b/sweep/tx_input_set.go @@ -3,6 +3,7 @@ package sweep import ( "fmt" "math" + "sort" "github.com/btcsuite/btcd/btcutil" "github.com/btcsuite/btcd/txscript" @@ -170,7 +171,9 @@ func (t *txInputSet) enoughInput() bool { // add adds a new input to the set. It returns a bool indicating whether the // input was added to the set. An input is rejected if it decreases the tx // output value after paying fees. -func (t *txInputSet) addToState(inp input.Input, constraints addConstraints) *txInputSetState { +func (t *txInputSet) addToState(inp input.Input, + constraints addConstraints) *txInputSetState { + // Stop if max inputs is reached. Do not count additional wallet inputs, // because we don't know in advance how many we may need. if constraints != constraintsWallet && @@ -181,64 +184,97 @@ func (t *txInputSet) addToState(inp input.Input, constraints addConstraints) *tx // If the input comes with a required tx out that is below dust, we // won't add it. + // + // NOTE: only HtlcSecondLevelAnchorInput returns non-nil RequiredTxOut. reqOut := inp.RequiredTxOut() if reqOut != nil { // Fetch the dust limit for this output. dustLimit := lnwallet.DustLimitForSize(len(reqOut.PkScript)) if btcutil.Amount(reqOut.Value) < dustLimit { + log.Errorf("Rejected input=%v due to dust required "+ + "output=%v, limit=%v", inp, reqOut.Value, + dustLimit) + + // TODO(yy): we should not return here for force + // sweeps. This means when sending sweeping request, + // one must be careful to not create dust outputs. In + // an extreme rare case, where the + // minRelayTxFee/discardfee is increased when sending + // the request, what's considered non-dust at the + // caller side will be dust here, causing a force sweep + // to fail. return nil } } // Clone the current set state. - s := t.clone() + newSet := t.clone() // Add the new input. - s.inputs = append(s.inputs, inp) + newSet.inputs = append(newSet.inputs, inp) // Add the value of the new input. value := btcutil.Amount(inp.SignDesc().Output.Value) - s.inputTotal += value + newSet.inputTotal += value // Recalculate the tx fee. - fee := s.weightEstimate(true).fee() + fee := newSet.weightEstimate(true).fee() // Calculate the new output value. if reqOut != nil { - s.requiredOutput += btcutil.Amount(reqOut.Value) + newSet.requiredOutput += btcutil.Amount(reqOut.Value) } - s.changeOutput = s.inputTotal - s.requiredOutput - fee + + // NOTE: `changeOutput` could be negative here if this input is using + // constraintsForce. + newSet.changeOutput = newSet.inputTotal - newSet.requiredOutput - fee // Calculate the yield of this input from the change in total tx output // value. - inputYield := s.totalOutput() - t.totalOutput() + inputYield := newSet.totalOutput() - t.totalOutput() switch constraints { // Don't sweep inputs that cost us more to sweep than they give us. case constraintsRegular: if inputYield <= 0 { + log.Debugf("Rejected regular input=%v due to negative "+ + "yield=%v", value, inputYield) + return nil } // For force adds, no further constraints apply. + // + // NOTE: because the inputs are sorted with force sweeps being placed + // at the start of the list, we should never see an input with + // constraintsForce come after an input with constraintsRegular. In + // other words, though we may have negative `changeOutput` from + // including force sweeps, `inputYield` should always increase when + // adding regular inputs. case constraintsForce: - s.force = true + newSet.force = true // We are attaching a wallet input to raise the tx output value above // the dust limit. case constraintsWallet: // Skip this wallet input if adding it would lower the output // value. + // + // TODO(yy): change to inputYield < 0 to allow sweeping for + // UTXO aggregation only? if inputYield <= 0 { + log.Debugf("Rejected wallet input=%v due to negative "+ + "yield=%v", value, inputYield) + return nil } // Calculate the total value that we spend in this tx from the // wallet if we'd add this wallet input. - s.walletInputTotal += value + newSet.walletInputTotal += value // In any case, we don't want to lose money by sweeping. If we - // don't get more out of the tx then we put in ourselves, do not + // don't get more out of the tx than we put in ourselves, do not // add this wallet input. If there is at least one force sweep // in the set, this does no longer apply. // @@ -250,17 +286,27 @@ func (t *txInputSet) addToState(inp input.Input, constraints addConstraints) *tx // value of the wallet input and what we get out of this // transaction. To prevent attaching and locking a big utxo for // very little benefit. - if !s.force && s.walletInputTotal >= s.totalOutput() { + if newSet.force { + break + } + + // TODO(yy): change from `>=` to `>` to allow non-negative + // sweeping - we won't gain more coins from this sweep, but + // aggregating small UTXOs. + if newSet.walletInputTotal >= newSet.totalOutput() { + // TODO(yy): further check this case as it seems we can + // never reach here because it'd mean `inputYield` is + // already <= 0? log.Debugf("Rejecting wallet input of %v, because it "+ "would make a negative yielding transaction "+ - "(%v)", - value, s.totalOutput()-s.walletInputTotal) + "(%v)", value, + newSet.totalOutput()-newSet.walletInputTotal) return nil } } - return &s + return &newSet } // add adds a new input to the set. It returns a bool indicating whether the @@ -334,6 +380,15 @@ func (t *txInputSet) tryAddWalletInputsIfNeeded() error { return err } + // Sort the UTXOs by putting smaller values at the start of the slice + // to avoid locking large UTXO for sweeping. + // + // TODO(yy): add more choices to CoinSelectionStrategy and use the + // configured value here. + sort.Slice(utxos, func(i, j int) bool { + return utxos[i].Value < utxos[j].Value + }) + for _, utxo := range utxos { input, err := createWalletTxInput(utxo) if err != nil { diff --git a/sweep/txgenerator.go b/sweep/txgenerator.go index e1f71e7cd98..3d37063031b 100644 --- a/sweep/txgenerator.go +++ b/sweep/txgenerator.go @@ -115,10 +115,10 @@ func generateInputPartitionings(sweepableInputs []txInput, if !txInputs.enoughInput() { // The change output is always a p2tr here. dl := lnwallet.DustLimitForSize(input.P2TRSize) - log.Debugf("Set value %v (r=%v, c=%v) below dust "+ - "limit of %v", txInputs.totalOutput(), - txInputs.requiredOutput, txInputs.changeOutput, - dl) + log.Debugf("Input set value %v (required=%v, "+ + "change=%v) below dust limit of %v", + txInputs.totalOutput(), txInputs.requiredOutput, + txInputs.changeOutput, dl) return sets, nil }