From 23cd2f40eb05e919a0c20a077d9af1a4f5047b95 Mon Sep 17 00:00:00 2001 From: Joost Jager Date: Mon, 14 Sep 2020 11:24:12 +0200 Subject: [PATCH 1/8] lnwallet: remove unused WebAPIEstimator field --- chainregistry.go | 1 - lnwallet/chainfee/estimator.go | 9 +-------- lnwallet/chainfee/estimator_test.go | 2 +- 3 files changed, 2 insertions(+), 10 deletions(-) diff --git a/chainregistry.go b/chainregistry.go index f5d4ac79..23a2f0f4 100644 --- a/chainregistry.go +++ b/chainregistry.go @@ -257,7 +257,6 @@ func newChainControlFromConfig(cfg *Config, localDB, remoteDB *channeldb.DB, chainfee.SparseConfFeeSource{ URL: cfg.NeutrinoMode.FeeURL, }, - defaultBitcoinStaticFeePerKW, ) if err := estimator.Start(); err != nil { diff --git a/lnwallet/chainfee/estimator.go b/lnwallet/chainfee/estimator.go index 98c74f61..13a04d76 100644 --- a/lnwallet/chainfee/estimator.go +++ b/lnwallet/chainfee/estimator.go @@ -540,23 +540,16 @@ type WebAPIEstimator struct { feesMtx sync.Mutex feeByBlockTarget map[uint32]uint32 - // defaultFeePerKw is a fallback value that we'll use if we're unable - // to query the API for any reason. - defaultFeePerKw SatPerKWeight - quit chan struct{} wg sync.WaitGroup } // NewWebAPIEstimator creates a new WebAPIEstimator from a given URL and a // fallback default fee. The fees are updated whenever a new block is mined. -func NewWebAPIEstimator( - api WebAPIFeeSource, defaultFee SatPerKWeight) *WebAPIEstimator { - +func NewWebAPIEstimator(api WebAPIFeeSource) *WebAPIEstimator { return &WebAPIEstimator{ apiSource: api, feeByBlockTarget: make(map[uint32]uint32), - defaultFeePerKw: defaultFee, quit: make(chan struct{}), } } diff --git a/lnwallet/chainfee/estimator_test.go b/lnwallet/chainfee/estimator_test.go index b8b8186f..d99424bc 100644 --- a/lnwallet/chainfee/estimator_test.go +++ b/lnwallet/chainfee/estimator_test.go @@ -192,7 +192,7 @@ func TestWebAPIFeeEstimator(t *testing.T) { fees: testFees, } - estimator := NewWebAPIEstimator(feeSource, 10) + estimator := NewWebAPIEstimator(feeSource) // Test that requesting a fee when no fees have been cached fails. _, err := estimator.EstimateFeePerKW(5) From 166be979dd96d51cc0bcfa79a5c61ca8ae2924e7 Mon Sep 17 00:00:00 2001 From: Joost Jager Date: Thu, 10 Sep 2020 09:05:54 +0200 Subject: [PATCH 2/8] lnwallet: add no cache option to web api estimator --- chainregistry.go | 1 + lnwallet/chainfee/estimator.go | 22 +++++++++++++++++++++- lnwallet/chainfee/estimator_test.go | 2 +- 3 files changed, 23 insertions(+), 2 deletions(-) diff --git a/chainregistry.go b/chainregistry.go index 23a2f0f4..15b421be 100644 --- a/chainregistry.go +++ b/chainregistry.go @@ -257,6 +257,7 @@ func newChainControlFromConfig(cfg *Config, localDB, remoteDB *channeldb.DB, chainfee.SparseConfFeeSource{ URL: cfg.NeutrinoMode.FeeURL, }, + false, ) if err := estimator.Start(); err != nil { diff --git a/lnwallet/chainfee/estimator.go b/lnwallet/chainfee/estimator.go index 13a04d76..341ee919 100644 --- a/lnwallet/chainfee/estimator.go +++ b/lnwallet/chainfee/estimator.go @@ -540,16 +540,21 @@ type WebAPIEstimator struct { feesMtx sync.Mutex feeByBlockTarget map[uint32]uint32 + // noCache determines whether the web estimator should cache fee + // estimates. + noCache bool + quit chan struct{} wg sync.WaitGroup } // NewWebAPIEstimator creates a new WebAPIEstimator from a given URL and a // fallback default fee. The fees are updated whenever a new block is mined. -func NewWebAPIEstimator(api WebAPIFeeSource) *WebAPIEstimator { +func NewWebAPIEstimator(api WebAPIFeeSource, noCache bool) *WebAPIEstimator { return &WebAPIEstimator{ apiSource: api, feeByBlockTarget: make(map[uint32]uint32), + noCache: noCache, quit: make(chan struct{}), } } @@ -566,6 +571,11 @@ func (w *WebAPIEstimator) EstimateFeePerKW(numBlocks uint32) (SatPerKWeight, err "accepted is %v", numBlocks, minBlockTarget) } + // Get fee estimates now if we don't refresh periodically. + if w.noCache { + w.updateFeeEstimates() + } + feePerKb, err := w.getCachedFee(numBlocks) if err != nil { return 0, err @@ -589,6 +599,11 @@ func (w *WebAPIEstimator) EstimateFeePerKW(numBlocks uint32) (SatPerKWeight, err // // NOTE: This method is part of the Estimator interface. func (w *WebAPIEstimator) Start() error { + // No update loop is needed when we don't cache. + if w.noCache { + return nil + } + var err error w.started.Do(func() { log.Infof("Starting web API fee estimator") @@ -608,6 +623,11 @@ func (w *WebAPIEstimator) Start() error { // // NOTE: This method is part of the Estimator interface. func (w *WebAPIEstimator) Stop() error { + // Update loop is not running when we don't cache. + if w.noCache { + return nil + } + w.stopped.Do(func() { log.Infof("Stopping web API fee estimator") diff --git a/lnwallet/chainfee/estimator_test.go b/lnwallet/chainfee/estimator_test.go index d99424bc..f98ae80c 100644 --- a/lnwallet/chainfee/estimator_test.go +++ b/lnwallet/chainfee/estimator_test.go @@ -192,7 +192,7 @@ func TestWebAPIFeeEstimator(t *testing.T) { fees: testFees, } - estimator := NewWebAPIEstimator(feeSource) + estimator := NewWebAPIEstimator(feeSource, false) // Test that requesting a fee when no fees have been cached fails. _, err := estimator.EstimateFeePerKW(5) From dae0e2194dfb4824d4d2de996d200bee58f31b11 Mon Sep 17 00:00:00 2001 From: Joost Jager Date: Mon, 14 Sep 2020 11:43:10 +0200 Subject: [PATCH 3/8] lnd: start fee estimator at a single point --- chainregistry.go | 21 ++++++--------------- 1 file changed, 6 insertions(+), 15 deletions(-) diff --git a/chainregistry.go b/chainregistry.go index 15b421be..66f7b3ce 100644 --- a/chainregistry.go +++ b/chainregistry.go @@ -253,17 +253,12 @@ func newChainControlFromConfig(cfg *Config, localDB, remoteDB *channeldb.DB, if cfg.NeutrinoMode.FeeURL != "" { ltndLog.Infof("Using API fee estimator!") - estimator := chainfee.NewWebAPIEstimator( + cc.feeEstimator = chainfee.NewWebAPIEstimator( chainfee.SparseConfFeeSource{ URL: cfg.NeutrinoMode.FeeURL, }, false, ) - - if err := estimator.Start(); err != nil { - return nil, err - } - cc.feeEstimator = estimator } walletConfig.ChainSource = chain.NewNeutrinoClient( @@ -366,9 +361,6 @@ func newChainControlFromConfig(cfg *Config, localDB, remoteDB *channeldb.DB, if err != nil { return nil, err } - if err := cc.feeEstimator.Start(); err != nil { - return nil, err - } } else if cfg.Litecoin.Active && !cfg.Litecoin.RegTest { ltndLog.Infof("Initializing litecoind backed fee estimator in "+ "%s mode", bitcoindMode.EstimateMode) @@ -385,9 +377,6 @@ func newChainControlFromConfig(cfg *Config, localDB, remoteDB *channeldb.DB, if err != nil { return nil, err } - if err := cc.feeEstimator.Start(); err != nil { - return nil, err - } } case "btcd", "ltcd": // Otherwise, we'll be speaking directly via RPC to a node. @@ -490,15 +479,17 @@ func newChainControlFromConfig(cfg *Config, localDB, remoteDB *channeldb.DB, if err != nil { return nil, err } - if err := cc.feeEstimator.Start(); err != nil { - return nil, err - } } default: return nil, fmt.Errorf("unknown node type: %s", homeChainConfig.Node) } + // Start fee estimator. + if err := cc.feeEstimator.Start(); err != nil { + return nil, err + } + wc, err := btcwallet.New(*walletConfig) if err != nil { fmt.Printf("unable to create wallet controller: %v\n", err) From fc3fd26a3da3825f4eca08aa47205324fc7bb6dd Mon Sep 17 00:00:00 2001 From: Joost Jager Date: Thu, 10 Sep 2020 09:43:35 +0200 Subject: [PATCH 4/8] config: allow web fee estimation on regtest --- chainregistry.go | 33 +++++++++++++++++++++++++-------- config.go | 2 ++ lncfg/neutrino.go | 2 +- 3 files changed, 28 insertions(+), 9 deletions(-) diff --git a/chainregistry.go b/chainregistry.go index 66f7b3ce..6b864652 100644 --- a/chainregistry.go +++ b/chainregistry.go @@ -2,6 +2,7 @@ package lnd import ( "encoding/hex" + "errors" "fmt" "io/ioutil" "net" @@ -249,16 +250,15 @@ func newChainControlFromConfig(cfg *Config, localDB, remoteDB *channeldb.DB, return nil, err } - // If the user provided an API for fee estimation, activate it now. + // Map the deprecated neutrino feeurl flag to the general fee + // url. if cfg.NeutrinoMode.FeeURL != "" { - ltndLog.Infof("Using API fee estimator!") + if cfg.FeeURL != "" { + return nil, errors.New("feeurl and " + + "neutrino.feeurl are mutually exclusive") + } - cc.feeEstimator = chainfee.NewWebAPIEstimator( - chainfee.SparseConfFeeSource{ - URL: cfg.NeutrinoMode.FeeURL, - }, - false, - ) + cfg.FeeURL = cfg.NeutrinoMode.FeeURL } walletConfig.ChainSource = chain.NewNeutrinoClient( @@ -485,6 +485,23 @@ func newChainControlFromConfig(cfg *Config, localDB, remoteDB *channeldb.DB, homeChainConfig.Node) } + // Override default fee estimator if an external service is specified. + if cfg.FeeURL != "" { + // Do not cache fees on regtest to make it easier to execute + // manual or automated test cases. + cacheFees := !cfg.Bitcoin.RegTest + + ltndLog.Infof("Using external fee estimator %v: cached=%v", + cfg.FeeURL, cacheFees) + + cc.feeEstimator = chainfee.NewWebAPIEstimator( + chainfee.SparseConfFeeSource{ + URL: cfg.FeeURL, + }, + !cacheFees, + ) + } + // Start fee estimator. if err := cc.feeEstimator.Start(); err != nil { return nil, err diff --git a/config.go b/config.go index 7df5adcd..0a30773f 100644 --- a/config.go +++ b/config.go @@ -210,6 +210,8 @@ type Config struct { MaxPendingChannels int `long:"maxpendingchannels" description:"The maximum number of incoming pending channels permitted per peer."` BackupFilePath string `long:"backupfilepath" description:"The target location of the channel backup file"` + FeeURL string `long:"feeurl" description:"Optional URL for external fee estimation. If no URL is specified, the method for fee estimation will depend on the chosen backend and network."` + Bitcoin *lncfg.Chain `group:"Bitcoin" namespace:"bitcoin"` BtcdMode *lncfg.Btcd `group:"btcd" namespace:"btcd"` BitcoindMode *lncfg.Bitcoind `group:"bitcoind" namespace:"bitcoind"` diff --git a/lncfg/neutrino.go b/lncfg/neutrino.go index b6f892bf..db4c3d4d 100644 --- a/lncfg/neutrino.go +++ b/lncfg/neutrino.go @@ -10,6 +10,6 @@ type Neutrino struct { MaxPeers int `long:"maxpeers" description:"Max number of inbound and outbound peers"` BanDuration time.Duration `long:"banduration" description:"How long to ban misbehaving peers. Valid time units are {s, m, h}. Minimum 1 second"` BanThreshold uint32 `long:"banthreshold" description:"Maximum allowed ban score before disconnecting and banning misbehaving peers."` - FeeURL string `long:"feeurl" description:"Optional URL for fee estimation. If a URL is not specified, static fees will be used for estimation."` + FeeURL string `long:"feeurl" description:"DEPRECATED: Optional URL for fee estimation. If a URL is not specified, static fees will be used for estimation."` AssertFilterHeader string `long:"assertfilterheader" description:"Optional filter header in height:hash format to assert the state of neutrino's filter header chain on startup. If the assertion does not hold, then the filter header chain will be re-synced from the genesis block."` } From b6ebf3f27d8a926c103b64227671652196d19f6a Mon Sep 17 00:00:00 2001 From: Joost Jager Date: Thu, 10 Sep 2020 15:48:39 +0200 Subject: [PATCH 5/8] lntest: use web fee estimator in itests --- lntest/fee_service.go | 106 +++++++++++++++++++++++++++++++++++++ lntest/fee_service_test.go | 39 ++++++++++++++ lntest/harness.go | 15 ++++++ lntest/node.go | 6 +++ 4 files changed, 166 insertions(+) create mode 100644 lntest/fee_service.go create mode 100644 lntest/fee_service_test.go diff --git a/lntest/fee_service.go b/lntest/fee_service.go new file mode 100644 index 00000000..68e7d435 --- /dev/null +++ b/lntest/fee_service.go @@ -0,0 +1,106 @@ +package lntest + +import ( + "context" + "encoding/json" + "fmt" + "io" + "net/http" + "sync" + + "github.com/lightningnetwork/lnd/lnwallet/chainfee" +) + +const ( + // feeServiceTarget is the confirmation target for which a fee estimate + // is returned. Requests for higher confirmation targets will fall back + // to this. + feeServiceTarget = 2 + + // feeServicePort is the tcp port on which the service runs. + feeServicePort = 16534 +) + +// feeService runs a web service that provides fee estimation information. +type feeService struct { + feeEstimates + + srv *http.Server + wg sync.WaitGroup + + url string + + lock sync.Mutex +} + +// feeEstimates contains the current fee estimates. +type feeEstimates struct { + Fees map[uint32]uint32 `json:"fee_by_block_target"` +} + +// startFeeService spins up a go-routine to serve fee estimates. +func startFeeService() *feeService { + f := feeService{ + url: fmt.Sprintf( + "http://localhost:%v/fee-estimates.json", feeServicePort, + ), + } + + // Initialize default fee estimate. + f.Fees = map[uint32]uint32{feeServiceTarget: 50000} + + listenAddr := fmt.Sprintf(":%v", feeServicePort) + f.srv = &http.Server{ + Addr: listenAddr, + } + + http.HandleFunc("/fee-estimates.json", f.handleRequest) + + f.wg.Add(1) + go func() { + defer f.wg.Done() + + if err := f.srv.ListenAndServe(); err != http.ErrServerClosed { + fmt.Printf("error: cannot start fee api: %v", err) + } + }() + + return &f +} + +// handleRequest handles a client request for fee estimates. +func (f *feeService) handleRequest(w http.ResponseWriter, r *http.Request) { + f.lock.Lock() + defer f.lock.Unlock() + + bytes, err := json.Marshal(f.feeEstimates) + if err != nil { + fmt.Printf("error: cannot serialize "+ + "estimates: %v", err) + + return + } + + _, err = io.WriteString(w, string(bytes)) + if err != nil { + fmt.Printf("error: cannot send estimates: %v", + err) + } +} + +// stop stops the web server. +func (f *feeService) stop() { + if err := f.srv.Shutdown(context.Background()); err != nil { + fmt.Printf("error: cannot stop fee api: %v", err) + } + + f.wg.Wait() +} + +// setFee changes the current fee estimate for the fixed confirmation target. +func (f *feeService) setFee(fee chainfee.SatPerKWeight) { + f.lock.Lock() + defer f.lock.Unlock() + + f.Fees[feeServiceTarget] = uint32(fee.FeePerKVByte()) +} diff --git a/lntest/fee_service_test.go b/lntest/fee_service_test.go new file mode 100644 index 00000000..c7ad38c4 --- /dev/null +++ b/lntest/fee_service_test.go @@ -0,0 +1,39 @@ +package lntest + +import ( + "io/ioutil" + "net/http" + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +// TestFeeService tests the itest fee estimating web service. +func TestFeeService(t *testing.T) { + service := startFeeService() + defer service.stop() + + service.setFee(5000) + + // Wait for service to start accepting connections. + var resp *http.Response + require.Eventually( + t, + func() bool { + var err error + resp, err = http.Get(service.url) // nolint:bodyclose + return err == nil + }, + 10*time.Second, time.Second, + ) + + defer resp.Body.Close() + + body, err := ioutil.ReadAll(resp.Body) + require.NoError(t, err) + + require.Equal( + t, "{\"fee_by_block_target\":{\"2\":20000}}", string(body), + ) +} diff --git a/lntest/harness.go b/lntest/harness.go index d680f9be..f8c0b7fc 100644 --- a/lntest/harness.go +++ b/lntest/harness.go @@ -22,6 +22,7 @@ import ( "github.com/lightningnetwork/lnd" "github.com/lightningnetwork/lnd/lnrpc" "github.com/lightningnetwork/lnd/lntest/wait" + "github.com/lightningnetwork/lnd/lnwallet/chainfee" "github.com/lightningnetwork/lnd/lnwire" "google.golang.org/grpc/grpclog" ) @@ -63,6 +64,10 @@ type NetworkHarness struct { // to main process. lndErrorChan chan error + // feeService is a web service that provides external fee estimates to + // lnd. + feeService *feeService + quit chan struct{} mtx sync.Mutex @@ -75,6 +80,8 @@ type NetworkHarness struct { func NewNetworkHarness(r *rpctest.Harness, b BackendConfig, lndBinary string) ( *NetworkHarness, error) { + feeService := startFeeService() + n := NetworkHarness{ activeNodes: make(map[int]*HarnessNode), nodesByPub: make(map[string]*HarnessNode), @@ -84,6 +91,7 @@ func NewNetworkHarness(r *rpctest.Harness, b BackendConfig, lndBinary string) ( netParams: r.ActiveNet, Miner: r, BackendCfg: b, + feeService: feeService, quit: make(chan struct{}), lndBinary: lndBinary, } @@ -251,6 +259,8 @@ func (n *NetworkHarness) TearDownAll() error { close(n.lndErrorChan) close(n.quit) + n.feeService.stop() + return nil } @@ -358,6 +368,7 @@ func (n *NetworkHarness) newNode(name string, extraArgs []string, BackendCfg: n.BackendCfg, NetParams: n.netParams, ExtraArgs: extraArgs, + FeeURL: n.feeService.url, }) if err != nil { return nil, err @@ -1404,6 +1415,10 @@ func (n *NetworkHarness) sendCoins(ctx context.Context, amt btcutil.Amount, return target.WaitForBalance(expectedBalance, true) } +func (n *NetworkHarness) SetFeeEstimate(fee chainfee.SatPerKWeight) { + n.feeService.setFee(fee) +} + // CopyFile copies the file src to dest. func CopyFile(dest, src string) error { s, err := os.Open(src) diff --git a/lntest/node.go b/lntest/node.go index 83d23b4c..222c4fe0 100644 --- a/lntest/node.go +++ b/lntest/node.go @@ -154,6 +154,8 @@ type NodeConfig struct { ProfilePort int AcceptKeySend bool + + FeeURL string } func (cfg NodeConfig) P2PAddr() string { @@ -232,6 +234,10 @@ func (cfg NodeConfig) genArgs() []string { args = append(args, "--accept-keysend") } + if cfg.FeeURL != "" { + args = append(args, "--feeurl="+cfg.FeeURL) + } + return args } From cdbde5dd4c186bcbadd9272cfa892deb8176f105 Mon Sep 17 00:00:00 2001 From: Joost Jager Date: Mon, 7 Sep 2020 13:29:28 +0200 Subject: [PATCH 6/8] sweep: do not combine exclusive and non-exclusive inputs --- sweep/bucket_list.go | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/sweep/bucket_list.go b/sweep/bucket_list.go index 4b3c67cd..12361565 100644 --- a/sweep/bucket_list.go +++ b/sweep/bucket_list.go @@ -9,9 +9,20 @@ func (b bucket) tryAdd(input *pendingInput) bool { if exclusiveGroup != nil { for _, input := range b { existingGroup := input.params.ExclusiveGroup - if existingGroup != nil && - *existingGroup == *exclusiveGroup { + // Don't add an exclusive group input if other inputs + // are non-exclusive. The exclusive group input may be + // invalid (for example in the case of commitment + // anchors) and could thereby block sweeping of the + // other inputs. + if existingGroup == nil { + return false + } + + // Don't combine inputs from the same exclusive group. + // Because only one input is valid, this may result in + // txes that are always invalid. + if *existingGroup == *exclusiveGroup { return false } } From 6df4fa84df29d2f402aac575c82e8e21ccfeab2c Mon Sep 17 00:00:00 2001 From: Joost Jager Date: Fri, 4 Sep 2020 13:04:18 +0200 Subject: [PATCH 7/8] sweep: clean up state mutation The add function tries to add an input to the current set. It therefore calculates what the new set would look like before actually adding. This commit isolates the state of the tentative set so that there is less opportunity for bugs to creep in. --- sweep/tx_input_set.go | 118 +++++++++++++++++++++++++----------------- 1 file changed, 71 insertions(+), 47 deletions(-) diff --git a/sweep/tx_input_set.go b/sweep/tx_input_set.go index 1f21f960..d2ce6e5a 100644 --- a/sweep/tx_input_set.go +++ b/sweep/tx_input_set.go @@ -30,9 +30,7 @@ const ( constraintsForce ) -// txInputSet is an object that accumulates tx inputs and keeps running counters -// on various properties of the tx. -type txInputSet struct { +type txInputSetState struct { // weightEstimate is the (worst case) tx weight with the current set of // inputs. weightEstimate input.TxWeightEstimator @@ -43,12 +41,39 @@ type txInputSet struct { // outputValue is the value of the tx output. outputValue btcutil.Amount - // feePerKW is the fee rate used to calculate the tx fee. - feePerKW chainfee.SatPerKWeight - // inputs is the set of tx inputs. inputs []input.Input + // walletInputTotal is the total value of inputs coming from the wallet. + walletInputTotal btcutil.Amount + + // force indicates that this set must be swept even if the total yield + // is negative. + force bool +} + +func (t *txInputSetState) clone() txInputSetState { + s := txInputSetState{ + weightEstimate: t.weightEstimate, + inputTotal: t.inputTotal, + outputValue: t.outputValue, + walletInputTotal: t.walletInputTotal, + force: t.force, + inputs: make([]input.Input, len(t.inputs)), + } + copy(s.inputs, t.inputs) + + return s +} + +// txInputSet is an object that accumulates tx inputs and keeps running counters +// on various properties of the tx. +type txInputSet struct { + txInputSetState + + // feePerKW is the fee rate used to calculate the tx fee. + feePerKW chainfee.SatPerKWeight + // dustLimit is the minimum output value of the tx. dustLimit btcutil.Amount @@ -56,16 +81,9 @@ type txInputSet struct { // the set. maxInputs int - // walletInputTotal is the total value of inputs coming from the wallet. - walletInputTotal btcutil.Amount - // wallet contains wallet functionality required by the input set to // retrieve utxos. wallet Wallet - - // force indicates that this set must be swept even if the total yield - // is negative. - force bool } // newTxInputSet constructs a new, empty input set. @@ -99,56 +117,57 @@ func (t *txInputSet) dustLimitReached() bool { // add adds a new input to the set. It returns a bool indicating whether the // input was added to the set. An input is rejected if it decreases the tx // output value after paying fees. -func (t *txInputSet) add(input input.Input, constraints addConstraints) bool { +func (t *txInputSet) addToState(inp input.Input, constraints addConstraints) *txInputSetState { // Stop if max inputs is reached. Do not count additional wallet inputs, // because we don't know in advance how many we may need. if constraints != constraintsWallet && len(t.inputs) >= t.maxInputs { - return false + return nil } // Can ignore error, because it has already been checked when // calculating the yields. - size, isNestedP2SH, _ := input.WitnessType().SizeUpperBound() + size, isNestedP2SH, _ := inp.WitnessType().SizeUpperBound() - // Add weight of this new candidate input to a copy of the weight - // estimator. - newWeightEstimate := t.weightEstimate + // Clone the current set state. + s := t.clone() + + // Add the new input. + s.inputs = append(s.inputs, inp) + + // Add weight of the new input. if isNestedP2SH { - newWeightEstimate.AddNestedP2WSHInput(size) + s.weightEstimate.AddNestedP2WSHInput(size) } else { - newWeightEstimate.AddWitnessInput(size) + s.weightEstimate.AddWitnessInput(size) } - value := btcutil.Amount(input.SignDesc().Output.Value) - newInputTotal := t.inputTotal + value + // Add the value of the new input. + value := btcutil.Amount(inp.SignDesc().Output.Value) + s.inputTotal += value - weight := newWeightEstimate.Weight() + // Recalculate the tx fee. + weight := s.weightEstimate.Weight() fee := t.feePerKW.FeeForWeight(int64(weight)) - // Calculate the output value if the current input would be - // added to the set. - newOutputValue := newInputTotal - fee - - // Initialize new wallet total with the current wallet total. This is - // updated below if this input is a wallet input. - newWalletTotal := t.walletInputTotal + // Calculate the new output value. + s.outputValue = s.inputTotal - fee // Calculate the yield of this input from the change in tx output value. - inputYield := newOutputValue - t.outputValue + inputYield := s.outputValue - t.outputValue switch constraints { // Don't sweep inputs that cost us more to sweep than they give us. case constraintsRegular: if inputYield <= 0 { - return false + return nil } // For force adds, no further constraints apply. case constraintsForce: - t.force = true + s.force = true // We are attaching a wallet input to raise the tx output value above // the dust limit. @@ -156,12 +175,12 @@ func (t *txInputSet) add(input input.Input, constraints addConstraints) bool { // Skip this wallet input if adding it would lower the output // value. if inputYield <= 0 { - return false + return nil } // Calculate the total value that we spend in this tx from the // wallet if we'd add this wallet input. - newWalletTotal += value + s.walletInputTotal += value // In any case, we don't want to lose money by sweeping. If we // don't get more out of the tx then we put in ourselves, do not @@ -176,24 +195,29 @@ func (t *txInputSet) add(input input.Input, constraints addConstraints) bool { // value of the wallet input and what we get out of this // transaction. To prevent attaching and locking a big utxo for // very little benefit. - if !t.force && newWalletTotal >= newOutputValue { + if !s.force && s.walletInputTotal >= s.outputValue { log.Debugf("Rejecting wallet input of %v, because it "+ "would make a negative yielding transaction "+ "(%v)", - value, newOutputValue-newWalletTotal) + value, s.outputValue-s.walletInputTotal) - return false + return nil } } - // Update running values. - // - // TODO: Return new instance? - t.inputTotal = newInputTotal - t.outputValue = newOutputValue - t.inputs = append(t.inputs, input) - t.weightEstimate = newWeightEstimate - t.walletInputTotal = newWalletTotal + return &s +} + +// add adds a new input to the set. It returns a bool indicating whether the +// input was added to the set. An input is rejected if it decreases the tx +// output value after paying fees. +func (t *txInputSet) add(input input.Input, constraints addConstraints) bool { + newState := t.addToState(input, constraints) + if newState == nil { + return false + } + + t.txInputSetState = *newState return true } From 08bb8ec54eb2458c22ebf16f8263eb563cb64197 Mon Sep 17 00:00:00 2001 From: Joost Jager Date: Fri, 4 Sep 2020 11:53:24 +0200 Subject: [PATCH 8/8] cnct: clear exclusive group on anchor sweep after confirmation The sweeper call UpdateParams does not update the exclusive group property of a pending sweep. This led to anchor outputs being swept after confirmation with an exclusive group restriction, which is not necessary. This commit changes the anchor resolver to not use UpdateParams anymore, but instead always re-offer the anchor input to the sweeper. The sweeper is modified so that a re-offering also updates the sweep parameters. --- contractcourt/anchor_resolver.go | 48 ++++++++++-------------- contractcourt/channel_arbitrator_test.go | 4 +- sweep/sweeper.go | 8 +++- 3 files changed, 27 insertions(+), 33 deletions(-) diff --git a/contractcourt/anchor_resolver.go b/contractcourt/anchor_resolver.go index c67265c7..bc9b251d 100644 --- a/contractcourt/anchor_resolver.go +++ b/contractcourt/anchor_resolver.go @@ -10,7 +10,6 @@ import ( "github.com/btcsuite/btcutil" "github.com/lightningnetwork/lnd/channeldb" "github.com/lightningnetwork/lnd/input" - "github.com/lightningnetwork/lnd/lnwallet" "github.com/lightningnetwork/lnd/sweep" ) @@ -86,41 +85,32 @@ func (c *anchorResolver) Resolve() (ContractResolver, error) { // situation. We don't want to force sweep anymore, because the anchor // lost its special purpose to get the commitment confirmed. It is just // an output that we want to sweep only if it is economical to do so. + // + // An exclusive group is not necessary anymore, because we know that + // this is the only anchor that can be swept. + // + // After a restart or when the remote force closes, the sweeper is not + // yet aware of the anchor. In that case, it will be added as new input + // to the sweeper. relayFeeRate := c.Sweeper.RelayFeePerKW() - resultChan, err := c.Sweeper.UpdateParams( - c.anchor, - sweep.ParamsUpdate{ + anchorInput := input.MakeBaseInput( + &c.anchor, + input.CommitmentAnchor, + &c.anchorSignDescriptor, + c.broadcastHeight, + ) + + resultChan, err := c.Sweeper.SweepInput( + &anchorInput, + sweep.Params{ Fee: sweep.FeePreference{ FeeRate: relayFeeRate, }, - Force: false, }, ) - - // After a restart or when the remote force closes, the sweeper is not - // yet aware of the anchor. In that case, offer it as a new input to the - // sweeper. An exclusive group is not necessary anymore, because we know - // that this is the only anchor that can be swept. - if err == lnwallet.ErrNotMine { - anchorInput := input.MakeBaseInput( - &c.anchor, - input.CommitmentAnchor, - &c.anchorSignDescriptor, - c.broadcastHeight, - ) - - resultChan, err = c.Sweeper.SweepInput( - &anchorInput, - sweep.Params{ - Fee: sweep.FeePreference{ - FeeRate: relayFeeRate, - }, - }, - ) - if err != nil { - return nil, err - } + if err != nil { + return nil, err } var ( diff --git a/contractcourt/channel_arbitrator_test.go b/contractcourt/channel_arbitrator_test.go index 1cdb8142..d3c85f26 100644 --- a/contractcourt/channel_arbitrator_test.go +++ b/contractcourt/channel_arbitrator_test.go @@ -2237,9 +2237,9 @@ func TestChannelArbitratorAnchors(t *testing.T) { t.Fatalf("expected anchor resolver, got %T", resolver) } - // The anchor resolver is expected to offer the anchor input to the + // The anchor resolver is expected to re-offer the anchor input to the // sweeper. - <-chanArbCtx.sweeper.updatedInputs + <-chanArbCtx.sweeper.sweptInputs // The mock sweeper immediately signals success for that input. This // should transition the channel to the resolved state. diff --git a/sweep/sweeper.go b/sweep/sweeper.go index 43f2a0d8..68698f5c 100644 --- a/sweep/sweeper.go +++ b/sweep/sweeper.go @@ -505,6 +505,9 @@ func (s *UtxoSweeper) collector(blockEpochs <-chan *chainntnfs.BlockEpoch) { log.Debugf("Already pending input %v received", outpoint) + // Update sweep parameters. + pendInput.params = input.params + // Add additional result channel to signal // spend of this input. pendInput.listeners = append( @@ -1131,8 +1134,9 @@ func (s *UtxoSweeper) handlePendingSweepsReq( // UpdateParams allows updating the sweep parameters of a pending input in the // UtxoSweeper. This function can be used to provide an updated fee preference -// that will be used for a new sweep transaction of the input that will act as a -// replacement transaction (RBF) of the original sweeping transaction, if any. +// and force flag that will be used for a new sweep transaction of the input +// that will act as a replacement transaction (RBF) of the original sweeping +// transaction, if any. The exclusive group is left unchanged. // // NOTE: This currently doesn't do any fee rate validation to ensure that a bump // is actually successful. The responsibility of doing so should be handled by