-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
[2/5]sweep: refactor fee estimation and introduce UtxoAggregator
#8422
Conversation
Important Auto Review SkippedAuto 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 To trigger a single review, invoke the 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? TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Additionally, you can add CodeRabbit Configration File (
|
eaee06b
to
28fbe26
Compare
28fbe26
to
ed7ade5
Compare
@coderabbitai review |
There was a problem hiding this 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.
// 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( |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 nLockTime
s are the same tho.
There was a problem hiding this comment.
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?
lntest/harness.go
Outdated
if p.ConfTarget != 0 { | ||
confTarget = p.ConfTarget | ||
} | ||
|
||
// If there's fee rate set, unset the conf target. | ||
if p.SatPerVByte != 0 { | ||
confTarget = 0 | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
@@ -65,6 +65,7 @@ func testCoopCloseWithHtlcs(ht *lntest.HarnessTest) { | |||
closeClient := alice.RPC.CloseChannel(&lnrpc.CloseChannelRequest{ | |||
ChannelPoint: chanPoint, | |||
NoWait: true, | |||
TargetConf: 6, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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,
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
ed7ade5
to
6c447a7
Compare
sweep/aggregator.go
Outdated
} | ||
} | ||
|
||
// createInputClusters creates a list of input clusters from the set of pending |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: godoc
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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() |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
818a65a
to
0d2054d
Compare
There was a problem hiding this 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.
lnrpc/rpc_utils.go
Outdated
ConfTarget: targetConf, | ||
FeeRate: satPerKw, | ||
} | ||
// TODO(yy): need to pass the configed max fee here. |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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{} |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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?
sweep/aggregator.go
Outdated
// 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: lock time => locktime
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
b491ba6
to
40dfafa
Compare
6bce6b0
to
21b9cbd
Compare
21b9cbd
to
9b18b49
Compare
40dfafa
to
ae5353d
Compare
9b18b49
to
470bf42
Compare
@yyforyongyu, remember to re-request review from reviewers when ready |
ae5353d
to
14ec634
Compare
470bf42
to
1446416
Compare
14ec634
to
b9e6ef2
Compare
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`.
1446416
to
5abfb39
Compare
@@ -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, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
sweep/walletsweep.go
Outdated
// Ensure a type of fee preference is specified to prevent using a | ||
// default below. | ||
if f.FeeRate == 0 && f.ConfTarget == 0 { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you mean?
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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?
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.UtxoAggregator
as an interface that defines how the inputs should be grouped. The original clustering logic is refactored into theSimpleAggregator
, that defines a simple strategy that implements this interface, which groups the inputs into clusters based on fee rates and locktime values.