-
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
contractcourt+sweep: fix fee function and deadline issue #8751
Conversation
Important Auto Review SkippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting 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 (
|
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.
Needs an itest for #8738.
// budget is too small. | ||
if l.deltaFeeRate == 0 && l.width != 1 { | ||
log.Errorf("Failed to init fee function: startingFeeRate=%v, "+ | ||
"endingFeeRate=%v, width=%v, delta=%v", start, end, | ||
confTarget, l.deltaFeeRate) | ||
l.width, l.deltaFeeRate) |
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 is the reason for this check? Can we remove it?
It seems like the check prevents broadcasting the transaction in cases where the budget is actually enough. For example:
confTarget
is 2- starting feerate calculated from
confTarget
is 30 sat/vB - ending feerate for budget is 30 sat/vB
If we broadcast immediately at 30 sat/vB, the transaction is likely to confirm in time. But currently an error is returned instead, and broadcasting is delayed until the next block when confTarget
is 1 and the branch above is taken instead. But we are likely to miss the deadline in that case, since we broadcasted late.
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.
As shown in the unit test, when conf target is 2, the max fee rate will be used. The check is added so we don't have a fee rate delta of 0, since in that case the fee func cannot do anything, also indicates the budget is too small.
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 it makes sense to keep this case in place and not broadcast the sweep when we still have not reached the deadline (conftarget=2) so that more inputs can eventually increase the endFeeRate. Eventually the sweep is triggered in case the deadline is reached.
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.
As shown in the unit test, when conf target is 2, the max fee rate will be used.
Right, bad example, sorry.
The check is added so we don't have a fee rate delta of 0, since in that case the fee func cannot do anything, also indicates the budget is too small.
In this case, shouldn't we just broadcast with max budget immediately and hope for the best?
Let's say conf target is 80 and fee rate delta is 0. Isn't it better to broadcast immediately at the max feerate? Returning an error means we will still broadcast at max feerate but we wait until 1 block away from the deadline, which basically guarantees we will miss the deadline.
I think it makes sense to keep this case in place and not broadcast the sweep when we still have not reached the deadline (conftarget=2) so that more inputs can eventually increase the endFeeRate.
It is very unlikely that future inputs will have the same deadline as the current inputs, so they are unlikely to be batched together.
Even so, there's no guarantee that future inputs will improve the situation -- they might not pay for their own weight 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.
In this case, shouldn't we just broadcast with max budget immediately and hope for the best?
I guess the question is where is the hope coming from:)
I don't know if the age of a tx is still a factor for the miners these days. If not and, if the tx gets confirmed at some blocks later, it means the network fee rate goes down -> the fee func will be created then?
The initial intention is to return an error here, so the user can increase the budget - for instance, when the htlc is large and the budget ratio is small, the user can intervene by specifying a larger budget via BumpFee
.
It is very unlikely that future inputs will have the same deadline as the current inputs, so they are unlikely to be batched together.
Yeah this is an issue IMO - if the deadlines are already large, I think they can be grouped - like, how different in terms of urgency they have between deadlines of 1008 and 1007 - think there should be a ladder structure, sth like the fee rate bucket we used to have.
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 don't know if the age of a tx is still a factor for the miners these days. If not and, if the tx gets confirmed at some blocks later, it means the network fee rate goes down -> the fee func will be created then?
Maybe the fee func will be created then, or maybe not. How sensitive is bitcoind's fee estimator to temporary drops in fee rate? If not too sensitive, then the fee func will continue to fail.
And bitcoind's fee estimator is backwards-looking, so it will always be delayed in adjusting downward. So by the time we actually broadcast the transaction, it might be too late (because deadline is missed or because we missed the lower-fee window and mempool fees went back up).
Overall, I think we're better off to have the transaction in the mempool so it can potentially be mined. Until we broadcast it, it can never be mined.
The initial intention is to return an error here, so the user can increase the budget - for instance, when the htlc is large and the budget ratio is small, the user can intervene by specifying a larger budget via
BumpFee
.
Perhaps an early sanity check specifically for BumpFee
is more appropriate? If budget is less than bitcoind's estimate, we can quickly return an error from BumpFee
instead of processing further.
For contractcourt sweeps, I think it's better to broadcast at max budget than to give up completely.
Yeah this is an issue IMO - if the deadlines are already large, I think they can be grouped - like, how different in terms of urgency they have between deadlines of 1008 and 1007 - think there should be a ladder structure, sth like the fee rate bucket we used to have.
+1
1245201
to
49b6994
Compare
49b6994
to
ed56a94
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.
Had some minor comments apart from that it's LGTM
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.
Still have an itest failing for neutrino:
harness_miner.go:217:
Error Trace: /home/runner/work/lnd/lnd/lntest/harness_miner.go:217
/home/runner/work/lnd/lnd/lntest/harness.go:1770
/home/runner/work/lnd/lnd/itest/lnd_multi-hop_test.go:2499
/home/runner/work/lnd/lnd/itest/lnd_multi-hop_test.go:152
Error: Received unexpected error:
want 1, got 2 in mempool: [08f01a438347fca122b815936cfa9417a66fbbaf2d3b02497eec7c4c382ca4aa 6b76702b74421fdc0a7b0fbdb12bd076bb7ff215dac9fdaae79ab5f54655355b]
Test: TestLightningNetworkDaemon/tranche03/134-of-155/neutrino/htlc_timeout_resolver_extract_preimage_remote/zeroconf=true/committype=ANCHORS
Messages: assert tx in mempool timeout
And possibly related failures on windows.
// budget is too small. | ||
if l.deltaFeeRate == 0 && l.width != 1 { | ||
log.Errorf("Failed to init fee function: startingFeeRate=%v, "+ | ||
"endingFeeRate=%v, width=%v, delta=%v", start, end, | ||
confTarget, l.deltaFeeRate) | ||
l.width, l.deltaFeeRate) |
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.
As shown in the unit test, when conf target is 2, the max fee rate will be used.
Right, bad example, sorry.
The check is added so we don't have a fee rate delta of 0, since in that case the fee func cannot do anything, also indicates the budget is too small.
In this case, shouldn't we just broadcast with max budget immediately and hope for the best?
Let's say conf target is 80 and fee rate delta is 0. Isn't it better to broadcast immediately at the max feerate? Returning an error means we will still broadcast at max feerate but we wait until 1 block away from the deadline, which basically guarantees we will miss the deadline.
I think it makes sense to keep this case in place and not broadcast the sweep when we still have not reached the deadline (conftarget=2) so that more inputs can eventually increase the endFeeRate.
It is very unlikely that future inputs will have the same deadline as the current inputs, so they are unlikely to be batched together.
Even so, there's no guarantee that future inputs will improve the situation -- they might not pay for their own weight either.
ed56a94
to
36cb2f6
Compare
@morehouse Yeah the itest failure is annoying, don't think it can be properly fixed until
As for now, I did an extra check in the test to make it pass. More work to do in 18.1, as I wanna fix these multi-hop tests so it's easier to maintain in the future. |
This commit changes how the deadline is calculated for CPFP anchor sweeping. In order to sweep the second-level HTLCs, we need to first get the FC tx confirmed. If we use a larger conf target for CPFP, we'd end up having few blocks to sweep the HTLCs, as these two sweeping txns share the deadline of the HTLC, as shown below, ``` More aggressive on the CPFP part. |-CPFP-|-----HTLC-----| Share the deadlines evenly. |---CPFP---|---HTLC---| More aggressive on the HTLC part. |-----CPFP-----|-HTLC-| ``` In this commit, we decide to share the deadlines evenly as a starting point so neither side will have a short of deadlines.
Replaced `testSweepAnchorCPFPLocalForceClose` with dedicated tests.
52ef42b
to
1b33068
Compare
This commit changes the logging levels and add a few more loggings based on the testing results from the testnet/mainnet.
1b33068
to
3475377
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 🐠
Fixes #8741
Fixes #8738
TODO: