Avoid internet traffic from tests #31339

issue vasild openend this issue on November 21, 2024
  1. vasild commented at 12:18 pm on November 21, 2024: contributor

    Current behaviour

    Tests should not try to open connections to the internet because:

    • they may succeed or fail unpredictably, depending on external environment
    • are slow
    • dox the developer to their ISP that they are running Bitcoin Core tests

    Expected behaviour

    Tests should only open local connections (e.g. on the lo interface).

    Enforce this in the CI, having it to detect non-local traffic and fail accordingly.

    Steps to reproduce

    Run the functional tests and monitor the traffic.

    Relevant log output

    No response

    How did you obtain Bitcoin Core

    Compiled from source

    What version of Bitcoin Core are you using?

    master@22ef95dbe3e467039e6cd18988e66557d94041d1

    Operating system and version

    Ubuntu 28.04 LTS

  2. l0rinc commented at 12:23 pm on November 21, 2024: contributor
    Having no fully end-to-end tests sounds a bit dangerous, since it’s part of the fundamental behavior of Bitcoin, not sure we should completely mock that away (i.e. certain bugs could creep in that work locally only - no idea how that would be possible, but that’s exactly the point).
  3. fanquake commented at 12:25 pm on November 21, 2024: member
    Which tests are you seeing this from? Neither the unit nor functional tests should be making outside connections. This has certainly been discussed/avoided previously so may be a recent regression.
  4. vasild commented at 12:25 pm on November 21, 2024: contributor

    I have the idea to log non local traffic with iptables and fail the CI. But I do not understand why nothing is logged - I added some quick and dirty commands to ci/test/03_test_script.sh:

     0iptables  -A OUTPUT -m addrtype \! --dst-type LOCAL -j LOG --log-prefix "eeeee1" --log-level emerg
 1iptables  -A OUTPUT -j LOG --log-prefix "eeeee2" --log-level emerg
 2... generate some traffic with telnet and ping ...
 3# confirm some packets matched:
 4iptables -v -x -n -L
 5...
 62024-11-21T11:38:45.5295264Z Chain OUTPUT (policy ACCEPT 19 packets, 1264 bytes)
 72024-11-21T11:38:45.5296495Z     pkts      bytes target     prot opt in     out     source               destination         
 82024-11-21T11:38:45.5298695Z       13      828 LOG        0    --  *      *       0.0.0.0/0            0.0.0.0/0            ADDRTYPE match dst-type !LOCAL LOG flags 0 level 0 prefix "eeeee1"
 92024-11-21T11:38:45.5300746Z       19     1264 LOG        0    --  *      *       0.0.0.0/0            0.0.0.0/0            LOG flags 0 level 0 prefix "eeeee2"
10...

    but nothing about those is found in dmesg(1) output or in /var/log.

    0ps ax
12024-11-21T11:38:45.5842111Z     PID TTY      STAT   TIME COMMAND
22024-11-21T11:38:45.5842436Z       1 pts/0    Ss+    0:00 /bin/bash
32024-11-21T11:38:45.5842983Z      36 ?        Ss     0:00 bash /home/runner/work/_temp/ci/test/03_test_script.sh
42024-11-21T11:38:45.5843441Z      68 ?        R      0:00 ps ax

    So syslog is not running, but still dmesg(1) should contain the messages? Full log https://github.com/vasild/bitcoin/actions/runs/11952402639/job/33318041653?pr=4

  5. vasild commented at 12:32 pm on November 21, 2024: contributor
    @l0rinc the current end to end tests do communicate locally via the lo interface. @fanquake Honestly, I forgot which test was that. During latest coredev I investigated that briefly with @maflcko or @0xB10C (?) and we observed the non local traffic with tcpdump(1). Now, my point is to enforce this in the CI, even if such test does not exist currently - has been fixed since coredev or my investigation back then was wrong.
  6. maflcko commented at 12:40 pm on November 21, 2024: member

    I think it makes sense to enforce this in CI (and have a tracking issue or pull for that). Otherwise, it is easy to regress, like in #30529 (comment)

    However, if there is a violation of this, or a bug, it would be good to add exact steps to reproduce, or an explanation. Claiming that this is broken on “Ubuntu 28.04 LTS” doesn’t seem helpful to uncover the bug, if there is one.

    Related: https://github.com/bitcoin/bitcoin/issues/30030

  7. jonatack commented at 6:14 pm on November 21, 2024: member

    Enforce this in the CI, having it to detect non-local traffic and fail accordingly.

    Concept ACK

  8. mzumsande commented at 8:04 pm on November 21, 2024: contributor

    Honestly, I forgot which test was that.

    Maybe it was #29605 (review) (I independently ran into that a few weeks ago and included a fix in #31142)

  9. theStack commented at 10:15 pm on November 21, 2024: contributor

    @fanquake Honestly, I forgot which test was that. During latest coredev I investigated that briefly with @maflcko or @0xB10C (?) and we observed the non local traffic with tcpdump(1). Now, my point is to enforce this in the CI, even if such test does not exist currently - has been fixed since coredev or my investigation back then was wrong.

    I think that was us back then, we looked at rpc_net.py and found this line: https://github.com/bitcoin/bitcoin/blob/17834bd1976df7a2ff6c2f5f05a59ae3fd3f6875/test/functional/rpc_net.py#L253 Opened #31343 for a proposed fix of this instance (maybe there are more).

    Enforce this in the CI, having it to detect non-local traffic and fail accordingly.

    Concept ACK

  10. vasild referenced this in commit 54a64825ad on Nov 22, 2024
  11. vasild commented at 2:09 pm on November 22, 2024: contributor

    @theStack, yes, it was that one, to 11.22.33.44, thanks!

    Lets keep the focus here on enforcing this in the CI, rather than individual tests that do that.

    I couldn’t find the output of iptables -j LOG in the CI - it is nowhere to be found in /var/log nor in dmesg output even though iptables -v -x -n -L shows some packets matched. Any help would be welcome!

    Using tcpdump(1) is another approach that got me further: #31349 and it detects indeed some interesting stuff:

    02024-11-22T13:18:59.9043792Z 13:13:38.030342 IP 172.18.0.2.58228 > 11.22.33.44.18444: Flags [S], seq 3319460523, win 64240, options [mss 1460,sackOK,TS val 1156831752 ecr 0,nop,wscale 7], length 0
12024-11-22T13:18:59.9044913Z 13:13:42.540986 IP 11.22.33.44.18444 > 172.18.0.2.58224: Flags [R.], seq 684480607, ack 0, win 0, length 0

    11.22.33.44 even responds to the test! And some more interesting stuff:

    02024-11-22T13:18:59.9052028Z 13:17:56.527339 IP 172.18.0.2.59916 > 0.0.0.1.18444: Flags [S], seq 2190025647, win 64240, options [mss 1460,sackOK,TS val 4215569398 ecr 0,nop,wscale 7], length 0
12024-11-22T13:18:59.9081176Z 13:18:17.646343 IP 172.18.0.2.45242 > 0.0.0.2.18444: Flags [S], seq 2461798101, win 64240, options [mss 1460,sackOK,TS val 274989920 ecr 0,nop,wscale 7], length 0
22024-11-22T13:18:59.9083474Z 13:18:18.286342 IP 172.18.0.2.47882 > 1.2.3.4.9050: Flags [S], seq 377974635, win 64240, options [mss 1460,sackOK,TS val 1999254858 ecr 0,nop,wscale 7], length 0
32024-11-22T13:18:59.9084632Z 13:18:18.472934 IP 1.2.3.4.9050 > 172.18.0.2.47876: Flags [R.], seq 319153034, ack 0, win 0, length 0

    (full log at https://github.com/vasild/bitcoin/actions/runs/11972758832/job/33380324078?pr=4)

  12. vasild referenced this in commit 3ec89f41d8 on Nov 22, 2024
  13. vasild referenced this in commit 3c4b2035e1 on Nov 25, 2024
  14. vasild referenced this in commit 67c6bce240 on Nov 25, 2024
  15. vasild referenced this in commit 1592a7dad4 on Nov 25, 2024
  16. vasild referenced this in commit 071e43ffae on Nov 25, 2024
  17. 0xB10C commented at 11:28 pm on November 25, 2024: contributor

    Did you consider running the docker container in CI with --network none?

    https://docs.docker.com/engine/network/drivers/none/

    Edit: Oh, I noticed we already have ci-ip6net as a custom network for IPv6 tests. Then we can’t use -network none, but setting --internal (https://docs.docker.com/reference/cli/docker/network/create/#internal) should work.

    Edit 2: --internal works, but maybe a bit to well. It also blocks the connection to GitHub for downloading the QA assets: https://github.com/0xB10C/bitcoin/actions/runs/12021121581/job/33511028509?pr=9#step:7:2785

  18. maflcko commented at 9:00 am on November 26, 2024: member

    Edit 2: --internal works

    Good point, but I don’t think this works conceptually to turn any and all network access in the tests into a failure, because a failure to access the network in the tests may not always lead to a test failure.

  19. vasild commented at 9:01 am on November 26, 2024: contributor
    @0xB10C my wish is to catch and report these, not just block them silently (only) from the CI environments.


vasild l0rinc fanquake maflcko jonatack mzumsande theStack 0xB10C

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