Skip to content

fix(walletapi): simplify sync loop to zero-SCID-only polling#30

Open
moralpriest wants to merge 1 commit into
DEROFDN:community-devfrom
moralpriest:fix/walletapi-sync-loop-simplification
Open

fix(walletapi): simplify sync loop to zero-SCID-only polling#30
moralpriest wants to merge 1 commit into
DEROFDN:community-devfrom
moralpriest:fix/walletapi-sync-loop-simplification

Conversation

@moralpriest

Copy link
Copy Markdown

Simplify the sync loop to only sync the native DERO (zero SCID) and remove the test_connectivity() gate from the loop condition.

Problem

The previous sync loop made RPC calls for every known token SCID on each iteration. This added overhead and created a concurrent map access issue when SyncHistory ran in the background (see PR #26). Additionally, the loop condition used test_connectivity() which made redundant RPC calls every 3 seconds.

Changes

  • Remove test_connectivity() from the loop condition; IsDaemonOnline() is sufficient
  • Replace EntriesNative iteration with zero-SCID-only sync
  • Token balances sync on demand when wallet API callers request them

Notes

  • This prevents concurrent map iteration and write panic when SyncHistory runs as a goroutine
  • Balance queries from the wallet API trigger per-SCID sync on demand
  • Reduces per-iteration RPC calls from N (all token SCIDs) to 1 (native DERO only)

Remove the test_connectivity() gate from the sync loop condition.
IsDaemonOnline() is sufficient to determine connection state since
it checks both WS and RPC client availability.

Remove EntriesNative iteration from the sync loop. Previously the
loop synced every known token SCID on each iteration, making many
sequential RPC calls. Now it only syncs the native DERO (zero SCID).
Token balances sync on demand when wallet API callers explicitly
request them.

This change prevents a concurrent map iteration and write panic when
running SyncHistory as a background goroutine (which also accesses
EntriesNative).

@Dirtybird99 Dirtybird99 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The !IsDaemonOnline() guard flip is a genuine improvement (the old code fell through to a failing sync attempt when the socket was nil), and Sync_Wallet_Memory_With_Daemon() is exactly internal(zerohash), so the registered-no-entries path is unchanged. Three qualifications:

  1. "Token balances sync on demand when wallet API callers request them" holds for the wallet JSON-RPC server (rpc_getbalance.go syncs the SCID first) but not for library consumers calling Get_Balance_scid()/Get_Balance() directly — those return the stored map value with no sync, so embedded-walletapi UIs will show stale token balances indefinitely, and token history stops updating outside RPC GetBalance calls. Deserves a loud release-note callout.
  2. "Prevents concurrent map iteration and write panic" — this removes the sync_loop range, but the second race from #26 remains: Save_Wallet()'s json.Marshal(w.account) iterates EntriesNative unlocked while a background SyncHistory writes it (details on #26).
  3. For unregistered wallets, daemon_height now freezes after connect (details and a suggested fix on #29).

Also worth stating explicitly in the description: #26 must not merge before this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants