sweep: remove possible RBF when sweeping new inputs by yyforyongyu · Pull Request #8091 · lightningnetwork/lnd

yyforyongyu · 2023-10-12T12:49:03Z

Depends on some of the test framework from,

Fixes #7972 and possible #7181

This PR removes a possible case that when sweeping new and old inputs, the new inputs may end up in a second transaction which accidentally attempts an RBF(and fails). It also refactors the sweeper and simplified the ticker logic, preparing for incoming refactors.

Roasbeef

Solid set of refactoring and additional tests! I need to sit a bit more with the RBF change. IIUC, this shouldn't affect our sweeping unless the new sweep actually violates a rule of RBF. One example would be using the same transaction, but adding an a new unconfirmed input.

ziggie1984

I only realized after reviewing the first part, that it's another PR 😅, so left nits I did not seeing when I reviewed the base PR for it.

In terms of the new PR, I really like the tests and the refactoring. What I am not so sure about is the fix of the RBF case, because I think it deteriorates the sweeper situation mainly because most node runners have those min-relay-anchors sitting around and marking almost all sweeps invalid as soon as a third party bundled a big tx with a lot of anchors (increasing the RBF fee condition). So in the old case, we would at least publish the newSet as well, having a change for the new inputs to get broadcasted. LMKWYT ?

yyforyongyu · 2023-10-17T11:02:26Z

What I am not so sure about is the fix of the RBF case, because I think it deteriorates the sweeper situation mainly because most node runners have those min-relay-anchors sitting around and marking almost all sweeps invalid as soon as a third party bundled a big tx with a lot of anchors (increasing the RBF fee condition). So in the old case, we would at least publish the newSet as well, having a change for the new inputs to get broadcasted. LMKWYT ?

There are two RBF happening at the moment,

when creating sweeping txes, if there are mixed inputs, at least two txes are created - txAll for all inputs, txNew for new inputs, as shown in [bug]: sweeper (bumpfee) creates invalid transaction (not paying higher fees when necessary) #7972, and the possible result,
- txAll gets in mempool and be replaced by txNew, or the other way around where txNew gets replace. Tho unwanted, this is at least a good case as there's no broadcasting errors.
- txAll gets in mempool and txNew gets a broadcast error, or the other way around, and we don't know which case will happen because we can't guarantee the tx propagation order.
when sweeping all inputs, retried inputs are already in mempool, thus creating an RBF case which may or may not be satisfied.

This PR aims to fix case 1 so we don't have this RBF condition - we can try broadcast txAll first, if it fails, we then broadcast txNew.

GeorgeTsagk · 2023-10-17T17:21:17Z

the reason you append to this slice is to notify all of the requesters?

GeorgeTsagk · 2023-10-17T17:24:25Z

should this be raised to a more visible log level, e.g Warnf ?

GeorgeTsagk · 2023-10-17T17:53:26Z

seems like allSets only returns sets of previously-published sweeps. Why not reflect that on the name, e.g existingSets ?

allSet uses all the inputs to make the sweeps tho.

The method `waitForSpend` makes it sounding like it's blocking while it's not, thus it's renamed to avoid confusion.

This commit attempts to make the polling logic in sweeper more linear. Previously, the sweep's timer is reset/restarted in multiple places, such as when a new input comes in, or a new block comes in, or a previous input being spent, making it difficult to follow. We now remove the old timer and replaces it with a simple polling logic - we will schedule sweeps every 5s(default), and if there's no input to be swept, we'd skip, just like the previous `scheduleSweep` does. It's also worthy noting that, although `scheduleSweep` triggers the timer to tick, by the time we do the actual sweep in `sweepCluster`, conditions may have changed. This is now also fixed because we only have one place to create the clusters and sweeps.

This commit changes how we create the input sets which are used to construct the sweeping transactions. Assume the sweeper has two inputs, one is new and one is retried, we'd end up having two transactions, - tx1: which spends both the new and old inputs. - tx2: which spends the new inputs only. When publishing these txes, depending on which one gets into the mempool first, the other one will be viewed as an RBF for the first one since they both spending the same input(the new input). This is now fixed by only attempt to publish the second tx when there isn't a first tx - when there is a tx1, it means the new inputs are already used in this tx along with retried inputs, hence there's no need to publish tx2 which spends the new inputs only.

ziggie1984

LGTM, one thing I want to highlight, we should definitely add a follow-up PR where we trigger the sweeper when the sweep has the force flag set. Because we have the variable batchwindow-duration people can mess-up there config and end up not sweeping time sensitive inputs.

Moreover it is worth noting that this PR is just one part of the fix in regards of the RBF problem (see yy's comment).

ziggie1984 · 2023-10-23T11:53:16Z

+		}
+
+		// If we have successfully swept all inputs, there's no need to
+		// sweep the new inputs as it'd create an RBF case.


