Page MenuHomePhabricator

[net] De-duplicate connection eviction logic
ClosedPublic

Authored by deadalnix on Oct 4 2018, 11:36.

Details

Summary

This is a backport of core PR11524

Test Plan
make check

Diff Detail

Repository
rABC Bitcoin ABC
Branch
evictionrefac
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 3510
Build 5096: Bitcoin ABC Buildbot (legacy)
Build 5095: arc lint + arc unit

Event Timeline

jasonbcox requested changes to this revision.Oct 4 2018, 20:51
jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
src/net.cpp
1074 ↗(On Diff #5267)

This is not entirely true. DoS attacks on these nodes could reduce their ping time as well. Not sure if it's worth updating the comment, but it should be noted.

1077 ↗(On Diff #5267)

Can't an attacker send the node brand new transactions to manipulate this metric? Generating these transactions is essentially free.

1104 ↗(On Diff #5267)

Seems odd to remove the intermediate value for group.size(). Please fix this.

This revision now requires changes to proceed.Oct 4 2018, 20:51
deadalnix added inline comments.
src/net.cpp
1074 ↗(On Diff #5267)

You'd have to know which nodes to DoS to begin with, or DoS the whole network. Both option seems highly unlikely.

1077 ↗(On Diff #5267)

Yes. But it gets rid of all the peer that do not do any work and just leach connections. If these txns are going to be propagated anyways, this is useful for you to recieve them :)

1104 ↗(On Diff #5267)

ok

resotre group size local variable

This revision is now accepted and ready to land.Oct 5 2018, 05:30
This revision was automatically updated to reflect the committed changes.