[7/5]contractcourt: make use of the new sweeper params #8514

yyforyongyu · 2024-03-02T11:40:54Z

Decided to open this final PR that makes use of the new sweeper params, so we can focus on deciding the proper deadlines and budgets to be used here.

TODO: add detailed docs here.

Depends on,

This change is

coderabbitai · 2024-03-02T11:41:00Z

Important

Auto Review Skipped

Auto reviews are disabled on this repository.

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.

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 testing code 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 testing code 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 testing code.
- @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.
Please see the configuration documentation for more information.
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/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

This commit removes the method `CreateSweepTx` and makes sure when sweeping the htlc output via the direct-preimage spend, it's offered via the `SweepInput` interface.

Previously we don't allow confTarget to be 0, which ended up the final position being never reached. We fix it here by allowing confTarget to be 0 in case the deadline has already been passed for a given input.

This commit adds a new config method `QueryIncomingCircuit` that can be used to query the payment's incoming circuit for giving its outgoing circuit key.

This commit moves the offering of second-level outputs one block earlier. The sweeper will check the required locktime and wait until it matures. This is needed so the second-level outputs can be aggregated properly.

If anything happens during the fee bumping process, and causes the input to be failed, we should be able to mark it as `PublishFailed`.

This commit adds an itest case that focuses on validating the CPFP logic in anchor sweeping.

The old commitment deadline is removed as it's no longer relevant.

This commit adds a new check for neutrino backend - when the inputs in the sweeping tx are spent by a third party, we will send back a `TxFailed` event to free the rest of the inputs for re-grouping.

Roasbeef

LGTM 🫡

ziggie1984

Fwiw still left some comments.

ziggie1984 · 2024-04-12T00:00:41Z

lncfg/sweeper.go

+	if s.NoDeadlineConfTarget < 144 {
+		return fmt.Errorf("nodeadlineconftarget must be at least 144")
+	}


I would prefer to have the limit not restricted. One argument for having a limit is that eventually we want to have this outputs confirmed. I was just referring to the fact, that all non-timesenstive-funds can always be recovered even if the node crashes and only the chanbackup is available.

ziggie1984 · 2024-04-12T00:50:19Z

sample-lnd.conf

+; fees when sweeping it.
+; sweeper.budget.tolocalratio=0.5
+
+; The amount in satoshis to allocate as the budget to pay fees when CPFPing a


I thought it might be more intuitive, we only CPFP if there are HTLCs at risk, so I thought it would make sense to just use their budget limit as a Cap ?

ziggie1984 · 2024-04-12T01:10:15Z

contractcourt/channel_arbitrator.go

+		// TODO(yy): If there's no corresponding incoming HTLC, it
+		// means we are the first hop, hence the payer. This is a
+		// tricky case - unlike a forwarding hop, we don't have an
+		// incoming HTLC that will time out, which means as long as we
+		// can learn the preimage, we can settle the invoice (before it
+		// expires?).


ok, but how would we cancel the htlc from the commitment as a sender ?

ziggie1984 · 2024-04-12T01:23:48Z

contractcourt/channel_arbitrator.go

-		c.cfg.ChanPoint, deadline, deadlineMinHeight, heightHint)
+	// Calculate the value left after subtracting the budget used for
+	// sweeping the time-sensitive HTLCs.
+	valueLeft := totalValue - calculateBudget(


Ok, so I was thinking that the buget value would cap a specific htlc rather than be considered as bundling all htlcs together and then considering the budget. I suggested taking every htlc separately into account and calculating the remaining value for every htlc with a single DeadlineHTLC , and then summing up the values for the remaining amount..

ziggie1984 · 2024-04-12T01:23:58Z

contractcourt/channel_arbitrator.go

+		c.cfg.Budget.DeadlineHTLC,
+	)
+
+	log.Debugf("ChannelArbitrator(%v): calculated valueLeft=%v, "+


Nit: Can we explain this value valueLeft or maybe substitute it for the difference htlcamt-budget, otherwise I find it very hard to reason about it when reading the logs.

ziggie1984 · 2024-04-13T10:52:07Z

contractcourt/channel_arbitrator.go

+		budget := calculateBudget(
+			value, c.cfg.Budget.AnchorCPFPRatio,
+			c.cfg.Budget.AnchorCPFP,
+		)


Ok understand, so I think capping the anchors is good. Wdyt about calculating the value under protection by considering every htlc separately. Lets say you have 2 0.2 BTC HTLCs on a 1 BTC channel with the HTLC_Budget_Cap 0.1 and a ratio of 50%. The left value would be: 0.4*0.5 = 0.2BTC, capped at 0.1BTC. So it suggests that the budget is only 0.1, tho when sweeping the htlcs, a budget of 0.1 each will be used for every htlc separately so the real budget would be 0.2BTC.

ziggie1984 · 2024-04-14T16:14:10Z

contractcourt/htlc_success_resolver.go

@@ -349,6 +349,25 @@ func (h *htlcSuccessResolver) broadcastReSignedSuccessTx() (
 			"height %v", h, h.htlc.RHash[:], waitHeight)
 	}

