test: cover addrv2 anchors by adding TorV3 to CAddress in messages.py #27452

pull pinheadmz wants to merge 4 commits into bitcoin:master from pinheadmz:test-anchors-addrv2 changing 5 files +143 −17
  1. pinheadmz commented at 2:44 PM on April 12, 2023: member

    Closes #27140

    Adds test coverage for #20516 to ensure that #20511 is completed and may be closed.

    This PR adds a test case to feature_anchors.py where an onion v3 address is set as a blocks-only relay peer and then shutdown, ensuring that the address is saved to anchors.dat in addrv2 format. We then ensure that bitcoin attempts to reconnect to that anchor address on restart.

    To compute the addrv2 serialization of the onion v3 address, I added logic to CAddress in messages.py. This new logic is covered by extending p2p_addrv2_relay.py to include an onion v3 address. Future work will be adding coverage for ipv6, torv2 and cjdns in these modules and also feature_proxy.py

    Also includes de/serialization unit test for CAddress in test framework.

  2. DrahtBot commented at 2:44 PM on April 12, 2023: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK jonatack, brunoerg, willcl-ark
    Concept ACK edgardot14y
    Approach ACK vasild

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #27509 (Relay own transactions only via short-lived Tor or I2P connections by vasild)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  3. DrahtBot added the label Tests on Apr 12, 2023
  4. brunoerg commented at 2:45 PM on April 12, 2023: contributor

    Did you take a look at #27295?

  5. pinheadmz commented at 2:49 PM on April 12, 2023: member

    Did you take a look at #27295?

    I will now!

  6. pinheadmz force-pushed on Apr 12, 2023
  7. in test/functional/feature_anchors.py:101 in f06f28ab88 outdated 
      96 | +        ONION_ADDR = "pg6mmjiyjmcrsslvykfwnntlaru7p5svn6y2ymmju6nubxndf4pscryd.onion:8333"
  97 | +        self.nodes[0].addconnection(ONION_ADDR, 'block-relay-only')
  98 | +
  99 | +        self.log.info("Check peer info")
 100 | +        peer_info = self.nodes[0].getpeerinfo()
 101 | +        assert len(peer_info) == 1
    brunoerg commented at 5:07 PM on April 12, 2023:

    I think you could use assert_equal here and in the other similar cases

            assert_equal(len(peer_info), 1)
    pinheadmz commented at 6:34 PM on April 12, 2023:

    sure, done thanks!

  8. brunoerg commented at 6:10 PM on April 12, 2023: contributor

    Concept ACK

  9. meuamigopedro commented at 6:15 PM on April 12, 2023: none

    Concept ACK It is important to include test cases to ensure that the code is working correctly and that adding new features like logic to compute the addrv2 serialization of the onion v3 address can help improve the functionality of Bitcoin. Also, it's always good to keep working on future coverage for other modules and features.

  10. pinheadmz force-pushed on Apr 12, 2023
  11. in test/functional/feature_anchors.py:132 in 076a36ed46 outdated 
     127 | +        with open(node_anchors_path, "wb") as file_handler:
 128 | +            # Cheat: modify service flags for this address
 129 | +            # even though we never connected to it
 130 | +            new_data = bytearray(data)[:-32]
 131 | +            new_data[services_index] = P2P_SERVICES
 132 | +            new_data_hash = hashlib.sha256(hashlib.sha256(new_data).digest()).digest()
    willcl-ark commented at 8:17 PM on May 30, 2023:

    Import helper from test_framework.messages?

                new_data_hash = hash256(new_data)
  12. in test/functional/feature_anchors.py:12 in 076a36ed46 outdated 
       8 |  import os
   9 |  
  10 | -from test_framework.p2p import P2PInterface
  11 | +from test_framework.p2p import P2PInterface, P2P_SERVICES
  12 | +from test_framework.socks5 import Socks5Configuration, Socks5Server
  13 | +from test_framework.messages import CAddress
    willcl-ark commented at 8:17 PM on May 30, 2023: 
    from test_framework.messages import CAddress, hash256
  13. in test/functional/feature_anchors.py:96 in 076a36ed46 outdated 
      85 | +        # Use proxies to catch outbound connections to networks with 256-bit addresses
  86 | +        onion_conf = Socks5Configuration()
  87 | +        onion_conf.auth = True
  88 | +        onion_conf.unauth = True
  89 | +        onion_conf.addr = ('127.0.0.1', 10000)
  90 | +        onion_conf.keep_alive = True
    willcl-ark commented at 8:33 PM on May 30, 2023:

    Why do we need to keep the Socks5Server alive to ensure success when performing "Check for addrv2 addresses in anchors.dat"?

    I changed this to False and saw intermittent failures on Line 123

    assert_equal(data[services_index], 0x00) # services == NONE
    pinheadmz commented at 2:13 PM on May 31, 2023:

    Good question. The Socks5 server we use in the test framework by default disconnects immediately after the handshake and sometimes would not register as a connected peer:

    https://cirrus-ci.com/task/6075793026056192?logs=ci#L3820-L3830

  14. pinheadmz force-pushed on May 31, 2023
  15. pinheadmz commented at 2:14 PM on May 31, 2023: member

    Thanks for the review Will, nits addressed and pushed

  16. DrahtBot added the label CI failed on May 31, 2023
  17. vasild commented at 4:19 PM on June 1, 2023: contributor

    Concept ACK, I guess we can forget about Torv2 and completely drop support about it from everywhere as if it never existed. The Tor network does not support those anymore.

  18. in test/functional/feature_anchors.py:88 in 210d57c048 outdated 
      79 | @@ -78,6 +80,60 @@ def run_test(self):
  80 |          self.log.info("When node starts, check if anchors.dat doesn't exist anymore")
  81 |          assert not os.path.exists(node_anchors_path)
  82 |  
  83 | +        self.log.info("Ensure addrv2 support")
  84 | +        # Use proxies to catch outbound connections to networks with 256-bit addresses
  85 | +        onion_conf = Socks5Configuration()
  86 | +        onion_conf.auth = True
  87 | +        onion_conf.unauth = True
  88 | +        onion_conf.addr = ('127.0.0.1', 10000)
    vasild commented at 4:22 PM on June 1, 2023:

    This could collide with another port randomly assigned by the testing framework (even if it looks like that it currently cannot, it may do in the future) and also could collide with another instance of this test run in parallel on the same machine. Better use p2p_port().

    pinheadmz commented at 3:15 PM on June 2, 2023:

    thanks, fixed.

  19. in test/functional/feature_anchors.py:136 in 210d57c048 outdated 
     127 | +            # Cheat: modify service flags for this address
 128 | +            # even though we never connected to it
 129 | +            new_data = bytearray(data)[:-32]
 130 | +            new_data[services_index] = P2P_SERVICES
 131 | +            new_data_hash = hash256(new_data)
 132 | +            file_handler.write(new_data + new_data_hash)
    vasild commented at 4:31 PM on June 1, 2023:

    Why is it necessary to modify the services?

    pinheadmz commented at 3:11 PM on June 2, 2023:

    We won't attempt an anchor connection to a host that doesn't have services:

    https://github.com/bitcoin/bitcoin/blob/b22408df162a224d94ac54e8443b57ef3fd2ca72/src/net.cpp#L1813

    In the test we don't actually bother setting up a listening node at the onion address we connect to with block-relay-only so the services remain null, even though we do still write the host information to anchors.dat

    I use this hack to ensure that we attempt a connection, and the last assertion in the test will fail without it

    jonatack commented at 9:23 PM on July 7, 2023:

    I think it would be helpful to explain this in the test.

  20. pinheadmz force-pushed on Jun 2, 2023
  21. pinheadmz force-pushed on Jun 2, 2023
  22. pinheadmz force-pushed on Jun 3, 2023
  23. DrahtBot removed the label CI failed on Jun 3, 2023
  24. jonatack commented at 10:29 PM on June 22, 2023: member

    Concept ACK, I guess we can forget about Torv2 and completely drop support about it from everywhere as if it never existed. The Tor network does not support those anymore.

    Concept ACK. Did you plan to drop the Tor v2 additions here? (No need to add code that we'll just remove.)

    As mentioned in #27295#pullrequestreview-1494066588, I'm not sure how this was coordinated with that pull, and these changes could possibly be split into preparatory changes (insofar as they don't break any tests) and implementation commits.

  25. pinheadmz force-pushed on Jun 23, 2023
  26. pinheadmz commented at 4:49 PM on June 23, 2023: member

    @brunoerg @jonatack thanks for your reviews, pushing to 455f8c4f0873df25eadd0cd59e67d5eb2a568621:

    Includes and expands on #27295 including a unit test to de- and re-serialize CAddress from every network type. TorV2 is also completely dropped from the test framework.

  27. pinheadmz force-pushed on Jun 23, 2023
  28. in test/functional/test_framework/messages.py:304 in b5e0398d8c outdated 
     298 | @@ -285,33 +299,56 @@ def deserialize_v2(self, f):
 299 |          self.nServices = deser_compact_size(f)
 300 |  
 301 |          self.net = struct.unpack("B", f.read(1))[0]
 302 | -        assert self.net in (self.NET_IPV4, self.NET_I2P)
 303 | +        assert self.net in (self.NET_IPV4, self.NET_IPV6,
 304 | +                            self.NET_I2P, self.NET_CJDNS,
 305 | +                            self.NET_TORV3)
    vasild commented at 11:47 AM on June 28, 2023:

    Can this use self.ADDRV2_NET_NAME?

            assert self.net in self.ADDRV2_NET_NAME
    pinheadmz commented at 5:04 PM on July 5, 2023:

    yes thanks

  29. in test/functional/test_framework/socks5.py:126 in b5e0398d8c outdated 
     121 | @@ -121,7 +122,8 @@ def handle(self):
 122 |              logger.exception("socks5 request handling failed.")
 123 |              self.serv.queue.put(e)
 124 |          finally:
 125 | -            self.conn.close()
 126 | +            if not self.serv.keep_alive:
 127 | +                self.conn.close()
    vasild commented at 12:01 PM on June 28, 2023:

    When would this socket be closed if keep_alive is true?

    pinheadmz commented at 5:29 PM on July 5, 2023:

    Good question, I'm just letting it drop out of scope at the end of the test, but looking into it more it may actually be staying alive until the entire test runner exits.

    Should I add a manual close function?

    pinheadmz commented at 6:03 PM on July 5, 2023:

    update: no longer needed

    pinheadmz commented at 3:25 PM on July 7, 2023:

    update update: manually calling onion_proxy.close() in the test now with a comment

  30. in test/functional/p2p_addrv2_relay.py:45 in b5e0398d8c outdated 
      42 | +        msg_size += addr.ADDRV2_ADDRESS_LENGTH[addr.NET_TORV3]
  43 |      else:
  44 |          addr.ip = f"123.123.123.{i % 256}"
  45 | -    addr.port = 8333 + i
  46 | +        msg_size += addr.ADDRV2_ADDRESS_LENGTH[addr.NET_IPV4]
  47 | +    msg_size += 4 + 1 + 1 + 2 + 1  # time, services, networkID, port, address length byte
    vasild commented at 1:07 PM on June 28, 2023:

    The port is serialized last, but in the comment is comes after networkID. Would it be better to calculate the message size separately, given an array of addresses as input?

    <details> <summary>[patch] calculate addrv2 message size separately</summary>

    diff --git i/test/functional/p2p_addrv2_relay.py w/test/functional/p2p_addrv2_relay.py
index 23d7c94fd0..f9a8c44be2 100755
--- i/test/functional/p2p_addrv2_relay.py
+++ w/test/functional/p2p_addrv2_relay.py
@@ -20,32 +20,27 @@ from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import assert_equal
 
 I2P_ADDR = "c4gfnttsuwqomiygupdqqqyy5y5emnk5c73hrfvatri67prd7vyq.b32.i2p"
 ONION_ADDR = "pg6mmjiyjmcrsslvykfwnntlaru7p5svn6y2ymmju6nubxndf4pscryd.onion"
 
 ADDRS = []
-msg_size = 1  # vector length byte
 for i in range(10):
     addr = CAddress()
     addr.time = int(time.time()) + i
     addr.port = 8333 + i
     addr.nServices = P2P_SERVICES
     # Add one I2P and one onion V3 address at an arbitrary position.
     if i == 5:
         addr.net = addr.NET_I2P
         addr.ip = I2P_ADDR
         addr.port = 0
-        msg_size += addr.ADDRV2_ADDRESS_LENGTH[addr.NET_I2P]
     elif i == 8:
         addr.net = addr.NET_TORV3
         addr.ip = ONION_ADDR
-        msg_size += addr.ADDRV2_ADDRESS_LENGTH[addr.NET_TORV3]
     else:
         addr.ip = f"123.123.123.{i % 256}"
-        msg_size += addr.ADDRV2_ADDRESS_LENGTH[addr.NET_IPV4]
-    msg_size += 4 + 1 + 1 + 2 + 1  # time, services, networkID, port, address length byte
     ADDRS.append(addr)
 
 
 class AddrReceiver(P2PInterface):
     addrv2_received_and_checked = False
 
@@ -59,12 +54,23 @@ class AddrReceiver(P2PInterface):
             self.addrv2_received_and_checked = True
 
     def wait_for_addrv2(self):
         self.wait_until(lambda: "addrv2" in self.last_message)
 
 
+def calc_addrv2_msg_size(addrs):
+    size = 1  # vector length byte
+    for addr in addrs:
+        size += 4  # time
+        size += 1  # services, COMPACTSIZE(P2P_SERVICES)
+        size += 1  # network id
+        size += 1  # address length byte
+        size += addr.ADDRV2_ADDRESS_LENGTH[addr.net]  # address
+        size += 2  # port
+    return size
+
 class AddrTest(BitcoinTestFramework):
     def set_test_params(self):
         self.setup_clean_chain = True
         self.num_nodes = 1
         self.extra_args = [["-whitelist=addr@127.0.0.1"]]
 
@@ -78,12 +84,13 @@ class AddrTest(BitcoinTestFramework):
         with self.nodes[0].assert_debug_log(['addrv2 message size = 1010']):
             addr_source.send_and_ping(msg)
 
         self.log.info('Check that addrv2 message content is relayed and added to addrman')
         addr_receiver = self.nodes[0].add_p2p_connection(AddrReceiver())
         msg.addrs = ADDRS
+        msg_size = calc_addrv2_msg_size(ADDRS)
         with self.nodes[0].assert_debug_log([
                 f'received: addrv2 ({msg_size} bytes) peer=0',
                 f'sending addrv2 ({msg_size} bytes) peer=1',
         ]):
             addr_source.send_and_ping(msg)
             self.nodes[0].setmocktime(int(time.time()) + 30 * 60)

    </details>

    pinheadmz commented at 5:32 PM on July 5, 2023:

    fantastic, thanks for the patch

  31. in test/functional/feature_anchors.py:110 in b5e0398d8c outdated 
     105 | +
 106 | +        self.log.info("Check peer info")
 107 | +        peer_info = self.nodes[0].getpeerinfo()
 108 | +        assert_equal(len(peer_info), 1)
 109 | +        assert_equal(peer_info[0]["network"], "onion")
 110 | +        assert_equal(peer_info[0]["connection_type"], "block-relay-only")
    vasild commented at 2:10 PM on June 28, 2023:

    Does this suffice to ensure 1 block-only was opened:

    <details> <summary>[patch] check log at shutdown</summary>

    diff --git i/test/functional/feature_anchors.py w/test/functional/feature_anchors.py
index e7cd642484..327533a340 100755
--- i/test/functional/feature_anchors.py
+++ w/test/functional/feature_anchors.py
@@ -91,29 +91,23 @@ class AnchorsTest(BitcoinTestFramework):
         self.log.info("Ensure addrv2 support")
         # Use proxies to catch outbound connections to networks with 256-bit addresses
         onion_conf = Socks5Configuration()
         onion_conf.auth = True
         onion_conf.unauth = True
         onion_conf.addr = ('127.0.0.1', p2p_port(self.num_nodes))
-        onion_conf.keep_alive = True
         onion_proxy = Socks5Server(onion_conf)
         onion_proxy.start()
         self.restart_node(0, extra_args=[f"-onion={onion_conf.addr[0]}:{onion_conf.addr[1]}"])
 
         self.log.info(f"Add 256-bit-address block-relay-only connections to node")
         ONION_ADDR = "pg6mmjiyjmcrsslvykfwnntlaru7p5svn6y2ymmju6nubxndf4pscryd.onion:8333"
         self.nodes[0].addconnection(ONION_ADDR, 'block-relay-only')
 
-        self.log.info("Check peer info")
-        peer_info = self.nodes[0].getpeerinfo()
-        assert_equal(len(peer_info), 1)
-        assert_equal(peer_info[0]["network"], "onion")
-        assert_equal(peer_info[0]["connection_type"], "block-relay-only")
-
         self.log.info("Stop node 0")
-        self.stop_node(0)
+        with self.nodes[0].assert_debug_log([f"DumpAnchors: Flush 1 outbound block-relay-only peer addresses to anchors.dat"]):
+            self.stop_node(0)
 
         self.log.info("Check for addrv2 addresses in anchors.dat")
         caddr = CAddress()
         caddr.net = CAddress.NET_TORV3
         [caddr.ip, port_str] = ONION_ADDR.split(":")
         caddr.port = int(port_str)

    </details>

    If "yes", then no need for keep_alive.

    pinheadmz commented at 5:50 PM on July 5, 2023:

    oh sweet! awesome, i'll remove the socks5.py commit and apply this instead.

    pinheadmz commented at 6:26 PM on July 5, 2023:

    https://cirrus-ci.com/task/5531648075235328?logs=ci#L4421

    Actually there is still a race condition there. I think the keep_alive is necessary for bitcoind to even acknowledge the blocks only peer exists. We can use debug log or getpeerinfo but seems like either way the socks proxy is closing too soon

  32. vasild commented at 2:11 PM on June 28, 2023: contributor

    Approach ACK b5e0398d8ced2ee998756e67fb90bc8d604d4ec3

  33. DrahtBot added the label Needs rebase on Jun 30, 2023
  34. pinheadmz force-pushed on Jul 5, 2023
  35. DrahtBot removed the label Needs rebase on Jul 5, 2023
  36. DrahtBot added the label CI failed on Jul 5, 2023
  37. in test/functional/feature_anchors.py:105 in 991a478f06 outdated 
      99 | +        self.log.info(f"Add 256-bit-address block-relay-only connections to node")
 100 | +        ONION_ADDR = "pg6mmjiyjmcrsslvykfwnntlaru7p5svn6y2ymmju6nubxndf4pscryd.onion:8333"
 101 | +        self.nodes[0].addconnection(ONION_ADDR, 'block-relay-only')
 102 | +
 103 | +        self.log.info("Stop node 0")
 104 | +        with self.nodes[0].assert_debug_log([f"DumpAnchors: Flush 1 outbound block-relay-only peer addresses to anchors.dat"]):
    jonatack commented at 1:14 AM on July 6, 2023:

    CI failing at this line.

    https://cirrus-ci.com/task/5531648075235328?logs=ci#L3397 https://cirrus-ci.com/task/5531648075235328?logs=ci#L4424

    Expected messages "['DumpAnchors: Flush 1 outbound block-relay-only peer addresses to anchors.dat']" does not partially match log:

     - 2023-07-05T18:23:01.237732Z [shutoff] [logging/timer.h:56] [Log] DumpAnchors: Flush 0 outbound block-relay-only peer addresses to anchors.dat started
 - 2023-07-05T18:23:01.286842Z [shutoff] [logging/timer.h:56] [Log] DumpAnchors: Flush 0 outbound block-relay-only peer addresses to anchors.dat completed
    jonatack commented at 1:34 AM on July 6, 2023:

    Reproduced this test failure locally on the 4th run of test/functional/feature_anchors.py (arm64 macOS 13.4.1)

    pinheadmz commented at 11:04 AM on July 6, 2023:

    Indeed :-( see #27452 (review) unless @vasild disagrees I'll revert part of my last update and put keep_alive back in socks5.py

  38. pinheadmz force-pushed on Jul 7, 2023
  39. pinheadmz commented at 3:27 PM on July 7, 2023: member

    @jonatack @vasild thanks for reviewing guys. I put back the keep_alive option to ensure the onion peer is logged, expecting successful CI on this push 🤞

  40. DrahtBot removed the label CI failed on Jul 7, 2023
  41. in test/functional/test_runner.py:83 in 805692f491 outdated 
      79 | @@ -80,6 +80,7 @@
  80 |      "ripemd160",
  81 |      "script",
  82 |      "segwit_addr",
  83 | +    "messages"
    jonatack commented at 7:26 PM on July 7, 2023:

    f6bd653

    <details><summary>suggested diff</summary><p>

    @@ -76,11 +76,11 @@ TEST_FRAMEWORK_MODULES = [
     "blocktools",
     "ellswift",
     "key",
+    "messages",
     "muhash",
     "ripemd160",
     "script",
     "segwit_addr",
-    "messages"
 ]

    </p></details>

    (Also, project convention is to leave a comma after the last element, to minimize the diff in future changes)

  42. in test/functional/test_framework/messages.py:1892 in 805692f491 outdated 
    1886 | @@ -1852,3 +1887,23 @@ def serialize(self):
1887 |      def __repr__(self):
1888 |          return "msg_sendtxrcncl(version=%lu, salt=%lu)" %\
1889 |              (self.version, self.salt)
1890 | +
1891 | +class TestFrameworkScript(unittest.TestCase):
1892 | +    def test_addrv2encodedecode(self):
    jonatack commented at 7:37 PM on July 7, 2023:

    https://github.com/bitcoin/bitcoin/commit/f6bd653b161749d4a87de9ef4a91bc32d4b3def3 naming, this would be more readable in case of a test failure (also, remove 2 extra LF at EOF)

    <details><summary>diff</summary><p>

     class TestFrameworkScript(unittest.TestCase):
-    def test_addrv2encodedecode(self):
+    def test_addrv2_encode_decode(self):
         def check_addrv2(ip, net):
@@ -1905,5 +1905,3 @@ class TestFrameworkScript(unittest.TestCase):
         check_addrv2("fc32:17ea:e415:c3bf:9808:149d:b5a2:c9aa", CAddress.NET_CJDNS)
-
-

    Running Unit Tests for Test Framework Modules
................E
======================================================================
ERROR: test_addrv2_encode_decode (test_framework.messages.TestFrameworkScript.test_addrv2_encode_decode)
----------------------------------------------------------------------

    </p></details>

  43. in test/functional/test_framework/messages.py:1901 in 805692f491 outdated 
    1896 | +            addr.ip = ip
1897 | +            ser = addr.serialize_v2()
1898 | +            actual = CAddress()
1899 | +            actual.deserialize_v2(BytesIO(ser))
1900 | +            self.assertEqual(actual.ip, ip)
1901 | +            self.assertEqual(actual.net, net)
    jonatack commented at 7:50 PM on July 7, 2023:

    https://github.com/bitcoin/bitcoin/commit/f6bd653b161749d4a87de9ef4a91bc32d4b3def3 nit, can be simplified

    <details><summary>diff</summary><p>

             def check_addrv2(ip, net):
             addr = CAddress()
-            addr.net = net
-            addr.ip = ip
+            addr.net, addr.ip = net, ip
             ser = addr.serialize_v2()
             actual = CAddress()
             actual.deserialize_v2(BytesIO(ser))
-            self.assertEqual(actual.ip, ip)
-            self.assertEqual(actual.net, net)
+            self.assertEqual(actual, addr)

    </p></details>

  44. in test/functional/feature_anchors.py:140 in 805692f491 outdated 
     134 | +            new_data_hash = hash256(new_data)
 135 | +            file_handler.write(new_data + new_data_hash)
 136 | +
 137 | +        self.log.info("Restarting node attempts to reconnect to anchors")
 138 | +        with self.nodes[0].assert_debug_log([f"Trying to make an anchor connection to {ONION_ADDR}"]):
 139 | +            self.start_node(0, extra_args=[f"-onion={onion_conf.addr[0]}:{onion_conf.addr[1]}"])
    jonatack commented at 9:21 PM on July 7, 2023:

    805692f

    <details><summary>Various suggestions</summary><p>

    diff --git a/test/functional/feature_anchors.py b/test/functional/feature_anchors.py
index 6c97eed9d86..87c6082b3e0 100755
--- a/test/functional/feature_anchors.py
+++ b/test/functional/feature_anchors.py
@@ -14,6 +14,7 @@ from test_framework.util import check_node_connections, assert_equal, p2p_port
 
 INBOUND_CONNECTIONS = 5
 BLOCK_RELAY_CONNECTIONS = 2
+ONION_ADDR = "pg6mmjiyjmcrsslvykfwnntlaru7p5svn6y2ymmju6nubxndf4pscryd.onion:8333"
 
 
 class AnchorsTest(BitcoinTestFramework):
@@ -56,7 +57,7 @@ class AnchorsTest(BitcoinTestFramework):
             else:
                 inbound_nodes_port.append(hex(int(addr_split[1]))[2:])
 
-        self.log.info("Stop node 0")
+        self.log.debug("Stop node")
         self.stop_node(0)
 
         # It should contain only the block-relay-only addresses
@@ -80,7 +81,7 @@ class AnchorsTest(BitcoinTestFramework):
                 tweaked_contents[20:20] = b'1'
                 out_file_handler.write(bytes(tweaked_contents))
 
-            self.log.info("Start node")
+            self.log.debug("Start node")
             self.start_node(0)
 
         self.log.info("When node starts, check if anchors.dat doesn't exist anymore")
@@ -97,11 +98,9 @@ class AnchorsTest(BitcoinTestFramework):
         onion_proxy.start()
         self.restart_node(0, extra_args=[f"-onion={onion_conf.addr[0]}:{onion_conf.addr[1]}"])
 
-        self.log.info(f"Add 256-bit-address block-relay-only connections to node")
-        ONION_ADDR = "pg6mmjiyjmcrsslvykfwnntlaru7p5svn6y2ymmju6nubxndf4pscryd.onion:8333"
+        self.log.info("Add 256-bit-address block-relay-only connection to node")
         self.nodes[0].addconnection(ONION_ADDR, 'block-relay-only')
-
-        self.log.info("Stop node 0")
+        self.log.debug("Stop node")
         with self.nodes[0].assert_debug_log([f"DumpAnchors: Flush 1 outbound block-relay-only peer addresses to anchors.dat"]):
             self.stop_node(0)
         # Manually close keep_alive proxy connection
@@ -110,7 +109,7 @@ class AnchorsTest(BitcoinTestFramework):
         self.log.info("Check for addrv2 addresses in anchors.dat")
         caddr = CAddress()
         caddr.net = CAddress.NET_TORV3
-        [caddr.ip, port_str] = ONION_ADDR.split(":")
+        caddr.ip, port_str = ONION_ADDR.split(":")
         caddr.port = int(port_str)
         # TorV3 addrv2 serialization:
         # time(4) | services(1) | networkID(1) | address length(1) | address(32)
@@ -122,7 +121,7 @@ class AnchorsTest(BitcoinTestFramework):
         data = bytes()
         with open(node_anchors_path, "rb") as file_handler:
             data = file_handler.read()
-            assert_equal(data[services_index], 0x00) # services == NONE
+            assert_equal(data[services_index], 0x00)  # services == NONE
             anchors2 = data.hex()
             assert expected_pubkey in anchors2

@@ -138,5 +138,6 @@ class AnchorsTest(BitcoinTestFramework):
         with self.nodes[0].assert_debug_log([f"Trying to make an anchor connection to {ONION_ADDR}"]):
             self.start_node(0, extra_args=[f"-onion={onion_conf.addr[0]}:{onion_conf.addr[1]}"])
+

if __name__ == "__main__":
     AnchorsTest().main()

--- a/test/functional/test_framework/socks5.py
+++ b/test/functional/test_framework/socks5.py
@@ -116,7 +116,7 @@ class Socks5Connection():
 
             cmdin = Socks5Command(cmd, atyp, addr, port, username, password)
             self.serv.queue.put(cmdin)
-            logger.info('Proxy: %s', cmdin)
+            logger.debug('Proxy: %s', cmdin)
             # Fall through to disconnect
         except Exception as e:
             logger.exception("socks5 request handling failed.")
@@ -160,4 +160,3 @@ class Socks5Server():
         s.connect(self.conf.addr)
         s.close()
         self.thread.join()
-

    (the newline changes are on purpose)

    </p></details>

  45. jonatack commented at 9:29 PM on July 7, 2023: member

    Tested Approach ACK

  46. test: add support for all networks in CAddress in messages.py
    Also removes TorV2 from messages.py
See https://github.com/bitcoin/bitcoin/pull/22050

Co-authored-by: Matthew Zipkin <pinheadmz@gmail.com>
    80f64a3d40
  47. test: cover TorV3 address in p2p_addrv2_relay 5aaf988ccc
  48. test: add keep_alive option to socks5 proxy in test_framework
    The Socks5 server we use in the test framework would disconnect
by default immediately after the handshake and sometimes would
not register as a connected peer by bitcoind.
    b4bee4bbf4
  49. pinheadmz force-pushed on Jul 10, 2023
  50. pinheadmz commented at 2:29 PM on July 10, 2023: member

    @jonatack thank you sir for the great review and suggested patches, all nits addressed

  51. in test/functional/feature_anchors.py:132 in a5c9e059b7 outdated 
     128 | +            assert expected_pubkey in anchors2
 129 | +
 130 | +        with open(node_anchors_path, "wb") as file_handler:
 131 | +            # Modify service flags for this address even though we never connected to it.
 132 | +            # This is necessary because on restart we will not attempt an anchor connection
 133 | +            # to a host without our required services, even if its address is in the anochrs.dat file
    jonatack commented at 11:08 PM on July 11, 2023:

    s/anochrs/anchors/

                # to a peer without HasAllDesirableServiceFlags, even if present in anchors.dat.
    pinheadmz commented at 5:25 PM on July 19, 2023:

    🤦 ty sir. fixed & pushed

  52. jonatack commented at 11:09 PM on July 11, 2023: member

    ACK a5c9e059b7f463f668fa404dfc652791c481fd95

  53. test: cover addrv2 support in anchors.dat with a TorV3 address ba8ab4fc54
  54. pinheadmz force-pushed on Jul 19, 2023
  55. jonatack commented at 6:19 PM on July 19, 2023: member

    ACK ba8ab4fc545800c4fb283a5ff0b19138a0451aba

  56. brunoerg approved
  57. brunoerg commented at 6:26 PM on July 24, 2023: contributor

    crACK ba8ab4fc545800c4fb283a5ff0b19138a0451aba

    code lgtm!

  58. willcl-ark approved
  59. willcl-ark commented at 10:16 AM on August 2, 2023: member

    ACK ba8ab4fc54

    Nice to have this additional coverage for TorV3 addresses 👍🏼

    It's not intruduced by this PR, but I notice a mypy error in feature_anchors.py where a tuple is assigned to onion_conf.addr which is initialized to None in socks5.py.

    image

    If you do end up re-touching this for some reason, you can fix the warning by specifying the type of .addr in socks5.py (which will also fix the other 3 ocurrences of it in the tests):

    diff --git a/test/functional/test_framework/socks5.py b/test/functional/test_framework/socks5.py
index 0ca06a7396..5315f39142 100644
--- a/test/functional/test_framework/socks5.py
+++ b/test/functional/test_framework/socks5.py
@@ -8,6 +8,7 @@ import socket
 import threading
 import queue
 import logging
+from typing import Tuple
 
 logger = logging.getLogger("TestFramework.socks5")
 
@@ -36,7 +37,7 @@ def recvall(s, n):
 class Socks5Configuration():
     """Proxy configuration."""
     def __init__(self):
-        self.addr = None # Bind address (must be set)
+        self.addr: Tuple[str, int] # Bind address (must be set)
         self.af = socket.AF_INET # Bind address family
         self.unauth = False  # Support unauthenticated
         self.auth = False  # Support authentication
  60. fanquake merged this on Aug 2, 2023
  61. fanquake closed this on Aug 2, 2023
  62. sidhujag referenced this in commit a3b45d2558 on Aug 9, 2023
  63. bitcoin locked this on Aug 2, 2024
Contributors
pinheadmzDrahtBotbrunoergmeuamigopedrowillcl-arkvasildjonatack
Labels
Tests
Linked (view graph)
#20511 anchors.dat doesn't support V2 addresses#20516 Well-defined CAddress disk serialization, and addrv2 anchors.dat#27140 Add support for all networks in `deserialize_v2` in test_framework
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-04-19 12:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me