Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Add maxFeeRatio parameter to sanityCheckFee in psbt coin selection #8600

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions cmd/commands/walletrpc_active.go
Original file line number Diff line number Diff line change
Expand Up @@ -1252,6 +1252,12 @@ var fundPsbtCommand = cli.Command{
Value: defaultUtxoMinConf,
},
coinSelectionStrategyFlag,
cli.Float64Flag{
Name: "max_fee_ratio",
Usage: "the maximum fee to total output amount ratio " +
"that this psbt should adhere to",
Value: chanfunding.DefaultMaxFeeRatio,
},
},
Action: actionDecorator(fundPsbt),
}
Expand Down Expand Up @@ -1390,6 +1396,8 @@ func fundPsbt(ctx *cli.Context) error {
}
}

req.MaxFeeRatio = ctx.Float64("max_fee_ratio")

walletClient, cleanUp := getWalletClient(ctx)
defer cleanUp()

Expand Down
7 changes: 7 additions & 0 deletions docs/release-notes/release-notes-0.19.0.md
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,9 @@
`sat_per_kw` which allows for more precise
fees](https://github.com/lightningnetwork/lnd/pull/9013).

* [The `walletrpc.FundPsbt` method now has a new option to specify the maximum
ziggie1984 marked this conversation as resolved.
Show resolved Hide resolved
fee to output amounts ratio.](https://github.com/lightningnetwork/lnd/pull/8600)

## lncli Additions

* [A pre-generated macaroon root key can now be specified in `lncli create` and
Expand All @@ -62,6 +65,9 @@
specify more precise fee
rates](https://github.com/lightningnetwork/lnd/pull/9013).

* The `lncli wallet fundpsbt` command now has a [`--max_fee_ratio` argument to
specify the max fees to output amounts ratio.](https://github.com/lightningnetwork/lnd/pull/8600)

# Improvements
## Functional Updates

Expand Down Expand Up @@ -151,6 +157,7 @@
* Boris Nagaev
* CharlieZKSmith
* Elle Mouton
* George Tsagkarelis
* Pins
* Viktor Tigerström
* Ziggie
616 changes: 314 additions & 302 deletions lnrpc/walletrpc/walletkit.pb.go

Large diffs are not rendered by default.

3 changes: 3 additions & 0 deletions lnrpc/walletrpc/walletkit.proto
Original file line number Diff line number Diff line change
Expand Up @@ -1417,6 +1417,9 @@ message FundPsbtRequest {

// The strategy to use for selecting coins during funding the PSBT.
lnrpc.CoinSelectionStrategy coin_selection_strategy = 10;

// The max fee to total output amount ratio that this psbt should adhere to.
double max_fee_ratio = 12;
}
message FundPsbtResponse {
/*
Expand Down
5 changes: 5 additions & 0 deletions lnrpc/walletrpc/walletkit.swagger.json
Original file line number Diff line number Diff line change
Expand Up @@ -1589,6 +1589,11 @@
"coin_selection_strategy": {
"$ref": "#/definitions/lnrpcCoinSelectionStrategy",
"description": "The strategy to use for selecting coins during funding the PSBT."
},
"max_fee_ratio": {
"type": "number",
"format": "double",
"description": "The max fee to total output amount ratio that this psbt should adhere to."
}
}
},
Expand Down
15 changes: 11 additions & 4 deletions lnrpc/walletrpc/walletkit_server.go
Original file line number Diff line number Diff line change
Expand Up @@ -1626,11 +1626,17 @@ func (w *WalletKit) FundPsbt(_ context.Context,
return nil, fmt.Errorf("unknown change output type")
}

maxFeeRatio := chanfunding.DefaultMaxFeeRatio
GeorgeTsagk marked this conversation as resolved.
Show resolved Hide resolved

if req.MaxFeeRatio != 0 {
maxFeeRatio = req.MaxFeeRatio
}

// Run the actual funding process now, using the channel funding
// coin selection algorithm.
return w.fundPsbtCoinSelect(
account, changeIndex, packet, minConfs, changeType,
feeSatPerKW, coinSelectionStrategy,
feeSatPerKW, coinSelectionStrategy, maxFeeRatio,
)

// The template is specified as a RPC message. We need to create a new
Expand Down Expand Up @@ -1833,8 +1839,8 @@ func (w *WalletKit) fundPsbtInternalWallet(account string,
func (w *WalletKit) fundPsbtCoinSelect(account string, changeIndex int32,
packet *psbt.Packet, minConfs int32,
changeType chanfunding.ChangeAddressType,
feeRate chainfee.SatPerKWeight, strategy base.CoinSelectionStrategy) (
*FundPsbtResponse, error) {
feeRate chainfee.SatPerKWeight, strategy base.CoinSelectionStrategy,
maxFeeRatio float64) (*FundPsbtResponse, error) {

// We want to make sure we don't select any inputs that are already
// specified in the template. To do that, we require those inputs to
Expand Down Expand Up @@ -1922,6 +1928,7 @@ func (w *WalletKit) fundPsbtCoinSelect(account string, changeIndex int32,
changeAmt, needMore, err := chanfunding.CalculateChangeAmount(
inputSum, outputSum, packetFeeNoChange,
packetFeeWithChange, changeDustLimit, changeType,
maxFeeRatio,
)
if err != nil {
return nil, fmt.Errorf("error calculating change "+
Expand Down Expand Up @@ -1978,7 +1985,7 @@ func (w *WalletKit) fundPsbtCoinSelect(account string, changeIndex int32,

selectedCoins, changeAmount, err := chanfunding.CoinSelect(
feeRate, fundingAmount, changeDustLimit, coins,
strategy, estimator, changeType,
strategy, estimator, changeType, maxFeeRatio,
)
if err != nil {
return fmt.Errorf("error selecting coins: %w", err)
Expand Down
92 changes: 92 additions & 0 deletions lnrpc/walletrpc/walletkit_server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -162,10 +162,18 @@ func TestFundPsbtCoinSelect(t *testing.T) {
// packet in bytes.
expectedFee btcutil.Amount

// maxFeeRatio is the maximum fee to total output amount ratio
// that we consider valid.
maxFeeRatio float64

// expectedErr is the expected concrete error. If not nil, then
// the error must match exactly.
expectedErr error

// expectedContainedErrStr is the expected string to be
// contained in the returned error.
expectedContainedErrStr string

// expectedErrType is the expected error type. If not nil, then
// the error must be of this type.
expectedErrType error
Expand All @@ -178,6 +186,7 @@ func TestFundPsbtCoinSelect(t *testing.T) {
}),
changeIndex: -1,
feeRate: chainfee.FeePerKwFloor,
maxFeeRatio: chanfunding.DefaultMaxFeeRatio,
expectedErrType: &chanfunding.ErrInsufficientFunds{},
}, {
name: "1 p2wpkh utxo, add p2wkh change",
Expand All @@ -193,6 +202,7 @@ func TestFundPsbtCoinSelect(t *testing.T) {
}),
changeIndex: -1,
feeRate: chainfee.FeePerKwFloor,
maxFeeRatio: chanfunding.DefaultMaxFeeRatio,
expectedUtxoIndexes: []int{0},
expectChangeOutputIndex: 1,
expectedFee: calcFee(0, 1, 1, 1, 0),
Expand All @@ -210,6 +220,7 @@ func TestFundPsbtCoinSelect(t *testing.T) {
}),
changeIndex: -1,
feeRate: chainfee.FeePerKwFloor,
maxFeeRatio: chanfunding.DefaultMaxFeeRatio,
changeType: chanfunding.P2TRChangeAddress,
expectedUtxoIndexes: []int{0},
expectChangeOutputIndex: 1,
Expand All @@ -228,6 +239,7 @@ func TestFundPsbtCoinSelect(t *testing.T) {
}),
changeIndex: -1,
feeRate: chainfee.FeePerKwFloor,
maxFeeRatio: chanfunding.DefaultMaxFeeRatio,
expectedUtxoIndexes: []int{0},
expectChangeOutputIndex: -1,
expectedFee: calcFee(0, 1, 1, 0, 0),
Expand All @@ -247,6 +259,7 @@ func TestFundPsbtCoinSelect(t *testing.T) {
}),
changeIndex: -1,
feeRate: chainfee.FeePerKwFloor,
maxFeeRatio: chanfunding.DefaultMaxFeeRatio,
GeorgeTsagk marked this conversation as resolved.
Show resolved Hide resolved
changeType: chanfunding.P2WKHChangeAddress,
expectedUtxoIndexes: []int{0},
expectChangeOutputIndex: -1,
Expand All @@ -267,6 +280,7 @@ func TestFundPsbtCoinSelect(t *testing.T) {
}),
changeIndex: -1,
feeRate: chainfee.FeePerKwFloor,
maxFeeRatio: chanfunding.DefaultMaxFeeRatio,
changeType: chanfunding.P2TRChangeAddress,
expectedUtxoIndexes: []int{0},
expectChangeOutputIndex: -1,
Expand All @@ -285,6 +299,7 @@ func TestFundPsbtCoinSelect(t *testing.T) {
}),
changeIndex: 0,
feeRate: chainfee.FeePerKwFloor,
maxFeeRatio: chanfunding.DefaultMaxFeeRatio,
changeType: chanfunding.ExistingChangeAddress,
expectedUtxoIndexes: []int{0},
expectChangeOutputIndex: 0,
Expand All @@ -303,6 +318,7 @@ func TestFundPsbtCoinSelect(t *testing.T) {
}),
changeIndex: 0,
feeRate: chainfee.FeePerKwFloor,
maxFeeRatio: chanfunding.DefaultMaxFeeRatio,
changeType: chanfunding.ExistingChangeAddress,
expectedUtxoIndexes: []int{0},
expectChangeOutputIndex: 0,
Expand All @@ -321,6 +337,7 @@ func TestFundPsbtCoinSelect(t *testing.T) {
}),
changeIndex: 0,
feeRate: chainfee.FeePerKwFloor,
maxFeeRatio: chanfunding.DefaultMaxFeeRatio,
changeType: chanfunding.ExistingChangeAddress,
expectedUtxoIndexes: []int{0},
expectChangeOutputIndex: 0,
Expand Down Expand Up @@ -369,6 +386,7 @@ func TestFundPsbtCoinSelect(t *testing.T) {
}),
changeIndex: 0,
feeRate: chainfee.FeePerKwFloor,
maxFeeRatio: chanfunding.DefaultMaxFeeRatio,
changeType: chanfunding.ExistingChangeAddress,
expectedUtxoIndexes: []int{0, 1},
expectChangeOutputIndex: 0,
Expand Down Expand Up @@ -417,6 +435,7 @@ func TestFundPsbtCoinSelect(t *testing.T) {
}),
changeIndex: -1,
feeRate: chainfee.FeePerKwFloor,
maxFeeRatio: chanfunding.DefaultMaxFeeRatio,
changeType: chanfunding.P2TRChangeAddress,
expectedUtxoIndexes: []int{0, 1},
expectChangeOutputIndex: 1,
Expand Down Expand Up @@ -456,6 +475,7 @@ func TestFundPsbtCoinSelect(t *testing.T) {
}),
changeIndex: -1,
feeRate: chainfee.FeePerKwFloor,
maxFeeRatio: chanfunding.DefaultMaxFeeRatio,
changeType: chanfunding.P2WKHChangeAddress,
expectedUtxoIndexes: []int{},
expectChangeOutputIndex: 1,
Expand Down Expand Up @@ -497,10 +517,74 @@ func TestFundPsbtCoinSelect(t *testing.T) {
}),
changeIndex: -1,
feeRate: chainfee.FeePerKwFloor,
maxFeeRatio: chanfunding.DefaultMaxFeeRatio,
changeType: chanfunding.P2TRChangeAddress,
expectedUtxoIndexes: []int{},
expectChangeOutputIndex: -1,
expectedFee: calcFee(1, 0, 1, 0, 0),
}, {
name: "1 p2wpkh utxo, existing p2wkh change, invalid fee ratio",
utxos: []*lnwallet.Utxo{
{
Value: 250,
PkScript: p2wkhScript,
},
},
packet: makePacket(&wire.TxOut{
Value: 50,
PkScript: p2wkhScript,
}),
changeIndex: 0,
feeRate: chainfee.FeePerKwFloor,
maxFeeRatio: chanfunding.DefaultMaxFeeRatio,
changeType: chanfunding.ExistingChangeAddress,
expectedUtxoIndexes: []int{0},
expectChangeOutputIndex: 0,
expectedFee: calcFee(0, 1, 0, 1, 0),

expectedContainedErrStr: "fee 0.00000111 BTC exceeds max fee " +
"(0.00000027 BTC) on total output value",
}, {
name: "1 p2wpkh utxo, existing p2wkh change, negative feeratio",
utxos: []*lnwallet.Utxo{
{
Value: 250,
PkScript: p2wkhScript,
},
},
packet: makePacket(&wire.TxOut{
Value: 50,
PkScript: p2wkhScript,
}),
changeIndex: 0,
feeRate: chainfee.FeePerKwFloor,
maxFeeRatio: chanfunding.DefaultMaxFeeRatio * (-1),
ziggie1984 marked this conversation as resolved.
Show resolved Hide resolved
changeType: chanfunding.ExistingChangeAddress,
expectedUtxoIndexes: []int{0},
expectChangeOutputIndex: 0,
expectedFee: calcFee(0, 1, 0, 1, 0),

expectedContainedErrStr: "maxFeeRatio must be between 0.00 " +
"and 1.00 got -0.20",
}, {
name: "1 p2wpkh utxo, existing p2wkh change, big fee ratio",
utxos: []*lnwallet.Utxo{
{
Value: 250,
PkScript: p2wkhScript,
},
},
packet: makePacket(&wire.TxOut{
Value: 50,
PkScript: p2wkhScript,
}),
changeIndex: 0,
feeRate: chainfee.FeePerKwFloor,
maxFeeRatio: 0.85,
changeType: chanfunding.ExistingChangeAddress,
expectedUtxoIndexes: []int{0},
expectChangeOutputIndex: 0,
expectedFee: calcFee(0, 1, 0, 1, 0),
}, {
name: "large existing p2tr input, fee estimation existing " +
"change output",
Expand Down Expand Up @@ -536,6 +620,7 @@ func TestFundPsbtCoinSelect(t *testing.T) {
}),
changeIndex: 0,
feeRate: chainfee.FeePerKwFloor,
maxFeeRatio: chanfunding.DefaultMaxFeeRatio,
changeType: chanfunding.ExistingChangeAddress,
expectedUtxoIndexes: []int{},
expectChangeOutputIndex: 0,
Expand Down Expand Up @@ -575,6 +660,7 @@ func TestFundPsbtCoinSelect(t *testing.T) {
"", tc.changeIndex, copiedPacket, 0,
tc.changeType, tc.feeRate,
rpcServer.cfg.CoinSelectionStrategy,
tc.maxFeeRatio,
)

switch {
Expand All @@ -588,6 +674,12 @@ func TestFundPsbtCoinSelect(t *testing.T) {
require.Error(tt, err)
require.ErrorAs(tt, err, &tc.expectedErr)

return
case tc.expectedContainedErrStr != "":
require.ErrorContains(
tt, err, tc.expectedContainedErrStr,
)

return
}

Expand Down
Loading
Loading