When adding/remove peers/nodes we track the scores so we can quickly query the state for quorum
detection.
Details
Details
- Reviewers
Fabien - Group Reviewers
Restricted Project - Maniphest Tasks
- Restricted Maniphest Task
- Commits
- rABCdf31b9e30915: [avalanche] Track registered and allocated scores in PeerManager.
ninja check-avalanche
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
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 |
| 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. |
| 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. |
| src/avalanche/test/peermanager_tests.cpp | ||
|---|---|---|
| 46 ↗ | (On Diff #32778) | OK |