sweepbatcher: add greedy batch selection algorithm #787

starius · 2024-06-30T03:12:19Z

New algorithm is tried to select a batch to add a sweep to. It selects a batch for the sweep using the greedy algorithm, which minimizes costs, and adds the sweep to the batch or creates new batch, if it is the most cost efficient alternative. If the batch does not accept the sweep (too long timeout distance), next batch is tried. If the whole new approach fails, the old algorithm is used, trying to add to any batch, or starting a new batch.

The PR also includes some refactorings and fixes small inaccuracies in fee estimations.

Pull Request Checklist

Update release_notes.md if your PR contains major features, breaking changes or bugfixes

lightninglabs-deploy · 2024-07-08T08:12:22Z

@bhandras: review reminder
@sputn1ck: review reminder
@hieblmi: review reminder

bhandras

Looks good, just a few comments!

bhandras · 2024-07-01T06:37:33Z

sweepbatcher/sweep_batcher.go

-		destAddr:               s.DestAddr,
-		minFeeRate:             s.MinFeeRate,
-	}, nil
+	return b.loadSweep(ctx, dbSweep.SwapHash, dbSweep.Outpoint,


bhandras · 2024-07-01T07:11:22Z

sweepbatcher/sweep_batcher.go

@@ -898,3 +886,12 @@ func (b *Batcher) loadSweep(ctx context.Context, swapHash lntypes.Hash,
 		minFeeRate:             s.MinFeeRate,
 	}, nil
 }
+
+// newBatchConfig creates new batch config.
+func (b *Batcher) newBatchConfig(maxTimeoutDistance int32) batchConfig {


bhandras · 2024-07-01T07:12:17Z

sweepbatcher/sweep_batcher.go

@@ -895,3 +875,18 @@ func (b *Batcher) newBatchConfig(maxTimeoutDistance int32) batchConfig {
 		customMuSig2Signer: b.customMuSig2Signer,
 	}
 }
+
+// newBatchKit creates new batch kit.
+func (b *Batcher) newBatchKit() batchKit {


bhandras · 2024-07-01T07:16:20Z

sweep/sweeper.go

+
+	// Add output.
+	if err := AddOutputEstimate(&weightEstimate, destAddr); err != nil {
+		return 0, 0, 0, fmt.Errorf("estimate fee: %w", err)


nit: error message seems out of context, perhaps a bit more detail could be helpful.

Fixed. Also added error message in case adding input fails.

bhandras · 2024-07-08T12:13:27Z

sweepbatcher/sweep_batcher.go

@@ -848,6 +846,19 @@ func (b *Batcher) loadSweep(ctx context.Context, swapHash lntypes.Hash,
 			swapHash[:6], err)
 	}

+	var minFeeRate chainfee.SatPerKWeight


I think this should actually be initialized to chainfee.AbsoluteFeePerKwFloor so if the custom fee rate provider is unset then it still makes sense.

minFeeRate is 0 be default. If customFeeRate is not provided, then rbfCache.FeeRate is also 0 and method batch.updateRbfRate() updates it to current fee rate according to batchConfTarget.

Added a comment to that code about this.

bhandras · 2024-07-08T13:25:24Z

sweepbatcher/greedy_batch_selection.go

+const specialId = -1
+
+// entity is either batch of sweep and it holds data important for selection.
+type entity struct {


I think entity is not necessarily the best name as it sounds a bit too general. Maybe consider something like sweepSet or sweepEntity? Also given that the role of this type is to store weight and fee information perhaps we can be even more specific: sweepFeeDetails?

I renamed entity to feeDetails.

sweepbatcher/greedy_batch_selection.go

bhandras · 2024-07-08T13:43:44Z

sweepbatcher/greedy_batch_selection.go

+}
+
+// estimateSweepWeight returns sweep's input weight and one sweep batch weight.
+func estimateSweepWeight(s *sweep) (*entity, *entity, error) {


nit: i think returning values instead of pointers is cleaner.

bhandras · 2024-07-08T13:47:18Z

sweepbatcher/greedy_batch_selection.go

+}
+
+// estimateSweepWeight returns sweep's input weight and one sweep batch weight.
+func estimateSweepWeight(s *sweep) (*entity, *entity, error) {


nit: perhaps naming this functions as estimateSweepFeeDelta or estimateSweepFeeIncrement could be more readable. Maybe the godoc can also be made clearer slightly.

Renamed to estimateSweepFeeIncrement and added more godoc:

// estimateSweepFeeIncrement returns fee details for adding the sweep to a batch // and for creating new batch with this sweep only. func estimateSweepFeeIncrement(s *sweep) (feeDetails, feeDetails, error) {

starius · 2024-07-08T17:16:17Z

@bhandras Thanks for the review! 💯

I fixed the comments. Please take another look!

bhandras

LGTM 🎉

bhandras · 2024-07-10T17:52:11Z

sweepbatcher/sweep_batcher.go

@@ -848,6 +846,22 @@ func (b *Batcher) loadSweep(ctx context.Context, swapHash lntypes.Hash,
 			swapHash[:6], err)
 	}

+	// minFeeRate is 0 be default. If customFeeRate is not provided, then


nit: s/be/by

Thanks! Fixed

bhandras · 2024-07-10T17:52:30Z

sweepbatcher/sweep_batcher.go

@@ -848,6 +846,19 @@ func (b *Batcher) loadSweep(ctx context.Context, swapHash lntypes.Hash,
 			swapHash[:6], err)
 	}

