sweepbatcher: allow adding groups of inputs #911

starius · 2025-04-01T03:52:04Z

A group of inputs can be added by passing it in SweepRequest.Inputs field.
All the inputs belong to the same swap and are added to the same batch.

Pull Request Checklist

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

bhandras

Looks really good, just a few nits and comments.

bhandras · 2025-04-01T19:27:01Z

sweepbatcher/greedy_batch_selection.go

 	if err != nil {
 		return fmt.Errorf("failed to estimate tx weight for "+
-			"sweep %x: %w", sweep.swapHash[:6], err)
+			"swap %s: %w", swap[:6], err)


nit: i'd not rename "sweep" to "swap" as sweep is more general. Applies to everywhere else too.

bhandras · 2025-04-01T19:50:52Z

sweepbatcher/sweep_batch.go

+	}
+
+	// Track if there is an existing sweep and a new sweep among the sweeps.
+	var hasExisting, hasNew bool


nit: instead of hasExisting and hasNew consider using a counter instead. It might make the code a bit more readable.

Replaced the flags with counts numExisting, numNew.

bhandras · 2025-04-01T19:52:40Z

sweepbatcher/sweep_batch.go

+	// Make sure the whole group is either new or existing. If this is not
+	// the case, this might be a bug, so print a warning.
+	if hasExisting && hasNew {
+		b.Warnf("There are existing and new sweeps among the group")


If using a counter approach we could also add more context to this warn log by logging the count of existing vs len(sweeps).

bhandras · 2025-04-01T19:53:35Z

sweepbatcher/sweep_batch.go

+	}
+
+	// Make sure all the sweeps spend different outpoints.
+	used := make(map[wire.OutPoint]struct{}, len(sweeps))


nit: instead of used we could call it outpoints for readability.

Renamed it to outpointsSet.

bhandras · 2025-04-01T20:24:49Z

sweepbatcher/sweep_batcher.go

+	// Group specifies multiple inputs in the same request. All the inputs
+	// belong to the same swap and are added to the same batch. If it is
+	// specified, OutPoint and Value must not be specified.
+	Group []Input


Could we just have the group? We don't really need the Output and Value then.

Fixed. Thanks!
Also renamed the field from Group to Inputs.

bhandras · 2025-04-01T20:25:53Z

sweepbatcher/sweep_batcher.go

+
+	if len(sweepReq.Group) != 0 {
+		// Make sure sweepReq.Outpoint, sweepReq.Value were not filled.
+		if sweepReq.Outpoint != (wire.OutPoint{}) {


... So then we wouldn't need these checks either.

bhandras · 2025-04-01T20:26:23Z

sweepbatcher/sweep_batcher.go

+		return sweeps, nil
+	}
+
+	s, err := b.loadSweep(


... And this loadSweep.

bhandras

LGTM 🎉

bhandras · 2025-04-02T10:36:48Z

sweepbatcher/sweep_batch.go

+
+	// Make sure the whole group is either new or existing. If this is not
+	// the case, this might be a bug, so print a warning.
+	if numExisting > 0 && numNew > 0 {


nit: we could also just check countExisting != len(sweeps).

We shouldn't return an error, if all the sweeps added are new. If all sweeps are new, countExisting=0 (and numNew=len(sweeps)).

bhandras · 2025-04-02T10:37:10Z

sweepbatcher/sweep_batch.go

-		if err != nil {
-			return false, err
+	// For an existing group, update the sweeps in the batch.
+	if numExisting > 0 {


nit: ... and countExisting == len(sweeps)

hieblmi

Nice improvement. I left a question.

hieblmi · 2025-04-02T13:13:16Z

sweepbatcher/sweep_batch.go

+		return false, err
+	}
+
+	// Track if there is an existing sweep and a new sweep among the sweeps.


nit: Track how many new and existing sweeps are among the sweeps.

Fixed. Thanks!

hieblmi · 2025-04-02T13:23:42Z

sweepbatcher/sweep_batch.go

-	// Add the sweep to the batch's sweeps.
-	b.Infof("adding sweep %x", sweep.swapHash[:6])
-	b.sweeps[sweep.outpoint] = *sweep
+	// Here is the code to add new sweeps to a batch.


should we check here that len(sweeps) is numNew and return an error otherwise.

This logically follows from the way numExisting and numNew were calculated and from the fact that we have checked above that only one of these categories has any sweeps in it.

I added the following double-check just in case:

} else if numNew != len(sweeps) { // Sanity check: all the sweeps must be either existing or new. // We have checked this above, let's check here as well. return false, fmt.Errorf("bug in numExisting and numNew logic:"+ " numExisting=%d, numNew=%d, len(sweeps)=%d, "+ "len(b.sweeps)=%d", numExisting, numNew, len(sweeps), len(b.sweeps))

hieblmi · 2025-04-02T13:25:02Z

sweepbatcher/sweep_batch.go

+		}
+
+		if err := b.persistSweep(ctx, *s, false); err != nil {
+			return true, err


does this count as accepted? Should we return false here?

It was accepted in-memory, but failed to be persisted. Loop will shutdown with this error anyway.

Also this matches to the previous behavior:

return true, b.persistSweep(ctx, *sweep, false)

starius · 2025-04-02T19:29:18Z

Added a check to addSweeps that the number of sweeps is not zero.

        // This must be a bug, so log a warning.
        if len(sweeps) == 0 {
                b.Warnf("An attempt to add zero sweeps.")

                return false, nil
        }

hieblmi

LGTM!

starius · 2025-04-03T12:48:34Z

Rebased

A group of inputs can be added by passing it in SweepRequest.Inputs field. All the inputs belong to the same swap and are added to the same batch.

starius force-pushed the group branch from ae2947c to 9f3b161 Compare April 1, 2025 04:18

starius changed the title ~~[WIP] sweepbatcher: allow adding groups of inputs~~ sweepbatcher: allow adding groups of inputs Apr 1, 2025

starius marked this pull request as ready for review April 1, 2025 04:18

starius requested review from bhandras, hieblmi and sputn1ck April 1, 2025 04:18

starius force-pushed the group branch 2 times, most recently from f46a1d9 to 790b8ed Compare April 1, 2025 15:56

starius mentioned this pull request Apr 1, 2025

sweepbatcher: add mode with presigned transactions #891

Merged

1 task

bhandras reviewed Apr 1, 2025

View reviewed changes

starius force-pushed the group branch from 790b8ed to 87619e0 Compare April 2, 2025 03:14

starius requested a review from bhandras April 2, 2025 03:17

bhandras approved these changes Apr 2, 2025

View reviewed changes

starius force-pushed the group branch from 87619e0 to 736cc04 Compare April 2, 2025 12:39

starius requested a review from bhandras April 2, 2025 12:59

hieblmi reviewed Apr 2, 2025

View reviewed changes

starius force-pushed the group branch 2 times, most recently from c1cb2bf to 8f050a7 Compare April 2, 2025 19:15

starius requested a review from hieblmi April 2, 2025 19:18

starius force-pushed the group branch 2 times, most recently from 45c24c0 to 283f31c Compare April 2, 2025 19:28

bhandras approved these changes Apr 3, 2025

View reviewed changes

hieblmi approved these changes Apr 3, 2025

View reviewed changes

starius force-pushed the group branch from 283f31c to e1a688e Compare April 3, 2025 12:48

sweepbatcher: allow adding groups of inputs

e8cf638

A group of inputs can be added by passing it in SweepRequest.Inputs field. All the inputs belong to the same swap and are added to the same batch.

starius force-pushed the group branch from e1a688e to e8cf638 Compare April 3, 2025 12:50

starius merged commit 46b4f56 into lightninglabs:master Apr 3, 2025
4 checks passed

starius deleted the group branch April 3, 2025 13:07

sweepbatcher: allow adding groups of inputs #911

sweepbatcher: allow adding groups of inputs #911

Uh oh!

Conversation

starius commented Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Checklist

Uh oh!

bhandras 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!

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!

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!

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!

bhandras 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!

Choose a reason for hiding this comment

Uh oh!

hieblmi 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!

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!

starius commented Apr 2, 2025

Uh oh!

hieblmi left a comment

Choose a reason for hiding this comment

Uh oh!

starius commented Apr 3, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

starius commented Apr 1, 2025 •

edited

Loading