From 3be761d97ec7717bcd0c58f40697e16dc7052e10 Mon Sep 17 00:00:00 2001 From: Davide Grilli Date: Tue, 3 Nov 2026 12:18:39 +0100 Subject: [PATCH] fix(net): bound every request with a timeout A server that stops answering without closing the socket left a running sync waiting forever. Requests now fail with TimeoutException after 30 s (10 s for the handshake, both configurable in ConnectAsync) and the connection is marked dead so the app reconnects. See ADR 0034. --- CHANGELOG.md | 4 + docs/adr/0034-bound-every-network-request.md | 28 +++++++ docs/adr/README.md | 1 + src/Core/Net/ElectrumClient.cs | 40 ++++++++-- .../Net/ElectrumClientTests.cs | 80 +++++++++++++++++++ 5 files changed, 147 insertions(+), 6 deletions(-) create mode 100644 docs/adr/0034-bound-every-network-request.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 70bf005..c97fa16 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,10 @@ release and covers the full history from the initial commit. ### Fixed +- `ElectrumClient`: every request is now bounded (30 s, 10 s for the handshake; configurable in + `ConnectAsync`). A server that stops answering without closing the socket made a running sync + wait forever; now the request fails with `TimeoutException`, the connection is marked dead + (`Disconnected` is raised, pending and later requests fail fast) and the app reconnects. - Fee rate parsing no longer depends on the system culture: on Italian/German systems `1.5` was read as `15` sat/vB (10x the intended fee). The Send form, the Donate form and the CLI `--feerate` now share `CoinAmount.TryParseFeeRate` (invariant culture, `.` or `,` as decimal diff --git a/docs/adr/0034-bound-every-network-request.md b/docs/adr/0034-bound-every-network-request.md new file mode 100644 index 0000000..ce583db --- /dev/null +++ b/docs/adr/0034-bound-every-network-request.md @@ -0,0 +1,28 @@ +# 0034. Bound every network request with a timeout + +- **Status:** Accepted +- **Area:** Network +- **Recorded:** 2026-11-03 + +## Context + +`ElectrumClient.RequestAsync` waited until the server answered or the caller cancelled. The only timeout in the stack was the 8 s ping of the ViewModel keep-alive, which does not run while a sync is in progress. A server that stops answering without closing the socket (half-open TCP, Android Doze, a stalled or hostile server) left a running sync waiting forever, with no error and no automatic recovery. + +## Decision + +- Every request has a timeout, `ElectrumClient.DefaultRequestTimeout` (30 s). The `server.version` handshake has its own, shorter `DefaultHandshakeTimeout` (10 s). Both are parameters of `ConnectAsync` (`requestTimeout`, `handshakeTimeout`), so tests use a few hundred milliseconds. +- The timer starts when the request obtains its in-flight slot: waiting behind the cap (ADR 0009) does not count. +- On expiry the request fails with `TimeoutException` (naming the method and server) and the **whole connection is marked dead**: the other pending requests fail, `IsConnected` becomes false, `Disconnected` is raised and later requests fail fast with `IOException`. After a missed answer the socket cannot be trusted, and the ViewModel reconnects through the existing path. A late reply is dropped because the request is no longer pending. +- The keep-alive is left as it is (it skips ticks while syncing): the request timeout now bounds a stalled sync, so a second mechanism is not needed. + +## Consequences + +- A stalled server makes a sync fail within the timeout instead of hanging; the existing retry and reconnect logic takes over. +- A legitimately slow reply above 30 s is treated as a dead connection. The largest realistic replies (header batches, large transactions, busy-address histories) are far below that on a working link; the value is a constructor parameter if a deployment needs more. +- One slow request tears down the shared connection and the requests in flight with it. This is deliberate: it is cheaper than a half-open socket that keeps accepting writes. + +## References + +- `src/Core/Net/ElectrumClient.cs` +- `tests/PalladiumWallet.Tests/Net/ElectrumClientTests.cs` +- Related: 0009 diff --git a/docs/adr/README.md b/docs/adr/README.md index 085f6ab..831697c 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -37,6 +37,7 @@ One ADR per feature or significant design decision. Format: `template.md`. Workf | 0031 | [Explicitly defer multisig, Lightning, Tor and hardware wallets](0031-defer-multisig-lightning-and-hardware-wallets.md) | Scope | Accepted | | 0032 | [Run tests, fuzz smoke and docs checks in GitHub Actions](0032-github-actions-ci.md) | Testing | Accepted | | 0033 | [Test the UI layer and CLI headlessly, gate coverage](0033-test-strategy-beyond-core.md) | Testing | Accepted | +| 0034 | [Bound every network request with a timeout](0034-bound-every-network-request.md) | Network | Accepted | ADRs `0002` onwards were written retroactively on 2026-11-03 from the code, comments and changelog. If one misstates the original reasoning, fix it in a pull request. diff --git a/src/Core/Net/ElectrumClient.cs b/src/Core/Net/ElectrumClient.cs index a5954ba..4f2ad40 100644 --- a/src/Core/Net/ElectrumClient.cs +++ b/src/Core/Net/ElectrumClient.cs @@ -41,6 +41,17 @@ public sealed class ElectrumClient : IAsyncDisposable // concurrent requests before throttling — no single constant is right for // every server, so callers may raise/lower it instead of recompiling. public const int DefaultMaxInFlight = 32; + + // Every request is bounded: a server that stops answering without closing the socket + // (half-open TCP, Android doze, a stalled or hostile server) would otherwise leave the + // caller waiting forever. The timer starts once the request holds an in-flight slot, so + // queueing behind the cap does not count. On expiry the request fails with + // TimeoutException and the whole connection is marked dead (pending requests fail, + // Disconnected is raised): after a missed answer the socket cannot be trusted, and a + // late reply is dropped because the request is no longer pending. + public static readonly TimeSpan DefaultRequestTimeout = TimeSpan.FromSeconds(30); + public static readonly TimeSpan DefaultHandshakeTimeout = TimeSpan.FromSeconds(10); + private readonly TimeSpan _requestTimeout; private readonly SemaphoreSlim _inFlight; private long _nextId; @@ -54,8 +65,9 @@ public sealed class ElectrumClient : IAsyncDisposable public event Action? Disconnected; private ElectrumClient(TcpClient tcp, Stream stream, string host, int port, bool useSsl, - int maxInFlight) + int maxInFlight, TimeSpan requestTimeout) { + _requestTimeout = requestTimeout; _tcp = tcp; _stream = stream; Host = host; @@ -68,7 +80,8 @@ public sealed class ElectrumClient : IAsyncDisposable public static async Task ConnectAsync(string host, int port, bool useSsl, CertificatePinStore? pins = null, CancellationToken ct = default, - int maxInFlight = DefaultMaxInFlight) + int maxInFlight = DefaultMaxInFlight, TimeSpan? requestTimeout = null, + TimeSpan? handshakeTimeout = null) { var tcp = new TcpClient { NoDelay = true }; try @@ -97,8 +110,10 @@ public sealed class ElectrumClient : IAsyncDisposable stream = ssl; } - var client = new ElectrumClient(tcp, stream, host, port, useSsl, maxInFlight); - await client.RequestAsync("server.version", ct, ClientName, ProtocolVersion); + var client = new ElectrumClient(tcp, stream, host, port, useSsl, maxInFlight, + requestTimeout ?? DefaultRequestTimeout); + await client.RequestCoreAsync(handshakeTimeout ?? DefaultHandshakeTimeout, + "server.version", ct, [ClientName, ProtocolVersion]); return client; } catch @@ -108,8 +123,12 @@ public sealed class ElectrumClient : IAsyncDisposable } } - public async Task RequestAsync(string method, CancellationToken ct = default, - params object?[] parameters) + public Task RequestAsync(string method, CancellationToken ct = default, + params object?[] parameters) => + RequestCoreAsync(_requestTimeout, method, ct, parameters); + + private async Task RequestCoreAsync(TimeSpan timeout, string method, + CancellationToken ct, object?[] parameters) { // Gate before going in-flight: beyond MaxInFlight pending requests // we wait for a response to free a slot, instead of flooding the server. @@ -144,6 +163,15 @@ public sealed class ElectrumClient : IAsyncDisposable _pending.TryRemove(id, out _); tcs.TrySetCanceled(ct); }); + + using var timeoutCts = new CancellationTokenSource(timeout); + await using var timeoutRegistration = timeoutCts.Token.Register(() => + { + if (!_pending.TryRemove(id, out _)) return; + tcs.TrySetException(new TimeoutException( + $"No response to '{method}' from {Host}:{Port} within {timeout.TotalSeconds:0.#}s.")); + try { _cts.Cancel(); } catch (ObjectDisposedException) { } + }); return await tcs.Task; } finally diff --git a/tests/PalladiumWallet.Tests/Net/ElectrumClientTests.cs b/tests/PalladiumWallet.Tests/Net/ElectrumClientTests.cs index cb1d8bf..dd2c910 100644 --- a/tests/PalladiumWallet.Tests/Net/ElectrumClientTests.cs +++ b/tests/PalladiumWallet.Tests/Net/ElectrumClientTests.cs @@ -147,6 +147,86 @@ public class ElectrumClientTests Assert.Equal(big, result); } + [Fact] + public async Task A_request_without_an_answer_fails_with_TimeoutException() + { + await using var server = new FakeElectrumServer(); + server.Handle("server.banner", _ => FakeElectrumServer.NoResponse); + await using var client = await ElectrumClient.ConnectAsync(server.Host, server.Port, + useSsl: false, requestTimeout: TimeSpan.FromMilliseconds(300)); + + var ex = await Assert.ThrowsAsync(() => client.BannerAsync().WaitAsync(Timeout)); + Assert.Contains("server.banner", ex.Message); + } + + [Fact] + public async Task After_a_timeout_the_client_is_disconnected_and_later_requests_fail_fast() + { + await using var server = new FakeElectrumServer(); + server.Handle("server.banner", _ => FakeElectrumServer.NoResponse); + await using var client = await ElectrumClient.ConnectAsync(server.Host, server.Port, + useSsl: false, requestTimeout: TimeSpan.FromMilliseconds(300)); + var disconnected = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + client.Disconnected += _ => disconnected.TrySetResult(); + + await Assert.ThrowsAsync(() => client.BannerAsync().WaitAsync(Timeout)); + await disconnected.Task.WaitAsync(Timeout); + + Assert.False(client.IsConnected); + await Assert.ThrowsAsync(() => client.BannerAsync().WaitAsync(Timeout)); + } + + [Fact] + public async Task A_timeout_fails_the_other_pending_requests_too() + { + await using var server = new FakeElectrumServer(); + server.Handle("server.banner", _ => FakeElectrumServer.NoResponse); + await using var client = await ElectrumClient.ConnectAsync(server.Host, server.Port, + useSsl: false, requestTimeout: TimeSpan.FromMilliseconds(300)); + + var a = client.BannerAsync(); + var b = client.BannerAsync(); + await Assert.ThrowsAnyAsync(() => a.WaitAsync(Timeout)); + await Assert.ThrowsAnyAsync(() => b.WaitAsync(Timeout)); + } + + [Fact] + public async Task A_slow_answer_within_the_timeout_still_succeeds() + { + await using var server = new FakeElectrumServer(); + server.Handle("server.banner", _ => { Thread.Sleep(200); return "hello"; }); + await using var client = await ElectrumClient.ConnectAsync(server.Host, server.Port, + useSsl: false, requestTimeout: TimeSpan.FromSeconds(10)); + + Assert.Equal("hello", await client.BannerAsync().WaitAsync(Timeout)); + Assert.True(client.IsConnected); + } + + [Fact] + public async Task A_handshake_that_never_completes_times_out() + { + await using var server = new FakeElectrumServer(); + server.Handle("server.version", _ => FakeElectrumServer.NoResponse); + + await Assert.ThrowsAsync(() => ElectrumClient.ConnectAsync( + server.Host, server.Port, useSsl: false, + handshakeTimeout: TimeSpan.FromMilliseconds(300)).WaitAsync(Timeout)); + } + + [Fact] + public async Task Waiting_for_an_in_flight_slot_does_not_count_towards_the_timeout() + { + await using var server = new FakeElectrumServer(); + server.Handle("server.banner", _ => { Thread.Sleep(250); return "ok"; }); + await using var client = await ElectrumClient.ConnectAsync(server.Host, server.Port, + useSsl: false, maxInFlight: 1, requestTimeout: TimeSpan.FromMilliseconds(600)); + + // Three serial requests of ~250 ms each: the last one waits ~500 ms for the slot. + var all = await Task.WhenAll(client.BannerAsync(), client.BannerAsync(), client.BannerAsync()) + .WaitAsync(Timeout); + Assert.All(all, b => Assert.Equal("ok", b)); + } + [Fact] public async Task La_cancellazione_del_token_annulla_la_richiesta_pendente() {