Loop Out Sweep Batcher #634

GeorgeTsagk · 2023-08-26T00:07:23Z

Description

This PR adds a batcher for loop out htlc sweeps.

ToDo

Tx crafting & rbf strategy
Re-org safety

Also closes #675, this change was required due to the increased demands from our mocking interfaces with the introduction of the sweepbatcher

sputn1ck

Did a first pass. As far as I can understand it it seems like an elegant approach.

However I'm not sure if the batcher is restart safe. E.g. what would happen if we have an inflight batch and restart loop. If no RBF of 'new' (but already broadcasted) sweeps would be braodcasted, would the confirmation logic be able to handle that?

batcher/sweep_batch.go

hieblmi

Really great work @GeorgeTsagk, your code comments help reviewing this PR. I will go over the functionality of this PR again more throughly - so far I got nits and questions for you :D

batcher/sweep_batcher.go

batcher/sweep_batch.go

batcher/sweep_batcher.go

swap_server_client.go

loopout.go

hieblmi

Added some more comments but will hold back with further review until design Qs are ironed out.

loopdb/sqlc/migrations/000003_sweep_batcher.up.sql

loopdb/sqlc/migrations/000003_sweep_batcher.down.sql

batcher/sweep_batcher.go

batcher/sweep_batch.go

GeorgeTsagk · 2023-10-10T15:51:59Z

There was a slight overhaul on the persistence aspect of batches. As suggested by @bhandras & @sputn1ck we now store all the sweeps that belong to a batch and do not wait for the swap to be resumed in order for their sweeps to be batched again.

bhandras

Partial review, more is coming.

loopdb/batch.go

bhandras · 2023-10-12T11:50:26Z

loopdb/batch.go

+	ID int32
+
+	// BatchID is the ID of the batch that the sweep belongs to.
+	BatchID lntypes.Hash


Why not store a slice of sweeps in the Batch object instead?

Still applies.

Since we're storing sweeps in their own table it seems simpler to me to separate them in the loopdb models.
In the application level (sweepbatcher pkg) sweeps are indeed stored within a batch

Reflecting the database scheme on the application level will add complexity to the application code. I advise not doing so.

In the application level (sweepbatcher pkg) sweeps are indeed stored within a batch

I think this aligns with what you just said.

I am only keeping batches/sweeps separate in the loopdb level (I consider loopdb to not be application code..?)

loopdb/interface.go

loopdb/batch.go

batcher/sweep_batcher.go

batcher/sweep_batch.go

bhandras · 2023-10-12T18:35:39Z

batcher/sweep_batch.go

+		Value:    int64(batchAmt - fee),
+	})
+
+	packet, err := psbt.NewFromUnsignedTx(batchTx)


I think we'd also need to sort the inputs and outputs to conform to BIP-69. AFAICT psbt lib doesn't do that.

txsort.InPlaceSort(batchTx)

This is still an issue.

Was this discussed offline? Marked as resolved but can't find the sorting

batcher/sweep_batch.go

GeorgeTsagk · 2023-10-12T18:51:23Z

Ty for the review @bhandras

Just pushed some changes, mostly affecting test files

bhandras

Getting close, great work on this last push!

sweepbatcher/sweep_batch.go

sweepbatcher/sweep_batcher.go

bhandras · 2024-01-18T19:58:18Z

executor.go

@@ -121,6 +125,20 @@ func (s *executor) run(mainCtx context.Context,
 		return mainCtx.Err()
 	}

+	batcherErrChan = make(chan error, 1)
+	batcherCtx, cancel := context.WithCancel(mainCtx)


So what I meant is that it's fine to just call s.batcher.Run(mainCtx).

loopout.go

bhandras

Looks ripe! Nice work @GeorgeTsagk 🥇
A few last nits from my side.

sweepbatcher/sweep_batch.go

sweepbatcher/sweep_batcher.go

loopout_test.go

bhandras

LGTM, great work @GeorgeTsagk ⚡ ⚡

bhandras · 2024-01-23T13:13:06Z

sweepbatcher/sweep_batcher.go

+			case <-ctx.Done():
+			}
+
+			_ = b.writeToErrChan(ctx, err)


q: Do we need to add this error to the batcher's error channel? It'll stop the batcher altogether.

