staticaddr: reconcile deposits after wallet indexing #1067
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 addresses a timing-sensitive issue where the system's in-memory view of deposits might be reconciled with 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
|
![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 time.Sleep to mitigate a race condition between receiving a new block notification and the lnd wallet finishing indexing of that block. While this is a pragmatic approach to address the race condition, the implementation can be improved by extracting the hardcoded sleep duration into a named constant for better readability and maintainability. I've also raised a question about the fragility of using a fixed sleep and whether a more deterministic synchronization mechanism with lnd might be available.
| // transactions of interest in the new block. This | ||
| // mitigates a race between wallet indexing and | ||
| // reconciling deposits. | ||
| time.Sleep(500 * time.Millisecond) |
There was a problem hiding this comment.
Using a hardcoded 'magic number' like 500 for the sleep duration can make the code harder to understand and maintain. It's better to define this value as a named constant at the package level (e.g., walletIndexDelay = 500 * time.Millisecond). This improves readability by giving the value a clear meaning and makes it easier to adjust in the future.
Additionally, relying on a fixed sleep duration to resolve a race condition is inherently fragile, as the required delay might vary depending on system load. Have you investigated if lnd provides a more deterministic signal for when wallet indexing is complete for a new block? If not, this workaround is understandable, but its potential flakiness is worth noting.
There was a problem hiding this comment.
+1
Can we wait for some observable event instead of sleeping?
IIUC, the root cause is that the output of ListUnspent WalletKit API can be outdated and reconcileDeposits uses ListUnspent to detect deposits. There is no way to detect this condition from ListUnspentResponse.
Maybe we should fix this in LND, adding the current height and block hash to ListUnspentResponse? Then we can retry ListUnspent until we get up-to-date results.
We are notified about new block in ChainNotifier component of LND, but track deposits using its WalletKit component which is also a root cause of the mismatch. Future idea: can ChainNotifier have some API to track deposits? Something similar to RegisterSpendNtfn but where you provide a pkScript and it notifies you when funds arrive to that pkScript. We can use RegisterConfirmationsNtfn without specifying txid to track any deposit to specific pkScript (e.g. static address in Loop). Limitation: repeated deposits to the same script won't trigger additional notifications because the notifier caches the first confirmation and treats subsequent reuse as duplicate. I remember this limitation was addressed somewhere, but I can't find where.
There was a problem hiding this comment.
+1
Can we wait for some observable event instead of sleeping?
IIUC, the root cause is that the output of
ListUnspentWalletKit API can be outdated andreconcileDepositsusesListUnspentto detect deposits. There is no way to detect this condition fromListUnspentResponse.
Yes!
Maybe we should fix this in LND, adding the current height and block hash to
ListUnspentResponse? Then we can retryListUnspentuntil we get up-to-date results.
The wallet controller has a method IsSynced which compares the wallet tip to the chain backend. I think we can use the result in the ListUnspent call to see if indexing is finished.
We are notified about new block in ChainNotifier component of LND, but track deposits using its WalletKit component which is also a root cause of the mismatch.
Future idea: can ChainNotifier have some API to track deposits? Something similar to RegisterSpendNtfn but where you provide a pkScript and it notifies you when funds arrive to that pkScript.We can use RegisterConfirmationsNtfn without specifying txid to track any deposit to specific pkScript (e.g. static address in Loop). Limitation: repeated deposits to the same script won't trigger additional notifications because the notifier caches the first confirmation and treats subsequent reuse as duplicate. I remember this limitation was addressed somewhere, but I can't find where.
We use ImportTaprootScript to add the pkscript to the wallet, then poll ListUnspent for changes. This hacks around the RegisterConfirmationsNtfn limitation.
2 participants
Before reconciling our in-mem view of deposits with lnd we give the wallet extra time to index transactions of interest in the new block. This mitigates a race between wallet indexing and reconciling deposits.