+	var minFeeRate chainfee.SatPerKWeight


bhandras · 2024-07-10T17:56:07Z

sweepbatcher/greedy_batch_selection.go

+	}
+
+	// Estimate new batch.
+	newBatch, err := estimateBatchWeight(batch)


nit: to avoid confusing names and improve readability, consider naming newBatch => feeDetails1 and batch2 => feeDetails2 or something similar as these are not batches.

I renamed them to fd1 and fd2.

bhandras · 2024-07-10T17:56:34Z

sweepbatcher/greedy_batch_selection.go

+// details has also IsExternalAddr flag. There is a rule that sweeps having flag
+// IsExternalAddr must go in individual batches. Cooperative spending is only
+// available if all the sweeps support cooperative spending path.
+func selectBatch(batches []feeDetails, sweep, oneSweepBatch feeDetails) (int32, error) {


nit: line too long

starius · 2024-07-11T02:38:33Z

Updated commit "sweepbatcher: fix inaccuracies in fee estimations": function publishBatch also used AddP2TROutput regardless of destination address type. Fixed it, similarly to publishBatchCoop function: use function AddOutputEstimate to estimate output size based on address type.

starius · 2024-07-11T13:32:05Z

Rebased

hieblmi

Great effort to save fees for the users of loop out💰. I've left a few comments.
I will test this locally next.

sweep/sweeper.go

sweepbatcher/sweep_batch.go

sweepbatcher/sweep_batcher.go

hieblmi · 2024-07-15T13:23:11Z

sweepbatcher/sweep_batcher.go

@@ -465,6 +470,19 @@ func (b *Batcher) handleSweep(ctx context.Context, sweep *sweep,
 		}
 	}

