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: add support for multiple fee preferences to UtxoSweeper #3026

Merged
merged 3 commits into from May 27, 2019

Conversation

Projects
None yet
4 participants
@wpaulino
Copy link
Collaborator

commented Apr 30, 2019

In this PR, we introduce support for arbitrary client fee preferences when accepting input sweep requests. This is possible with the addition of fee rate buckets. Fee rate buckets are buckets that contain inputs with similar fee rates within a specific range, e.g., 1-10 sat/vbyte, 11-20 sat/vbyte, etc. Having these buckets allows us to batch and sweep inputs from different clients with similar fee rates within a single transaction, allowing us to save on chain fees. Implementing these fee rate buckets also lays down the groundwork required for being able to bump the fee rates for particular inputs.

We also get rid of the UtxoSweeper's default fee preference. Any clients using it to sweep inputs specify the same default fee preference to not change their behavior. Each of these can be fine-tuned later on given their use cases.

@wpaulino wpaulino requested review from joostjager and Roasbeef Apr 30, 2019

@wpaulino wpaulino added this to the 0.7 milestone Apr 30, 2019

@wpaulino wpaulino force-pushed the wpaulino:sweeper-fee-preference branch from 789a157 to e8d3da8 Apr 30, 2019

@joostjager
Copy link
Collaborator

left a comment

An alternative implementation is to add a MultiFeeSweeper that creates an instance of the original unchanged UtxoSweeper per fee preference.

Show resolved Hide resolved sweep/sweeper.go
Show resolved Hide resolved sweep/sweeper.go Outdated
Show resolved Hide resolved sweep/sweeper.go
Show resolved Hide resolved sweep/sweeper.go Outdated
Show resolved Hide resolved sweep/sweeper.go Outdated

@wpaulino wpaulino force-pushed the wpaulino:sweeper-fee-preference branch 4 times, most recently from 69f19e7 to 7d88d5f May 8, 2019

@wpaulino

This comment has been minimized.

Copy link
Collaborator Author

commented May 8, 2019

PTAL @joostjager.

Show resolved Hide resolved sweep/bucket.go Outdated
Show resolved Hide resolved sweep/sweeper.go
feeRate, err := DetermineFeePerKw(s.cfg.FeeEstimator, *feePreference)
if err != nil {
return nil, err
}

This comment has been minimized.

Copy link
@joostjager

joostjager May 13, 2019

Collaborator

Could the whole idea of conf targets be moved outside of UtxoSweeper?

This comment has been minimized.

Copy link
@wpaulino

wpaulino May 13, 2019

Author Collaborator

As in allow the sweeper to only take in fee rates, so the caller would be responsible for mapping a target to a fee rate?

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef May 14, 2019

Member

IMO it make sense to have it live here. Callers are expressing their time preference w.r.t their expectation of when the sweep would happen. Pushing this mapping on the callers would just add code duplication at each call site, when it can instead all be consolidated here.

This comment has been minimized.

Copy link
@wpaulino

wpaulino May 14, 2019

Author Collaborator

Yeah it's necessary to have it done at the UtxoSweeper level anyway in order to get the confirmation target.

This comment has been minimized.

Copy link
@joostjager

joostjager May 14, 2019

Collaborator

I was thinking about a wrapper struct around the UtxoSweeper for callers that want to specify a conf target. So callers still don't need to do the mapping themselves.

UtxoSweeper isn't the easiest code to understand, so splitting off what's possible may help.

But not a big issue.

This comment has been minimized.

Copy link
@joostjager

joostjager May 16, 2019

Collaborator

I think we have to be realistic at follow up, pure refactor prs. They are mostly not going to happen. Also we would incur a risk twice. First changes in this pr and when that is all good, we change the system again with new risks.

Imo it is better to try to do it first time right. Less overall costs. We surely don't want to over-engineer, but the size of the sweeper as it currently is justifies some effort in separation responsibilities. Costs over the life time of the software of future devs trying to understand code adds up.

An alternative design that could be good is to offer inputs to UtxoSweeper with a GetFeeRate callback. That way callers can pass in a function that either returns a static fee rate, a fee rate calculated from a conf target or something more advanced that also takes into account the value of the input for example. For UtxoSweeper it'll be all the same. When it sweeps, it just collects fee rates and does its clustering magic.

For conf targets, there could be an object feeEstimationCache that caches the fee rate estimates for the block. The getFeeRate() callback for an input could be something like this closure:
func() {return feeEstimationCache.GetFeeRate(defaultCommitSweepTarget)}

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef May 16, 2019

