bound the bitcoin core rpc connect and read timeouts, exempting the methods that rescan synchronously

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<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 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()) {

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\":{}}";
+    }
+}