Details

Reviewers

Group Reviewers

Restricted Project

Commits

rSTAGING6d6ceea734a2: Add test-before-evict discipline to addrman
rABC6d6ceea734a2: Add test-before-evict discipline to addrman

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 3532
Build 5140: Bitcoin ABC Buildbot (legacy)
Build 5139: arc lint + arc unit

Event Timeline

deadalnix created this revision.Oct 2 2018, 12:24

Herald added a reviewer: Restricted Project. · View Herald TranscriptOct 2 2018, 12:24

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B3480: Diff 5226.Oct 2 2018, 12:42

rebase

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B3512: Diff 5275.Oct 4 2018, 21:49

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()