[3/5]sweep: make pending inputs stateful #8423

yyforyongyu · 2024-01-25T01:40:53Z

Depends on

lnwallet: skip testmempoolaccept when the backend version is too low #8505

This PR prepares for #8424 by introducing states on the pending inputs so they are easier to be managed, in specific, SweepState is introduced and each pendingInput now has a state. Previously the field publishAttempts sort of tracks the pending input's state implicitly. We now replace it with the explicit state, and update it at different stages,

when it's first received, it's StateInit, which can be included in a new sweeping tx.
when it's included in a tx, it's StatePendingPublish, which cannot be included in another sweeping tx.
when it's published, it's StatePublished, which needs to be RBF-aware.
when it's successfully swept, it's StateSwept, which is the terminal state.
when it's excluded, it's StateExcluded, which is the terminal state.
otherwise, it's StateFailed, which is the terminal state, and we should analyze the failure.

With this change, it also means we won't give up sweeping a given input after 10 attempts anymore.

Summary by CodeRabbit

New Features
- Introduced stateful pending inputs in the sweeper for improved lifecycle management.
- Added functionality for aggregating inputs into clusters based on locktime and fee rates for sweeping transactions.
- New mock implementations for testing purposes, enhancing the development and testing workflow.
Enhancements
- Updated fee estimation logic across various components to require explicit specification of SatPerVbyte or TargetConf.
- Revised the handling of anchor resolution and exclusivity, improving error messaging and outcomes.
- Enhanced fee preference handling with the introduction of FeeEstimateInfo type, replacing FeePreference in multiple areas.
- Improved transaction record management in the database with new serialization, deserialization, and migration functions.
Bug Fixes
- Fixed conflicting nLockTime values in sweep transactions with the introduction of ErrLocktimeConflict.
- Addressed issues related to fee rate estimation and preference conflicts, enhancing error handling and user feedback.
Tests
- Extended test coverage with new test cases for transaction record management, fee estimation, and input aggregation.

coderabbitai · 2024-01-25T01:40:58Z

Important

Auto Review Skipped

Auto reviews are disabled on base/target branches other than the default branch. Please add the base/target branch pattern to the list of additional branches to be reviewed in the settings.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository.

To trigger a single review, invoke the @coderabbitai review command.

Walkthrough

The recent updates focus on refining fee estimation and transaction handling mechanisms across various components. Notably, the transition from FeePreference to FeeEstimateInfo across multiple files indicates a more nuanced approach to fee management. Additionally, the introduction of TargetConf fields, improvements in transaction record management, and enhancements in sweep transaction logic underscore efforts to optimize transaction execution and lifecycle management. The inclusion of mock implementations and test adjustments further supports robustness and reliability in testing environments.

Changes

Files	Change Summary
`chainntnfs/mocks.go`, `htlcswitch/mock.go`, `lnwallet/chainfee/mocks.go`, `sweep/mocks.go`, `sweep/store_mock.go`	Introduced or updated mock implementations for various interfaces and testing purposes.
`cmd/lncli/walletrpc_types.go`	Marked `NextBroadcastHeight` as deprecated in `PendingSweep` struct.
`contractcourt/.go`, `lnrpc/rpc_utils.go`, `lnrpc/walletrpc/walletkit_server.go`, `sweep/.go` (excluding mocks and tests)	Replaced `FeePreference` with `FeeEstimateInfo` and made related adjustments in fee handling and transaction logic.
`docs/release-notes/release-notes-0.18.0.md`, `itest/*`	Documented changes and updated integration tests to include `TargetConf` field and handle new fee estimation logic.
`lntest/harness.go`, `server.go`, `sweep/aggregator.go`, `sweep/store.go`, `sweep/txgenerator.go`, `sweep/walletsweep.go`	Various enhancements including input aggregation, transaction record management, and error handling improvements.

🐇✨
In the land of code, where transactions flow,
A rabbit hopped, with changes in tow.
"FeeEstimateInfo," it cheerfully said,
Making transactions smoother, ahead.
With every hop, and every line,
It sought to make the blockchain shine.
🌟🐾

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>.
- Generate unit-tests for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit tests for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai generate interesting stats about this repository and render them as a table.
- @coderabbitai show all the console.log statements in this repository.
- @coderabbitai read src/utils.ts and generate unit tests.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
The JSON schema for the configuration file is available here.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

