Page MenuHomePhabricator

[avalanche] Output a proof registration state
ClosedPublic

Authored by Fabien on Jan 6 2022, 13:39.

Details

Reviewers
tyler-smith
Group Reviewers
Restricted Project
Maniphest Tasks
Restricted Maniphest Task
Commits
rABC9998707227e3: [avalanche] Output a proof registration state
Summary

This gives extra details on why a proof failed to register. Currently unused, it will be used in various places to improve the error reporting and simplify the code based on what the failure cause is.

Ref T1854.

Test Plan
ninja all check-all

Diff Detail

Repository
rABC Bitcoin ABC
Branch
avalanche_registration_state
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 17913
Build 35649: Build Difflint-circular-dependencies · build-without-wallet · build-diff · build-debug · build-clang-tidy · build-clang
Build 35648: arc lint + arc unit

Event Timeline

Fabien requested review of this revision.Jan 6 2022, 13:39
deadalnix added inline comments.
src/avalanche/peermanager.cpp
215 ↗(On Diff #31662)

REJECTED vs EVICTED? Are they synonyms? if not, what's the difference?

src/avalanche/peermanager.cpp
215 ↗(On Diff #31662)

They are synonyms, I used a different word to make easier to distinguish between AddProofStatus and ProofRegistrationResult but obviously it didn't work and caused more confusion.

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

Make consistent use of the REJECTED naming

Not a strict change request but something to consider. Other than this issue, this changeset looks good/correct.

src/avalanche/peermanager.cpp
174

AFAICT there is a 1-1 mapping between ProofRegistrationResult values and the message uses, so I think it would be ideal to not pass the message here but instead have a const defined next to the ProofRegistrationResult enum that defines a string message for each ProofRegistrationResult value.

I think this is cleaner than having unexplained string values throughout the function and eliminates the chances of a call to invalidate passing mismatching arguments.

src/avalanche/peermanager.cpp
174

There is a twist here: the invalidate lambda is taking a ProofRegistrationResult, but the function returns a ProofRegistrationState (via the reference parameter). The "state" is mostly the final status + the "result" enum (kind of an error code) + a couple message strings for normal and debug print.

The 1-1 matching here is intentional but not a mandatory thing, e.g. the TxValidationResult enum have a TX_CONSENSUS value that can accommodate several different error messages. I don't think there is a need to enforce this 1-1 relationship, otherwise it should be done at the parent class ValidationResult level which is out of scope for this diff.

Looks good; no logical changes and the verify result tagging looks like a smart abstraction for this. The tests for the result tags look correct and comprehensive.

This revision is now accepted and ready to land.Jan 30 2022, 23:09