[avalanche] Make the peer data proof and delegation optional
AbandonedPublic
Actions

Authored by Fabien on Apr 15 2021, 12:42.

Details

Reviewers

PiRK
deadalnix

Group Reviewers

Restricted Project

Summary

The processor PeerData is currently a nullable pointer which is only
constructed if a proof is supplied.
This diff makes the PeerData a permanent member of the processor and
change the proof and delegation to be optional. This will allow for
adding other non optional members to the PeerData structure and extract
its initialization from the Processor.

Test Plan

ninja all check-all

Diff Detail

Repository

rABC Bitcoin ABC

Branch

avalanche_peer_data_optional_proof

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 15515
Build 30950: Build Diff	build-clang-tidy · lint-circular-dependencies · build-diff · build-debug · build-without-wallet · build-clang
Build 30949: arc lint + arc unit

Event Timeline

Fabien created this revision.Apr 15 2021, 12:42

Herald added a reviewer: Restricted Project. · View Herald TranscriptApr 15 2021, 12:42

Fabien requested review of this revision.Apr 15 2021, 12:42

Fabien added a child revision: D9419: [avalanche] Move the session key to the PeerData structure.Apr 15 2021, 12:46

Harbormaster completed remote builds in B15515: Diff 28189.Apr 15 2021, 12:54

PiRK requested changes to this revision.Apr 16 2021, 13:25

PiRK mentioned this in D9394: [avalanche] rename getProof to getLocalProof and return a pointer.

PiRK added a subscriber: PiRK.

PiRK added inline comments.

src/avalanche/processor.h
304	The doxygen documentation is now wrong and should be fixed. Now that the method has a more straightforward behavior, it can probably be reduced to a single line. In D9394 I renamed it to `getLocalProof`, because I anticipate that `getProof` will at some point be needed to get any proof from the peerset or orphan proofs, to relay them. It is not necessary to do it in this revision, but I mention it as a possible way of completely getting rid of the documentation by having all the information in the name.

This revision now requires changes to proceed.Apr 16 2021, 13:25

Address comment:

Update doxygen doc
Rename getProof => getLocalProof which is more accurate

Also make the proof optional a reference in init.cpp

Harbormaster completed remote builds in B15526: Diff 28200.Apr 16 2021, 16:35

PiRK accepted this revision.Apr 16 2021, 17:00

This revision is now accepted and ready to land.Apr 16 2021, 17:00

The rationale of this patch doesn't rally make sense. If data aren't optional, then don't put them in the PeerData. If the name turn out to b confusing then rename.

src/avalanche/processor.h
251 ↗	(On Diff #28200)	In what world can we have a delegation but no proof? I don't think this reflect the set of constraints we want here. Th optional smart pointer was best.
src/init.cpp
2456 ↗	(On Diff #28200)	Why would we expect our g_avalanche to contain an invalid proof? Why do we delegate to the network layer the responsibility to check proof validity (and, as an asside, this is duplicated from the initialization code, or at least there was an attempt, so you KNOW this doesn't make sense, the code is telling you).