From 4cd5fab7360edce748e8ac74dea5c290fdbc01fa Mon Sep 17 00:00:00 2001 From: Davide Grilli Date: Sun, 19 Jul 2026 17:11:39 +0200 Subject: [PATCH] fix(sync): bound keep-alive ping and resume a sync stuck on lock/unlock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three issues surfaced by testing the previous lock/unlock reconnect fix on Android with a large wallet: - The keep-alive ping had no timeout, so a "half-open" TCP connection (remote end gone with no FIN/RST ever delivered, the common outcome of Doze/mobile-radio suspend after a longer lock) left it awaiting a response that never arrives — the failure path that tears the dead client down was never reached. Bound the ping to 8s and added a re-entrancy guard so overlapping 20s ticks can't pile up concurrent reconnect attempts while one is stuck. - CheckConnectionOnResumeAsync bailed out whenever a sync was already in progress, so a lock/unlock during an active sync (which has no per-request timeout of its own) left it hung indefinitely instead of recovering. It now cancels the stuck sync and tears down the dead client, marking the interruption as self-inflicted (_resumeRecovering) so the UI shows "reconnecting" and restarts immediately instead of a transient error message. - WalletSynchronizer.ExportCaches persisted raw transaction bytes only for already-verified transactions, discarding anything downloaded but not yet through Merkle-proof verification when a sync was interrupted mid-way — forcing a full re-download of a large wallet's transactions on every resume instead of resuming straight into proof verification. Track confirmed txids at download time (_confirmedTxids), independent of verification status, and export/preload against that instead. Also unified the three sync progress messages onto one template reporting against the sync's total transaction/proof counts rather than this session's download count, so a resumed sync immediately shows "transactions n/n" instead of a misleading "0/0" before jumping into proof verification. --- .../ViewModels/MainWindowViewModel.Sync.cs | 28 ++++-- src/App/ViewModels/MainWindowViewModel.cs | 98 ++++++++++++++----- src/Core/Spv/WalletSynchronizer.cs | 34 ++++++- 3 files changed, 125 insertions(+), 35 deletions(-) diff --git a/src/App/ViewModels/MainWindowViewModel.Sync.cs b/src/App/ViewModels/MainWindowViewModel.Sync.cs index fe6480b..f69d098 100644 --- a/src/App/ViewModels/MainWindowViewModel.Sync.cs +++ b/src/App/ViewModels/MainWindowViewModel.Sync.cs @@ -314,11 +314,13 @@ public partial class MainWindowViewModel } catch (OperationCanceledException) { - // Intentional cancellation due to server change request — not an error. + // Intentional cancellation: a server change request, or CheckConnectionOnResumeAsync + // recovering a sync stuck on a socket that died while the app was suspended + // (_resumeRecovering) — neither is an error the user needs to see as one. cancelled = true; IsConnected = false; - ConnectionStatus = Loc.Tr("conn.none"); - ConnectionStatusShort = Loc.Tr("conn.none"); + ConnectionStatus = _resumeRecovering ? Loc.Tr("conn.reconnecting") : Loc.Tr("conn.none"); + ConnectionStatusShort = _resumeRecovering ? Loc.Tr("conn.reconnecting") : Loc.Tr("conn.none"); StatusMessage = ""; } catch (CertificatePinMismatchException ex) @@ -331,9 +333,22 @@ public partial class MainWindowViewModel catch (Exception ex) { IsConnected = _client?.IsConnected == true; - ConnectionStatus = IsConnected ? ConnectionStatus : Loc.Tr("conn.none"); - ConnectionStatusShort = IsConnected ? Loc.Tr("conn.connectedto") : Loc.Tr("conn.none"); - StatusMessage = $"{Loc.Tr("msg.error")}: {DescribeError(ex)}"; + if (_resumeRecovering && !IsConnected) + { + // We tore the connection down ourselves (see CheckConnectionOnResumeAsync); + // whether that surfaces here as a cancellation or as a transport exception + // is a race, not a real failure — show "reconnecting" and restart right away + // instead of a scary error message and a wait for the next keep-alive tick. + cancelled = true; + ConnectionStatus = Loc.Tr("conn.reconnecting"); + ConnectionStatusShort = Loc.Tr("conn.reconnecting"); + } + else + { + ConnectionStatus = IsConnected ? ConnectionStatus : Loc.Tr("conn.none"); + ConnectionStatusShort = IsConnected ? Loc.Tr("conn.connectedto") : Loc.Tr("conn.none"); + StatusMessage = $"{Loc.Tr("msg.error")}: {DescribeError(ex)}"; + } if (_account is not null) { _syncFailed = true; @@ -345,6 +360,7 @@ public partial class MainWindowViewModel finally { IsSyncing = false; + _resumeRecovering = false; } // If cancelled due to a server change, restart immediately with the new server. diff --git a/src/App/ViewModels/MainWindowViewModel.cs b/src/App/ViewModels/MainWindowViewModel.cs index a4a3cb4..9be6aeb 100644 --- a/src/App/ViewModels/MainWindowViewModel.cs +++ b/src/App/ViewModels/MainWindowViewModel.cs @@ -77,6 +77,7 @@ public partial class MainWindowViewModel : ViewModelBase // ---- keep-alive ---- private bool _autoReconnect; private bool _syncFailed; + private bool _resumeRecovering; private readonly DispatcherTimer _keepAliveTimer; // ---- server UI sync ---- @@ -161,41 +162,60 @@ public partial class MainWindowViewModel : ViewModelBase _ = CheckForUpdatesAsync(); } + private bool _keepAliveRunning; + private async System.Threading.Tasks.Task KeepAliveTickAsync() { - if (IsSyncing) + // Re-entrancy guard: without it, a ping stuck on a half-open socket (see + // below) would let every subsequent 20s tick pile up another concurrent + // ping/reconnect attempt on top of it. + if (IsSyncing || _keepAliveRunning) return; - if (_client is { IsConnected: true } client) + _keepAliveRunning = true; + try { - // If the wallet is open and the last sync failed, retry automatically. - if (_syncFailed && _account is not null) + if (_client is { IsConnected: true } client) { - await ConnectAndSync(); - return; - } - try - { - await client.PingAsync(); - } - catch - { - // TcpClient.Connected only reflects the last known socket state, so a - // connection killed silently while the app was suspended (e.g. Android - // Doze after screen lock) still reports IsConnected == true. A failed - // ping is the only reliable signal here: tear the dead client down so - // the next tick reconnects instead of retrying forever on a dead socket. - await DisconnectAsync(); - if (_autoReconnect) + // If the wallet is open and the last sync failed, retry automatically. + if (_syncFailed && _account is not null) { - ConnectionStatus = Loc.Tr("conn.reconnecting"); await ConnectAndSync(); + return; + } + try + { + // A "half-open" TCP connection (remote end gone with no FIN/RST + // ever delivered — the common outcome of Android Doze/mobile-radio + // suspend killing the route silently) never fails the write, so + // PingAsync would otherwise await a response that never arrives. + // Bound it explicitly instead of relying on it to throw. + using var timeoutCts = new System.Threading.CancellationTokenSource(System.TimeSpan.FromSeconds(8)); + await client.PingAsync(timeoutCts.Token); + } + catch + { + // TcpClient.Connected only reflects the last known socket state, so a + // connection killed silently while the app was suspended still reports + // IsConnected == true. A failed/timed-out ping is the only reliable + // signal here: tear the dead client down so the next tick reconnects + // instead of retrying forever on a dead socket. + await DisconnectAsync(); + if (_autoReconnect) + { + ConnectionStatus = Loc.Tr("conn.reconnecting"); + await ConnectAndSync(); + } } } + else if (_autoReconnect) + { + ConnectionStatus = Loc.Tr("conn.reconnecting"); + await ConnectAndSync(); + } } - else if (_autoReconnect) + finally { - ConnectionStatus = Loc.Tr("conn.reconnecting"); - await ConnectAndSync(); + _keepAliveRunning = false; } } @@ -206,7 +226,35 @@ public partial class MainWindowViewModel : ViewModelBase /// during Doze). public async System.Threading.Tasks.Task CheckConnectionOnResumeAsync() { - if (IsSyncing) return; + if (IsSyncing) + { + // A sync in progress when the phone locked may be stuck awaiting a + // response on a socket that died silently while suspended (same + // half-open-TCP scenario as the keep-alive ping, but sync requests have + // no timeout of their own). Cancel it and tear the client down instead + // of leaving it hung forever. _resumeRecovering tells ConnectAndSync's + // error handling that this interruption is self-inflicted and expected, + // so it shows "reconnecting" and restarts immediately instead of + // flashing a scary error message and waiting for the next keep-alive tick. + _resumeRecovering = true; + ConnectionStatus = Loc.Tr("conn.reconnecting"); + ConnectionStatusShort = Loc.Tr("conn.reconnecting"); + _syncCts.Cancel(); + // Deliberately not DisconnectAsync() here: it also nulls _synchronizer, + // which still holds whatever this sync already downloaded/verified (e.g. + // thousands of Merkle proofs on a large wallet). Nulling it before the + // cancelled ConnectAndSync unwinds would discard that progress, forcing a + // full restart instead of a resume. Only tear down the dead socket here; + // ConnectAndSync's own cancellation handling persists the partial cache + // (PersistPartialTxCache) before it recreates the synchronizer — same + // sequencing already used for the "server changed mid-sync" case. + if (_client is { } deadClient) + { + _client = null; + try { await deadClient.DisposeAsync(); } catch { } + } + return; + } await KeepAliveTickAsync(); } diff --git a/src/Core/Spv/WalletSynchronizer.cs b/src/Core/Spv/WalletSynchronizer.cs index 2048db9..7e3886e 100644 --- a/src/Core/Spv/WalletSynchronizer.cs +++ b/src/Core/Spv/WalletSynchronizer.cs @@ -75,6 +75,14 @@ public sealed class WalletSynchronizer(IWalletAccount account, ElectrumClient cl private readonly ConcurrentDictionary _txCache = new(); + // txids known confirmed (height > 0) as of download time, independent of whether their + // Merkle proof has been verified yet — lets ExportCaches persist raw tx bytes for a + // confirmed transaction interrupted before verification, instead of forcing a + // re-download on the next sync just because _verifiedAtHeight hasn't caught up. Every + // entry here has txHeights[txid] > 0 at the time it was recorded, i.e. server-confirmed; + // unconfirmed (mempool/RBF-able) transactions are deliberately never added. + private readonly ConcurrentDictionary _confirmedTxids = new(); + // Concurrent: written incrementally by individual merkle-verification tasks as they // complete (§7.4 progressive verification), not just once after they all finish. private readonly ConcurrentDictionary _verifiedAtHeight = new(); @@ -116,7 +124,12 @@ public sealed class WalletSynchronizer(IWalletAccount account, ElectrumClient cl Dictionary? anchoredUpTo = null) { foreach (var (txid, hex) in rawTxHex) + { _txCache.TryAdd(txid, Transaction.Parse(hex, network)); + // ExportCaches only ever wrote confirmed transactions here (see its own + // filter), so every preloaded entry is safe to mark confirmed too. + _confirmedTxids.TryAdd(txid, 0); + } foreach (var (txid, height) in verifiedAt) if (!_verifiedAtHeight.ContainsKey(txid)) _verifiedAtHeight[txid] = height; @@ -147,7 +160,7 @@ public sealed class WalletSynchronizer(IWalletAccount account, ElectrumClient cl Dictionary AnchoredUpTo) ExportCaches(Network network) { - var rawHex = _verifiedAtHeight.Keys + var rawHex = _confirmedTxids.Keys .Where(_txCache.ContainsKey) .ToDictionary(txid => txid, txid => _txCache[txid].ToHex()); @@ -232,17 +245,30 @@ public sealed class WalletSynchronizer(IWalletAccount account, ElectrumClient cl && (!_verifiedAtHeight.TryGetValue(kv.Key, out var h) || h != kv.Value)) .ToList(); + // Total/already-cached counts (not just this session's downloads): on a sync resumed + // after an interruption, `missing` is often empty because everything was already + // fetched last time (see _confirmedTxids/PreloadCaches) — reporting against the total + // shows "n/n transactions" immediately instead of a misleading "0/0" before jumping + // straight to proof verification. + var totalTx = txHeights.Count; + var alreadyCached = totalTx - missing.Count; + + string DownloadVerifyStatus(int downloaded, int verified) => + $"transactions {downloaded}/{totalTx}, proofs {verified}/{toVerify.Count}…"; + if (missing.Count > 0 || toVerify.Count > 0) - Progress?.Invoke($"downloading {missing.Count} txs, verifying {toVerify.Count} proofs…"); + Progress?.Invoke(DownloadVerifyStatus(alreadyCached, 0)); var dlDone = 0; await Task.WhenAll(missing.Select(txid => RetryOnBusyAsync(async () => { var raw = await client.GetTransactionAsync(txid, ct); _txCache[txid] = Transaction.Parse(raw, network); + if (txHeights[txid] > 0) + _confirmedTxids.TryAdd(txid, 0); var n = Interlocked.Increment(ref dlDone); if (n % 50 == 0 || n == missing.Count) - Progress?.Invoke($"tx {n}/{missing.Count}, proofs 0/{toVerify.Count}…"); + Progress?.Invoke(DownloadVerifyStatus(alreadyCached + n, 0)); }, ct))); SyncResult BuildSnapshot() => @@ -273,7 +299,7 @@ public sealed class WalletSynchronizer(IWalletAccount account, ElectrumClient cl _verifiedAtHeight[txid] = height; var n = Interlocked.Increment(ref merkDone); if (n % 50 == 0 || n == toVerify.Count) - Progress?.Invoke($"tx {missing.Count}/{missing.Count}, proofs {n}/{toVerify.Count}…"); + Progress?.Invoke(DownloadVerifyStatus(totalTx, n)); if (n % PartialResultBatchSize == 0) PartialResult?.Invoke(BuildSnapshot()); }, ct)).ToList();