+	// If custom fee rate provider is used, run the greedy algorithm of
+	// batch selection to minimize costs.
+	if b.customFeeRate != nil {


Why do we try to minimize only if there's a custom fee provider active? Isn't it possible to optimize when multiple batches exist with the internal fee rate calculation?

Also, how are you planning to enable the custom fee rates? Will this be a user configured parameter which will be passed in as option to batcher := sweepbatcher.NewBatcher(?

Great idea! We can always use it. I pushed two commits:

sweepbatcher: always fill sweep.minFeeRate

sweepbatcher: always try greedy batch selection

to implement this.

Updated commit "sweepbatcher: always fill sweep.minFeeRate". Previous version of it caused an extra fee bump and failure of test TestCustomSweepConfTarget. Fixed.

Also pushed commit "loopout_test: enable logging in sweepbatcher", it helped debugging the issue.

sweepbatcher/sweep_batcher.go

sweepbatcher/greedy_batch_selection.go

sweepbatcher/greedy_batch_selection_test.go

sweepbatcher/sweep_batcher.go

sweepbatcher/greedy_batch_selection.go

hieblmi

LGTM 💾 Frequent loop-out users will love this feature!

GeorgeTsagk · 2024-07-16T09:21:33Z

sweepbatcher/greedy_batch_selection.go

+// of each alternative. Based on the estimates of selectBatch(), this method
+// adds the sweep to the batch that results in the least overall fee increase,
+// or creates new batch for it. If this method fails for whatever reason, the
+// caller falls back to the simple algorithm (method handleSweep).


👍 good idea to keep the existing mechanism as fall back

sweepbatcher/greedy_batch_selection.go

GeorgeTsagk · 2024-07-16T09:39:54Z

sweepbatcher/greedy_batch_selection.go

+	batchId, err := selectBatch(
+		batches, sweepFeeDetails, newBatchFeeDetails,
+	)
+	if err != nil {
+		return fmt.Errorf("batch selection algorithm failed for sweep "+
+			"%x: %w", sweep.swapHash[:6], err)
+	}
+
+	// If the best option is to start new batch, do it.
+	if batchId == newBatchSignal {
+		return b.spinUpNewBatch(ctx, sweep)
+	}
+
+	// Locate the batch to add the sweep to.
+	bestBatch, has := b.batches[batchId]
+	if !has {
+		return fmt.Errorf("batch selection algorithm returned "+
+			"batch id %d which doesn't exist, for sweep %x",
+			batchId, sweep.swapHash[:6])
+	}
+
+	// Add the sweep to the batch.
+	accepted, err := bestBatch.addSweep(ctx, sweep)


This may need to be enhanced

Currently, addSweep will also run checks that are related to the timeout window of the batch vs the sweep that is being added. See here https://github.com/lightninglabs/loop/blob/master/sweepbatcher/sweep_batch.go#L440-L447

This means that the single best solution that the greedy algorithm has calculated may be rejected because the timeout heights of the sweeps are too far away. I believe this is not the desired behavior?

I see 2 possible paths here:

a) Create a version of addSweep that does not check for the timeout window limits, allowing swaps with very distant timeout heights to be in the same batch
b) Enhance algorithm to return an array of "best batches", sorted from best to worst. Then on the above line (76) we would have to iterate over the results and try to addSweep in order from the best to worst option

Of course there's option "c) keep it as is"
which will probably cause high failure rates when calling greedyAddSweep, which is OK as long as we fall back to legacy batch selection

How often does it happen that a sweep is rejected based on the timeout? If that's not a common case then we can KISS and just try and add to the selected greedy batch, and if timeout prevents addition then fail the greedy algo and fall back to the default. Wdyt?
Didn't see your c), is a high failure rate expected?

Didn't see your c), is a high failure rate expected?

That highly depends on the type of looper, frequent loopers will probably encounter this more often, as they are the ones having many batches running in parallel

I like the option b) Enhance algorithm to return an array of "best batches", sorted from best to worst.

The selection algorithm already has a metric by which is compares all the alternatives, so it is easy to extend to return a sorted array.

I implemented it and pushed a temporary commit with this change. Please take a look! I'll squash it before merging.

GeorgeTsagk

awesome 🔥
LGTM

GeorgeTsagk · 2024-07-18T07:28:40Z

sweepbatcher/greedy_batch_selection.go