Member

IMO we shouldn't push all this burden on the caller. The goal is to be able to allow the caller to express a fee preference. In the end, that preference may not be exactly met, but within a ball park range, particularly if there's a slightly different mapping that results in lnd paying lower fees over all. The code in question as is is also just a few lines: calling to the estimator. If we want to add some more magic between the caller and the estimator, then we can do that at the estimator interface level.

If we look at the current use cases within lnd, every time a caller goes to request that something be swept, it has a rough time frame w.r.t when it should be swept. Scenarios such as sweeping an HTLC have a very clear time frame (to avoid the race), while presences such as sweeping a CSV output on a confirmed output don't (as long as it isn't a revoked commitment ;)).

Yeah agree that it would be a nice separation of concerns.
but the size of the sweeper as it currently is justifies some effort in separation responsibilities.

The prior approach had an even more direct separation of concerns, but here we are now. Let's not forget the reality of things as they exist today: chain fess are rising, and users need to be able to specify a time preference. Even our own systems need this capability in order to scale and be sustainable. We have an opportunity to get something incrementally better in (atm we basically have nothing) before the next scheduled major release. Lets focus on the use cases at hand that we've experienced the need for first had (and has also been requested time after time by users of lnd).

An alternative design that could be good is to offer inputs to UtxoSweeper with a GetFeeRate callback

In the near future, do we ever see this being anything other than the default? All our current use cases just want to express a preference, and have the fee rate be loosely adjusted (there're no guarantees after all) if that is at risk of not being achieved. Down the line if we have a compelling use case, this can be added in: for these special inputs, the sweeper skips its regular mapping and uses that function instead.

This comment has been minimized.

Copy link
@joostjager

joostjager May 17, 2019

Collaborator

The code in question as is is also just a few lines: calling to the estimator.

It is more than a few lines, it is also the re-targeting part. I think an object shouldn't have too many responsibilities and the conf target mapping is something that can be extracted. But in the end it comes down to a believe about what is the best design.

In the near future, do we ever see this being anything other than the default?

As I now understand it, there are two implementations of this callback: either conf target based (returns a diff. fee rate every block) and a static fee rate (passed in on the cmd line).

This comment has been minimized.

Copy link
@halseth

halseth May 21, 2019

Collaborator

I believe this could be both simplified and made more safe for many of our use cases by specifying an absolute block height conf target instead the current conf target in number of blocks. Say we need a sweep to confirm by a specific height, the sweeper would start out low and increase the feerate as the target approaches.

Most important IMO is to adjust the fee rate dynamically, which could be one of the really powerful usecases of a sweeper like this.

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef May 22, 2019

Member

The clients themselves can target an absolute height by adjusting their specified relative conf target based on the current height. So if it's block 15, and they want this confirmed by block 25, then they can use a conf target of 10 (although things won't exactly match up).

Show resolved Hide resolved sweep/sweeper.go
Show resolved Hide resolved sweep/sweeper_test.go Outdated
Show resolved Hide resolved sweep/sweeper.go Outdated
Show resolved Hide resolved sweep/sweeper.go Outdated
Show resolved Hide resolved sweep/sweeper.go
Show resolved Hide resolved sweep/bucket.go Outdated
Show resolved Hide resolved sweep/bucket.go Outdated

