contractcourt: use the sweeper for HTLC offered remote timeout resolu… #9062
Conversation
Roasbeef
added
enhancement
|
Important Review skippedAuto reviews are limited to specific labels. Labels to auto review (1)
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Roasbeef
force-pushed
the
htlc-resolution-sweeper
branch
from
fb5c245
to
3da8e85
Compare
There was a problem hiding this comment.
Yeah I also noticed this in #8922 where the direct spend is not offered to the sweeper and was planning to fix it afterwards. I think we need to handle the case where there's already a force close in the process and the output has already been offered to the utxo nursery. In that case, during the startup, we need to delete it from the nursery and offer it to the sweeper?
My idea to handle that is if the close is already in process, and offered, then this proceeds as normal (re the |
Roasbeef
force-pushed
the
htlc-resolution-sweeper
branch
from
3da8e85
to
fba0059
Compare
|
Pushed up a new version, itests+unit tests should be passing now. It also starts to send all new remote HTLC timeout outputs to the sweeper (prior commit was just taproot outputs). |
With the latest version, I modified the logic slightly:
I also needed to make a change to the way we generate reports for outputs. Previously it would skip asking the |
Roasbeef
force-pushed
the
htlc-resolution-sweeper
branch
2 times, most recently
from
6c191bd
to
2491692
Compare
There was a problem hiding this comment.
Is there a chance we fix this after #8922 - think things would be more clear there. However if the custom channel PR depends on this then nvm.
| // the nursery.Sign details are only non-nil for anchor channels. We | ||
| // then further restrict this path by ensuring we'll only return nil for | ||
| // non-anchor local commitmetn sweeps. | ||
| if h.htlcResolution.SignDetails == nil && h. |
There was a problem hiding this comment.
Trying to digest the change - I think it means we won't create reports here for spending of outputs from a local commitment with a legacy channel type, but for direct spending, aka, spending of the remote commitment, we'd always handle it here no matter it's anchor or legacy?
There was a problem hiding this comment.
Yeah I think we can make this section a bit more readable if we break out the various != checks into a new enum that explains the type of resolution we're doing.
I think would look something like:
var timeoutResolution uint8
const (
anchorLocalResolution = iota // h.htlcResolution.SignDetails != nil
remoteResolution // h.htlcResolution.SignedTimeoutTx == nil
legacyNurseryResolution // h.htlcResolution.SignDetails == nil
)There was a problem hiding this comment.
I think it means we won't create reports here for spending of outputs from a local commitment with a legacy channel type, but for direct spending, aka, spending of the remote commitment, we'd always handle it here no matter it's anchor or legacy?
With this change, the goal is that only non-anchor, local force close sweeps go through the nursery.
The CNCT now handles generating the report when the remote party has gone to chain and we want to sweep their output.
The nursery handles the report for legacy channels, where we've gone to chain ourselves.
There was a problem hiding this comment.
An enum would help a lot with readability and reducing confusion in the future.
There was a problem hiding this comment.
The CNCT now handles generating the report when the remote party has gone to chain and we want to sweep their output.
Cool didn't notice that before. Btw there are helpers created here already so i guess things will be more clear once that pr is merged.
Originally I set out to fix some existing TODOs/bugs as relates to the new aux chan feature, then realized that I'd have to update the nursery to understand some of the new interfaces. Instead of doing that, I opted to do this existing item on our wish list so things are more uniform. With this PR, this issue (assuming all the edge cases re upgrades are accounted for) is in sight: #3688 |
Along the way we refactor the test to eliminate some unnecessary line length.
…tion In this commit, we bring the timeout resolver more in line with the success resolver by using the sweeper to handle the HTLC offered remote timeout outputs. These are outputs that we can sweep directly from the remote party's commitment transaction when they broadcast their version of the commitment transaction. With this change, we slim down the scope slightly by only doing this for anchor channels. Non-anchor channels will continue to use the utxonursery for this output type for now.
Roasbeef
force-pushed
the
htlc-resolution-sweeper
branch
from
2491692
to
903c8fc
Compare
| // the nursery.Sign details are only non-nil for anchor channels. We | ||
| // then further restrict this path by ensuring we'll only return nil for | ||
| // non-anchor local commitmetn sweeps. | ||
| if h.htlcResolution.SignDetails == nil && h. |
There was a problem hiding this comment.
An enum would help a lot with readability and reducing confusion in the future.
| // the nursery.Sign details are only non-nil for anchor channels. We | ||
| // then further restrict this path by ensuring we'll only return nil for | ||
| // non-anchor local commitmetn sweeps. | ||
| if h.htlcResolution.SignDetails == nil && h. |
There was a problem hiding this comment.
The CNCT now handles generating the report when the remote party has gone to chain and we want to sweep their output.
Cool didn't notice that before. Btw there are helpers created here already so i guess things will be more clear once that pr is merged.
…tion
In this commit, we bring the timeout resolver more in line with the success resolver by using the sweeper to handle the HTLC offered remote timeout outputs. These are outputs that we can sweep directly from the remote party's commitment transaction when they broadcast their version of the commitment transaction.
With this change, we slim down the scope slightly by only doing this for anchor channels. Non-anchor channels will continue to use the utxonursery for this output type for now.