Page MenuHomePhabricator

Add test-before-evict discipline to addrman
ClosedPublic

Authored by deadalnix on Oct 2 2018, 12:24.

Details

Summary

Changes addrman to use the test-before-evict discipline in which an
address is to be evicted from the tried table is first tested and if
it is still online it is not evicted.

Adds tests to provide test coverage for this change.

This change was suggested as Countermeasure 3 in
Eclipse Attacks on Bitcoin’s Peer-to-Peer Network, Ethan Heilman,
Alison Kendler, Aviv Zohar, Sharon Goldberg. ePrint Archive Report
2015/263. March 2015.

This is a backport of Core PR9037

Depends on D1861

Test Plan
make check

Diff Detail

Repository
rABC Bitcoin ABC
Branch
addtestevict
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 3480
Build 5036: Bitcoin ABC Buildbot (legacy)
Build 5035: arc lint + arc unit

Event Timeline

jasonbcox requested changes to this revision.Oct 5 2018, 06:18
jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
src/addrman.cpp
640 ↗(On Diff #5275)

Use of count() against a particular number appears more error-prone than find(), especially if the underlying implementation changes container types.

Secondly, according to this: https://stackoverflow.com/questions/25490357/checking-for-existence-in-stdmap-count-vs-find find() may possibly be more performant.

664 ↗(On Diff #5275)

Considering these are the only places ADDRMAN_REPLACEMENT_HOURS is used, the * (60 * 60) should be applied at the time of definition. Improves readability and reduces chance of future typos.

Probably best to change the name to ADDRMAN_REPLACEMENT_SECONDS as well. Update comments accordingly.

668 ↗(On Diff #5275)

Calling GetAdjustedTime() multiple times across this code block/group gives a bad code smell. This is how heisenbugs happen ;) I recommend an intermediate variable holding the value of GetAdjustedTime().

704 ↗(On Diff #5275)

Same here regarding count() vs find()

This revision now requires changes to proceed.Oct 5 2018, 06:18

Change constant to use seconds

This revision is now accepted and ready to land.Oct 7 2018, 16:51
This revision was automatically updated to reflect the committed changes.