// feeRate determines the fee rate of a bucket. This is done by calculating the
// average fee rate of all inputs within it.
func (b *bucket) feeRate() lnwallet.SatPerKWeight {

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef May 14, 2019

Member

Likely can be deferred as a follow up, but we may also eventually want to introduce the concept of "strict buckets". As an example, a strict bucket of 1 sat/byte would allow a caller to express that they only want that input swept at that specified fee rate and care less about batching.

Show resolved Hide resolved sweep/bucket.go Outdated
Show resolved Hide resolved sweep/sweeper.go Outdated
Show resolved Hide resolved sweep/sweeper.go Outdated

@wpaulino wpaulino force-pushed the wpaulino:sweeper-fee-preference branch 2 times, most recently from e6512d3 to 15036e5 May 16, 2019

@wpaulino

This comment has been minimized.

Copy link
Collaborator Author

commented May 16, 2019

@wpaulino wpaulino force-pushed the wpaulino:sweeper-fee-preference branch from 15036e5 to 86584d5 May 16, 2019

Show resolved Hide resolved contractcourt/commit_sweep_resolver.go
Show resolved Hide resolved sweep/sweeper.go Outdated

if numBlocksLeft > threshold {
continue
}

This comment has been minimized.

Copy link
@joostjager

joostjager May 16, 2019

Collaborator

I am not sure if we should do this already now. I'd be worried that on average users are going to end up paying more. Is this an actual problem to solve? To me it seems that being able to adjust the conf target or the fee rate directly is enough control.

Suppose the conf target is 6. We published with a fee that should get is confirmed after max 6 blocks. Suppose it would happen after exactly 6 blocks, which is within what is specified. We wouldn't need to do anything. But this logic starts bumping the fee up already after 1 block to the 5 block fee estimate.

This comment has been minimized.

Copy link
@wpaulino

wpaulino May 16, 2019

Author Collaborator

It's definitely a bit aggressive at the moment, but the functionality itself is still useful. Perhaps a better approach is to only start aggressively bumping after a positive delta (e.g., 2 blocks) from the intended confirmation height?

This comment has been minimized.

Copy link
@joostjager

joostjager May 16, 2019

Collaborator

Yes, so I am arguing that it may be too aggressive. If I pay for 6 confs and after 1 block I already become nervous and up the fee, isn't that strange?

We should also not lose sight of the problem to solve. Is it really this strict enforcement of a deadline or do we just want to get some more control over the sweeper process without hard guarantuees?

This comment has been minimized.

Copy link
@wpaulino

wpaulino May 16, 2019

Author Collaborator

If I pay for 6 confs and after 1 block I already become nervous and up the fee, isn't that strange?

That's why the threshold exists. The point is to increase the fee rate only if we need to in order to meet the confirmation target. If the fee rate doesn't change, then we won't update it.

The problem to solve is that confirmation targets won't be retried when scheduling/performing a sweep. If fees rise, the sweeper would handle that. With the buckets however, confirmation targets aren't retried. We could keep the behavior the same by querying for the same confirmation target like it previously would, but it's also about enforcing what "confirmation target" actually means. The current behavior doesn't hold the guarantees the term implies.

This comment has been minimized.

Copy link
@halseth

halseth May 21, 2019

Collaborator

It could be useful to distinguish inputs that we want to be swept within X blocks, and inputs that has to be swept within X blocks (absolute block height).

feeRate, err := DetermineFeePerKw(s.cfg.FeeEstimator, *feePreference)
if err != nil {
return nil, err
}

This comment has been minimized.

Copy link
@joostjager

joostjager May 16, 2019

Collaborator

I think we have to be realistic at follow up, pure refactor prs. They are mostly not going to happen. Also we would incur a risk twice. First changes in this pr and when that is all good, we change the system again with new risks.

Imo it is better to try to do it first time right. Less overall costs. We surely don't want to over-engineer, but the size of the sweeper as it currently is justifies some effort in separation responsibilities. Costs over the life time of the software of future devs trying to understand code adds up.

An alternative design that could be good is to offer inputs to UtxoSweeper with a GetFeeRate callback. That way callers can pass in a function that either returns a static fee rate, a fee rate calculated from a conf target or something more advanced that also takes into account the value of the input for example. For UtxoSweeper it'll be all the same. When it sweeps, it just collects fee rates and does its clustering magic.

For conf targets, there could be an object feeEstimationCache that caches the fee rate estimates for the block. The getFeeRate() callback for an input could be something like this closure:
func() {return feeEstimationCache.GetFeeRate(defaultCommitSweepTarget)}

// is done by determining the fee rate bucket they should belong in.
bucketInputs := make(map[lnwallet.SatPerKWeight]pendingInputs)
for _, input := range s.pendingInputs {
bucket := s.bucketForFeeRate(input.feeRate)

This comment has been minimized.

Copy link
@joostjager

joostjager May 16, 2019

Collaborator

With the fee rate callback suggestion above, this would become bucket := s.bucketForFeeRate(input.getFeeRate())

Show resolved Hide resolved sweep/sweeper.go
expectedConfHeight)

pendInput.feeRate = newFeeRate
}

This comment has been minimized.

Copy link
@joostjager

joostjager May 16, 2019

Collaborator

With the getFeeRate() suggestion, this would all be moved behind that call back and abstracted away from UtxoSweeper.

This comment has been minimized.

Copy link
@halseth

halseth May 21, 2019

Collaborator

Could also be a method on the FeePreference struct.

