net: relay I2P addresses even if not reachable (by us) #22211

pull vasild wants to merge 5 commits into bitcoin:master from vasild:i2p_IsRelayable changing 3 files +41 −16
  1. vasild commented at 12:25 pm on June 10, 2021: member

    Nodes that can reach the I2P network (have set -i2psam=) will relay I2P addresses even without this patch. However, nodes that can’t reach the I2P network will not. This was done as a precaution in #20119 before anybody could connect to I2P because then, for sure, it would have been useless.

    Now, however, we have I2P support and a bunch of I2P nodes, so get all nodes on the network to relay I2P addresses to help with propagation, similarly to what we do with Tor addresses.

  2. net: relay I2P addresses even if not reachable (by us) 
    Nodes that can reach the I2P network (have set `-i2psam=`) will relay
I2P addresses even without this patch. However, nodes that can't reach
the I2P network will not. This was done as a precaution in
https://github.com/bitcoin/bitcoin/pull/20119 before anybody could
connect to I2P because then, for sure, it would have been useless.

Now, however, we have I2P support and a bunch of I2P nodes, so get all
nodes on the network to relay I2P addresses to help with propagation,
similarly to what we do with Tor addresses.
    ba45f02708
  3. fanquake added the label P2P on Jun 10, 2021
  4. vasild commented at 12:26 pm on June 10, 2021: member
    cc @sipa
  5. vasild commented at 12:28 pm on June 10, 2021: member
    Would be good to get this in 22.0.
  6. kristapsk approved
  7. kristapsk commented at 12:42 pm on June 10, 2021: contributor
    utACK ba45f0270815d54ae3290efc16324c2ff1984565
  8. jonatack commented at 12:47 pm on June 10, 2021: member

    utACK ba45f0270815d54ae3290efc16324c2ff1984565 IsRelayable() is only called by RelayAddress() in the codebase. My I2P+onion+clearnet node has a slowly increasing number of I2P connections, both inbound and outbound, currently 9-10, but the number will increase once v22 is released and it seems good to relay them.

    More test coverage may be good.

  9. test: use NODE_* constants instead of magic numbers 
    We just assigned `NODE_NETWORK | NODE_WITNESS` to `nServices` a few
lines above. Use that for verifying correctness instead of `9`.
    86742811ce
  10. vasild commented at 1:57 pm on June 11, 2021: member

    ba45f02708..823b85e158: append tests to ensure the behavior

    More test coverage may be good.

    Here it is! :)

  11. in test/functional/p2p_addrv2_relay.py:33 in 823b85e158 outdated 
    29+    # Add one I2P address at an arbitrary position.
30+    if i == 5:
31+        addr.net = addr.NET_I2P
32+        addr.ip = I2P_ADDR
33+    else:
34+        addr.ip = "123.123.123.{}".format(i % 256)
    jonatack commented at 3:21 pm on June 11, 2021:

    drive-by nit, f-strings for string interpolation

    linter needs appeasing for “test/functional/p2p_addrv2_relay.py:19:1: F401 ’test_framework.util.assert_equal’ imported but unused”

    vasild commented at 5:21 pm on June 11, 2021:
    Done.
  12. in test/functional/test_framework/messages.py:21 in 823b85e158 outdated 
    17@@ -18,6 +18,7 @@
