Page MenuHomePhabricator

[avalanche] Move the initial proof verification to PeerData constructor
AbandonedPublic

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

Details

Reviewers
deadalnix
Group Reviewers
Restricted Project
Summary

This better separate the responsabilities in the code and allow for
expanding the verifications easily. This will be later used for checking
an imported delegation.

Depends on D9420.

Test Plan
ninja all check-all

Diff Detail

Repository
rABC Bitcoin ABC
Branch
avalanche_peer_data_verify_proof
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 15518
Build 30956: Build Difflint-circular-dependencies · build-without-wallet · build-diff · build-debug · build-clang-tidy · build-clang
Build 30955: arc lint + arc unit

Event Timeline

Fabien requested review of this revision.Apr 15 2021, 12:53
deadalnix requested changes to this revision.Apr 19 2021, 23:47
deadalnix added a subscriber: deadalnix.
deadalnix added inline comments.
src/avalanche/processor.cpp
140

Ok, this is getting somewhere, but see, if you kept the smart pointer, you'd have been able to return something nullable. Now you *HAVE TO* construct the PeerData object, which makes no sense when there is an error.

You threw away the guarantee that the PeerData object is properly constructed with that design.

But then you'll have another problem: what if this should actually be null, and it's not an error? Well, this is why the factory method should be about the processor, not the PeerData. Bonus point, you don't need to make PeerData part of the API.

It's all win, but you got to ask yourself the right question before going down some path. It's not some hypothetical future change that might fubar things up, this is the code you just wrote telling you this. Read it.

238

Can't you do a ctor style init?

242

Not sure where this was introduced, but urg. This is pretty much the definition of temporal coupling: https://blog.ploeh.dk/2011/05/24/DesignSmellTemporalCoupling/

src/avalanche/processor.h
205

What i the purpose of this? Is this some independent entity that is of general use outside the Processor?

If yes, then please add it to the description of the diff.

If not, then please, stop. Ask yourself how you came to create some new publicly exposed entity that is of no real purpose and go fix that problem. It seems to me that the only purpose is initialization, which means go fix the initialization of this whole thing instead if patching layers on top of layers of wrong design. Don't you see it? You've now just enabled the mess to spread to the whole codebase instead of staying contained here.

How is that a win?

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