From e49b6a4a6325bb342ab63bea1ab5fbca81c7ac01 Mon Sep 17 00:00:00 2001 From: Craig Raw Date: Fri, 21 Aug 2026 12:25:41 +0200 Subject: [PATCH] bound the bitcoin core rpc connect and read timeouts, exempting the methods that rescan synchronously --- .../cormorant/bitcoind/BitcoindTransport.java | 45 +++++++- .../BitcoindTransportTimeoutTest.java | 106 ++++++++++++++++++ 2 files changed, 150 insertions(+), 1 deletion(-) create mode 100644 src/test/java/com/sparrowwallet/sparrow/net/cormorant/bitcoind/BitcoindTransportTimeoutTest.java diff --git a/src/main/java/com/sparrowwallet/sparrow/net/cormorant/bitcoind/BitcoindTransport.java b/src/main/java/com/sparrowwallet/sparrow/net/cormorant/bitcoind/BitcoindTransport.java index 45054c98..84984cae 100644 --- a/src/main/java/com/sparrowwallet/sparrow/net/cormorant/bitcoind/BitcoindTransport.java +++ b/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.Files; 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 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 class BitcoindTransport implements Transport { 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 class BitcoindTransport implements Transport { //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 class BitcoindTransport implements Transport { } 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 class BitcoindTransport implements Transport { 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()) { diff --git a/src/test/java/com/sparrowwallet/sparrow/net/cormorant/bitcoind/BitcoindTransportTimeoutTest.java b/src/test/java/com/sparrowwallet/sparrow/net/cormorant/bitcoind/BitcoindTransportTimeoutTest.java new file mode 100644 index 00000000..5f7e9e71 --- /dev/null +++ b/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\":{}}"; + } +}