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

    This is being fixed in two parts:

  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.
  20. vasild referenced this in commit e84a81295e on Nov 26, 2024
  21. vasild referenced this in commit 8799018bd5 on Nov 27, 2024
  22. vasild referenced this in commit 803ed4638b on Nov 27, 2024
  23. vasild referenced this in commit 21b974ff76 on Nov 28, 2024
  24. vasild referenced this in commit 59bdf9cc78 on Nov 29, 2024
  25. vasild referenced this in commit 192bc1c8d1 on Nov 29, 2024
  26. vasild referenced this in commit 6807a3e5be on Nov 29, 2024
  27. vasild referenced this in commit 375ebac037 on Dec 2, 2024
  28. vasild referenced this in commit c88464d754 on Dec 3, 2024
  29. vasild referenced this in commit feabffd808 on Dec 4, 2024
  30. vasild referenced this in commit 925ee8707e on Dec 4, 2024
  31. vasild referenced this in commit 56dbe78934 on Dec 5, 2024
  32. vasild referenced this in commit 5f3b24711e on Dec 5, 2024
  33. vasild referenced this in commit a6c3c9defb on Dec 5, 2024
  34. achow101 referenced this in commit 8ad2c90274 on Dec 11, 2024
  35. vasild referenced this in commit 46e38e2e33 on Dec 11, 2024
  36. vasild referenced this in commit 95fc90610a on Dec 17, 2024
  37. vasild referenced this in commit 0ac9caf7be on Dec 23, 2024
  38. vasild referenced this in commit d8cd80e814 on Dec 23, 2024
  39. vasild referenced this in commit 48843ec694 on Dec 24, 2024
  40. vasild referenced this in commit bbfc58a0af on Jan 6, 2025
  41. Sjors referenced this in commit e839388e91 on Jan 13, 2025
  42. laanwj added the label Tests on Jan 14, 2025
  43. vasild referenced this in commit 69e076664d on Jan 15, 2025
  44. Sjors referenced this in commit 815b2f0e73 on Jan 15, 2025
  45. Sjors referenced this in commit 292ff86b74 on Jan 15, 2025
  46. Sjors referenced this in commit bb8a470b92 on Jan 15, 2025
  47. Sjors referenced this in commit 757e4ff503 on Jan 15, 2025


vasild l0rinc fanquake maflcko jonatack mzumsande theStack 0xB10C

Labels
Tests

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