staticaddr: dynamic deposit conf target#1065
staticaddr: dynamic deposit conf target#1065hieblmi wants to merge 23 commits intolightninglabs:masterfrom
Conversation
Summary of ChangesHello @hieblmi, 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 updates the static address loop-in client to dynamically adapt to server-side confirmation requirements. It streamlines the process for small swaps by reducing the client's minimum confirmation threshold, intelligently prioritizes deposits based on their confirmation status, and provides clearer feedback to users when deposits require additional confirmations. These changes improve the client's efficiency and user experience when interacting with servers that implement risk-based confirmation policies. 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. 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
|
hieblmi
changed the title
![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 dynamic server-side confirmation requirements for static address loop-ins. The client's minimum confirmation requirement is lowered to 1, and the deposit selection logic is updated to prioritize deposits with more confirmations. Additionally, the client now logs detailed information when a swap fails due to insufficient confirmations, improving user feedback. The changes look good and align with the goal of providing more flexibility for loop-in swaps. I've added a couple of minor suggestions to improve log message formatting for better readability.
| log.Warnf("Insufficient deposit confirmations, max wait: "+ | ||
| "%d blocks", confDetails.MaxBlocksToWait) |
There was a problem hiding this comment.
For better readability and to avoid string concatenation, you can include the full format string directly in log.Warnf.
log.Warnf("Insufficient deposit confirmations, max wait: %d blocks", confDetails.MaxBlocksToWait)| log.Warnf(" Deposit %s: %d/%d confirmations "+ | ||
| "(need %d more blocks)", | ||
| dep.Outpoint, dep.CurrentConfirmations, | ||
| dep.RequiredConfirmations, dep.BlocksToWait) |
There was a problem hiding this comment.
For better readability and to avoid string concatenation, you can include the full format string directly in log.Warnf.
log.Warnf(" Deposit %s: %d/%d confirmations (need %d more blocks)",
dep.Outpoint, dep.CurrentConfirmations,
dep.RequiredConfirmations, dep.BlocksToWait)
hieblmi
force-pushed
the
dyn-deposit-confs
branch
from
b59d0dc to
3d7e24f
Compare
hieblmi
force-pushed
the
dyn-deposit-confs
branch
9 times, most recently
from
48555a2 to
bb2cfd2
Compare
hieblmi
force-pushed
the
dyn-deposit-confs
branch
from
bb2cfd2 to
2bf22a1
Compare
hieblmi
changed the title
|
@hieblmi, remember to re-request review from reviewers when ready |
In successive commits we want to access lnrpc specifc methods and objects, so here we import github.com/lightningnetwork/lnd v0.20.1-beta into looprpc and update relevant versions.
…lRequest In this commit we import the lnd's lightning.proto into client.proto to then be able to use lnrpc.OutPoint and lnrpc.OpenChannelRequest instead of the looprpc.OutPoint type. This is safe because the lnrpc and looprpc versions of OutPoint are the same type.
Add macaroon permissions for the new StaticOpenChannel RPC method and change StaticAddressLoopIn action from read to execute to match other swap operations.
Block-based deposit fetching from the internal lnd wallet was susceptible to wallet syncing issues. Replace it with interval-based polling. Reconciliation errors are now logged instead of being fatal, improving resilience during transient failures.
Protect the shimPending variable with a sync.Mutex since it is accessed from multiple goroutines: the main loop goroutine and the server error handling goroutine. Without synchronization this is a data race.
Return an error instead of just logging when chainhash.NewHash fails in the ChanPending handler. The hash variable would be nil and crash on the subsequent String() call.
…ublished Both OpeningChannel and ChannelPublished lacked OnExpiry transitions. handleBlockNotification fires OnExpiry on every new block once the deposit is expired, regardless of the current state. Since both states use NoOpAction or FinalizeDepositAction which release the FSM mutex briefly, an OnExpiry SendEvent can sneak in. Add self-transitions so the event is safely absorbed.
Duplicate outpoints in the request lead to fee miscalculation and an invalid PSBT with the same input listed twice. Validate early and return a clear error message.
Remove unreachable error check after filterNewDeposits which does not return an error. The err variable was already handled from the ListUnspent call above and could never be non-nil at this point.
If the PSBT finalize step succeeds but the stream fails before ChanPending, deposits would remain stuck in OpeningChannel until the next daemon restart. Run the recovery logic immediately so deposits are resolved without requiring a restart.
Add tests for the following edge cases requested in review: - Reorg: channel tx reorged, UTXOs reappear as unspent, deposits return to Deposited state. - Daemon restart during channel opening: deposits in OpeningChannel recovered based on UTXO status (spent → ChannelPublished, unspent → Deposited). - Mempool eviction: tx evicted, UTXOs unspent, deposits return to Deposited. - Mempool rejection: tx never accepted, same recovery as eviction. - Stream errors: lnd stream fails before PSBT finalize, error returned without errPsbtFinalized so deposits can be safely rolled back. - PSBT finalize then stream abort: finalize succeeds but stream dies before ChanPending, error wrapped with errPsbtFinalized so caller triggers recovery instead of blind rollback. - Duplicate outpoints: already covered by TestOpenChannelDuplicateOutpoints.
Add DepositConfirmationStatus and InsufficientConfirmationsDetails messages to support structured error responses when deposits lack sufficient confirmations for dynamic risk requirements.
Reduce MinConfs from 6 to 3 to allow faster swap attempts while the server enforces risk-based confirmation requirements. Update SelectDeposits to prioritize more-confirmed deposits first, increasing the likelihood of server acceptance. Add client-side logging of insufficient confirmation details from server error responses.
hieblmi
force-pushed
the
dyn-deposit-confs
branch
from
2bf22a1 to
830c398
Compare
3 participants
Update static address loop-in client to support dynamic server-side confirmation requirements.
Changes:
Proto updates:
Backwards compatible: Clients with older servers will continue to work normally. The server determines actual confirmation requirements.