sweepbatcher: batch change outputs

[hieblmi]

The batcher receives the ability to sign and publish batches with change outputs.

Change is created when the clients choose fractional swap amounts from the input values to be swapped. Usually the change is sent back to client's addresses.

If change is provided to ensurePresigned, presign and publishPresigned, it is expected to be placed within the passed []sweep.
Each sweep optionally holds a pointer to a change output. Only the first sweep in a sweep group called primary deposit will hold a pointer to the respective swap's change output.

• unit test coverage for change outputs

[hieblmi]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces the ability to handle change outputs in sweep batches, which is a significant feature. I've found a critical bug in fee calculation within CheckSignedTx that needs to be addressed. I've also pointed out a minor code simplification and a gap in test coverage for the new logic.

[gemini-code-assist]

Code Review

The pull request introduces functionality to handle change outputs in sweep batcher, specifically for presigned transactions. The changes involve modifying the sweep struct, updating transaction construction to include change outputs, and adding a function to retrieve and de-duplicate change outputs. The code also includes unit tests for the new functionality. The changes look good overall, but there are a few areas that could be improved for robustness and clarity.

[starius]

Multiple sweeps holding the same change reference will produce a single change output in the batch transaction.

This seems to be outdated.

[starius]

LGTM! 🥇

Added a couple of proposals.

[starius]

LGTM! 🌴

One comment pending: #976 (comment)

[starius]

LGTM pending subtle issues with dust limits and relay fee! 💾

Please add min relay fee to constructUnsignedTx in a separate commit, since it is a separate factor.

[starius]

This test case passes even if changeOutputWeight is not added. Also its name is confusing, because the sweep is not placed in "existing medium batch", but it also added to a new batch.

I propose to replace this test case which another one which works as control for "low fee change sweep, placed in new batch":

diff --git a/sweepbatcher/greedy_batch_selection_test.go b/sweepbatcher/greedy_batch_selection_test.go
index 33b0d5a..0a819f9 100644
--- a/sweepbatcher/greedy_batch_selection_test.go
+++ b/sweepbatcher/greedy_batch_selection_test.go
@@ -835,8 +835,8 @@ func TestSelectBatches(t *testing.T) {
                },
 
                {
-                       name: "high fee change sweep, placed in existing " +
-                               "medium batch",
+                       name: "low fee sweep without change, placed in "+
+                               "existing batch",
                        batches: []feeDetails{
                                {
                                        BatchId: 1,
@@ -845,14 +845,14 @@ func TestSelectBatches(t *testing.T) {
                                },
                        },
                        sweep: feeDetails{
-                               FeeRate: highFeeRate,
-                               Weight:  coopInputWeight + changeOutputWeight,
+                               FeeRate: lowFeeRate,
+                               Weight:  coopInputWeight,
                        },
                        oneSweepBatch: feeDetails{
-                               FeeRate: highFeeRate,
+                               FeeRate: lowFeeRate,
                                Weight:  coopNewBatchWeight,
                        },
-                       wantBestBatchesIds: []int32{newBatchSignal, 1},
+                       wantBestBatchesIds: []int32{1, newBatchSignal},
                },
        }
 

[starius]

In test TestPresigned/dust_main_output I saw:

fee 0.00000065 BTC

Such a fee is too low (below relay fee).

I think we should fail here if the fee is below relay fee rate. The function should accept min relay fee rate as an argument, we already have the value in many places, where the function is called.

Feerate can be found with chainfee.NewSatPerKWeight(fee, weight).

Please add a unit test for constructUnsignedTx for this error and adjust test TestPresigned/dust_main_output not to crash on this error. Additionally, you can add another test case for TestPresigned to reproduce this new error (fee below relay fee).

[starius]

destAddr and batchPkScript used as batch destination address in most cases are a p2wkh address, not taproot.

I propose to use our new function utils.DustLimitForPkScript(batchPkScript) here instead of lnwallet.DustLimitForSize(input.P2TRSize) to get the same values. The numeric values are different. Taproot is 330, p2wkh is 294.

I discovered this by changing change amounts by 2 satoshis in the test cases:

diff --git a/sweepbatcher/sweep_batcher_presigned_test.go b/sweepbatcher/sweep_batcher_presigned_test.go
index fd28bc7..2ad90ab 100644
--- a/sweepbatcher/sweep_batcher_presigned_test.go
+++ b/sweepbatcher/sweep_batcher_presigned_test.go
@@ -1318,7 +1318,7 @@ func testPresigned_presigned_group_with_dust_main_output(t *testing.T,
        }
        dustLimit := int64(lnwallet.DustLimitForSize(input.P2TRSize))
        change := &wire.TxOut{
-               Value:    inputValue - dustLimit + 1,
+               Value:    inputValue - dustLimit - 1,
                PkScript: []byte{0xaf, 0xfe},
        }
 
@@ -1401,7 +1401,7 @@ func testPresigned_presigned_group_with_dust_change(t *testing.T,
        }
        dustLimit := lnwallet.DustLimitForSize(input.P2TRSize)
        change := &wire.TxOut{
-               Value:    int64(dustLimit - 1),
+               Value:    int64(dustLimit + 1),
                PkScript: []byte{0xaf, 0xfe},
        }
 

My expectation was that inputs would be added successfully (and the tests would fail, as they test failures). However it was not the case, because these dustLimit variables used in sweep_batch_test.go are higher than the actual limit.

Please use utils.DustLimitForPkScript(batchPkScript) here below in this file.