multi: add static address loop in monitor#1165
Conversation
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 implements support for static address loop-in swaps within the monitor system. It extends the existing swap type definitions to include a new variant, updates the RPC interface to allow clients to interact with and filter these swaps, and ensures that the internal marshalling and filtering logic correctly processes these transactions. Highlights
New Features🧠 You can now enable Memory (public preview) 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. 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 the 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 counterproductive. 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. Footnotes
|
|
Looking for ConceptACK/suggestions. Still need to test. |
![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 support for a new swap type, STATIC_ADDRESS_LOOP_IN, updating the internal swap structures, marshalling logic, filtering, and RPC definitions. The feedback highlights a potential nil pointer dereference in marshallSwap when both HtlcAddressP2TR and HtlcAddressP2WSH are nil, and suggests adding a nil check for HtlcAddressP2WSH to prevent a panic.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if loopSwap.HtlcAddressP2TR != nil { | ||
| htlcAddressP2TR = loopSwap.HtlcAddressP2TR.EncodeAddress() | ||
| htlcAddress = htlcAddressP2TR | ||
| } else { | ||
| htlcAddressP2WSH = loopSwap.HtlcAddressP2WSH.EncodeAddress() | ||
| htlcAddress = htlcAddressP2WSH | ||
| } |
There was a problem hiding this comment.
In the swap.TypeStaticAddressLoopIn case, if loopSwap.HtlcAddressP2TR is nil, the code falls back to the else block and calls loopSwap.HtlcAddressP2WSH.EncodeAddress(). However, if loopSwap.HtlcAddressP2WSH is also nil (which is highly likely for static address loop-in swaps that primarily use Taproot/P2TR), this will result in a nil pointer dereference panic.
To prevent this, we should add a nil check for loopSwap.HtlcAddressP2WSH before calling EncodeAddress().
| if loopSwap.HtlcAddressP2TR != nil { | |
| htlcAddressP2TR = loopSwap.HtlcAddressP2TR.EncodeAddress() | |
| htlcAddress = htlcAddressP2TR | |
| } else { | |
| htlcAddressP2WSH = loopSwap.HtlcAddressP2WSH.EncodeAddress() | |
| htlcAddress = htlcAddressP2WSH | |
| } | |
| if loopSwap.HtlcAddressP2TR != nil { | |
| htlcAddressP2TR = loopSwap.HtlcAddressP2TR.EncodeAddress() | |
| htlcAddress = htlcAddressP2TR | |
| } else if loopSwap.HtlcAddressP2WSH != nil { | |
| htlcAddressP2WSH = loopSwap.HtlcAddressP2WSH.EncodeAddress() | |
| htlcAddress = htlcAddressP2WSH | |
| } |
starius
left a comment
starius
left a comment
There was a problem hiding this comment.
Concept ACK for API shape.
One implementation note. Monitor snapshots and subscribes only to s.swaps, populated from loop.Client.FetchSwaps and live statusChan updates. Static address loop-ins live under staticLoopInManager.GetAllSwaps() and (ListStaticAddressSwaps), so the current changes alone will not make loop monitor see them.
Some bridging is needed to deliver static loop-in updates into monitor's SwapStatus stream: initial snapshot plus live state updates. That also requires an explicit state mapping from static loop-in FSM states to generic SwapState, and filling the generic SwapInfo fields safely, especially HTLC address, amount, label, last hop, timestamps, and costs.
| htlcAddressP2TR = loopSwap.HtlcAddressP2TR.EncodeAddress() | ||
| htlcAddress = htlcAddressP2TR | ||
| } else { | ||
| htlcAddressP2WSH = loopSwap.HtlcAddressP2WSH.EncodeAddress() |
There was a problem hiding this comment.
Static address is always a P2TR, never a P2WSH, so we can just assert that loopSwap.HtlcAddressP2TR is set and return an error otherwise.
| if (swapInfo.SwapType == swap.TypeIn || | ||
| swapInfo.SwapType == swap.TypeStaticAddressLoopIn) && |
There was a problem hiding this comment.
We can use !swapType.IsOut() or even swapType.IsIn() instead of (swapInfo.SwapType == swap.TypeIn || swapInfo.SwapType == swap.TypeStaticAddressLoopIn)
So we can keep two distinct SwapType instances for loop-in and static loop-in, but use IsOut/IsIn in situations where both loop-in and static loop-in work the same way.
There was a problem hiding this comment.
I kind of agree. My only concern is that the difference between TypeStaticAddressLoopIn, TypeIn, and type.IsIn() could become a bit brittle.
Internally, is it common in the codebase to classify TypeStaticAddressLoopIn as a generic In swap? If yes, I am okay with that.
If not, I think we should establish a clear separation in how we name and classify these swap types, so we do not introduce ambiguity later.
| LOOP_IN = 2; | ||
|
|
||
| // STATIC_ADDRESS_LOOP_IN indicates a static address loop in swap. | ||
| STATIC_ADDRESS_LOOP_IN = 3; |
There was a problem hiding this comment.
This change seems unrelated to the loop monitor integration of static loop-ins. If we need it for completeness, I propose to do it in a separate commit or in a follow-up PR.
There was a problem hiding this comment.
The thing here is, how can we tell the monitor that this is a static loop in swap and that we are sending a new notification type, not the classic swap in or swap out notification?
SwapStatus is currently a flat struct returned in the rpc Monitor, and SwapType is the field that carries this information. Adding this option here was the best design I found so far.
Are you thinking about another option? How would that look?
|
Thanks @GustavoStingelin for putting up this draft PR! I am looking a bit ahead of where we'd update the monitor with the static state, and it should probably be Keeping future swap types in mind maybe it would make sense to send lightweight notifications Maybe we could have a small monitor publisher in each existing and new swap config that does something like: What do you think @GustavoStingelin @starius ? |
I like the idea, but I have two questions:
In that case, we could show the second state twice in the monitor and lose the first state transition from the monitor log. |
|
I am thinking that, in the future, we could have something like this: message MonitorEvent {
string id = 1;
bytes id_bytes = 2;
SwapKind kind = 3;
int64 amount = 4;
int64 initiation_time = 5;
int64 last_update_time = 6;
string label = 7;
// Human friendly normalized lifecycle.
MonitorPhase phase = 8;
// Machine specific exact state.
string state = 9;
oneof details {
LoopInMonitorDetails loop_in = 20;
LoopOutMonitorDetails loop_out = 21;
StaticLoopInMonitorDetails static_loop_in = 22;
// Future swap types can be added here.
}
}Where:
The idea is to separate the high level swap category, the normalized monitor lifecycle, and the exact internal state. That should make the monitor API easier to extend without overloading one field with multiple meanings. |
from
|
I tend to not assemble info in the static address state machine and then send it to the monitor, rather just send a lightweight reference which the monitor can assemble from.
This is a good point! We might cache the swap state at that point in time to not miss anything, but that might defeat I think as long as the data we are returning to the monitor is already in memory we can think about this optimization later. |
Cool, I totally agree. I think doing that broader refactor in this PR would be too much, but I’m also not very happy with my current local code. It has some ugly workarounds to support both state models. So what do you think about shifting to the new monitor approach now? Or would you prefer to take a look at my current code first and decide from there? Also, just to confirm, since we release the client and loopd together, a breaking protobuf API change should not be a problem, right? |
GustavoStingelin
force-pushed
the
feat/monitor-static-address
branch
from
c93d2dd to
db63a8b
Compare
| "state": "INITIATED", | ||
| "static_loop_in_state": "SIGN_HTLC_TX", |
There was a problem hiding this comment.
@hieblmi It is the thing that I'm mostly concerned about
GustavoStingelin
force-pushed
the
feat/monitor-static-address
branch
from
db63a8b to
73bc60a
Compare
GustavoStingelin
changed the title
|
@claude review |
ReviewOverview: this PR wires static-address loop-in swaps into the generic Potential issues
Nice catches / good practices
Minor
Nothing here looks blocking — mostly scoping/consistency questions and small test/DRY nits. Nice work on the race-condition handling for the snapshot/live-update overlap. |
| } | ||
| } | ||
|
|
||
| func (s *swapClientServer) staticAddressLoopInSwapInfos( |
There was a problem hiding this comment.
Please add godoc for each new symbol, including private methods. The goal (sometimes not achieved) in this code base is that everything has a godoc, including test files, struct members, consts, etc, i.e. all symbols.
Also please comment non-trivial logic in the code, prepending code comments, explaining what is going on, before come code sections.
The purpose of this is to explain to code readers (both human and LLMs) what this thing does or what is about to happen.
| SwapHash: swp.SwapHash, | ||
| SwapType: swap.TypeStaticAddressLoopIn, | ||
| StaticAddressLoopInState: swp.GetState(), | ||
| HtlcAddressP2TR: staticAddress, |
There was a problem hiding this comment.
HTLC address is not the same as staticAddress. These are two different addresses and it is very important to separate them. We got issues previously when people sent funds to HTLC addresses by mistake.
HTLC address also exists and is a property of a static loop-in (as well as of other swap types). It should be loaded/computed and put here.
Static address is a long-living address which is reused by (potentially) many deposits and many static loop-ins. Probably we need a new field to pass it here. The new field for the static address will only be filled if the swap is a static loop-in.
There was a problem hiding this comment.
Imo we shouldn't show the htlc address in the static world as this is a backup transaction by the server that is rarely ever shown as we try to avoid publishing it and instead sweep directly.
If we still think this is useful information we could recreate like it is done in loopin.go:
func (l *StaticAddressLoopIn) getHtlc(chainParams *chaincfg.Params)
| case loopin.Failed, loopin.SucceededTransitioningFailed: | ||
| return loopdb.StateFailAbandoned |
There was a problem hiding this comment.
I think loopdb.StateFailAbandoned is a wrong state for Failed and SucceededTransitioningFailed.
@hieblmi What do you think?
There was a problem hiding this comment.
Technically not correct as abandoned is reserved in the old loop world for manual swap abandonment, if a swap is borked e.g., similar to how lnd allows abandoning channels.
@GustavoStingelin do we need the mapping of the static states into the old states?
I think we can just rely on the static swap state and leave the old state empty? So just use
StaticAddressLoopInState: swp.GetState(),
| return false | ||
| } | ||
|
|
||
| return !swp.LastUpdate.After(lastUpdate) |
There was a problem hiding this comment.
I think we should differentiate by swap state as well, not only by time. If two updates share the same time, but have different states, they should not be deduplicated.
| LightningClient: d.lnd.Client, | ||
| } | ||
| openChannelManager = openchannel.NewManager(openChannelCfg) | ||
| statusChan := make(chan loop.SwapInfo) |
There was a problem hiding this comment.
The channel is not buffered. There is a write to it in staticLoopInStatusUpdater.sendUpdate which can potentially block. sendUpdate is called from a swap's SendUpdate, so this may block the swap's progress, which is not great.
I propose to make this channel buffered to mitigate this risk.
There was a problem hiding this comment.
yeah, it was previously blocking in the old loopin/out, will change it
| State: staticAddressLoopInGenericState(swp.GetState()), | ||
| Cost: loopdb.SwapCost{ | ||
| Server: btcutil.Amount( | ||
| staticAddressLoopInSwapServerCost(swp), |
There was a problem hiding this comment.
I would maybe add/keep a comment saying this mirrors ListStaticAddressSwaps: only actual persisted client-visible server cost is reported; onchain/offchain remain zero until static loop-ins persist real fee data.
| case swp := <-s.statusChan: | ||
| s.swapsLock.Lock() | ||
| s.swaps[swp.SwapHash] = swp | ||
| if swp.SwapType != swap.TypeStaticAddressLoopIn { |
There was a problem hiding this comment.
Claude raised an interesting point. We do now have an inconsistency of loop monitor and loop listswaps / loop swapinfo.
Users might see a swap hash in loop monitor and try to check its information in loop listswaps or loop swapinfo and be confused that the swap is missing (because it is a static loop-in).
I propose to add a note on CLI side to loop listswaps output and its docs that it covers only traditional loop-ins and loop-outs, not static loop-ins. We have command loop static listswaps which should be used instead - tell this in the message.
For loop swapinfo <swap> we don't have a direct replacement in the static world. @hieblmi would it make sense to extend loop swapinfo <swap> so it works for static loop-ins as well? Or is it better to add loop static swapinfo <swap> command and in loop swapinfo tell the user to use the new command instead if we detect that the hash is of a static loop-in?
There was a problem hiding this comment.
For
loop swapinfo <swap>we don't have a direct replacement in the static world. @hieblmi would it make sense to extendloop swapinfo <swap>so it works for static loop-ins as well? Or is it better to addloop static swapinfo <swap>command and inloop swapinfotell the user to use the new command instead if we detect that the hash is of a static loop-in?
I think we should either add loop static swapinfo <swap> or extend existing loop static listswaps with a --swap_hash param. In the future the goal is to get rid of loop swapinfo respectively replace it with loop static [swapinfo|listswaps]
hieblmi
left a comment
hieblmi
left a comment
There was a problem hiding this comment.
Thanks for the changes so far @GustavoStingelin 🚀 I've left some comments.
Overall I wonder if we could simplify the monitoring approach without having to modify all too much of the existing flows and also be extensible when future swap types need to be monitored.
What about if we seed a staticSwaps map[lntypes.Hash]loop.SwapInfo once from the database during daemon startup, then update it in processStatusUpdates. Monitor could register and snapshot both legacy and static maps under the same lock.
That would eliminate:
- Full database reads on every monitor connection.
- Snapshot/live timestamp deduplication.
- Most of the race-specific logic.
- The need to put static swaps into the legacy s.swaps map or expose them through
ListSwaps.
| // Just return the server's in-memory cache here too as we also want to | ||
| // return temporary failures to the client. | ||
| s.swapsLock.Lock() | ||
| defer s.swapsLock.Unlock() |
There was a problem hiding this comment.
the unlock can be done right after s.swaps has been read. This is to avoid holding it if marsahllSwap calls the asset client.
| return | ||
| } | ||
|
|
||
| err = f.sendUpdate(ctx) |
There was a problem hiding this comment.
Here comes back my initial performance concern, since we don't want to slow further down the state transitions of the state machine.
We should add godocs pointing out that sendUpdate is not supposed to do heavy lifting. If the updates become to heavy(like calculating htlc address from given parameters or something else) we could queue the monitor updates. But for now lgtm.
4 participants
Implements monitor support for static address loop-in swaps.