Add NoopAdd HTLCs
#9871
Add NoopAdd HTLCs
#9871
Conversation
|
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 Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
GeorgeTsagk
force-pushed
the
htlc-noop-add
branch
from
2bf6f1f to
b3831da
Compare
|
NACK, in my opinion we should be very careful progressing in this direction:
|
Thanks for your attention and the review so far |
| if _, ok := customRecords[noopTLV]; ok { | ||
| entryType = NoopAdd | ||
| } | ||
|
|
There was a problem hiding this comment.
There isn't an error path here yet, but we should return an error if this type is used for any channel type other than the aux channels. This'll serve to contain the logic a bit.
There was a problem hiding this comment.
Why error out instead of no-op'ing the noop?
i.e
if lc.ChanType().HasTapscriptRoot() {
entryType = NoopAdd
}if this for any reason is happening over a normal channel, the TLV field will be ignored and won't change any state whatsoever
There was a problem hiding this comment.
if that happens to a normal channel which does not understand the TLV, you propose just treating it as a normal Add ? But I think we should error the channel if a peer tries to trick us into adding a NOOPADD for a normal channel.
There was a problem hiding this comment.
if that happens to a normal channel which does not understand the TLV, you propose just treating it as a normal Add
Yes, if for any reason (maybe even by accident) the noop gets signaled then we should simply revert to old behavior.
But I think we should error the channel if a peer tries to trick us into adding a NOOPADD for a normal channel.
Need to account for potential bugs in software. If for any reason the flag "escapes" the scope of being set only over custom channels we wouldn't want to cause force-closes.
There was a problem hiding this comment.
Why error out instead of no-op'ing the noop?
To make the failure explicit, as it should never happen. If we silently downgrade an unexpected noop HTLC (say from a legacy static key channel), then it may be harder to figure out what happened after the fact.
We can put this check in validateCommitmentSanity, as it's already possible that we'll error out if a peer or the switch sends us a bad/invalid HTLC.
There was a problem hiding this comment.
Need to account for potential bugs in software. If for any reason the flag "escapes" the scope of being set only over custom channels we wouldn't want to cause force-closes.
If that's the case we should let the FC happen so we know something is wrong, otherwise the bug will be sitting there silently forever.
GeorgeTsagk
force-pushed
the
htlc-noop-add
branch
from
b3831da to
0808083
Compare
|
Pushed a version which adds more docs, and simplifies the crediting logic Also in order to avoid violating the chan reserve check I now take into account the accumulated balance delta for the party we're performing the check for. So now we check if balance + delta exceeds reserve, to make the call on if we apply the noop. I added the "failure" path which reverts to old behavior Lines 10087 to 10100 in 0808083 |
|
|
||
| // TestNoopAddSettle tests that adding and settling an HTLC with no-op, no | ||
| // balances are actually affected. | ||
| func TestNoopAddSettle(t *testing.T) { |
There was a problem hiding this comment.
could you also add a test which tests the case with the reserve limit ?
There was a problem hiding this comment.
Great recommendation, added a test that exposes the behavior around the channel reserve
There was a problem hiding this comment.
Combined with my previous comment (about extracting the logic into a standalone function), I think we should do a table-driven test here that's as exhaustive of edge cases as possible. We really don't want to ever run into an integer underflow or similar situation.
|
I think this change before further work needs an design ACK from @yyforyongyu who has the most expertise in the htlcswitch package. So I would keep myself as a third reviewer. |
|
For any new TLV, I think they need to be properly documented in https://github.com/Roasbeef/blips/blob/tap-blip/blip-tap.md and any other relevant places. The taproot assets protocol is getting more and more hard to understand because of missing definitions (see also lightninglabs/taproot-assets#1446). |
lnwallet/channel.go
Outdated
| @@ -4489,6 +4553,14 @@ func (lc *LightningChannel) ProcessChanSyncMsg(ctx context.Context, | |||
| // Next, we'll need to send over any updates we sent as part of | |||
| // this new proposed commitment state. | |||
| for _, logUpdate := range commitDiff.LogUpdates { | |||
| //nolint:ll | |||
| if htlc, ok := logUpdate.UpdateMsg.(*lnwire.UpdateAddHTLC); ok { | |||
| delete(htlc.CustomRecords, uint64(NoopHtlcType.TypeVal())) | |||
There was a problem hiding this comment.
Same comment as above, we need to make sure retransmission of the noop HTLC works properly. If our sig covers the HTLC as a noop, but we don't send the TLV in the add message, then we'll trigger a force closed based on signature mismatch.
There was a problem hiding this comment.
I added a test case to TestChanSyncOweCommitment that uses the noop record in the appropriate channel type
After removing these leftover lines it seems to be working as expected, with the channel remaining alive and the balances being as expected
There was a problem hiding this comment.
Didn't want to add a whole new test specifically for noop HTLCs, as it would introduce a lot of unnecessary diff.
But if this test case is not sufficient in terms of coverage can always add more tests
GeorgeTsagk
force-pushed
the
htlc-noop-add
branch
from
7b25935 to
13b9ffb
Compare
There was a problem hiding this comment.
Did a first pass, mostly to load some context. Looks pretty contained, which is super nice.
Just to verify my understanding: The current version will still actually send and credit the HTLC to the receiver until the receiver has a satoshi balance over their reserve? And only then will it really become a no-op?
And if the no-op goes on chain, it looks and behaves the same way as a normal "add" HTLC?
|
|
||
| // TestNoopAddSettle tests that adding and settling an HTLC with no-op, no | ||
| // balances are actually affected. | ||
| func TestNoopAddSettle(t *testing.T) { |
There was a problem hiding this comment.
Combined with my previous comment (about extracting the logic into a standalone function), I think we should do a table-driven test here that's as exhaustive of edge cases as possible. We really don't want to ever run into an integer underflow or similar situation.
Yes it should behave as a normal add HTLC. Parallel HTLCs will each have an non-dust output on the commitment transaction, but once settled off-chain, we'll credit the sender. There's another optimization that we can do here to instead just aggregate many aux HTLCs into a single one progressively. The need to have similar time locks to do it safely though. Going even further, there's a more elaborate path where there's a single HTLC that commits to a tree of future HTLCs, CTV style. I think that's out of scope for now, as we'd want to add something like that to the BOLT specs themselves, as they're a way to effectively make commitment transactions never grow in size with additional HTLCs, which makes the process of just force closing much cheaper. |
GeorgeTsagk
force-pushed
the
htlc-noop-add
branch
from
13b9ffb to
ed6a7ab
Compare
| lnwire.NewMSatFromSatoshis(tc.remoteBalance) | ||
| } | ||
|
|
||
| aliceChan.evaluateNoOpHtlc( |
There was a problem hiding this comment.
Unfortunately this unit test doesn't seem to be able to actually catch the edge cases it should.
Perhaps we need to refactor the balanceAboveReserve to make sure no over or underflows happen.
Because currently there clearly is an invalid cast (delta := lnwire.MilliSatoshi(balanceDeltas.GetForParty(party))) that leads to a value overflow:
There was a problem hiding this comment.
Unfortunately this unit test doesn't seem to be able to actually catch the edge cases it should.
The table driven cases are pretty exhaustive at this point (after adding the negative deltas cases -- which is what should expose the above behavior)
The thing is that the underflow effectively caused the right subtraction result, that's why the tests were happy.
https://go.dev/play/p/DvsoAr8lUiv
Although this worked, I'd prefer not to rely on the underflow behavior of golang (even though well defined)
I switched this to be identical to how we handle negative deltas in a different part of existing codebase:
Lines 4729 to 4738 in 76bdb1e
There was a problem hiding this comment.
So reverted the changes caused by this comment and now the delta remains as int64 all the way until balanceAboveReserve
GeorgeTsagk
force-pushed
the
htlc-noop-add
branch
from
76bdb1e to
4f0d187
Compare
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a "noop add" HTLC type, which is a clever mechanism for updating channel state via an HTLC that effectively returns its value to the sender upon settlement.
My review has identified a critical bug related to a potential integer underflow in balance calculations, a high-severity issue with a misleading comment, and a medium-severity issue regarding an informal comment. Addressing these points will significantly improve the robustness and maintainability of this new feature.
lnwallet/channel.go
Outdated
| func balanceAboveReserve(party lntypes.ChannelParty, delta int64, | ||
| channel *channeldb.OpenChannel) bool { | ||
|
|
||
| channel.RLock() | ||
| defer channel.RUnlock() | ||
|
|
||
| c := channel | ||
|
|
||
| localReserve := lnwire.NewMSatFromSatoshis(c.LocalChanCfg.ChanReserve) | ||
| remoteReserve := lnwire.NewMSatFromSatoshis(c.RemoteChanCfg.ChanReserve) | ||
|
|
||
| switch { | ||
| case party.IsLocal(): | ||
| totalLocal := c.LocalCommitment.LocalBalance | ||
| if delta >= 0 { | ||
| totalLocal += lnwire.MilliSatoshi(delta) | ||
| } else { | ||
| totalLocal -= lnwire.MilliSatoshi(-1 * delta) | ||
| } | ||
|
|
||
| return totalLocal > localReserve | ||
|
|
||
| case party.IsRemote(): | ||
| totalRemote := c.RemoteCommitment.RemoteBalance | ||
| if delta >= 0 { | ||
| totalRemote += lnwire.MilliSatoshi(delta) | ||
| } else { | ||
| totalRemote -= lnwire.MilliSatoshi(-1 * delta) | ||
| } | ||
|
|
||
| return totalRemote > remoteReserve | ||
| } | ||
|
|
||
| return false | ||
| } |
There was a problem hiding this comment.
There's a potential for an integer underflow here. totalLocal is a lnwire.MilliSatoshi (which is a uint64), and delta is an int64. If delta is negative and its absolute value is greater than totalLocal, the subtraction totalLocal -= lnwire.MilliSatoshi(-1 * delta) will underflow. This would cause incorrect balance calculations, potentially leading to channel failures.
To prevent this, all balance calculations involving deltas should be performed using signed integers before converting back to lnwire.MilliSatoshi. The same issue exists for the remote party's balance calculation.
switch {
case party.IsLocal():
// To avoid underflows, we'll do the balance arithmetic using
// signed integers.
newBalance := int64(c.LocalCommitment.LocalBalance) + delta
if newBalance < 0 {
return false
}
return lnwire.MilliSatoshi(newBalance) > localReserve
case party.IsRemote():
// To avoid underflows, we'll do the balance arithmetic using
// signed integers.
newBalance := int64(c.RemoteCommitment.RemoteBalance) + delta
if newBalance < 0 {
return false
}
return lnwire.MilliSatoshi(newBalance) > remoteReserve
}There was a problem hiding this comment.
I think we know that the negative delta can never be greater than the balance, as that would indicate a greater issue in the lightning channel state machine. We verify what the balance is before creating the add entry in the log update:
Lines 6018 to 6030 in 4f0d187
If that was the case we'd run into it here too:
Lines 4743 to 4752 in 4f0d187
I'm not against adding extra checks, but that would mean we'd have to change a few func signatures to return errors.
There was a problem hiding this comment.
I think we know that the negative delta can never be greater than the balance, as that would indicate a greater issue in the lightning channel state machine. We verify what the balance is before creating the add entry in the log update:
A comment that explains the assumption here would be appropriate.
Otherwise someone coming across this in the future will have to dig through a lot of stuff to find out why this is safe.
GeorgeTsagk
force-pushed
the
htlc-noop-add
branch
from
4f0d187 to
9fedbd6
Compare
lnwallet/aux_signer.go
Outdated
|
|
||
| // NoopAddHtlcType is the (golang) type of the TLV record that's used to signal | ||
| // that an HTLC should be a noop HTLC. | ||
| type NoopAddHtlcType = tlv.TlvType65544 |
There was a problem hiding this comment.
why are we defining the same type twice?
There was a problem hiding this comment.
the previous one is a var
var NoOpHtlcType tlv.TlvType65544
this one is a golang type
type NoopAddHtlcType = tlv.TlvType65544
I think I can do better with naming, I can rename the first one to NoOpHtlcTLVEntry and second one to NoOpHtlcTLVType
| // balanceAboveReserve checks if the balance for the provided party is above the | ||
| // configured reserve. It also uses the balance delta for the party, to account | ||
| // for entry amounts that have been processed already. | ||
| func balanceAboveReserve(party lntypes.ChannelParty, delta int64, |
There was a problem hiding this comment.
I'm not sure I follow this check - since it's only called when we are processing the Settle, and before that there must be an Add. To lock in that Add, we must have already checked that the channel balance wouldn't drop below the reserve, plus a few other checks such as the fee buffer. So this method should always return true? Unless we don't check for reserves when processing tap HTLCs?
There was a problem hiding this comment.
To lock in that Add, we must have already checked that the channel balance wouldn't drop below the reserve,
This would be for the remote side. If Alice is sending to Bob, here we want to check if Bob is below reserve. Currently if Bob is at 0% (no reserve) we can still send him 1sat even if that doesn't take him above the configured reserve. The noop-add will actually nullify the send only if Bob is sitting above reserve.
This requirement is related to how tap assets work, as we need to make sure we have a sufficient amount in order to anchor off-chain metadata that needs to be present on-chain.
| channel *channeldb.OpenChannel) bool { | ||
|
|
||
| channel.RLock() | ||
| defer channel.RUnlock() |
There was a problem hiding this comment.
This usually means a method should exist on the struct itself.
There was a problem hiding this comment.
I moved it here after addressing a recommendation in a previous comment
#9871 (comment)
Happy to change it again
GeorgeTsagk
force-pushed
the
htlc-noop-add
branch
from
9fedbd6 to
579cd29
Compare
|
@Roasbeef: review reminder |
There was a problem hiding this comment.
I think this new type adds lots of complexity to the existing flow, and the expectation from the usage of Noop adds must be explicitly documented (maybe in tapd). I think the purpose of noop can be defeated via multiple ways,
- if the remote sets a large channel reserve, sats will always be claimed by the receiver.
- if the remote receives lots of payments and refuses to settle, they can claim them via FC - this can be mitigated via restricting the num of inflight payments.
- if the custom channel is used to send normal HTLCs, the receiver is in danger as the payments can be claimed back.
I guess these are handled in the tapd side?
| totalLocal -= lnwire.MilliSatoshi(-1 * delta) | ||
| } | ||
|
|
||
| return totalLocal > localReserve |
There was a problem hiding this comment.
I think it doesn't really matter if we're greater vs greater-equal than the value here, worst case we need to send an extra 1msat to pass the check
What really matters here is that both peers perform the same check with the same values, as there absolutely needs to be an agreement here on whether the check is satisfied (otherwise it would lead to balance disagreements -> conflicting commitments -> force closure)
Added some context around this in the code too
| } | ||
|
|
||
| if noopFlag && !chanType.HasTapscriptRoot() { | ||
| lc.log.Warnf("Received flag for noop-add over a channel that " + |
There was a problem hiding this comment.
Let's make this an error since it's dangerous for the receiver? IIUC in tap chan we can also send normal HTLCs, I'm wondering if the receiver can refuse receiving noop Adds, say via a config. Or is there a way to check that if it's sending normal htlcs we disable it?
There was a problem hiding this comment.
One important thing to keep in mind here is that all of the code introduced in this PR is not used unless external software (i.e tapd in our case) explicitly tells LND to treat this as a noop HTLC (via the noop TLV). The control guards around whether the TLV is set lies externally.
Specifically we only set it if the intent of the tap payment is clearly to send assets and not sats. If a normal (sats) payment is initiated we don't set the flag.
There was a problem hiding this comment.
I'm wondering if the receiver can refuse receiving noop Adds, say via a config
If we sign as if we're doing it, but they don't, then that leads to a force close just like if any of the other channel rendering attributes are ignored.
Or is there a way to check that if it's sending normal htlcs we disable it?
When this rolls out, there'll be a new feature bit used to signal if both sides understand the feature.
| // also verify that the noop-adds will start the nullification only once | ||
| // Bob is above reserve. | ||
| reserveTarget := (numHtlc / 2) * htlcAmt | ||
| bobReserve := bobBalance + reserveTarget |
There was a problem hiding this comment.
Does it mean an attacker can just increase their chan reserve and receive sats that are supposedly not credited to them in the noop Add context, hence the sender may falsely think they are getting the sats back?
There was a problem hiding this comment.
Interesting point, but again given that this is only used in the context of custom channels, and specifically since the TLV flag will only be set for HTLCs that carry dust amt plus metadata the exposure is not very large.
I guess we could add some guard during the custom channel funding against the remote setting a very large reserve in order to totally eliminate this threat.
There was a problem hiding this comment.
To add to this, the reserve of a channel is set by the opposite party. So a peer can't unilaterally pick a very large reserve. It's also the case that this is beneficial to both parties, since it enables sending without continually pushing another right-above-dust value to the opposite party.
As we're the ones that accept the reserve proposals, we'll reject it if we consider the reserve too high, as that impacts the usability of the channel.
Thanks for the great points @yyforyongyu I'd like to reply to the last 2 bullets: The fact that the HTLCs still actually carry the amount while in-flight is also sort of a feature. We want to be able to materialize the HTLCs on chain if that needs to happen, as the metadata that are present in the outpoint must be preserved (that's where the state of the custom channels resides). Below dust amounts would contribute towards fees and the HTLCs would get lost into the void.
Yes, the custom channel can be used to send normal HTLCs but it would never do so and also set the noop TLV flag for it. That's a strong responsibility of the external software -- to make sane decisions around when an HTLC should be treated as a noop. |
GeorgeTsagk
force-pushed
the
htlc-noop-add
branch
from
579cd29 to
0e84b7d
Compare
We add a new update type to the payment descriptor to describe this new type of htlc. This type of HTLC will only end up being set if explicitly signalled by external software.
We also add the IsAdd helper to the AuxHtlcDescriptor, as external software using the aux framework might want to know which type of HTLC this is.
We update the lightning channel state machine in some key areas. If the noop TLV is set in the update_add_htlc custom records then we change the entry type to noop. When settling the HTLC if the type is noop we credit the satoshi amount back to the sender.
Adds some simple tests to check the noop HTLC logic of the lightning channel.
To make sure we don't cause force-closures because of commit sig mismatches, we add a test case to verify that the retransmitted HTLC matches the original HTLC.
GeorgeTsagk
force-pushed
the
htlc-noop-add
branch
from
0e84b7d to
97bbbf1
Compare
|
Rebased on latest |
There was a problem hiding this comment.
LGTM⛵️
For future references, this PR adds a new feature via introducing a new htlc type, NoOp, which allows carrying info via HTLCs without extra cost in sats. The sender pays sats as normal when sending HTLCs, and the receiver will return the sats back via settlements. To safely use this feature, the custom channels must make sure that,
- limit the remote's channel reserve to a sane value, to avoid
NoOpnot taking effect. - the receiver must be careful when receiving normal sats in custom channels, as it can create fake settlement of invoices.
There was a problem hiding this comment.
Looking really good!
I think only more thing is missing on this PR. We should add an explicit check that if the remote party sends an HTLC w/ this special TLV active, and we aren't using the overlay channel type, we reject it. This is an extra defensive check to further firewall this feature to ensure it only applies to this special channel type.
| // don't need to record the amount as it | ||
| // was never sent over to the other | ||
| // side. | ||
| if u.noOpSettle { |
| } | ||
|
|
||
| if noopFlag && !chanType.HasTapscriptRoot() { | ||
| lc.log.Warnf("Received flag for noop-add over a channel that " + |
There was a problem hiding this comment.
We should return an error here instead.
There was a problem hiding this comment.
I can't really see why this is needed:
-
Assuming that both parties run LND and the same checks then both will result in treating this as a normal HTLC. This code line here is purely informational, it's not that we simply log if the remote party tried "cheating". The noop flag only signals the intention for an HTLC to be treated as noop, but the checks may render it a normal HTLC -- and that is the above case, an HTLC that set the noop flag ended up being reverted to a normal HTLC and we just let the user know. If on the other hand the two parties run different software and disagree on whether an HTLC should be a noop, then they'd force close, and that's also something we'll try to avoid.
-
Returning an error here means we allow for a couple of call sites to fail for a not really crucial reason. It is possible that the intention to create a noop is signaled, but the criteria are just not met. I'm not confident that we should error out all these call sites for something that is predicted behavior:
diskHtlcToPayDesc,logUpdateToPayDesc,remoteLogUpdateToPayDesc,htlcAddDescriptor,ReceiveHTLC
Correct that since we only apply the noop on settle, each pending HTLC still needs that above-dust-amount to anchor it to the commitment transaction. If they go to chain, then the receiver can pull the committed amount, and also that above-dust-amount. The only direct way to get rid of this final limitation would be to switch to a zero-value output. However, those are disallowed by default policy. May we need the degens to
So normal HTLCs wouldn't be marked as noop. Only these special HTLCs would be, currently we drive that type based on the set of custom records we see. Another approach here would be to add another aux interface to allow that check to be dynamic. Left a comment to error out instead of warn to make this case explicit. |
Description
Adds a new type of HTLC named "noop add", which behaves identically to normal HTLCs except for the settling part. If upon settlement the receiver has an above dust balance, then the amount is returned back to the sender and the only thing that ends up being updated is the aux leaf of the commitment, which will successfully reflect any overlay changes.
Replacement for #9430