Show resolved Hide resolved contractcourt/commit_sweep_resolver.go
if input == nil || input.OutPoint() == nil || input.SignDesc() == nil {
return nil, errors.New("nil input received")
}

// Ensure the client provided a sane fee preference.
feeRate, err := s.validateFeePreference(feePreference)

This comment has been minimized.

Copy link
@halseth

halseth May 21, 2019

Collaborator

This should be dymanic IMO. If the fee market changes drastically right after this fee rate is calculated, it will never be updated.

This comment has been minimized.

Copy link
@wpaulino

wpaulino May 21, 2019

Author Collaborator

Yeah this is already the case if the client specifies a confirmation target. If a fee rate is specified, then it indicates the client would like to sweep only at that fee rate (or somewhere close due to the bucket structure).

feeRate, err := DetermineFeePerKw(s.cfg.FeeEstimator, *feePreference)
if err != nil {
return nil, err
}

This comment has been minimized.

Copy link
@halseth

halseth May 21, 2019

Collaborator

I believe this could be both simplified and made more safe for many of our use cases by specifying an absolute block height conf target instead the current conf target in number of blocks. Say we need a sweep to confirm by a specific height, the sweeper would start out low and increase the feerate as the target approaches.

Most important IMO is to adjust the fee rate dynamically, which could be one of the really powerful usecases of a sweeper like this.

Show resolved Hide resolved sweep/sweeper.go Outdated
func (s *UtxoSweeper) bucketForFeeRate(
feeRate lnwallet.SatPerKWeight) lnwallet.SatPerKWeight {

step := s.cfg.FeeRateSpacing

This comment has been minimized.

Copy link
@halseth

halseth May 21, 2019

Collaborator

Using fixed buckets seems inherently limiting. For instance would you never sweep two inputs having feerates of 9 and 11 sat/b together, even though it could be cheaper to group them together in one 11 sat/b transaction.

Another approach would be to look at the lowest feerate input, and keep clustering it with other inputs as long as that is cheaper than sweeping them separately.

This comment has been minimized.

Copy link
@wpaulino

wpaulino May 21, 2019

Author Collaborator

Yeah there are definitely multiple ways of clustering inputs that would produce more efficient sweeps depending on the scenario -- the current implementation only serves as a starting point. In the future, it'd be nice if the sweeper was aware of the best way to cluster inputs given different scenarios.

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef May 22, 2019

Member

One other relevant mempool level detail is that we can't add new unconfirmed inputs to an existing transaction. As a result, the transaction level clustering can't vary too much between retargets. With the bucket approach the transactions in each bucket can stay relatively stable, but say if our 1 sat/byte input gets bumped to 5 sat/byte, then it'll still be in the same transaction cluster but will push the actual fee rate of that bucket up a bit.

Show resolved Hide resolved sweep/sweeper.go Outdated

if numBlocksLeft > threshold {
continue
}

This comment has been minimized.

Copy link
@halseth

halseth May 21, 2019

Collaborator

It could be useful to distinguish inputs that we want to be swept within X blocks, and inputs that has to be swept within X blocks (absolute block height).

expectedConfHeight)

pendInput.feeRate = newFeeRate
}

This comment has been minimized.

Copy link
@halseth

halseth May 21, 2019

Collaborator

Could also be a method on the FeePreference struct.

Show resolved Hide resolved sweep/sweeper.go

@wpaulino wpaulino force-pushed the wpaulino:sweeper-fee-preference branch 4 times, most recently from eb5a916 to 177f626 May 22, 2019

Show resolved Hide resolved sweep/sweeper.go Outdated
feeRate, err := DetermineFeePerKw(s.cfg.FeeEstimator, *feePreference)
if err != nil {
return nil, err
}

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef May 22, 2019

Member

The clients themselves can target an absolute height by adjusting their specified relative conf target based on the current height. So if it's block 15, and they want this confirmed by block 25, then they can use a conf target of 10 (although things won't exactly match up).

@Roasbeef
Copy link
Member

left a comment

Tentative LGTM 🐚

Will start to run some live tests on testnet+mainnet (mainnet is more interesting since fees are starting to fluctuate more) and will report back if anything stands out (using the other RPC to monitor sweeping progress).

@wpaulino wpaulino force-pushed the wpaulino:sweeper-fee-preference branch from 177f626 to cfdae18 May 23, 2019

Show resolved Hide resolved sweep/sweeper.go Outdated

