Page MenuHomePhabricator

[avalanche] let PeerManager::addNode deal with signature verification
AbandonedPublic

Authored by PiRK on May 8 2021, 21:16.

Details

Reviewers
deadalnix
Group Reviewers
Restricted Project
Summary

While working on peer discovery (D9478), I found that the current API makes it difficult to verify avahello signatures then add the node without checking the delegation twice, which can be expensive.

To check the signature, the delegated public key is needed. This key is obtained using Delegation::verify, which is called in PeerManager::addNode.
This change adds an optional parameter sigData to addNode, with the signature data that needs to be checked.

The parameter is optional because we don't need to check a signature when manually adding a peer using the addavalanchenode RPC.

Test Plan

ninja all check-all

Event Timeline

PiRK requested review of this revision.May 8 2021, 21:16

An alternative solution is to extract the delegated key from the delegation without checking it, use it to check the signature, then call PeerManager::addNode which will then verify the delegation . I like this solution a bit less, because it is more code : the pubkey is initialized twice it requires an additional Delegation::getPubKey method to return the last level of the delegation.

set a default parameter to avoid repeating std::nullopt in tests

deadalnix requested changes to this revision.May 8 2021, 22:32
deadalnix added a subscriber: deadalnix.

Just check the signature as soon as the messages arrives. There is no point making this dependent on the network layer.

src/avalanche/peermanager.cpp
33 ↗(On Diff #28392)

The fact this doesn't depend on anything else in this function but the parameter you added is a sure sign that this doesn't belong here.

Why do you want to run what precedes this without a valid signature anyways?

This revision now requires changes to proceed.May 8 2021, 22:32

I'm going to try with const CPubKey &Delegation::getMaster(const Proof &proof) const;

src/avalanche/peermanager.cpp
35 ↗(On Diff #28392)

OK.

Why do you want to run what precedes this without a valid signature anyways?

I don't think adding a new valid proof hurts anything even if that node turns out not to be the owner, but I agree that it is not the point of addNode