Page MenuHomePhabricator

[avalanche] let the PeerManager take ownership of the proof
AbandonedPublic

Authored by PiRK on May 1 2021, 09:47.

Details

Reviewers
deadalnix
Group Reviewers
Restricted Project
Summary

When a proof is added via the addavalanchenode RPC, we can avoid doing a copy.
Currently there are two public methods that can be used to register proofs into the PeerManager: addNode and getPeerId. They both register the proof via the private method fetchOrCreatePeer.

This makes all 3 methods take the proof by value rather than by constant ref. It updates all calls to addNode to pass the proof as an rvalue, to avoid an unecessary copy. getPeerId is currently only used register the node's own proof, and this is the only case where we want to do a copy, as the proof is also stored in the peerData struct of the processor (in the future we might want to just store the proofid and get the Proof from the PeerManager when needed).

Test Plan

ninja all check-all

Event Timeline

PiRK requested review of this revision.May 1 2021, 09:47
deadalnix requested changes to this revision.May 2 2021, 12:18
deadalnix added a subscriber: deadalnix.

This change does not looks like it make sense to me, because you don't know if it is actually taking ownership or not.

src/avalanche/peermanager.cpp
194

Does it take ownership of the proof when going through this code path?

205

Or this copdepath?

This revision now requires changes to proceed.May 2 2021, 12:18
src/avalanche/peermanager.cpp
205

This code path will take ownership, in some cases, once we start storing the orphan proofs.

I'm abandoning this for now, it is supposed to be an optimization and not strictly needed to make peer discovery work. No need for yet another blocking dependency.