addrman: apply time penalty in nTime check #36208

mzumsande commented at 3:23 PM on September 9, 2026: contributor

There is an existing check in AddSingle() that prevents us from increasing the RefCount of an address when the same announcement is received again from a different peer, which regularly happens with gossip relay - an addr can take different routes through the network to us.

However, the check is ineffective: when the entry was first added, its nTime was stored with a 2h time penalty applied, so addr.nTime > pinfo->nTime holds even for an identical announcement. As a result, a single announcement relayed to us by several peers could increase an address's multiplicity in the new table, up to 8.

Fix this by applying the penalty in the comparison too, matching the nTime update check a few lines above.

This is just a small logic bugfix, it doesn't change things in adversarial scenarios, since peers can just do multiple announcements with changed nTime if they want to boost their RefCount artificially.

addrman: apply time penalty in nTime check

When receiving a single self-announcement of a node from
multiple peers (via gossip relay), we don't want to
increase the multiplicity of that address.
Therefore, we need to take into account the time penalty
of the check.

5f58a43fd7

DrahtBot added the label P2P on Sep 9, 2026

DrahtBot commented at 3:23 PM on September 9, 2026: contributor

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

External sites

Code coverage & benchmarks: https://corecheck.dev/bitcoin/bitcoin/pulls/36208
Ralph review <img src=https://review.fish.foo/assets/bitcoin/bitcoin/36208.png alt="Ralph review status"> https://git.fish.foo/bitcoin/bitcoin/pulls/36208#author-ralph

Reviews

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

Type	Reviewers
ACK	stratospher
Concept ACK	frankomosh, fametrano

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

stratospher approved

stratospher commented at 8:53 PM on October 1, 2026: contributor

ACK 5f58a43f. nice find!

