test: Disable automatic connections per default in the functional tests

mzumsande commented at 11:30 pm on July 18, 2021: contributor

A node normally doesn’t make automatic connections to peers in the functional tests because neither DNS seeds nor hardcoded peers are available on regtest. However, when random entries are inserted into addrman as part of a functional test (e.g. while testing addr relay), ThreadOpenConnections will periodically try to connect to them, resulting in log entries such as: [opencon] [net.cpp:400] [ConnectNode] trying connection 18.166.1.1:8333 lastseen=0.0hrs

I don’t think it’s desirable that functional tests try to connect to random computers on the internet, aside from the possibility that at some point in time someone out there might actually answer in a way to ruin a test.

This PR fixes this problem by disabling ThreadOpenConnections by adding -connect=0 to the default args, and adding exceptions only when needed for the test to pass.

DrahtBot commented at 11:53 pm on July 18, 2021: contributor

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

Conflicts

No conflicts as of last run.

DrahtBot added the label Tests on Jul 19, 2021

DrahtBot added the label Needs rebase on Jul 19, 2021

jnewbery commented at 12:26 pm on July 19, 2021: contributor

Concept ACK

mzumsande force-pushed on Jul 19, 2021

DrahtBot removed the label Needs rebase on Jul 19, 2021

maflcko commented at 4:31 pm on July 19, 2021: member

Any reason to not disable this globally?

See also fa730e91576

mzumsande commented at 5:25 pm on July 19, 2021: contributor

Any reason to not disable this globally?

To expand on the last sentence of the OP (I tried disabling this globally by adding f.write("connect=0\n") to write_config in util.py):

With ThreadOpenConnections disabled completely in the functional tests, possible future problems with it would not be noticed there
Covering any logic from ThreadOpenConnections in functional tests would not be possible. Tricks like neutralizing a global -connect=0 with a local -noconnect=0 in a single test do not seem to restore the default behavior - am I missing some other way to do this?
feature_config_args.py already covers parts from ThreadOpenConnections (log messages), so these tests would have to be removed as well.

maflcko commented at 5:36 pm on July 19, 2021: member

With ThreadOpenConnections disabled completely in the functional tests, possible future problems with it would not be noticed there

There could be one (unit or function) test to call it?

Covering any logic from ThreadOpenConnections in functional tests would not be possible. Tricks like neutralizing a global -connect=0 with a local -noconnect=0 in a single test do not seem to restore the default behavior - am I missing some other way to do this?

There could be a option passed to write_config, similar to extra_config to skip noconnect

feature_config_args.py already covers parts from ThreadOpenConnections (log messages), so these tests would have to be removed as well.

See above

mzumsande commented at 10:11 am on July 20, 2021: contributor

Thanks, good idea! I’ll try this, might be a couple of days until I’ll get to it, will change to draft until then.

mzumsande marked this as a draft on Jul 20, 2021

amitiuttarwar commented at 6:55 pm on July 20, 2021: contributor

concept ACK

practicalswift commented at 8:14 pm on July 24, 2021: contributor

Super Strong Concept ACK

The act of running the test suite should never cause any communication with the outside world, and certainly not cause TCP connections to random computers on the Internet :)

test: Disable automatic connections by default

This prevents the node from trying to connect to random IPs on the internet
while running the functional tests. Exceptions are added when required for
the test to pass.

8ca51af1ec

mzumsande force-pushed on Jul 26, 2021

mzumsande renamed this:
~~test: Disable automatic connections when addrman is non-empty~~
test: Disable automatic connections per default in the functional tests
on Jul 26, 2021

mzumsande commented at 5:22 pm on July 26, 2021: contributor

As suggested by @MarcoFalke, I now reversed the logic to disable automatic connections by default and add exceptions only when required (feature_config_args.py and feature_anchors.py). I changed OP and title to reflect this.

mzumsande marked this as ready for review on Jul 26, 2021

tryphe commented at 10:05 am on July 30, 2021: contributor

Concept ACK, light code review ACK 8ca51af1ece371b6f1bdb88b96f16020cc787d13

maflcko merged this on Jul 30, 2021

maflcko closed this on Jul 30, 2021

amitiuttarwar commented at 5:20 pm on July 30, 2021: contributor

@mzumsande- did you consider the approach of adding -connect=0 as a command line arg in the TestNode initializer (aka around here)?

I’m thinking something like

0        if not any("-connect" in s for s in self.extra_args):
1            self.args.append("-connect=0")

When looking at #22098 (comment), I realized that test doesn’t actually need rebase or an exception added, because passing in something like self.start_node(0, ["-connect=fakenodeaddr"]) from the individual test appends the -connect arg as a command-line argument, which overrules what’s been set in the .conf file.

So this means the tests setting self.disable_autoconnect would sometimes be unnecessary?

I think my proposed alternative could simplify the test framework code (don’t have to pass around the disable_autoconnect param), and reduce overhead when implementing an individual test (don’t have to set an additional field, can just pass -connect=whatever). But I didn’t fully hash this out, so I easily could be missing something.

What do you think?

amitiuttarwar commented at 5:42 pm on July 30, 2021: contributor

hm, I think my suggestion ☝️ does not work for the tests that hit normal automatic connections logic in ThreadOpenConnections. The DNS seeding can be enabled using -dnsseed, but if we pass -connect=anything, ThreadOpenConnections skips using addrman. darn.

so setting as a command line arg instead of in the conf file is still an option, but we’d need an additional field regardless. still could be nice to have more straightforward logs (instead of having two -connect=x, we’d just have one), but the benefit is definitely reduced compared to what I was thinking.

sidhujag referenced this in commit d4463a61cf on Aug 1, 2021

mzumsande commented at 2:05 pm on August 22, 2021: contributor

Sorry for not answering sooner - I had shortly thought about this option before creating this PR, but thought it would fit in better in util.py where similar networking parameters such as dnsseed are set. I didn’t think about the log issue.

hm, I think my suggestion point_up does not work for the tests that hit normal automatic connections logic in ThreadOpenConnections.

I agree, it wouldn’t work for tests like feature_anchors.py without a parameter.

So with the upside of having the networking command-line args in one place, and the downside of having two log items, I don’t have a strong preference here.

amitiuttarwar commented at 7:36 pm on August 22, 2021: contributor

@mzumsande -

So with the upside of having the networking command-line args in one place, and the downside of having two log items, I don’t have a strong preference here.

Agreed, let’s leave it as is. Thanks!

Fabcien referenced this in commit c58c98c444 on Feb 2, 2022

bitcoin locked this on Aug 22, 2022

mzumsande deleted the branch on Oct 13, 2023

test: Disable automatic connections per default in the functional tests #22490

Conflicts