-
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
sweeper: relax anchor sweeping when there's no deadline pressure #7965
sweeper: relax anchor sweeping when there's no deadline pressure #7965
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.
Concept ACK, I think this is a good way forward, but we need to maybe force some second level htlc transaction now ?
Based on eugene's input, I think we will have problems with HTLC-Success Transactions because we should only sweep them if they are economical and they might have zero-fees attached which means they need wallet utxos (aggregating seems unlikely to work or is not deterministic enough)
"output=%v, limit=%v", inp, reqOut.Value, | ||
dustLimit) | ||
|
||
// TODO(yy): we should not return here for force |
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 you mean we should take care of this case before this function, and if we registered an input with a required dust output we just continue and anticipate the sweep to fail ?
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 imo we should return an error here - so that the caller know this would fail.
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 case we remain with the default mempool feelimit of bitcoind (3000 sats/vbyte) I think this case should be marked as an error, because that means our HTLC acceptor engine did create an HTLC output although it should have be trimmed away ?
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.
But I think we cannot just continue for Force-Sweeps as well if we hit this case, because the input is signed with the SIGHASH-Single|AnyoneCanPay or what do you mean exactly by this comment?
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.
But I think we cannot just continue for Force-Sweeps as well if we hit this case, because the input is signed with the SIGHASH-Single|AnyoneCanPay
What do you mean?
In case we remain with the default mempool feelimit of bitcoind (3000 sats/vbyte) I think this case should be marked as an error
Yeah agree, changed to log an error instead.
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 I was not 100% in regards of the comment, why do you want to distinguish here for force and non-forced sweeps and why should we not return early if we figure out that this is a required dust-output ?
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 really depends on how we define force
. Atm it means to collect the input although it may be uneconomical, while I think what it really means is, this output needs to be swept because it has other implications, such as CPFP the closing tx, and we should make it happen and fail loudly when it fails.
return nil | ||
} | ||
|
||
// For force adds, no further constraints apply. | ||
// | ||
// NOTE: because the inputs are sorted with force sweeps being placed |
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: I would intuitively pack this not in the constraintsRegular
section.
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.
you mean to put it there or not to?
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 I am not sure if your comment holds when a new regular Input has such a size that at the current fee-level would be negative yielding ?
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.
If it's negative yielding it should not be added?
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 was just a misunderstanding on my side in regards of the comment. Maybe something like this:
// In other words, though we may have negative `changeOutput` from
// including force sweeps, `inputYield` should always increase when
// adding regular inputs which are positive yielding`
@@ -393,6 +394,12 @@ func (t *txInputSet) tryAddWalletInputsIfNeeded() error { | |||
return err | |||
} | |||
|
|||
// Sort the UTXOs by putting smaller values at the start of the slice | |||
// to avoid locking large UTXO for sweeping. | |||
sort.Slice(utxos, func(i, j int) bool { |
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 do this in the wallet lib maybe or what be really slick connect it to the lnd parameteer where we select random or ordered wallet inputs: https://github.com/lightningnetwork/lnd/blob/master/sample-lnd.conf#L149
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.
oh yeah forgot about that config! added a todo here and we can fix it in later refactoring PRs.
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 you think doing this sorting in the btcwallet lib is not really worth it? I am good with both
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.
btcwallet
does the sorting rn when it does its own coin selection for SendCoins
. The existing behavior for ListUnspent
is to return in decreasing order IIUC. We can thread through a new arg to allow that behavior to be modified, but don't think it's a blocking factor here.
We always need to sweep them (assuming you're referring to second level HTLC transactions). Otherwise if we don't, then we can trigger loss of funds if the outgoing link sweeps with preimage (after we go on chain), and we fail to sweep in the incoming link (after we had force closed for w/e reason). |
// can be used in two sweeping transactions, and our rebroadcaster will | ||
// retry the failed one. We should instead understand why the input is | ||
// failed in the first place, and start tracking input states in | ||
// sweeper to avoid 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.
Yeah for the full-node backends, in theory we can either try to parse the error message to figure out why the input was rejected, or just re-publish and bisect to see why the transaction was rejected. A third route would be starting to use tstmempoolaccept
, which I think returns structured information w.r.t why something wouldn't have been accepted.
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 fix this in the coming PRs which improve our sweeper.
sweep/tx_input_set.go
Outdated
func (t *txInputSet) tryAddWalletInputsIfNeeded() error { | ||
// Skip adding wallet input if this is not a force sweep. | ||
if !t.force { |
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.
If what we want here is to reduce unnecessary UTXO consumption by the sweeper due to stuff like anchors, then we can remove this section here that preemptively sends the anchor sweeps to the sweeper upon force close:
lnd/contractcourt/channel_arbitrator.go
Lines 1112 to 1132 in 90effda
// The commitment transaction has been broadcast, but it | |
// doesn't necessarily need to be the commitment | |
// transaction version that is going to be confirmed. To | |
// be sure that any of those versions can be anchored | |
// down, we now submit all anchor resolutions to the | |
// sweeper. The sweeper will keep trying to sweep all of | |
// them. | |
// | |
// Note that the sweeper is idempotent. If we ever | |
// happen to end up at this point in the code again, no | |
// harm is done by re-offering the anchors to the | |
// sweeper. | |
anchors, err := c.cfg.Channel.NewAnchorResolutions() | |
if err != nil { | |
return StateError, closeTx, err | |
} | |
err = c.sweepAnchors(anchors, triggerHeight) | |
if err != nil { | |
return StateError, closeTx, err | |
} |
We don't set those to Force because it makes no sense to sweep them if it's uneconomical
This isn't always the case. Normally yes, but if we did make the decision to go to chain, then we must sweeps those HTLCs as otherwise we won't be able to cancel back the HTLC on the incoming link. If mempools are congested, then this can lead to cascading force closes.
This PR attempts to short circuit that and avoid going to chain at all. But if we did decide to go to chain, then we must force close.
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 current code doesn't set it to Force though, so the 2nd-level won't happen if it's not economical? As far as #7685 goes, I think it makes more sense to abandon the sweep and give up on the incoming HTLC because then you're not losing value_of_outgoing_htlc + negative_outgoing_sweep and instead just lose value_of_outgoing_htlc
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 isn't always the case. Normally yes, but if we did make the decision to go to chain, then we must sweeps those HTLCs as otherwise
Does this not only apply for Outgoing HTLCs, why should we care for HTLC-Success transactions if they are not worth sweeping economically ? There should be no incoming link at risk for the Success-Tx ?
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.
My bad, I was looking at a branch before #7966 was applied. So this should only apply to the incoming side. However, I still don't think it makes sense to sweep the outgoing HTLC at a loss when we should just take the loss and not sweep it if possible
Re the anchor issues, right now we always add lnd/contractcourt/channel_arbitrator.go Lines 1325 to 1345 in 90effda
Instead, we should only upgrade to force once we think it's actually needed. Implementation wise, this may allow us to bite off a smaller piece of the sweeper deadline awareness, but only for anchor outputs. Rather than insert this at the core of the sweeper, we'd have the anchor resolver listen for block epochs, then re-offer the anchor inputs with There might be a slight danger here though, if by the time we realize we need to bump the fee, the transactions has already fallen out of the mempool. IMO, this'll always apply to any scenario related to fee bumping, as without package relay, we can't ensure that the parent is still "in the mempool". Thoughts? |
The anchor resolve is then created+launched after something confirms. This one tries to sweep at 1 sat/byte: lnd/contractcourt/anchor_resolver.go Lines 87 to 122 in 90effda
I think the primary goal of this PR was to stop this variant (which is always offered) from taking up wallet input to confirm the various versions? Alternatively, we can also try to cancel the old anchor versions once something confirms. Once a commitment confirms, we know exactly which versions we want to sweep, so we can give up on the others. |
a6aba91
to
13a226d
Compare
We need to track why a potential Force-Close is failing to get into mempool, because we might want to launch the CPFP operation of the Remote Commitment if our FC is not allowed into mempool because another transaction already spents the funding output? Meaning that we need launch also the CPFP operations for the Remote Commitment (+remote dangling one) or at least check whether those are in our mempool prior to launching the Anchor CPFPs? |
11c6283
to
bfb0554
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.
The latest set of changes makes sense to me. itests failing atm
54e8cab
to
a3dd675
Compare
Updated the PR to relax on deadline-aware anchor sweeping. It changes the sweeper from always sweeping anchor for a local force close to only do so when there is an actual time pressure. This doesn't completely solve the issue that we may lock up a wallet UTXO to sweep an anchor using a very low fee rate - need more investigation and possibly a refactor on the sweeper.
This is true prior to the fee spikes. I think after the fee spikes, it becomes conditional as it can be uneconomical to sweep the HTLC.
Think this is handled by our rebroadcaster? |
@Roasbeef: review reminder |
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 but its close 👍
"output=%v, limit=%v", inp, reqOut.Value, | ||
dustLimit) | ||
|
||
// TODO(yy): we should not return here for force |
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 case we remain with the default mempool feelimit of bitcoind (3000 sats/vbyte) I think this case should be marked as an error, because that means our HTLC acceptor engine did create an HTLC output although it should have be trimmed away ?
"output=%v, limit=%v", inp, reqOut.Value, | ||
dustLimit) | ||
|
||
// TODO(yy): we should not return here for force |
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.
But I think we cannot just continue for Force-Sweeps as well if we hit this case, because the input is signed with the SIGHASH-Single|AnyoneCanPay or what do you mean exactly by this comment?
return nil | ||
} | ||
|
||
// For force adds, no further constraints apply. | ||
// | ||
// NOTE: because the inputs are sorted with force sweeps being placed |
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 I am not sure if your comment holds when a new regular Input has such a size that at the current fee-level would be negative yielding ?
when the CPFP requirements are met. This is now | ||
[changed](https://github.com/lightningnetwork/lnd/pull/7965) to only attempt | ||
CPFP based on the deadline of the commitment transaction. The anchor sweeping | ||
won't happen unless a relevant HTLC will timeout in 144 blocks. |
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: unless a relevant HTLC will timeout in less than 144 blocks ?
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.
Will the change to not offer the anchor output at all to the sweeper mean that users won't be able to use BumpFee
anymore to CPFP before that deadline is hit?
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.
very good point, you are right we would not be able to bump the commitment transaction here (because we only accept it if it's in our pending_sweep map):
https://github.com/lightningnetwork/lnd/blob/master/sweep/sweeper.go#L1537-L1540
I think thats something we don't want (imo)
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 unless the deadline is 144 blocks away, this anchor sweep won't be attempted - I think another option is to make the 144
blocks configurable.
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.
actually i think you can still use BumpFee
. When the deadline is more than 144 blocks away, the anchor input will still be sent to our sweeper, the difference is the force flag won't be used.
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.
Ahh correct, oversaw that. So it will be able to bump the closing transaction cool !
itest/lnd_multi-hop_test.go
Outdated
// deadline pressure for Bob on the channel Alice->Bob at the moment. | ||
// For Bob's local commitment tx, there's only one incoming HTLC which | ||
// he doesn't have the preimage yet. Thus this anchor won't be | ||
// force-swpet. |
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/swpet/swept/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
// For Bob's local commitment tx, there's only one incoming HTLC which | ||
// he doesn't have the preimage yet. Thus this anchor won't be | ||
// force-swpet. | ||
hasAnchorSweep := false |
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: maybe call this hasAnchorCpfpSweep ?
itest/lnd_channel_backup_test.go
Outdated
ht.MineBlocksAndAssertNumTxes(1, 1) | ||
blocksMined++ | ||
} | ||
|
||
// After Carol's output matures, she should also reclaim her | ||
// funds. | ||
// | ||
// The commit sweep resolver publishes the sweep tx at | ||
// defaultCSV-1 and we already mined one block after 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.
Nit: I think the comment is not quite right, we did already mine more than 1 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.
yeah fixed - think we should avoid testing multiple commitment types in a single test in the future, it's a bit too involved to maintain🤦🏻
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.
very close, just think the comment about dust should be removed
@@ -393,6 +394,12 @@ func (t *txInputSet) tryAddWalletInputsIfNeeded() error { | |||
return err | |||
} | |||
|
|||
// Sort the UTXOs by putting smaller values at the start of the slice | |||
// to avoid locking large UTXO for sweeping. | |||
sort.Slice(utxos, func(i, j int) bool { |
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.
btcwallet
does the sorting rn when it does its own coin selection for SendCoins
. The existing behavior for ListUnspent
is to return in decreasing order IIUC. We can thread through a new arg to allow that behavior to be modified, but don't think it's a blocking factor here.
// Also signal that this is a force sweep, so that the anchor | ||
// will be swept even if it isn't economical purely based on the | ||
// anchor value. | ||
// preference. Because this is a cpfp-operation, the anchor |
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 what we want to start to do there is actually use testmempoolaccept
before we broadcast things. So we become less "spray and pray" in general.
when the CPFP requirements are met. This is now | ||
[changed](https://github.com/lightningnetwork/lnd/pull/7965) to only attempt | ||
CPFP based on the deadline of the commitment transaction. The anchor sweeping | ||
won't happen unless a relevant HTLC will timeout in 144 blocks. |
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.
Will the change to not offer the anchor output at all to the sweeper mean that users won't be able to use BumpFee
anymore to CPFP before that deadline is hit?
a3dd675
to
ce9d903
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.
Important PR, LGTM 🚀
"output=%v, limit=%v", inp, reqOut.Value, | ||
dustLimit) | ||
|
||
// TODO(yy): we should not return here for force |
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 I was not 100% in regards of the comment, why do you want to distinguish here for force and non-forced sweeps and why should we not return early if we figure out that this is a required dust-output ?
return nil | ||
} | ||
|
||
// For force adds, no further constraints apply. | ||
// | ||
// NOTE: because the inputs are sorted with force sweeps being placed |
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 was just a misunderstanding on my side in regards of the comment. Maybe something like this:
// In other words, though we may have negative `changeOutput` from
// including force sweeps, `inputYield` should always increase when
// adding regular inputs which are positive yielding`
ce9d903
to
caea3b7
Compare
This commit sorts wallet UTXOs by their values when using them for sweeping inputs. This way we'd avoid locking large UTXOs when sweeping inputs and also provide an opportunity to aggregate wallet UTXOs.
This commit changes from always sweeping anchor for a local force close to only do so when there is an actual time pressure. After this change, a forced anchor sweeping will only be attempted when the deadline is less than 144 blocks.
Since we now only perform CPFP when both the fee rate is higher and the deadline is less than 144, we need to update the test to reflect that Bob will not CPFP the force close tx for the channle Alice->Bob.
This commit updates all related tests to reflect the latest anchor sweeping behavior. Previously, anchor sweeping is always attempted as CPFP when a force close is broadcast, while now it only happens when the deadline is less than 144. For non-CPFP purpose sweeping, it will happen after one block is mined after the force close transaction is confirmed as the anchor will be resent to the sweeper with a floor fee rate, hence making it economical to sweep.
caea3b7
to
06cca85
Compare
updated release notes |
This PR prepares for #7549. It changes from always sweeping anchor for a local force close to only do so when there is an actual time pressure. After this change, a forced anchor sweeping will only be attempted when the deadline is less than 144 blocks.
A second change is, when supplying a wallet utxo to a force sweeping, we'd always try the smallest utxo first, which provides a chance to aggregate the small wallet utxos and keep the larger utxos available for other purposes.
There's also an attempt to make the anchor sweeping configurable #7939, which I think we could instead make it configurable so that users can decide to attach a wallet utxo to sweep the anchors or not.
NOTE: itest won't pass atm, looking for feedback first.