Page MenuHomePhabricator

[avalanche] Track registered and allocated scores in PeerManager.
ClosedPublic

Authored by tyler-smith on Mar 16 2022, 09:15.

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Maniphest Tasks
Restricted Maniphest Task
Commits
rABCdf31b9e30915: [avalanche] Track registered and allocated scores in PeerManager.
Summary

When adding/remove peers/nodes we track the scores so we can quickly query the state for quorum
detection.

Test Plan

ninja check-avalanche

Diff Detail

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

Event Timeline

Fabien requested changes to this revision.EditedMar 16 2022, 14:08
Fabien added a subscriber: Fabien.

Mostly good, additions/subtractions/asserts are done in the right place.
The main problem is the name of the variables being confusing. The tests can be improved a bit (see suggestions).

src/avalanche/peermanager.h
204 ↗(On Diff #32737)

You don't register or allocate a score, do you ?

src/avalanche/test/peermanager_tests.cpp
1685 ↗(On Diff #32737)

Don't rely on peer id, retrieve it instead

1687 ↗(On Diff #32737)

You never checked that the scores sum up in the presence of several peers

1694 ↗(On Diff #32737)
uint32_t score1 = 10000000 * MIN_VALID_PROOF_SCORE;
auto proof = buildRandomProof(score1);
1716 ↗(On Diff #32737)

Dito

1747 ↗(On Diff #32737)

You can also test that removing a peer with nodes reduce the allocated score as expected

This revision now requires changes to proceed.Mar 16 2022, 14:08

Rename properties and improve tests.

Fabien requested changes to this revision.Mar 17 2022, 08:35
Fabien added inline comments.
src/avalanche/peermanager.h
203 ↗(On Diff #32778)

Let's try to align with the getavalancheinfo rpc as much as possible, since the names have already been discussed there totalPeersScore.

src/avalanche/test/peermanager_tests.cpp
46 ↗(On Diff #32778)

I don't see the point of splitting this, there is no reason not to use the actual facility directly.
You can still check the registration is successful with BOOST_CHECK_NE(TestPeerManager::registerAndGetPeerId(pm, proof), NO_PEER);

This revision now requires changes to proceed.Mar 17 2022, 08:35
src/avalanche/test/peermanager_tests.cpp
46 ↗(On Diff #32778)

The peerid changes when the peer is moved out of and back into the valid pool, so getting the peerid when it's first registered doesn't help.

Update variable and method names.

Fabien added inline comments.
src/avalanche/test/peermanager_tests.cpp
46 ↗(On Diff #32778)

OK

This revision is now accepted and ready to land.Mar 18 2022, 08:35
Mengerian added a task: Restricted Maniphest Task.Mar 18 2022, 16:07