even though this might not affect clearnet nodes that much because of the high clearnet address gossip volume, I think onion/i2p-only nodes would have slightly less addresses with higher nRefCount after this, which is a good thing! (haven't tried it out)

here's how current nRefCount distribution looks like for an onion/i2p only node: <img width="1113" height="524" alt="Image" src="https://github.com/user-attachments/assets/afff9dd5-c7d6-4e33-9802-62dcaa5af3f2" />

taki-abedesselam commented at 7:54 PM on October 14, 2026: none

Nice catch. One subtlety: the new check runs after the nTime refresh above it, which sets pinfo->nTime = addr.nTime - time_penalty. So whenever that refresh fires, the check becomes addr.nTime - time_penalty <= addr.nTime - time_penalty → always true → return false, blocking genuinely newer announcements too, not just re-relayed duplicates. (The existing test misses it because its +1s steps never trigger the refresh.)

Comparing against the pre-refresh value fixes it, while still blocking duplicates:

const NodeSeconds prev_time{pinfo->nTime};
// ... existing refresh of pinfo->nTime ...
if (addr.nTime - time_penalty <= prev_time) {
    return false;
}

danielabrozzoni commented at 1:51 AM on October 16, 2026: member

I would like to have more clarity around when we want to attempt increasing an address' multiplicity vs when we want to simply update its timestamp. I think it would be nice to at least have some documentation around this, and the current "do not update if no new information is present" comment is not very explicative :)

For example, consider AddSingle being called with the following values:

time_penalty = 2 hours
the address is already present in our addrman, and pinfo->nTime is "today at 14:30"
addr.nTime is "today at 17:00"
update_interval = 1 hour (because timestamps are recent, let's say it's "today at 18:00" right now)

This check:

if (pinfo->nTime < addr.nTime - update_interval - time_penalty)

would return false ("today at 14:30" is bigger than "today at 17:00 minus 3h" ("today at 14:00")), so we would not update the timestamp in our addrman.

However, this check is also false:

if (addr.nTime - time_penalty <= pinfo->nTime) {

("today at 17:00 minus 2h" is bigger than "today at 14:30"). So we would not return from the function, and go on and attempt increasing the multiplicity of the address in the addrman.

frankomosh commented at 6:55 PM on October 22, 2026: contributor

Concept ACK. I reproduced the issue, and without the fix, the new test gets multiplicity 3 instead of 1.

in src/test/addrman_tests.cpp:406 in 5f58a43fd7

 401 | +    for (unsigned int i = 1; i < 20; ++i) {
 402 | +        CNetAddr source{ResolveIP(ToString(i) + ".2.2.2")};
 403 | +        addrman->Add({addr_penalty}, source, /*time_penalty=*/2h);
 404 | +    }
 405 | +    AddressPosition addr_pos_penalty = addrman->FindAddressEntry(addr_penalty).value();
 406 | +    BOOST_CHECK_EQUAL(addr_pos_penalty.multiplicity, 1U);

frankomosh commented at 11:24 AM on October 23, 2026:

Besides checking that relayed duplicates don’t add buckets, maybe we could also check that a relayed newer announcement still can. With this mutant, which blocks every relayed bump:

-        if (addr.nTime - time_penalty <= pinfo->nTime) {
+        if (time_penalty > 0s || addr.nTime - time_penalty <= pinfo->nTime) {

the unit and functional tests would still pass.

fametrano commented at 11:39 AM on October 25, 2026: none

Concept ACK

One issue should be fixed before merge: on 5f58a43fd7, a relayed announcement more than an hour newer than the previous one can no longer add a bucket, so a node that announces itself once a day stays at multiplicity 1. This confirms the point taki-abedesselam made above.

I measured it with addrman unit tests run on head, on its base 59fd053dac, and with the prev_time comparison. In one test an address announces itself once a day for 30 days, and nine peers relay each announcement with the 2h penalty. Its multiplicity ends at 1 on head, 7 on base and 5 with prev_time. A second test varies how much newer a relayed second announcement is. Head tries a bucket only when it is 1s to 1h newer. Base tries one at every gap measured, identical and older included, which is the bug this PR fixes. prev_time tries one whenever it is newer, and never when it is identical. With prev_time, the PR's test and the daily test both pass, and the daily test fails on frankomosh's mutant, so it would close that gap too.

One side effect to weigh: with prev_time, a node's own announcement, which has no penalty, also tries a bucket when it is more than an hour newer. Neither base nor head does that. I can share both tests as a patch.

mzumsande commented at 1:02 PM on October 26, 2026: contributor

Nice catch. One subtlety: the new check runs after the nTime refresh above it, which sets pinfo->nTime = addr.nTime - time_penalty. So whenever that refresh fires, the check becomes addr.nTime - time_penalty <= addr.nTime - time_penalty → always true → return false, blocking genuinely newer announcements too, not just re-relayed duplicates. (The existing test misses it because its +1s steps never trigger the refresh.)

Good point! I will fix this with the next push (how depends on the discussion below).

I would like to have more clarity around when we want to attempt increasing an address' multiplicity vs when we want to simply update its timestamp. I think it would be nice to at least have some documentation around this, and the current "do not update if no new information is present" comment is not very explicative :)

I think this describes a real inconsistency also present in master: We may update the RefCount for an address, even though we didn't update nTime.

As far as I understand it, it's due to historical reasons: In fba4088941, Satoshi introduced the update interval: int64 nUpdateInterval = (fCurrentlyOnline ? 60 * 60 : 24 * 60 * 60); Back then, it made sense because changes were written to a DB on disk, and no AddrMan existed, so there was a cost attached to frequent updates. When @sipa introduced AddrMan later, it was kept, and never changed since. However, it wasn't applied to the newly introduced RefCount multiplicity rules. I think the way to think about that is that if an address is rumoured frequently, it is more likely to belong to real node, so it deserves a higher RefCount. However, a single announcement received via different paths should not lead to an increase.

As for the nTime 1h rule, I'm not really sure it makes any sense today: AddrMan lives in memory now so updates are cheap, plus we don't really "leak" the timestamp to the outside, because 1) GETADDR answers are cached for 24h, and 2) for gossip relay, the AddrMan timestamp is not important.

So I am tempted to just remove the 1h updated interval to simplify the code, and instead update every time new info is present (as we already do with the RefCount). What do you think?