-
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
[8/5]: update BumpFee
RPC and always sweep on startup
#8613
[8/5]: update BumpFee
RPC and always sweep on startup
#8613
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 (
|
856dcad
to
13765a7
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.
Reviewed 9 of 9 files at r1, 6 of 6 files at r2, 1 of 1 files at r3, 9 of 9 files at r4, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @yyforyongyu)
cmd/lncli/walletrpc_active.go
line 238 at r3 (raw file):
Usage: "a manual fee expressed in sat/vbyte that " + "should be used when sweeping the output", Hidden: true,
Wait, so we want to force users to only use the budget?
I don't think that matches the existing mental model they have when they think of fee bumping. Now they'll need to look at their target fee rate, compute the weight of the txn, look at the output value then use that to arrive a the budget based on the total fee absolute fee at that rate.
lnrpc/walletrpc/walletkit.proto
line 1190 at r2 (raw file):
will be used to pay the fees specified by the budget. */ uint64 budget = 7;
I think we should be careful here not to break behavior for existing clients.
Or rather if it isn't set, then we should default back to the existing behavior of trying to meet the target fee rate as long as the sweep is still positive yield.
lnrpc/walletrpc/walletkit_server.go
line 975 at r2 (raw file):
} if in.Budget == 0 {
This'll break the call for all older clients that aren't aware of the new field. I think we should either try to pick a good default here (not sure how to do that tbh) or treat is as something purely optional to maintain the prior behavior.
c0f5ac9
to
b3c168a
Compare
13765a7
to
59d5580
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.
Reviewed 1 of 1 files at r3, 17 of 17 files at r5, 4 of 4 files at r6, 4 of 4 files at r7, 3 of 3 files at r8, 1 of 1 files at r9, 4 of 4 files at r10, 1 of 1 files at r11, 14 of 14 files at r12, 1 of 1 files at r13, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @yyforyongyu)
lnrpc/walletrpc/walletkit_server.go
line 958 at r9 (raw file):
// validateBumpFeeRequest makes sure the deprecated fields are not used when // the new fields are set. func validateBumpFeeRequest(in *BumpFeeRequest) (
👍
a0fe020
to
8d8b87d
Compare
8d8b87d
to
bac1198
Compare
b3c168a
to
3d699a9
Compare
3d699a9
to
bc4592c
Compare
bac1198
to
b82fcd3
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.
A couple of questions.
Can be rebased now to point to the main feature branch. |
itest/lnd_sweep_test.go
Outdated
tx := ht.SendCoins(alice, alice, btcutil.SatoshiPerBitcoin) | ||
txid := tx.TxHash() | ||
|
||
// Alice now tries to bump the first input on this 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.
input -> 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.
fixed
itest/lnd_sweep_test.go
Outdated
return pendingSweep.SatPerVbyte, sweepTx | ||
} | ||
|
||
// First bump request - we'll sepcify nothing except `Immediate` to let |
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.
sepcify -> specify
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
// the sweeper handles the fee, and we expect a fee func that has, | ||
// - starting fee rate: 1 sat/vbyte (min relay fee rate). | ||
// - deadline: 1008 (default deadline). | ||
// - budget: 50% of the input value. |
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.
Initially I thought this was more aggressive than the existing "positive yield" heuristic, but then I realized that the old version would be willing to use even 99% of the input valuer, as long as we were able to get even just 1 satoshsi out the other end:
Lines 232 to 270 in 1c229e4
// Calculate the yield of this input from the change in total tx output | |
// value. | |
inputYield := newSet.totalOutput() - t.totalOutput() | |
switch constraints { | |
// Don't sweep inputs that cost us more to sweep than they give us. | |
case constraintsRegular: | |
if inputYield <= 0 { | |
log.Debugf("Rejected regular input=%v due to negative "+ | |
"yield=%v", value, inputYield) | |
return nil | |
} | |
// For force adds, no further constraints apply. | |
// | |
// NOTE: because the inputs are sorted with force sweeps being placed | |
// at the start of the list, we should never see an input with | |
// constraintsForce come after an input with constraintsRegular. In | |
// other words, though we may have negative `changeOutput` from | |
// including force sweeps, `inputYield` should always increase when | |
// adding regular inputs. | |
case constraintsForce: | |
newSet.force = true | |
// We are attaching a wallet input to raise the tx output value above | |
// the dust limit. | |
case constraintsWallet: | |
// Skip this wallet input if adding it would lower the output | |
// value. | |
// | |
// TODO(yy): change to inputYield < 0 to allow sweeping for | |
// UTXO aggregation only? | |
if inputYield <= 0 { | |
log.Debugf("Rejected wallet input=%v due to negative "+ | |
"yield=%v", value, inputYield) | |
return nil | |
} |
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 behavior can be extremely aggressive, and nondeterministic.
sweep/aggregator.go
Outdated
locktime) | ||
|
||
// Init the slice if not found. | ||
inputList, ok := result[locktime] |
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 about trying to merge based on lock time compatibility? If something has a lock time of 5
, then it can be bundled with a transaction that has a lock time of 3
. Or would we rather optimize for inclusion into the mempool as soon as possible?
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 the nLockTime
s are different they cannot be bundled tho. I think most of the time this grouping logic won't be hit, as if the inputs have different locktimes would end up having different deadlines, and when we are here these inputs are already grouped by their deadlines, so they should share the same locktimes.
This is more of mitigation against this rare case brought by @morehouse here - that when two blocks arrive at the same time, it's possible we can end up mixing two inputs if their CLTVDelta
s have a difference of 1 block, which IMO even makes this even less likely to happen as I don't think people choose CLTVDelta this way.
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 about trying to merge based on lock time compatibility?
HTLC-Timeouts with differing locktimes cannot be batched since their signatures commit to different locktimes. But timeout path spends on the remote commitment can be batched if the current height exceeds all individual time locks, since the transaction can be re-signed.
I'm not sure whether this code allows batching in the second case or not.
... which IMO even makes this even less likely to happen as I don't think people choose CLTVDelta this way.
The case is unlikely to occur on accident. But an attacker could easily force it to happen so they can steal funds.
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 an attacker could easily force it to happen so they can steal funds.
How so?
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.
How so?
The attacker gets to choose CLTV deltas for each hop. LND will accept any delta in the range 80-2016 by default.
So it is trivial for the attacker to create HTLCs with the same deadlines and different timelocks:
- Attacker offers HTLC1 with incoming CLTV of
X
and outgoing CLTV ofX-82
. - Attacker offers HTLC2 with incoming CLTV of
X
and outgoing CLTV ofX-81
. - Attacker offers HTLC3 with incoming CLTV of
X
and outgoing CLTV ofX-80
.
Each outgoing HTLC has a different locktime, while the deadline for each to confirm is the same (X
)
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 config the incoming HTLC's cltv delta ourselves via --bitcoin.timelockdelta=
? plus the big assumption here is we need to see two blocks come in at the same time, otherwise they'd be in separate sweeps, which I won't say it's easy.
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 config the incoming HTLC's cltv delta ourselves via
--bitcoin.timelockdelta=
?
That only sets the value LND advertises. LND will actually accept HTLCs with larger deltas:
Lines 2861 to 2876 in 07b6af4
timeDelta := policy.TimeLockDelta | |
if incomingTimeout < outgoingTimeout+timeDelta { | |
l.log.Warnf("incoming htlc(%x) has incorrect time-lock value: "+ | |
"expected at least %v block delta, got %v block delta", | |
payHash[:], timeDelta, incomingTimeout-outgoingTimeout) | |
// Grab the latest routing policy so the sending node is up to | |
// date with our current policy. | |
cb := func(upd *lnwire.ChannelUpdate) lnwire.FailureMessage { | |
return lnwire.NewIncorrectCltvExpiry( | |
incomingTimeout, *upd, | |
) | |
} | |
failure := l.createFailureWithUpdate(false, originalScid, cb) | |
return NewLinkError(failure) | |
} |
plus the big assumption here is we need to see two blocks come in at the same time, otherwise they'd be in separate sweeps, which I won't say it's easy.
Yes, it's difficult to make that happen. And this PR currently prevents all batching on restart, so the attacker can't force a bad batch by crashing LND.
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'm not sure whether this code allows batching in the second case or not.
I don't think this PR takes advantage of the case that the remote HTLC can be swept with a locktime greater than and can therefore be bundled with other htlcs or ?
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.
That only sets the value LND advertises. LND will actually accept HTLCs with larger deltas:
Weird, as the comments say check for equality - think this is unintended?
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 think this PR takes advantage of the case that the remote HTLC can be swept with a locktime greater than and can therefore be bundled with other htlcs or ?
I think they are added via sweepMatureOutputs
.
64a5c05
to
504006c
Compare
504006c
to
80f05d1
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.
Reviewed 3 of 9 files at r1, 27 of 27 files at r14, 5 of 8 files at r15, 1 of 4 files at r16, 1 of 4 files at r17, 1 of 3 files at r18, 5 of 5 files at r19, 5 of 5 files at r20, 1 of 1 files at r21, 14 of 14 files at r22, 2 of 2 files at r23, 5 of 8 files at r25, 1 of 4 files at r26, 1 of 4 files at r27, 1 of 3 files at r28, 5 of 5 files at r29, 3 of 5 files at r30, 1 of 1 files at r31, 13 of 14 files at r32, 2 of 2 files at r33, 1 of 1 files at r34, 1 of 2 files at r35, 1 of 1 files at r36, all commit messages.
Reviewable status: 35 of 36 files reviewed, 8 unresolved discussions (waiting on @Roasbeef and @yyforyongyu)
itest/lnd_sweep_test.go
line 825 at r36 (raw file):
// 4. Wallet UTXOs requirements are met - neither Alice nor Bob needs wallet // utxos to finish their sweeps. func testSweepCommitOutputAndAnchor(ht *lntest.HarnessTest) {
This comment at the beginning is extremely helpful when reviewing the rest of the test. Thank you for putting it in!
itest/lnd_sweep_test.go
line 929 at r36 (raw file):
aliceChan := ht.GetChannelByChanPoint(alice, chanPoint) // TODO(yy): It looks like the funder is paying the fundee's anchor?
Does this mean our channel state machine has a bug?
Only one important question but otherwise I don't see anything wrong here. |
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.
LFG 🫡
I wonder if the following scenario is somehow critical in terms of efficiency: Imagine we have 2 inputs currently grouped together in an input set which already made it into the mempool. But the fee is too low so we decide to bump the fee one of the inputs, the currently logic (correct me if I am wrong or maybe there is already an itest for it) would in case the desired fee params are greater, create an RBF transaction with the bumped input (maybe some other inputs which are still pending and not part of an input set yet) and basically replace the old transaction from the mempool. Now the non-bumped input has basically no chance to be confirmed (maybe only if the fee-function and height delta increases the feerate more than the bump-request ?). It only gets regrouped in a new input set if the bumped transaction confirms. So maybe things we could do here:
Final questions, is the rebroadcaster always updated when new RBFs happen in the sweeper subsystem ? |
078dd0e
to
04868bb
Compare
This commit makes sure the time-sensitive outputs are swept immediately during startup.
04868bb
to
ca184d8
Compare
We will go with this option, as this will align with your suggestion on reconstructing the input set from mempool too. A rough process goes,
More thoughts, future stuff,
yea, the rebroadcast is cancel when a tx is confirmed, failed, or replaced. |
LGTM as soon as CI passes |
65a08e6
to
97e6a99
Compare
This commit adds a test case to check the no deadline sweeping behavior. The sweeper is updated to make sure it can handle the case for neutrino backend.
This commit changes how we transform from a deadline option to a concrete deadline value - previously this is done when we decide to cluster inputs, and we now move it to a step earlier - once an input is received via `SweeperInput`, we will immediately transform its optional deadline into a real value. For inputs that come with a deadline option, since the Some will be used, it makes no difference. For inputs with None as their deadlines, we need this change to make sure the default deadlines are assigned accurately.
97e6a99
to
e329503
Compare
e329503
to
5da30df
Compare
This is the final PR of the sweeper series, in this PR,
BumpFee
is updated to use the new params.contractcourt
now always performs an immediate sweep during startup, by leveraging the new flagImmediate
.Think this would be better than introducing a dual-time ticker to mitigate potential DoS vectors.
Looking for concept ACK.
I'm also thinking maybe we should add a new RPC so the user can easily ask the fee bumper to bump the fee by increasing the position used in the linear fee function, something in 0.18.1?
This change is