-
Notifications
You must be signed in to change notification settings - Fork 118
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
StaticAddr: Withdraw arbitrary amounts #860
base: master
Are you sure you want to change the base?
Changes from 1 commit
92f2d9f
c9da71a
a7cbcbd
7e22d20
8ed9cb7
5b9f050
1fc8818
cf53844
b4e4ef8
b6f8d4d
8d13cf0
9551390
4679230
c3fae72
fd63b62
2ec9c78
7f1e5d6
24e7b55
6d449b5
909ec27
39f77fe
843ef4b
d0d480a
8e0c23a
7487b90
e2446ca
d460e5e
0a1f78b
8bdade6
69cf9f1
1fd8238
00f8d5f
633ca67
ac4f9ba
b10a422
31750d5
2cd8145
3482993
39963b2
d2012e7
c724604
f773985
edf5f60
fcff53b
fb1305e
ea3c5e5
e72d40b
32690c5
22b22e0
bb5a6a5
d8ec87d
701b187
dc21d43
b8698e2
5e30133
ad122d2
023d593
bdfc16d
fc90aa0
c0e9d7a
f8862c7
f8599ba
c1f3f75
c2281b8
6982adb
0d30733
fb9562b
0d4fbae
57c9171
90057d8
b646ed1
827a65b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,7 @@ import ( | |
"reflect" | ||
"strings" | ||
|
||
"github.com/btcsuite/btcd/btcec/v2/schnorr" | ||
"github.com/btcsuite/btcd/btcec/v2/schnorr/musig2" | ||
"github.com/btcsuite/btcd/btcutil" | ||
"github.com/btcsuite/btcd/chaincfg" | ||
|
@@ -75,6 +76,7 @@ type newWithdrawalRequest struct { | |
respChan chan *newWithdrawalResponse | ||
destAddr string | ||
satPerVbyte int64 | ||
amount int64 | ||
} | ||
|
||
// newWithdrawalResponse is used to return withdrawal info and error to the | ||
|
@@ -156,10 +158,10 @@ func (m *Manager) Run(ctx context.Context, currentHeight uint32) error { | |
err) | ||
} | ||
|
||
case request := <-m.newWithdrawalRequestChan: | ||
case req := <-m.newWithdrawalRequestChan: | ||
txHash, pkScript, err = m.WithdrawDeposits( | ||
ctx, request.outpoints, request.destAddr, | ||
request.satPerVbyte, | ||
ctx, req.outpoints, req.destAddr, | ||
req.satPerVbyte, req.amount, | ||
) | ||
if err != nil { | ||
log.Errorf("Error withdrawing deposits: %v", | ||
|
@@ -174,7 +176,7 @@ func (m *Manager) Run(ctx context.Context, currentHeight uint32) error { | |
err: err, | ||
} | ||
select { | ||
case request.respChan <- resp: | ||
case req.respChan <- resp: | ||
|
||
case <-ctx.Done(): | ||
// Notify subroutines that the main loop has | ||
|
@@ -261,8 +263,8 @@ func (m *Manager) WaitInitComplete() { | |
|
||
// WithdrawDeposits starts a deposits withdrawal flow. | ||
func (m *Manager) WithdrawDeposits(ctx context.Context, | ||
outpoints []wire.OutPoint, destAddr string, satPerVbyte int64) (string, | ||
string, error) { | ||
outpoints []wire.OutPoint, destAddr string, satPerVbyte int64, | ||
amount int64) (string, string, error) { | ||
|
||
if len(outpoints) == 0 { | ||
return "", "", fmt.Errorf("no outpoints selected to " + | ||
|
@@ -272,7 +274,8 @@ func (m *Manager) WithdrawDeposits(ctx context.Context, | |
// Ensure that the deposits are in a state in which they can be | ||
// withdrawn. | ||
deposits, allActive := m.cfg.DepositManager.AllOutpointsActiveDeposits( | ||
outpoints, deposit.Deposited) | ||
outpoints, deposit.Deposited, | ||
) | ||
|
||
if !allActive { | ||
return "", "", ErrWithdrawingInactiveDeposits | ||
|
@@ -303,7 +306,7 @@ func (m *Manager) WithdrawDeposits(ctx context.Context, | |
} | ||
|
||
finalizedTx, err := m.createFinalizedWithdrawalTx( | ||
ctx, deposits, withdrawalAddress, satPerVbyte, | ||
ctx, deposits, withdrawalAddress, satPerVbyte, amount, | ||
) | ||
if err != nil { | ||
return "", "", err | ||
|
@@ -355,7 +358,8 @@ func (m *Manager) WithdrawDeposits(ctx context.Context, | |
|
||
func (m *Manager) createFinalizedWithdrawalTx(ctx context.Context, | ||
deposits []*deposit.Deposit, withdrawalAddress btcutil.Address, | ||
satPerVbyte int64) (*wire.MsgTx, error) { | ||
satPerVbyte int64, selectedWithdrawalAmount int64) (*wire.MsgTx, | ||
error) { | ||
|
||
// Create a musig2 session for each deposit. | ||
withdrawalSessions, clientNonces, err := m.createMusig2Sessions( | ||
|
@@ -380,32 +384,55 @@ func (m *Manager) createFinalizedWithdrawalTx(ctx context.Context, | |
).FeePerKWeight() | ||
} | ||
|
||
outpoints := toOutpoints(deposits) | ||
resp, err := m.cfg.StaticAddressServerClient.ServerWithdrawDeposits( | ||
ctx, &staticaddressrpc.ServerWithdrawRequest{ | ||
Outpoints: toPrevoutInfo(outpoints), | ||
ClientNonces: clientNonces, | ||
ClientSweepAddr: withdrawalAddress.String(), | ||
TxFeeRate: uint64(withdrawalSweepFeeRate), | ||
}, | ||
params, err := m.cfg.AddressManager.GetStaticAddressParameters( | ||
ctx, | ||
) | ||
if err != nil { | ||
return nil, err | ||
return nil, fmt.Errorf("couldn't get confirmation height for "+ | ||
"deposit, %w", err) | ||
} | ||
|
||
addressParams, err := m.cfg.AddressManager.GetStaticAddressParameters( | ||
ctx, | ||
// Send change back to the static address. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I propose this change to make it more clear that address is the same. s/to the static address/to the same static address/ |
||
staticAddress, err := m.cfg.AddressManager.GetStaticAddress(ctx) | ||
if err != nil { | ||
log.Warnf("error retrieving taproot address %w", err) | ||
|
||
return nil, fmt.Errorf("withdrawal failed") | ||
} | ||
|
||
changeAddress, err := btcutil.NewAddressTaproot( | ||
schnorr.SerializePubKey(staticAddress.TaprootKey), | ||
m.cfg.ChainParams, | ||
) | ||
if err != nil { | ||
return nil, fmt.Errorf("couldn't get confirmation height for "+ | ||
"deposit, %w", err) | ||
return nil, err | ||
} | ||
|
||
prevOuts := m.toPrevOuts(deposits, addressParams.PkScript) | ||
outpoints := toOutpoints(deposits) | ||
prevOuts := m.toPrevOuts(deposits, params.PkScript) | ||
totalValue := withdrawalValue(prevOuts) | ||
withdrawalTx, err := m.createWithdrawalTx( | ||
outpoints, totalValue, withdrawalAddress, | ||
withdrawalSweepFeeRate, | ||
withdrawalTx, withdrawAmount, changeAmount, err := m.createWithdrawalTx( | ||
outpoints, totalValue, btcutil.Amount(selectedWithdrawalAmount), | ||
withdrawalAddress, changeAddress, withdrawalSweepFeeRate, | ||
) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
// Request the server to sign the withdrawal transaction. | ||
// | ||
// The withdrawal and change amount are sent to the server with the | ||
// expectation that the server just signs the transaction, without | ||
// performing fee calculations and dust considerations. The client is | ||
// responsible for that. | ||
resp, err := m.cfg.StaticAddressServerClient.ServerWithdrawDeposits( | ||
ctx, &staticaddressrpc.ServerWithdrawRequest{ | ||
Outpoints: toPrevoutInfo(outpoints), | ||
ClientNonces: clientNonces, | ||
ClientWithdrawalAddr: withdrawalAddress.String(), | ||
WithdrawAmount: int64(withdrawAmount), | ||
ChangeAmount: int64(changeAmount), | ||
}, | ||
) | ||
if err != nil { | ||
return nil, err | ||
|
@@ -613,9 +640,10 @@ func byteSliceTo66ByteSlice(b []byte) ([musig2.PubNonceSize]byte, error) { | |
} | ||
|
||
func (m *Manager) createWithdrawalTx(outpoints []wire.OutPoint, | ||
withdrawlAmount btcutil.Amount, clientSweepAddress btcutil.Address, | ||
feeRate chainfee.SatPerKWeight) (*wire.MsgTx, | ||
error) { | ||
totalWithdrawalAmount btcutil.Amount, | ||
selectedWithdrawalAmount btcutil.Amount, withdrawAddr btcutil.Address, | ||
changeAddress *btcutil.AddressTaproot, feeRate chainfee.SatPerKWeight) ( | ||
*wire.MsgTx, btcutil.Amount, btcutil.Amount, error) { | ||
|
||
// First Create the tx. | ||
msgTx := wire.NewMsgTx(2) | ||
|
@@ -628,33 +656,101 @@ func (m *Manager) createWithdrawalTx(outpoints []wire.OutPoint, | |
}) | ||
} | ||
|
||
// Estimate the fee. | ||
weight, err := withdrawalFee(len(outpoints), clientSweepAddress) | ||
var ( | ||
hasChange bool | ||
dustLimit = lnwallet.DustLimitForSize(input.P2TRSize) | ||
withdrawalAmount btcutil.Amount | ||
changeAmount btcutil.Amount | ||
) | ||
|
||
// Estimate the transaction weight without change. | ||
weight, err := withdrawalTxWeight(len(outpoints), withdrawAddr, false) | ||
if err != nil { | ||
return nil, err | ||
return nil, 0, 0, err | ||
} | ||
feeWithoutChange := feeRate.FeeForWeight(weight) | ||
|
||
pkscript, err := txscript.PayToAddrScript(clientSweepAddress) | ||
if err != nil { | ||
return nil, err | ||
// If the user selected a fraction of the sum of the selected deposits | ||
// to withdraw, check if a change output is needed. | ||
if selectedWithdrawalAmount > 0 { | ||
// Estimate the transaction weight with change. | ||
weight, err = withdrawalTxWeight( | ||
len(outpoints), withdrawAddr, true, | ||
) | ||
if err != nil { | ||
return nil, 0, 0, err | ||
} | ||
feeWithChange := feeRate.FeeForWeight(weight) | ||
|
||
// The available change that can cover fees is the total | ||
// selected deposit amount minus the selected withdrawal amount. | ||
change := totalWithdrawalAmount - selectedWithdrawalAmount | ||
|
||
switch { | ||
case change-feeWithChange >= dustLimit: | ||
// If the change can cover the fees without turning into | ||
// dust, add a non-dust change output. | ||
hasChange = true | ||
changeAmount = change - feeWithChange | ||
withdrawalAmount = selectedWithdrawalAmount | ||
|
||
case change-feeWithChange >= 0: | ||
// If the change is dust, we give it to the miners. | ||
hasChange = false | ||
withdrawalAmount = selectedWithdrawalAmount | ||
|
||
default: | ||
// If the fees eat into our withdrawal amount, we fail | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, the feeWithChange fees eat into our withdrawal amount, but this doesn't mean that feeWithoutChange do, since it is lower than feeWithChange. This means, that some amounts fail, even though it is possible to withdraw without change. And we fallback to withdrawing without change in general case when it is not enough funds for the change. So I think for consistency we should do the same here. Can we replace |
||
// the withdrawal. | ||
return nil, 0, 0, fmt.Errorf("the change doesn't " + | ||
"cover for fees. Consider lowering the fee " + | ||
"rate or increase the withdrawal amount") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. s/increase/decrease/ IIUC |
||
} | ||
} else { | ||
// If the user wants to withdraw the full amount, we don't need | ||
// a change output. | ||
hasChange = false | ||
withdrawalAmount = totalWithdrawalAmount - feeWithoutChange | ||
} | ||
|
||
fee := feeRate.FeeForWeight(weight) | ||
if withdrawalAmount < dustLimit { | ||
return nil, 0, 0, fmt.Errorf("withdrawal amount is below " + | ||
"dust limit") | ||
} | ||
|
||
if changeAmount < 0 { | ||
return nil, 0, 0, fmt.Errorf("change amount is negative") | ||
} | ||
|
||
// Create the sweep output | ||
sweepOutput := &wire.TxOut{ | ||
Value: int64(withdrawlAmount) - int64(fee), | ||
PkScript: pkscript, | ||
withdrawScript, err := txscript.PayToAddrScript(withdrawAddr) | ||
if err != nil { | ||
return nil, 0, 0, err | ||
} | ||
|
||
msgTx.AddTxOut(sweepOutput) | ||
// Create the withdrawal output. | ||
msgTx.AddTxOut(&wire.TxOut{ | ||
Value: int64(withdrawalAmount), | ||
PkScript: withdrawScript, | ||
}) | ||
|
||
if hasChange { | ||
changeScript, err := txscript.PayToAddrScript(changeAddress) | ||
if err != nil { | ||
return nil, 0, 0, err | ||
} | ||
|
||
return msgTx, nil | ||
msgTx.AddTxOut(&wire.TxOut{ | ||
Value: int64(changeAmount), | ||
PkScript: changeScript, | ||
}) | ||
} | ||
|
||
return msgTx, withdrawalAmount, changeAmount, nil | ||
} | ||
|
||
// withdrawalFee returns the weight for the withdrawal transaction. | ||
func withdrawalFee(numInputs int, | ||
sweepAddress btcutil.Address) (lntypes.WeightUnit, error) { | ||
func withdrawalTxWeight(numInputs int, sweepAddress btcutil.Address, | ||
hasChange bool) (lntypes.WeightUnit, error) { | ||
|
||
var weightEstimator input.TxWeightEstimator | ||
for i := 0; i < numInputs; i++ { | ||
|
@@ -676,6 +772,11 @@ func withdrawalFee(numInputs int, | |
sweepAddress) | ||
} | ||
|
||
// If there's a change output add the weight of the static address. | ||
if hasChange { | ||
weightEstimator.AddP2TROutput() | ||
} | ||
|
||
return weightEstimator.Weight(), nil | ||
} | ||
|
||
|
@@ -814,13 +915,14 @@ func (m *Manager) republishWithdrawals(ctx context.Context) error { | |
// DeliverWithdrawalRequest forwards a withdrawal request to the manager main | ||
// loop. | ||
func (m *Manager) DeliverWithdrawalRequest(ctx context.Context, | ||
outpoints []wire.OutPoint, destAddr string, satPerVbyte int64) (string, | ||
string, error) { | ||
outpoints []wire.OutPoint, destAddr string, satPerVbyte int64, | ||
amount int64) (string, string, error) { | ||
|
||
request := newWithdrawalRequest{ | ||
outpoints: outpoints, | ||
destAddr: destAddr, | ||
satPerVbyte: satPerVbyte, | ||
amount: amount, | ||
respChan: make(chan *newWithdrawalResponse), | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I propose to add to godoc, that amount=0 means "withdraw everything".