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

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

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 15662
Build 31229: Build Diff	build-without-wallet · lint-circular-dependencies · build-clang · build-debug · build-diff · build-clang-tidy
Build 31228: arc lint + arc unit

Event Timeline

PiRK created this revision.May 8 2021, 21:16

Herald added a reviewer: Restricted Project. · View Herald TranscriptMay 8 2021, 21:16

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.

PiRK edited the summary of this revision. (Show Details)May 8 2021, 21:22

Harbormaster completed remote builds in B15662: Diff 28391.May 8 2021, 21:31

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

Harbormaster completed remote builds in B15663: Diff 28392.May 8 2021, 21:49

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`