From dde605d3752795d9fcb32c9381bed8f515e42031 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Tue, 5 Sep 2023 12:42:23 +0800 Subject: [PATCH] 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 4f53d2098..586b901d9 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 6a2cccd86..29c61bd81 100644 --- a/sweep/tx_input_set.go +++ b/sweep/tx_input_set.go @@ -179,11 +179,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 } } @@ -205,6 +219,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 @@ -215,10 +232,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 @@ -227,7 +254,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 } @@ -236,7 +269,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. // @@ -248,9 +281,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 20e80a00c..188148884 100644 --- a/sweep/txgenerator.go +++ b/sweep/txgenerator.go @@ -113,10 +113,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 }