@wpaulino wpaulino force-pushed the wpaulino:sweeper-fee-preference branch 3 times, most recently from 97b9e54 to 74dc42d May 23, 2019

@wpaulino

This comment has been minimized.

Copy link
Collaborator Author

commented May 24, 2019

Pushed a new version that removes re-evaluating fee rates for inputs with confirmation targets.

@wpaulino wpaulino force-pushed the wpaulino:sweeper-fee-preference branch from 74dc42d to cf3b4db May 24, 2019

@wpaulino

This comment has been minimized.

Copy link
Collaborator Author

commented May 24, 2019

Show resolved Hide resolved sweep/sweeper.go
Show resolved Hide resolved sweep/sweeper.go Outdated
Show resolved Hide resolved sweep/sweeper.go Outdated
Show resolved Hide resolved sweep/sweeper.go Outdated
Show resolved Hide resolved sweep/sweeper.go Outdated
// First, we'll group together all inputs with similar fee rates. This
// is done by determining the fee rate bucket they should belong in.
for op, input := range s.pendingInputs {
feeRate, err := s.validateFeePreference(input.feePreference)

This comment has been minimized.

Copy link
@joostjager

joostjager May 24, 2019

Collaborator

Re-suggesting to add input.GetFeeRate(). Sweeper is already a large struct and imo it should be made smaller rather than larger if we change things.

wpaulino added some commits Apr 30, 2019

multi: support arbitrary client fee preferences to UtxoSweeper
In this commit, we introduce support for arbitrary client fee
preferences when accepting input sweep requests. This is possible with
the addition of fee rate buckets. Fee rate buckets are buckets that
contain inputs with similar fee rates within a specific range, e.g.,
1-10 sat/vbyte, 11-20 sat/vbyte, etc. Having these buckets allows us to
batch and sweep inputs from different clients with similar fee rates
within a single transaction, allowing us to save on chain fees.

With this addition, we can now get rid of the UtxoSweeper's default fee
preference. As of this commit, any clients using the it to sweep inputs
specify the same fee preference to not change their behavior. Each of
these can be fine-tuned later on given their use cases.
sweep: broadcast sweep transactions in descending fee rate order
In this commit, we address another issue that arose with the
introduction of the fee rate buckets. We'll use an example to explain
the problem space:

Let's say we have inputs A, B, and C within the same fee rate bucket. If
A's fee rate is bumped to a higher bucket, then it's currently possible
for the lower fee rate bucket to be swept first, which would produce an
invalid RBF transaction since we're removing an input from the original
without providing a higher fee. By the time we get to the higher fee
rate bucket, we broadcast a valid RBF transaction _only_ sweeping input
A, which would evict the transaction sweeping inputs B and C from the
mempool.

To prevent this eviction, we can simply broadcast the higher fee rate
sweep transactions first, to ensure we have valid RBF transactions.

@wpaulino wpaulino force-pushed the wpaulino:sweeper-fee-preference branch from cf3b4db to 682aebd May 24, 2019

@Roasbeef
Copy link
Member

left a comment

LGTM 🎉

Happy to move forward with this as is, to ultimately circle back with a retargeting algorithm once we vet a few of our current ideas in the wild in the current fe climate.

@joostjager
Copy link
Collaborator

left a comment

Two non blocking comments left. I am happy with the scope of the PR as it is. LGTM

if input == nil || input.OutPoint() == nil || input.SignDesc() == nil {
return nil, errors.New("nil input received")
}

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

This comment has been minimized.

Copy link
@joostjager

joostjager May 27, 2019

Collaborator

If the fee estimate is currently above the maximum but will possible drop below later, we still want to return an error here? It means that resolvers go into an error state until lnd is restarted.

// bucketForFeeReate determines the proper bucket for a fee rate. This is done
// in order to batch inputs with similar fee rates together.
func (s *UtxoSweeper) bucketForFeeRate(
feeRate lnwallet.SatPerKWeight) lnwallet.SatPerKWeight {

This comment has been minimized.

Copy link
@joostjager

joostjager May 27, 2019

Collaborator

Return value could just as well be an int or other identifier, as the actual value isn't used other than for grouping.

@Roasbeef Roasbeef merged commit 0343327 into lightningnetwork:master May 27, 2019

1 of 2 checks passed

coverage/coveralls Coverage decreased (-0.03%) to 60.313%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@wpaulino wpaulino deleted the wpaulino:sweeper-fee-preference branch May 27, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.