From 535b0387d12a209e138626c56396aaa1bfe5fbd9 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Tue, 5 Sep 2023 11:14:22 +0800 Subject: [PATCH 1/8] sweep: use longer variable name for clarity in `addToState` --- sweep/tx_input_set.go | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/sweep/tx_input_set.go b/sweep/tx_input_set.go index c42d35f77da..935e4a62ca5 100644 --- a/sweep/tx_input_set.go +++ b/sweep/tx_input_set.go @@ -170,7 +170,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 && @@ -191,27 +193,27 @@ func (t *txInputSet) addToState(inp input.Input, constraints addConstraints) *tx } // 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 + 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. @@ -222,7 +224,7 @@ func (t *txInputSet) addToState(inp input.Input, constraints addConstraints) *tx // For force adds, no further constraints apply. case constraintsForce: - s.force = true + newSet.force = true // We are attaching a wallet input to raise the tx output value above // the dust limit. @@ -235,7 +237,7 @@ func (t *txInputSet) addToState(inp input.Input, constraints addConstraints) *tx // 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 @@ -250,17 +252,19 @@ 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 && + newSet.walletInputTotal >= newSet.totalOutput() { + 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 From 091af9313b40bdb4b0ca9b1b00c7f4b84cf69215 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Tue, 5 Sep 2023 12:42:23 +0800 Subject: [PATCH 2/8] sweeper: add more docs and debug logs --- sweep/sweeper.go | 6 ++++++ sweep/tx_input_set.go | 47 ++++++++++++++++++++++++++++++++++++++++--- sweep/txgenerator.go | 8 ++++---- 3 files changed, 54 insertions(+), 7 deletions(-) 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 935e4a62ca5..b6a8415ea05 100644 --- a/sweep/tx_input_set.go +++ b/sweep/tx_input_set.go @@ -183,11 +183,25 @@ func (t *txInputSet) addToState(inp input.Input, // 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 } } @@ -209,6 +223,9 @@ func (t *txInputSet) addToState(inp input.Input, if reqOut != nil { newSet.requiredOutput += btcutil.Amount(reqOut.Value) } + + // 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 @@ -219,10 +236,20 @@ func (t *txInputSet) addToState(inp input.Input, // 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: newSet.force = true @@ -231,7 +258,13 @@ func (t *txInputSet) addToState(inp input.Input, 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 } @@ -240,7 +273,7 @@ func (t *txInputSet) addToState(inp input.Input, 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. // @@ -252,9 +285,17 @@ func (t *txInputSet) addToState(inp input.Input, // 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 !newSet.force && - newSet.walletInputTotal >= newSet.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, 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 } From 460d824990b201760f2e8b0e4f35a334c26877ac Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Thu, 7 Sep 2023 00:52:57 +0800 Subject: [PATCH 3/8] sweep: prioritize smaller inputs when adding wallet UTXOs This commit sorts wallet UTXOs by their values when using them for sweeping inputs. This way we'd avoid locking large UTXOs when sweeping inputs and also provide an opportunity to aggregate wallet UTXOs. --- sweep/tx_input_set.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/sweep/tx_input_set.go b/sweep/tx_input_set.go index b6a8415ea05..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" @@ -379,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 { From e8f48801c2bbe9063d7a4b429cd18be0670a3467 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Thu, 14 Sep 2023 16:37:48 +0800 Subject: [PATCH 4/8] contractcourt+itest: relax anchor sweeping for CPFP purpose This commit changes from always sweeping anchor for a local force close to only do so when there is an actual time pressure. After this change, a forced anchor sweeping will only be attempted when the deadline is less than 144 blocks. --- contractcourt/channel_arbitrator.go | 37 +++++++++++++----- itest/lnd_channel_force_close_test.go | 55 ++++++++++++++++++++------- 2 files changed, 68 insertions(+), 24 deletions(-) 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/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. From 3b49694909fedc9fb4a9e44195420d553c2686bf Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Thu, 14 Sep 2023 17:45:43 +0800 Subject: [PATCH 5/8] docs: update release notes --- docs/release-notes/release-notes-0.17.1.md | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) 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 From 61ae9af6fd24d5a2d46f21f7cd2fbd2ee87fc091 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Fri, 15 Sep 2023 13:29:09 +0800 Subject: [PATCH 6/8] itest: update test `testMultiHopHtlcLocalChainClaim` to skip CPFP Since we now only perform CPFP when both the fee rate is higher and the deadline is less than 144, we need to update the test to reflect that Bob will not CPFP the force close tx for the channle Alice->Bob. --- itest/lnd_multi-hop_test.go | 35 +++++++++++++++++++---------------- 1 file changed, 19 insertions(+), 16 deletions(-) diff --git a/itest/lnd_multi-hop_test.go b/itest/lnd_multi-hop_test.go index 129fde9104a..466216c3823 100644 --- a/itest/lnd_multi-hop_test.go +++ b/itest/lnd_multi-hop_test.go @@ -962,18 +962,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 +1052,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 +1072,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 +1090,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 +1101,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 +1123,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 From 97a7a7e896963c6a9d9252ee903529585a33bc3a Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Fri, 15 Sep 2023 16:00:20 +0800 Subject: [PATCH 7/8] itest: fix `testMultiHopRemoteForceCloseOnChainHtlcTimeout` --- itest/lnd_multi-hop_test.go | 30 +++++++++++++++++++++--------- 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/itest/lnd_multi-hop_test.go b/itest/lnd_multi-hop_test.go index 466216c3823..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, From 06cca85f4be47e5b635978427b6433c7bc93434f Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Fri, 15 Sep 2023 21:48:40 +0800 Subject: [PATCH 8/8] itest: update related tests to reflect anchor sweeping This commit updates all related tests to reflect the latest anchor sweeping behavior. Previously, anchor sweeping is always attempted as CPFP when a force close is broadcast, while now it only happens when the deadline is less than 144. For non-CPFP purpose sweeping, it will happen after one block is mined after the force close transaction is confirmed as the anchor will be resent to the sweeper with a floor fee rate, hence making it economical to sweep. --- itest/lnd_channel_backup_test.go | 53 +++++++++++++++++++++----------- itest/lnd_onchain_test.go | 24 ++++++++------- 2 files changed, 48 insertions(+), 29 deletions(-) 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_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")