Page MenuHomePhabricator

[avalanche] Protect nodes with high availability score from eviction
ClosedPublic

Authored by Fabien on Aug 9 2021, 12:49.

Details

Reviewers
deadalnix
Group Reviewers
Restricted Project
Maniphest Tasks
Restricted Maniphest Task
Restricted Maniphest Task
Commits
rABC27787de5399e: [avalanche] Protect nodes with high availability score from eviction
Summary

This protects up to 128 nodes that participate to the avalanche network from being evicted.
This is complementary to the main protection mechanism (to be developped) that will protect the best node for each peer.

Ref T1634.

Test Plan
ninja all check-all

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Fabien requested review of this revision.Aug 9 2021, 12:49
Fabien planned changes to this revision.Aug 9 2021, 14:01

Remove debug string in test

deadalnix requested changes to this revision.Aug 10 2021, 12:38
deadalnix added a subscriber: deadalnix.

I haven't reviewed everything, but it is clear that this is mixing refactoring with actual functionality. Please extract the refactoring in their own diff.

src/net.cpp
959 ↗(On Diff #29368)

There is no reason to force the creation of an std::function here. this is already a template.

996 ↗(On Diff #29368)

16 seems really really low here.

This revision now requires changes to proceed.Aug 10 2021, 12:38

Rebase after D9863.
Bump the max quantity of protected nodes to 128. This is actually up to 1/32 of all the connected nodes.

deadalnix requested changes to this revision.Aug 10 2021, 18:13

The approach looks reasonable overall, but I don't think the avalanche book is very necessary.

src/net.cpp
1105 ↗(On Diff #29373)

Just do

availabilityScore = node->m_avalanche_state
    ? node->m_avalanche_state->getAvailabilityScore()
    : -std::numeric_limits<double>::infinity();

Something like that. no need to keep that boolean around.

This revision now requires changes to proceed.Aug 10 2021, 18:13

Remove the boolean and use std::isinf() to filter which nodes to protect instead.

deadalnix added inline comments.
src/net.cpp
999 ↗(On Diff #29377)

Maybe having a thresold is enough? Like n.availabilityScore > 0 ? Do we really want to protect trully aweful peers?

This revision is now accepted and ready to land.Aug 10 2021, 20:36
src/net.cpp
999 ↗(On Diff #29377)

I planned to do something similar as a replacement for D9862. For now a score of 0 either means that the node was never selected or the score was not updated yet, and you want to protect only for the latter case. So my idea was to apply a penalty to the nodes that were never selected so that their score will be negative, and filter using availabilityScore >= 0; I can use the same filter here.

Filter on nodes with score >= 0 rather than != inf

Only protect nodes with score > 0 (and not >= 0) after D9867 is abandonned

Mengerian added a task: Restricted Maniphest Task.Tue, Sep 14, 18:24