test: extend the SOCKS5 Python proxy to actually connect to a destination

vasild commented at 3:30 pm on February 11, 2024: contributor

If requested, make the SOCKS5 Python proxy redirect connections to a set of given destinations. Actually act as a real proxy, connecting the client to a destination, except that the destination is not what the client asked for.

This would enable us to “connect” to Tor addresses from the functional tests.

Plus a few other minor improvements in the test framework as individual commits.

These changes are part of #29415 but they make sense on their own and would be good to have them, regardless of the fate of #29415. Also, if this is merged, that would reduce the size of #29415, thus the current standalone PR.

DrahtBot commented at 3:30 pm on February 11, 2024: contributor

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

Code Coverage

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	pinheadmz

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#29431 (test/BIP324: disconnection scenarios during v2 handshake by stratospher)

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.

DrahtBot added the label Tests on Feb 11, 2024

in test/functional/test_framework/p2p.py:194 in 05f8c032b8 outdated

187@@ -188,6 +188,10 @@ def peer_connect_helper(self, dstaddr, dstport, net, timeout_factor):
188         self.on_connection_send_msg = None
189         self.recvbuf = b""
190         self.magic_bytes = MAGIC_BYTES[net]
191+        if dstport != 0:
192+            self.p2p_connected_to_node = True
193+        else:
194+            self.p2p_connected_to_node = False

Empact commented at 7:13 am on February 27, 2024:

nit: you can assign the comparison directly

vasild commented at 12:57 pm on February 29, 2024:

Done

DrahtBot added the label Needs rebase on Feb 27, 2024

vasild force-pushed on Feb 29, 2024

vasild commented at 12:57 pm on February 29, 2024: contributor

05f8c032b8...4267c206f4: rebase due to conflicts and address #29420 (review)

DrahtBot added the label CI failed on Feb 29, 2024

DrahtBot removed the label Needs rebase on Feb 29, 2024

DrahtBot removed the label CI failed on Feb 29, 2024

test: improve debug log message from P2PConnection::connection_made()

This is used in both cases - TCP server (accept) and TCP client (connect).
The message "Connected & Listening address:port" is confusing.

Print both ends of the TCP connection.

9a9b545c7f

test: extend the SOCKS5 Python proxy to actually connect to a destination

If requested, make the SOCKS5 Python proxy redirect connections to a set
of given destinations. Actually act as a real proxy, connecting the
client to a destination, except that the destination is not what the
client asked for.

This would enable us to "connect" to Tor addresses from the functional
tests.

1e76be0431

test: support WTX INVs from P2PDataStore and fix a comment 1e2a31ea33

test: set P2PConnection.p2p_connected_to_node in peer_connect_helper()

Set `P2PConnection.p2p_connected_to_node` in
`P2PConnection.peer_connect_helper()` instead of
`TestNode.add_p2p_connection()` and
`TestNode.add_outbound_p2p_connection()`.

This way tests can create an instance of `P2PConnection` and use
`P2PConnection.peer_connect_helper()` directly.

9c50b2fe44

vasild force-pushed on Mar 18, 2024

vasild commented at 4:39 pm on March 18, 2024: contributor

4267c206f4...9c50b2fe44: pick changes due to suggestions from https://github.com/bitcoin/bitcoin/pull/29415

in test/functional/test_framework/p2p.py:225 in 9a9b545c7f outdated

221+        info = transport.get_extra_info("socket")
222+        us = info.getsockname()
223+        them = info.getpeername()
224+        logger.debug(f"Connected: us={us[0]}:{us[1]}, them={them[0]}:{them[1]}")
225+        self.dstaddr = them[0]
226+        self.dstport = them[1]

pinheadmz commented at 6:26 pm on April 17, 2024:

https://github.com/bitcoin/bitcoin/pull/29420/commits/9a9b545c7fc044242c4dd8a4916b2ba13b3ea850

Why do we need to replace these values? I changed this to:

0        assert self.dstaddr == them[0]
1        assert self.dstport == them[1]

…and all the p2p tests still passed

vasild commented at 3:40 pm on May 7, 2024:

I set dstaddr and dstport here in connection_made() because when accepting connections (from the python node point of view) they are set to "0" and 0 respectively (in master):

https://github.com/bitcoin/bitcoin/blob/ef09f535b7b540d997f8c2bfa67b3386711bc8b4/test/functional/test_framework/p2p.py#L203

https://github.com/bitcoin/bitcoin/blob/ef09f535b7b540d997f8c2bfa67b3386711bc8b4/test/functional/test_framework/p2p.py#L182-L186

When I add those asserts I get:

0    assert self.dstaddr == them[0]
1           ^^^^^^^^^^^^^^^^^^^^^^^
2AssertionError

vasild commented at 10:23 am on July 2, 2024:

from comment #29415 (review):

Why didn’t we need to do this on master? The old log message was Connected & Listening: 127.0.0.1:15689 not Connected & Listening: 0:0

When accepting connections (from the python node point of view), then indeed 0:0 is printed (on master):