+			return nil
+		}
+
+		log.Infof("Batch selection algorithm returned batch id %d for "+


nit: I think it's better to do Debugf here

GeorgeTsagk · 2024-07-18T07:29:41Z

sweepbatcher/greedy_batch_selection.go

+	// Sort the alternatives by cost. The lower the cost, the better.
+	sort.Slice(alternatives, func(i, j int) bool {
+		return alternatives[i].cost < alternatives[j].cost
+	})


beautiful 💯

The reason is because we want to know in advance if fee rates come from an external source or are determined by sweepbatcher internally. Also remove WithNoBumping option. It is now a part of WithCustomFeeRate: if WithCustomFeeRate is passed, automatic fee bumping by sweepbatcher is disabled.

It adds output to transaction weight estimator depending on address type.

If custom fee rates are used, the algorithm is tried. It selects a batch for the sweep using the greedy algorithm, which minimizes costs, and adds the sweep to the batch. If it fails, old algorithm is used, trying to add to any batch, or starting a new batch.

Use SigHashDefault instead of SigHashAll in weight estimations. Actual code uses SigHashDefault, not SigHashAll. SigHashAll is 1wu larger. Use the actual destination address in weight estimator, not taproot always. In the test the type of address is P2WPKH, not taproot. Updated testSweepFetcher to calculate fee, fee rate and weight accurately and to compare the data observed in the published transaction with the estimates.

This is useful to debug. It used to fail silently.

Use customFeeRate if it is provided, otherwise use wallet's EstimateFeeRate. Added flag SkipNextBump to rbfCache to avoid extra bumping upon updating fee rate externally. Fix test TestSweepBatcherCloseDuringAdding, it didn't have confTarget and failed in wallet.EstimateFeeRate call.

Now that sweep.minFeeRate is always set, greedy batch selection has all needed inputs to work even without customFeeRate provider.

starius force-pushed the sweepbatcher-greedy-batch-selection branch from 34c2c1c to 5747074 Compare June 30, 2024 03:18

starius marked this pull request as ready for review July 1, 2024 02:39

starius requested review from hieblmi, bhandras and sputn1ck July 1, 2024 02:40

bhandras reviewed Jul 8, 2024

View reviewed changes

starius force-pushed the sweepbatcher-greedy-batch-selection branch from 6b0293a to bf645d0 Compare July 8, 2024 17:11

starius requested a review from bhandras July 8, 2024 17:14

bhandras approved these changes Jul 10, 2024

View reviewed changes

starius force-pushed the sweepbatcher-greedy-batch-selection branch 2 times, most recently from 18ab904 to 64dc3f2 Compare July 11, 2024 02:36

starius force-pushed the sweepbatcher-greedy-batch-selection branch from 64dc3f2 to 64d9938 Compare July 11, 2024 13:31

starius requested a review from GeorgeTsagk July 11, 2024 13:37

levmi assigned starius Jul 11, 2024

starius mentioned this pull request Jul 12, 2024

sweepbatcher: add mixed batches as an option #791

Merged

1 task

hieblmi reviewed Jul 15, 2024

View reviewed changes

starius force-pushed the sweepbatcher-greedy-batch-selection branch 2 times, most recently from ee100d7 to fece888 Compare July 15, 2024 18:03

starius requested a review from hieblmi July 15, 2024 18:07

hieblmi approved these changes Jul 16, 2024

View reviewed changes

GeorgeTsagk reviewed Jul 16, 2024

View reviewed changes

GeorgeTsagk approved these changes Jul 18, 2024

View reviewed changes

starius added 4 commits July 18, 2024 13:38

sweepbatcher: fix copy-paste in sweep creation

1290ebe

sweepbatcher: fix copy-paste with batchConfig

1f1a274

sweepbatcher: fix copy-paste with batch kit

ccd1b31

starius added 8 commits July 18, 2024 13:38

sweep: factor out function AddOutputEstimate

019d3c6

It adds output to transaction weight estimator depending on address type.

sweepbatcher: log batcher failures

f5e97c0

This is useful to debug. It used to fail silently.

sweep: fix godoc of GetSweepFeeDetails

5dac7ca

sweepbatcher: always try greedy batch selection

026cf0d

Now that sweep.minFeeRate is always set, greedy batch selection has all needed inputs to work even without customFeeRate provider.

loopout_test: enable logging in sweepbatcher

7a61d5e

starius force-pushed the sweepbatcher-greedy-batch-selection branch from 60d06ab to 7a61d5e Compare July 18, 2024 16:40

starius merged commit 4c49039 into lightninglabs:master Jul 18, 2024
4 checks passed

starius deleted the sweepbatcher-greedy-batch-selection branch July 18, 2024 17:19

sweepbatcher: add greedy batch selection algorithm #787

sweepbatcher: add greedy batch selection algorithm #787

Conversation

starius commented Jun 30, 2024 • edited Loading

Pull Request Checklist

lightninglabs-deploy commented Jul 8, 2024

bhandras 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

starius commented Jul 8, 2024

bhandras 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

starius commented Jul 11, 2024

starius commented Jul 11, 2024

hieblmi 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

hieblmi 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

hieblmi Jul 16, 2024 • edited Loading

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

GeorgeTsagk 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

starius commented Jun 30, 2024 •

edited

Loading

hieblmi Jul 16, 2024 •

edited

Loading