discard stale electrum responses with mismatched ids
What the developer wrote
Authored by Craig Raw
discard stale electrum responses with mismatched ids
What changed, and why it matters
This commit changes how Sparrow Wallet's connection to Electrum servers handles replies. Previously, the app tried to match an entire sent request object to a received response object to decide if the reply belonged to the current request. The new code instead compares only the numeric 'id' fields found in the JSON, and keeps reading and discarding any responses whose ids don't match. It also adds optional wire-level logging. The change is a defensive fix against 'stale' or out-of-order server responses being mistaken for the answer to the current request.
Treat as a hardening/defensive fix. Review whether the regex id extraction correctly handles batched requests, non-numeric ids, and JSON escaping; ensure the discard loop cannot be induced into an infinite loop by a malicious or buggy server that never returns matching ids. No immediate emergency action is indicated by the diff alone, but users relying on custom Electrum servers should update when a release containing this commit is available.
Security signals we found
Request/response correlation now validated by id sets rather than full object equality
Loop discards responses with mismatched ids, mitigating stale or injected response confusion
Adds disabled wire logger for traffic analysis
No explicit security framing, CVE, or advisory in commit or supplied references
Evidence from the diff
TcpTransport.pass() previously deserialized the sent and received JSON into a private Rpc class and looped until Objects.equals(sentRpc, recvRpc). That equality was based solely on the ‘id’ field, but required the whole object shape to parse successfully. The patch replaces this with a regex-based extractor (ID_PATTERN) that collects all numeric ‘id’ values into a LinkedHashSet, then loops reading responses until the set of received ids equals the set of sent ids. It also introduces a disabled-by-default ‘electrum.wire’ logger and logs discarded mismatches. The change is functionally a request/response correlation hardening, likely intended to prevent stale or unsolicited responses from being returned as the result of a pending call.
Changed components
src/main/java/com/sparrowwallet/sparrow/net/TcpTransport.javaElectrum server TCP transport layerRequest/response matching logic in pass()Inspect captured patch +28 / −26
diff --git a/src/main/java/com/sparrowwallet/sparrow/net/TcpTransport.java b/src/main/java/com/sparrowwallet/sparrow/net/TcpTransport.java
index 95528f4..8ba70ff 100644
--- a/src/main/java/com/sparrowwallet/sparrow/net/TcpTransport.java
+++ b/src/main/java/com/sparrowwallet/sparrow/net/TcpTransport.java
@@ -3,7 +3,6 @@ package com.sparrowwallet.sparrow.net;
import com.github.arteam.simplejsonrpc.server.JsonRpcServer;
import com.google.common.base.Splitter;
import com.google.common.net.HostAndPort;
-import com.google.gson.Gson;
import com.sparrowwallet.sparrow.io.Config;
import org.jetbrains.annotations.NotNull;
import org.slf4j.Logger;
@@ -17,20 +16,26 @@ import java.net.Socket;
import java.net.SocketTimeoutException;
import java.nio.charset.StandardCharsets;
import java.util.Arrays;
-import java.util.Objects;
+import java.util.Collections;
+import java.util.LinkedHashSet;
+import java.util.Set;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.locks.Condition;
import java.util.concurrent.locks.ReentrantLock;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
public class TcpTransport implements CloseableTransport, TimeoutCounter {
private static final Logger log = LoggerFactory.getLogger(TcpTransport.class);
+ private static final Logger wireLog = LoggerFactory.getLogger("electrum.wire");
public static final int DEFAULT_MAX_TIMEOUT = 34;
private static final int[] BASE_READ_TIMEOUT_SECS = {3, 8, 16, DEFAULT_MAX_TIMEOUT};
private static final int[] SLOW_READ_TIMEOUT_SECS = {34, 68, 124, 208};
public static final long PER_REQUEST_READ_TIMEOUT_MILLIS = 50;
public static final int SOCKET_READ_TIMEOUT_MILLIS = 5000;
+ private static final Pattern ID_PATTERN = Pattern.compile("\"id\"\\s*:\\s*(\\d+)");
protected final HostAndPort server;
protected final SocketFactory socketFactory;
@@ -57,7 +62,6 @@ public class TcpTransport implements CloseableTransport, TimeoutCounter {
private final SubscriptionService subscriptionService = new SubscriptionService();
private Exception lastException;
- private final Gson gson = new Gson();
public TcpTransport(HostAndPort server) {
this(server, null);
@@ -77,19 +81,22 @@ public class TcpTransport implements CloseableTransport, TimeoutCounter {
@Override
public @NotNull String pass(@NotNull String request) throws IOException {
+ Set<String> sentIdSet = extractIdSet(request);
clientRequestLock.lock();
try {
- Rpc sentRpc = request.startsWith("{") ? gson.fromJson(request, Rpc.class) : null;
- Rpc recvRpc;
- String recv;
-
//Count number of requests in batched query to increase read timeout appropriately
requestIdCount = Splitter.on("\"id\"").splitToList(request).size() - 1;
writeRequest(request);
+
+ String recv;
+ Set<String> recvIdSet;
do {
recv = readResponse();
- recvRpc = recv.startsWith("{") ? gson.fromJson(response, Rpc.class) : null;
- } while(!Objects.equals(recvRpc, sentRpc));
+ recvIdSet = extractIdSet(recv);
+ if(!sentIdSet.equals(recvIdSet)) {
+ log.info("Discarding stale response with ids " + recvIdSet + " (expected " + sentIdSet + ")");
+ }
+ } while(!sentIdSet.equals(recvIdSet));
return recv;
} finally {
@@ -106,6 +113,8 @@ public class TcpTransport implements CloseableTransport, TimeoutCounter {
throw new IllegalStateException("Socket connection has not been established.");
}
+ wireLog.info("> " + request);
+
PrintWriter out = new PrintWriter(new BufferedWriter(new OutputStreamWriter(socket.getOutputStream(), StandardCharsets.UTF_8)));
out.println(request);
out.flush();
@@ -183,6 +192,7 @@ public class TcpTransport implements CloseableTransport, TimeoutCounter {
while(running) {
try {
String received = readInputStream(in);
+ wireLog.info("< " + received);
if(received.contains("method") && !received.contains("error")) {
//Handle subscription notification
jsonRpcServer.handle(received, subscriptionService);
@@ -294,24 +304,15 @@ public class TcpTransport implements CloseableTransport, TimeoutCounter {
return readTimeoutIndex;
}
- private static class Rpc {
- public String id;
-
- @Override
- public boolean equals(Object o) {
- if(this == o) {
- return true;
- }
- if(o == null || getClass() != o.getClass()) {
- return false;
- }
- Rpc rpc = (Rpc) o;
- return Objects.equals(id, rpc.id);
+ private static Set<String> extractIdSet(String json) {
+ if(json == null || json.isEmpty()) {
+ return Collections.emptySet();
}
-
- @Override
- public int hashCode() {
- return Objects.hash(id);
+ Matcher m = ID_PATTERN.matcher(json);
+ Set<String> ids = new LinkedHashSet<>();
+ while(m.find()) {
+ ids.add(m.group(1));
}
+ return ids;
}
}
diff --git a/src/main/resources/logback.xml b/src/main/resources/logback.xml
index df2f857..7d6cba2 100644
--- a/src/main/resources/logback.xml
+++ b/src/main/resources/logback.xml
@@ -37,6 +37,7 @@
<logger name="org.springframework.web.socket.sockjs.client.SockJsClient" level="OFF" />
<logger name="org.springframework.web.socket.sockjs.client.DefaultTransportRequest" level="OFF" />
<logger name="org.xbill.DNS.dnssec.DnsSecVerifier" level="ERROR" />
+ <logger name="electrum.wire" level="OFF" />
<contextListener class="ch.qos.logback.classic.jul.LevelChangePropagator"/>
Why this scored 57/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.