CodeRabbit Discord Community

Join our Discord Community to get help, request features, and share feedback.

ProofOfKeags

I'm once again really impressed by the quality of these PRs. I have a couple comments to work through on this one but it's pretty much ready to ship.

sweep/sweeper.go

ProofOfKeags · 2024-02-08T22:55:13Z

sweep/sweeper.go

+
+	// Do a non-blocking read on the spent event channel.
+	select {
+	case details := <-mempoolSpent.Spend:


Is this guaranteed to be immediately available?

Only if this outpoint has already been spent in mempool, so it will block when a) the tx is not in mempool, or b) the tx has already been confirmed, which are both fine as we are doing a non-blocking read here.

Can you point to the code where this happens? I don't think that the result is immediately available

Sure, so here are the lines,

SubscribeMempoolSpent calls NotifySpend here:

lnd/chainntnfs/bitcoindnotify/bitcoind.go

Line 1060 in d41b86b

return event, b.chainConn.NotifySpent(ops)

NotifySpend then calls LookupInputSpend here

LookupInputSpend will return the tx if it's already in mempool, as validated by this test.

That doesn't look immediate because the call to onRelevantTx puts the tx into a concurrent queue

Turns out you are right. This leads to a bigger change as there's no way to do mempool look ups for btcd, and for bitcoind it's easier as we already have the method in place. So we'll need to

implement gettxspendingprevout in btcd, Add gettxspendingprevout for btcd and fix version check btcsuite/btcd#2125

add a mempool lookup method in btcwallet, chain: add mempool lookup for outpoints btcsuite/btcwallet#910

make use of it here.

Now looks super clean 👍

sweep/sweeper.go

ProofOfKeags · 2024-02-08T23:06:33Z

sweep/sweeper.go

-	if pendingInput.publishAttempts > 0 {
-		pendingInput.minPublishHeight = s.currentHeight
-	}
+	// TODO(yy): a dedicated state?


No I think resetting to an initial state is ideal if we can get away with it.

Hmm I am not really sure, imagine the user tries to decrease the fee, we should prob. report that we already published a higher fee rate tx and this operation makes no sense ?

imagine the user tries to decrease the fee

I don't think this is possible?

Can't they lower it through BumpFee?

Hmm I am not really sure, imagine the user tries to decrease the fee, we should prob. report that we already published a higher fee rate tx and this operation makes no sense ?

Maybe? More states, more problems: you have to ensure all possible operations that yield state transitions can be interleaved in different ways and have predictable behavior.

I don't think this is possible?

Yeah, as since we use testmempoolaccept now, we'd get an error that the fee increase wasn't high enough.

I think it is more correct to to use a new state though as: the input may have already been published with active RBF state by the time a user attempt to do a manual fee bump.

sweep/sweeper.go

Crypt-iQ · 2024-02-21T16:59:37Z

sweep/sweeper.go

