sweepbatcher: add mode with presigned transactions #891

starius · 2025-02-27T22:22:57Z

In this mode sweepbatcher uses transactions provided by the Presigned helper.
Transactions are signed upon adding an input to a batch.

A single Batcher instance can handle both presigned and regular batches.
Currently presigned and non-presigned sweeps never appear in the same batch.

Pull Request Checklist

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

hieblmi

Amazing work @starius. I've left a couple of questions in my first pass.

hieblmi · 2025-03-04T12:54:48Z

sweepbatcher/sweep_batch.go

+		for _, s := range b.sweeps {
+			if !s.presigned {
+				b.log().Infof("failed to add sweep %x to the "+
+					"batch, because the batch has "+


nit: maybe clarify "failed to add presigned sweep %x..."

hieblmi · 2025-03-04T12:55:10Z

sweepbatcher/sweep_batch.go

+		// Ensure that all the sweeps in the batch don't use presigned.
+		for _, s := range b.sweeps {
+			if s.presigned {
+				b.log().Infof("failed to add sweep %x to the "+


nit: maybe clarify "failed to add non-presigned sweep %x..."

hieblmi · 2025-03-04T12:58:04Z

sweepbatcher/sweep_batch.go

+
+	for _, sweep := range b.sweeps {
+		if sweep.presigned {
+			return true, nil


I think if all sweeps are presigned we return true, false otherwise. So we should return false here if !sweep.presigned, otherwise a presigned batch could still have non-presigned sweeps.

I rewrote the function to ensure that all the sweeps have the same presigned status. If there is a mix, the function returns an error. This would be a bug.

hieblmi · 2025-03-04T13:04:26Z

sweepbatcher/sweep_batcher.go

@@ -564,8 +628,40 @@ func (b *Batcher) Run(ctx context.Context) error {
 	}
 }

+// PresignSweep creates and stores presigned 1:1 transactions for the sweep.
+// This method must be called prior to AddSweep if presigned mode is enabled.


I see this is ensured in the batch by b.ensurePresigned

I updated ensurePresigned and SignTx interface (added argument presignedOnly), so ensurePresigned actually ensured that it is presigned, not tried to sign (become read-only). Also added a test making sure that the batcher crashes if AddSweep is called before calling PresignSweep (for a presigned sweep). (AddSweep can't return that error, since it only pushes a sweep to the event loop.)

hieblmi · 2025-03-04T13:04:46Z

sweepbatcher/sweep_batcher.go

+	}
+
+	// Find the feerate needed to get into next block. Use conf_target=2,
+	nextBlockFeerate, err := b.wallet.EstimateFeeRate(ctx, 2)


nit: camel case nextBlockFeeRate

hieblmi · 2025-03-05T12:06:41Z

sweepbatcher/sweep_batch.go

+				return false, nil
+			}
+		} else {
+			if err := b.ensurePresigned(ctx, sweep); err != nil {


if err == nil here, don't we then have to call b.presign for the sweep here?

If ensurePresigned returned no error, this means, it has been presigned already. No need to presign again. If not presigned, then crash the batcher (this is only possible because of a bug).

hieblmi · 2025-03-05T12:08:20Z

sweepbatcher/sweep_batch.go

+	// use the same mode (presigned or regular).
+	if sweep.presigned {
+		// Ensure that all the sweeps in the batch use presigned mode.
+		for _, s := range b.sweeps {


instead of iterating, do you think it makes sense to add a presigned flag to the batch struct that we can check before adding a sweep to a batch?

I plan to allow half-presigned batches in the future to use a regular sweep an an anchor and such a flag would be removed. So I decided to keep this calculated not to forget to remove it in the future :)

hieblmi · 2025-03-05T12:17:15Z

sweepbatcher/presigned.go

+		batchAmt += sweep.value
+	}
+
+	// Find when at least one sweep expires.


nit: "find the sweep with the earliest expiry"

hieblmi · 2025-03-05T12:38:45Z

sweepbatcher/presigned.go

+	}
+
+	// Find the feerate needed to get into next block. Use conf_target=2,
+	nextBlockFeerate, err := b.wallet.EstimateFeeRate(ctx, 2)


why is it 2? Maybe create a constant?

I updated this place:

// priorityConfTarget defines the confirmation target for quick // inclusion in a block. A value of 2, rather than 1, is used to prevent // overestimation caused by temporary anomalies, such as periods without // blocks. const priorityConfTarget = 2 // Find the feerate needed to get into next block. nextBlockFeeRate, err := b.wallet.EstimateFeeRate( ctx, priorityConfTarget, )

This fee rate is only used to define a high fee rate (10x of this value and at least 100 sats/vbyte).

BTW the fee estimator fails with conf_target=1: #898
Maybe it worth making this a global constant in Loop and use it in all such places.

Updated the comment:

// priorityConfTarget defines the confirmation target for quick // inclusion in a block. A value of 2, rather than 1, is used to prevent // fee estimator from failing. // See https://github.com/lightninglabs/loop/issues/898 const priorityConfTarget = 2

lightninglabs-deploy · 2025-04-08T19:48:06Z

@hieblmi: review reminder

hieblmi

It's great work @starius, I've left a couple of more comments, looks almost good to me.

hieblmi · 2025-04-10T17:30:31Z

sweepbatcher/sweep_batcher.go

+
+	// DestPkScript returns destination pkScript used in a presigned
+	// transaction sweeping the inputs. Returns an error, if such tx
+	// doesn't exist. If there are many such transactions, returns any


its not possible that if there are many such transactions, that they'd have different pkScripts, I assume.

Yes, all of the transactions should have the same destination address, by construction. However this is not enforced by PresignedHelper. In theory someone can call Presign on transactions with the same list of inputs and different destination addresses and all of the addresses would be considered valid. We can demand PresignedHelper to check that address is always the same, but this would make queries heavier. (Checking in Presign that destination address is the same in all presigned transactions with such inputs.)

hieblmi · 2025-04-10T17:54:42Z

sweepbatcher/sweep_batch.go

+				return false, nil
+			}
+
+		// If this is new batch being formed, make sure we already have


nit: if this is a

hieblmi · 2025-04-10T18:00:10Z

sweepbatcher/sweep_batch.go

@@ -1960,7 +2116,21 @@ func (b *batch) persist(ctx context.Context) error {

 // getBatchDestAddr returns the batch's destination address. If the batch
 // has already generated an address then the same one will be returned.
+// The method must not be used in presigned mode. Use getSweepsDestAddr instead.


should we call this method in the comment getPresignedSweepsDestAddr to clarify?

Renamed. Thanks!

hieblmi · 2025-04-10T18:01:17Z

sweepbatcher/presigned.go

+
+// getSweepsDestAddr returns the destination address used by a group of sweeps.
+// The method must be used in presigned mode only.
+func (b *batch) getSweepsDestAddr(ctx context.Context,


could call it getPresignedSweepsDestAddr to make it clearer that this only makes sense in the context of presigned batches.

hieblmi · 2025-04-10T18:05:01Z

sweepbatcher/presigned.go

+		return fmt.Errorf("presignedHelper is not installed")
+	}
+	if len(b.sweeps) != 0 {
+		return fmt.Errorf("ensurePresigned is done when adding to an " +


nit: shoud be done...

hieblmi · 2025-04-10T18:23:06Z

sweepbatcher/presigned.go

+	for _, sweep := range newSweeps {
+		batchAmt += sweep.value
+	}
+	const presignedOnly = true


maybe we could call this flag loadOnly or something similar?

Renamed to loadOnly.

hieblmi · 2025-04-10T18:24:50Z

sweepbatcher/presigned.go

+)
+
+// ensurePresigned checks that we can sign a 1:1 transaction sweeping the input.
+func (b *batch) ensurePresigned(ctx context.Context, newSweeps []*sweep) error {


does ensurePresigned imply that there should be already be a transaction with given inputs that has a populated witness? I don't see a check anywhere in this method or sub-method that checks for a witness.

I added a check to CheckSignedTx which is called by this method:

// Make sure all inputs of signedTx have a non-empty witness. for _, txIn := range signedTx.TxIn { if len(txIn.Witness) == 0 { return fmt.Errorf("input %s of signed tx is not signed", txIn.PreviousOutPoint) } }

hieblmi · 2025-04-10T18:27:48Z

sweepbatcher/presigned.go

+		}
+
+		// Try to presign this transaction.
+		err = presigner.Presign(ctx, tx, batchAmt)


Just a side question: Currently the result is not yet used. Is it intended to then pass this pre-signed transaction into our store? I guess once the batch is confirmed we could clean up all the presigned fee versions.

Also, would Presigner handle persisting the txns? Otherwise wouldn't that require another client change?

PresignHelper is responsible for storing the transactions. SweepBatcher relies on helper.SignTx to be able to load transactions stored by previous helper.Presign and helper.SignTx calls.

I guess once the batch is confirmed we could clean up all the presigned fee versions.

This is done. There is the method CleanupTransactions which is called by batch.handleConf method.

hieblmi · 2025-04-10T18:29:07Z

sweepbatcher/presigned.go

+		ctx, tx, batchAmt, minRelayFee, feeRate, presignedOnly,
+	)
+	if err != nil {
+		return 0, fmt.Errorf("failed to sign tx: %w", err),


is it trying to sign when presignedOnly is passed in?

presignedOnly = false in this call, so it tries to sign interactively and if it fails, goes to the store of presigned transactions.

hieblmi · 2025-04-10T18:32:38Z

sweepbatcher/presigned.go

+}
+
+// CheckSignedTx makes sure that signedTx matches the unsignedTx. It checks
+// according to criteria specified in the description of PresignedHelper.SignTx.


looks like the input sequence numbers aren't checked as described in the godoc of SignTx.

Sequence numbers are compared using unsignedMap. Also there is a test case to verify this.

starius · 2025-04-16T18:00:36Z

Found a couple of edge cases. Converting to a draft until the fix is ready.

hieblmi

Left some comments and questions for the latest set of changes.

sweepbatcher/sweep_batcher.go

sweepbatcher/sweep_batch.go

sweepbatcher/presigned.go

starius · 2025-05-01T04:24:06Z

@hieblmi Thanks for comments! 🎉
I fixed them.

Also found a small edge case for function constructUnsignedTx. Fixed it in commit sweepbatcher: fix rounding in constructUnsignedTx.

starius · 2025-05-02T01:51:54Z

Updated the last commit sweepbatcher: add mode with presigned transactions. Skipped ensurePresigned check in AddSweep if the sweep is fully confirmed (the presigned transaction is cleaned up by this time).

hieblmi

Really amazing work on this @starius. LGTM!

starius · 2025-05-05T16:18:22Z

@hieblmi I expanded the purging test to verify that if the sweeps are online at the time of purging (i.e. confirmed tx doesn't have them and they have to be re-added to the batcher) they can be successfully added to an existing batch.

I did it in a temp commit to make it easier to re-review the change.

starius · 2025-05-05T20:18:39Z

@hieblmi One more change. I replaces helper.IsPresigned() method with a field in SweepInfo. This makes things easier on naut side. I pushed it in a separate temp commit.

bhandras

Looks really good! My only complaint is the possible timeout overflow, otherwise LGTM!

sweepbatcher/sweep_batch_test.go

sweepbatcher/sweep_batch.go

sweepbatcher/sweep_batcher.go

sweepbatcher/sweep_batch.go

bhandras · 2025-05-08T07:27:54Z

sweepbatcher/presigned.go

+	)
+
+	// Calculate the locktime value to use for high feerate transactions.
+	highFeeRateLockTime := uint32(timeout - timeoutThreshold)


This can overflow if timeout is less than 50.

I changed this to fix this edge case. Thanks! Added a test case.

// Calculate the locktime value to use for high feerate transactions. // If timeout <= timeoutThreshold, don't set LockTime (keep value 0). var highFeeRateLockTime uint32 if timeout > timeoutThreshold { highFeeRateLockTime = uint32(timeout - timeoutThreshold) }

sweepbatcher/presigned.go

sweepbatcher/sweep_batch.go

sweepbatcher/sweep_batcher_presigned_test.go

starius · 2025-05-08T17:29:09Z

@bhandras I pushed two commits to squash them later. They have the fixes for your comments.
Please take another look!

bhandras

Really nice additions @starius 🥇 LGTM! 🎉

Also added a unit test for it.

Previously it may round one satoshi towards zero resulting in fee-rate slightly lower than requested.

This is needed to run additional checks in AddSweep in the next commit.

In this mode sweepbatcher uses transactions provided by the Presigned helper. Transactions are signed upon adding an input to a batch. A single Batcher instance can handle both presigned and regular batches. Currently presigned and non-presigned sweeps never appear in the same batch.

starius requested review from bhandras, sputn1ck and hieblmi February 27, 2025 22:22

starius self-assigned this Feb 28, 2025

starius force-pushed the presigned branch from ecf6a73 to b93c255 Compare February 28, 2025 03:43

starius marked this pull request as ready for review February 28, 2025 03:55

starius force-pushed the presigned branch 2 times, most recently from 3cbc2e6 to 418bd2d Compare March 4, 2025 17:18

hieblmi reviewed Mar 5, 2025

View reviewed changes

starius requested a review from hieblmi March 6, 2025 14:15

starius force-pushed the presigned branch 4 times, most recently from 613903a to b115f41 Compare March 18, 2025 18:19

starius force-pushed the presigned branch from b115f41 to 8331c96 Compare March 24, 2025 17:11

starius force-pushed the presigned branch 6 times, most recently from 6abbc68 to 0006a7e Compare April 6, 2025 03:38

starius force-pushed the presigned branch from 0006a7e to 9f97c88 Compare April 9, 2025 03:23

starius mentioned this pull request Apr 10, 2025

sweepbatcher: add mode with presigned transactions and CPFP #888

Closed

1 task

hieblmi reviewed Apr 10, 2025

View reviewed changes

starius force-pushed the presigned branch from 9f97c88 to f5195c4 Compare April 15, 2025 04:40

starius requested a review from hieblmi April 15, 2025 14:41

levmi removed the request for review from sputn1ck April 15, 2025 15:26

starius marked this pull request as draft April 16, 2025 18:00

starius force-pushed the presigned branch 2 times, most recently from 55dfa9f to 5c68867 Compare April 29, 2025 17:12

hieblmi reviewed Apr 30, 2025

View reviewed changes

starius force-pushed the presigned branch from 5c68867 to 5645c24 Compare May 1, 2025 04:23

starius requested a review from hieblmi May 1, 2025 04:24

starius force-pushed the presigned branch 2 times, most recently from 930d193 to 065ff70 Compare May 2, 2025 01:50

starius force-pushed the presigned branch from 065ff70 to 1bd22e1 Compare May 2, 2025 04:36

hieblmi approved these changes May 2, 2025

View reviewed changes

bhandras reviewed May 8, 2025

View reviewed changes

bhandras self-requested a review May 8, 2025 20:07

bhandras approved these changes May 8, 2025

View reviewed changes

starius added 10 commits May 8, 2025 17:24

sweepbatcher: make func constructUnsignedTx pure

1983cc2

Also added a unit test for it.

sweepbatcher: fix rounding in constructUnsignedTx

149bffc

Previously it may round one satoshi towards zero resulting in fee-rate slightly lower than requested.

test: implement MinRelayFee RPC

45aa156

test: implement SignPsbt RPC

cd599b0

test: allow intercepting PublishTransaction

09d28b5

sweepbatcher: auto-increment sweep.ID in StoreMock

b7d3bc4

sweepbatcher: rm unused field batchKit.returnChan

78ca6bd

sweepbatcher: load sweeps in AddSweep

bb837a4

This is needed to run additional checks in AddSweep in the next commit.

sweepbatcher: use WithoutCancel for notifications

a4a2bfb

starius force-pushed the presigned branch from 04ddb7b to b9b256f Compare May 8, 2025 20:25

starius merged commit 125d83f into lightninglabs:master May 8, 2025
4 checks passed

starius deleted the presigned branch May 8, 2025 20:44

sweepbatcher: add mode with presigned transactions #891

sweepbatcher: add mode with presigned transactions #891

Conversation

starius commented Feb 27, 2025 • 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

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

lightninglabs-deploy commented Apr 8, 2025

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

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 Apr 16, 2025

hieblmi left a comment

Choose a reason for hiding this comment

starius commented May 1, 2025

starius commented May 2, 2025

hieblmi left a comment

Choose a reason for hiding this comment

starius commented May 5, 2025

starius commented May 5, 2025

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

starius commented May 8, 2025

bhandras left a comment

Choose a reason for hiding this comment

starius commented Feb 27, 2025 •

edited

Loading