0$ ./test/functional/p2p_addr_relay.py -l debug --nocleanup
1$ ./test/functional/combine_logs.py |grep 'Connected & L'
2...
3 test  2024-07-02T10:18:18.494000Z TestFramework.p2p (DEBUG): Connected & Listening: 127.0.0.1:14829 
4 test  2024-07-02T10:18:18.609000Z TestFramework.p2p (DEBUG): Connected & Listening: 127.0.0.1:14829 
5 test  2024-07-02T10:18:20.556000Z TestFramework.p2p (DEBUG): Connected & Listening: 0:0 
6 test  2024-07-02T10:18:20.719000Z TestFramework.p2p (DEBUG): Connected & Listening: 0:0 
7 test  2024-07-02T10:18:20.934000Z TestFramework.p2p (DEBUG): Connected & Listening: 127.0.0.1:14829 
8 test  2024-07-02T10:18:22.265000Z TestFramework.p2p (DEBUG): Connected & Listening: 0:0

in test/functional/test_framework/socks5.py:147 in 1e76be0431 outdated

142+                    while not done:
143+                        rlist, _, xlist = select.select(sockets, [], sockets)
144+                        if len(xlist) > 0:
145+                            raise IOError("Exceptional condition on socket")
146+                        for s in rlist:
147+                            data = s.recv(4096)

pinheadmz commented at 6:48 pm on April 17, 2024:

https://github.com/bitcoin/bitcoin/pull/29420/commits/1e76be04318a484124b82199260f23b72801e459

any reason for this particular buffer size? some quick research suggests that 4kB and 8kB are common values for this, I’m just wondering if it really affects us.

vasild commented at 3:44 pm on May 7, 2024:

No, I just picked some value that looks reasonable, anything would work. For example 1 or 5 would still work but do not look reasonable because they will cause a lot of iteration loops.

in test/functional/test_framework/socks5.py:54 in 1e76be0431 outdated

49+        #     "node": python_p2p_node,
50+        #     "requested_to_addr": "the_client_asked_to_connect_to_this_addr.onion",
51+        # }
52+        # Redirect the i'th connection to destinations[i] to_addr:to_port
53+        self.destinations = []
54+        self.destinations_used = 0

pinheadmz commented at 7:54 pm on April 17, 2024:

https://github.com/bitcoin/bitcoin/pull/29420/commits/1e76be04318a484124b82199260f23b72801e459

Instead of the counter, you could also use dest = self.destiantions.pop() below and then log your warning once the array is empty

vasild commented at 3:46 pm on May 7, 2024:

I slightly prefer to keep the array unmodified in case the data in it is needed at some later point (for logging or something).

in test/functional/test_framework/socks5.py:143 in 1e76be0431 outdated

138+                    self.conn.setblocking(False)
139+                    conn_to.setblocking(False)
140+                    sockets = [self.conn, conn_to]
141+                    done = False
142+                    while not done:
143+                        rlist, _, xlist = select.select(sockets, [], sockets)

pinheadmz commented at 7:55 pm on April 17, 2024:

https://github.com/bitcoin/bitcoin/pull/29420/commits/1e76be04318a484124b82199260f23b72801e459

had to read the docs on this one but I agree it is correct 👍

pinheadmz approved

pinheadmz commented at 8:18 pm on April 17, 2024: member

ACK 9c50b2fe44e4744204c51b82e86174db1d749cfe

Reviewed each commit and left some questions below. Built and ran all unit and functional tests on arm/macos. I am already familiar with this feature from #29415 and have run those tests as well.

One nitty nitty nit nit: commit message 9a9b545c7fc044242c4dd8a4916b2ba13b3ea850 uses c++ syntax instead of python ;-)

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA256
 2
 3ACK 9c50b2fe44e4744204c51b82e86174db1d749cfe
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmYgKwwACgkQ5+KYS2KJ
 7yTptJQ/+OksRFOpcFGtbEYIaaAaiWAlLJVBQYa2ljpRvSxecoWqmcgP6wvaXGSsA
 8SxBiu7Y+1fZqBEaRLeOLm7GtZiZesc87Cu2BJGM5SvxLkiF6l7YqtwISq51Z0cZu
 9C15s/eSexvpLFp+tGvlcm9au5iZShU7nGMVNAEz5vfFUJOQBDVDJ1mib1hxaLRaV
104U97uoYQcOVZDz+fdzisekP1wpklXZTDQiS9grbHMBmqeT45ugxgUEC+DVWFtm5W
1101JBblBPNLvlOFiNpHCy0MUNXtg7n1ER2ee677ZTP05ck9hOga0gu8g8doemvZ91
12QPs2nLq0dHvaFtlYl2N3Ce3aRBX6I3qfegKWmrBWm54KB3mOSrV3OP2j0UMUGmcM
13nwPQruUg+GnBsEWK9RgvLAmSFbkGEJzS0nqFfOkRLLEwNWAzMUYAooXs4J7xLVcA
14AR8ncBnz9zVBkdhrHbT8wYcf22N/W+k3fwxCgTxHPiAt10UXAzkdfnRfwjjQG7NU
15xmP20eAFsE9FWOnfkVIh/VCzcgS+wT629gaVYJqF9hX16K9uKH2AuOQeCLnRiX3D
16E1B1YnspQklZAy0ejc2BZEuwBc+MjtSEowTPK9ENT8NJoEO2y3YrnGcbXThyVrBQ
17yvLU1dfljFfSq2seaNP4fpKJT8cKXAdCHj/HlowVsYw9hDs/PKU=
18=VgCO
19-----END PGP SIGNATURE-----

pinheadmz’s public key is on keybase

test: extend the SOCKS5 Python proxy to actually connect to a destination #29420

Code Coverage

Reviews

Conflicts