+	// Deduct one block so this input is offered to the sweeper one block


This is still needed I guess although we force sweeps now ?

ziggie1984 · 2024-04-14T23:50:55Z

itest/lnd_multi-hop_test.go

+	// The above mined block will trigger Carol's sweeper to publish the
+	// anchor sweeping tx.
+	//
+	// TODO(yy): should instead cancel the broadcast of the anchor sweeping


I am not sure whether I understand this TODO correctly, we cannot remove the anchor-cpfp, so you want to RBF the wallet input it consumes ?

ziggie1984 · 2024-04-14T23:59:22Z

sweep/fee_bumper.go

+
+		// Do a non-blocking read to see if the output has been spent.
+		select {
+		case spend, ok := <-spendEvent.Spend:


I wonder if the time lacking in the RegisterSpendNtfn rescan logic could cause problems here during restarts here?

contractcourt/commit_sweep_resolver.go

saubyk added this to the v0.18.0 milestone Mar 3, 2024

saubyk assigned yyforyongyu Mar 3, 2024

morehouse mentioned this pull request Mar 6, 2024

[bug]: anchor sweep fee rate not adjusted in state StateCommitmentBroadcasted #8522

Closed

yyforyongyu force-pushed the arbitrator-use-sweeper branch from d95f1b5 to 9d4a6f6 Compare March 19, 2024 04:58

yyforyongyu force-pushed the elle-sweeper-fee-bump branch from 33c575c to bed81ea Compare March 19, 2024 04:59

yyforyongyu marked this pull request as ready for review March 19, 2024 04:59

saubyk requested review from ProofOfKeags and Crypt-iQ March 19, 2024 16:32

yyforyongyu force-pushed the elle-sweeper-fee-bump branch from bed81ea to e0d8c63 Compare March 20, 2024 03:58

yyforyongyu force-pushed the arbitrator-use-sweeper branch 2 times, most recently from cf30795 to 43fa9fa Compare March 20, 2024 09:33

yyforyongyu changed the base branch from elle-sweeper-fee-bump to master March 20, 2024 09:33

yyforyongyu changed the base branch from master to elle-sweeper-fee-bump March 20, 2024 09:34

yyforyongyu force-pushed the elle-sweeper-fee-bump branch 2 times, most recently from 4e5cfb8 to f420f90 Compare March 20, 2024 10:56

yyforyongyu force-pushed the arbitrator-use-sweeper branch 2 times, most recently from 3a6b9e4 to 8bbadc1 Compare March 21, 2024 02:19

yyforyongyu force-pushed the elle-sweeper-fee-bump branch 2 times, most recently from 39bdd14 to 6280f73 Compare March 21, 2024 06:31

yyforyongyu force-pushed the arbitrator-use-sweeper branch 2 times, most recently from 0ed10be to 3f47dde Compare March 21, 2024 12:35

ziggie1984 mentioned this pull request Mar 25, 2024

[feature]: Include ParentFee when doing a CPFP using bumpFee RPC #7652

Open

yyforyongyu changed the base branch from elle-sweeper-fee-bump to elle-sweeper-finalize March 25, 2024 13:00

yyforyongyu force-pushed the arbitrator-use-sweeper branch from 3f47dde to e194841 Compare March 25, 2024 13:01

yyforyongyu changed the title ~~[6/5]contractcourt: make use of the new sweeper params~~ [7/5]contractcourt: make use of the new sweeper params Mar 25, 2024

yyforyongyu force-pushed the elle-sweeper-finalize branch from 0250874 to 6e2eebc Compare March 25, 2024 13:06

yyforyongyu force-pushed the arbitrator-use-sweeper branch from e194841 to 97c8574 Compare March 25, 2024 13:07

yyforyongyu mentioned this pull request Mar 25, 2024

[6/5]sweep: make sweeper ready to be used in contractcourt #8581

Merged

5 tasks

yyforyongyu force-pushed the elle-sweeper-finalize branch from 6e2eebc to d62a75f Compare March 26, 2024 03:02

yyforyongyu added 7 commits April 12, 2024 00:54

contractcourt: specify deadline and budget for nursery

a5d7e85

contractcourt+sweep: offer direct-preimage spend via SweepInput

996e0fb

This commit removes the method `CreateSweepTx` and makes sure when sweeping the htlc output via the direct-preimage spend, it's offered via the `SweepInput` interface.

sweep: make sure max fee rate can be reached

4c18d8a

Previously we don't allow confTarget to be 0, which ended up the final position being never reached. We fix it here by allowing confTarget to be 0 in case the deadline has already been passed for a given input.

multi: query circuit map inside contractcourt

ceabae3

This commit adds a new config method `QueryIncomingCircuit` that can be used to query the payment's incoming circuit for giving its outgoing circuit key.

contractcourt: add locks in SubscribeChannelEvents

48cd001

contractcourt: offer second-level outputs at CSV-1

082a460

This commit moves the offering of second-level outputs one block earlier. The sweeper will check the required locktime and wait until it matures. This is needed so the second-level outputs can be aggregated properly.

sweep: allow published input to be marked as PublishFailed

91061a6

If anything happens during the fee bumping process, and causes the input to be failed, we should be able to mark it as `PublishFailed`.

yyforyongyu force-pushed the arbitrator-use-sweeper branch from bd1027c to 6549ba3 Compare April 11, 2024 18:23

yyforyongyu force-pushed the elle-new-sweeper branch from 03aac0e to ddacbca Compare April 11, 2024 18:24

yyforyongyu force-pushed the arbitrator-use-sweeper branch 2 times, most recently from bc4592c to 0b31496 Compare April 12, 2024 07:01

multi: add itest testSweepAnchorCPFPLocalForceClose

583307f

This commit adds an itest case that focuses on validating the CPFP logic in anchor sweeping.

yyforyongyu force-pushed the arbitrator-use-sweeper branch 2 times, most recently from 0f530eb to 8d55db8 Compare April 12, 2024 17:59

yyforyongyu added 9 commits April 13, 2024 02:09

itest+lntest: add itest testSweepHTLCs to check HTLC sweepings

cc2b47e

itest: remove old CPFP tests

3bdcace

The old commitment deadline is removed as it's no longer relevant.

itest+lntest: fix channel force close itest

2e38d02

itest: fix multi-hop itest

cb2f5d4

itest: fix channel backup tests

265bbc6

itest: fix revocation itest

5f3b444

itest+lntest: fix onchain tests

dd17504

itest: fix watchtower test

cf983cf

sweep: catch third party spent in fee bumper for neutrino

4639a39

This commit adds a new check for neutrino backend - when the inputs in the sweeping tx are spent by a third party, we will send back a `TxFailed` event to free the rest of the inputs for re-grouping.

yyforyongyu force-pushed the arbitrator-use-sweeper branch from 8d55db8 to 4639a39 Compare April 12, 2024 18:11

Roasbeef approved these changes Apr 12, 2024

View reviewed changes

Roasbeef merged this pull request into lightningnetwork:elle-new-sweeper Apr 12, 2024
19 of 27 checks passed

yyforyongyu deleted the arbitrator-use-sweeper branch April 13, 2024 06:44

yyforyongyu mentioned this pull request Apr 13, 2024

[8/5]: update BumpFee RPC and always sweep on startup #8613

Merged

coderabbitai bot approved these changes Apr 14, 2024

View reviewed changes

ziggie1984 reviewed Apr 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[7/5]contractcourt: make use of the new sweeper params #8514

[7/5]contractcourt: make use of the new sweeper params #8514

yyforyongyu commented Mar 2, 2024 •

edited by Roasbeef

Loading

coderabbitai bot commented Mar 2, 2024 •

edited

Loading

Auto Review Skipped

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (`.coderabbit.yaml`)

Documentation and Community

Roasbeef left a comment

ziggie1984 left a comment

ziggie1984 Apr 12, 2024

ziggie1984 Apr 12, 2024

ziggie1984 Apr 12, 2024

ziggie1984 Apr 12, 2024

ziggie1984 Apr 12, 2024

ziggie1984 Apr 13, 2024

ziggie1984 Apr 14, 2024

ziggie1984 Apr 14, 2024

ziggie1984 Apr 14, 2024

[7/5]contractcourt: make use of the new sweeper params #8514

[7/5]contractcourt: make use of the new sweeper params #8514

Conversation

yyforyongyu commented Mar 2, 2024 • edited by Roasbeef Loading

coderabbitai bot commented Mar 2, 2024 • edited Loading

Auto Review Skipped

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (.coderabbit.yaml)

Documentation and Community

Roasbeef left a 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

yyforyongyu commented Mar 2, 2024 •

edited by Roasbeef

Loading

coderabbitai bot commented Mar 2, 2024 •

edited

Loading

CodeRabbit Configration File (`.coderabbit.yaml`)