rpcserver: add wallet_synced to GetInfoResponse#10507
rpcserver: add wallet_synced to GetInfoResponse#10507ziggie1984 merged 4 commits intolightningnetwork:masterfrom
wallet_synced to GetInfoResponse#10507Conversation
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 enhances 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. 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
walled_is_synced to ListUnspentResponse
![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 wallet_is_synced field to the ListUnspent RPC response, which is a useful addition for clients to know if the wallet's view of UTXOs is up-to-date. The implementation is clean and duplicated correctly across both the main rpcserver and the walletrpc sub-server. The new integration test covers the intended functionality well by simulating a node restart and sync process. The changes to protobuf and swagger definitions are also correct. I have one minor suggestion for the release notes.
hieblmi
changed the title
walled_is_synced to ListUnspentResponseWalletSynced to GetInfo
hieblmi
changed the title
WalletSynced to GetInfowallet_synced to GetInfoResponse
hieblmi
force-pushed
the
wallet-tip
branch
2 times, most recently
from
dd9059b to
f42bcf0
Compare
|
|
||
| // Whether the wallet is fully synced to the best chain. This indicates the | ||
| // wallet's internal sync state with the backing chain source. | ||
| bool wallet_synced = 23; |
There was a problem hiding this comment.
Can we expose the block at which the wallet is?
uint32 wallet_block_height = 24;
string wallet_block_hash = 25;
Then we can track this field to reach the block we're handling deposits for:
blockSub := RegisterBlockEpochNtfn()
block := <-blockSub
info := GetInfo()
for info.WalletBlockHeight < block.Height {
sleep
info = GetInfo()
}
ListUnspent()There was a problem hiding this comment.
If you look at how the newly introduced field is put together you land at
lnd/lnwallet/btcwallet/btcwallet.go
Line 1680 in 9a83b38
So your example would collapse to
for {
info := GetInfo()
if info.WalletSynced { break }
sleep
}
ListUnspent()
|
|
||
| // Whether the wallet is fully synced to the best chain. This indicates the | ||
| // wallet's internal sync state with the backing chain source. | ||
| bool wallet_synced = 23; |
There was a problem hiding this comment.
What about hooking this into the existing state subscription service?
Lines 38 to 59 in aaaf235
There was a problem hiding this comment.
It sounds like it would fit there, but the StateService which exposes this state lives in the rpcperms which doesn't have access to the chain state (yet), so I think the wallet sync state is better exposed in the rpcserver.go where we do already have access.
|
@Roasbeef, @starius, to summarize, the wallet sync-status is needed because we currently can't detect multiple spends/receives to the same address within a block. So we work around that by adding the common address to the wallet and scan it with The right way to fix this imo is to make the Imo, this PR fits nicely into the current output of lnd's A new interface for multispends in |
Abdulkbk
left a comment
Abdulkbk
left a comment
There was a problem hiding this comment.
Resolve conflicts and rebase, otherwise LGTM
hieblmi
force-pushed
the
wallet-tip
branch
7 times, most recently
from
c4b8ed4 to
8c0efad
Compare
hieblmi
force-pushed
the
wallet-tip
branch
5 times, most recently
from
b3983a2 to
c4f9c2e
Compare
hieblmi
force-pushed
the
wallet-tip
branch
3 times, most recently
from
f44b9fb to
a744b1a
Compare
starius
left a comment
starius
left a comment
There was a problem hiding this comment.
LGTM! 🏆
Left a couple of nits.
🔴 PR Severity: CRITICAL
🔴 Critical (1 file)
🟡 Medium (1 file)
🟢 Low (3 files)
Excluded from severity calculation:
AnalysisThis PR is classified as CRITICAL because it modifies
The PR appears well-structured with appropriate test coverage added in the itest files. The changes are relatively small in scope (15 non-generated lines in rpcserver.go), but the criticality of the file necessitates expert review. To override, add a |
🔴 PR Severity: CRITICAL
🔴 Critical (1 file)
🟡 Medium (1 file)
🟢 Low (2 files)
ℹ️ Excluded from severity calculation
AnalysisThis PR is classified as CRITICAL due to changes in
The PR also includes proto file changes (API surface expansion) and test coverage, which is good practice. The majority of the diff is auto-generated protobuf code. Recommendation: Requires review from a maintainer familiar with the RPC server and wallet subsystems. To override, add a |
🔴 PR Severity: CRITICAL
🔴 Critical (1 file)
🟡 Medium (1 file)
🟢 Low (1 file)
📝 Excluded from severity calculation (4 files)
AnalysisThis PR adds a The change adds wallet synchronization status to the node's info endpoint, which is a straightforward addition, but any modifications to rpcserver.go need careful scrutiny to ensure:
The bulk of the changes (4,922 lines) are auto-generated protobuf code and should not require manual review beyond verifying they were generated correctly. Recommendation: This PR requires review from an engineer familiar with the RPC server architecture and wallet subsystem integration. To override, add a |
rpcserver.go
Outdated
| // by many wallets (and also our itests) to make sure everything's up to | ||
| // date, we add the router's state to it. So the flag will only toggle | ||
| // to true once the router was also able to catch up. | ||
| var isSynced bool |
There was a problem hiding this comment.
With this change, neutrino nodes running with assume chan valid will never see isSynced set to true.
There was a problem hiding this comment.
Great regression catch, I've fixed it by passing the wallet-sync status on to isSynced, like isSynced := isWalletSynced.
In this commit, we add a new `wallet_synced` boolean field to the GetInfoResponse message. This field exposes the wallet's internal sync state with the backing chain source, providing visibility into whether the wallet has caught up to the current chain tip. This is distinct from the existing `synced_to_chain` field, which represents a composite sync state that also considers the router and blockbeat dispatcher. The new field allows callers to distinguish between wallet sync delays and other subsystem sync states.
In this commit, we extend the chainSyncInfo struct with a new isWalletSynced field that tracks the wallet's sync state independently from the composite isSynced field. The GetInfo RPC handler now populates the WalletSynced response field from this new struct field. A debug log line is added to GetInfo to help diagnose sync state issues, showing both the composite sync status and the wallet-specific sync status. Currently isWalletSynced mirrors isSynced since both ultimately derive from the same underlying wallet sync check. This prepares the plumbing for future differentiation where wallet sync state could be tracked separately from router and blockbeat dispatcher states.
In this commit, we add an integration test that verifies the wallet_synced field in GetInfoResponse correctly reflects the wallet's sync state. The test creates a node, verifies wallet_synced becomes true after initial sync, then stops the node and mines blocks while it's offline. After restart, the test polls GetInfo to observe the wallet catching up, ideally capturing the transition from wallet_synced=false to true. The test is registered in the "wallet sync" test case group.
🟡 PR Severity: MEDIUM (Override Active)
Override DetectedThis PR has a File SummaryCritical Tier (1 file):
Medium Tier (1 file):
Auto-generated Files (2 files):
Low Tier (3 files):
AnalysisThe manual override to MEDIUM severity is active and has been respected. Without the override, this PR would have been classified as CRITICAL due to changes in However, the actual code changes are relatively minor:
The override to MEDIUM appears appropriate given the limited scope of the actual changes. To change severity, modify the |
6 participants
Change Description
This PR adds a new field
wallet_syncedto theGetInfoResponseto inform the caller if the wallet is still syncing to the best chain tip.Steps to Test
See the attached itest.