htlcswitch+routing: handle nil pointer dereference properly #9303
Conversation
yyforyongyu
added
bug fix
no-changelog
size/micro
|
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 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 (
|
yyforyongyu
force-pushed
the
fix-circuit-closed
branch
from
dd41381 to
3f888f3
Compare
|
|
||
| // If the circuit is in the process of closing, we will return a nil as | ||
| // there's another packet handling undergoing. | ||
| if errors.Is(err, ErrCircuitClosing) { |
There was a problem hiding this comment.
Hmm I am not sure if we should return nil here, because we shouldn't call the settle logic twice if we are already closing the circuit and if we do so shouldn't we return an error signaling that the circuit is closing rather than allowing it to be called multiple times and neglecting the error ?
There was a problem hiding this comment.
I also think we should return an error here and let it be handled - unfortunately we cannot easily do it atm. I view this as a followup of 8c0c53e - in addition of catching the closed case, we should also catch the closing case. Tho it does feel like the logic should be put inside closeCircuit?
There was a problem hiding this comment.
ahh ok given this fact I think it is ok to place the logic here. But I would recommend to add a comment here why we can end up calling this logic here twice the current comment I would not be able to explain what this really means in detail or how this might happen:
there's another packet handling undergoing.
i this really only the case if resolution and the normal htlcPlex loggic trigger shortly after another ?
We have two sources which can call `handlePacketSettle`, either through the link's `<-s.htlcPlex`, or the `<-s.resolutionMsgs`, which means the `closeCircuit` could be call twice. Previously we already caught this case inside `closeCircuit`, in that we would return a nil circuit upon seeing `ErrUnknownCircuit`, indicating the circuit was removed. However, we still need to account the case when the circuit is the process of being closed, which is now fixed as we will ignore when seeing `ErrCircuitClosing`.
yyforyongyu
force-pushed
the
fix-circuit-closed
branch
from
3f888f3 to
fde235d
Compare
In a rare case when the critical log is triggered when using postgres as db backend, the `payment` could be nil cause the server is shutting down, causing the payment fetching to return an error. We now cache its state before fetching it from the db.
yyforyongyu
force-pushed
the
fix-circuit-closed
branch
from
fde235d to
7882e1c
Compare
This is the final cherry-picking from the blockbeat series PR #9260, which fixes the case on how we handle a closing circuit and a nil pointer dereference in the payment lifecycle: