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

sweep+rpc: add support to bump fee of inputs/transactions #3140

Merged
merged 6 commits into from Jun 12, 2019

Conversation

@wpaulino
Copy link
Collaborator

@wpaulino wpaulino commented May 29, 2019

In this PR, we add support to the UtxoSweeper to bump the fee of an input it's currently attempting to sweep and to bump the fee of a transaction through CPFP. We also add a new RPC and a corresponding lncli command to expose the functionality to users.

This RPC takes a different approach than bitcoind's bumpfee command. lnd has a central batching engine, known as the UtxoSweeper, in which inputs with similar fee rates are batched together to save on transaction fees. Due to this, we cannot rely on bumping the fee on a specific transaction, since transactions can change at any point with the addition of new inputs. The list of inputs that currently exist within the UtxoSweeper can be retrieved through the PendingSweeps RPC.

When bumping the fee of an input that currently exists within the UtxoSweeper, a higher fee transaction will be created that replaces the lower fee transaction through the Replace-By-Fee (RBF) policy.

This RPC also serves useful when wanting to perform a Child-Pays-For-Parent (CPFP), where the child transaction pays for its parent's fee. This can be done by specifying an outpoint within the low fee transaction that is under the control of the wallet.

Note that this RPC currently doesn't perform any validation checks on the fee preference being provided. For now, the responsibility of ensuring that the new fee preference is sufficient is delegated to the user.

Depends on #3089.

Fixes #609.
Fixes #2977.

sweep/sweeper.go Outdated Show resolved Hide resolved
Loading
sweep/sweeper.go Outdated
resultChan := make(chan Result, 1)
inputReq := &sweepInputMessage{
input: req.input,
feePreference: req.feePreference,
Copy link
Member

@Roasbeef Roasbeef May 31, 2019

Choose a reason for hiding this comment

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

So it's the job of the caller to compute the proper fee rate that'll cause the entire package to be bumped by a satisfactory amount?

Loading

Copy link
Collaborator Author

@wpaulino wpaulino May 31, 2019

Choose a reason for hiding this comment

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

Yes. I'm not a fan of this, but from our previous discussions offline it seemed like this was the approach to take for now. We should be able to compute this quite easily for full nodes, but things get a bit trickier with light clients.

Loading

cmd/lncli/commands.go Outdated Show resolved Hide resolved
Loading
lnrpc/rpc.proto Outdated Show resolved Hide resolved
Loading
rpcserver.go Outdated Show resolved Hide resolved
Loading
rpcserver.go Outdated Show resolved Hide resolved
Loading
rpcserver.go Outdated
feePreference := sweep.FeePreference{
ConfTarget: uint32(in.TargetConf),
FeeRate: satPerKw,
}
Copy link
Collaborator

@joostjager joostjager Jun 3, 2019

Choose a reason for hiding this comment

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

Validation here in case both or none are set?

Loading

Copy link
Collaborator Author

@wpaulino wpaulino Jun 3, 2019

Choose a reason for hiding this comment

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

Validation is done within the UtxoSweeper.BumpFee call.

Loading

Copy link
Collaborator

@joostjager joostjager Jun 4, 2019

Choose a reason for hiding this comment

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

Just checking that exactly one of those fields is set could may be done in a sweeper.NewFeePreference(confTarget, feeRate) constructor. It simplifies consumers of this struct, because they don't all need to do this check.

Loading

Copy link
Collaborator Author

@wpaulino wpaulino Jun 5, 2019

Choose a reason for hiding this comment

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

Some other RPCs rely on a fee preference not being set to choose the default. Adding a constructor that applies additional validation checks would lead to some confusion IMO. Either we should unify the behavior of FeePreference all around, or we introduce a new SweepPreference type that applies the validation needed for sweeps only.

Loading

rpcserver.go Outdated Show resolved Hide resolved
Loading
sweep/sweeper.go Outdated Show resolved Hide resolved
Loading
@wpaulino wpaulino force-pushed the sweeper-bumpfee branch from 43640ff to 0353813 Jun 3, 2019
@wpaulino
Copy link
Collaborator Author

@wpaulino wpaulino commented Jun 3, 2019

Loading

@wpaulino wpaulino force-pushed the sweeper-bumpfee branch from 0353813 to 923d46d Jun 3, 2019
sweep/sweeper.go Show resolved Hide resolved
Loading
@wpaulino
Copy link
Collaborator Author

@wpaulino wpaulino commented Jun 5, 2019

PTAL @joostjager.

Loading

@Roasbeef
Copy link
Member

@Roasbeef Roasbeef commented Jun 6, 2019

Can be rebased now that #3089 is in.

Loading

@wpaulino wpaulino force-pushed the sweeper-bumpfee branch from 6f3d736 to eab8169 Jun 6, 2019
@wpaulino
Copy link
Collaborator Author

@wpaulino wpaulino commented Jun 6, 2019

Rebased!

Loading

return nil, fmt.Errorf("unknown input witness %v", op)
}

