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

Branch

avalanche_protect_avanodes

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 16288
Build 32443: Build Diff	build-without-wallet · build-diff · build-clang-tidy · build-debug · build-clang · lint-circular-dependencies
Build 32442: arc lint + arc unit

Event Timeline

Fabien created this revision.Aug 9 2021, 12:49

Herald added a reviewer: Restricted Project. · View Herald TranscriptAug 9 2021, 12:49

Fabien requested review of this revision.Aug 9 2021, 12:49

Harbormaster completed remote builds in B16287: Diff 29355.Aug 9 2021, 13:00

Fabien planned changes to this revision.Aug 9 2021, 14:01

Remove debug string in test

Harbormaster completed remote builds in B16288: Diff 29356.Aug 9 2021, 14:16

Fabien added a child revision: D9858: [avalanche] Protect from eviction the best node for each peer.Aug 9 2021, 14:58

Rebase

Harbormaster completed remote builds in B16294: Diff 29368.Aug 10 2021, 07:05

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.

Harbormaster completed remote builds in B16298: Diff 29373.Aug 10 2021, 14:19

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

Fabien edited the summary of this revision. (Show Details)Aug 10 2021, 18:28

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

Harbormaster completed remote builds in B16301: Diff 29377.Aug 10 2021, 19:40

deadalnix accepted this revision.Aug 10 2021, 20:36

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

Fabien added inline comments.Aug 11 2021, 06:58

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.