Page MenuHomePhabricator

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

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 16305
Build 32475: Build Difflint-circular-dependencies · build-diff · build-clang-tidy · build-clang · build-without-wallet · build-debug
Build 32474: arc lint + arc unit

Event Timeline

Fabien requested review of this revision.Aug 9 2021, 14:58
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
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.

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

src/net.cpp
1165 ↗(On Diff #29390)

And this is why you use trailing comas :)

src/net.cpp
1165 ↗(On Diff #29390)

yes I missed it in D9857...

This comment was removed by deadalnix.
deadalnix requested changes to this revision.Aug 14 2021, 01:08

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.

src/net.cpp
1034 ↗(On Diff #29390)

This is int he right order.

This revision now requires changes to proceed.Aug 14 2021, 01:08
Mengerian edited tasks, added Restricted Maniphest Task; removed Restricted Maniphest Task.Sep 14 2021, 18:24
Mengerian edited the summary of this revision. (Show Details)