-
Notifications
You must be signed in to change notification settings - Fork 2.2k
contractcourt+sweep: make anchor inputs exclusive #10117
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: make anchor inputs exclusive #10117
Conversation
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.
Summary of Changes
Hello @yyforyongyu, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request enhances the robustness of the sweeping mechanism for anchor outputs by ensuring they are processed in isolation. By preventing anchor inputs from being batched with other transaction inputs, the system mitigates potential transaction failures that could arise from node restarts or asynchronous spending notifications, thereby improving the reliability of anchor output management.
Highlights
- Anchor Input Sweeping Isolation: Anchor inputs are now configured to be swept individually in their own transactions. This is achieved by assigning a unique
ExclusiveGroup
identifier (derived from the channel ID) to each anchor input when it's passed to the sweeper, preventing it from being batched with other inputs. This change addresses potential issues with transaction failures upon node restarts, where previously grouped anchor inputs might cause issues if not all inputs are available. - Sweeper Parameter Documentation Clarification: The documentation comment for the
ExclusiveGroup
parameter within thesweep.Params
struct has been updated. The revised description clarifies that if this identifier is set for an input, it prevents that specific input from being batched with any other inputs, rather than just with inputs sharing the same identifier. This aligns the documentation with the intended behavior and usage.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/
folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
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.
Code Review
This pull request isolates anchor sweeps into their own transactions by introducing an ExclusiveGroup
parameter. This is a good change to prevent cascading failures on restart, as described in the PR description. The implementation looks correct. I've added one suggestion to improve the clarity of the documentation for the new ExclusiveGroup
field.
e4c40f1
to
b410c69
Compare
b410c69
to
f7e2669
Compare
What do you think about not sweeping the Anchor at all ? Wallet-Inputs are very crucial for the security of the sweeper subsystem, should we really block a wallet input to swepp around 184 sats ? Moreover the anchor can also be swept way more efficiently by a third party and since we will start not batching anchors it means less return for every sweep. I would rather completely remove the sweeping of the anchor (after the commitment has been confirmed) wdyt ? |
+1 cf: #8997 |
Yeah that's also in my radar, I'm thinking finalizing #8680 for 0.20, tho this PR would already fix #8997 given anchor is now never grouped?
Why would anchor sweeping take wallet inputs when it's not for CPFP? Note that after this PR, the effect is that the anchor won't be swept most of the time, unless the min fee drops below 1 sat/vb, so we just provide best-effort delivery here. |
How can the 330 sats input be spend otherwise ? If not via a wallet-input it will be smaller then dust ? Yeah I know if the fee is greater than 1 sat/vbyte it already becomes not economical to sweep it, so probably there are pros and cons, I currently don't see many pros sweeping this anchor tbh. Is the main reason that we want to keep the utxo set clean ? I tend to really remove this code, but if you think it might only be a long term possibility I am ok with this as well. |
That's a different question - either removing it or not, I want to make it clear that the non-CPFP anchor sweeping doesn't take any wallet inputs. And the dust is just a function of the mempool min feerate, we may soon see below 1 sat/vb feerate, tho that's not the point. So I'm not sure about the pros and cons analysis here.
I think for a minor release we don't want to introduce a large change like that. Also given the anchor isn't swept anyway, so does it really matter? And if 330 sats becomes more valuable, and if we do see 0.1 sat/vb, it means the dust is now 33 sats? And what about large node runners? If they have multiple FCs, and want to batch-sweep anchors, should we allow that? So there are a few more questions to be asked. The current way however, won't sweep the anchor most of the time - if the anchor does get swept, it means the feerate becomes very low, and, likely the value goes up a lot. Also if you really wanna remove this code (so do I), feel free to open a PR so we can have more discussion 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 the approach is ok, however, it would be good to know at what exact circumstances the anchor output gets swept (and where this is determined), because creating those tiny outputs isn't ideal. I'd also favor to remove the anchor sweeping in the long term (after involving some node runners).
// ExclusiveGroup is an identifier that, if set, ensures this input is | ||
// swept in a transaction by itself, and not batched with any other | ||
// inputs. |
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.
// ExclusiveGroup is an identifier that, if set, ensures this input is
// swept in a transaction by itself and that the input will never be co-spent
// by an input of the same exclusive group.
Maybe this could clarify the usage of this field a bit more
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 the old doc is outdated - when it's set, not only will it not be grouped with the inputs of the same exclusive group ID, but also we won't group it with any other inputs, so it's actually not will never be co-spent by an input of the same exclusive group.
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 wasn't aware how the exclusive group mechanics works, initially I assumed that all sweeps that have the same exclusive group are spent together, because I assumed they somehow belong together using the id. So that's why I thought it would be good to change the comment to clarify the functionality for anybody new who looks into the code.
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 with the new behavior it now simply means it won't be batched with any other inputs. I think once we get rid of anchor sweeping this confusing flag can also be removed.
sweep/sweeper.go
Outdated
// addition sweep transactions of those inputs will be removed from the wallet. | ||
func (s *UtxoSweeper) removeExclusiveGroup(group uint64) { | ||
// removeExclusiveGroup removes all inputs in the given exclusive group except | ||
// the input speified by the outpoint. This function is called when one of the |
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.
typo: specified
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 was this outpoint previously not removed, can this have some side-effects to the CPFP case ?
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.
typo fixed.
why was this outpoint previously not removed, can this have some side-effects to the CPFP case ?
I'm not sure if that's the case - what do you mean the outpoint was previously not removed?
// 1. Alice's CPFP-anchor sweeping is not attempted, instead, it should be | ||
// swept using the no deadline path and failed due it's not economical. |
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 should update the test to assert the behavior that the anchor is not spent. I see failures because the fee rate delta is zero, which is probably equivalent of it not being economical to spend the anchor, or is this very specific to the test? Would it be possible to also have the case where the anchor is swept as a non-CPFP?
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 a bit.
I see failures because the fee rate delta is zero, which is probably equivalent of it not being economical to spend the anchor, or is this very specific to the test?
Correct, and there's another log, which also stops the anchor sweeping,
2025-08-04 16:39:41.442 [INF] SWPR fee_bumper.go:1768: Change amt 0.00000206 BTC below dustlimit 0.00000330 BTC, not adding change output
We have two places to stop the sweeping,
- when the input has a budget that cannot create a non-zero delta fee func, we would stop, this is related to the starting fee rate, or in other words, the deadline delta.
- when the fee func is created, but it can only have a dust output, we will also stop.
Would it be possible to also have the case where the anchor is swept as a non-CPFP?
This is not possible given we hardcoded the dust limit here,
Line 46 in 8f94929
func DustLimitForSize(scriptSize int) btcutil.Amount { |
So it's always 330 sats, which is why the anchors won't be swept, until we make GetDustThreshold
to also account the live min relay fee from the mempool (which we should btw).
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 ok TIL that the 330 sat are computed with a static min relay fee of 3 sat/vb. So basically we'll never sweep if that is not made dynamic, because we'd always leave less than 330 sat?
sweep/sweeper.go
Outdated
// addition sweep transactions of those inputs will be removed from the wallet. | ||
func (s *UtxoSweeper) removeExclusiveGroup(group uint64) { | ||
// removeExclusiveGroup removes all inputs in the given exclusive group except | ||
// the input speified by the outpoint. This function is called when one of the |
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 was this outpoint previously not removed, can this have some side-effects to the CPFP case ?
@@ -1493,7 +1500,9 @@ func (s *UtxoSweeper) markInputsSwept(tx *wire.MsgTx, isOurTx bool) { | |||
|
|||
// Remove all other inputs in this exclusive group. | |||
if input.params.ExclusiveGroup != nil { | |||
s.removeExclusiveGroup(*input.params.ExclusiveGroup) | |||
s.removeExclusiveGroup( | |||
*input.params.ExclusiveGroup, outpoint, |
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 this be a problem here that we are not removing the output which is now swept by a third party ?
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.
It's always meant to remove the other inputs, not the one that's being spent - note that we check input.terminated()
in removeExclusiveGroup
to skip the spent ones.
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.
makes sense and we make sure we remove the transaction in the function handleInputSpent
if IsOurTx=False
Seeking clarification on the potential of tiny outputs being created by sweeping the anchor outputs standalone.
|
We now make sure to sweep each anchor input in its own sweeping tx, if economically feasible.
We now make sure when removing inputs identified by the exclusive group ID, we only remove the other one, not the one that invoked the removal.
f7e2669
to
4d7622a
Compare
@gemini-code-assist Seeking clarification on the potential of tiny outputs being created by sweeping the anchor outputs standalone.
|
Hi @yyforyongyu, thanks for reaching out! I can clarify these points for you:
cc @saubyk |
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
@@ -767,6 +775,8 @@ func (s *UtxoSweeper) removeExclusiveGroup(group uint64) { | |||
continue | |||
} | |||
|
|||
log.Debugf("Removing exclusive group for input %v", input) |
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: add the exclusive group in the log as well ?
@@ -1493,7 +1500,9 @@ func (s *UtxoSweeper) markInputsSwept(tx *wire.MsgTx, isOurTx bool) { | |||
|
|||
// Remove all other inputs in this exclusive group. | |||
if input.params.ExclusiveGroup != nil { | |||
s.removeExclusiveGroup(*input.params.ExclusiveGroup) | |||
s.removeExclusiveGroup( | |||
*input.params.ExclusiveGroup, outpoint, |
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.
makes sense and we make sure we remove the transaction in the function handleInputSpent
if IsOurTx=False
grouped with other non-time sensitive outputs such as `to_local` outputs, | ||
which potentially allow the sweeping tx to be pinned. This is now | ||
[fixed](https://github.com/lightningnetwork/lnd/pull/10117) by moving sweeping | ||
anchors into its own tx, which means the anchor outputs won't be swept in 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.
would be cool to actually group at least anchors together, seems like sub 1 sat/vbyte transactions can now be propagated quite reliably
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.
Gotta say I'm very confused, as on one hand you were saying you really wanna the anchor sweeping to be removed but now you think it's cool to group them?
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 did not know about the sub 1 sat/vbyte rule going to be removed by the core-devs, moreover you explained to me that anchors don't block a wallet input so given this new knowledge I would say there is nothing wrong with grouping them and therefore being able to recover valuable sathosis ?
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 gonna create a PR to remove the anchor sweeping - based on your current view, it seems unnecessary to create that 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.
Curious what your opinion is, but I would say either we remove the anchors or we make sure we can group different anchors from different channel force closes and make also sure we can broadcast them sub 1 sat/vbyte ? Current version as mentioned before never sweeps the anchors anyways so I think we need an improvement anyways here going forward ?
Was doing some tests locally, and the only thing we might to rethink, we will now basically always see this until all other outputs of the force close are swept, not sure if we want this logging (quite confusing):
Can we remove these log message somehow, given the fact that we never sweep the anchor currently it seems unnecesssary. So we will see this for every block which comes in ... |
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.
Can we remove these log message somehow, given the fact that we never sweep the anchor currently it seems unnecesssary.
Could make sense to just add that it's likely uneconomical to sweep. Otherwise LGTM ⚡
yeah wanna remove them too, sth maybe downgrading to warning - but again this would shadow the real urgent case where budget is too small for htlcs.
We don't want to leak abstraction here - the err logs come from the fee func, and it only knows the func but never interprets its broader context, like budget too small, or value being uneconomical. I think what we should do is to make sure to use the starting feerate to filter the inputs too, Lines 246 to 255 in 8f94929
Atm when grouping inputs, we would filter using the min relay fee, then the starting feerate, but only optionally. Basically what we need to do is to make this Lines 142 to 151 in 8f94929
|
We now make sure to sweep each anchor input in its own sweeping tx, if economically feasible.
Alternative to #10060, instead of grouping anchor inputs with others, we now sweep them independently. Given that the anchor resolver doesn't persist its state on disk, during restart, it's likely the anchor input will be offered to the sweeper again, causing the sweeping tx to fail due to missing inputs. If we happen to sweep other inputs in the same tx, they should be filtered out and retried; however, due to the async nature of spending notifications, they are likely to fail due to this check,
lnd/sweep/fee_bumper.go
Lines 688 to 700 in 4389067