Page MenuHomePhabricator

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

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 Diffbuild-clang-tidy · lint-circular-dependencies · build-diff · build-debug · build-without-wallet · build-clang
Build 30949: arc lint + arc unit

Event Timeline

Fabien requested review of this revision.Apr 15 2021, 12:42
PiRK requested changes to this revision.Apr 16 2021, 13:25
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

This revision is now accepted and ready to land.Apr 16 2021, 17:00
deadalnix requested changes to this revision.Apr 19 2021, 23:16
deadalnix added a subscriber: deadalnix.

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).

This revision now requires changes to proceed.Apr 19 2021, 23:16