Nit: ... there's no need to sweep the new inputs separately ...

ziggie1984 · 2023-10-23T11:54:38Z

+			return nil
+		}
+
+		// We'd end up there if there's no retried inputs. In this


Nit: Comment needs an update as well now

Roasbeef · 2023-10-25T02:00:43Z

Ultimately, the RBF rules are pretty finicky, and the only way we can ensure we're about to publish a replaceable transaction, is by using testmempoolaccept each time. So I consider this a stop gap to start to refractor, simplify the code, and fix any low hanging fruits before we start to integrate something like that.

Roasbeef

LGTM 🛩️

Roasbeef · 2023-10-25T02:06:26Z

 	)
-
-	// In case of an unexpected error, don't try to recover.
-	if err != nil && err != lnwallet.ErrDoubleSpend {


Why doesn't this try to explicitly catch the double spend error any longer?

Is the rationale that since we reschedule everything above, we don't need to specially handle this control flow?

Exactly we want to make sure, that even unexpected errors increase their attempt counter allowing them to be removed after the default of 10 attempts (default value). Moreover we use the error to signal, that we try to publish all the new inputs in a separate transaction when the combined one (combination of retried and new inputs) failed.

yyforyongyu added the utxo sweeping label Oct 12, 2023

yyforyongyu added this to the v0.18.0 milestone Oct 12, 2023

yyforyongyu self-assigned this Oct 12, 2023

yyforyongyu force-pushed the remove-rbf-sweeper branch from 3106609 to 1e14ae2 Compare October 12, 2023 18:39

Roasbeef reviewed Oct 13, 2023

View reviewed changes

yyforyongyu force-pushed the remove-rbf-sweeper branch 5 times, most recently from 6e27721 to 48273b2 Compare October 13, 2023 12:08

yyforyongyu requested a review from ziggie1984 October 13, 2023 12:17

yyforyongyu force-pushed the remove-rbf-sweeper branch from 48273b2 to a34c370 Compare October 13, 2023 17:29

ziggie1984 reviewed Oct 14, 2023

View reviewed changes

yyforyongyu force-pushed the remove-rbf-sweeper branch from a34c370 to f4bf4f0 Compare October 17, 2023 13:08

GeorgeTsagk reviewed Oct 17, 2023

View reviewed changes

yyforyongyu force-pushed the remove-rbf-sweeper branch 5 times, most recently from 2bcdf68 to 7cd9fce Compare October 19, 2023 02:02

saubyk requested review from Roasbeef and ziggie1984 October 19, 2023 15:25

ziggie1984 requested changes Oct 21, 2023

View reviewed changes

Comment thread sweep/sweeper.go Outdated

yyforyongyu added 6 commits October 23, 2023 10:24

sweep: add new method handleNewInput

3997368

sweep: rename waitForSpend to monitorSpend to avoid confusion

39a7cc6

The method `waitForSpend` makes it sounding like it's blocking while it's not, thus it's renamed to avoid confusion.

sweep: add new method handleInputSpent

f1d0f9f

sweep: add new method handleSweep

62b5869

sweep+lnd: move ticker creation into sweeper

4ba0909

yyforyongyu added 2 commits October 23, 2023 10:44

sweep: add new method rescheduleInputs and refactor sweep

acc15d8

yyforyongyu force-pushed the remove-rbf-sweeper branch from 7cd9fce to 5d3c72b Compare October 23, 2023 02:49

yyforyongyu added 3 commits October 23, 2023 13:01

itest: update itest to reflect new sweeper behavior

0816f91

docs: update release note re RBF fix

05fbe8b

trivial: fix typo and nits

dba4c8e

yyforyongyu force-pushed the remove-rbf-sweeper branch from 5d3c72b to dba4c8e Compare October 23, 2023 05:03

yyforyongyu requested a review from ziggie1984 October 23, 2023 05:10

ziggie1984 approved these changes Oct 23, 2023

View reviewed changes

Roasbeef approved these changes Oct 25, 2023

View reviewed changes

Roasbeef merged commit a1fa195 into lightningnetwork:master Oct 25, 2023

yyforyongyu deleted the remove-rbf-sweeper branch October 26, 2023 00:24

Conversation

yyforyongyu commented Oct 12, 2023

Uh oh!

Roasbeef left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ziggie1984 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yyforyongyu commented Oct 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ziggie1984 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Roasbeef commented Oct 25, 2023

Uh oh!

Roasbeef left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ziggie1984 left a comment •

edited

Loading

yyforyongyu commented Oct 17, 2023 •

edited

Loading

ziggie1984 left a comment •

edited

Loading