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

FundPsbt and EstimateFee improvements #4845

Closed
wants to merge 1 commit into from
Closed
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
1,563 changes: 791 additions & 772 deletions lnrpc/rpc.pb.go

Large diffs are not rendered by default.

9 changes: 8 additions & 1 deletion lnrpc/rpc.proto
Original file line number Diff line number Diff line change
Expand Up @@ -891,6 +891,13 @@ message EstimateFeeRequest {
// The target number of blocks that this transaction should be confirmed
// by.
int32 target_conf = 2;

// The minimum number of confirmations each one of your outputs used for
// the transaction must satisfy.
int32 min_confs = 3;

// Whether unconfirmed outputs should be used as inputs for the transaction.
bool spend_unconfirmed = 4;
}

message EstimateFeeResponse {
Expand Down Expand Up @@ -2962,7 +2969,7 @@ message AddInvoiceResponse {
invoices with an add_index greater than this one.
*/
uint64 add_index = 16;

/*
The payment address of the generated invoice. This value should be used
in all payments for this invoice as we require it for end to end
Expand Down
16 changes: 16 additions & 0 deletions lnrpc/rpc.swagger.json
Original file line number Diff line number Diff line change
Expand Up @@ -1988,6 +1988,22 @@
"required": false,
"type": "integer",
"format": "int32"
},
{
"name": "min_confs",
"description": "The minimum number of confirmations each one of your outputs used for\nthe transaction must satisfy.",
"in": "query",
"required": false,
"type": "integer",
"format": "int32"
},
{
"name": "spend_unconfirmed",
"description": "Whether unconfirmed outputs should be used as inputs for the transaction.",
"in": "query",
"required": false,
"type": "boolean",
"format": "boolean"
}
],
"tags": [
Expand Down
255 changes: 137 additions & 118 deletions lnrpc/walletrpc/walletkit.pb.go

Large diffs are not rendered by default.

7 changes: 7 additions & 0 deletions lnrpc/walletrpc/walletkit.proto
Original file line number Diff line number Diff line change
Expand Up @@ -532,6 +532,13 @@ message FundPsbtRequest {
*/
uint32 sat_per_vbyte = 4;
}

// The minimum number of confirmations each one of your outputs used for
// the transaction must satisfy.
int32 min_confs = 5;

// Whether unconfirmed outputs should be used as inputs for the transaction.
bool spend_unconfirmed = 6;
}
message FundPsbtResponse {
/*
Expand Down
10 changes: 10 additions & 0 deletions lnrpc/walletrpc/walletkit.swagger.json
Original file line number Diff line number Diff line change
Expand Up @@ -803,6 +803,16 @@
"type": "integer",
"format": "int64",
"description": "The fee rate, expressed in sat/vbyte, that should be used to spend the\ninput with."
},
"min_confs": {
"type": "integer",
"format": "int32",
"description": "The minimum number of confirmations each one of your outputs used for\nthe transaction must satisfy."
},
"spend_unconfirmed": {
"type": "boolean",
"format": "boolean",
"description": "Whether unconfirmed outputs should be used as inputs for the transaction."
}
}
},
Expand Down
20 changes: 19 additions & 1 deletion lnrpc/walletrpc/walletkit_server.go
Original file line number Diff line number Diff line change
Expand Up @@ -981,6 +981,24 @@ func (w *WalletKit) FundPsbt(_ context.Context,
"specify either target_conf or set_per_vbyte")
}

var (
// minConfs can be provided by the RPC request,
// but defaults to defaultMinConf (1).
minConfs = int32(defaultMinConf)
)

switch {
// If min_confs was specified as non-zero in the RPC request,
// and spend_unconfirmed is false (the default if not provided),
// set minConfs to that number
case req.GetMinConfs() != 0 && !req.GetSpendUnconfirmed():
minConfs = req.GetMinConfs()
// If spend_unconfirmed is true, set minConfs to 0
// to allow for spending of unconfirmed utxos
case req.GetSpendUnconfirmed():
minConfs = 0
}

// The RPC parsing part is now over. Several of the following operations
// require us to hold the global coin selection lock so we do the rest
// of the tasks while holding the lock. The result is a list of locked
Expand All @@ -992,7 +1010,7 @@ func (w *WalletKit) FundPsbt(_ context.Context,
if len(packet.UnsignedTx.TxIn) > 0 {
// Get a list of all unspent witness outputs.
utxos, err := w.cfg.Wallet.ListUnspentWitness(
defaultMinConf, defaultMaxConf,
minConfs, defaultMaxConf,
)
if err != nil {
return err
Expand Down
6 changes: 3 additions & 3 deletions lntest/mock/walletcontroller.go
Original file line number Diff line number Diff line change
Expand Up @@ -86,15 +86,15 @@ func (w *WalletController) SendOutputs(outputs []*wire.TxOut,

// CreateSimpleTx currently returns dummy values.
func (w *WalletController) CreateSimpleTx(outputs []*wire.TxOut,
_ chainfee.SatPerKWeight, _ bool) (*txauthor.AuthoredTx, error) {
_ chainfee.SatPerKWeight, _ int32, _ bool) (*txauthor.AuthoredTx, error) {

return nil, nil
}

// ListUnspentWitness is called by the wallet when doing coin selection. We just
// need one unspent for the funding transaction.
func (w *WalletController) ListUnspentWitness(minconfirms,
maxconfirms int32) ([]*lnwallet.Utxo, error) {
func (w *WalletController) ListUnspentWitness(minConfs,
maxConfs int32) ([]*lnwallet.Utxo, error) {

// If the mock already has a list of utxos, return it.
if w.Utxos != nil {
Expand Down
19 changes: 13 additions & 6 deletions lnwallet/btcwallet/btcwallet.go
Original file line number Diff line number Diff line change
Expand Up @@ -298,7 +298,7 @@ func (b *BtcWallet) IsOurAddress(a btcutil.Address) bool {
//
// This is a part of the WalletController interface.
func (b *BtcWallet) SendOutputs(outputs []*wire.TxOut,
feeRate chainfee.SatPerKWeight, minconf int32, label string) (*wire.MsgTx, error) {
feeRate chainfee.SatPerKWeight, minConfs int32, label string) (*wire.MsgTx, error) {

// Convert our fee rate from sat/kw to sat/kb since it's required by
// SendOutputs.
Expand All @@ -309,13 +309,13 @@ func (b *BtcWallet) SendOutputs(outputs []*wire.TxOut,
return nil, lnwallet.ErrNoOutputs
}

// Sanity check minconf.
if minconf < 0 {
// Sanity check minConfs.
if minConfs < 0 {
return nil, lnwallet.ErrInvalidMinconf
}

return b.wallet.SendOutputs(
outputs, defaultAccount, minconf, feeSatPerKB, label,
outputs, defaultAccount, minConfs, feeSatPerKB, label,
)
}

Expand All @@ -333,7 +333,8 @@ func (b *BtcWallet) SendOutputs(outputs []*wire.TxOut,
//
// This is a part of the WalletController interface.
func (b *BtcWallet) CreateSimpleTx(outputs []*wire.TxOut,
feeRate chainfee.SatPerKWeight, dryRun bool) (*txauthor.AuthoredTx, error) {
feeRate chainfee.SatPerKWeight, minConfs int32,
dryRun bool) (*txauthor.AuthoredTx, error) {

// The fee rate is passed in using units of sat/kw, so we'll convert
// this to sat/KB as the CreateSimpleTx method requires this unit.
Expand All @@ -343,6 +344,12 @@ func (b *BtcWallet) CreateSimpleTx(outputs []*wire.TxOut,
if len(outputs) < 1 {
return nil, lnwallet.ErrNoOutputs
}

// Sanity check minConfs.
if minConfs < 0 {
return nil, lnwallet.ErrInvalidMinconf
}

for _, output := range outputs {
// When checking an output for things like dusty-ness, we'll
// use the default mempool relay fee rather than the target
Expand All @@ -357,7 +364,7 @@ func (b *BtcWallet) CreateSimpleTx(outputs []*wire.TxOut,
}
}

return b.wallet.CreateSimpleTx(defaultAccount, outputs, 1, feeSatPerKB, dryRun)
return b.wallet.CreateSimpleTx(defaultAccount, outputs, minConfs, feeSatPerKB, dryRun)
}

// LockOutpoint marks an outpoint as locked meaning it will no longer be deemed
Expand Down
6 changes: 3 additions & 3 deletions lnwallet/interface.go
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ type WalletController interface {
//
// NOTE: This method requires the global coin selection lock to be held.
SendOutputs(outputs []*wire.TxOut,
feeRate chainfee.SatPerKWeight, minconf int32, label string) (*wire.MsgTx, error)
feeRate chainfee.SatPerKWeight, minConfs int32, label string) (*wire.MsgTx, error)

// CreateSimpleTx creates a Bitcoin transaction paying to the specified
// outputs. The transaction is not broadcasted to the network. In the
Expand All @@ -202,7 +202,7 @@ type WalletController interface {
//
// NOTE: This method requires the global coin selection lock to be held.
CreateSimpleTx(outputs []*wire.TxOut, feeRate chainfee.SatPerKWeight,
dryRun bool) (*txauthor.AuthoredTx, error)
minConfs int32, dryRun bool) (*txauthor.AuthoredTx, error)

// ListUnspentWitness returns all unspent outputs which are version 0
// witness programs. The 'minconfirms' and 'maxconfirms' parameters
Expand All @@ -213,7 +213,7 @@ type WalletController interface {
// outputs with at least 'minconfirms'.
//
// NOTE: This method requires the global coin selection lock to be held.
ListUnspentWitness(minconfirms, maxconfirms int32) ([]*Utxo, error)
ListUnspentWitness(minConfs, maxConfs int32) ([]*Utxo, error)

// ListTransactionDetails returns a list of all transactions which are
// relevant to the wallet over [startHeight;endHeight]. If start height
Expand Down
108 changes: 81 additions & 27 deletions lnwallet/interface_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2536,6 +2536,8 @@ func testLastUnusedAddr(miner *rpctest.Harness,
// testCreateSimpleTx checks that a call to CreateSimpleTx will return a
// transaction that is equal to the one that is being created by SendOutputs in
// a subsequent call.
// All test cases are doubled-up: one for testing unconfirmed inputs,
// one for testing only confirmed inputs.
func testCreateSimpleTx(r *rpctest.Harness, w *lnwallet.LightningWallet,
_ *lnwallet.LightningWallet, t *testing.T) {

Expand All @@ -2547,51 +2549,100 @@ func testCreateSimpleTx(r *rpctest.Harness, w *lnwallet.LightningWallet,

// The test cases we will run through for all backends.
testCases := []struct {
outVals []int64
feeRate chainfee.SatPerKWeight
valid bool
outVals []int64
feeRate chainfee.SatPerKWeight
valid bool
unconfirmed bool
}{
{
outVals: []int64{},
feeRate: 2500,
valid: false, // No outputs.
outVals: []int64{},
feeRate: 2500,
valid: false, // No outputs.
unconfirmed: false,
},

{
outVals: []int64{200},
feeRate: 2500,
valid: false, // Dust output.
outVals: []int64{200},
feeRate: 2500,
valid: false, // Dust output.
unconfirmed: true,
},
{
outVals: []int64{200},
feeRate: 2500,
valid: false, // Dust output.
unconfirmed: false,
},
{
outVals: []int64{1e8},
feeRate: 2500,
valid: true,
unconfirmed: true,
},
{
outVals: []int64{1e8},
feeRate: 2500,
valid: true,
unconfirmed: false,
},
{
outVals: []int64{1e8, 2e8, 1e8, 2e7, 3e5},
feeRate: 2500,
valid: true,
unconfirmed: true,
},

{
outVals: []int64{1e8},
feeRate: 2500,
valid: true,
outVals: []int64{1e8, 2e8, 1e8, 2e7, 3e5},
feeRate: 2500,
valid: true,
unconfirmed: false,
},
{
outVals: []int64{1e8, 2e8, 1e8, 2e7, 3e5},
feeRate: 2500,
valid: true,
outVals: []int64{1e8, 2e8, 1e8, 2e7, 3e5},
feeRate: 12500,
valid: true,
unconfirmed: true,
},
{
outVals: []int64{1e8, 2e8, 1e8, 2e7, 3e5},
feeRate: 12500,
valid: true,
outVals: []int64{1e8, 2e8, 1e8, 2e7, 3e5},
feeRate: 12500,
valid: true,
unconfirmed: false,
},
{
outVals: []int64{1e8, 2e8, 1e8, 2e7, 3e5},
feeRate: 50000,
valid: true,
outVals: []int64{1e8, 2e8, 1e8, 2e7, 3e5},
feeRate: 50000,
valid: true,
unconfirmed: true,
},
{
outVals: []int64{1e8, 2e8, 1e8, 2e7, 3e5},
feeRate: 50000,
valid: true,
unconfirmed: false,
},
{
outVals: []int64{1e8, 2e8, 1e8, 2e7, 3e5, 1e8, 2e8,
1e8, 2e7, 3e5},
feeRate: 44250,
valid: true,
unconfirmed: true,
},
{
outVals: []int64{1e8, 2e8, 1e8, 2e7, 3e5, 1e8, 2e8,
1e8, 2e7, 3e5},
feeRate: 44250,
valid: true,
feeRate: 44250,
valid: true,
unconfirmed: false,
},
}

for i, test := range testCases {
var minConfs int32 = 1

if test.unconfirmed {
minConfs = 0
}

feeRate := test.feeRate

// Grab some fresh addresses from the miner that we will send
Expand All @@ -2617,9 +2668,12 @@ func testCreateSimpleTx(r *rpctest.Harness, w *lnwallet.LightningWallet,
}

// Now try creating a tx spending to these outputs.
// Note: this test runs CreateSimpleTx with a minConfs
// value of 1.
createTx, createErr := w.CreateSimpleTx(
outputs, feeRate, true,
outputs, feeRate, 1, true,
)

switch {
case test.valid && createErr != nil:
fmt.Println(spew.Sdump(createTx.Tx))
Expand All @@ -2635,7 +2689,7 @@ func testCreateSimpleTx(r *rpctest.Harness, w *lnwallet.LightningWallet,
// _very_ similar to the one we just created being sent. The
// only difference is that the dry run tx is not signed, and
// that the change output position might be different.
tx, sendErr := w.SendOutputs(outputs, feeRate, 1, labels.External)
tx, sendErr := w.SendOutputs(outputs, feeRate, minConfs, labels.External)
switch {
case test.valid && sendErr != nil:
t.Fatalf("got unexpected error when sending tx: %v",
Expand Down
Loading