18 Classes use __slots__ to ensure extraneous attributes aren't accidentally added
19 by tests, compromising their intended effect.
20 """
21+from base64 import b32encode, b32decode
    jonatack commented at 3:22 pm on June 11, 2021:
    nit, sort
    vasild commented at 5:20 pm on June 11, 2021:
    Done.
  13. in test/functional/p2p_addrv2_relay.py:75 in 823b85e158 outdated 
    73-                'sending addrv2 (131 bytes) peer=1',
74+                # The I2P address is not added to node's own addrman because it has no
75+                # I2P reachability (thus 10 - 1 = 9).
76+                'Added 9 addresses from 127.0.0.1: 0 tried',
77+                'received: addrv2 (159 bytes) peer=0',
78+                'sending addrv2 (159 bytes) peer=1',
    jonatack commented at 4:43 pm on June 11, 2021:

    (maybe) add an assert independent of the logging to back up the comment in lines 71-72, if you think it is useful

    0         assert addr_receiver.addrv2_received_and_checked
1+        assert_equal(len(self.nodes[0].getnodeaddresses(count=0, network="i2p")), 0)
    vasild commented at 5:20 pm on June 11, 2021:
    Added.
  14. jonatack commented at 4:51 pm on June 11, 2021: member

    ACK 823b85e158161279f053e38935e7033d7d0a23a1 verified the test fails without ba45f0270815d54ae3290efc16324c2ff1984565 with sending addrv2 (118 bytes) peer=1 (118 bytes instead of 159)

     02021-06-11T16:15:19.507000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_znhw4kg4
 12021-06-11T16:15:20.028000Z TestFramework (INFO): Create connection that sends addrv2 messages
 22021-06-11T16:15:20.131000Z TestFramework (INFO): Send too-large addrv2 message
 32021-06-11T16:15:20.188000Z TestFramework (INFO): Check that addrv2 message content is relayed and added to addrman
 42021-06-11T16:15:22.319000Z TestFramework (ERROR): Assertion failed
 5Traceback (most recent call last):
 6  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 128, in main
 7    self.run_test()
 8  File "/home/jon/projects/bitcoin/bitcoin/test/functional/p2p_addrv2_relay.py", line 78, in run_test
 9    addr_receiver.wait_for_addrv2()
10  File "/usr/lib/python3.9/contextlib.py", line 124, in __exit__
11    next(self.gen)
12  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/test_node.py", line 400, in assert_debug_log
13    self._raise_assertion_error('Expected messages "{}" does not partially match log:\n\n{}\n\n'.format(str(expected_msgs), print_log))
14  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/test_node.py", line 166, in _raise_assertion_error
15    raise AssertionError(self._node_msg(msg))
16AssertionError: [node 0] Expected messages "['Added 9 addresses from 127.0.0.1: 0 tried', 'received: addrv2 (159 bytes) peer=0', 'sending addrv2 (159 bytes) peer=1']" does not partially match log:
17
18 - 2021-06-11T16:15:20.296366Z [msghand] [net_processing.cpp:2404] [ProcessMessage] received: addrv2 (159 bytes) peer=0
19 - 2021-06-11T16:15:20.296802Z [msghand] [addrman.cpp:33] [GetNewBucket] IP 123.123.123.0 mapped to AS0 belongs to new bucket 628
20 - 2021-06-11T16:15:20.296892Z [msghand] [addrman.cpp:33] [GetNewBucket] IP 123.123.123.1 mapped to AS0 belongs to new bucket 628
21 - 2021-06-11T16:15:20.296976Z [msghand] [addrman.cpp:33] [GetNewBucket] IP 123.123.123.2 mapped to AS0 belongs to new bucket 628
22 - 2021-06-11T16:15:20.297026Z [msghand] [addrman.cpp:33] [GetNewBucket] IP 123.123.123.3 mapped to AS0 belongs to new bucket 628
23 - 2021-06-11T16:15:20.297075Z [msghand] [addrman.cpp:33] [GetNewBucket] IP 123.123.123.4 mapped to AS0 belongs to new bucket 628
24 - 2021-06-11T16:15:20.297122Z [msghand] [addrman.cpp:33] [GetNewBucket] IP 123.123.123.6 mapped to AS0 belongs to new bucket 628
25 - 2021-06-11T16:15:20.297165Z [msghand] [addrman.cpp:33] [GetNewBucket] IP 123.123.123.7 mapped to AS0 belongs to new bucket 628
26 - 2021-06-11T16:15:20.297217Z [msghand] [addrman.cpp:33] [GetNewBucket] IP 123.123.123.8 mapped to AS0 belongs to new bucket 628
27 - 2021-06-11T16:15:20.297266Z [msghand] [addrman.cpp:33] [GetNewBucket] IP 123.123.123.9 mapped to AS0 belongs to new bucket 628
28 - 2021-06-11T16:15:20.297293Z [msghand] [addrman.h:532] [Add] Added 9 addresses from 127.0.0.1: 0 tried, 9 new
29 - 2021-06-11T16:15:20.297604Z [msghand] [net_processing.cpp:2404] [ProcessMessage] received: ping (8 bytes) peer=0
30 - 2021-06-11T16:15:20.297661Z [msghand] [net.cpp:2959] [PushMessage] sending pong (8 bytes) peer=0
31 - 2021-06-11T16:15:20.347034Z [http] [httpserver.cpp:237] [http_request_cb] Received a POST request for / from 127.0.0.1:35120
32 - 2021-06-11T16:15:20.347555Z [httpworker.0] [rpc/request.cpp:174] [parse] ThreadRPCServer method=setmocktime user=__cookie__
33 - 2021-06-11T16:15:20.352683Z (mocktime: 2021-06-11T16:45:20Z) [opencon] [net.cpp:401] [ConnectNode] trying connection 123.123.123.8:8341 lastseen=2.5hrs
34 - 2021-06-11T16:15:20.398215Z (mocktime: 2021-06-11T16:45:20Z) [msghand] [net.cpp:2959] [PushMessage] sending ping (8 bytes) peer=0
35 - 2021-06-11T16:15:20.398608Z (mocktime: 2021-06-11T16:45:20Z) [msghand] [net.cpp:2959] [PushMessage] sending ping (8 bytes) peer=1
36 - 2021-06-11T16:15:20.398852Z (mocktime: 2021-06-11T16:45:20Z) [msghand] [net.cpp:2959] [PushMessage] sending addrv2 (118 bytes) peer=1
37 - 2021-06-11T16:15:20.399445Z (mocktime: 2021-06-11T16:45:20Z) [msghand] [net_processing.cpp:2404] [ProcessMessage] received: pong (8 bytes) peer=0
38 - 2021-06-11T16:15:20.399591Z (mocktime: 2021-06-11T16:45:20Z) [msghand] [net_processing.cpp:2404] [ProcessMessage] received: pong (8 bytes) peer=1
  15. test: implement ser/unser of I2P addresses in functional tests 33e211d2a4
  16. test: make CAddress in functional tests comparable 
    This way we can compare CAddress objects using `==` or even
arrays of CAddress using `array1 == array2`.
    e7468139a1
  17. test: ensure I2P addresses are relayed 
    This test would fail if `CNetAddr::IsRelayable()` returns `false` for
I2P addresses, given that this test node does not have I2P connectivity.
    7593b06bd1
  18. vasild force-pushed on Jun 11, 2021
  19. vasild commented at 5:20 pm on June 11, 2021: member
    823b85e158...7593b06bd1: address review suggestions and pet the linter
  20. jonatack commented at 9:25 am on June 12, 2021: member
    ACK 7593b06bd1262f438bf34769ecc00e9c22328e56
  21. kristapsk approved
  22. kristapsk commented at 8:41 pm on June 16, 2021: contributor
    ACK 7593b06bd1262f438bf34769ecc00e9c22328e56. Code looks correct, tested that functional test suite passes and also that test/functional/p2p_addrv2_replay.py fails if I undo changes in IsRelayable().
  23. naumenkogs commented at 9:44 am on June 18, 2021: member
    ACK 7593b06bd1262f438bf34769ecc00e9c22328e56. Thank you for taking care of i2p.
  24. luke-jr referenced this in commit 0ffe41289b on Jun 27, 2021
  25. luke-jr referenced this in commit ba3ea5680f on Jun 27, 2021
  26. luke-jr referenced this in commit 0d292557cd on Jun 27, 2021
  27. luke-jr referenced this in commit 8459ce56da on Jun 27, 2021
  28. luke-jr referenced this in commit b639380fe0 on Jun 29, 2021
  29. laanwj added this to the milestone 22.0 on Jul 8, 2021
  30. laanwj commented at 2:51 pm on July 15, 2021: member
    Code review ACK 7593b06bd1262f438bf34769ecc00e9c22328e56
  31. laanwj merged this on Jul 15, 2021
  32. laanwj closed this on Jul 15, 2021
  33. vasild deleted the branch on Jul 16, 2021
  34. sidhujag referenced this in commit ae9a90b03d on Jul 23, 2021
  35. Fabcien referenced this in commit a1509f778a on Feb 1, 2022
  36. gwillen referenced this in commit 6361a13268 on Jun 1, 2022
  37. DrahtBot locked this on Aug 18, 2022


vasild kristapsk jonatack naumenkogs laanwj

Labels
P2P

Milestone
22.0

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: 2024-10-04 22:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me