Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sweepbatcher: add mode with presigned transactions #891

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
441 changes: 441 additions & 0 deletions sweepbatcher/presigned.go

Large diffs are not rendered by default.

848 changes: 848 additions & 0 deletions sweepbatcher/presigned_test.go

Large diffs are not rendered by default.

145 changes: 136 additions & 9 deletions sweepbatcher/sweep_batch.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import (
"github.com/btcsuite/btcd/btcec/v2/schnorr/musig2"
"github.com/btcsuite/btcd/btcutil"
"github.com/btcsuite/btcd/btcutil/psbt"
"github.com/btcsuite/btcd/chaincfg"
"github.com/btcsuite/btcd/chaincfg/chainhash"
"github.com/btcsuite/btcd/txscript"
"github.com/btcsuite/btcd/wire"
Expand Down Expand Up @@ -120,6 +121,9 @@ type sweep struct {
// but it failed. We try to spend a sweep cooperatively only once. This
// status is not persisted in the DB.
coopFailed bool

// presigned is set, if the sweep should be handled in presigned mode.
presigned bool
}

// batchState is the state of the batch.
Expand Down Expand Up @@ -173,6 +177,14 @@ type batchConfig struct {
// Note that musig2SignSweep must be nil in this case, however signer
// client must still be provided, as it is used for non-coop spendings.
customMuSig2Signer SignMuSig2

// presignedHelper provides methods used when presigned batches are
// enabled.
presignedHelper PresignedHelper

// chainParams are the chain parameters of the chain that is used by
// batches.
chainParams *chaincfg.Params
}

// rbfCache stores data related to our last fee bump.
Expand Down Expand Up @@ -460,7 +472,9 @@ func (b *batch) Errorf(format string, params ...interface{}) {
}

// addSweep tries to add a sweep to the batch. If this is the first sweep being
// added to the batch then it also sets the primary sweep ID.
// added to the batch then it also sets the primary sweep ID. If presigned mode
// is enabled, the result depends on the outcome of presignedHelper.Presign for
// a non-empty batch. For an empty batch, the input needs to pass PresignSweep.
func (b *batch) addSweep(ctx context.Context, sweep *sweep) (bool, error) {
done, err := b.scheduleNextCall()
defer done()
Expand Down Expand Up @@ -566,6 +580,54 @@ func (b *batch) addSweep(ctx context.Context, sweep *sweep) (bool, error) {
}
}

// If presigned mode is enabled, we should first presign the new version
// of batch transaction. Also ensure that all the sweeps in the batch
// 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 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

if !s.presigned {
b.log().Infof("failed to add sweep %x to the "+
"batch, because the batch has "+
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

"non-presigned sweep %x",
sweep.swapHash[:6], s.swapHash[:6])

return false, nil
}
}

if len(b.sweeps) != 0 {
if err := b.presign(ctx, sweep); err != nil {
b.log().Infof("failed to add sweep %x to the "+
"batch, because failed to presign new "+
"version of batch tx: %v",
sweep.swapHash[:6], err)

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

return false, fmt.Errorf("failed to check "+
"signing of input %x, this means that "+
"batcher.PresignSweep was not called "+
"prior to AddSweep for this input: %w",
sweep.swapHash[:6], err)
}
}
} else {
// 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 "+
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

"batch, because the batch has "+
"presigned sweep %x",
sweep.swapHash[:6], s.swapHash[:6])

return false, nil
}
}
}

// Past this point we know that a new incoming sweep passes the
// acceptance criteria and is now ready to be added to this batch.

Expand Down Expand Up @@ -863,6 +925,23 @@ func (b *batch) isUrgent(skipBefore time.Time) bool {
return true
}

// isPresigned returns if the batch uses presigned mode. Currently presigned and
// non-presigned sweeps never appear in the same batch. Fails if the batch is
// empty.
func (b *batch) isPresigned() (bool, error) {
if len(b.sweeps) == 0 {
return false, fmt.Errorf("the batch is empty")
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}

return false, nil
}

