Conversation
…ex 0 When createHtlcTx produces a transaction with a change output at index 0, createHtlcSweepTx correctly determines htlcInputIndex=1 to identify the HTLC output, but then unconditionally reads TxOut[0].Value for htlcOutValue. This uses the change amount instead of the HTLC amount, producing an incorrect sweep transaction with wrong fee calculation. Use TxOut[htlcInputIndex].Value so the sweep always references the correct HTLC output.
SweepHtlcTimeoutAction used a select with a default case containing a blocking time.After. When the context was canceled, it logged the error but continued the retry loop instead of returning. The default case also meant that ctx.Done was only checked when it was already signaled, while an hour-long sleep blocked without listening for cancellation. Replace the default+time.After pattern with a proper select on both ctx.Done and time.After so the function exits promptly on shutdown.
recoverLoopIns wrote to the activeLoopIns map from inside a goroutine without any synchronization. The map is shared state accessed from the manager's main Run loop, creating a data race. Move the map write before the goroutine spawn so it happens synchronously during recovery. Also fix the stale log message that said "OnStart" instead of "OnRecover".
handleWithdrawal did not check the error returned by RegisterSpendNtfn before spawning a goroutine to read from the notification channels. If registration failed, spentChan would be nil and the goroutine would block forever on a nil channel read. Add the missing error check so the function returns early on registration failure.
The %w formatting verb is only meaningful for fmt.Errorf where it enables error wrapping. In log.Errorf it prints as %%!w(error=...), producing garbled log output. Use %v and add the missing colon separator.
The comment for ServerPubkey incorrectly said "ClientPubkey is the client's pubkey" due to a copy-paste from the field above.
In manager.go, deferred shim cleanup was calling FundingStateStep with the original request ctx. If the user had already canceled that context, the cleanup RPC would run with a canceled context and could fail to remove the pending shim. I changed that cleanup path to use context.WithoutCancel(ctx) so the cancellation RPC still has a live context.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request delivers a collection of minor but important fixes across various components of the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 series of fixes across the staticaddr functionality. The changes include correcting comments and log messages, fixing data races and potential nil pointer dereferences, and adding missing error handling. The logic for handling context cancellation during cleanup has also been improved. Overall, the changes enhance the robustness and correctness of the code. I've found one minor issue with a log message that was not updated along with the code logic.
hieblmi
force-pushed
the
static-fixes
branch
from
e77d326 to
8435549
Compare
starius
left a comment
starius
left a comment
There was a problem hiding this comment.
LGTM! 🚀
Great work! I left few comments.
| fee := feeRate.FeeForWeight(weightEstimator.Weight()) | ||
|
|
||
| htlcOutValue := htlcTx.TxOut[0].Value | ||
| htlcOutValue := htlcTx.TxOut[htlcInputIndex].Value |
There was a problem hiding this comment.
Please add a regression test, so this or a similar bug does not re-appear again in the future.
|
|
||
| f.Errorf("unable to create and publish htlc timeout sweep "+ | ||
| "tx: %v, retrying in %v", err, time.Hour.String()) | ||
| "tx: %v, retrying in %v", err, time.Hour.String()) |
There was a problem hiding this comment.
- Make a const instead of using
time.Hourdirectly. - Reuse the const below in
case <-time.After - When logging, you can pass it directly, no
.String()call is needed. %v and %s do t automatically
| for { | ||
| err := f.createAndPublishHtlcTimeoutSweepTx(ctx) | ||
| if err == nil { | ||
| break |
There was a problem hiding this comment.
s/break/return OnHtlcTimeoutSweepPublished/
and remove return OnHtlcTimeoutSweepPublished in the end of the function
| select { | ||
| case <-ctx.Done(): | ||
| f.Errorf("%v", ctx.Err()) | ||
| return f.HandleError(ctx.Err()) |
There was a problem hiding this comment.
Add a comment, explaining when this ctx may be cancelled and why it is ok to return an error here (giving up the broadcasting attempts).
|
|
||
| // Send the OnRecover event to the state machine. | ||
| go func(fsm *FSM, swapHash lntypes.Hash) { | ||
| go func(fsm *FSM) { |
| var ( | ||
| confChan chan *chainntnfs.TxConfirmation | ||
| confErrChan chan error | ||
| ) | ||
| confChan, confErrChan, err = |
There was a problem hiding this comment.
You can remove var and use :=
A new err will be created - this is better. Currently we use err across goroutines, which is risky.
| if err != nil { | ||
| log.Errorf("Error registering confirmation "+ | ||
| "notification: %v", err) | ||
| return |
There was a problem hiding this comment.
new line before return and after }
| if err != nil { | ||
| log.Errorf("Error registering confirmation "+ | ||
| "notification: %v", err) | ||
| return |
There was a problem hiding this comment.
I think we should rework the code watching for withdrawal confirmation. We can do it in a follow-up PR.
- If Register* calls fail, not just log an error and stop, but retry again at next block detection. We use this retry approach in other parts: retrying on next block.
- Handle reorgs. Currently if it the withdrawal tx gets one confirmation, but gets reorged, we do not automatically handle this situation.
| shimPending = false | ||
| } | ||
| } | ||
| defer maybeCancelShim() |
There was a problem hiding this comment.
Can remove maybeCancelShim name and just do this?
defer func() {
// code of maybeCancelShim
}()| } | ||
| _, err := m.cfg.LightningClient.FundingStateStep( | ||
| ctx, cancelMsg, | ||
| context.WithoutCancel(ctx), cancelMsg, |
There was a problem hiding this comment.
I found more cases like this. Can you check the commits and git am them into the branch, please?
2 participants
A small collection of fixes for issues that agents identified.