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

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

Merged

Conversation

yyforyongyu
Copy link
Collaborator

@yyforyongyu yyforyongyu commented Jan 25, 2024

This PR prepares for #8424 by refactoring the fee estimation and clustering logic,

  • FeePreference is now an interface, which makes it possible to implement different fee estimation logics and more importantly, makes it easier to write unit tests as we now mock its behavior.
  • Added UtxoAggregator as an interface that defines how the inputs should be grouped. The original clustering logic is refactored into the SimpleAggregator, that defines a simple strategy that implements this interface, which groups the inputs into clusters based on fee rates and locktime values.

Copy link

coderabbitai bot commented Jan 25, 2024

Important

Auto Review Skipped

Auto reviews are disabled on base/target branches other than the default branch. Please add the base/target branch pattern to the list of additional branches to be reviewed in the settings.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository.

To trigger a single review, invoke the @coderabbitai review command.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit-tests for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit tests for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit tests.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

CodeRabbit Discord Community

Join our Discord Community to get help, request features, and share feedback.

@yyforyongyu yyforyongyu force-pushed the sweeper-rbf-1 branch 10 times, most recently from eaee06b to 28fbe26 Compare January 27, 2024 02:13
@saubyk saubyk added this to the v0.18.0 milestone Jan 28, 2024
@yyforyongyu yyforyongyu added utxo sweeping llm-review add to a PR to have an LLM bot review it labels Jan 30, 2024
@yyforyongyu yyforyongyu self-assigned this Jan 30, 2024
@yyforyongyu
Copy link
Collaborator Author

@coderabbitai review

Copy link
Collaborator

@ProofOfKeags ProofOfKeags left a comment

Choose a reason for hiding this comment

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

Again this seems very straightforward. I think this is good to go. All of my comments are on improving the core logic here which I think the follow up PRs will handle.

sweep/walletsweep.go Outdated Show resolved Hide resolved
sweep/walletsweep.go Show resolved Hide resolved
// is determined by calculating the average fee rate of all inputs within that
// cluster. In addition to the created clusters, inputs that did not specify a
// required lock time are returned.
func (s *SimpleAggregator) clusterByLockTime(
Copy link
Collaborator

Choose a reason for hiding this comment

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

what's the actual probability that these have equal locktimes? seems unlikely no? Don't we want to bucket on locktime distance too or is that too dangerous because of needing to aggressively time out htlcs?

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah looks like this is just reshuffling logical interfaces.

Copy link
Member

Choose a reason for hiding this comment

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

what's the actual probability that these have equal locktimes?

Happens pretty often when a channel needs to go to chain with a buncha HTLCs which are MPP shards so part of the same logical payment, and if they traverse a similar route, then they'll end up with close or identical CLTV values.

You need to cluster by lock time to ensure the inputs can be spent in the same transaction. Some of them are also second level inputs, so the lock time is actually only signed in the txn.

Copy link
Member

Choose a reason for hiding this comment

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

In theory we can have more intelligent clustering as well, since given a lock time of T, and input with a lock time K with K < T can be bundled together.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah that's exactly what I was thinking but we do have to consider how late we are willing to be on sweeping. Conceptually it feels alright if we're at least willing to delay by CLTV∂/2 and then we can ensure that the diameter of the cluster is no larger than that and then sign the transaction with the latest locktime requirement. That said, this is optimization and is certainly delay-able.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For single|anyone txns we cannot bundle them unless the nLockTimes are the same tho.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So this sent me down a rabbit hole where I was trying to convince myself that this was true. Check my reasoning here:

All sighash types commit to nLockTime, so we can't bundle many different second-stage htlc txs unless they have the same locktime because even if we were to change the locktime of the overall tx to satisfy all of the different CLTV constraints in the timeouts, the signatures would be invalidated because of the nLockTime change?

sweep/aggregator.go Show resolved Hide resolved
sweep/aggregator.go Show resolved Hide resolved
sweep/aggregator.go Show resolved Hide resolved
Comment on lines 1001 to 1006
if p.ConfTarget != 0 {
confTarget = p.ConfTarget
}

// If there's fee rate set, unset the conf target.
if p.SatPerVByte != 0 {
confTarget = 0
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems like an opportunity to make this setting an Either.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Could you help me by providing some code snippets🙇 Tried to use it but no luck.

@@ -242,21 +238,12 @@ type UtxoSweeper struct {
currentHeight int32
}

// feeDeterminer defines an alias to the function signature of
// `DetermineFeePerKw`.
type feeDeterminer func(chainfee.Estimator,
Copy link
Member

Choose a reason for hiding this comment

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

Interesting that this was also a private type def in the first place 🤔

@@ -2137,124 +2135,6 @@ func TestSweeperShutdownHandling(t *testing.T) {
require.Error(t, err)
}

// TestFeeRateForPreference checks `feeRateForPreference` works as expected.
func TestFeeRateForPreference(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

This test is still valid _after_the refactor, no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah - since this method is now merged into Estimate, the tests now happen in TestFeeEstimateInfo.

// chain backend(`bitcoind`) to give a fee estimation, or a customized fee
// function which handles fee calculation based on the specified
// urgency(deadline).
type FeePreference interface {
Copy link
Member

Choose a reason for hiding this comment

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

So is this where the new fee retargeting logic will live?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Initially yes. Now it's kept so it's easier to be mocked in tests.

sweep/sweeper_test.go Outdated Show resolved Hide resolved
sweep/sweeper_test.go Outdated Show resolved Hide resolved
sweep/aggregator.go Show resolved Hide resolved
sweep/aggregator.go Show resolved Hide resolved
@@ -65,6 +65,7 @@ func testCoopCloseWithHtlcs(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
Collaborator 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.

rpcsLog.Debugf("[openchannel]: using fee of %v sat/kw for funding tx",
int64(feeRate))
// Skip estimating fee rate for PSBT funding.
if in.FundingShim == nil || in.FundingShim.GetPsbtShim() == nil {
Copy link
Member

Choose a reason for hiding this comment

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

Why should the fee rate be skipped? IIUC, you can provide something with empty or partial inputs to have coin selection figure out the rest (change addrs, etc too)

Copy link
Collaborator

Choose a reason for hiding this comment

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

This change seems unrelated to the PR - I'm missing the motivation here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The fee estimation needs to be skipped because of this check,

lnd/rpcserver.go

Lines 1879 to 1882 in a3bf2c7

if req.SatPerByte != 0 || req.SatPerVbyte != 0 || req.TargetConf != 0 { // nolint:staticcheck
return nil, fmt.Errorf("specifying fee estimation parameters " +
"is not supported for PSBT funding")
}

This change seems unrelated to the PR - I'm missing the motivation here?

Because we enforce fee params check in CalculateFeeRate, if we don't pass them, it will fail there. And if we pass them, it will fail for PSBT funding.

@@ -185,6 +185,14 @@
[MinConf](https://github.com/lightningnetwork/lnd/pull/8097)(minimum number
of confirmations) has been added to the `WalletBalance` RPC call.

* 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
Collaborator 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
Collaborator 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
Collaborator 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.

@yyforyongyu yyforyongyu changed the base branch from master to elle-sweeper-store-fees February 20, 2024 03:19
}
}

// createInputClusters creates a list of input clusters from the set of pending
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: godoc

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

rpcsLog.Debugf("[openchannel]: using fee of %v sat/kw for funding tx",
int64(feeRate))
// Skip estimating fee rate for PSBT funding.
if in.FundingShim == nil || in.FundingShim.GetPsbtShim() == nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This change seems unrelated to the PR - I'm missing the motivation here?

func (s *SimpleAggregator) bucketForFeeRate(
feeRate chainfee.SatPerKWeight) int {

relayFeeRate := s.FeeEstimator.RelayFeePerKW()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we're no longer using the static s.relayFeeRate, this relayFeeRate can change between iterations and potentially bucket inputs that shouldn't be bucketed together. I think instead we should call RelayFeePerKW once at the top of the calling function and then pass that value in here.

I haven't checked the usage of s.relayFeeRate in later PRs yet, but after this PR it's only used in the anchor resolver post-force close and it may be a stale value. Not a huge deal though, but would be good to sweep the anchor with a valid min relay fee.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good call! This aggregator will become obsolete once the fee bumper is in, so I'll double check the new aggregator to make sure we won't have this issue there.

return 0
}

return 1 + int(feeRate-relayFeeRate)/s.FeeRateBucketSize
Copy link
Collaborator

@Crypt-iQ Crypt-iQ Feb 20, 2024

Choose a reason for hiding this comment

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

I believe this can underflow now since the check that's supposed to guard against this in Estimate via clusterBySweepFeeRate may be using a different value when comparing feeRate w/ the value from RelayFeePerKW

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Cool will make this old aggregation logic obsolete.

@yyforyongyu yyforyongyu force-pushed the sweeper-rbf-1 branch 2 times, most recently from 818a65a to 0d2054d Compare February 20, 2024 20:40
Copy link
Collaborator

@ziggie1984 ziggie1984 left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for the PR.

ConfTarget: targetConf,
FeeRate: satPerKw,
}
// TODO(yy): need to pass the configed max fee here.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: :s/configed/configured/g

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed 😅

rpcserver.go Outdated
ConfTarget: uint32(target),
}

// Since we are providing an fee estimation as an RPC response, there's
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: :s/an fee/a fee/g

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed!


// Since we are providing an fee estimation as an RPC response, there's
// no need to set a max feerate here, so we use 0.
feePerKw, err := feePref.Estimate(r.server.cc.FeeEstimator, 0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Q: so we don't cap the fee when sending coins, or what's the use case for the EstimateFee rpc ? When this rpc is used to make decisions when to trigger another RPC (e.g. sendcoins) we probably should also add the MaxFee cap there ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we need to update a few RPCs to allow specifying a max fee rate cap, including sendcoins. As for the usage of EstimateFee, I think it's used when we wanna open channels?

@@ -9,6 +9,8 @@ import (
// mockFeeEstimator implements a mock fee estimator. It closely resembles
// lnwallet.StaticFeeEstimator with the addition that fees can be changed for
// testing purposes in a thread safe manner.
//
// TODO(yy): replace it with chainfee.MockEstimator once it's merged.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Q: isn't this chainfee.MockEstimator part of this PR ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah but I wanna remove mockFeeEstimator and use chainfee.MockEstimator only - should be doable once the sweeper refactor is done.

`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 he fee estimator can do a
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: :s/so he fee estimator/so the fee estimator/g


relayFeeRate := s.FeeEstimator.RelayFeePerKW()

// Create an isolated bucket for sweeps at the minimum fee rate. This
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

// the FeePreference above
fee chainfee.SatPerKWeight
// Create a mock fee estimator.
estimator := &chainfee.MockEstimator{}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Here you are using it chainfee.MockEstimator ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hmm not sure I follow?

// with equal locktime together. Each cluster contains a sweep fee rate, which
// is determined by calculating the average fee rate of all inputs within that
// cluster. In addition to the created clusters, inputs that did not specify a
// required lock time are returned.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: lock time => locktime

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated

@lightninglabs-deploy
Copy link

@yyforyongyu, remember to re-request review from reviewers when ready

Thus we can use shorter method signatures. In doing so we also remove an
old TODO in one use case of `CreateSweepTx`.
This commit refactors the sweeper so the method `feeRateForPreference`
is now moved to `FeePreference`, which makes our following refactor
easier to handle.
This commit moves `DetermineFeePerKw` into the `Estimate` method on
`FeePreference`. A few callsites previously calling `DetermineFeePerKw`
without the max fee rate is now also temporarily fixed by forcing them
to use `Estimate` with the default sweeper max fee rate.
Results from running,
```
gofmt -d -w -r 'FeePreference -> FeeEstimateInfo' .
```
This commit adds a new interface `FeePreference` which makes it easier
to write unit tests and allows more customized implementation in
following commits.
This commit refactors the grouping logic into a new interface
`UtxoAggregator`, which makes it easier to write tests and opens
possibility for future customized clustering strategies.

The old clustering logic is kept as and moved into `SimpleAggregator`.
@yyforyongyu yyforyongyu merged commit a61f881 into lightningnetwork:elle-sweeper-store-fees Mar 18, 2024
26 of 28 checks passed
@yyforyongyu yyforyongyu deleted the sweeper-rbf-1 branch March 18, 2024 15:43
@@ -1484,7 +1146,7 @@ func (s *UtxoSweeper) CreateSweepTx(inputs []input.Input, feePref FeePreference,
}

tx, _, err := createSweepTx(
inputs, nil, pkScript, currentBlockHeight, feePerKw,
inputs, nil, pkScript, uint32(s.currentHeight), feePerKw,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't s.currentHeight be atomic? It looks like a data race is possible here since CreateSweepTx runs in a separate goroutine than collector, where currentHeight is updated.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good call! There's a todo in the following PR, and CreateSweepTx will be removed in the final PR.

// Ensure a type of fee preference is specified to prevent using a
// default below.
if f.FeeRate == 0 && f.ConfTarget == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

There is really no default below

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

what do you mean?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was pointing out that there was no default case in the switch statement but rereading it now, I think that was the intention.

@@ -185,6 +185,14 @@
[MinConf](https://github.com/lightningnetwork/lnd/pull/8097)(minimum number
of confirmations) has been added to the `WalletBalance` RPC call.

* Previously when callng `SendCoins`, `SendMany`, `OpenChannel` and
`CloseChannel` for coop close, it is allowed to specify both an empty
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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
llm-review add to a PR to have an LLM bot review it utxo sweeping
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

9 participants