i2p: log connection was refused due to arbitrary port #29393

pull brunoerg wants to merge 1 commits into bitcoin:master from brunoerg:2024-02-i2p-log-connect changing 2 files +6 −13
  1. brunoerg commented at 5:59 PM on February 6, 2024: contributor

    For I2P, we do not try to connect if port is != 0. However, we do not have anything that indicates it or any error when trying to connect with port != 0. This PR adds a log for it. Also, it improves the functional test. With this log we can ensure the reason we won't connect is the port, in the current test, we cannot ensure it.

  2. DrahtBot commented at 6:00 PM on February 6, 2024: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage

    For detailed information about the code coverage, see the test coverage report.

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK jonatack, kristapsk, epiccurious, vasild, achow101

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

  3. in test/functional/p2p_i2p_ports.py:26 in 950f3602d6 outdated 
      33 | -            if not re.search(r"Expected messages .* does not partially match log", str(e)):
  34 | -                raise AssertionError(f"Assertion raised as expected, but with an unexpected message: {str(e)}")
  35 | -        if not raised:
  36 | -            raise AssertionError("Assertion should have been raised")
  37 | +        with node.assert_debug_log(expected_msgs=[f"Connection refused due to arbitrary port: 8333"]):
  38 | +            node.addnode(node=addr, command="onetry")
    epiccurious commented at 8:22 PM on February 6, 2024:

    Question on your approach here - why drop the try .. catch ... if not raised instead of just replacing line 27?

    brunoerg commented at 8:30 PM on February 6, 2024:

    Because they are not necessary anymore. That try catch was a hack to check the logs because we didn't have a direct way to check in the logs that the reason was the port.

  4. epiccurious commented at 8:23 PM on February 6, 2024: contributor

    Concept ACK 950f3602d61f51d3c42c051ed9139916f1ee23bd.

  5. kristapsk approved
  6. kristapsk commented at 10:21 PM on February 6, 2024: contributor

    cr utACK 950f3602d61f51d3c42c051ed9139916f1ee23bd

  7. DrahtBot requested review from epiccurious on Feb 6, 2024
  8. epiccurious approved
  9. DrahtBot requested review from epiccurious on Feb 7, 2024
  10. epiccurious commented at 2:02 AM on February 7, 2024: contributor

    Tested ACK 950f3602d61f51d3c42c051ed9139916f1ee23bd.

  11. DrahtBot removed review request from epiccurious on Feb 7, 2024
  12. in src/i2p.cpp:220 in 950f3602d6 outdated 
     216 | @@ -217,6 +217,7 @@ bool Session::Connect(const CService& to, Connection& conn, bool& proxy_error)
 217 |      // Refuse connecting to arbitrary ports. We don't specify any destination port to the SAM proxy
 218 |      // when connecting (SAM 3.1 does not use ports) and it forces/defaults it to I2P_SAM31_PORT.
 219 |      if (to.GetPort() != I2P_SAM31_PORT) {
 220 | +        Log("Connection refused due to arbitrary port: %s", to.GetPort());
    jonatack commented at 8:56 PM on February 8, 2024:

    Suggest making this log output more helpful like the one it replaces by providing the addr, and also making it more similar to the other I2P error logs.

            Log("Error connecting to %s, connection refused due to arbitrary port %s", to.ToStringAddrPort(), to.GetPort());

    <details><summary>full diff with test</summary><p>

    --- a/src/i2p.cpp
+++ b/src/i2p.cpp
@@ -217,7 +217,7 @@ bool Session::Connect(const CService& to, Connection& conn, bool& proxy_error)
     // Refuse connecting to arbitrary ports. We don't specify any destination port to the SAM proxy
     // when connecting (SAM 3.1 does not use ports) and it forces/defaults it to I2P_SAM31_PORT.
     if (to.GetPort() != I2P_SAM31_PORT) {
-        Log("Connection refused due to arbitrary port: %s", to.GetPort());
+        Log("Error connecting to %s, connection refused due to arbitrary port %s", to.ToStringAddrPort(), to.GetPort());
         proxy_error = false;

diff --git a/test/functional/p2p_i2p_ports.py b/test/functional/p2p_i2p_ports.py
index 44ec002b2a6..20dcb50a574 100755
--- a/test/functional/p2p_i2p_ports.py
+++ b/test/functional/p2p_i2p_ports.py
@@ -22,7 +22,7 @@ class I2PPorts(BitcoinTestFramework):
 
         self.log.info("Ensure we don't try to connect if port!=0")
         addr = "zsxwyo6qcn3chqzwxnseusqgsnuw3maqnztkiypyfxtya4snkoka.b32.i2p:8333"
-        with node.assert_debug_log(expected_msgs=[f"Connection refused due to arbitrary port: 8333"]):
+        with node.assert_debug_log(expected_msgs=[f"Error connecting to {addr}, connection refused due to arbitrary port 8333"]):
             node.addnode(node=addr, command="onetry")

    </p></details>

    brunoerg commented at 9:27 PM on February 8, 2024:

    Good suggestion.

  13. jonatack commented at 8:59 PM on February 8, 2024: member

    Approach ACK

  14. i2p: log connection was refused due to arbitrary port 5b358cdd1a
  15. brunoerg force-pushed on Feb 8, 2024
  16. brunoerg commented at 9:37 PM on February 8, 2024: contributor

    Force-pushed addressing #29393 (review)

  17. jonatack commented at 9:45 PM on February 8, 2024: member

    ACK 5b358cdd1a5f5d2fe87a9e41c638996eab2e2796

  18. DrahtBot requested review from kristapsk on Feb 8, 2024
  19. DrahtBot requested review from epiccurious on Feb 8, 2024
  20. kristapsk approved
  21. kristapsk commented at 11:01 PM on February 8, 2024: contributor

    re-ACK 5b358cdd1a5f5d2fe87a9e41c638996eab2e2796

  22. DrahtBot removed review request from epiccurious on Feb 8, 2024
  23. DrahtBot requested review from epiccurious on Feb 8, 2024
  24. epiccurious approved
  25. DrahtBot requested review from epiccurious on Feb 9, 2024
  26. epiccurious commented at 2:58 PM on February 9, 2024: contributor

    re-ACK 5b358cdd1a5f5d2fe87a9e41c638996eab2e2796.

  27. DrahtBot removed review request from epiccurious on Feb 9, 2024
  28. in src/i2p.cpp:220 in 5b358cdd1a 
     216 | @@ -217,6 +217,7 @@ bool Session::Connect(const CService& to, Connection& conn, bool& proxy_error)
 217 |      // Refuse connecting to arbitrary ports. We don't specify any destination port to the SAM proxy
 218 |      // when connecting (SAM 3.1 does not use ports) and it forces/defaults it to I2P_SAM31_PORT.
 219 |      if (to.GetPort() != I2P_SAM31_PORT) {
 220 | +        Log("Error connecting to %s, connection refused due to arbitrary port %s", to.ToStringAddrPort(), to.GetPort());
    vasild commented at 11:44 AM on March 1, 2024:

    nit, feel free to ignore: usually "connection refused" means that the connection attempt has been actively declined by the destination host (that is, they send us TCP RST packet). Maybe it would be more clear to say "Refusing to connect to %s because its port is not %d", to.ToStringAddrPort(), I2P_SAM31_PORT. E.g. "Refusing to connect to foo.i2p:1234 because its port is not 0".

    brunoerg commented at 12:55 PM on March 1, 2024:

    Sounds good, but I'll leave it as is due to previous reviews.

  29. vasild approved
  30. vasild commented at 11:53 AM on March 1, 2024: contributor

    ACK 5b358cdd1a5f5d2fe87a9e41c638996eab2e2796

    How did you come to this? Did you encounter some !=0 I2P ports in the wild? I don't have any in my addrman:

    $ bitcoin-cli getnodeaddresses 0 |jq 'map(select(.address |test("\\.i2p$"; "i"))) |map(select(.port != 0)) |length'
0

$ bitcoin-cli getnodeaddresses 0 |jq 'map(select(.address |test("\\.i2p$"; "i"))) |map(select(.port == 0)) |length'
1605
  31. brunoerg commented at 12:58 PM on March 1, 2024: contributor

    How did you come to this? Did you encounter some !=0 I2P ports in the wild?

    I didn't encounter in practice, in fact, I was testing some nuances of i2p and tried to add it using addnode to check the behaviour, then I noticed that. Reading the functional test I realized that the way we were checking it was weird and did not really ensure the problem is the port.

  32. achow101 commented at 2:02 AM on March 9, 2024: member

    ACK 5b358cdd1a5f5d2fe87a9e41c638996eab2e2796

  33. achow101 merged this on Mar 9, 2024
  34. achow101 closed this on Mar 9, 2024
  35. in test/functional/p2p_i2p_ports.py:30 in 5b358cdd1a 
      38 | +            node.addnode(node=addr, command="onetry")
  39 |  
  40 |          self.log.info("Ensure we try to connect if port=0 and get an error due to missing I2P proxy")
  41 |          addr = "h3r6bkn46qxftwja53pxiykntegfyfjqtnzbm6iv6r5mungmqgmq.b32.i2p:0"
  42 | -        with node.assert_debug_log(expected_msgs=[f"Error connecting to {addr}"]):
  43 | +        with node.assert_debug_log(expected_msgs=[f"Error connecting to {addr}: Cannot connect to {PROXY}"]):
    maflcko commented at 2:27 PM on July 1, 2024:

    This is reverted in https://github.com/bitcoin/bitcoin/pull/30345/files (as of now)

    brunoerg commented at 2:45 PM on July 1, 2024:

    left a comment there

  36. PastaPastaPasta referenced this in commit 72ac093e9c on Oct 24, 2024
  37. PastaPastaPasta referenced this in commit 2eec08e9e8 on Oct 24, 2024
  38. PastaPastaPasta referenced this in commit bd607f049d on Oct 24, 2024
  39. PastaPastaPasta referenced this in commit aaccc9ea51 on Oct 24, 2024
  40. bitcoin locked this on Jul 1, 2025
Contributors
brunoergDrahtBotepiccuriouskristapskjonatackvasildachow101maflcko
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-02 15:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me