Hmm, not sure. If we don't it will only fail on the batch/swap level. One reason to error out the whole daemon is that a restart may fix the monitor spend issue. But that could also "deadlock" the whole daemon just for the sake of 1 swap

sputn1ck

LGTM! Congrats on the big PR.

I have a small set of nits that would be nice to haves.

loopdb/sql_store.go

sweepbatcher/sweep_batch.go

sputn1ck · 2024-01-23T13:44:55Z

sweepbatcher/sweep_batch.go

+
+	// If the provided sweep is nil, we can't proceed with any checks, so
+	// we just return early.
+	if sweep == nil {


why could a sweep be nil?

idk, always nice to check?

sputn1ck · 2024-01-23T14:12:00Z

sweepbatcher/sweep_batch.go

+		// To be sure that we're good, parse and validate that the
+		// combined signature is indeed valid for the sig hash and the
+		// internal pubkey.
+		err = b.verifySchnorrSig(


nit: combine sigs already checks if the sig is valid. removing the b.verifySchnorrSig part removes some code in this PR so i deem it worthy to remove.

See https://github.com/btcsuite/btcd/blob/master/btcec/schnorr/musig2/context.go#L650, which is called from https://github.com/lightningnetwork/lnd/blob/7d03b43ddfa11674a5002b4ecb5516ccc99f5303/input/musig2.go#L510

sputn1ck · 2024-01-23T14:12:36Z

client.go

@@ -162,27 +164,36 @@ func NewClient(dbDir string, loopDB loopdb.SwapStore,
 		Lnd: cfg.Lnd,
 	}

+	verifySchnorrSig := func(pubKey *btcec.PublicKey, hash, sig []byte) error {


this could be removed, as we verify the sig when combining.

alexbosworth · 2024-01-23T15:29:16Z

does this need a release notes comment?

alexbosworth · 2024-01-23T16:16:06Z

any readme notes needed on config settings?

GeorgeTsagk self-assigned this Aug 26, 2023

bhandras self-requested a review August 29, 2023 13:43

GeorgeTsagk force-pushed the sweep-batcher branch from 9b82ff8 to 6429806 Compare September 1, 2023 18:01

sputn1ck reviewed Sep 4, 2023

View reviewed changes

batcher/sweep_batch.go Outdated Show resolved Hide resolved

batcher/sweep_batch.go Outdated Show resolved Hide resolved

batcher/sweep_batch.go Outdated Show resolved Hide resolved

batcher/sweep_batch.go Outdated Show resolved Hide resolved

GeorgeTsagk force-pushed the sweep-batcher branch from 6429806 to be05283 Compare September 5, 2023 17:55

GeorgeTsagk force-pushed the sweep-batcher branch 4 times, most recently from ac3d79e to 2f3bce1 Compare September 15, 2023 18:52

GeorgeTsagk marked this pull request as ready for review September 15, 2023 18:53

GeorgeTsagk force-pushed the sweep-batcher branch 5 times, most recently from 95938d0 to fc13d50 Compare September 22, 2023 09:42

GeorgeTsagk requested a review from sputn1ck September 22, 2023 09:49

GeorgeTsagk force-pushed the sweep-batcher branch 2 times, most recently from f5fc60f to 3adf5aa Compare September 25, 2023 16:06

hieblmi reviewed Sep 26, 2023

View reviewed changes

GeorgeTsagk force-pushed the sweep-batcher branch from 3adf5aa to 844f27e Compare September 27, 2023 20:30

GeorgeTsagk requested a review from hieblmi September 27, 2023 20:31

hieblmi reviewed Sep 29, 2023

View reviewed changes

GeorgeTsagk force-pushed the sweep-batcher branch from 844f27e to d82674a Compare October 10, 2023 15:49

GeorgeTsagk requested a review from hieblmi October 10, 2023 15:52

bhandras reviewed Oct 12, 2023

View reviewed changes

GeorgeTsagk force-pushed the sweep-batcher branch from d82674a to e50afef Compare October 12, 2023 18:50

GeorgeTsagk force-pushed the sweep-batcher branch 2 times, most recently from 1a71384 to b910c03 Compare October 30, 2023 11:29

GeorgeTsagk requested a review from sputn1ck January 18, 2024 14:36

GeorgeTsagk force-pushed the sweep-batcher branch 2 times, most recently from 85580fb to 53129d2 Compare January 18, 2024 17:23

bhandras reviewed Jan 18, 2024

View reviewed changes

GeorgeTsagk force-pushed the sweep-batcher branch 3 times, most recently from 0fc4e68 to fec3c5c Compare January 19, 2024 16:08

GeorgeTsagk requested a review from bhandras January 19, 2024 16:11

bhandras reviewed Jan 20, 2024

View reviewed changes

GeorgeTsagk force-pushed the sweep-batcher branch from fec3c5c to aa4427b Compare January 23, 2024 09:39

GeorgeTsagk requested a review from bhandras January 23, 2024 09:46

bhandras approved these changes Jan 23, 2024

View reviewed changes

sputn1ck approved these changes Jan 23, 2024

View reviewed changes

GeorgeTsagk added 12 commits January 23, 2024 20:38

loopdb+sweepbatcher: add sweepbatcher store

99608ad

multi: move StoreMock to loopdb

e0d8595

swapserverrpc: add prevouts to MuSig2SignSweep req

23d308c

looprpc: add is_external_addr to loopout req

627eb5c

multi: use isExternalAddr flag

a9be69b

utils: add htlc & swap related helpers

b43fa11

loop+test: enhance epoch subscription for multiple subscribers

26e239c

sweepbatcher: add sweep batch

56784ab

sweepbatcher: add sweep batcher

7081fb7

sweepbatcher: add batcher tests

849d26b

loop: integrate sweepbatcher to loopout flow

0914074

loop: fix loopout and client tests for sweeper integration

33fdde9

GeorgeTsagk force-pushed the sweep-batcher branch from aa4427b to 33fdde9 Compare January 23, 2024 18:38

loopd: update release notes

6f75a11

GeorgeTsagk merged commit df2db80 into lightninglabs:master Jan 23, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Loop Out Sweep Batcher #634

Loop Out Sweep Batcher #634

GeorgeTsagk commented Aug 26, 2023 •

edited

Loading

sputn1ck left a comment

hieblmi left a comment •

edited

Loading

hieblmi left a comment

GeorgeTsagk commented Oct 10, 2023

bhandras left a comment

bhandras Oct 12, 2023

bhandras Oct 31, 2023

GeorgeTsagk Nov 13, 2023 •

edited

Loading

bhandras Nov 13, 2023

GeorgeTsagk Nov 14, 2023 •

edited

Loading

bhandras Oct 12, 2023

bhandras Oct 31, 2023

sputn1ck Jan 18, 2024

GeorgeTsagk commented Oct 12, 2023

bhandras left a comment

bhandras Jan 18, 2024

bhandras left a comment

bhandras left a comment

bhandras Jan 23, 2024

GeorgeTsagk Jan 23, 2024

sputn1ck left a comment

sputn1ck Jan 23, 2024

GeorgeTsagk Jan 23, 2024

sputn1ck Jan 23, 2024

sputn1ck Jan 23, 2024

alexbosworth commented Jan 23, 2024

alexbosworth commented Jan 23, 2024

Loop Out Sweep Batcher #634

Loop Out Sweep Batcher #634

Conversation

GeorgeTsagk commented Aug 26, 2023 • edited Loading

Description

ToDo

sputn1ck left a comment

Choose a reason for hiding this comment

hieblmi left a comment • edited Loading

Choose a reason for hiding this comment

hieblmi left a comment

Choose a reason for hiding this comment

GeorgeTsagk commented Oct 10, 2023

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

GeorgeTsagk Nov 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GeorgeTsagk Nov 14, 2023 • 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 commented Oct 12, 2023

bhandras left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bhandras left a comment

Choose a reason for hiding this comment

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

sputn1ck 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

alexbosworth commented Jan 23, 2024

alexbosworth commented Jan 23, 2024

GeorgeTsagk commented Aug 26, 2023 •

edited

Loading

hieblmi left a comment •

edited

Loading

GeorgeTsagk Nov 13, 2023 •

edited

Loading

GeorgeTsagk Nov 14, 2023 •

edited

Loading