Page MenuHomePhabricator

[avalanche] Don't require the proof when adding an avalanche node
ClosedPublic

Authored by Fabien on Jun 24 2021, 12:32.

Details

Reviewers
deadalnix
Group Reviewers
Restricted Project
Maniphest Tasks
Restricted Maniphest Task
Commits
rABCcfceda94a10e: [avalanche] Don't require the proof when adding an avalanche node
Summary

An avalanche node is bound to an avalanche peer, which itself requires a
proof. So far the PeerManager::addNode() was fetching the peer from
the supplied proof, creating it as neeeded. This is an inconvenient API
when we need to add a node while the proof is not yet known (and the
peer doesn't exist yet).

This diff updates the method so that it does not require a proof
anymore. The proof is looked up from the node's delegation, and adding
the node fails if there is no peer with that proof. This will allow for
adding nodes pending for a peer that would need to be bound together at
a later time. Another consequence is that it removes the case of the
proof not matching the delegation.

It would have been possible to keep the current API and add the proof
first if supplied, but since there is a single callsite (outside of
tests) it is easier to just update it.

Depends on D9698.

Ref T1634.

Test Plan
ninja all check-all

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Fabien requested review of this revision.Jun 24 2021, 12:32
deadalnix requested changes to this revision.Jun 24 2021, 16:12
deadalnix added a subscriber: deadalnix.
deadalnix added inline comments.
src/avalanche/peermanager.cpp
33 ↗(On Diff #28969)

Do this check first. There is no point verifying the delegation if you aren't going to add it anyways.

This revision now requires changes to proceed.Jun 24 2021, 16:12
src/avalanche/peermanager.cpp
24 ↗(On Diff #28969)

Who or what is creating the  fetchOrCreatePeer function now?

src/avalanche/peermanager.cpp
24 ↗(On Diff #28969)

This is done when adding a proof (via getPeerId). At the moment the only callsite for addNode is in the rpc (test excepted).

33 ↗(On Diff #28969)

This will make sense later when we add the node as waiting for a proof, but I'll revert for now and do the change when needed.

Check the proof exists before verifying the delegation

This revision is now accepted and ready to land.Jun 28 2021, 12:23