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 |
lnwallet/channel.go
Outdated
| if channel.BalanceAboveReserve(party) { | ||
| d := int64(entry.Amount) | ||
| balanceDeltas.ModifyForParty( | ||
| party.CounterParty(), | ||
| func(acc int64) int64 { | ||
| return acc + d | ||
| }, | ||
| ) | ||
| } else { | ||
| d := int64(entry.Amount) | ||
| balanceDeltas.ModifyForParty( | ||
| party, | ||
| func(acc int64) int64 { | ||
| return acc + d | ||
| }, | ||
| ) | ||
| } |
There was a problem hiding this comment.
This probably does work at least for LND's implementation but it does not have to because the ChannelState maschine for local and remote can diverge example:
- Local and Remote Commitment can diverge what you are now doing is the following:
Imagine Node A and Node B, Node B starts the channel with 0 balance.
a) Now A sends Node B and UpdateAdd (with NOOP set) and a CommitSig + Node B sends A the Revoke. All good so far. Node B is still below Reserve on both Node A's commitment and B's.
b) Node B send UpdateFullFill to Node A (NOOP ADD was credited to Node B, because B is below Reserve).
c) Node B sends also A the CommitSig for this new State. For Node A B is above reserve now on the local commitment.
d) Node A before sending Revoking his old state, sends another NOOP-ADD (everything works concurrently although for LND it will always immediately send the revoke).
e) Node B receives the New NOOP-ADD from A and will still credit it to Node A because when he settles it, he still is below is reserve on the local commitment, although he is already above the reserve on Node A.
Node A does look at it at the same way so both sides will do the same and the change works, but as mentioned above there are no guarantees that this will be done by other implementations. And I think it is super hard to document with all this "Ifs".
We might find a more lean way to do what you are describing. I think it is worth trying to investigate other design choices. This is a quick solution but it brings high burden probably to maintain in the future.
There was a problem hiding this comment.
e) Node B receives the New NOOP-ADD from A and will still credit it to Node A because when he settles it, he still is below is reserve on the local commitment, although he is already above the reserve on Node A.
Are you also assuming Node A sends a sig right after the noop add? As only once a sig is received will the views be evaluated to (maybe) apply the noop add.
IIUC, you're referring to the case where balances can diverge momentarily due to concurrent commit sig sends by both parties?
but as mentioned above there are no guarantees that this will be done by other implementations.
This is intended to be isolated to just aux channels. It doesn't solve a problem otherwise. As I commented below, we can gate this feature (reject commit if violated) on if the channel is an aux channel or not.
There was a problem hiding this comment.
We might find a more lean way to do what you are describing. I think it is worth trying to investigate other design choices. This is a quick solution but it brings high burden probably to maintain in the future.
The original stab at this involved the old trick we came up with way back to not have dust HTLCs at all: each send is actually two HTLCs, one in either direction, that nets out to the same asset balance. We started to implement that, but threw it out, as it has implications to the force close logic: an extra set of HTLCs to resolve when we go on chain per outstanding payment.
The high level problem this solves is that: sometimes you want to have an HTLC exist in the commitment transaction/logs, but not actually apply it. Just by existing, it may anchor some useful information for a higher level protocol.
This noop idea was attractive, as the diff is relatively small (can be made smaller with my comment above). It also doesn't change any of the chain handling logic. The trade off is that in the case of a force close, we can't do the noop trick off-chain, so the funds will go to the settler.
There was a problem hiding this comment.
Are you also assuming Node A sends a sig right after the noop add? As only once a sig is received will the views be evaluated to (maybe) apply the noop add.
Yes as far as I can tell there is no restriction that Alice needs to revoke her old commitment but just can send UpdatAdd + CommitSig to NodeB which as a consequence will always stay below the reserve on this commitment. As mentioned it will work with the current implementation but it will basically have this requirement which eventually we need to decide whether we are content with:
"A NOOPADD will add an HTLC to the receiving side if the Receiving Side is below it's channel reserve on the Receiving Commitment Transaction (Applies ONLY for Aux Channels)".
This is intended to be isolated to just aux channels. It doesn't solve a problem otherwise. As I commented below, we can gate this feature (reject commit if violated) on if the channel is an aux channel or not.#
Restricting it only to Aux Channels is definitely the way if we go. this route.
The original stab at this involved the old trick we came up with way back to not have dust HTLCs at all: each send is actually two HTLCs, one in either direction, that nets out to the same asset balance. We started to implement that, but threw it out, as it has implications to the force close logic: an extra set of HTLCs to resolve when we go on chain per outstanding payment.
Ahh ok yeah this does not seem better from a design perspective.
There was a problem hiding this comment.
Local and Remote Commitment can diverge what you are now doing is the following:
Imagine Node A and Node B, Node B starts the channel with 0 balance.
a) Now A sends Node B and UpdateAdd (with NOOP set) and a CommitSig + Node B sends A the Revoke. All good so far. Node B is still below Reserve on both Node A's commitment and B's.
b) Node B send UpdateFullFill to Node A (NOOP ADD was credited to Node B, because B is below Reserve).
c) Node B sends also A the CommitSig for this new State. For Node A B is above reserve now on the local commitment.
d) Node A before sending Revoking his old state, sends another NOOP-ADD (everything works concurrently although for LND it will always immediately send the revoke).
e) Node B receives the New NOOP-ADD from A and will still credit it to Node A because when he settles it, he still is below is reserve on the local commitment, although he is already above the reserve on Node A.
Not sure if your example still applies if you don't consult your local commitment for the remote party's balance, but the remote one instead?
This is what the BalanceAboveReserve helper does now
Lines 2139 to 2145 in b3831da
There was a problem hiding this comment.
Yes as far as I can tell there is no restriction that Alice needs to revoke her old commitment but just can send UpdatAdd + CommitSig to NodeB which as a consequence will always stay below the reserve on this commitment. As mentioned it will work with the current implementation but it will basically have this requirement which eventually we need to decide whether we are content with:
You're correct that an implementation can send a signature before revoking their existing commitment after receiving a signature. You can only do this once though, as you can't receive any other new states until you revoke.
As for the reserve, the aux channels will eventually have their own higher level reserve enforcement. So this allows the reserve to be much lower, even zero cone this is in place.
Restricting it only to Aux Channels is definitely the way if we go. this route.
Yep, this has always been the deployment path. This feature won't be activated for anything other than the set of aux channels.
| 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.
lnwallet/channel.go
Outdated
| if channel.BalanceAboveReserve(party) { | ||
| d := int64(entry.Amount) | ||
| balanceDeltas.ModifyForParty( | ||
| party.CounterParty(), | ||
| func(acc int64) int64 { | ||
| return acc + d | ||
| }, | ||
| ) | ||
| } else { | ||
| d := int64(entry.Amount) | ||
| balanceDeltas.ModifyForParty( | ||
| party, | ||
| func(acc int64) int64 { | ||
| return acc + d | ||
| }, | ||
| ) | ||
| } |
There was a problem hiding this comment.
e) Node B receives the New NOOP-ADD from A and will still credit it to Node A because when he settles it, he still is below is reserve on the local commitment, although he is already above the reserve on Node A.
Are you also assuming Node A sends a sig right after the noop add? As only once a sig is received will the views be evaluated to (maybe) apply the noop add.
IIUC, you're referring to the case where balances can diverge momentarily due to concurrent commit sig sends by both parties?
but as mentioned above there are no guarantees that this will be done by other implementations.
This is intended to be isolated to just aux channels. It doesn't solve a problem otherwise. As I commented below, we can gate this feature (reject commit if violated) on if the channel is an aux channel or not.
lnwallet/channel.go
Outdated
| if channel.BalanceAboveReserve(party) { | ||
| d := int64(entry.Amount) | ||
| balanceDeltas.ModifyForParty( | ||
| party.CounterParty(), | ||
| func(acc int64) int64 { | ||
| return acc + d | ||
| }, | ||
| ) | ||
| } else { | ||
| d := int64(entry.Amount) | ||
| balanceDeltas.ModifyForParty( | ||
| party, | ||
| func(acc int64) int64 { | ||
| return acc + d | ||
| }, | ||
| ) | ||
| } |
There was a problem hiding this comment.
We might find a more lean way to do what you are describing. I think it is worth trying to investigate other design choices. This is a quick solution but it brings high burden probably to maintain in the future.
The original stab at this involved the old trick we came up with way back to not have dust HTLCs at all: each send is actually two HTLCs, one in either direction, that nets out to the same asset balance. We started to implement that, but threw it out, as it has implications to the force close logic: an extra set of HTLCs to resolve when we go on chain per outstanding payment.
The high level problem this solves is that: sometimes you want to have an HTLC exist in the commitment transaction/logs, but not actually apply it. Just by existing, it may anchor some useful information for a higher level protocol.
This noop idea was attractive, as the diff is relatively small (can be made smaller with my comment above). It also doesn't change any of the chain handling logic. The trade off is that in the case of a force close, we can't do the noop trick off-chain, so the funds will go to the settler.
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). |
channeldb/channel.go
Outdated
| func (c *OpenChannel) BalanceAboveReserve( | ||
| party lntypes.ChannelParty, delta int64) bool { | ||
|
|
||
| c.Lock() |
There was a problem hiding this comment.
Also why create the method here vs on the channel state machine (one layer up)?
There was a problem hiding this comment.
I need to acquire the locks to read the local commitment & channel config, so I added the method where the locks were readily available
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, remember to re-request review from reviewers when ready |
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
GeorgeTsagk
force-pushed
the
htlc-noop-add
branch
from
8ef8d74 to
e6c948c
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 new "noop add" HTLC type, which is a great feature for managing channel states without affecting balances under certain conditions. The implementation looks mostly correct, but I've found a critical bug related to unit conversion (msat vs sat) that could lead to incorrect balance calculations and behavior. I've also pointed out some areas where comments and naming could be improved for clarity. The tests for the new functionality are comprehensive but one of them seems to confirm the buggy behavior and will need to be updated along with the fix.
lnwallet/channel.go
Outdated
| party lntypes.ChannelParty, balanceDeltas *lntypes.Dual[int64]) { | ||
|
|
||
| channel := lc.channelState | ||
| delta := btcutil.Amount(balanceDeltas.GetForParty(party)) |
There was a problem hiding this comment.
There's a critical bug here concerning unit conversion. The balanceDeltas map stores values in milli-satoshis, but balanceDeltas.GetForParty(party) is directly cast to btcutil.Amount, which is in satoshis. This means milli-satoshis are being treated as satoshis, which will lead to incorrect behavior of balanceAboveReserve.
For example, a delta of 25001 msat will be treated as 25001 sat, which is 1000x larger.
To fix this, you should convert the milli-satoshi value to satoshis before casting.
Additionally, the test TestEvaluateNoOpHtlc in lnwallet/channel_test.go appears to be flawed as it confirms the current buggy behavior. It should be updated along with this fix.
| delta := btcutil.Amount(balanceDeltas.GetForParty(party)) | |
| delta := lnwire.MilliSatoshi(balanceDeltas.GetForParty(party)).ToSatoshis() |
There was a problem hiding this comment.
Wow, that's actually true... I don't understand how we introduced the lntypes.Dual but then used int64 for the balanceDeltas. I think we should add a refactor commit to this PR that changes that to lnwire.MilliSatoshi, that would've made it way more clear, cc @GeorgeTsagk.
There was a problem hiding this comment.
We can add a local type alias (for example type msat = lnwire.MilliSatoshi to keep things short in this part of the code.
There was a problem hiding this comment.
Oh, interesting, this was introduced by addressing a previous comment
GeorgeTsagk
force-pushed
the
htlc-noop-add
branch
from
e6c948c to
76bdb1e
Compare
|
Pushed a version that addresses the critical bug reported here I correctly cast the Enhanced the table driven tests for |
| 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
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.
GeorgeTsagk
force-pushed
the
htlc-noop-add
branch
from
76bdb1e to
4f0d187
Compare
|
/gemini review |
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.
| 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.
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
4f0d187 to
9fedbd6
Compare
|
|
||
| // 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?
|
|
||
| switch { | ||
| case party.IsLocal(): | ||
| totalLocal := c.LocalCommitment.LocalBalance |
There was a problem hiding this comment.
the balance should come from commitChain.tip().ourBalance as c.LocalCommitment is not updated yet?
| @@ -3833,7 +3924,7 @@ func (lc *LightningChannel) validateCommitmentSanity(theirLogCounter, | |||
| // Go through all updates, checking that they don't violate the | |||
| // channel constraints. | |||
| for _, entry := range updates { | |||
| if entry.EntryType == Add { | |||
| if entry.isAdd() { | |||
There was a problem hiding this comment.
We should also update recordSettlement at L4762
| // 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?
| channel := lc.channelState | ||
| delta := balanceDeltas.GetForParty(party) | ||
|
|
||
| // If the receiver has existing balance above reserve then we go ahead |
There was a problem hiding this comment.
The dual Local/Remote of a commitment has nothing to do with Sender/Receiver of an HTLC here - either we are receiving or sending HTLCs, there are always two rounds of commit_sig and revoke_and_ack being exchanged.
| chanType channeldb.ChannelType) updateType { | ||
|
|
||
| noopTLV := uint64(NoOpHtlcType.TypeVal()) | ||
| if _, ok := records[noopTLV]; ok && chanType.HasTapscriptRoot() { |
There was a problem hiding this comment.
We should at least log an error here when there is such a tlv record, yet the channel isn't aux chan.
| @@ -2993,6 +3007,19 @@ func (lc *LightningChannel) evaluateHTLCView(view *HtlcView, | |||
| ) | |||
| if rmvHeight == 0 { | |||
| switch { | |||
| // If this a noop add, then when we settle the | |||
| // HTLC, we actually credit the sender with the | |||
| // amount again, thus making it a noop. Noop | |||
There was a problem hiding this comment.
I don't think it's accurate as we conditionally cancel the amount.
| 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.
|
|
||
| // NoOpAdd is an update type that adds a new HTLC entry into the log. | ||
| // This differs from the normal Add type, in that when settled the | ||
| // balance will go back to the sender, rather than be credited for the |
There was a problem hiding this comment.
We should make it clear that this balance goes back conditionally based on the reserve.
| @@ -58,6 +64,8 @@ func (u updateType) String() string { | |||
| return "Settle" | |||
| case FeeUpdate: | |||
| return "FeeUpdate" | |||
| case NoOpAdd: | |||
| return "NoopAdd" | |||
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