refactor(walletapi): remove redundant test_connectivity from Notify_broadcaster#29
Conversation
…roadcaster The Notify_broadcaster function received a goroutine that called test_connectivity() on every Height notification. This is redundant because receiving the notification itself already proves the daemon connection is alive. The timer.Reset call at the top of the function already serves as the liveness signal by keeping Keep_Connectivity from issuing unnecessary ping checks. Removing this goroutine reduces overhead on every block notification and eliminates a potential race where test_connectivity could briefly set Connected=false if the daemon is slow to respond to the Echo/GetInfo calls that follow the Height notification.
Dirtybird99
left a comment
There was a problem hiding this comment.
Agreed on the redundancy for liveness — timer.Reset(timeout) at the top of Notify_broadcaster already feeds the Keep_Connectivity watchdog.
One consequence to be aware of: test_connectivity is also what refreshes daemon_height/daemon_topoheight (via GetInfo). Registered wallets stay fresh through the sync loop — GetEncryptedBalanceAtTopoHeight(-1) sets daemon_height = result.DHeight on success. But for unregistered wallets that call errors out before the height assignment. So once #30 also removes test_connectivity from the sync loop, an unregistered wallet's daemon_height freezes at its connect-time value (Connect() calls test_connectivity once) — breaking "show chain height while waiting to register" UX.
Suggested fix (here or in #30): update the daemon heights in the unregistered error path of GetEncryptedBalanceAtTopoHeight, or keep a GetInfo on Height notifications when !w.account.Registered.
Remove the
go test_connectivity()goroutine from the Notify_broadcaster function in the Height notification handler.Problem
Notify_broadcaster received a goroutine that called test_connectivity() on every Height notification. This is redundant because:
Changes
go test_connectivity()from the Height case in Notify_broadcasterNotes