Page MenuHomePhabricator

[avalanche] Manipulate peers via PeerId
ClosedPublic

Authored by deadalnix on Tue, Jun 30, 16:12.

Details

Reviewers
jasonbcox
Group Reviewers
Restricted Project
Commits
rABCc23e06f18a83: [avalanche] Manipulate peers via PeerId
Summary

This ensure we do not expose information from internal data structure and can propose a stable API.

Test Plan
ninja all check-avalanche

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

deadalnix created this revision.Tue, Jun 30, 16:12
Herald added a reviewer: Restricted Project. · View Herald TranscriptTue, Jun 30, 16:12
deadalnix requested review of this revision.Tue, Jun 30, 16:12
jasonbcox requested changes to this revision.Tue, Jun 30, 22:23
jasonbcox added a subscriber: jasonbcox.

The nits aren't a big deal, but the comment in selectPeerImpl() needs to be addressed in some way.

src/avalanche/peermanager.cpp
10 ↗(On Diff #21899)

Nit: Add a TODO otherwise this comment is confusing

135 ↗(On Diff #21899)

I can't find a reference to this behavior in the rest of the code or tests. Assuming this is a mistake, it indicates there's a missing test case somewhere. More specifically, this could impact selectPeer() in weird ways.

src/avalanche/peermanager.h
12 ↗(On Diff #21899)

Nit: Introduction of PeerId could have been its own diff which would eliminate extra scrolling during review due to extra context in this diff.

This revision now requires changes to proceed.Tue, Jun 30, 22:23
deadalnix updated this revision to Diff 21915.Tue, Jun 30, 23:27

Check insertion and remove refactoring leftover

jasonbcox accepted this revision.Wed, Jul 1, 01:32
This revision is now accepted and ready to land.Wed, Jul 1, 01:32
This revision was automatically updated to reflect the committed changes.