[avalanche] Check the delegation generated at startup is valid
ClosedPublic
Actions

Authored by Fabien on Apr 26 2021, 10:14.

Details

Reviewers

deadalnix

Group Reviewers

Restricted Project

Maniphest Tasks

Restricted Maniphest Task

Commits

rABCa668273790a0: [avalanche] Check the delegation generated at startup is valid

Summary

This makes sure we don't generate an invalid delegation. The proof and
delegation verification are moved to their own function to unbloat the
processor factory.

The appropriated tests will be added when the -avadelegation is added as it will allow for triggering the failures.

Ref T1635.

Test Plan

ninja check-functional

Diff Detail

Repository

rABC Bitcoin ABC

Branch

check_delegation

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 15982
Build 31863: Build Diff	lint-circular-dependencies · build-without-wallet · build-diff · build-debug · build-clang-tidy · build-clang
Build 31862: arc lint + arc unit

Event Timeline

Fabien created this revision.Apr 26 2021, 10:14

Herald added a reviewer: Restricted Project. · View Herald TranscriptApr 26 2021, 10:14

Fabien requested review of this revision.Apr 26 2021, 10:14

Harbormaster completed remote builds in B15576: Diff 28268.Apr 26 2021, 10:30

Why does this depends on D9365 . It seems to me that verifying that the delegation is correct isn't really something that depends on being able to supply it. If anything, it should be done no matter what.

src/avalanche/processor.cpp
304 ↗	(On Diff #28268)	Once again, is this checking for the delegation's validity? Why? If not, then this code is even more puzzling.
321 ↗	(On Diff #28268)	Is this used anywhere?
test/functional/abc_rpc_avalancheproof.py
267 ↗	(On Diff #28268)	The fact that this comment bars no resemblance to the error message reported is a sure sign something's not quite right. You can't express what is going on in a consistent manner because there is nothing consistent about this behavior to express. I think I kinda understand where the edge case come from, but ultimately, that means the code needs to be reworked, and reading that test should have been a clear sign this is the case.