From 4a4e0c73f7c31c5ac4e4cad5076bd183902725d2 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Sun, 10 Jan 2021 21:06:47 +0100 Subject: [PATCH 1/6] lnwallet/chanfunding: add Inputs/Outputs to assemblers This will be used to try to estimate how much the funding transaction will decrease our wallet balance. --- lnwallet/chanfunding/assembler.go | 10 +++++ lnwallet/chanfunding/canned_assembler.go | 24 ++++++++++++ lnwallet/chanfunding/psbt_assembler.go | 47 ++++++++++++++++++++++++ lnwallet/chanfunding/wallet_assembler.go | 22 +++++++++++ 4 files changed, 103 insertions(+) diff --git a/lnwallet/chanfunding/assembler.go b/lnwallet/chanfunding/assembler.go index 2a78f375..4fcb8676 100644 --- a/lnwallet/chanfunding/assembler.go +++ b/lnwallet/chanfunding/assembler.go @@ -103,6 +103,16 @@ type Intent interface { // change. LocalFundingAmt() btcutil.Amount + // Inputs returns all inputs to the final funding transaction that we + // know about. Note that there might be more, but we are not (yet) + // aware of. + Inputs() []wire.OutPoint + + // Outputs returns all outputs of the final funding transaction that we + // know about. Note that there might be more, but we are not (yet) + // aware of. + Outputs() []*wire.TxOut + // Cancel allows the caller to cancel a funding Intent at any time. // This will return any resources such as coins back to the eligible // pool to be used in order channel fundings. diff --git a/lnwallet/chanfunding/canned_assembler.go b/lnwallet/chanfunding/canned_assembler.go index b512a722..5bd88682 100644 --- a/lnwallet/chanfunding/canned_assembler.go +++ b/lnwallet/chanfunding/canned_assembler.go @@ -98,6 +98,30 @@ func (s *ShimIntent) ThawHeight() uint32 { return s.thawHeight } +// Inputs returns all inputs to the final funding transaction that we +// know about. For the ShimIntent this will always be none, since it is funded +// externally. +func (s *ShimIntent) Inputs() []wire.OutPoint { + return nil +} + +// Outputs returns all outputs of the final funding transaction that we +// know about. Since this is an externally funded channel, the channel output +// is the only known one. +func (s *ShimIntent) Outputs() []*wire.TxOut { + _, txOut, err := s.FundingOutput() + if err != nil { + log.Warnf("Unable to find funding output for shim intent: %v", + err) + + // Failed finding funding output, return empty list of known + // outputs. + return nil + } + + return []*wire.TxOut{txOut} +} + // FundingKeys couples our multi-sig key along with the remote party's key. type FundingKeys struct { // LocalKey is our multi-sig key. diff --git a/lnwallet/chanfunding/psbt_assembler.go b/lnwallet/chanfunding/psbt_assembler.go index a40bae3c..4d15ff53 100644 --- a/lnwallet/chanfunding/psbt_assembler.go +++ b/lnwallet/chanfunding/psbt_assembler.go @@ -392,6 +392,53 @@ func (i *PsbtIntent) Cancel() { i.ShimIntent.Cancel() } +// Inputs returns all inputs to the final funding transaction that we know +// about. These are only known after the PSBT has been verified. +func (i *PsbtIntent) Inputs() []wire.OutPoint { + var inputs []wire.OutPoint + + switch i.State { + + // We return the inputs to the pending psbt. + case PsbtVerified: + for _, in := range i.PendingPsbt.UnsignedTx.TxIn { + inputs = append(inputs, in.PreviousOutPoint) + } + + // We return the inputs to the final funding tx. + case PsbtFinalized, PsbtFundingTxCompiled: + for _, in := range i.FinalTX.TxIn { + inputs = append(inputs, in.PreviousOutPoint) + } + + // In all other states we cannot know the inputs to the funding tx, and + // return an empty list. + default: + } + + return inputs +} + +// Outputs returns all outputs of the final funding transaction that we +// know about. These are only known after the PSBT has been verified. +func (i *PsbtIntent) Outputs() []*wire.TxOut { + switch i.State { + + // We return the outputs of the pending psbt. + case PsbtVerified: + return i.PendingPsbt.UnsignedTx.TxOut + + // We return the outputs of the final funding tx. + case PsbtFinalized, PsbtFundingTxCompiled: + return i.FinalTX.TxOut + + // In all other states we cannot know the final outputs, and return an + // empty list. + default: + return nil + } +} + // PsbtAssembler is a type of chanfunding.Assembler wherein the funding // transaction is constructed outside of lnd by using partially signed bitcoin // transactions (PSBT). diff --git a/lnwallet/chanfunding/wallet_assembler.go b/lnwallet/chanfunding/wallet_assembler.go index 554654fe..9cdf9b10 100644 --- a/lnwallet/chanfunding/wallet_assembler.go +++ b/lnwallet/chanfunding/wallet_assembler.go @@ -152,6 +152,28 @@ func (f *FullIntent) CompileFundingTx(extraInputs []*wire.TxIn, return fundingTx, nil } +// Inputs returns all inputs to the final funding transaction that we +// know about. Since this funding transaction is created all from our wallet, +// it will be all inputs. +func (f *FullIntent) Inputs() []wire.OutPoint { + var ins []wire.OutPoint + for _, coin := range f.InputCoins { + ins = append(ins, coin.OutPoint) + } + + return ins +} + +// Outputs returns all outputs of the final funding transaction that we +// know about. This will be the funding output and the change outputs going +// back to our wallet. +func (f *FullIntent) Outputs() []*wire.TxOut { + outs := f.ShimIntent.Outputs() + outs = append(outs, f.ChangeOutputs...) + + return outs +} + // Cancel allows the caller to cancel a funding Intent at any time. This will // return any resources such as coins back to the eligible pool to be used in // order channel fundings. From 3cc31ae8410f489c7601f307703fdc38d5983f3b Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Mon, 11 Jan 2021 11:03:00 +0100 Subject: [PATCH 2/6] lnwallet: add CheckReservedValue --- lnwallet/wallet.go | 94 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 94 insertions(+) diff --git a/lnwallet/wallet.go b/lnwallet/wallet.go index 66ee5582..296fbf87 100644 --- a/lnwallet/wallet.go +++ b/lnwallet/wallet.go @@ -5,6 +5,7 @@ import ( "crypto/sha256" "errors" "fmt" + "math" "net" "sync" "sync/atomic" @@ -32,6 +33,12 @@ const ( // The size of the buffered queue of requests to the wallet from the // outside word. msgBufferSize = 100 + + // anchorChanReservedValue is the amount we'll keep around in the + // wallet in case we have to fee bump anchor channels on force close. + // TODO(halseth): update constant to target a specific commit size at + // set fee rate. + anchorChanReservedValue = btcutil.Amount(10_000) ) var ( @@ -39,6 +46,12 @@ var ( // contribution handling process if the process should be paused for // the construction of a PSBT outside of lnd's wallet. ErrPsbtFundingRequired = errors.New("PSBT funding required") + + // ErrReservedValueInvalidated is returned if we try to publish a + // transaction that would take the walletbalance below what we require + // to keep around to fee bump our open anchor channels. + ErrReservedValueInvalidated = errors.New("reserved wallet balance " + + "invalidated") ) // PsbtFundingRequired is a type that implements the error interface and @@ -757,6 +770,87 @@ func (l *LightningWallet) handleFundingReserveRequest(req *InitFundingReserveMsg req.err <- nil } +// CheckReservedValue checks whether publishing a transaction with the given +// inputs and outputs would violate the value we reserve in the wallet for +// bumping the fee of anchor channels. The numAnchorChans argument should be +// set the the number of open anchor channels controlled by the wallet after +// the transaction has been published. +// +// If the reserved value is violated, the returned error will be +// ErrReservedValueInvalidated. The method will also return the current +// reserved value, both in case of success and in case of +// ErrReservedValueInvalidated. +// +// NOTE: This method should only be run with the CoinSelectLock held. +func (l *LightningWallet) CheckReservedValue(in []wire.OutPoint, + out []*wire.TxOut, numAnchorChans int) (btcutil.Amount, error) { + + // Get all unspent coins in the wallet. + witnessOutputs, err := l.ListUnspentWitness(0, math.MaxInt32) + if err != nil { + return 0, err + } + + ourInput := make(map[wire.OutPoint]struct{}) + for _, op := range in { + ourInput[op] = struct{}{} + } + + // When crafting a transaction with inputs from the wallet, these coins + // will usually be locked in the process, and not be returned when + // listing unspents. In this case they have already been deducted from + // the wallet balance. In case they haven't been properly locked, we + // check whether they are still listed among our unspents and deduct + // them. + var walletBalance btcutil.Amount + for _, in := range witnessOutputs { + // Spending an unlocked wallet UTXO, don't add it to the + // balance. + if _, ok := ourInput[in.OutPoint]; ok { + continue + } + + walletBalance += in.Value + } + + // Now we go through the outputs of the transaction, if any of the + // outputs are paying into the wallet (likely a change output), we add + // it to our final balance. + for _, txOut := range out { + _, addrs, _, err := txscript.ExtractPkScriptAddrs( + txOut.PkScript, &l.Cfg.NetParams, + ) + if err != nil { + // Non-standard outputs can safely be skipped because + // they're not supported by the wallet. + continue + } + + for _, addr := range addrs { + if !l.IsOurAddress(addr) { + continue + } + + walletBalance += btcutil.Amount(txOut.Value) + + // We break since we don't want to double count the output. + break + } + } + + // We reserve a given amount for each anchor channel. + reserved := btcutil.Amount(numAnchorChans) * anchorChanReservedValue + + if walletBalance < reserved { + walletLog.Debugf("Reserved value=%v above final "+ + "walletbalance=%v with %d anchor channels open", + reserved, walletBalance, numAnchorChans) + return reserved, ErrReservedValueInvalidated + } + + return reserved, nil +} + // initOurContribution initializes the given ChannelReservation with our coins // and change reserved for the channel, and derives the keys to use for this // channel. From eaf97418bef38752300a8b9c5b3b63add2e1ecbf Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Mon, 11 Jan 2021 08:47:38 +0100 Subject: [PATCH 3/6] lnwallet: check Reserved value on funding request --- lnwallet/wallet.go | 147 +++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 142 insertions(+), 5 deletions(-) diff --git a/lnwallet/wallet.go b/lnwallet/wallet.go index 296fbf87..82ba1983 100644 --- a/lnwallet/wallet.go +++ b/lnwallet/wallet.go @@ -316,7 +316,12 @@ type LightningWallet struct { // monotonically integer. All requests concerning the channel MUST // carry a valid, active funding ID. fundingLimbo map[uint64]*ChannelReservation - limboMtx sync.RWMutex + + // reservationIDs maps a pending channel ID to the reservation ID used + // as key in the fundingLimbo map. Used to easily look up a channel + // reservation given a pending channel ID. + reservationIDs map[[32]byte]uint64 + limboMtx sync.RWMutex // lockedOutPoints is a set of the currently locked outpoint. This // information is kept in order to provide an easy way to unlock all @@ -347,6 +352,7 @@ func NewLightningWallet(Cfg Config) (*LightningWallet, error) { msgChan: make(chan interface{}, msgBufferSize), nextFundingID: 0, fundingLimbo: make(map[uint64]*ChannelReservation), + reservationIDs: make(map[[32]byte]uint64), lockedOutPoints: make(map[wire.OutPoint]struct{}), fundingIntents: make(map[[32]byte]chanfunding.Intent), quit: make(chan struct{}), @@ -417,6 +423,7 @@ func (l *LightningWallet) LockedOutpoints() []*wire.OutPoint { func (l *LightningWallet) ResetReservations() { l.nextFundingID = 0 l.fundingLimbo = make(map[uint64]*ChannelReservation) + l.reservationIDs = make(map[[32]byte]uint64) for outpoint := range l.lockedOutPoints { l.UnlockOutpoint(outpoint) @@ -523,16 +530,16 @@ func (l *LightningWallet) RegisterFundingIntent(expectedID [32]byte, // PsbtFundingVerify looks up a previously registered funding intent by its // pending channel ID and tries to advance the state machine by verifying the // passed PSBT. -func (l *LightningWallet) PsbtFundingVerify(pid [32]byte, +func (l *LightningWallet) PsbtFundingVerify(pendingChanID [32]byte, packet *psbt.Packet) error { l.intentMtx.Lock() defer l.intentMtx.Unlock() - intent, ok := l.fundingIntents[pid] + intent, ok := l.fundingIntents[pendingChanID] if !ok { return fmt.Errorf("no funding intent found for "+ - "pendingChannelID(%x)", pid[:]) + "pendingChannelID(%x)", pendingChanID[:]) } psbtIntent, ok := intent.(*chanfunding.PsbtIntent) if !ok { @@ -543,7 +550,49 @@ func (l *LightningWallet) PsbtFundingVerify(pid [32]byte, return fmt.Errorf("error verifying PSBT: %v", err) } - return nil + // Get the channel reservation for that corresponds to this pending + // channel ID. + l.limboMtx.Lock() + pid, ok := l.reservationIDs[pendingChanID] + if !ok { + l.limboMtx.Unlock() + return fmt.Errorf("no channel reservation found for "+ + "pendingChannelID(%x)", pendingChanID[:]) + } + + pendingReservation, ok := l.fundingLimbo[pid] + l.limboMtx.Unlock() + + if !ok { + return fmt.Errorf("no channel reservation found for "+ + "reservation ID %v", pid) + } + + // Now the the PSBT has been verified, we can again check whether the + // value reserved for anchor fee bumping is respected. + numAnchors, err := l.currentNumAnchorChans() + if err != nil { + return err + } + + // If this commit type is an anchor channel we add that to our counter, + // but only if we are contributing funds to the channel. This is done + // to still allow incoming channels even though we have no UTXOs + // available, as in bootstrapping phases. + if pendingReservation.partialState.ChanType.HasAnchors() && + intent.LocalFundingAmt() > 0 { + numAnchors++ + } + + // We check the reserve value again, this should already have been + // checked for regular FullIntents, but now the PSBT intent is also + // populated. + return l.WithCoinSelectLock(func() error { + _, err := l.CheckReservedValue( + intent.Inputs(), intent.Outputs(), numAnchors, + ) + return err + }) } // PsbtFundingFinalize looks up a previously registered funding intent by its @@ -727,6 +776,46 @@ func (l *LightningWallet) handleFundingReserveRequest(req *InitFundingReserveMsg thawHeight = shimIntent.ThawHeight() } + // Now that we have a funding intent, we'll check whether funding a + // channel using it would violate our reserved value for anchor channel + // fee bumping. We first get our current number of anchor channels. + numAnchors, err := l.currentNumAnchorChans() + if err != nil { + fundingIntent.Cancel() + + req.err <- err + req.resp <- nil + return + } + + // If this commit type is an anchor channel we add that to our counter, + // but only if we are contributing funds to the channel. This is done + // to still allow incoming channels even though we have no UTXOs + // available, as in bootstrapping phases. + if req.CommitType == CommitmentTypeAnchorsZeroFeeHtlcTx && + fundingIntent.LocalFundingAmt() > 0 { + numAnchors++ + } + + // Check the reserved value using the inputs and outputs given by the + // intent. Not that for the PSBT intent type we don't yet have the + // funding tx ready, so this will always pass. We'll do another check + // when the PSBT has been verified. + err = l.WithCoinSelectLock(func() error { + _, err := l.CheckReservedValue( + fundingIntent.Inputs(), fundingIntent.Outputs(), + numAnchors, + ) + return err + }) + if err != nil { + fundingIntent.Cancel() + + req.err <- err + req.resp <- nil + return + } + // The total channel capacity will be the size of the funding output we // created plus the remote contribution. capacity := localFundingAmt + remoteFundingAmt @@ -761,6 +850,7 @@ func (l *LightningWallet) handleFundingReserveRequest(req *InitFundingReserveMsg // request. l.limboMtx.Lock() l.fundingLimbo[id] = reservation + l.reservationIDs[req.PendingChanID] = id l.limboMtx.Unlock() // Funding reservation request successfully handled. The funding inputs @@ -770,6 +860,51 @@ func (l *LightningWallet) handleFundingReserveRequest(req *InitFundingReserveMsg req.err <- nil } +// currentNumAnchorChans returns the current number of anchor channels the +// wallet should be ready to fee bump if needed. +func (l *LightningWallet) currentNumAnchorChans() (int, error) { + // Count all anchor channels that are open or pending + // open, or waiting close. + chans, err := l.Cfg.Database.FetchAllChannels() + if err != nil { + return 0, err + } + + var numAnchors int + for _, c := range chans { + if c.ChanType.HasAnchors() { + numAnchors++ + } + } + + // We also count pending close channels. + pendingClosed, err := l.Cfg.Database.FetchClosedChannels( + true, + ) + if err != nil { + return 0, err + } + + for _, c := range pendingClosed { + c, err := l.Cfg.Database.FetchHistoricalChannel( + &c.ChanPoint, + ) + if err != nil { + // We don't have a guarantee that all channels re found + // in the historical channels bucket, so we continue. + walletLog.Warnf("Unable to fetch historical "+ + "channel: %v", err) + continue + } + + if c.ChanType.HasAnchors() { + numAnchors++ + } + } + + return numAnchors, nil +} + // CheckReservedValue checks whether publishing a transaction with the given // inputs and outputs would violate the value we reserve in the wallet for // bumping the fee of anchor channels. The numAnchorChans argument should be @@ -984,6 +1119,7 @@ func (l *LightningWallet) handleFundingCancelRequest(req *fundingReserveCancelMs delete(l.fundingLimbo, req.pendingFundingID) pid := pendingReservation.pendingChanID + delete(l.reservationIDs, pid) l.intentMtx.Lock() if intent, ok := l.fundingIntents[pid]; ok { @@ -1534,6 +1670,7 @@ func (l *LightningWallet) handleFundingCounterPartySigs(msg *addCounterPartySigs // Funding complete, this entry can be removed from limbo. l.limboMtx.Lock() delete(l.fundingLimbo, res.reservationID) + delete(l.reservationIDs, res.pendingChanID) l.limboMtx.Unlock() l.intentMtx.Lock() From 185ba77f8e05c497ab3edec742811a16bfc981b7 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Thu, 14 Jan 2021 11:49:27 +0100 Subject: [PATCH 4/6] sweep: allow specified outputs to sweep tx We'll use this to attach an output for the value reserved for anchor commitments fee bumping if the user requests a send_all transaction. --- rpcserver.go | 2 +- sweep/sweeper.go | 6 ++--- sweep/sweeper_test.go | 2 +- sweep/txgenerator.go | 48 +++++++++++++++++++++++++++------------ sweep/txgenerator_test.go | 2 +- sweep/walletsweep.go | 43 ++++++++++++++++++++++++++++------- sweep/walletsweep_test.go | 10 ++++---- 7 files changed, 80 insertions(+), 33 deletions(-) diff --git a/rpcserver.go b/rpcserver.go index 1d0c707c..19072226 100644 --- a/rpcserver.go +++ b/rpcserver.go @@ -1256,7 +1256,7 @@ func (r *rpcServer) SendCoins(ctx context.Context, // safe manner, so no need to worry about locking. sweepTxPkg, err := sweep.CraftSweepAllTx( feePerKw, lnwallet.DefaultDustLimit(), - uint32(bestHeight), targetAddr, wallet, + uint32(bestHeight), nil, targetAddr, wallet, wallet.WalletController, wallet.WalletController, r.server.cc.FeeEstimator, r.server.cc.Signer, ) diff --git a/sweep/sweeper.go b/sweep/sweeper.go index b7f20274..5869e456 100644 --- a/sweep/sweeper.go +++ b/sweep/sweeper.go @@ -1192,8 +1192,8 @@ func (s *UtxoSweeper) sweep(inputs inputSet, feeRate chainfee.SatPerKWeight, // Create sweep tx. tx, err := createSweepTx( - inputs, s.currentOutputScript, uint32(currentHeight), feeRate, - dustLimit(s.relayFeeRate), s.cfg.Signer, + inputs, nil, s.currentOutputScript, uint32(currentHeight), + feeRate, dustLimit(s.relayFeeRate), s.cfg.Signer, ) if err != nil { return fmt.Errorf("create sweep tx: %v", err) @@ -1487,7 +1487,7 @@ func (s *UtxoSweeper) CreateSweepTx(inputs []input.Input, feePref FeePreference, } return createSweepTx( - inputs, pkScript, currentBlockHeight, feePerKw, + inputs, nil, pkScript, currentBlockHeight, feePerKw, dustLimit(s.relayFeeRate), s.cfg.Signer, ) } diff --git a/sweep/sweeper_test.go b/sweep/sweeper_test.go index 10cbd05a..2851a643 100644 --- a/sweep/sweeper_test.go +++ b/sweep/sweeper_test.go @@ -354,7 +354,7 @@ func assertTxFeeRate(t *testing.T, tx *wire.MsgTx, outputAmt := tx.TxOut[0].Value fee := btcutil.Amount(inputAmt - outputAmt) - _, estimator := getWeightEstimate(inputs, 0) + _, estimator := getWeightEstimate(inputs, nil, 0) txWeight := estimator.weight() expectedFee := expectedFeeRate.FeeForWeight(int64(txWeight)) diff --git a/sweep/txgenerator.go b/sweep/txgenerator.go index 0afe04bf..10b3c7ee 100644 --- a/sweep/txgenerator.go +++ b/sweep/txgenerator.go @@ -131,12 +131,14 @@ func generateInputPartitionings(sweepableInputs []txInput, return sets, nil } -// createSweepTx builds a signed tx spending the inputs to a the output script. -func createSweepTx(inputs []input.Input, outputPkScript []byte, - currentBlockHeight uint32, feePerKw chainfee.SatPerKWeight, - dustLimit btcutil.Amount, signer input.Signer) (*wire.MsgTx, error) { +// createSweepTx builds a signed tx spending the inputs to the given outputs, +// sending any leftover change to the change script. +func createSweepTx(inputs []input.Input, outputs []*wire.TxOut, + changePkScript []byte, currentBlockHeight uint32, + feePerKw chainfee.SatPerKWeight, dustLimit btcutil.Amount, + signer input.Signer) (*wire.MsgTx, error) { - inputs, estimator := getWeightEstimate(inputs, feePerKw) + inputs, estimator := getWeightEstimate(inputs, outputs, feePerKw) txFee := estimator.fee() var ( @@ -210,6 +212,16 @@ func createSweepTx(inputs []input.Input, outputPkScript []byte, totalInput += btcutil.Amount(o.SignDesc().Output.Value) } + // Add the outputs given, if any. + for _, o := range outputs { + sweepTx.AddTxOut(o) + requiredOutput += btcutil.Amount(o.Value) + } + + if requiredOutput+txFee > totalInput { + return nil, fmt.Errorf("insufficient input to create sweep tx") + } + // The value remaining after the required output and fees, go to // change. Not that this fee is what we would have to pay in case the // sweep tx has a change output. @@ -219,7 +231,7 @@ func createSweepTx(inputs []input.Input, outputPkScript []byte, // above. if changeAmt >= dustLimit { sweepTx.AddTxOut(&wire.TxOut{ - PkScript: outputPkScript, + PkScript: changePkScript, Value: int64(changeAmt), }) } else { @@ -287,8 +299,8 @@ func createSweepTx(inputs []input.Input, outputPkScript []byte, // getWeightEstimate returns a weight estimate for the given inputs. // Additionally, it returns counts for the number of csv and cltv inputs. -func getWeightEstimate(inputs []input.Input, feeRate chainfee.SatPerKWeight) ( - []input.Input, *weightEstimator) { +func getWeightEstimate(inputs []input.Input, outputs []*wire.TxOut, + feeRate chainfee.SatPerKWeight) ([]input.Input, *weightEstimator) { // We initialize a weight estimator so we can accurately asses the // amount of fees we need to pay for this sweep transaction. @@ -297,13 +309,19 @@ func getWeightEstimate(inputs []input.Input, feeRate chainfee.SatPerKWeight) ( // be more efficient on-chain. weightEstimate := newWeightEstimator(feeRate) - // Our sweep transaction will pay to a single segwit p2wkh address, - // ensure it contributes to our weight estimate. If the inputs we add - // have required TxOuts, then this will be our change address. Note - // that if we have required TxOuts, we might end up creating a sweep tx - // without a change output. It is okay to add the change output to the - // weight estimate regardless, since the estimated fee will just be - // subtracted from this already dust output, and trimmed. + // Our sweep transaction will always pay to the given set of outputs. + for _, o := range outputs { + weightEstimate.addOutput(o) + } + + // If there is any leftover change after paying to the given outputs + // and required outputs, it will go to a single segwit p2wkh address. + // This will be our change address, so ensure it contributes to our + // weight estimate. Note that if we have other outputs, we might end up + // creating a sweep tx without a change output. It is okay to add the + // change output to the weight estimate regardless, since the estimated + // fee will just be subtracted from this already dust output, and + // trimmed. weightEstimate.addP2WKHOutput() // For each output, use its witness type to determine the estimate diff --git a/sweep/txgenerator_test.go b/sweep/txgenerator_test.go index 377ac24f..0543543d 100644 --- a/sweep/txgenerator_test.go +++ b/sweep/txgenerator_test.go @@ -39,7 +39,7 @@ func TestWeightEstimate(t *testing.T) { )) } - _, estimator := getWeightEstimate(inputs, 0) + _, estimator := getWeightEstimate(inputs, nil, 0) weight := int64(estimator.weight()) if weight != expectedWeight { t.Fatalf("unexpected weight. expected %d but got %d.", diff --git a/sweep/walletsweep.go b/sweep/walletsweep.go index 06c41e5a..920f6cab 100644 --- a/sweep/walletsweep.go +++ b/sweep/walletsweep.go @@ -148,13 +148,24 @@ type WalletSweepPackage struct { CancelSweepAttempt func() } +// DeliveryAddr is a pair of (address, amount) used to craft a transaction +// paying to more than one specified address. +type DeliveryAddr struct { + // Addr is the address to pay to. + Addr btcutil.Address + + // Amt is the amount to pay to the given address. + Amt btcutil.Amount +} + // CraftSweepAllTx attempts to craft a WalletSweepPackage which will allow the -// caller to sweep ALL outputs within the wallet to a single UTXO, as specified -// by the delivery address. The sweep transaction will be crafted with the -// target fee rate, and will use the utxoSource and outpointLocker as sources -// for wallet funds. +// caller to sweep ALL outputs within the wallet to a list of outputs. Any +// leftover amount after these outputs and transaction fee, is sent to a single +// output, as specified by the change address. The sweep transaction will be +// crafted with the target fee rate, and will use the utxoSource and +// outpointLocker as sources for wallet funds. func CraftSweepAllTx(feeRate chainfee.SatPerKWeight, dustLimit btcutil.Amount, - blockHeight uint32, deliveryAddr btcutil.Address, + blockHeight uint32, deliveryAddrs []DeliveryAddr, changeAddr btcutil.Address, coinSelectLocker CoinSelectionLocker, utxoSource UtxoSource, outpointLocker OutpointLocker, feeEstimator chainfee.Estimator, signer input.Signer) (*WalletSweepPackage, error) { @@ -261,9 +272,25 @@ func CraftSweepAllTx(feeRate chainfee.SatPerKWeight, dustLimit btcutil.Amount, inputsToSweep = append(inputsToSweep, &input) } - // Next, we'll convert the delivery addr to a pkScript that we can use + // Create a list of TxOuts from the given delivery addresses. + var txOuts []*wire.TxOut + for _, d := range deliveryAddrs { + pkScript, err := txscript.PayToAddrScript(d.Addr) + if err != nil { + unlockOutputs() + + return nil, err + } + + txOuts = append(txOuts, &wire.TxOut{ + PkScript: pkScript, + Value: int64(d.Amt), + }) + } + + // Next, we'll convert the change addr to a pkScript that we can use // to create the sweep transaction. - deliveryPkScript, err := txscript.PayToAddrScript(deliveryAddr) + changePkScript, err := txscript.PayToAddrScript(changeAddr) if err != nil { unlockOutputs() @@ -273,7 +300,7 @@ func CraftSweepAllTx(feeRate chainfee.SatPerKWeight, dustLimit btcutil.Amount, // Finally, we'll ask the sweeper to craft a sweep transaction which // respects our fee preference and targets all the UTXOs of the wallet. sweepTx, err := createSweepTx( - inputsToSweep, deliveryPkScript, blockHeight, feeRate, + inputsToSweep, txOuts, changePkScript, blockHeight, feeRate, dustLimit, signer, ) if err != nil { diff --git a/sweep/walletsweep_test.go b/sweep/walletsweep_test.go index 032f30c0..dbdab3d1 100644 --- a/sweep/walletsweep_test.go +++ b/sweep/walletsweep_test.go @@ -288,7 +288,8 @@ func TestCraftSweepAllTxCoinSelectFail(t *testing.T) { utxoLocker := newMockOutpointLocker() _, err := CraftSweepAllTx( - 0, 100, 10, nil, coinSelectLocker, utxoSource, utxoLocker, nil, nil, + 0, 100, 10, nil, nil, coinSelectLocker, utxoSource, + utxoLocker, nil, nil, ) // Since we instructed the coin select locker to fail above, we should @@ -313,7 +314,8 @@ func TestCraftSweepAllTxUnknownWitnessType(t *testing.T) { utxoLocker := newMockOutpointLocker() _, err := CraftSweepAllTx( - 0, 100, 10, nil, coinSelectLocker, utxoSource, utxoLocker, nil, nil, + 0, 100, 10, nil, nil, coinSelectLocker, utxoSource, + utxoLocker, nil, nil, ) // Since passed in a p2wsh output, which is unknown, we should fail to @@ -347,8 +349,8 @@ func TestCraftSweepAllTx(t *testing.T) { utxoLocker := newMockOutpointLocker() sweepPkg, err := CraftSweepAllTx( - 0, 100, 10, deliveryAddr, coinSelectLocker, utxoSource, utxoLocker, - feeEstimator, signer, + 0, 100, 10, nil, deliveryAddr, coinSelectLocker, utxoSource, + utxoLocker, feeEstimator, signer, ) if err != nil { t.Fatalf("unable to make sweep tx: %v", err) From 422008e3c05c9b74d591c85677ca5f8497b0e7a1 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Sun, 10 Jan 2021 19:54:49 +0100 Subject: [PATCH 5/6] lnwallet+funding+rpcserver: check reserved value before PublishTransaction For a few manual send cases that can be initiated by the user, we check the reserved value. --- lnwallet/wallet.go | 21 +++++++++++ rpcserver.go | 93 +++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 112 insertions(+), 2 deletions(-) diff --git a/lnwallet/wallet.go b/lnwallet/wallet.go index 82ba1983..e550d1e8 100644 --- a/lnwallet/wallet.go +++ b/lnwallet/wallet.go @@ -986,6 +986,27 @@ func (l *LightningWallet) CheckReservedValue(in []wire.OutPoint, return reserved, nil } +// CheckReservedValueTx calls CheckReservedValue with the inputs and outputs +// from the given tx, with the number of anchor channels currently open in the +// database. +// +// NOTE: This method should only be run with the CoinSelectLock held. +func (l *LightningWallet) CheckReservedValueTx(tx *wire.MsgTx) (btcutil.Amount, + error) { + + numAnchors, err := l.currentNumAnchorChans() + if err != nil { + return 0, err + } + + var inputs []wire.OutPoint + for _, txIn := range tx.TxIn { + inputs = append(inputs, txIn.PreviousOutPoint) + } + + return l.CheckReservedValue(inputs, tx.TxOut, numAnchors) +} + // initOurContribution initializes the given ChannelReservation with our coins // and change reserved for the channel, and derives the keys to use for this // channel. diff --git a/rpcserver.go b/rpcserver.go index 19072226..35a84c7f 100644 --- a/rpcserver.go +++ b/rpcserver.go @@ -1056,7 +1056,25 @@ func (r *rpcServer) sendCoinsOnChain(paymentMap map[string]int64, return nil, err } - tx, err := r.server.cc.Wallet.SendOutputs(outputs, feeRate, minconf, label) + // We first do a dry run, to sanity check we won't spend our wallet + // balance below the reserved amount. + authoredTx, err := r.server.cc.Wallet.CreateSimpleTx( + outputs, feeRate, true, + ) + if err != nil { + return nil, err + } + + _, err = r.server.cc.Wallet.CheckReservedValueTx(authoredTx.Tx) + if err != nil { + return nil, err + } + + // If that checks out, we're failry confident that creating sending to + // these outputs will keep the wallet balance above the reserve. + tx, err := r.server.cc.Wallet.SendOutputs( + outputs, feeRate, minconf, label, + ) if err != nil { return nil, err } @@ -1253,7 +1271,9 @@ func (r *rpcServer) SendCoins(ctx context.Context, // With the sweeper instance created, we can now generate a // transaction that will sweep ALL outputs from the wallet in a // single transaction. This will be generated in a concurrent - // safe manner, so no need to worry about locking. + // safe manner, so no need to worry about locking. The tx will + // pay to the change address created above if we needed to + // reserve any value, the rest will go to targetAddr. sweepTxPkg, err := sweep.CraftSweepAllTx( feePerKw, lnwallet.DefaultDustLimit(), uint32(bestHeight), nil, targetAddr, wallet, @@ -1264,6 +1284,75 @@ func (r *rpcServer) SendCoins(ctx context.Context, return nil, err } + // Before we publish the transaction we make sure it won't + // violate our reserved wallet value. + var reservedVal btcutil.Amount + err = wallet.WithCoinSelectLock(func() error { + var err error + reservedVal, err = wallet.CheckReservedValueTx( + sweepTxPkg.SweepTx, + ) + return err + }) + + // If sending everything to this address would invalidate our + // reserved wallet balance, we create a new sweep tx, where + // we'll send the reserved value back to our wallet. + if err == lnwallet.ErrReservedValueInvalidated { + sweepTxPkg.CancelSweepAttempt() + + rpcsLog.Debugf("Reserved value %v not satisfied after "+ + "send_all, trying with change output", + reservedVal) + + // We'll request a change address from the wallet, + // where we'll send this reserved value back to. This + // ensures this is an address the wallet knows about, + // allowing us to pass the reserved value check. + changeAddr, err := r.server.cc.Wallet.NewAddress( + lnwallet.WitnessPubKey, true, + ) + if err != nil { + return nil, err + } + + // Send the reserved value to this change address, the + // remaining funds will go to the targetAddr. + outputs := []sweep.DeliveryAddr{ + { + Addr: changeAddr, + Amt: reservedVal, + }, + } + + sweepTxPkg, err = sweep.CraftSweepAllTx( + feePerKw, lnwallet.DefaultDustLimit(), + uint32(bestHeight), outputs, targetAddr, wallet, + wallet.WalletController, wallet.WalletController, + r.server.cc.FeeEstimator, r.server.cc.Signer, + ) + if err != nil { + return nil, err + } + + // Sanity check the new tx by re-doing the check. + err = wallet.WithCoinSelectLock(func() error { + _, err := wallet.CheckReservedValueTx( + sweepTxPkg.SweepTx, + ) + return err + }) + if err != nil { + sweepTxPkg.CancelSweepAttempt() + + return nil, err + } + } else if err != nil { + sweepTxPkg.CancelSweepAttempt() + + return nil, err + } + rpcsLog.Debugf("Sweeping all coins from wallet to addr=%v, "+ "with tx=%v", in.Addr, spew.Sdump(sweepTxPkg.SweepTx)) From 8c78f20ffa027186902298fbe067406e22d3b677 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Sun, 10 Jan 2021 19:59:48 +0100 Subject: [PATCH 6/6] itest: add testAnchorReservedValue We test that Alice as expected will not be allowed to spend her wallet balance below the reserved amount. --- lntest/itest/lnd_onchain_test.go | 200 +++++++++++++++++++++++++- lntest/itest/lnd_test_list_on_test.go | 4 + lntest/itest/log_error_whitelist.txt | 3 + 3 files changed, 206 insertions(+), 1 deletion(-) diff --git a/lntest/itest/lnd_onchain_test.go b/lntest/itest/lnd_onchain_test.go index bb03165d..6cc656a7 100644 --- a/lntest/itest/lnd_onchain_test.go +++ b/lntest/itest/lnd_onchain_test.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "fmt" + "strings" "github.com/btcsuite/btcd/txscript" "github.com/btcsuite/btcutil" @@ -11,7 +12,9 @@ import ( "github.com/lightningnetwork/lnd/lnrpc/walletrpc" "github.com/lightningnetwork/lnd/lntest" "github.com/lightningnetwork/lnd/lntest/wait" + "github.com/lightningnetwork/lnd/lnwallet" "github.com/lightningnetwork/lnd/sweep" + "github.com/stretchr/testify/require" ) // testCPFP ensures that the daemon can bump an unconfirmed transaction's fee @@ -22,7 +25,7 @@ func testCPFP(net *lntest.NetworkHarness, t *harnessTest) { // Skip this test for neutrino, as it's not aware of mempool // transactions. if net.BackendCfg.Name() == "neutrino" { - t.Skipf("skipping reorg test for neutrino backend") + t.Skipf("skipping CPFP test for neutrino backend") } // We'll start the test by sending Alice some coins, which she'll use to @@ -160,3 +163,198 @@ func testCPFP(net *lntest.NetworkHarness, t *harnessTest) { t.Fatalf(err.Error()) } } + +// testAnchorReservedValue tests that we won't allow sending transactions when +// that would take the value we reserve for anchor fee bumping out of our +// wallet. +func testAnchorReservedValue(net *lntest.NetworkHarness, t *harnessTest) { + // Start two nodes supporting anchor channels. + args := commitTypeAnchors.Args() + alice, err := net.NewNode("Alice", args) + require.NoError(t.t, err) + + defer shutdownAndAssert(net, t, alice) + + bob, err := net.NewNode("Bob", args) + require.NoError(t.t, err) + + defer shutdownAndAssert(net, t, bob) + + ctxb := context.Background() + ctxt, _ := context.WithTimeout(ctxb, defaultTimeout) + err = net.ConnectNodes(ctxt, alice, bob) + require.NoError(t.t, err) + + // Send just enough coins for Alice to open a channel without a change output. + const ( + chanAmt = 1000000 + feeEst = 8000 + ) + + ctxt, _ = context.WithTimeout(context.Background(), defaultTimeout) + err = net.SendCoins(ctxt, chanAmt+feeEst, alice) + require.NoError(t.t, err) + + // Alice opens a channel that would consume all the funds in her + // wallet, without a change output. This should not be allowed. + resErr := lnwallet.ErrReservedValueInvalidated.Error() + + ctxt, _ = context.WithTimeout(context.Background(), defaultTimeout) + _, err = net.OpenChannel( + ctxt, alice, bob, + lntest.OpenChannelParams{ + Amt: chanAmt, + }, + ) + if err == nil || !strings.Contains(err.Error(), resErr) { + t.Fatalf("expected failure, got: %v", err) + } + + // Alice opens a smaller channel. This works since it will have a + // change output. + ctxt, _ = context.WithTimeout(context.Background(), defaultTimeout) + aliceChanPoint := openChannelAndAssert( + ctxt, t, net, alice, bob, + lntest.OpenChannelParams{ + Amt: chanAmt / 2, + }, + ) + + ctxt, _ = context.WithTimeout(ctxb, defaultTimeout) + err = alice.WaitForNetworkChannelOpen(ctxt, aliceChanPoint) + require.NoError(t.t, err) + + ctxt, _ = context.WithTimeout(ctxb, defaultTimeout) + err = bob.WaitForNetworkChannelOpen(ctxt, aliceChanPoint) + require.NoError(t.t, err) + + // Alice tries to send all coins to an internal address. This is + // allowed, since the final wallet balance will still be above the + // reserved value. + addrReq := &lnrpc.NewAddressRequest{ + Type: lnrpc.AddressType_WITNESS_PUBKEY_HASH, + } + ctxt, _ = context.WithTimeout(ctxb, defaultTimeout) + resp, err := alice.NewAddress(ctxt, addrReq) + require.NoError(t.t, err) + + sweepReq := &lnrpc.SendCoinsRequest{ + Addr: resp.Address, + SendAll: true, + } + ctxt, _ = context.WithTimeout(ctxb, defaultTimeout) + _, err = alice.SendCoins(ctxt, sweepReq) + require.NoError(t.t, err) + + block := mineBlocks(t, net, 1, 1)[0] + + // The sweep transaction should have exactly one input, the change from + // the previous SendCoins call. + sweepTx := block.Transactions[1] + if len(sweepTx.TxIn) != 1 { + t.Fatalf("expected 1 inputs instead have %v", len(sweepTx.TxIn)) + } + + // It should have a single output. + if len(sweepTx.TxOut) != 1 { + t.Fatalf("expected 1 output instead have %v", len(sweepTx.TxOut)) + } + + // Wait for Alice to see her balance as confirmed. + waitForConfirmedBalance := func() int64 { + var balance int64 + err := wait.NoError(func() error { + req := &lnrpc.WalletBalanceRequest{} + ctxt, _ = context.WithTimeout(ctxb, defaultTimeout) + resp, err := alice.WalletBalance(ctxt, req) + if err != nil { + return err + } + + if resp.TotalBalance == 0 { + return fmt.Errorf("no balance") + } + + if resp.UnconfirmedBalance > 0 { + return fmt.Errorf("unconfirmed balance") + } + + balance = resp.TotalBalance + return nil + }, defaultTimeout) + require.NoError(t.t, err) + + return balance + } + + _ = waitForConfirmedBalance() + + // Alice tries to send all funds to an external address, the reserved + // value must stay in her wallet. + minerAddr, err := net.Miner.NewAddress() + require.NoError(t.t, err) + + sweepReq = &lnrpc.SendCoinsRequest{ + Addr: minerAddr.String(), + SendAll: true, + } + ctxt, _ = context.WithTimeout(ctxb, defaultTimeout) + _, err = alice.SendCoins(ctxt, sweepReq) + require.NoError(t.t, err) + + // We'll mine a block which should include the sweep transaction we + // generated above. + block = mineBlocks(t, net, 1, 1)[0] + + // The sweep transaction should have exactly one inputs as we only had + // the the single output from above in the wallet. + sweepTx = block.Transactions[1] + if len(sweepTx.TxIn) != 1 { + t.Fatalf("expected 1 inputs instead have %v", len(sweepTx.TxIn)) + } + + // It should have two outputs, one being the miner address, the other + // one being the reserve going back to our wallet. + if len(sweepTx.TxOut) != 2 { + t.Fatalf("expected 2 outputs instead have %v", len(sweepTx.TxOut)) + } + + // The reserved value is now back in Alice's wallet. + aliceBalance := waitForConfirmedBalance() + + // Alice closes channel, should now be allowed to send everything to an + // external address. + closeChannelAndAssert(ctxt, t, net, alice, aliceChanPoint, false) + + newBalance := waitForConfirmedBalance() + if newBalance <= aliceBalance { + t.Fatalf("Alice's balance did not increase after channel close") + } + + // We'll wait for the balance to reflect that the channel has been + // closed and the funds are in the wallet. + + sweepReq = &lnrpc.SendCoinsRequest{ + Addr: minerAddr.String(), + SendAll: true, + } + ctxt, _ = context.WithTimeout(ctxb, defaultTimeout) + _, err = alice.SendCoins(ctxt, sweepReq) + require.NoError(t.t, err) + + // We'll mine a block which should include the sweep transaction we + // generated above. + block = mineBlocks(t, net, 1, 1)[0] + + // The sweep transaction should have two inputs, the change output from + // the previous sweep, and the output from the coop closed channel. + sweepTx = block.Transactions[1] + if len(sweepTx.TxIn) != 2 { + t.Fatalf("expected 2 inputs instead have %v", len(sweepTx.TxIn)) + } + + // It should have a single output. + if len(sweepTx.TxOut) != 1 { + t.Fatalf("expected 1 output instead have %v", len(sweepTx.TxOut)) + } +} diff --git a/lntest/itest/lnd_test_list_on_test.go b/lntest/itest/lnd_test_list_on_test.go index cbbc8b37..7f8404f7 100644 --- a/lntest/itest/lnd_test_list_on_test.go +++ b/lntest/itest/lnd_test_list_on_test.go @@ -230,6 +230,10 @@ var allTestCases = []*testCase{ name: "cpfp", test: testCPFP, }, + { + name: "anchors reserved value", + test: testAnchorReservedValue, + }, { name: "macaroon authentication", test: testMacaroonAuthentication, diff --git a/lntest/itest/log_error_whitelist.txt b/lntest/itest/log_error_whitelist.txt index 71398c92..a8ac3398 100644 --- a/lntest/itest/log_error_whitelist.txt +++ b/lntest/itest/log_error_whitelist.txt @@ -251,3 +251,6 @@