// publish creates and publishes the latest batch transaction to the network.
func (b *batch) publish(ctx context.Context) error {
var (
Expand All @@ -888,7 +967,19 @@ func (b *batch) publish(ctx context.Context) error {
b.publishErrorHandler(err, errMsg, b.log())
}

fee, err, signSuccess = b.publishMixedBatch(ctx)
// Determine if we should use presigned mode for the batch.
presigned, err := b.isPresigned()
if err != nil {
return fmt.Errorf("failed to determine if the batch %d uses "+
"presigned mode: %w", b.id, err)
}

if presigned {
fee, err, signSuccess = b.publishPresigned(ctx)
} else {
fee, err, signSuccess = b.publishMixedBatch(ctx)
}

if err != nil {
if signSuccess {
logPublishError("publish error", err)
Expand Down Expand Up @@ -959,9 +1050,9 @@ func (b *batch) createPsbt(unsignedTx *wire.MsgTx, sweeps []sweep) ([]byte,

// constructUnsignedTx creates unsigned tx from the sweeps, paying to the addr.
// It also returns absolute fee (from weight and clamped).
func (b *batch) constructUnsignedTx(sweeps []sweep,
address btcutil.Address) (*wire.MsgTx, lntypes.WeightUnit,
btcutil.Amount, btcutil.Amount, error) {
func constructUnsignedTx(sweeps []sweep, address btcutil.Address,
currentHeight int32, feeRate chainfee.SatPerKWeight) (*wire.MsgTx,
lntypes.WeightUnit, btcutil.Amount, btcutil.Amount, error) {

// Sanity check, there should be at least 1 sweep in this batch.
if len(sweeps) == 0 {
Expand All @@ -971,7 +1062,7 @@ func (b *batch) constructUnsignedTx(sweeps []sweep,
// Create the batch transaction.
batchTx := &wire.MsgTx{
Version: 2,
LockTime: uint32(b.currentHeight),
LockTime: uint32(currentHeight),
}

// Add transaction inputs and estimate its weight.
Expand Down Expand Up @@ -1023,7 +1114,7 @@ func (b *batch) constructUnsignedTx(sweeps []sweep,

// Find weight and fee.
weight := weightEstimate.Weight()
feeForWeight := b.rbfCache.FeeRate.FeeForWeight(weight)
feeForWeight := feeRate.FeeForWeight(weight)

// Clamp the calculated fee to the max allowed fee amount for the batch.
fee := clampBatchFee(feeForWeight, batchAmt)
Expand Down Expand Up @@ -1108,8 +1199,8 @@ func (b *batch) publishMixedBatch(ctx context.Context) (btcutil.Amount, error,

// Construct unsigned batch transaction.
var err error
tx, weight, feeForWeight, fee, err = b.constructUnsignedTx(
sweeps, address,
tx, weight, feeForWeight, fee, err = constructUnsignedTx(
sweeps, address, b.currentHeight, b.rbfCache.FeeRate,
)
if err != nil {
return 0, fmt.Errorf("failed to construct tx: %w", err),
Expand Down Expand Up @@ -1769,6 +1860,28 @@ func (b *batch) handleSpend(ctx context.Context, spendTx *wire.MsgTx) error {
// handleConf handles a confirmation notification. This is the final step of the
// batch. Here we signal to the batcher that this batch was completed.
func (b *batch) handleConf(ctx context.Context) error {
// If the batch is in presigned mode, cleanup presignedHelper.
presigned, err := b.isPresigned()
if err != nil {
return fmt.Errorf("failed to determine if the batch %d uses "+
"presigned mode: %w", b.id, err)
}

if presigned {
b.Infof("Cleaning up presigned store")

inputs := make([]wire.OutPoint, 0, len(b.sweeps))
for _, sweep := range b.sweeps {
inputs = append(inputs, sweep.outpoint)
}

err := b.cfg.presignedHelper.CleanupTransactions(ctx, inputs)
if err != nil {
return fmt.Errorf("failed to clean up store for "+
"batch %d, inputs %v: %w", b.id, inputs, err)
}
}

b.Infof("confirmed in txid %s", b.batchTxid)
b.state = Confirmed

Expand Down Expand Up @@ -1811,7 +1924,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.
func (b *batch) getBatchDestAddr(ctx context.Context) (btcutil.Address, error) {
// Determine if we should use presigned mode for the batch.
presigned, err := b.isPresigned()
if err != nil {
return nil, fmt.Errorf("failed to determine if the batch %d "+
"uses presigned mode: %w", b.id, err)
}

// Make sure that the method is not used for presigned batches.
if presigned {
return nil, fmt.Errorf("getBatchDestAddr used in presigned " +
"mode")
}

var address btcutil.Address

// If a batch address is set, use that. Otherwise, generate a
Expand Down
Loading