bound the bitcoin core rpc connect and read timeouts, exempting the methods that rescan synchronously
What changed, and why it matters
This commit fixes a bug where Sparrow Wallet's connection to a Bitcoin Core node could hang forever if the node accepted the connection but never replied. The patch adds sensible time limits for making and reading connections, while carefully allowing very long waits only for two specific Bitcoin Core operations that legitimately take hours. Without the fix, a user connecting to a slow, stuck, or maliciously unresponsive node could have their wallet interface freeze indefinitely.
Review and merge the patch. Ensure the read timeout floor remains at or above the Electrum client timeout as the comment states. Consider whether 300s is appropriate for all non-rescan RPCs and monitor for any reports of legitimate calls being cut off. No immediate incident response is indicated beyond applying the fix.
Security signals we found
Denial-of-service via unresponsive Bitcoin Core RPC: a stuck or malicious node could hang the calling thread forever
Missing network timeouts on HttpURLConnection defaults (0 = infinite)
Tor/onion connections require longer connect timeout due to circuit setup
Synchronous rescan RPCs (importdescriptors, loadwallet) legitimately exceed normal read timeout and are exempted
Regression test added to prevent reintroduction of infinite hang
Evidence from the diff
BitcoindTransport previously used java.net.HttpURLConnection without setting connect or read timeouts, which default to 0 (infinite). The patch sets connectTimeout to 15s (60s for onion/proxied connections to allow Tor circuit build) and readTimeout to max(300s, Config.get().getMaxServerTimeout()*1000). It exempts importdescriptors and loadwallet from the read timeout because those RPCs perform synchronous rescans that can block for hours. A new isRescanningMethod() helper parses the JSON-RPC method field with Jackson to avoid matching parameter values. A regression test verifies that a silent server now triggers a SocketTimeoutException instead of hanging.
Changed components
src/main/java/com/sparrowwallet/sparrow/net/cormorant/bitcoind/BitcoindTransport.javaBitcoin Core RPC transport layer in Sparrow WalletCormorant bitcoind integrationInspect captured patch +150 / −1
### src/main/java/com/sparrowwallet/sparrow/net/cormorant/bitcoind/BitcoindTransport.java
@@ -1,8 +1,12 @@
package com.sparrowwallet.sparrow.net.cormorant.bitcoind;
+import com.fasterxml.jackson.core.JsonFactory;
+import com.fasterxml.jackson.core.JsonParser;
+import com.fasterxml.jackson.core.JsonToken;
import com.github.arteam.simplejsonrpc.client.Transport;
import com.sparrowwallet.drongo.Network;
import com.sparrowwallet.sparrow.AppServices;
+import com.sparrowwallet.sparrow.io.Config;
import com.sparrowwallet.sparrow.io.Server;
import com.sparrowwallet.sparrow.io.Storage;
import com.sparrowwallet.sparrow.net.Protocol;
@@ -18,12 +22,26 @@
import java.nio.file.Path;
import java.security.cert.Certificate;
import java.util.Base64;
+import java.util.Set;
public class BitcoindTransport implements Transport {
private static final Logger log = LoggerFactory.getLogger(BitcoindTransport.class);
public static final String COOKIE_FILENAME = ".cookie";
+ //Neither timeout was set, and HttpURLConnection defaults both to 0 (infinite), so a node that could not be reached, or that accepted the connection but never replied, hung
+ //the calling thread forever. A node is expected to be quickly reachable, but connecting to an onion address must first build a circuit.
+ private static final int CONNECT_TIMEOUT_MILLIS = 15_000;
+ private static final int ONION_CONNECT_TIMEOUT_MILLIS = 60_000;
+ //A backstop against a hung thread rather than a limit on how long the user waits, since TcpTransport already bounds the Electrum requests these calls serve. Set well beyond
+ //any legitimate call, and never below the timeout the Electrum layer above allows.
+ private static final int READ_TIMEOUT_MILLIS = 300_000;
+ //A rescan is synchronous, so importing a descriptor with an early birthday can block for hours, as can loading a wallet Bitcoin Core must first catch up to the tip
+ private static final Set<String> RESCANNING_METHODS = Set.of("importdescriptors", "loadwallet");
+ private static final JsonFactory JSON_FACTORY = new JsonFactory();
+
private final Server bitcoindServer;
+ //Package visible so tests need not wait it out
+ int readTimeoutMillis;
private URL bitcoindUrl;
private File cookieFile;
private Long cookieFileTimestamp;
@@ -42,6 +60,7 @@ public BitcoindTransport(Server bitcoindServer, String bitcoindWallet, File bitc
private BitcoindTransport(Server bitcoindServer, String bitcoindWallet) {
this.bitcoindServer = bitcoindServer;
+ this.readTimeoutMillis = Math.max(READ_TIMEOUT_MILLIS, Config.get().getMaxServerTimeout() * 1000);
try {
String serverUrl = bitcoindServer.getUrl();
if(!bitcoindServer.getHostAndPort().hasPort()) {
@@ -60,7 +79,8 @@ public String pass(String request) throws IOException {
//loopback nor private addresses, while routing a clearnet node through it would expose the RPC credentials below to an exit node.
//Configuring a node that is neither local nor onion warns the user on testing the connection or closing the dialog, see ServerSettingsController.
Proxy proxy = AppServices.getProxy();
- HttpURLConnection connection = proxy != null && Protocol.isOnionAddress(bitcoindServer) ? (HttpURLConnection)bitcoindUrl.openConnection(proxy) : (HttpURLConnection)bitcoindUrl.openConnection();
+ boolean useProxy = proxy != null && Protocol.isOnionAddress(bitcoindServer);
+ HttpURLConnection connection = useProxy ? (HttpURLConnection)bitcoindUrl.openConnection(proxy) : (HttpURLConnection)bitcoindUrl.openConnection();
if(connection instanceof HttpsURLConnection httpsURLConnection) {
SSLSocketFactory sslSocketFactory = getSSLSocketFactory();
@@ -80,6 +100,8 @@ public String pass(String request) throws IOException {
}
connection.setDoOutput(true);
+ connection.setConnectTimeout(useProxy ? ONION_CONNECT_TIMEOUT_MILLIS : CONNECT_TIMEOUT_MILLIS);
+ connection.setReadTimeout(isRescanningMethod(request) ? 0 : readTimeoutMillis);
log.debug("> " + request);
@@ -126,6 +148,27 @@ public String pass(String request) throws IOException {
return response;
}
+ //Parses the method as TcpTransport does for Electrum notifications, so that a parameter value cannot match
+ static boolean isRescanningMethod(String request) {
+ try(JsonParser parser = JSON_FACTORY.createParser(request)) {
+ if(parser.nextToken() != JsonToken.START_OBJECT) {
+ return false;
+ }
+ while(parser.nextToken() == JsonToken.FIELD_NAME) {
+ String field = parser.currentName();
+ JsonToken value = parser.nextToken();
+ if("method".equals(field)) {
+ return value == JsonToken.VALUE_STRING && RESCANNING_METHODS.contains(parser.getText());
+ }
+ parser.skipChildren();
+ }
+ return false;
+ } catch(Exception e) {
+ log.warn("Could not parse JSON-RPC request method, applying the default read timeout: " + e.getMessage());
+ return false;
+ }
+ }
+
private String getBitcoindAuthEncoded() throws IOException {
if(cookieFile != null) {
if(!cookieFile.exists()) {
### src/test/java/com/sparrowwallet/sparrow/net/cormorant/bitcoind/BitcoindTransportTimeoutTest.java
@@ -0,0 +1,106 @@
+package com.sparrowwallet.sparrow.net.cormorant.bitcoind;
+
+import com.sparrowwallet.sparrow.SparrowWallet;
+import com.sparrowwallet.sparrow.io.Config;
+import com.sparrowwallet.sparrow.io.Server;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+import java.io.BufferedReader;
+import java.io.IOException;
+import java.io.InputStreamReader;
+import java.net.InetAddress;
+import java.net.ServerSocket;
+import java.net.Socket;
+import java.net.SocketTimeoutException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Path;
+import java.time.Duration;
+
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTimeoutPreemptively;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+/**
+ * Regression tests for the timeouts in BitcoindTransport#pass. Neither was set, and HttpURLConnection defaults both to
+ * 0 (infinite), so a node that accepted a request and then never replied hung the calling thread forever.
+ */
+public class BitcoindTransportTimeoutTest {
+ @TempDir
+ private static Path tempHome;
+
+ private static final int TEST_READ_TIMEOUT_MILLIS = 500;
+
+ @BeforeAll
+ public static void setUp() {
+ //Config.get() caches its instance for the life of the JVM, so keep these tests from loading the developer's real config and leaving it cached for those that follow
+ System.setProperty(SparrowWallet.APP_HOME_PROPERTY, tempHome.toString());
+ }
+
+ @AfterAll
+ public static void tearDown() {
+ System.clearProperty(SparrowWallet.APP_HOME_PROPERTY);
+ }
+
+ @Test
+ public void nonRespondingNodeTimesOutInsteadOfHangingForever() throws Exception {
+ try(ServerSocket serverSocket = new ServerSocket(0, 1, InetAddress.getLoopbackAddress())) {
+ startSilentServer(serverSocket);
+
+ BitcoindTransport transport = new BitcoindTransport(new Server("http://127.0.0.1:" + serverSocket.getLocalPort()), "test", "user:pass");
+ //Shorten the timeout this test must wait out - the production value is not under test
+ transport.readTimeoutMillis = TEST_READ_TIMEOUT_MILLIS;
+
+ //If this hangs instead of throwing, the regression is back
+ assertTimeoutPreemptively(Duration.ofSeconds(5), () -> {
+ IOException thrown = assertThrows(IOException.class, () -> transport.pass(request("getblockchaininfo")));
+ assertTrue(thrown instanceof SocketTimeoutException || thrown.getCause() instanceof SocketTimeoutException, "Expected a read timeout, got: " + thrown);
+ });
+ }
+ }
+
+ @Test
+ public void readTimeoutIsNotStricterThanTheElectrumClientAllows() {
+ //Bounding these calls more tightly than the Electrum client they serve would only fail the request sooner
+ BitcoindTransport transport = new BitcoindTransport(new Server("http://127.0.0.1:1"), "test", "user:pass");
+ assertTrue(transport.readTimeoutMillis >= Config.get().getMaxServerTimeout() * 1000);
+ }
+
+ @Test
+ public void onlyRescanningMethodsAreExemptFromTheReadTimeout() {
+ //Both rescan synchronously - importdescriptors from the descriptor birthday, loadwallet from the block the wallet was last unloaded at
+ assertTrue(BitcoindTransport.isRescanningMethod(request("importdescriptors")));
+ assertTrue(BitcoindTransport.isRescanningMethod(request("loadwallet")));
+ assertFalse(BitcoindTransport.isRescanningMethod(request("getblockchaininfo")));
+ assertFalse(BitcoindTransport.isRescanningMethod(request("listwallets")));
+ //Only the method field is inspected, so a parameter value must not match
+ assertFalse(BitcoindTransport.isRescanningMethod("{\"id\":1,\"method\":\"getwalletinfo\",\"params\":{\"note\":\"importdescriptors\"}}"));
+ assertFalse(BitcoindTransport.isRescanningMethod("not valid json"));
+ assertFalse(BitcoindTransport.isRescanningMethod(""));
+ }
+
+ /**
+ * Accepts the request, then never replies - a node that has hung on the call.
+ */
+ private void startSilentServer(ServerSocket serverSocket) {
+ Thread serverThread = new Thread(() -> {
+ try(Socket accepted = serverSocket.accept()) {
+ BufferedReader in = new BufferedReader(new InputStreamReader(accepted.getInputStream(), StandardCharsets.UTF_8));
+ while(in.readLine() != null) {
+ //Drain the request, then go silent
+ }
+ } catch(Exception e) {
+ //Expected once the read timeout fires and the connection is torn down
+ }
+ });
+ serverThread.setDaemon(true);
+ serverThread.start();
+ }
+
+ private static String request(String method) {
+ return "{\"id\":1,\"method\":\"" + method + "\",\"params\":{}}";
+ }
+}Why this scored 62/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.