sweepbatcher: add mixed batches as an option #791

starius · 2024-07-12T17:21:53Z

Option WithMixedBatch instructs sweepbatcher to create mixed batches with regard to cooperativeness. Such a batch can include both sweeps signed both cooperatively and non-cooperatively. If cooperative signing fails for a sweep, transaction is updated to sign that sweep non-cooperatively and another round of cooperative signing runs on the remaining sweeps. The remaining sweeps are signed in non-cooperative (more expensive) way. If the whole procedure fails for whatever reason, the batch is signed non-cooperatively (the fallback).

Updated greedy batch selection algorithm to take into account whether mixed batches are enabled.

Pull Request Checklist

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

hieblmi

I appreciate your effort towards more flexible batcher features. On my first pass I just have a question regarding the leafWitnessSize. Great catch btw 🥇

hieblmi · 2024-07-15T14:38:07Z

sweepbatcher/sweep_batch.go

@@ -962,21 +962,21 @@ func (b *batch) debugLogTx(msg string, tx *wire.MsgTx) {

 // coopSignBatchTx collects the necessary signatures from the server in order
 // to cooperatively sweep the funds.
-func (b *batch) coopSignBatchTx(ctx context.Context, packet *psbt.Packet,
+func (b *batch) coopSignBatchTx(ctx context.Context, tx *wire.MsgTx,


hieblmi · 2024-07-15T14:40:16Z

sweepbatcher/sweep_batch.go

-	var psbtBuf bytes.Buffer
-	err = packet.Serialize(&psbtBuf)
+	// Create PSBT and prevOuts.
+	psbtBytes, prevOuts, err := b.createPsbt(batchTx, sweeps)


hieblmi · 2024-07-15T14:52:18Z

swap/htlc.go

@@ -283,10 +281,20 @@ func (h *Htlc) AddSuccessToEstimator(estimator *input.TxWeightEstimator) error {
 			trHtlc.InternalPubKey, successLeaf, timeoutLeafHash[:],
 		)

-		estimator.AddTapscriptInput(maxSuccessWitnessSize, tapscript)
+		// The leaf witness size must be calculated without the byte


that's a very good catch fixing the fee overpayment 🥇

The godoc says only the total size of all elements on the stack that are consumed by the revealed script should be counted. but we also count sigLength and preimg_length which I am unsure if they are pushed to the stack? I tought just the sig and the preimage are pushed?

IIUC, sizes of elements also consume space in encoded transaction, even though they are not themselves pushed to stack. In sweepbatcher, I added comparison of real tx weight and the estimated weight, logging a warning in case of a discrepancy. The current value of leafWitnessSize (with elements' sizes) results in weight estimates equal to real tx size.

Oh that's a very nice catch!

starius · 2024-07-22T18:10:13Z

Converting to draft for now.

Changes are needed in greedy batch selection algorithm, to adjust to the fact that batches can be mixed now.

bhandras

Really nice work!! 🎉 Just a few comments, but overall looks very good already!

bhandras · 2024-07-23T14:26:31Z

swap/htlc.go

@@ -283,10 +281,20 @@ func (h *Htlc) AddSuccessToEstimator(estimator *input.TxWeightEstimator) error {
 			trHtlc.InternalPubKey, successLeaf, timeoutLeafHash[:],
 		)

-		estimator.AddTapscriptInput(maxSuccessWitnessSize, tapscript)
+		// The leaf witness size must be calculated without the byte


Oh that's a very nice catch!

bhandras · 2024-07-23T14:32:47Z

sweepbatcher/sweep_batch.go

 	if ok {
+		// Preserve coopFailed value not to forget about cooperative
+		// spending failure in this sweep.
+		sweepValue := *sweep


nit: i think naming this variable as tmp could be more readable.

bhandras · 2024-07-23T14:34:04Z

sweepbatcher/sweep_batch.go

+		if err != nil {
+			b.log.Warnf("Mixed batch publish error: %v", err)
+		} else {
+			coopSuccess = true


Would this fail if it fails to replace the tx in the mempool?

I changed publishMixedBatch interface: now it returns signSuccess bool result, as well as publishBatchCoop function. Also I renamed coopSuccess local variable here to signSuccess, since it indicates success of signing (and all preceding actions), which may be fully non-cooperative in case of publishMixedBatch.

bhandras · 2024-07-23T15:58:52Z

sweepbatcher/sweep_batch.go

+	txHash := tx.TxHash()
+	b.log.Infof("attempting to publish mixed tx=%v with feerate=%v, "+
+		"weight=%v, feeForWeight=%v, fee=%v, sweeps=%d (%d cooperative"+
+		" + %d non-cooperative), destAddr=%s",


nit: Would be nice to also log the swap hashes for additional debug info.

Done. Now log lines look like this:

attempting to publish mixed tx=xxx with feerate=10000 sat/kw, weight=559 wu, feeForWeight=0.00005590 BTC, fee=0.00005590 BTC, sweeps=1, 0 cooperative: () and 1 non-cooperative (01d0fabd251f), destAddr=xxx attempting to publish mixed tx=xxxx with feerate=10000 sat/kw, weight=3377 wu, feeForWeight=0.00033770 BTC, fee=0.00033770 BTC, sweeps=9, 2 cooperative: (938834d6c636, 34ec81dbdaf9) and 7 non-cooperative (01d0fabd251f, 5778f985db75, 91d3827f052f, 4b78063b9c22, f5411ec7e51e, aae761377f3b, 067dc6a81018), destAddr=xxx

bhandras · 2024-07-23T16:00:08Z

sweepbatcher/sweep_batch.go

+	err := b.wallet.PublishTransaction(
+		ctx, tx, labels.LoopOutBatchSweepSuccess(b.id),
+	)
+	if err != nil {


Ideally I think we'd want to filter out the case where replace fails (but the tx itself is valid) so we don't end up ditching the coops sweeps in favor of revealing the scripts etc.

Done. See the comment above, about "signSuccess".

~~I think this requires checking for publishing errors like output already spent I think.~~

Discussed offline.

starius · 2024-07-30T03:04:28Z

Updated greedy batch selection algorithm to take into account whether mixed batches are enabled!

lightninglabs-deploy · 2024-08-06T03:30:41Z

@bhandras: review reminder
@hieblmi: review reminder
@GeorgeTsagk: review reminder

bhandras

LGTM, awesome work! 🎉

starius · 2024-08-12T22:38:49Z

Rebased. Removed the commit already merged in #804

starius · 2024-08-13T03:39:25Z

NB: after #809 is merged, the custom transaction label generator should be used in mixed batch code as well.

starius · 2024-08-13T13:55:47Z

Rebased

starius · 2024-08-19T15:32:26Z

Pushed a fix for signing code. Real SignOutputRaw requires prevOuts for all inputs, not only the ones being signed. The commit will be squashed later.

hieblmi

Great work and nice test coverage to back it up, including the integration test.

hieblmi · 2024-08-20T09:56:24Z

sweepbatcher/sweep_batch.go

+	// mixedBatch instructs sweepbatcher to create mixed batches with regard
+	// to cooperativeness. Such a batch can include sweeps signed both
+	// cooperatively and non-cooperatively. If cooperative signing fails for
+	// a sweep, transaction is updated to sign that sweep non-cooperatively


really cool system 👌

Thanks! 😎

hieblmi · 2024-08-20T09:59:31Z

sweepbatcher/sweep_batch.go

+func (b *batch) constructUnsignedTx(sweeps []sweep,
+	address btcutil.Address) (*wire.MsgTx, lntypes.WeightUnit,
+	btcutil.Amount, btcutil.Amount, error) {
+


nit: could return early if sweeps is empty.

hieblmi · 2024-08-20T10:03:43Z

sweepbatcher/sweep_batch.go

+				false
+		}
+
+		// Keep track of if any new sweep failed to sign cooperatively.


nit: Keep track if any...

hieblmi · 2024-08-20T10:14:31Z

sweepbatcher/sweep_batch.go

+	}
+
+	// Publish the transaction.
+	err := b.wallet.PublishTransaction(


nit:

label := labels.LoopOutBatchSweepSuccess(b.id) err := b.wallet.PublishTransaction(ctx, tx, label) if err != nil { return 0, fmt.Errorf("publishing tx failed: %w", err), true }

It should be b.cfg.txLabeler(b.id), not labels.LoopOutBatchSweepSuccess(b.id), see #809

hieblmi · 2024-08-20T10:23:04Z

sweepbatcher/sweep_batch.go

+	err := b.wallet.PublishTransaction(
+		ctx, tx, labels.LoopOutBatchSweepSuccess(b.id),
+	)
+	if err != nil {


~~I think this requires checking for publishing errors like output already spent I think.~~

Discussed offline.

... instead of psbt.Packet. Make the code simpler. Function worked with packet.UnsignedTx only, so it is easier to pass tx directly.

Unload method publishBatchCoop. Also this code will be reused in new method for mixed batches soon.

Size of a signature affects the weight of transaction, which is verified in tests. Also method SignOutputRaw now returns the number of signatures matching the number of signature descriptors to prevent crashes in tests where multiple inputs are signed.

Option WithMixedBatch instructs sweepbatcher to create mixed batches with regard to cooperativeness. Such a batch can include both sweeps signed both cooperatively and non-cooperatively. If cooperative signing fails for a sweep, transaction is updated to sign that sweep non-cooperatively and another round of cooperative signing runs on the remaining sweeps. The remaining sweeps are signed in non-cooperative (more expensive) way. If the whole procedure fails for whatever reason, the batch is signed non-cooperatively (the fallback).

Treat coopFailed flag the same as nonCoopHint. The former is what we found in previos signing attempts, the later is what the caller signalled to us.

starius requested review from bhandras, hieblmi and GeorgeTsagk July 12, 2024 17:21

starius self-assigned this Jul 12, 2024

starius force-pushed the sweepbatcher-mixed-batches branch from cce93ad to 9b82990 Compare July 12, 2024 18:57

starius marked this pull request as ready for review July 13, 2024 02:25

hieblmi reviewed Jul 15, 2024

View reviewed changes

starius mentioned this pull request Jul 15, 2024

sweepbatcher: add greedy batch selection algorithm #787

Merged

1 task

starius force-pushed the sweepbatcher-mixed-batches branch from 9b82990 to 2364192 Compare July 18, 2024 17:48

starius marked this pull request as draft July 22, 2024 18:09

bhandras reviewed Jul 23, 2024

View reviewed changes

starius force-pushed the sweepbatcher-mixed-batches branch 2 times, most recently from 1be3a90 to 3d26a70 Compare July 30, 2024 02:47

starius marked this pull request as ready for review July 30, 2024 03:04

starius requested review from bhandras and hieblmi July 30, 2024 03:04

starius mentioned this pull request Aug 5, 2024

swap: fix leaf estimation in AddSuccessToEstimator #804

Merged

1 task

starius requested a review from sputn1ck August 6, 2024 15:47

bhandras approved these changes Aug 6, 2024

View reviewed changes

starius force-pushed the sweepbatcher-mixed-batches branch from 3d26a70 to 59911be Compare August 12, 2024 22:37

starius force-pushed the sweepbatcher-mixed-batches branch from 59911be to f21ec9a Compare August 13, 2024 13:55

hieblmi approved these changes Aug 20, 2024

View reviewed changes

starius force-pushed the sweepbatcher-mixed-batches branch from fcc5d41 to 2e27f4c Compare August 20, 2024 18:55

starius added 6 commits August 21, 2024 14:05

sweepbatcher: coopSignBatchTx accepts wire.MsgTx

5cd6c0c

... instead of psbt.Packet. Make the code simpler. Function worked with packet.UnsignedTx only, so it is easier to pass tx directly.

sweepbatcher: factor out method createPsbt

2c2c427

Unload method publishBatchCoop. Also this code will be reused in new method for mixed batches soon.

sweepbatcher: use coopFailed in greedy selection

d007cf6

Treat coopFailed flag the same as nonCoopHint. The former is what we found in previos signing attempts, the later is what the caller signalled to us.

sweepbatcher: make greedy algo mixed batch aware

b171f76

starius force-pushed the sweepbatcher-mixed-batches branch from 2e27f4c to b171f76 Compare August 21, 2024 17:06

starius merged commit a6e9a2b into lightninglabs:master Aug 21, 2024
4 checks passed

starius deleted the sweepbatcher-mixed-batches branch August 21, 2024 17:11

sweepbatcher: add mixed batches as an option #791

sweepbatcher: add mixed batches as an option #791

Conversation

starius commented Jul 12, 2024 • edited Loading

Pull Request Checklist

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

starius commented Jul 22, 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

hieblmi Aug 20, 2024 • edited Loading

Choose a reason for hiding this comment

starius commented Jul 30, 2024

lightninglabs-deploy commented Aug 6, 2024

bhandras left a comment

Choose a reason for hiding this comment

starius commented Aug 12, 2024

starius commented Aug 13, 2024

starius commented Aug 13, 2024

starius commented Aug 19, 2024

hieblmi left a comment • 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

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 Aug 20, 2024 • edited Loading

Choose a reason for hiding this comment

starius commented Jul 12, 2024 •

edited

Loading

hieblmi Aug 20, 2024 •

edited

Loading

hieblmi left a comment •

edited

Loading

hieblmi Aug 20, 2024 •

edited

Loading