test: node will not try to connect to anchors from an unreachable network #34431

brunoerg commented at 2:28 PM on January 28, 2026: contributor

This PR adds a test to check that a node will not try to connect to an anchor from an unreachable network when initializing. It kills and can be tested with the following mutant:

diff --git a/src/net.cpp b/src/net.cpp
index 6b79e913e8..cd8aa4c502 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -2774,7 +2774,7 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect, std
             if (anchor && !m_anchors.empty()) {
                 const CAddress addr = m_anchors.back();
                 m_anchors.pop_back();
-                if (!addr.IsValid() || IsLocal(addr) || !g_reachable_nets.Contains(addr) ||
+                if (!addr.IsValid() || IsLocal(addr) ||
                     !m_msgproc->HasAllDesirableServiceFlags(addr.nServices) ||
                     outbound_ipv46_peer_netgroups.contains(m_netgroupman.GetGroup(addr))) continue;
                 addrConnect = addr;

Also, it fixes a bug in assert_debug_log which has a timing asymmetry between expected and unexpected message checking. When all expected messages were found early, the function would return immediately without waiting for the full timeout period. This created a race condition where unexpected messages appearing after the expected messages (but before the timeout) would not be detected.

test: check node will not attempt to connect to anchors from unreachable networks d39c7196b5

test: fix unexpected_msgs logic in assert_debug_log

The assert_debug_log has a timing asymmetry between expected and unexpected message
checking. When all expected messages were found early, the function would return
immediately without waiting for the full timeout period. This created a race condition
where unexpected messages appearing after the expected messages (but before the timeout)
would not be detected

5efca62f3d

DrahtBot added the label Tests on Jan 28, 2026

DrahtBot commented at 2:28 PM on January 28, 2026: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34431.

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

bitcoin deleted a comment on Jan 28, 2026

in test/functional/test_framework/test_node.py:579 in 5efca62f3d

 575 | @@ -576,8 +576,9 @@ def join_log(log):
 576 |              while remaining_expected and remaining_expected[-1] in log:
 577 |                  remaining_expected.pop()
 578 |              if not remaining_expected:
 579 | -                return
 580 | -            if time.time() >= time_end:
 581 | +                if time.time() >= time_end:

maflcko commented at 3:09 PM on January 28, 2026:

not sure about this. assert_debug_log should not be used with a timeout value at all, because that makes tests hard to read and understand.

Putting more features behind the timeout here seems backward.

Also, it seems brittle, because the timeout can be scaled by the test runner, so a failing test on one machine may pass on the next machine when the timeout is decreased, which seems backward.

Also, scaling the timeout up will then needlessly block the test

brunoerg commented at 4:17 PM on January 28, 2026:

Yes, was wondering about the same, not trusting the timeout is better and would avoid any chance of having intermittent issues as well. My first idea was something like:

         self.log.info("Check node will not attempt to connect to anchors from unreachable networks")
-        with self.nodes[0].assert_debug_log(timeout=60, expected_msgs=["Loaded 1 addresses from \"anchors.dat\""],
-            unexpected_msgs=[f"Trying to make an anchor connection to {ONION_ADDR}"]):
-            self.start_node(0)
+        assertion_raised = False
+        try:
+            with self.nodes[0].assert_debug_log([f"Trying to make an anchor connection to {ONION_ADDR}"]):
+                self.start_node(0)
+        except AssertionError:
+            assertion_raised = True
+        assert_equal(assertion_raised, True)

What do you think?

maflcko commented at 4:22 PM on January 28, 2026:

I think the same issues exist here as well:

Also, it seems brittle, because the timeout can be scaled by the test runner, so a failing test on one machine may pass on the next machine when the timeout is decreased, which seems backward.

Also, scaling the timeout up will then needlessly block the test

brunoerg commented at 4:34 PM on January 28, 2026:

Increasing or decreasing the timeout here would not be a huge problem here because we're expecting this log to never appear, so the only chance of having an issue here would be if someone tries to test it against the provided mutant. Even in this case, I think it would hard to have an issue because this is one of the first actions when starting a node.

Ensuring it's not appearing may have less issues then trying to assert it?

maflcko commented at 4:49 PM on January 28, 2026:

Why not. If someone sets the timeout factor to 999, how long will the test take and do nothing all the time?

brunoerg commented at 4:56 PM on January 28, 2026:

Fair enough. Going to close it, since there is no other way to test it.

maflcko commented at 5:07 PM on January 28, 2026:

I am not familiar with the internal code, but in theory the idea is that you do any action which only can happen after:

code on master:

        self.log.info("Check node will not attempt to connect to anchors from unreachable networks")
        with self.nodes[0].assert_debug_log(timeout=60, expected_msgs=["Loaded 1 addresses from \"anchors.dat\""],
            unexpected_msgs=[f"Trying to make an anchor connection to {ONION_ADDR}"]):
            self.start_node(0)
            # any action that happens after trying to make anchor connections
            # maybe adding a real connection and waiting for it to be established?

brunoerg closed this on Jan 28, 2026

brunoerg deleted the branch on Jan 28, 2026