+
+	// Do a non-blocking read on the spent event channel.
+	select {
+	case details := <-mempoolSpent.Spend:


Can you point to the code where this happens? I don't think that the result is immediately available

sweep/sweeper.go

Crypt-iQ · 2024-02-22T16:10:11Z

contractcourt/anchor_resolver.go

+		// This happens when the same anchor output is swept by both
+		// remote and local force close transactions. When one
+		// confirms, the other is excluded.
+		case sweep.ErrExclusiveGroupSpend:


If the anchor resolver exists, then the commitment transaction is confirmed so I don't think this case can be hit?

Good call, think this should be handled at the sweepAnchors side in channel arbitrator, tho not sure if it's needed as we are not catching the result there anyway.

ziggie1984

I like the new statefulness of the sweeper inputs, in addition I think its very good that we removed themaxattempt number for failed published inputs, which had some side effects especially removing inputs hence also the notifications for inputs which did not confirm quickly.

Had questions mostly.

As commented in the prior PRs it makes sense to merge/approve those PRs as a package.

sweep/sweeper.go

sweep/sweeper_test.go

ziggie1984 · 2024-02-27T09:45:36Z

sweep/sweeper.go

-	if pendingInput.publishAttempts > 0 {
-		pendingInput.minPublishHeight = s.currentHeight
-	}
+	// TODO(yy): a dedicated state?


Hmm I am not really sure, imagine the user tries to decrease the fee, we should prob. report that we already published a higher fee rate tx and this operation makes no sense ?

sweep/sweeper.go

morehouse

I like that this PR leverages the mempool to check for previous spends of each input. Maybe we should leverage the mempool even more to detect and outbid spends from other parties.

morehouse · 2024-03-25T21:36:25Z

sweep/sweeper.go

+		// If this input has been included in a sweep tx that's not
+		// published yet, we'd skip this input and wait for the sweep
+		// tx to be published.
+		if input.state == StatePendingPublish {
+			continue
+		}


I think we should log an error in this case.

StatePendingPublish is a temporary state that should transition to StatePublished or StatePublishFailed before sweep returns. If we see it here, something is broken.

And if we do somehow get to this state, the continue makes us forever ignore that input... Would it be better to reattempt sweeping the input?

Nice catch - this will be clearer in PR 5 tho, where we peel off the publishing logic from the sweeper and create a tx publisher - it will make more sense to use this state there. But I do find a potential place we may end up stuck in this state in the final PR, need to think about it more..

How can it get stuck in this state? IIUC, we have a single event loop in the sweeper as is. After something is marked as StatePendingPublish, then it can only go to published or failed as mentioned above.

The only case I can see plainly is if the call to StoreTx fails (something something postgres txn issue), so then the transaction is published, but all inputs in that set are still marked as StatePendingPublish. In that case, we should mark the publish attempt as failed.

morehouse · 2024-03-25T21:44:23Z

sweep/sweeper.go

+	txid := tx.TxHash()
+	tr, err := s.cfg.Store.GetTx(txid)


We look up the transaction in the mempool to get tx, and then we look up tx.TxHash() in the store to get the RBF info. Perhaps we should determine the RBF info directly from the tx instead.

This would be more accurate in situations where another party is also allowed to spend the input (e.g., HTLC spends).

hmmm not sure if I understand it correctly, are you saying the stored tx can be different from the tx fetched from the mempool?

Perhaps we should determine the RBF info directly from the tx instead.

Deriving the fee information requires access to all the previous inputs to have access to the total input sum. So we'd need to iteratively fetch the transactions of all the referenced inputs, but some of those might already be confirmed, so we can't get it solely from the mempool.

Exactly imagine the case where and HTLC is swept by the preimage after we registered it with the sweeper. But this preimage monitoring is already handled in the resolver. Not sure if we wanna open this discussion here because we immediately fails the incoming back as soon as we see the preimage in mempool

morehouse · 2024-03-25T22:00:01Z

sweep/sweeper.go

+// state Init. When spent, it will query the sweeper store to fetch the fee
+// info of the spending transction, and construct an RBFInfo based on it.
+// Suppose an error occurs, fn.None is returned.
+func (s *UtxoSweeper) decideStateAndRBFInfo(op wire.OutPoint) (


So the basic idea for this function is that it allows us to recover RBF info after a restart, right?

I wonder if we should also use this function to detect a competing spend from another party. For example, suppose we have an HTLC-Timeout / preimage spend bidding war going on. It would be good to detect the other party's fee/rate so we can replace their transaction in the mempool.

Or does the "fee bumper" appropriately handle this case?

So the basic idea for this function is that it allows us to recover RBF info after a restart, right?

Exactly. And the rest should be handled by the fee bumper - however, the whole recovering-from-restart is not implemented in these PR series in order to limit the scope. With the existing design, I think it should be straightforward - we just need to grab the inputs here, make them into a set, and re-offer it to the fee bumper.

As for the race case, it's more complicated. My general approach is to avoid it as much as possible, by extracting the preimage from the mempool, #7564 helps, tho it'd be nicer if we could also have #7615. As for the role of fee bumper, it just blindly does RBF when a new block comes in, and will use up all the budget configured by the user by the time the race happens.

For example, suppose we have an HTLC-Timeout / preimage spend bidding war going on. It would be good to detect the other party's fee/rate so we can replace their transaction in the mempool.

So for that, we created this PR (#7564) which allows resolvers to directly get notified if a spend of a transaction happens in the mempool.

morehouse · 2024-03-25T22:06:16Z

A rebase would also be nice -- it seems there's some already merged changes in the diff.

Roasbeef · 2024-03-26T23:46:52Z

Maybe we should leverage the mempool even more to detect and outbid spends from other parties.

I don't think we should extend the scope of this PR further. We should also be careful to not let considerations of more adversarial edge cases (malicious pinning and other funny business) overshadow the more fundamental considerations of making sure everything can be confirmed in a timely manner in routine conditions (eg: we have no general solution for "pinning"). Lest we spend a lot of time and energy attempting to target more adversarial scenarios that may not be precisely defined, invariably adding more complexity to protect against edge cases that are rare with unclear payoffs for would be attackers.

I think if we want to add more elaborate strategies for fee bumping, time sensitive bidding battles, etc -- we should also consider design paths that allow more of this logic to live encapsulated within the resolver. The resolver's duty is to make sure that the contract is fully resolved on chain. Part of that duty can be extended to include broader mempool awareness and defensive strategies in the face of anomalous confirmation delays (pinning, etc, etc).

Roasbeef

LGTM 🏄🏾

Roasbeef · 2024-03-27T00:00:20Z

sweep/sweeper.go

 			Err: ErrExclusiveGroupSpend,
 		})

+		// Update the input's state as it can no longer be swept.
+		input.state = StateExcluded


Will this actually modify the value in the map, or do we also need to do a re-assignment?

Looks to be fine: https://go.dev/play/p/W06zPXRwDFS

yeah input is a pointer so we are fine here.

Roasbeef · 2024-03-27T00:04:11Z

sweep/sweeper.go

+	//
+	// TODO(yy): sweeper is made to communicate via go channels, so no
+	// locks are needed to access the map. However, it'd be safer if we
+	// turn this pendingInputs into a SyncMap in case we wanna add


We could alternatively just have pendingInputs be a variable that lives in the scope of the main event loop.

Non-blocking suggestion.

Roasbeef · 2024-03-27T00:09:23Z

sweep/sweeper.go

+		// If this input has been included in a sweep tx that's not
+		// published yet, we'd skip this input and wait for the sweep
+		// tx to be published.
+		if input.state == StatePendingPublish {
+			continue
+		}


How can it get stuck in this state? IIUC, we have a single event loop in the sweeper as is. After something is marked as StatePendingPublish, then it can only go to published or failed as mentioned above.

The only case I can see plainly is if the call to StoreTx fails (something something postgres txn issue), so then the transaction is published, but all inputs in that set are still marked as StatePendingPublish. In that case, we should mark the publish attempt as failed.

Roasbeef · 2024-03-27T00:12:08Z

sweep/sweeper.go

@@ -157,6 +158,19 @@ func (s SweepState) String() string {
 	}
 }

