test: non-inbound version message promotes address to tried table #36279

naiyoma commented at 8:32 PM on September 16, 2026: contributor

A VERSION message from a non-inbound peer calls AddrMan::Good(), which moves the address from the new table to the tried table. This was previously untested.

This PR kills the mutant at https://bitcoincore.space/src/net_processing.cpp#3475

DrahtBot added the label Tests on Sep 16, 2026

DrahtBot commented at 8:33 PM on September 16, 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/36279.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
ACK	brunoerg

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

naiyoma renamed this:
~~test: only non-inbound version message promotes address to tried table~~
test: non-inbound version message promotes address to tried table
on Sep 16, 2026

in src/test/net_tests.cpp:1667 in 14ed2012da

1662 | +    const CNetAddr source{LookupHost("2.3.4.5", /*fAllowLookup=*/false).value()};
1663 | +    const CAddress addr_outbound{Lookup("5.6.7.8", 8333, /*fAllowLookup=*/false).value(), NODE_NONE};
1664 | +    const CAddress addr_inbound{Lookup("6.7.8.9", 8333, /*fAllowLookup=*/false).value(), NODE_NONE};
1665 | +    // Good() returns false for an address it does not already know, so the entries must
1666 | +    // first exist before the handshake for the promotion to happen.
1667 | +    BOOST_REQUIRE(m_node.addrman->Add({addr_outbound, addr_inbound}, source));

brunoerg commented at 12:33 PM on September 18, 2026:

I think that adding both addresses with the same source is flaky? Afaik, the addrman in this test is not deterministic, so nKey is random per run. Easiest fix I see is to add the addresses one at a time, after the previous one has left the new table.

brunoerg commented at 12:48 PM on September 18, 2026:

Suggestion (from LLM):

diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp
index 73bb04dc73..4dc73984b2 100644
--- a/src/test/net_tests.cpp
+++ b/src/test/net_tests.cpp
@@ -1662,10 +1662,13 @@ BOOST_AUTO_TEST_CASE(only_non_inbound_version_message_promotes_addr_to_tried)
     const CNetAddr source{LookupHost("2.3.4.5", /*fAllowLookup=*/false).value()};
     const CAddress addr_outbound{Lookup("5.6.7.8", 8333, /*fAllowLookup=*/false).value(), NODE_NONE};
     const CAddress addr_inbound{Lookup("6.7.8.9", 8333, /*fAllowLookup=*/false).value(), NODE_NONE};
-    // Good() returns false for an address it does not already know, so the entries must
-    // first exist before the handshake for the promotion to happen.
-    BOOST_REQUIRE(m_node.addrman->Add({addr_outbound, addr_inbound}, source));
-    BOOST_REQUIRE_EQUAL(m_node.addrman->Size(/*net=*/std::nullopt, /*in_new=*/true), 2U);
+    // Good() returns false for an address it does not already know, so each entry must
+    // first exist before its handshake for the promotion to happen. The addresses are
+    // added one at a time: the fixture's addrman is not deterministic, and adding both
+    // to the new table at once could (rarely) place them in the same bucket position,
+    // evicting the first.
+    BOOST_REQUIRE(m_node.addrman->Add({addr_outbound}, source));
+    BOOST_REQUIRE_EQUAL(m_node.addrman->Size(/*net=*/std::nullopt, /*in_new=*/true), 1U);
     BOOST_REQUIRE_EQUAL(m_node.addrman->Size(/*net=*/std::nullopt, /*in_new=*/false), 0U);
 
     auto& connman = static_cast<ConnmanTestMsg&>(*m_node.connman);
@@ -1691,10 +1694,13 @@ BOOST_AUTO_TEST_CASE(only_non_inbound_version_message_promotes_addr_to_tried)
 
     BOOST_REQUIRE(!node_outbound.fDisconnect);
     BOOST_CHECK_EQUAL(m_node.addrman->Size(/*net=*/std::nullopt, /*in_new=*/false), 1U);
-    BOOST_CHECK_EQUAL(m_node.addrman->Size(/*net=*/std::nullopt, /*in_new=*/true), 1U);
+    BOOST_CHECK_EQUAL(m_node.addrman->Size(/*net=*/std::nullopt, /*in_new=*/true), 0U);
+    BOOST_CHECK(m_node.addrman->FindAddressEntry(addr_outbound).value().tried);
     m_node.peerman->FinalizeNode(node_outbound);
 
     // An inbound peer's address stays in the new table.
+    BOOST_REQUIRE(m_node.addrman->Add({addr_inbound}, source));
+    BOOST_REQUIRE_EQUAL(m_node.addrman->Size(/*net=*/std::nullopt, /*in_new=*/true), 1U);
     CNode node_inbound{/*id=*/1,
                        /*sock=*/nullptr,
                        /*addrIn=*/addr_inbound,
@@ -1715,6 +1721,7 @@ BOOST_AUTO_TEST_CASE(only_non_inbound_version_message_promotes_addr_to_tried)
     BOOST_REQUIRE(!node_inbound.fDisconnect);
     BOOST_CHECK_EQUAL(m_node.addrman->Size(/*net=*/std::nullopt, /*in_new=*/false), 1U);
     BOOST_CHECK_EQUAL(m_node.addrman->Size(/*net=*/std::nullopt, /*in_new=*/true), 1U);
+    BOOST_CHECK(!m_node.addrman->FindAddressEntry(addr_inbound).value().tried);

naiyoma commented at 2:28 PM on September 29, 2026:

Makes sense, also removed some duplicate checks, thanks

naiyoma force-pushed on Sep 29, 2026

test: only non-inbound version message promotes address to tried table

A VERSION message from a non-inbound peer calls AddrMan::Good(),
moving the peer's address from the new table to the tried table.
Inbound peers are excluded.

Co-authored-by: Bruno Garcia <brunoely.gc@gmail.com>

3b88ebaea4

naiyoma force-pushed on Sep 29, 2026

DrahtBot added the label CI failed on Sep 29, 2026

DrahtBot removed the label CI failed on Sep 29, 2026

brunoerg commented at 4:11 PM on September 29, 2026: contributor

ACK 3b88ebaea4935a305d5e4c2cf158f75ea9118359