-
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
[7/5]contractcourt: make use of the new sweeper params #8514
[7/5]contractcourt: make use of the new sweeper params #8514
Conversation
Important Auto Review SkippedAuto reviews are disabled on this repository. 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 (
|
d95f1b5
to
9d4a6f6
Compare
33c575c
to
bed81ea
Compare
bed81ea
to
e0d8c63
Compare
cf30795
to
43fa9fa
Compare
4e5cfb8
to
f420f90
Compare
3a6b9e4
to
8bbadc1
Compare
39bdd14
to
6280f73
Compare
0ed10be
to
3f47dde
Compare
3f47dde
to
e194841
Compare
0250874
to
6e2eebc
Compare
e194841
to
97c8574
Compare
6e2eebc
to
d62a75f
Compare
This commit removes the method `CreateSweepTx` and makes sure when sweeping the htlc output via the direct-preimage spend, it's offered via the `SweepInput` interface.
Previously we don't allow confTarget to be 0, which ended up the final position being never reached. We fix it here by allowing confTarget to be 0 in case the deadline has already been passed for a given input.
This commit adds a new config method `QueryIncomingCircuit` that can be used to query the payment's incoming circuit for giving its outgoing circuit key.
This commit moves the offering of second-level outputs one block earlier. The sweeper will check the required locktime and wait until it matures. This is needed so the second-level outputs can be aggregated properly.
If anything happens during the fee bumping process, and causes the input to be failed, we should be able to mark it as `PublishFailed`.
bd1027c
to
6549ba3
Compare
03aac0e
to
ddacbca
Compare
bc4592c
to
0b31496
Compare
This commit adds an itest case that focuses on validating the CPFP logic in anchor sweeping.
0f530eb
to
8d55db8
Compare
The old commitment deadline is removed as it's no longer relevant.
This commit adds a new check for neutrino backend - when the inputs in the sweeping tx are spent by a third party, we will send back a `TxFailed` event to free the rest of the inputs for re-grouping.
8d55db8
to
4639a39
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 🫡
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.
Fwiw still left some comments.
if s.NoDeadlineConfTarget < 144 { | ||
return fmt.Errorf("nodeadlineconftarget must be at least 144") | ||
} |
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 would prefer to have the limit not restricted. One argument for having a limit is that eventually we want to have this outputs confirmed. I was just referring to the fact, that all non-timesenstive-funds can always be recovered even if the node crashes and only the chanbackup is available.
; fees when sweeping it. | ||
; sweeper.budget.tolocalratio=0.5 | ||
|
||
; The amount in satoshis to allocate as the budget to pay fees when CPFPing 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.
I thought it might be more intuitive, we only CPFP if there are HTLCs at risk, so I thought it would make sense to just use their budget limit as a Cap ?
// TODO(yy): If there's no corresponding incoming HTLC, it | ||
// means we are the first hop, hence the payer. This is a | ||
// tricky case - unlike a forwarding hop, we don't have an | ||
// incoming HTLC that will time out, which means as long as we | ||
// can learn the preimage, we can settle the invoice (before it | ||
// expires?). |
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.
ok, but how would we cancel the htlc from the commitment as a sender ?
c.cfg.ChanPoint, deadline, deadlineMinHeight, heightHint) | ||
// Calculate the value left after subtracting the budget used for | ||
// sweeping the time-sensitive HTLCs. | ||
valueLeft := totalValue - calculateBudget( |
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.
Ok, so I was thinking that the buget value would cap a specific htlc rather than be considered as bundling all htlcs together and then considering the budget. I suggested taking every htlc separately into account and calculating the remaining value for every htlc with a single DeadlineHTLC
, and then summing up the values for the remaining amount..
c.cfg.Budget.DeadlineHTLC, | ||
) | ||
|
||
log.Debugf("ChannelArbitrator(%v): calculated valueLeft=%v, "+ |
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: Can we explain this value valueLeft
or maybe substitute it for the difference htlcamt-budget
, otherwise I find it very hard to reason about it when reading the logs.
budget := calculateBudget( | ||
value, c.cfg.Budget.AnchorCPFPRatio, | ||
c.cfg.Budget.AnchorCPFP, | ||
) |
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.
Ok understand, so I think capping the anchors is good. Wdyt about calculating the value under protection by considering every htlc separately. Lets say you have 2 0.2 BTC HTLCs on a 1 BTC channel with the HTLC_Budget_Cap 0.1 and a ratio of 50%. The left value would be: 0.4*0.5 = 0.2BTC, capped at 0.1BTC. So it suggests that the budget is only 0.1, tho when sweeping the htlcs, a budget of 0.1 each will be used for every htlc separately so the real budget would be 0.2BTC.
@@ -349,6 +349,25 @@ func (h *htlcSuccessResolver) broadcastReSignedSuccessTx() ( | |||
"height %v", h, h.htlc.RHash[:], waitHeight) | |||
} | |||
|
|||
// Deduct one block so this input is offered to the sweeper one block |
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 is still needed I guess although we force sweeps now ?
// The above mined block will trigger Carol's sweeper to publish the | ||
// anchor sweeping tx. | ||
// | ||
// TODO(yy): should instead cancel the broadcast of the anchor sweeping |
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 am not sure whether I understand this TODO correctly, we cannot remove the anchor-cpfp, so you want to RBF the wallet input it consumes ?
|
||
// Do a non-blocking read to see if the output has been spent. | ||
select { | ||
case spend, ok := <-spendEvent.Spend: |
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 wonder if the time lacking in the RegisterSpendNtfn
rescan logic could cause problems here during restarts here?
Decided to open this final PR that makes use of the new sweeper params, so we can focus on deciding the proper deadlines and budgets to be used here.
TODO: add detailed docs here.
Depends on,
contractcourt
#8581This change is