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.

Sign up for GitHub

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

[2/5]sweep: refactor fee estimation and introduce UtxoAggregator #8422

Merged
merged 9 commits into from
Mar 18, 2024
Merged

[2/5]sweep: refactor fee estimation and introduce UtxoAggregator #8422

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
c3f9c28
sweep+contractcourt: track best height in `UtxoSweeper`
yyforyongyu Oct 23, 2023
9745a1c
sweep: replace `feeRateForPreference` with `Estimate`
yyforyongyu Oct 24, 2023
aedf971
multi: merge `DetermineFeePerKw` and `Estimate`
yyforyongyu Nov 3, 2023
38985ad
multi: rename `FeePreference` to `FeeEstimateInfo`
yyforyongyu Nov 3, 2023
32a2ca4
sweep+lnrpc: add new interface `FeePreference`
yyforyongyu Nov 3, 2023
b5f58ee
sweep+lnd: introduce `UtxoAggregator` to handle clustering inputs
yyforyongyu Oct 24, 2023
b249451
lntest: reflect the updated fee estimator in tests
yyforyongyu Jan 25, 2024
81874f8
rpcserver: skip calculating fee rate for PSBT funding
yyforyongyu Jan 26, 2024
5abfb39
docs: update release notes
yyforyongyu Jan 9, 2024
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
2 changes: 1 addition & 1 deletion contractcourt/anchor_resolver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ func (c *anchorResolver) Resolve() (ContractResolver, error) {
resultChan, err := c.Sweeper.SweepInput(
&anchorInput,
sweep.Params{
Fee: sweep.FeePreference{
Fee: sweep.FeeEstimateInfo{
FeeRate: relayFeeRate,
},
},
Expand Down
2 changes: 1 addition & 1 deletion contractcourt/channel_arbitrator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1359,7 +1359,7 @@ func (c *ChannelArbitrator) sweepAnchors(anchors *lnwallet.AnchorResolutions,
_, err = c.cfg.Sweeper.SweepInput(
&anchorInput,
sweep.Params{
Fee: sweep.FeePreference{
Fee: sweep.FeeEstimateInfo{
ConfTarget: deadline,
},
Force: force,
Expand Down
2 changes: 1 addition & 1 deletion contractcourt/commit_sweep_resolver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -351,7 +351,7 @@ func (c *commitSweepResolver) Resolve() (ContractResolver, error) {
// sweeper.
c.log.Infof("sweeping commit output")

feePref := sweep.FeePreference{ConfTarget: commitOutputConfTarget}
feePref := sweep.FeeEstimateInfo{ConfTarget: commitOutputConfTarget}
resultChan, err := c.Sweeper.SweepInput(inp, sweep.Params{Fee: feePref})
if err != nil {
c.log.Errorf("unable to sweep input: %v", err)
Expand Down
15 changes: 11 additions & 4 deletions contractcourt/commit_sweep_resolver_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package contractcourt

import (
"fmt"
"testing"
"time"

Expand Down Expand Up @@ -127,9 +128,15 @@ func (s *mockSweeper) SweepInput(input input.Input, params sweep.Params) (

s.sweptInputs <- input

// TODO(yy): use `mock.Mock` to avoid the conversion.
fee, ok := params.Fee.(sweep.FeeEstimateInfo)
if !ok {
return nil, fmt.Errorf("unexpected fee type: %T", params.Fee)
}

// Update the deadlines used if it's set.
if params.Fee.ConfTarget != 0 {
s.deadlines = append(s.deadlines, int(params.Fee.ConfTarget))
if fee.ConfTarget != 0 {
s.deadlines = append(s.deadlines, int(fee.ConfTarget))
}

result := make(chan sweep.Result, 1)
Expand All @@ -140,8 +147,8 @@ func (s *mockSweeper) SweepInput(input input.Input, params sweep.Params) (
return result, nil
}

func (s *mockSweeper) CreateSweepTx(inputs []input.Input, feePref sweep.FeePreference,
currentBlockHeight uint32) (*wire.MsgTx, error) {
func (s *mockSweeper) CreateSweepTx(inputs []input.Input,
feePref sweep.FeeEstimateInfo) (*wire.MsgTx, error) {

// We will wait for the test to supply the sweep tx to return.
sweepTx := <-s.createSweepTxChan
Expand Down
12 changes: 4 additions & 8 deletions contractcourt/htlc_success_resolver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,7 +263,7 @@ func (h *htlcSuccessResolver) broadcastReSignedSuccessTx() (
_, err := h.Sweeper.SweepInput(
&secondLevelInput,
sweep.Params{
Fee: sweep.FeePreference{
Fee: sweep.FeeEstimateInfo{
ConfTarget: secondLevelConfTarget,
},
},
Expand Down Expand Up @@ -375,7 +375,7 @@ func (h *htlcSuccessResolver) broadcastReSignedSuccessTx() (
_, err = h.Sweeper.SweepInput(
inp,
sweep.Params{
Fee: sweep.FeePreference{
Fee: sweep.FeeEstimateInfo{
ConfTarget: sweepConfTarget,
},
},
Expand Down Expand Up @@ -432,17 +432,13 @@ func (h *htlcSuccessResolver) resolveRemoteCommitOutput() (
// transaction, that we'll use to move these coins back into
// the backing wallet.
//
// TODO: Set tx lock time to current block height instead of
// zero. Will be taken care of once sweeper implementation is
// complete.
//
// TODO: Use time-based sweeper and result chan.
var err error
h.sweepTx, err = h.Sweeper.CreateSweepTx(
[]input.Input{inp},
sweep.FeePreference{
sweep.FeeEstimateInfo{
ConfTarget: sweepConfTarget,
}, 0,
},
)
if err != nil {
return nil, err
Expand Down
4 changes: 2 additions & 2 deletions contractcourt/htlc_timeout_resolver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -486,7 +486,7 @@ func (h *htlcTimeoutResolver) sweepSecondLevelTx() error {
_, err := h.Sweeper.SweepInput(
inp,
sweep.Params{
Fee: sweep.FeePreference{
Fee: sweep.FeeEstimateInfo{
ConfTarget: secondLevelConfTarget,
},
Force: true,
Expand Down Expand Up @@ -702,7 +702,7 @@ func (h *htlcTimeoutResolver) handleCommitSpend(
_, err = h.Sweeper.SweepInput(
inp,
sweep.Params{
Fee: sweep.FeePreference{
Fee: sweep.FeeEstimateInfo{
ConfTarget: sweepConfTarget,
},
},
Expand Down
4 changes: 2 additions & 2 deletions contractcourt/interfaces.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,8 @@ type UtxoSweeper interface {
// CreateSweepTx accepts a list of inputs and signs and generates a txn
// that spends from them. This method also makes an accurate fee
// estimate before generating the required witnesses.
CreateSweepTx(inputs []input.Input, feePref sweep.FeePreference,
currentBlockHeight uint32) (*wire.MsgTx, error)
CreateSweepTx(inputs []input.Input,
feePref sweep.FeeEstimateInfo) (*wire.MsgTx, error)

// RelayFeePerKW returns the minimum fee rate required for transactions
// to be relayed.
Expand Down
2 changes: 1 addition & 1 deletion contractcourt/utxonursery.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -823,7 +823,7 @@ func (u *UtxoNursery) sweepMatureOutputs(classHeight uint32,
utxnLog.Infof("Sweeping %v CSV-delayed outputs with sweep tx for "+
"height %v", len(kgtnOutputs), classHeight)

feePref := sweep.FeePreference{ConfTarget: kgtnOutputConfTarget}
feePref := sweep.FeeEstimateInfo{ConfTarget: kgtnOutputConfTarget}
for _, output := range kgtnOutputs {
// Create local copy to prevent pointer to loop variable to be
// passed in with disastrous consequences.
Expand Down
8 changes: 8 additions & 0 deletions docs/release-notes/release-notes-0.18.0.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,14 @@
* [Allow callers of ListSweeps to specify the start height](
https://github.com/lightningnetwork/lnd/pull/7372).

* Previously when callng `SendCoins`, `SendMany`, `OpenChannel` and
`CloseChannel` for coop close, it is allowed to specify both an empty
Copy link
Member

Choose a reason for hiding this comment

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

Good area to start using an fn.Either as mentioned before to encode this restriction into the types themselves.

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like the tlv package is not updated in go mod yet, will do and see how it can help.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like this PR did not touch any of these functions, currently SatPerVbyte and TargetConf are still both optional fields at least in sendCoins, would there be upcoming PRs to enforce this from the client side?

Copy link
Member Author

Choose a reason for hiding this comment

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

SendCoins use CalculateFeeRate which enforce this check.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah so I think there should be a check here on the lncli side to ensure at least either targetconf or satvbyte is set. Maybe for OpenChannel, CloseChannel etc. as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Updating the docs on flags as well as they are both optional.

I do not have much context about this change or why it is needed but we can either ensure these lncli commands have these flags or if they are missing the SendCoins RPC function and other functions can set a default before it is passed to the CalculateFeeRate function, for user experience.

Copy link
Member Author

Choose a reason for hiding this comment

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

think an error will be returned from the cli if not set since it's calling the RPC? Don't think it's a good idea to set the default here - it's a very sensitive param, and the users need to set it themselves.

Copy link
Contributor

Choose a reason for hiding this comment

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

That is true but I think a prelim check for these kind of things is usually done on the lncli function.

Also my thinking is that it is no sensitive since initially users did not need to worry about it and a default was used under the hoodie if not supplied.

`SatPerVbyte` and `TargetConf`, and a default conf target of 6 will be used.
This is [no longer allowed](
https://github.com/lightningnetwork/lnd/pull/8422) and the caller must
specify either `SatPerVbyte` or `TargetConf` so the fee estimator can do a
proper fee estimation.

## lncli Updates

* [Documented all available lncli commands](https://github.com/lightningnetwork/lnd/pull/8181).
Expand Down
1 change: 1 addition & 0 deletions htlcswitch/mock.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ func (m *mockPreimageCache) SubscribeUpdates(
return nil, nil
}

// TODO(yy): replace it with chainfee.MockEstimator.
type mockFeeEstimator struct {
byteFeeIn chan chainfee.SatPerKWeight
relayFee chan chainfee.SatPerKWeight
Expand Down
5 changes: 3 additions & 2 deletions itest/lnd_channel_funding_fund_max_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -322,8 +322,9 @@ func sweepNodeWalletAndAssert(ht *lntest.HarnessTest, node *node.HarnessNode) {

// Send all funds back to the miner node.
node.RPC.SendCoins(&lnrpc.SendCoinsRequest{
Addr: minerAddr.String(),
SendAll: true,
Addr: minerAddr.String(),
SendAll: true,
TargetConf: 6,
})

// Ensures we don't leave any transaction in the mempool after sweeping.
Expand Down
2 changes: 2 additions & 0 deletions itest/lnd_coop_close_with_htlcs_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ func coopCloseWithHTLCs(ht *lntest.HarnessTest) {
closeClient := alice.RPC.CloseChannel(&lnrpc.CloseChannelRequest{
ChannelPoint: chanPoint,
NoWait: true,
TargetConf: 6,
Copy link
Member

Choose a reason for hiding this comment

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

So then does this already have implications to the existing/default RPC behavior?

Copy link
Member Author

Choose a reason for hiding this comment

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

This will affect users who specify neither feerate nor conf target, which is unlikely? So the previous behavior is, when nothing specified, default to conf target of 6, while the current behavior is, you must specify either conf target or feerate.

})
ht.AssertChannelInactive(bob, chanPoint)

Expand Down Expand Up @@ -184,6 +185,7 @@ func coopCloseWithHTLCsWithRestart(ht *lntest.HarnessTest) {
ChannelPoint: chanPoint,
NoWait: true,
DeliveryAddress: newAddr.Address,
TargetConf: 6,
})

// Assert that both nodes see the channel as waiting for close.
Expand Down
5 changes: 3 additions & 2 deletions itest/lnd_funding_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -528,8 +528,9 @@ func sendAllCoinsConfirm(ht *lntest.HarnessTest, node *node.HarnessNode,
addr string) {

sweepReq := &lnrpc.SendCoinsRequest{
Addr: addr,
SendAll: true,
Addr: addr,
SendAll: true,
TargetConf: 6,
}
node.RPC.SendCoins(sweepReq)
ht.MineBlocksAndAssertNumTxes(1, 1)
Expand Down
54 changes: 35 additions & 19 deletions itest/lnd_misc_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -777,16 +777,18 @@ func testSweepAllCoins(ht *lntest.HarnessTest) {

// Ensure that we can't send coins to our own Pubkey.
ainz.RPC.SendCoinsAssertErr(&lnrpc.SendCoinsRequest{
Addr: ainz.RPC.GetInfo().IdentityPubkey,
SendAll: true,
Label: sendCoinsLabel,
Addr: ainz.RPC.GetInfo().IdentityPubkey,
SendAll: true,
Label: sendCoinsLabel,
TargetConf: 6,
})

// Ensure that we can't send coins to another user's Pubkey.
ainz.RPC.SendCoinsAssertErr(&lnrpc.SendCoinsRequest{
Addr: ht.Alice.RPC.GetInfo().IdentityPubkey,
SendAll: true,
Label: sendCoinsLabel,
Addr: ht.Alice.RPC.GetInfo().IdentityPubkey,
SendAll: true,
Label: sendCoinsLabel,
TargetConf: 6,
})

// With the two coins above mined, we'll now instruct Ainz to sweep all
Expand All @@ -798,23 +800,34 @@ func testSweepAllCoins(ht *lntest.HarnessTest) {

// Send coins to a testnet3 address.
ainz.RPC.SendCoinsAssertErr(&lnrpc.SendCoinsRequest{
Addr: "tb1qfc8fusa98jx8uvnhzavxccqlzvg749tvjw82tg",
SendAll: true,
Label: sendCoinsLabel,
Addr: "tb1qfc8fusa98jx8uvnhzavxccqlzvg749tvjw82tg",
SendAll: true,
Label: sendCoinsLabel,
TargetConf: 6,
})

// Send coins to a mainnet address.
ainz.RPC.SendCoinsAssertErr(&lnrpc.SendCoinsRequest{
Addr: "1MPaXKp5HhsLNjVSqaL7fChE3TVyrTMRT3",
Addr: "1MPaXKp5HhsLNjVSqaL7fChE3TVyrTMRT3",
SendAll: true,
Label: sendCoinsLabel,
TargetConf: 6,
})

// Send coins to a compatible address without specifying fee rate or
// conf target.
ainz.RPC.SendCoinsAssertErr(&lnrpc.SendCoinsRequest{
Addr: ht.Miner.NewMinerAddress().String(),
SendAll: true,
Label: sendCoinsLabel,
})

// Send coins to a compatible address.
ainz.RPC.SendCoins(&lnrpc.SendCoinsRequest{
Addr: ht.Miner.NewMinerAddress().String(),
SendAll: true,
Label: sendCoinsLabel,
Addr: ht.Miner.NewMinerAddress().String(),
SendAll: true,
Label: sendCoinsLabel,
TargetConf: 6,
})

// We'll mine a block which should include the sweep transaction we
Expand Down Expand Up @@ -911,10 +924,11 @@ func testSweepAllCoins(ht *lntest.HarnessTest) {
// If we try again, but this time specifying an amount, then the call
// should fail.
ainz.RPC.SendCoinsAssertErr(&lnrpc.SendCoinsRequest{
Addr: ht.Miner.NewMinerAddress().String(),
Amount: 10000,
SendAll: true,
Label: sendCoinsLabel,
Addr: ht.Miner.NewMinerAddress().String(),
Amount: 10000,
SendAll: true,
Label: sendCoinsLabel,
TargetConf: 6,
})

// With all the edge cases tested, we'll now test the happy paths of
Expand All @@ -940,8 +954,9 @@ func testSweepAllCoins(ht *lntest.HarnessTest) {
// Let's send some coins to the main address.
const amt = 123456
resp := ainz.RPC.SendCoins(&lnrpc.SendCoinsRequest{
Addr: mainAddrResp.Address,
Amount: amt,
Addr: mainAddrResp.Address,
Amount: amt,
TargetConf: 6,
})
block := ht.MineBlocksAndAssertNumTxes(1, 1)[0]
sweepTx := block.Transactions[1]
Expand Down Expand Up @@ -1024,6 +1039,7 @@ func testListAddresses(ht *lntest.HarnessTest) {
Addr: addr,
Amount: addressDetail.Balance,
SpendUnconfirmed: true,
TargetConf: 6,
})
}

Expand Down
26 changes: 16 additions & 10 deletions itest/lnd_onchain_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,8 +240,9 @@ func runCPFP(ht *lntest.HarnessTest, alice, bob *node.HarnessNode) {
// Send the coins from Alice to Bob. We should expect a transaction to
// be broadcast and seen in the mempool.
sendReq := &lnrpc.SendCoinsRequest{
Addr: resp.Address,
Amount: btcutil.SatoshiPerBitcoin,
Addr: resp.Address,
Amount: btcutil.SatoshiPerBitcoin,
TargetConf: 6,
}
alice.RPC.SendCoins(sendReq)
txid := ht.Miner.AssertNumTxsInMempool(1)[0]
Expand Down Expand Up @@ -383,8 +384,9 @@ func testAnchorReservedValue(ht *lntest.HarnessTest) {
resp := alice.RPC.NewAddress(req)

sweepReq := &lnrpc.SendCoinsRequest{
Addr: resp.Address,
SendAll: true,
Addr: resp.Address,
SendAll: true,
TargetConf: 6,
}
alice.RPC.SendCoins(sweepReq)

Expand Down Expand Up @@ -432,8 +434,9 @@ func testAnchorReservedValue(ht *lntest.HarnessTest) {
minerAddr := ht.Miner.NewMinerAddress()

sweepReq = &lnrpc.SendCoinsRequest{
Addr: minerAddr.String(),
SendAll: true,
Addr: minerAddr.String(),
SendAll: true,
TargetConf: 6,
}
alice.RPC.SendCoins(sweepReq)

Expand Down Expand Up @@ -469,8 +472,9 @@ func testAnchorReservedValue(ht *lntest.HarnessTest) {
// 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,
Addr: minerAddr.String(),
SendAll: true,
TargetConf: 6,
}
alice.RPC.SendCoins(sweepReq)

Expand Down Expand Up @@ -602,6 +606,7 @@ func testAnchorThirdPartySpend(ht *lntest.HarnessTest) {
sweepReq := &lnrpc.SendCoinsRequest{
Addr: minerAddr.String(),
SendAll: true,
TargetConf: 6,
MinConfs: 0,
SpendUnconfirmed: true,
}
Expand Down Expand Up @@ -755,8 +760,9 @@ func testRemoveTx(ht *lntest.HarnessTest) {
// We send half the amount to that address generating two unconfirmed
// outpoints in our internal wallet.
sendReq := &lnrpc.SendCoinsRequest{
Addr: resp.Address,
Amount: initialWalletAmt / 2,
Addr: resp.Address,
Amount: initialWalletAmt / 2,
TargetConf: 6,
}
alice.RPC.SendCoins(sendReq)
txID := ht.Miner.AssertNumTxsInMempool(1)[0]
Expand Down
1 change: 1 addition & 0 deletions itest/lnd_psbt_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1539,6 +1539,7 @@ func sendAllCoinsToAddrType(ht *lntest.HarnessTest,
Addr: resp.Address,
SendAll: true,
SpendUnconfirmed: true,
TargetConf: 6,
})

ht.MineBlocksAndAssertNumTxes(1, 1)
Expand Down
5 changes: 3 additions & 2 deletions itest/lnd_recovery_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,8 +254,9 @@ func testOnchainFundRecovery(ht *lntest.HarnessTest) {

minerAddr := ht.Miner.NewMinerAddress()
req := &lnrpc.SendCoinsRequest{
Addr: minerAddr.String(),
Amount: minerAmt,
Addr: minerAddr.String(),
Amount: minerAmt,
TargetConf: 6,
}
resp := node.RPC.SendCoins(req)

Expand Down
Loading
Loading