Paths

Table of Contentst

Differential D9858

[avalanche] Protect from eviction the best node for each peer
AbandonedPublic
Actions

Authored by Fabien on Aug 9 2021, 14:58.

Details

Reviewers

deadalnix

Group Reviewers

Restricted Project

Maniphest Tasks

Restricted Maniphest Task

Summary

For each avalanche peer, protect the node with the best availability score from eviction.
This occurs before we protect the 16 best nodes (independently of the peer).

Note that there is no filter on the avalanche peers to consider in this diff. A threshold on the proof score can be added later but is outside the scope of this diff.

Depends on D9853 and D9857.

Ref T1855.

Test Plan

ninja all check-all

Diff Detail

Repository

rABC Bitcoin ABC

Branch

avalanche_protect_best_node_each_peer

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 16311
Build 32487: Build Diff	lint-circular-dependencies · build-without-wallet · build-diff · build-debug · build-clang-tidy · build-clang
Build 32486: arc lint + arc unit

Event Timeline

Fabien created this revision.Aug 9 2021, 14:58

Herald added a reviewer: Restricted Project. · View Herald TranscriptAug 9 2021, 14:58

Fabien requested review of this revision.Aug 9 2021, 14:58

Harbormaster completed remote builds in B16289: Diff 29357.Aug 9 2021, 15:12

Rebase

Harbormaster completed remote builds in B16295: Diff 29369.Aug 10 2021, 07:13

• dalerogers added a subscriber: • dalerogers.Aug 10 2021, 07:26

This comment was removed by Fabien.

Fabien added a child revision: D9862: [avalanche] Don't protect nodes from peers with low score.Aug 10 2021, 11:08

deadalnix requested changes to this revision.Aug 10 2021, 12:42

deadalnix added a subscriber: deadalnix.

deadalnix added inline comments.

src/net.cpp
967 ↗	(On Diff #29369)	The datastructure you are building here already exists in the PeerManager, no? It seems like this is calling for refactoring to leverage this.
1143 ↗	(On Diff #29369)	These lambdas do not outlive the call, so using a generic `&` is fine, IMO.

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

Fabien added inline comments.Aug 11 2021, 09:16

src/net.cpp
967 ↗	(On Diff #29369)	There is no such structure, the peerid is managed by the avalanche::PeerManager and the score is part of the AvalancheState of each CNode.

Rebase, use [&] capture in lamda, don't protect nodes with score < 0.

Harbormaster completed remote builds in B16305: Diff 29381.Aug 11 2021, 09:41

Fabien added a child revision: D9867: [avalanche] Add a penalty to the availability score for the nodes we don't poll.Aug 11 2021, 12:32

Don't protect nodes with score of = 0, only > 0

Harbormaster completed remote builds in B16311: Diff 29390.Aug 12 2021, 12:04

deadalnix added inline comments.Aug 13 2021, 00:48

src/net.cpp
1165	And this is why you use trailing comas :)

Fabien added inline comments.Aug 13 2021, 07:39

src/net.cpp
1165	yes I missed it in D9857...

• geometrydash added a subscriber: • geometrydash.Aug 13 2021, 09:43

This comment was removed by Fabien.

deadalnix added a comment.Aug 14 2021, 00:58

This comment was removed by deadalnix.

I don't really like the design of this.

There is a lot of bookkeeping that is being done and datastructures suited for this kinda exist int he peer manager. Isn't it possible to run a similar algorithm in the peer manager and tag one node per peer there? There need to be a routine in there to cleanup outdated nodes anyways, so that seems appropriate to do it as part of that maintenance process.