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.

@Dirtybird99

Copy link
Copy Markdown

This PR edits sync_loop, so flagging a latent bug in the same function worth fixing alongside it (originally filed as #31):

Summary

Wallet_Memory.sync_loop is intended to stop when the wallet closes, but its quit check uses break inside a select — which exits the select, not the enclosing for. The sync loop therefore never terminates.

func (w *Wallet_Memory) sync_loop() {
//logger = globals.Logger
for {
select {
case <-w.Quit:
break
default:
}

func (w *Wallet_Memory) sync_loop() {
	for {
		select {
		case <-w.Quit:
			break        // <-- exits the select, NOT the for loop
		default:
		}

Impact

After Close_Encrypted_Wallet() (which only sleeps 1s "to give goroutines some time to quit", walletapi/wallet_disk.go:149-150), the sync loop keeps running: it continues polling the daemon every timeout seconds and keeps calling save_if_disk() against a wallet the caller believes is closed. Applications that close one wallet and open another accumulate leaked sync loops, duplicate RPC traffic, and concurrent writers against wallet files.

Fix

select {
case <-w.Quit:
	return
default:
}

(Any other long-running loops keyed on w.Quit are worth auditing for the same pattern.)

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