Cancel back outgoing dust htlcs before commitment is confirmed. #9068
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 WalkthroughThe changes enhance the handling of canceled Hash Time-Locked Contracts (HTLCs) within the contract arbitration process. New methods for inserting and fetching canceled HTLCs are added to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Poem
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
ziggie1984
changed the title
ziggie1984
force-pushed
the
cancel-back-dust-htlc
branch
2 times, most recently
from
923c6b0
to
fa7a925
Compare
ziggie1984
force-pushed
the
cancel-back-dust-htlc
branch
from
fa7a925
to
4f96e28
Compare
|
Hi reviewers I am not happy about the following in this PR maybe someone has a nice idea how to make it more clean: So right now when we locally force close the channel we would fail the dust 2 times, meaning that the second time will cause the log error saying the closeCircuit is already gone. This is currently needed because we need to cancel dust even if the force-close is not initiated by us or the force-close is initiated by us but not by LND, broadcasting the force-close via some other means. This would right now cause some annying log entry similar to: Example: Probably we should make the extra work and remove the outgoing htlc from the commitSet as soon as we cancel the incoming back. Will investigate. |
ziggie1984
force-pushed
the
cancel-back-dust-htlc
branch
from
4f96e28
to
9aca3ec
Compare
|
We could query the circuitMap and not attempt the cancelling of the incoming htlc but maybe just failing it and hitting the error is as efficient ? |
There was a problem hiding this comment.
Nice refactors! My main comment is we could cancel the dust even earlier, once we've decided there are chain actions to be taken here,
lnd/contractcourt/channel_arbitrator.go
Lines 955 to 960 in edd9ade
The current design may end up calling the canceling logic twice, as indicated from this state transition diagram,
lnd/contractcourt/briefcase.go
Lines 139 to 183 in edd9ade
ziggie1984
force-pushed
the
cancel-back-dust-htlc
branch
7 times, most recently
from
12fb4ee
to
67cd269
Compare
|
Updated the commit-structure and also the code design quite a bit. Needed to persist the canceled htlcs in the This PR is also created in a way so we can now add the canceling of non-dust htlcs as well in a followup PR. I think might still be missing some unit-tests, need to dig into all the tests in the contractcourt, but lets see whether this new approach is the way we wanna go before adding more tests. |
|
@coderabbitai review |
Actions performedReview triggered.
|
ziggie1984
added
size/kilo
There was a problem hiding this comment.
Overall I think I understand the approach but my main first-round feedback is that we need to tighten up terminology. I wish this wasn't the case but it impedes my ability to understand the details of the rest of the PR and makes it pretty difficult for me to tell whether or not the code is doing the right thing.
Another thing to note is that I'm somewhat less experienced with the contractcourt package and so I'm less able to bring expertise into the review and give charitable interpretations to the things I encounter.
Overall I think the bias towards extracting out common functionality into helper functions is very good and I think I understand the high level approach here. Nothing stands out to me as obviously wrong but I can't comment very well on whether this change is complete with respect to doing all of the things it needs to to safely accomplish its goal.
Hopefully future review rounds will help both in tightening the implementation as well as me taking some time out of band to better familiarize myself with the overall structure.
contractcourt/channel_arbitrator.go
Outdated
| @@ -682,7 +682,7 @@ func (c *ChannelArbitrator) relaunchResolvers(commitSet *CommitSet, | |||
| // chain actions may exclude some information, but we cannot recover it | |||
| // for these older nodes at the moment. | |||
| var confirmedHTLCs []channeldb.HTLC | |||
| if commitSet != nil { | |||
| if commitSet != nil && commitSet.ConfCommitKey != nil { | |||
There was a problem hiding this comment.
Is this one of those situations where we are using pointers to simulate optional behavior or is this a pointer because of memory profiling characteristics?
contractcourt/channel_arbitrator.go
Outdated
| // Send the msg to the switch. | ||
| if len(msgsToSend) == 0 { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
I think the comment here is wrong. However, this seems unnecessary in the first place, right? Even if it has a zero length you can still send that to the DeliverResolutionMsg no?
There was a problem hiding this comment.
I think returning early here makes sense, but will change if another person also agrees in changing it and letting it run through with a length of 0.
contractcourt/channel_arbitrator.go
Outdated
| // resolveBreachedHTLCs resolves all HTLCs that are breached transaction was | ||
| // detected. Resovling here means that it fails back the corresponding incoming | ||
| // HTLCs for a given outgoing HTLC on the current remote commitment set | ||
| // (including the remote pending commitment set). | ||
| func (c *ChannelArbitrator) resolveBreachedHTLCs(commitSet CommitSet) error { |
There was a problem hiding this comment.
Seems like this isn't doing resolution beyond the incoming cancellations.
There was a problem hiding this comment.
correct, what about: resolveOutgoingBreachedHTLCs func name ?
There was a problem hiding this comment.
Well I think all this is doing is cancelling the incoming counterparts of the outgoing breached HTLC.
I've been thinking generally about what we should call this situation where we cancel an incoming HTLC prior to its outgoing counterpart being cancelled, I think it is an "abandoned forward". This steals some terminology we have from other parts of the codebase (abandoned channel). What I like about it is that it correctly describes what is happening: we were given a forward that we attempted and now we are no longer attempting to complete the forward. We may have our reasons why we believe that abandoning it is safe, but from an HTLC resolution perspective we are decoupling the results of the outgoing and incoming htlc. It is this decoupling that I think constitutes an "abandon".
There was a problem hiding this comment.
removed the function and called the cancelIncomingHTLCs => abandonForwards
There was a problem hiding this comment.
I don't think abandonForwards is a better name. To me it's more confusing as we are not abandoning either a forwarding packet or an outgoing HTLC here. I can only see that we are abandoning the whole forwarding action as a whole, maybe that's related but far-fetched as canceling this incoming HTLC is just part of the whole action. The issue #7683 already explained it well, that we are failing the incoming HTLC backward from the upstream side, so I think cancelIncomingHTLCs works. If we wanna more context we can name it cancelUpstreamHTLC, cancelHTLCBackward, or cancelUpstreamIncoming, but not abondonForwards - we don't even expose the concept of forwarding packets in channel arbitrator, which should live in the switch.
There was a problem hiding this comment.
ok I am going for cancelUpstreamHTLC hope everyone is fine with it ?
There was a problem hiding this comment.
We can call it cancelUpstreamHTLCs but I think we run the danger of overloading terms and so people have to keep track of all of the qualifiers to figure out what's going on. I also don't think cancelUpstreamHTLCs is meaningfully better (or worse) than cancelIncomingHTLCs. I am not attached to abandonForwards specifically. I do like it, but I think it's important that we capture (concisely) the idea that:
"Cancel the upstream component of the forwarding action whose downstream htlc is this outgoing htlc"
The forward is the structure that binds the incoming and outgoing htlcs together. I am adamantly against saying "cancel outgoing htlc" anywhere in this codebase. It is incredibly confusing and flat out wrong. We are cancelling inbound htlcs that were forwarded and whose outbound counterparts are below the dust threshold of the downstream channel when that channel force closes. That's kind of a mouthful so we need a term that captures that essence.
There was a problem hiding this comment.
ok kept it at abandonForwards!
ziggie1984
force-pushed
the
cancel-back-dust-htlc
branch
2 times, most recently
from
02be788
to
c7b7058
Compare
ziggie1984
force-pushed
the
cancel-back-dust-htlc
branch
from
c7b7058
to
6c4db66
Compare
There was a problem hiding this comment.
ACK 6c4db66 Think we should change the name abondanForward then it's good to go.
There was a problem hiding this comment.
High level feedback is still around terminology. I do think that it is much better this time around, but needs more cleanup. Overall I think the logic looks decent. I tried not to nitpick on style. The one thing I'm noticing about it though is that I want to make sure that the "no deadline -> no htlcs" is the right condition to check. I think it might miss one important case.
contractcourt/channel_arbitrator.go
Outdated
| // resolveIncomingDust resolves the incoming dust HTLCs because they do not have | ||
| // an output on the commitment transaction and cannot be resolved onchain. We | ||
| // mark them as failed here. | ||
| func (c *ChannelArbitrator) resolveIncomingDust( |
There was a problem hiding this comment.
I think maybe we should just call this failIncomingDust since there's no other resolution type taking place
contractcourt/channel_arbitrator.go
Outdated
| // If we can fail an HTLC immediately (an outgoing HTLC | ||
| // with no contract and it was not canceled before), | ||
| // then we'll assemble an HTLC fail packet to send. | ||
| // If we can fail outgoing dangling HTLCs and dust HTLCs |
There was a problem hiding this comment.
I think we need to remove "If" here.
contractcourt/briefcase.go
Outdated
| @@ -1475,12 +1475,18 @@ func decodeBreachResolution(r io.Reader, b *BreachResolution) error { | |||
| return binary.Read(r, endian, &b.FundingOutPoint.Index) | |||
| } | |||
|
|
|||
| func encodeHtlcSetKey(w io.Writer, h *HtlcSetKey) error { | |||
| err := binary.Write(w, endian, h.IsRemote) | |||
| func encodeHtlcSetKey(w io.Writer, h fn.Option[HtlcSetKey]) error { | |||
There was a problem hiding this comment.
I wouldn't bother having this take an option. Just have it take the thing and put this function in a WhenSome or something like that.
There was a problem hiding this comment.
changed the paramter Option to a normal htlcSetKey and unwrapping the Option in the caller function.
| @@ -590,7 +590,7 @@ func TestChannelArbitratorRemoteForceClose(t *testing.T) { | |||
| chanArb.cfg.ChainEvents.RemoteUnilateralClosure <- &RemoteUnilateralCloseInfo{ | |||
| UnilateralCloseSummary: uniClose, | |||
| CommitSet: CommitSet{ | |||
| ConfCommitKey: &RemoteHtlcSet, | |||
| ConfCommitKey: fn.Some(RemoteHtlcSet), | |||
There was a problem hiding this comment.
I'm noticing that there's never a time that this is set to None. Is it every possible for it to be None?
There was a problem hiding this comment.
Looking at the code, I don't really see a case where we check for it being nil, seems like legacy usecase, however its part of the db so removing this structure would require a migration. However it gives us the possibility to know which commitment set confirmed, when querying the db. So let's keep it ?
ziggie1984
force-pushed
the
cancel-back-dust-htlc
branch
3 times, most recently
from
0b1d6a0
to
d6bcbaa
Compare
ziggie1984
force-pushed
the
cancel-back-dust-htlc
branch
from
d6bcbaa
to
ab0c831
Compare
Refactor the part where we are failing back the incoming htlc when the channel of the corresponding outgoing htlc is force closed. We do this because in furture commits we separate the logic when we fail back the incoming htlc (abandonForward). Right now we fail abandon dust forwards and non-dust forwards only when the commitment transaction is confirmed. Later we will move the canceling of the upstream htlc when the commitment transaction is broadcasted instead of waiting until the commitment tx is confirmed. The reason for that is that dust cannot be enforced onchain anyways so there is no reason to wait.
We distinguish between dangling and dust htlcs. This does not change any logic but only introduces new types to later act on them differently when we begin to fail dust htlcs earlier in a later commit.
ziggie1984
force-pushed
the
cancel-back-dust-htlc
branch
from
ab0c831
to
1728f0e
Compare
We will now cancel dust htlcs on the local/remote commits after we decided to go onchain. This can be done because dust cannot be enforced onchain and therefore there is no way to also reveil the preimage onchain.
Now that we cancel dust htlcs prematurely even before the commitment tx is confirmed we don't consider dust htlcs when creating the cpfp transaction.
Now outgoing dust-htlcs are canceled back before the commitment is confirmed onchain.
ziggie1984
force-pushed
the
cancel-back-dust-htlc
branch
from
1728f0e
to
59f3268
Compare
Fixes #7969