sweepbatcher: add mode with presigned transactions #891
Conversation
Also added a unit test for it.
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.
sweepbatcher/sweep_batch.go
Outdated
| for _, s := range b.sweeps { | ||
| if !s.presigned { | ||
| b.log().Infof("failed to add sweep %x to the "+ | ||
| "batch, because the batch has "+ |
There was a problem hiding this comment.
nit: maybe clarify "failed to add presigned sweep %x..."
sweepbatcher/sweep_batch.go
Outdated
| // 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 "+ |
There was a problem hiding this comment.
nit: maybe clarify "failed to add non-presigned sweep %x..."
sweepbatcher/sweep_batch.go
Outdated
|
|
||
| for _, sweep := range b.sweeps { | ||
| if sweep.presigned { | ||
| return true, nil |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| @@ -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. | |||
There was a problem hiding this comment.
I see this is ensured in the batch by b.ensurePresigned
There was a problem hiding this comment.
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.)
sweepbatcher/sweep_batcher.go
Outdated
| } | ||
|
|
||
| // Find the feerate needed to get into next block. Use conf_target=2, | ||
| nextBlockFeerate, err := b.wallet.EstimateFeeRate(ctx, 2) |
There was a problem hiding this comment.
nit: camel case nextBlockFeeRate
| return false, nil | ||
| } | ||
| } else { | ||
| if err := b.ensurePresigned(ctx, sweep); err != nil { |
There was a problem hiding this comment.
if err == nil here, don't we then have to call b.presign for the sweep here?
There was a problem hiding this comment.
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).
| // 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 { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 :)
sweepbatcher/presigned.go
Outdated
| batchAmt += sweep.value | ||
| } | ||
|
|
||
| // Find when at least one sweep expires. |
There was a problem hiding this comment.
nit: "find the sweep with the earliest expiry"
sweepbatcher/presigned.go
Outdated
| } | ||
|
|
||
| // Find the feerate needed to get into next block. Use conf_target=2, | ||
| nextBlockFeerate, err := b.wallet.EstimateFeeRate(ctx, 2) |
There was a problem hiding this comment.
why is it 2? Maybe create a constant?
There was a problem hiding this comment.
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).
This is needed to implement ensurePresigned as read-only, so it only checks if a presigned transaction exists, not tries to sign it.
|
@hieblmi: review reminder |
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
release_notes.mdif your PR contains major features, breaking changes or bugfixes