test: addrman: test self-announcement time penalty handling #34303

brunoerg commented at 12:07 PM on January 15, 2026: contributor

This PR adds a test case for addrman that verifies that addresses announcing themselves (addr == source) are exempt from time penalties, while addresses announced by others receive the expected penalty.

It fixes the following mutant (https://corecheck.dev/mutation/src/addrman.cpp#L561):

diff --git a/src/addrman.cpp b/src/addrman.cpp
index 206b54118e..c6a045fd8d 100644
--- a/src/addrman.cpp
+++ b/src/addrman.cpp
@@ -558,7 +558,7 @@ bool AddrManImpl::AddSingle(const CAddress& addr, const CNetAddr& source, std::c
     AddrInfo* pinfo = Find(addr, &nId);

     // Do not set a penalty for a source's self-announcement
-    if (addr == source) {
+    if (addr != source) {
         time_penalty = 0s;
     }

DrahtBot added the label Tests on Jan 15, 2026

DrahtBot commented at 12:07 PM on January 15, 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/34303.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	maflcko, fjahr, naiyoma, achow101
Concept ACK	w0xlt
Stale ACK	stratospher

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

fjahr commented at 12:09 PM on January 15, 2026: contributor

Concept ACK

w0xlt commented at 12:43 AM on January 16, 2026: contributor

Concept ACK

in src/test/addrman_tests.cpp:146 in 1a2dbd8338

 141 | +    addresses = addrman->GetAddr(/*max_addresses=*/0, /*max_pct=*/0, /*network=*/std::nullopt);
 142 | +    BOOST_REQUIRE_EQUAL(addresses.size(), 2U);
 143 | +
 144 | +    CAddress retrieved_addr2{addresses[0]};
 145 | +    BOOST_CHECK(retrieved_addr2.nTime < base_time);
 146 | +    BOOST_CHECK(retrieved_addr2.nTime <= base_time - time_penalty);

maflcko commented at 2:10 PM on January 16, 2026:

Could those use exact checks, when using SetMockTime to pin the time?

brunoerg commented at 4:24 PM on January 16, 2026:

Yes, just pushed addressing it.

brunoerg force-pushed on Jan 16, 2026

stratospher commented at 12:22 PM on January 19, 2026: contributor

ACK f86cf27.

DrahtBot requested review from fjahr on Jan 19, 2026

in src/test/addrman_tests.cpp:112 in f86cf27999

 106 | @@ -106,6 +107,46 @@ BOOST_AUTO_TEST_CASE(addrman_simple)
 107 |      BOOST_CHECK(addrman->Size() >= 1);
 108 |  }
 109 |  
 110 | +BOOST_AUTO_TEST_CASE(addrman_penalty_self_announcement)
 111 | +{
 112 | +    SetMockTime(12345678);

maflcko commented at 12:42 PM on January 19, 2026:

    SetMockTime(Now<NodeSeconds>());

nit: Could remove the hard-coded constant here, and the hard-coded check below?

    BOOST_CHECK(base_time.time_since_epoch() == 12335678s);

Which seems like a self-test of the chrono utils?

brunoerg commented at 12:51 PM on January 27, 2026:

Yes, better to remove. Just pushed addressing it.

maflcko approved

maflcko commented at 12:42 PM on January 19, 2026: member

left another nit. Feel free to ignore.

test: addrman: test self-announcement time penalty handling

Verify that addresses announcing themselves (addr == source) are exempt
from time penalties, while addresses announced by others receive the
expected penalty.

e770392084

brunoerg force-pushed on Jan 27, 2026

brunoerg commented at 12:52 PM on January 27, 2026: contributor

Sorry for the delay. Force-pushed addressing #34303 (review).

in src/test/addrman_tests.cpp:129 in e770392084

 124 | +    BOOST_CHECK(addrman->Add({caddr1}, source1, time_penalty));
 125 | +
 126 | +    auto addr_pos1{addrman->FindAddressEntry(caddr1)};
 127 | +    BOOST_REQUIRE(addr_pos1.has_value());
 128 | +
 129 | +    std::vector<CAddress> addresses{addrman->GetAddr(/*max_addresses=*/0, /*max_pct=*/0, /*network=*/std::nullopt)};

fjahr commented at 1:31 PM on January 27, 2026:

nit: There isn't really a need for two GetAddr calls, right? We could add both of the addresses and then do one call to get the addresses and do checks on both of them?

maflcko commented at 2:08 PM on January 27, 2026: member

review ACK e770392084aa52e5568cf001da4d537fda1d71b3 🐤

<details><summary>Show signature</summary>

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK e770392084aa52e5568cf001da4d537fda1d71b3 🐤
qFZJ7zWCOrFuGBXQVr3HX1e+D8NSGoZZwYbN5qF+9d4N0MMVfwjx7RQs9K5UWevPKEcx7gLdxnV72atssrqGDw==

</details>

DrahtBot requested review from stratospher on Jan 27, 2026

in src/test/addrman_tests.cpp:145 in e770392084

 140 | +    BOOST_CHECK(addrman->Add({caddr2}, source2, time_penalty));
 141 | +
 142 | +    addresses = addrman->GetAddr(/*max_addresses=*/0, /*max_pct=*/0, /*network=*/std::nullopt);
 143 | +    BOOST_REQUIRE_EQUAL(addresses.size(), 2U);
 144 | +
 145 | +    CAddress retrieved_addr2{addresses[0]};

fjahr commented at 4:48 PM on January 27, 2026:

nit: I was kind of surprised that this works every time. I didn't dive in fully but I guess the vRandom order just works out magically when there are only two addresses and the randomness is deterministic when running the tests? Might be worth a comment.

naiyoma commented at 2:02 PM on January 28, 2026:

I think this is happening because the test uses DETERMINISTIC = true : insecure_rand is initialized with a fixed seed and nKey is set to a constant, so we get the same random state every run.

with !DETERMINISTIC, this would fail intermittently BOOST_CHECK(retrieved_addr2.nTime == base_time - time_penalty);, since addresses[0] would sometimes be the self-announced addr.

./test/addrman_tests.cpp(146): error: in "addrman_tests/addrman_penalty_self_announcement": check retrieved_addr2.nTime == base_time - time_penalty has failed

+1 on adding a comment

brunoerg commented at 5:23 PM on January 28, 2026:

Yes, it works due to DETERMINISTIC = true which sets nKey to 1, as @naiyoma explained. I'm just going to leave as-is to not invalidate the ACKs.

fjahr commented at 4:51 PM on January 27, 2026: contributor

Code review ACK e770392084aa52e5568cf001da4d537fda1d71b3

naiyoma commented at 5:16 PM on January 28, 2026: contributor

tACK e770392084aa52e5568cf001da4d537fda1d71b3 I have compiled and tested this locally with: build/bin/test_bitcoin --run_test=addrman_tests/addrman_penalty_self_announcement test passed as expected

achow101 commented at 10:23 PM on January 28, 2026: member

ACK e770392084aa52e5568cf001da4d537fda1d71b3

achow101 merged this on Jan 28, 2026

achow101 closed this on Jan 28, 2026

brunoerg deleted the branch on Jan 29, 2026

salasjuarezederaldair60-eng commented at 8:27 AM on January 29, 2026: none

veerificar