[avalanche] Send the proof master pubkey with AVAHELLO
AbandonedPublic
Actions

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

Repository

rABC Bitcoin ABC

Branch

proofmaster_avahello

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 15697
Build 31298: Build Diff	build-without-wallet · build-debug · lint-circular-dependencies · build-diff · build-clang-tidy · build-clang
Build 31297: arc lint + arc unit

Event Timeline

PiRK created this revision.May 12 2021, 16:05

Herald added a reviewer: Restricted Project. · View Herald TranscriptMay 12 2021, 16:05

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.

Harbormaster completed remote builds in B15697: Diff 28436.May 12 2021, 16:22

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.

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)May 18 2021, 16:11

PiRK edited the summary of this revision. (Show Details)

PiRK added a parent revision: D9544: [avalanche] introduce LimitedProofId.

Harbormaster completed remote builds in B15756: Diff 28530.May 18 2021, 16:21

deadalnix requested changes to this revision.May 18 2021, 20:46

deadalnix added inline comments.

src/avalanche/delegation.h
54 ↗	(On Diff #28530)	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.

Harbormaster completed remote builds in B15763: Diff 28539.May 19 2021, 13:06

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.