Merge bitcoin/bitcoin#28077: I2P: also sleep after errors in Accept() & destroy the session if we get an unexpected error

5c8e15c451ec870b9dd4eb843ec6ca3ba64cda4f i2p: destroy the session if we get an unexpected error from the I2P router (Vasil Dimov)
762404a68c114e8831cdfae937627174544b55a7 i2p: also sleep after errors in Accept() (Vasil Dimov)

Pull request description:

  ### Background

  In the `i2p::sam::Session` class:

  `Listen()` does:
  * if the session is not created yet
    * create the control socket and on it:
    * `HELLO`
    * `SESSION CREATE ID=sessid`
    * leave the control socked opened
  * create a new socket and on it:
  * `HELLO`
  * `STREAM ACCEPT ID=sessid`
  * read reply (`STREAM STATUS`), `Listen()` only succeeds if it contains `RESULT=OK`

  Then a wait starts, for a peer to connect. When connected,

  `Accept()` does:
  * on the socket from `STREAM ACCEPT` from `Listen()`: read the Base64 identification of the connecting peer

  ### Problem

  The I2P router may be in such a state that this happens in a quick succession (many times per second, see https://github.com/bitcoin/bitcoin/issues/22759#issuecomment-1609907115): `Listen()`-succeeds, `Accept()`-fails.

  `Accept()` fails because the I2P router sends something that is not Base64 on the socket: `STREAM STATUS RESULT=I2P_ERROR MESSAGE="Session was closed"`

  We only sleep after failed `Listen()` because the assumption was that if `Accept()` fails then the next `Listen()` will also fail.

  ### Solution

  Avoid filling the log with "Error accepting:" messages and sleep also after a failed `Accept()`.

  ### Extra changes

  * Reset the error waiting time after one successful connection. Otherwise the timer will remain high due to problems that have been solved long time in the past.

  * Increment the wait time less aggressively.

  * Handle the unexpected "Session was closed" message more gracefully (don't log stupid messages like `Cannot decode Base64: "STREAM STATUS...`) and destroy the session right way.

ACKs for top commit:
  achow101:
    ACK 5c8e15c451ec870b9dd4eb843ec6ca3ba64cda4f
  jonatack:
    re-ACK 5c8e15c451ec870b9dd4eb843ec6ca3ba64cda4f

Tree-SHA512: 1d47958c50eeae9eefcb668b8539fd092adead93328e4bf3355267819304b99ab41cbe1b5dbedbc3452c2bc389dc8330c0e27eb5ccb880e33dc46930a1592885

1 files changed, 49 insertions, 16 deletions

diff --git a/src/i2p.cpp b/src/i2p.cpp
index 5a3dde54ce..05a5dde396 100644
--- a/src/i2p.cpp
+++ b/src/i2p.cpp
@@ -12,6 +12,7 @@
 #include <netaddress.h>
 #include <netbase.h>
 #include <random.h>
+#include <sync.h>
 #include <tinyformat.h>
 #include <util/fs.h>
 #include <util/readwritefile.h>
@@ -153,27 +154,59 @@ bool Session::Listen(Connection& conn)
 
 bool Session::Accept(Connection& conn)
 {
-    try {
-        while (!*m_interrupt) {
-            Sock::Event occurred;
-            if (!conn.sock->Wait(MAX_WAIT_FOR_IO, Sock::RECV, &occurred)) {
-                throw std::runtime_error("wait on socket failed");
-            }
+    AssertLockNotHeld(m_mutex);
 
-            if (occurred == 0) {
-                // Timeout, no incoming connections or errors within MAX_WAIT_FOR_IO.
-                continue;
-            }
+    std::string errmsg;
+    bool disconnect{false};
 
-            const std::string& peer_dest =
-                conn.sock->RecvUntilTerminator('\n', MAX_WAIT_FOR_IO, *m_interrupt, MAX_MSG_SIZE);
+    while (!*m_interrupt) {
+        Sock::Event occurred;
+        if (!conn.sock->Wait(MAX_WAIT_FOR_IO, Sock::RECV, &occurred)) {
+            errmsg = "wait on socket failed";
+            break;
+        }
 
-            conn.peer = CService(DestB64ToAddr(peer_dest), I2P_SAM31_PORT);
+        if (occurred == 0) {
+            // Timeout, no incoming connections or errors within MAX_WAIT_FOR_IO.
+            continue;
+        }
 
-            return true;
+        std::string peer_dest;
+        try {
+            peer_dest = conn.sock->RecvUntilTerminator('\n', MAX_WAIT_FOR_IO, *m_interrupt, MAX_MSG_SIZE);
+        } catch (const std::runtime_error& e) {
+            errmsg = e.what();
+            break;
         }
-    } catch (const std::runtime_error& e) {
-        Log("Error accepting: %s", e.what());
+
+        CNetAddr peer_addr;
+        try {
+            peer_addr = DestB64ToAddr(peer_dest);
+        } catch (const std::runtime_error& e) {
+            // The I2P router is expected to send the Base64 of the connecting peer,
+            // but it may happen that something like this is sent instead:
+            // STREAM STATUS RESULT=I2P_ERROR MESSAGE="Session was closed"
+            // In that case consider the session damaged and close it right away,
+            // even if the control socket is alive.
+            if (peer_dest.find("RESULT=I2P_ERROR") != std::string::npos) {
+                errmsg = strprintf("unexpected reply that hints the session is unusable: %s", peer_dest);
+                disconnect = true;
+            } else {
+                errmsg = e.what();
+            }
+            break;
+        }
+
+        conn.peer = CService(peer_addr, I2P_SAM31_PORT);
+
+        return true;
+    }
+
+    Log("Error accepting%s: %s", disconnect ? " (will close the session)" : "", errmsg);
+    if (disconnect) {
+        LOCK(m_mutex);
+        Disconnect();
+    } else {
         CheckControlSock();
     }
     return false;