Page MenuHomePhabricator

[avalanche] Send the proof master pubkey with AVAHELLO
AbandonedPublic

Authored by PiRK on May 12 2021, 16:05.

Details

Reviewers
deadalnix
Group Reviewers
Restricted Project
Summary

The proof master key is needed to check the delegation and the signature included in the AvaHello message. The LimitedProofId is required to check that the provided proof master key actually belongs to the advertised proof, by allowing to compute the proofid using the limited id and master key, and then comparing the computed id to the one included in the delegation.

The delegation verification no longers needs to be done in PeerManager::addNode, as the node is banned if the delegation verification fails on avahello reception.

Depends on D9544

Test Plan

ninja all check-all

Diff Detail

Event Timeline

PiRK requested review of this revision.May 12 2021, 16:05

This is intended to replace D9497, because enforcing that a delegation needs at least one level seems to require more change in more places in the codebase, and building dummy delegation to self seems like a clumsy hack.

I don't know if I should put any effort on keeping the message deserialization compatible with previous versions. If we don't want to store the signature and wait for the proof to be able to check it, there is not much we can do and adding compatibility code seems unneccessary, but i'm not sure what happens when a new node receives an old message and finds a pubkey when it expects a schnorrSig.

No need for it to be compatible.

PiRK planned changes to this revision.May 13 2021, 17:01

Ideas : create a LimitedProofId without the master key, then make ProofId = hash(LimitedProofId + master key) .Then Avahello can contain the LimitedProofId and the master key, and the delegation can be verified with regards to the proof before we can see the proof.

Other changes can be considered for the proof, to make it easier to collaboratively build a proof (merkle tree...)

Rebase on D9544

include the limited proof id in the AvaHello message, use it to compute the ProofId, properly check the delegation (dg.proofid must match the computed proofid).

Now we are able to confidently verify the delegation and the signature as soon as the AvaHello message arrives.

PiRK edited the summary of this revision. (Show Details)
deadalnix requested changes to this revision.May 18 2021, 20:46
deadalnix added inline comments.
src/avalanche/delegation.h
54

That does not make sense, the delegation already has the proof id as its member.

This revision now requires changes to proceed.May 18 2021, 20:46

pass a limited proof id to Delegation::verify instead of the already computed proofid, and refactor the code that generates a proofid from a limited id to avoid code duplication.

deadalnix requested changes to this revision.May 19 2021, 23:17
deadalnix added inline comments.
src/avalanche/delegation.cpp
66 ↗(On Diff #28539)

You are calling two different thing master.

src/avalanche/delegation.h
54 ↗(On Diff #28539)

Why aren't these part of the delegation?

It seems that they are necessary to verify anyways, and the branching all over the place over the second master being option is a hint that something is not quite right.

This revision now requires changes to proceed.May 19 2021, 23:17
PiRK planned changes to this revision.May 20 2021, 15:41

Working on adding all the data needed to verify the delegation inside the delegation.