+// RBFInfo stores the information required to perform a RBF bump on a pending
+// sweeping tx.
+type RBFInfo struct {


To be fully aware of all the RBF rules, don't we also need to track information related to the set of inputs in the publishing transaction? As an example: if the inputs are unconfirmed, if a new unconfirmed input was added in a recent iteration, etc.

Though part of me wonders if we can just get away more with the existing spray-and-pray approach...

yeah so the idea is, since we have txid, we can fetch the tx and get its inputs. meanwhile we have a set of inputs tracked in the sweeper, so can then use tx.TxIn to decide the starting state of the inputs in the sweeper.

Though part of me wonders if we can just get away more with the existing spray-and-pray approach...

Def, we just relentlessly call testmempoolaccept, not feasible for neutrino tho.

Roasbeef · 2024-03-27T00:18:03Z

sweep/sweeper.go

+	txid := tx.TxHash()
+	tr, err := s.cfg.Store.GetTx(txid)


Perhaps we should determine the RBF info directly from the tx instead.

Deriving the fee information requires access to all the previous inputs to have access to the total input sum. So we'd need to iteratively fetch the transactions of all the referenced inputs, but some of those might already be confirmed, so we can't get it solely from the mempool.

Roasbeef · 2024-03-27T00:20:02Z

sweep/sweeper.go

@@ -1041,36 +1023,6 @@ func (s *UtxoSweeper) markInputsPendingPublish(tr *TxRecord,

 		// Record another publish attempt.
 		pi.publishAttempts++
-


Can we also get rid of publishAttempts here? Elsewhere, we used this to detect if an input was published or not, but now we have SweepState. We could even morph things slightly and store the amount of attempts directly into the state.

Roasbeef · 2024-03-27T00:22:06Z

sweep/sweeper.go

-	if pendingInput.publishAttempts > 0 {
-		pendingInput.minPublishHeight = s.currentHeight
-	}
+	// TODO(yy): a dedicated state?


I don't think this is possible?

Yeah, as since we use testmempoolaccept now, we'd get an error that the fee increase wasn't high enough.

Roasbeef · 2024-03-27T00:23:30Z

sweep/sweeper.go

-	if pendingInput.publishAttempts > 0 {
-		pendingInput.minPublishHeight = s.currentHeight
-	}
+	// TODO(yy): a dedicated state?


I think it is more correct to to use a new state though as: the input may have already been published with active RBF state by the time a user attempt to do a manual fee bump.

Roasbeef · 2024-03-27T00:25:14Z

sweep/sweeper.go

@@ -1421,7 +1421,7 @@ func (s *UtxoSweeper) handleNewInput(input *sweepInputMessage) {
 	)
 	if err != nil {
 		err := fmt.Errorf("wait for spend: %w", err)
-		s.signalResult(pi, Result{Err: err})
+		s.markInputFailed(pi, err)


As is, this can only error out if we fail to obtain the spend notification. At this point, shouldn't we just tear down the entire sweeper to crash and then force a full daemon restart? If we can't get spend notifications, then we don't know when anything is confirmed, which would impact the sweeper's ability to know when it should stop trying to increase the fee.

This is a very good point! Will address this in PR 6 to avoid rebase conflicts.

Roasbeef · 2024-03-27T00:28:59Z

sweep/sweeper.go

+
+	// Extract the spending tx from the option.
+	var tx *wire.MsgTx
+	txOption.WhenSome(func(t wire.MsgTx) {


Can alternatively write this as:

tx := txOption.UnwrapOr(nil)

hmmm seems we need to update fn mod, gonna defer this change in later PRs.

ziggie1984

LGTM 👌, had only nits in the last pass through.

ziggie1984 · 2024-03-21T11:26:00Z

sweep/sweeper.go

+
+	// Do a non-blocking read on the spent event channel.
+	select {
+	case details := <-mempoolSpent.Spend:


Now looks super clean 👍

ziggie1984 · 2024-03-21T12:44:35Z

sweep/sweeper_test.go

+	//
+	// TODO(yy): this means the behavior of not having a mempool is the
+	// same as an erroneous mempool. The question is should we
+	// differentiate the two from their returned values?


I think as long as having no mempool returns an error, it seems to be fine and I don't think we need this TODO

cool removed the TODO.

ziggie1984 · 2024-03-21T12:52:09Z

sweep/sweeper.go

+
+	// StateExcluded is the state of a pending input that has been excluded
+	// and can no longer be swept. For instance, when one of the three
+	// anchor sweeping transactions confirmed, the rest two will be


Nit: :s/rest two/remaining two/g

ziggie1984 · 2024-03-21T13:13:40Z

sweep/sweeper_test.go

+		{Index: 3}: input3,
+	}
+
+	// We expect only the inputs with `StateInit` and `StatePendingPublish`


Nit: :s/StatePendingPublish/StatePublishFailed/g

sweep/sweeper.go

ziggie1984 · 2024-03-26T22:53:55Z

sweep/sweeper.go

+		pi.state = StatePublished
+	}
+
+	s.pendingInputs[outpoint] = pi
 	log.Tracef("input %v added to pendingInputs", outpoint)


Nit: Trace comment start with Captial Letters.

ziggie1984 · 2024-03-26T23:57:48Z

sweep/sweeper.go

+
+	// Exit early if it's not found.
+	//
+	// NOTE: this is not accurate for backends don't support mempool


Nit: :%s/backends don't support/backends that don't support/g

ziggie1984 · 2024-03-26T23:58:40Z

sweep/sweeper.go

+	// NOTE: this is not accurate for backends don't support mempool
+	// lookup:
+	// - for neutrino we don't have a mempool.
+	// - for bitcoind below v24.0.0 we don't have `gettxspendingprevout`.


I think this comment is not quite right, although we don't have the gettxspendingprevout we have a mempool cache for prior versions, so I think thats accurate as well for bitcoind < 24, not sure when we introduced the mempool cache though.

good catch - i think for bitcoind we can assume it's always supported as the cache has been there for at least a year now, removed the comment.

ziggie1984 · 2024-03-27T00:23:43Z

sweep/sweeper.go

+	txid := tx.TxHash()
+	tr, err := s.cfg.Store.GetTx(txid)


Exactly imagine the case where and HTLC is swept by the preimage after we registered it with the sweeper. But this preimage monitoring is already handled in the resolver. Not sure if we wanna open this discussion here because we immediately fails the incoming back as soon as we see the preimage in mempool

sweep/sweeper.go

This commit starts tracking the state of a pending input so it's easier to control the sweeping flow and provide RBF awareness in the future.

This commit changes how a new input sweep request is handled - now we will query the mempool and see if it's already been spent. If so, we'll update its state as we may need to RBF this input.

This commit uniforms and put the deletion of pending inputs in a single point.

This commit attaches RBFInfo to an input before it's been published or it's already been published.

This commit removes the logic where we remove an input when it's been published more than 10 times. This is needed as in our future fee bumper, we might start with a low fee and rebroadcast the same input for hundred of blocks.

…ublish` Now that the refactor is done, we start patching unit tests for these two methods. Minor changes are also made based on the feedback from the tests.

So we can use the methods implemented on the chain RPC client.

sweeper This commit implements a new method, `LookupInputMempoolSpend` to do lookups in the mempool. This method is useful in the case when we only want to know whether an input is already been spent in the mempool by the time we call.

Thus this method `decideStateAndRBFInfo` won't touch the state changes of a given input.

yyforyongyu mentioned this pull request Jan 25, 2024

[5/5]sweep: introduce budget-based deadline aware fee bumper #8424

Merged

7 tasks

yyforyongyu requested review from morehouse and ziggie1984 January 25, 2024 01:45

yyforyongyu force-pushed the sweeper-rbf-2 branch 6 times, most recently from 6058edc to dc65cd1 Compare January 27, 2024 02:14

saubyk added this to the v0.18.0 milestone Jan 28, 2024

yyforyongyu force-pushed the sweeper-rbf-2 branch from dc65cd1 to 26a2ed6 Compare January 29, 2024 01:32

yyforyongyu self-assigned this Jan 30, 2024

yyforyongyu added utxo sweeping llm-review add to a PR to have an LLM bot review it labels Jan 30, 2024

ProofOfKeags self-requested a review February 8, 2024 22:24

ProofOfKeags reviewed Feb 8, 2024

View reviewed changes

yyforyongyu force-pushed the sweeper-rbf-2 branch from 26a2ed6 to 957a019 Compare February 20, 2024 20:46

yyforyongyu requested review from Roasbeef, Crypt-iQ and ellemouton as code owners February 20, 2024 20:46

yyforyongyu changed the base branch from master to elle-sweeper-rbf-1 February 20, 2024 20:46

yyforyongyu removed the request for review from ellemouton February 20, 2024 20:56

coderabbitai bot reviewed Feb 20, 2024

View reviewed changes

sweep/sweeper.go Outdated Show resolved Hide resolved

yyforyongyu force-pushed the sweeper-rbf-2 branch from 957a019 to 6277f67 Compare February 20, 2024 23:22

Crypt-iQ reviewed Feb 22, 2024

View reviewed changes

yyforyongyu force-pushed the sweeper-rbf-2 branch from 6277f67 to 31d0b58 Compare February 22, 2024 18:23

ziggie1984 reviewed Feb 27, 2024

View reviewed changes

yyforyongyu force-pushed the elle-sweeper-rbf-1 branch 2 times, most recently from 598119e to 21b9cbd Compare February 27, 2024 15:48

yyforyongyu force-pushed the elle-new-sweeper branch from 98d691e to a605174 Compare March 21, 2024 06:27

yyforyongyu force-pushed the sweeper-rbf-2 branch 2 times, most recently from 5a14b90 to 3e70f2f Compare March 25, 2024 13:04

morehouse reviewed Mar 25, 2024

View reviewed changes

yyforyongyu force-pushed the sweeper-rbf-2 branch from 3e70f2f to ae42569 Compare March 26, 2024 03:00

yyforyongyu force-pushed the elle-new-sweeper branch from a605174 to 8e20164 Compare March 26, 2024 05:08

yyforyongyu force-pushed the sweeper-rbf-2 branch from ae42569 to dba41a2 Compare March 26, 2024 05:09

Roasbeef approved these changes Mar 27, 2024

View reviewed changes

ziggie1984 approved these changes Mar 27, 2024

View reviewed changes

yyforyongyu force-pushed the elle-new-sweeper branch from 8e20164 to 893799a Compare March 27, 2024 04:49

yyforyongyu added 15 commits March 27, 2024 12:49

sweep: make pending input stateful

ce8c8c9

This commit starts tracking the state of a pending input so it's easier to control the sweeping flow and provide RBF awareness in the future.

multi: query mempool spend when a new input is received

d67c7f4

This commit changes how a new input sweep request is handled - now we will query the mempool and see if it's already been spent. If so, we'll update its state as we may need to RBF this input.

sweep: delete pending inputs based on their states

41d9a81

This commit uniforms and put the deletion of pending inputs in a single point.

sweep: add fee info for published inputs

be3dfa4

This commit attaches RBFInfo to an input before it's been published or it's already been published.

sweep: don't give up an input based on number of attempts

f886f88

This commit removes the logic where we remove an input when it's been published more than 10 times. This is needed as in our future fee bumper, we might start with a low fee and rebroadcast the same input for hundred of blocks.

sweep: add method markInputFailed

207e006

sweep: patch unit tests for markInputsSwept and `markInputsPendingP…

4d3441d

…ublish` Now that the refactor is done, we start patching unit tests for these two methods. Minor changes are also made based on the feedback from the tests.

sweep: remove unused param testSpendChan

b12497b

sweep: fix logging

800f5ba

lnd+sweep: remove unused NextAttemptDeltaFunc

b078ad1

btcdnotify: use chain.NewRPCClientWithConfig to init RPC client

6471835

So we can use the methods implemented on the chain RPC client.

docs: update release notes

97a6f0a

lntest+itest: fix testOpenChannelLockedBalance

041aecf

sweep: refactor attachAvailableRBFInfo to decideStateAndRBFInfo

fa2c3cf

Thus this method `decideStateAndRBFInfo` won't touch the state changes of a given input.

yyforyongyu force-pushed the sweeper-rbf-2 branch from dba41a2 to fa2c3cf Compare March 27, 2024 05:22

yyforyongyu merged commit 987f524 into lightningnetwork:elle-new-sweeper Mar 27, 2024
25 of 27 checks passed

yyforyongyu deleted the sweeper-rbf-2 branch March 27, 2024 08:34

		@@ -1041,36 +1023,6 @@ func (s UtxoSweeper) markInputsPendingPublish(tr TxRecord,

		// Record another publish attempt.
		pi.publishAttempts++

[3/5]sweep: make pending inputs stateful #8423

[3/5]sweep: make pending inputs stateful #8423

Conversation

yyforyongyu commented Jan 25, 2024 • edited

Summary by CodeRabbit

coderabbitai bot commented Jan 25, 2024 • edited

Auto Review Skipped

Walkthrough

Changes

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (.coderabbit.yaml)

CodeRabbit Discord Community

ProofOfKeags left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ziggie1984 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morehouse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morehouse commented Mar 25, 2024

Roasbeef commented Mar 26, 2024

Roasbeef left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ziggie1984 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yyforyongyu commented Jan 25, 2024 •

edited

coderabbitai bot commented Jan 25, 2024 •

edited

CodeRabbit Configration File (`.coderabbit.yaml`)