Skip to content

fix(walletapi): run SyncHistory in goroutine to avoid blocking sync loop#26

Open
moralpriest wants to merge 1 commit into
DEROFDN:community-devfrom
moralpriest:fix/walletapi-async-synchistory
Open

fix(walletapi): run SyncHistory in goroutine to avoid blocking sync loop#26
moralpriest wants to merge 1 commit into
DEROFDN:community-devfrom
moralpriest:fix/walletapi-async-synchistory

Conversation

@moralpriest

Copy link
Copy Markdown

SyncHistory performs a binary-search scan of the blockchain to find transactions affecting the wallet, making sequential RPC calls that can take minutes on restored wallets. When called synchronously inside Sync_Wallet_Memory_With_Daemon_internal, it blocks the sync loop and prevents daemon_height updates until completed.

Problem

After wallet restoration, SyncHistory takes minutes to complete. During this time the sync loop is frozen — no height updates, no balance updates, sync indicator appears stuck.

Changes

  • Change w.SyncHistory(scid) to go w.SyncHistory(scid)
  • SyncHistory now runs as a background goroutine
  • The sync loop continues to poll and update heights independently

Notes

  • SyncHistory errors are logged but do not interrupt the sync loop
  • History scanning runs in the background while balance polling continues
  • The existing sync_multilock prevents concurrent SyncHistory calls

SyncHistory performs a binary-search scan of the blockchain to find
transactions affecting the wallet, making sequential RPC calls that can
take minutes on restored wallets. When called synchronously inside
Sync_Wallet_Memory_With_Daemon_internal, it blocks the sync loop and
prevents daemon_height updates until completed.

Running SyncHistory as a goroutine allows the sync loop to continue
polling and updating heights while history is scanned in the background.

@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.

Reviewed against the community-dev base. The goal is right — a minutes-long SyncHistory shouldn't freeze the sync loop — but as-is this introduces two crash-class races:

  1. Standalone, this is a fatal crash, not a panic. sync_loop iterates the entries map (for k := range w.account.EntriesNative, daemon_communication.go:209) while the new goroutine writes it (w.account.EntriesNative[scid] = entries, lines 598/603). Concurrent map iteration + write is a Go runtime fatal throw — the deferred recover() inside SyncHistory cannot catch it; the whole wallet process dies. PR #30 removes that loop, so this PR hard-depends on #30 landing first — worth stating in the description and enforcing in merge order.
  2. Even with #30 merged, save_if_disk()Save_Wallet()json.Marshal(w.account) iterates EntriesNative with no lock — and it runs both periodically in sync_loop and on the line right after go w.SyncHistory(scid). SyncHistory holds only sync_multilock; the save path holds neither. Same fatal class — or a torn account snapshot encrypted to disk. The map/slice mutations in SyncHistory and the marshal in Save_Wallet need a common mutex (e.g. wrap SyncHistory's EntriesNative writes in w.Lock() and take the same lock around the account marshal).
  3. Minor: every balance change spawns a goroutine that queues on sync_multilock; during a long restore these pile up unboundedly. A coalescing guard fixes that and shrinks the race window (see inline comment).

w.account.Balance[scid] = b
w.Unlock()
w.SyncHistory(scid) // also update statement
go w.SyncHistory(scid) // also update statement (run in background to avoid blocking sync loop)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggest coalescing instead of unconditional spawn, so repeated balance changes during a long scan don't queue goroutines on sync_multilock (needs a synchistory_pending int32 field and sync/atomic import):

if atomic.CompareAndSwapInt32(&w.synchistory_pending, 0, 1) {
	go func() {
		defer atomic.StoreInt32(&w.synchistory_pending, 0)
		w.SyncHistory(scid)
	}()
}

@Dirtybird99

Copy link
Copy Markdown

Follow-up to my review above — this is the concrete locking defect that makes go w.SyncHistory(scid) in this PR unsafe, and the fix that would unblock it (originally filed as #33):

Summary

Commit 6f9bd2c converted the transaction-history readers (Show_Transfers, Get_Payments_DestinationPort, Get_Payments_TXID, GetTXKey) from w.Lock() to w.RLock(). But the writer of w.account.EntriesNativeSyncHistory — never takes w.Lock() at all; it holds only the package-level sync_multilock, which readers don't touch. The RWMutex therefore provides no exclusion against the one goroutine that actually mutates the map.

Writer (holds only sync_multilock):

entries = entries[:i]
i--
w.account.EntriesNative[scid] = entries
logger.Info("syncing loop skipped ", "i", i, "skip", skip)
}
if i <= 0 {
w.account.EntriesNative[scid] = entries[:0] // discard all entries
logger.Info("syncing loop discarding all entries", "i", i)

Readers (RLock only — no shared lock with the writer):

derohe/walletapi/wallet.go

Lines 266 to 268 in 6f9bd2c

func (w *Wallet_Memory) Show_Transfers(scid crypto.Hash, coinbase bool, in bool, out bool, min_height, max_height uint64, sender, receiver string, dstport, srcport uint64) []rpc.Entry {
w.RLock()
defer w.RUnlock()

Additionally, Save_Wallet serializes the whole account with json.Marshal(w.account) — which iterates EntriesNative (map + entry slices) — without any lock shared with SyncHistory:

account_serialized, err := json.Marshal(w.account)

Impact

  • Wallet RPC server calls (GetTransfers, etc.) reading entries while the sync loop's SyncHistory prunes/appends them: concurrent map read/write → runtime fatal throw (not a recoverable panic), or torn slice reads.
  • Save_Wallet marshaling during a history scan: same fatal class, or a corrupted account snapshot encrypted to disk.
  • This is also the root cause that makes PR fix(walletapi): run SyncHistory in goroutine to avoid blocking sync loop #26 (go w.SyncHistory(scid)) unsafe as written — see the review discussion there. Fixing locking here first would unblock that PR.

Fix

Have SyncHistory (and synchistory_block's entry appends) perform all w.account.EntriesNative mutations under w.Lock() — the readers' RLocks then actually exclude the writer — and take w.RLock() around the json.Marshal(w.account) in Save_Wallet. Run go test -race ./walletapi/... with a simulated concurrent sync to confirm.

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