sweepbatcher: notify caller about confirmations by starius · Pull Request #919 · lightninglabs/loop

starius · 2025-04-09T03:08:59Z

Add fields ConfChan and ConfErrChan to sweepbatcher.SpendNotifier type which is a part of SweepRequest passed to AddSweep method.

This is needed to reuse confirmation notifications on the calling side the same way it is done for spending notifications.

Added missing tests for notifications sent through channels of SpendNotifier.

Pull Request Checklist

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

sputn1ck · 2025-04-10T10:02:16Z

@@ -213,8 +213,8 @@ type dbBatch struct {
 	// ID is the unique identifier of the batch.


Maybe stupid question, but missing lots of context. This commit changes more than the mock, and feels a bit complicated, can you maybe elaborate what the changes to the other files are?

I updated the commit message:

sweepbatcher: align dbBatch type with DB schema Previously, dbBatch had a State field (enum: Open, Closed, Confirmed), but in the database it is represented as a boolean Confirmed. The Closed state was stored the same way as Open. This wasn't an issue in practice, since an Open batch is quickly transitioned to Closed after startup. However, the in-memory mock stores plain dbBatch instances, leading to inconsistent behavior between the mock and the real DB-backed store. This commit updates dbBatch to match the database representation by replacing

Also added a note to the description of Closed const:

// Closed is the state in which the batch is no longer able to accept // new sweeps. NOTE: this state exists only in-memory. In the database // it is stored as Open and converted to Closed after a spend // notification arrives (quickly after start of Batch.Run). Closed batchState = 1

bhandras

Looks good. Perhaps we don't even need the "closed" state?

bhandras · 2025-04-10T11:53:03Z

+		// If the sweep's notifier is empty then this means that a swap
+		// is not waiting to read an update from it, so we can skip
+		// the notification part.
+		if s.notifier == nil || *s.notifier == (SpendNotifier{}) {


Why check *s.notifier == (SpendNotifier{} ?

It was a copy-paste. I updated this line:

if s.notifier == nil || s.notifier.SpendErrChan == nil {

bhandras · 2025-04-10T12:08:40Z


 	if row.Confirmed {
-		batch.State = batchOpen
+		batch.State = batchConfirmed


I assume this just resulted in the batches restarting (and then getting to confirmed state again after spend notification received)?

Yes, and then after confirmation notification (after 3 confs).

Open -(spend notif)-> Closed -(conf notif)-> Confirmed

bhandras · 2025-04-10T12:13:46Z

 	// Then we get the total amount that was swept by the batch.
 	totalSwept, err := b.store.TotalSweptAmount(ctx, parentBatchID)
 	if err != nil {
+		cancel()


Hmm, this is a great fix, but how did we ever not get cancelled?

This code path is executed only when a fully confirmed sweep is added again to the sweeper. This may happen if the daemon was down when the sweep was fully confirmed (got 3rd confirmation). This is not the main code path.

+1, great catch

bhandras · 2025-04-10T12:22:09Z

+			// manages to be added during this time, it will be
+			// detected as missing when analyzing the spend
+			// notification and will be added to new batch.
+			batch.state = Open


But this also means that any new sweeps could be added, and then we just would try to publish the batch again (and fail as inputs are spent). It's robust so not a huge issue, but perhaps we could avoid.

Avoiding this would require a DB migration, which doesn’t seem worthwhile just to improve this edge case.

starius · 2025-04-13T21:09:59Z

Looks good. Perhaps we don't even need the "closed" state?

I think it makes sense to keep Closed to make this thing easier to understand / debug.

Also maybe greedy batch selection can work worse if we let it add to Closed.

starius · 2025-04-26T04:36:26Z

Converting to draft. I'm working to add more commits here and to rebase it on top of #891

bhandras

Really nice set of fixes! Making sweep-batcher more robust one step at a time 🚀
LGTM 🎉

bhandras · 2025-05-09T17:42:23Z

-	// new sweeps.
+	// new sweeps. NOTE: this state exists only in-memory. In the database
+	// it is stored as Open and converted to Closed after a spend
+	// notification arrives (quickly after start of Batch.Run).


bhandras · 2025-05-09T17:43:16Z

+			// manages to be added during this time, it will be
+			// detected as missing when analyzing the spend
+			// notification and will be added to new batch.
+			batch.state = Open


bhandras · 2025-05-09T18:40:33Z

+
+		spendDetail := SpendDetail{
+			Tx: spendTx,
+			OnChainFeePortion: getFeePortionPaidBySweep(


Alternatively we could communicate the fee portion once the sweep is fully-confirmed. wdyt?

I added OnChainFeePortion to type ConfDetail, which is send through ConfChan.

I did it in a separate commit to make it clear what changed. I plan to squash it with the previous commit before merging.

lightninglabs-deploy · 2025-06-06T20:35:26Z

@hieblmi: review reminder
@sputn1ck: review reminder

Forgot to return the calculated total value.

Support sending errors to error channels returned by RegisterSpendNtfn and RegisterConfirmationsNtfn.

Function monitorSpendAndNotify used to cancel the context passed to RegisterSpendNtfn right after starting the goroutine processing results. Spend notifications were missed. Now the context is canceled when the goroutine finishes.

starius · 2025-06-11T15:59:36Z

I removed commits related to presigned mode from this PR. I'll open another PR.

hieblmi

LGTM, great set of fixes!

hieblmi · 2025-06-12T12:49:59Z

 	// Then we get the total amount that was swept by the batch.
 	totalSwept, err := b.store.TotalSweptAmount(ctx, parentBatchID)
 	if err != nil {
+		cancel()


+1, great catch

hieblmi · 2025-06-12T12:51:12Z

 			primarySweep.outpoint.String())

-		for {
+		select {


please fix typo in commit message.

hieblmi · 2025-06-12T12:53:06Z

 	b.state = Closed

-	return b.persist(ctx)
+	if err := b.persist(ctx); err != nil {


nit: can do err = b.persist...

Fixed. Thanks!

hieblmi · 2025-06-12T12:53:47Z

 	b.state = Closed

-	return b.persist(ctx)
+	if err := b.persist(ctx); err != nil {


another great catch!

hieblmi · 2025-06-12T13:51:53Z

+			// continue.
+			case <-notifier.QuitChan:
+			}
+		}(s.notifier)


The loop always had exactly one iteration.

If monitorConfirmations fails, we still want to persist the state to DB.

Previously, dbBatch had a State field (enum: Open, Closed, Confirmed), but in the database it is represented as a boolean Confirmed. The Closed state was stored the same way as Open. This wasn't an issue in practice, since an Open batch is quickly transitioned to Closed after startup. However, the in-memory mock stores plain dbBatch instances, leading to inconsistent behavior between the mock and the real DB-backed store. This commit updates dbBatch to match the database representation by replacing the State field with a Confirmed boolean.

Add fields ConfChan and ConfErrChan to SpendNotifier type which is a part of SweepRequest passed to AddSweep method. This is needed to reuse confirmation notifications on the calling side the same way it is done for spending notifications.

sputn1ck

LGTM!

sputn1ck · 2025-06-12T14:50:38Z

-			case <-ctx.Done():
-				return
-			}
+		case <-ctx.Done():


prior we returned here and in all other ctx.done cases, is this intended? We just fall through to the last return right?

Prior we returned in all the cases of the select, so the for loop always used to have exactly one iteration. That is why the for loop is not needed and return statements can be removed, because this is the end of the function anyway.

starius force-pushed the conf-chan branch from 391bef5 to 270ecbc Compare April 9, 2025 03:19

starius marked this pull request as ready for review April 9, 2025 04:06

starius requested review from bhandras, hieblmi and sputn1ck April 9, 2025 04:15

sputn1ck reviewed Apr 10, 2025

View reviewed changes

bhandras reviewed Apr 10, 2025

View reviewed changes

levmi assigned starius Apr 10, 2025

starius force-pushed the conf-chan branch from 270ecbc to 7a7e996 Compare April 13, 2025 20:56

starius requested review from bhandras and sputn1ck April 13, 2025 21:15

levmi removed the request for review from hieblmi April 15, 2025 15:29

starius marked this pull request as draft April 26, 2025 04:35

starius changed the title ~~sweepbatcher: notify caller about confirmations~~ [WIP] sweepbatcher: notify caller about confirmations Apr 26, 2025

starius force-pushed the conf-chan branch 2 times, most recently from a67a666 to e234022 Compare April 29, 2025 03:52

starius changed the title ~~[WIP] sweepbatcher: notify caller about confirmations~~ sweepbatcher: notify caller about confirmations Apr 29, 2025

starius marked this pull request as ready for review April 29, 2025 03:53

starius force-pushed the conf-chan branch 4 times, most recently from 494ebfb to c292ec2 Compare May 5, 2025 20:46

levmi requested review from hieblmi and removed request for sputn1ck May 6, 2025 14:22

starius force-pushed the conf-chan branch from 0190178 to ef315c3 Compare May 8, 2025 20:44

starius requested a review from sputn1ck May 8, 2025 21:29

bhandras approved these changes May 9, 2025

View reviewed changes

starius force-pushed the conf-chan branch from ef315c3 to b339904 Compare May 15, 2025 01:11

starius force-pushed the conf-chan branch from 0fafa55 to 381ab93 Compare May 15, 2025 03:40

starius added 8 commits June 11, 2025 10:34

sweepbatcher: mark channels receive- or send-only

6a32b46

sweepbatcher: unblock sending if notifier quits

501faa8

sweepbatcher: send spend error to notifier

39f859a

test: fix error messages

7636ffd

sweepbatcher: fix mock TotalSweptAmount

c036c2a

Forgot to return the calculated total value.

sweepbatcher: fix mistake in batch reading from DB

cb7c31c

test/chainnotifier_mock: support errors

2f05934

Support sending errors to error channels returned by RegisterSpendNtfn and RegisterConfirmationsNtfn.

sweepbatcher: cancel spendCtx after processing

01664ad

Function monitorSpendAndNotify used to cancel the context passed to RegisterSpendNtfn right after starting the goroutine processing results. Spend notifications were missed. Now the context is canceled when the goroutine finishes.

starius force-pushed the conf-chan branch from 381ab93 to 4198cbd Compare June 11, 2025 15:15

starius requested a review from bhandras June 11, 2025 15:58

hieblmi approved these changes Jun 12, 2025

View reviewed changes

starius force-pushed the conf-chan branch from 4198cbd to ea4c7fe Compare June 12, 2025 15:30

starius added 5 commits June 12, 2025 12:31

sweepbatcher: remove unneeded for loops

fa1bc48

The loop always had exactly one iteration.

sweepbatcher: store batch status before monitoring

7edbfea

If monitorConfirmations fails, we still want to persist the state to DB.

sweepbatcher: test spending notification and error

8466e0f

starius force-pushed the conf-chan branch from ea4c7fe to c70257a Compare June 12, 2025 15:32

sputn1ck approved these changes Jun 12, 2025

View reviewed changes

starius merged commit 59f67de into lightninglabs:master Jun 12, 2025
4 checks passed

starius deleted the conf-chan branch June 12, 2025 18:15

		@@ -213,8 +213,8 @@ type dbBatch struct {
		// ID is the unique identifier of the batch.

Uh oh!

Conversation

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

Uh oh!

Pull Request Checklist

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!

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!

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!

Uh oh!

starius commented Apr 13, 2025

Uh oh!

starius commented Apr 26, 2025

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lightninglabs-deploy commented Jun 6, 2025

Uh oh!

starius commented Jun 11, 2025

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!

Choose a reason for hiding this comment

Uh oh!

sputn1ck left a comment

Choose a reason for hiding this comment

Uh oh!

sputn1ck Jun 12, 2025 • 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!

starius commented Apr 9, 2025 •

edited

Loading

sputn1ck Jun 12, 2025 •

edited

Loading