#36208 addrman: apply time penalty in nTime check
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #36208 addrman: apply time penalty in nTime check | Ready | 1 | Mild | 2 + 13 tests | |||
|
|
|
|
| |||
Reviewability: ReadyStalePaused Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs. Size: lines added or modified outside tests, then in tests; greener = smaller. | |||||||
Goal
Subtracts the applicable time penalty from an address's timestamp when checking whether an incoming address announcement contains new information in AddrMan. This prevents identical announcements relayed across multiple peers from mistakenly passing the timestamp freshness check and increasing the address's multiplicity in the "new" table.
Problem: When an address entry is first stored in AddrMan, a time penalty (typically 2 hours) is subtracted from its timestamp. When the identical announcement arrives from other gossip relay peers, AddrMan compares the raw announcement timestamp against the stored penalized timestamp without adjusting for the penalty, causing the check to falsely conclude that newer information arrived and inflating the address's multiplicity up to 8.
Evidence of importance:
- PR description: "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."
- Author clarifies limitation in PR description: "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."
Reviewability
Ready
Nothing known that would invalidate a review now.
Waiting on (from the thread read):
- reviewer: feedback on whether to remove the 1h update interval 2026-10-26
- author: push fix for the nTime refresh / bucket addition bug and address test suggestion 2026-10-26
Agreement
Mild
objection that needs a change was answered, no reply from the objector (taki-abedesselam: The new check runs after the nTime refresh, causing genuinely newer announcements to be blocked whenever the refresh fir; fametrano: A relayed announcement more than an hour newer than the previous one can no longer add a bucket, keeping nodes that anno)
Thread notes: mzumsande acknowledged the bug raised by taki-abedesselam and fametrano, promising to fix it in the next push, answered danielabrozzoni's documentation suggestion by explaining historical context, and asked reviewers for feedback on removing the 1h update interval.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| taki-abedesselam (none) | objection, correctness | The new check runs after the nTime refresh, causing genuinely newer announcements to be blocked whenever the refresh fires. | answered · author agreed to fix with next push, discussed removing the 1h interval | changed | yes | 2026-10-14: '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.nTi' Status from: 2026-10-26: mzumsande: 'Good point! I will fix this with the next push (how depends on the discussion below).' |
| danielabrozzoni (member) | suggestion, maintenance | Clarify and document the intended behavior around updating timestamps versus increasing multiplicity in AddSingle. | answered · author explained historical context and suggested removing the 1h update interval instead | answered | yes | 2026-10-16: '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' Status from: 2026-10-26: mzumsande: '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`.' |
| frankomosh (contributor) | suggestion, safety | Add a test checking that a relayed newer announcement can still add buckets / bump multiplicity. | open | answered | no | 2026-10-23: 'Besides checking that relayed duplicates don’t add buckets, maybe we could also check that a relayed newer announcement still can.' |
| fametrano (none) | objection, correctness | A relayed announcement more than an hour newer than the previous one can no longer add a bucket, keeping nodes that announce daily at multiplicity 1. | answered · author agreed to fix with next push, discussed removing the 1h interval | changed | yes | 2026-10-25: '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' Status from: 2026-10-26: mzumsande: 'Good point! I will fix this with the next push (how depends on the discussion below).' |
Support:
- stratospher (member) (ACK, 2026-10-01) [read]: onion/i2p-only nodes will have slightly fewer addresses with inflated nRefCount
- frankomosh (contributor) (Concept ACK, 2026-10-22) [tested]: reproduced the issue, and without the fix, the new test gets multiplicity 3 instead of 1
- fametrano (none) (Concept ACK, 2026-10-25): (no reason given) [not substantive]
Participants:
- stratospher (member, support, 1 statements 2026-10-01 to 2026-10-01): ACKed, noted it reduces unwarranted nRefCount increases on onion/i2p-only nodes
- taki-abedesselam (none, objection, 1 statements 2026-10-14 to 2026-10-14): pointed out a bug where newer announcements are blocked when an nTime refresh fires
- danielabrozzoni (member, question, 1 statements 2026-10-16 to 2026-10-16): asked for clarity and documentation on the interaction between timestamp updates and multiplicity increases
- frankomosh (contributor, support, 2 statements 2026-10-22 to 2026-10-23): Concept ACKed, verified multiplicity behavior, and suggested an additional test case for newer announcements
- fametrano (none, objection, 1 statements 2026-10-25 to 2026-10-25): Concept ACKed but objected that relayed announcements >1h newer cannot add buckets, confirming taki-abedesselam's bug
Files
14 lines under test/bench/ci.
- src/test/addrman_tests.cpp +13/-1
- src/addrman.cpp +2/-1
Card
The short text the ranking pass compares across PRs.
This PR subtracts the gossip time penalty from an incoming address's timestamp when AddrMan evaluates whether the announcement contains newer information than what is already stored. Currently, because stored entries have a 2-hour penalty applied on insertion, comparing unpenalized incoming timestamps against stored penalized timestamps causes identical gossip announcements from different peers to appear newer than the existing record. This circumvents the intended check and allows a single broadcast to artificially increase an address's multiplicity in the new table up to 8. The author notes this is a logic bugfix for benign multi-path relay rather than an adversarial defense, as an attacker could still intentionally send increments to nTime.
Data
thread read through 2026-10-26 (7 statements), head 5f58a43fd7
labels: P2P
Categories
P2P
P3 · bug fix (ranking pass: Grouped with the addrman bookkeeping fixes it interacts with.)
Fixes a logic bug where AddrMan's timestamp check fails to account for the initial 2-hour penalty, unintentionally inflating an address's multiplicity in the "new" table up to 8 on identical relayed announcements. As the author notes, this is a minor non-adversarial bug fix that does not alter resilience against targeted attacks.
Membership: Labeled P2P and modifies address management logic in src/addrman.cpp.
Factors: security/stability 1, bug 1, performance 0, user value 0, leverage 0