signDesc := &input.SignDescriptor{
Copy link
Member

@Roasbeef Roasbeef Jun 7, 2019

Choose a reason for hiding this comment

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

Don't we need to provide a more populated sign descriptor? At the very least, the pubkey information.

Loading

Copy link
Collaborator Author

@wpaulino wpaulino Jun 7, 2019

Choose a reason for hiding this comment

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

The only field required seems to be the output. This is also what CraftSweepAllTx does.

Loading

cmd/lncli/types.go Outdated Show resolved Hide resolved
Loading
@wpaulino wpaulino force-pushed the sweeper-bumpfee branch 3 times, most recently from fe08390 to 1a05267 Jun 10, 2019
lntest/node.go Show resolved Hide resolved
Loading
lnrpc/walletrpc/walletkit_server.go Outdated Show resolved Hide resolved
Loading
sweep/sweeper.go Show resolved Hide resolved
Loading
sweep/sweeper.go Outdated Show resolved Hide resolved
Loading
feePreference FeePreference) (chan Result, error) {

// Ensure the client provided a sane fee preference.
if _, err := s.feeRateForPreference(feePreference); err != nil {
Copy link
Collaborator

@joostjager joostjager Jun 11, 2019

Choose a reason for hiding this comment

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

Still feels a bit strange that we validate against a moving fee rate here.

Loading

Copy link
Collaborator Author

@wpaulino wpaulino Jun 11, 2019

Choose a reason for hiding this comment

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

The fee preference won't always map to a moving fee rate. I see this more as just a sanity check to ensure it respects the min and max fee rate we'll allow.

Loading

Copy link
Collaborator

@joostjager joostjager Jun 12, 2019

Choose a reason for hiding this comment

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

I think I made the point before, but what I find strange is that we may reject an input here when the fee estimate (let's say X) exceeds the max. While if the fee estimate was below max, we would have added and when - later - the fee estimate would increase to X we would just wait until it goes down again. Seems inconsistent, but no blocker.

Loading

Copy link
Collaborator Author

@wpaulino wpaulino Jun 12, 2019

Choose a reason for hiding this comment

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

I understand your point, but I see this more as a sanity check to ensure we don't ever see some kind of insane fee rate.

Loading

wpaulino added 2 commits Jun 11, 2019
We want to make sure clients are aware of their own fee preferences,
rather than relying on defaults.
wpaulino added 4 commits Jun 11, 2019
In this commit, we introduce the ability to bump the fee of an input
within the UtxoSweeper. Once its fee rate is bumped, a replacement
transaction (RBF) will be broadcast with the newer fee rate (assuming
the newer fee rate is high enough to be valid), replacing any
conflicting lower fee rate transactions.

Note that this currently doesn't validate the fee preference of the
bump. This responsibility is delegated to the caller, so care must be
taken to ensure the new fee preference is sufficient.
This RPC exposes the recently added BumpFee functionality to the
UtxoSweeper in order to allow users of the RPC to manually bump fees of
low fee inputs/transactions.
@wpaulino wpaulino force-pushed the sweeper-bumpfee branch from 1a05267 to 638355b Jun 11, 2019
@wpaulino
Copy link
Collaborator Author

@wpaulino wpaulino commented Jun 11, 2019

Loading

Copy link
Member

@Roasbeef Roasbeef left a comment

LGTM 🐲

Loading

case nil:
return &BumpFeeResponse{}, nil
case lnwallet.ErrNotMine:
break
Copy link
Collaborator

@joostjager joostjager Jun 12, 2019

Choose a reason for hiding this comment

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

break can be removed

Loading

sweep/sweeper.go Outdated Show resolved Hide resolved
Loading
@wpaulino wpaulino merged commit 5af4022 into lightningnetwork:master Jun 12, 2019
2 checks passed
Loading
@wpaulino wpaulino deleted the sweeper-bumpfee branch Jun 12, 2019
@halseth halseth mentioned this pull request Oct 24, 2019
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment