ci: detect outbound internet traffic generated while running tests #31349
+65
−3
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31349. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
2 tasks
54a6482 to
3ec89f4
Compare
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
|
Nice. Conecpt ACK! |
Merged
|
Concept ACK i'm slightly worried this may generate false positive. As is, this detects traffic on the entire (virtual) machine while running the tests. Are there no other daemons running on the CI instance that could interfere with this? |
3ec89f4 to
3c4b203
Compare
|
@laanwj, Right! And Another source of false positive could be if somebody from the outside initiates communication to the VM to which it responds. E.g. an outsider tries to connect to the VM to which it responds with an outbound packet e.g. TCP RST. At least that should be obvious from the error log, showing the incoming packet first (I just pushed a slight change for that). Maybe also the traffic-from-another-daemon could be obvious - e.g. if there is traffic to |
3c4b203 to
67c6bce
Compare
67c6bce to
1592a7d
Compare
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
Exactly. For all we know, the CI VM is firewalled off sufficiently that this can't happen, but we don't know.
Ah yes, as long as it's only some extra logging, having a manual factor in this is fine. It only becomes critical if network traffic would cause a CI failure. i'm not aware of a straightforward way to "log network traffic of this process and subproceses only". Yes, it could be done with a linux network namespace, but that's a lot of hassle.
Seeing this, it might already be namespaced. Though a process namespace doesn't necessarily mean the network namespace is isolated. |
1592a7d to
071e43f
Compare
|
My intention here is to fail the CI because otherwise the log will be buried in the CI output and nobody will notice it. It follows that if this fails randomly with false positives when one would have to investigate it manually for arbitrary PRs which is highly highly highly undesirable. |
maflcko
reviewed
Nov 25, 2024
|
Ran this on my CI runner which has 8.8.8.8 configured as DNS server for docker. https://cirrus-ci.com/task/5500763260059648?logs=ci#L1137 Edit: My understanding is as follows: The DNS requests normally go to a local DNS resolver which then asks an upstream resolver. The upstream resolver (possibly your ISP) indirectly learns that you are running Bitcoin Core tests, even if there was no direct communication over a non-loopback interface. Edit 2: full tcpdump output here: |
This comment was marked as abuse.
This comment was marked as abuse.
|
Concept ACK. Per https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2024-11-26#1069602: "it turns out the owners of 1.2.3.4, 11.22.33.44 and 8.8.8.8, if they would bother, would know the IP address of every dev who runs the functional tests at home." |
071e43f to
8799018
Compare
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
8799018 to
803ed46
Compare
|
About the false positives - I think it is worth trying this in its current mode where any detected traffic is assumed to have originated from the tests and fails the CI. If this ever fails the CI for another reason (false positive), then it would be easy to turn this into a "report in the logs only but don't fail" by removing the About the DNS traffic - I did |
bbfc58a to
69e0766
Compare
|
If people want to run this manually, outside of the CI in a way that causes false positive (discussion above), what about something like this: --- i/ci/test/03_test_script.sh
+++ w/ci/test/03_test_script.sh
@@ -184,13 +184,15 @@ function traffic_monitor_end()
# "permission denied" if they are not owned by root:root.
chown root:root "$f"
out="$(tcpdump -n -r "$f" --direction=out tcp or udp)"
if [ -n "$out" ] ; then
echo "Error: outbound TCP or UDP packets on the non loopback interface generated during $test_name tests:" >&2
tcpdump -n -r "$f" tcp or udp
- exit 1
+ if [ -z "$INTERNET_TRAFFIC_EXPECTED" ] ; then
+ exit 1
+ fi
fi
done
}
if [ "$RUN_UNIT_TESTS" = "true" ]; then
traffic_monitor_begin "unitparallel"and then, obviously, set |
This was referenced Jan 16, 2025
69e0766 to
76feae2
Compare
|
|
This was referenced Feb 21, 2025
Closed
76feae2 to
4652f75
Compare
|
|
This was referenced Jul 3, 2025
1 task
|
🐙 This pull request conflicts with the target branch and needs rebase. |
fanquake
added a commit
that referenced
this pull request
Aug 5, 2025
3333d3f ci: Only pass documented env vars (MarcoFalke) Pull request description: The CI currently inherits almost all env vars from the host. This was problematic in the past and causing non-determinism, e.g. the fix in commit fa12558. It is still problematic today, see e.g. #31349 (comment), or #32935 This fixes #32935 by only passing env vars documented in `./ci/test/00_setup_env.sh`. Implementation-wise, instead of cramming the python code into the `python -c ""` statement, just start a fresh py file, which is easier to handle. ACKs for top commit: willcl-ark: ACK 3333d3f Tree-SHA512: f922e481a844128d7fbf773563278a3992c178ead60a3050eceb9ded2aad979afc815a5cbdb9f68494493c5d8d942cdd1111c21e32a5746d19505b87745cb84a
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Prevent generating outbound traffic on a non-loopback interface during tests.
[1111:1111::1]:53:previous releases, depends DEBUGTSan, depends, guimultiprocess, i686, DEBUGno wallet, libbitcoinkernel02_run_container.sh:--cap-add NET_RAWexit 1line added to03_test_script.shin this PR.Summary of each VM wrt the new check:
32-bit CentOS, dash, gui(Cirrus CI)ARM, unit tests, no functional tests(Cirrus CI)ASan + LSan + UBSan + integer, no depends, USDT(GitHub)MSan, depends(Cirrus CI)TSan, depends, gui(Cirrus CI)Win64 native fuzz, VS 2022(GitHub)Win64 native, VS 2022(GitHub)Win64-cross(Cirrus CI)fuzzer,address,undefined,integer, no depends(Cirrus CI)lint(Cirrus CI)macOS 14 native, arm64, fuzz(GitHub)macOS 14 native, arm64, no depends, sqlite only, gui(GitHub)macOS-cross, gui, no tests(Cirrus CI)multiprocess, i686, DEBUG(Cirrus CI)no wallet, libbitcoinkernel(Cirrus CI)previous releases, depends DEBUG(Cirrus CI)test each commit(GitHub)tidy(Cirrus CI)✔️: test is run under tcpdump (it works!)
-: test is not run in that VM
1:
tcpdump: en0: You don't have permission to capture on that device(does not work)2: tests are run but directly from
.github/workflows/ci.ymlinstead of fromci/test/03_test_script.sh(does not work)Resolves #31339