Page MenuHomePhabricator

[avalanche] Make proof replacement check explicit
AbandonedPublic

Authored by sdulfari on May 6 2022, 00:09.

Details

Reviewers
deadalnix
Group Reviewers
Restricted Project
Summary

The unpatched if condition is not robust to additions of new states of AddProofStatus.
This patch makes the condition more explicit which is easier to read and more robust to
future changes.

Test Plan
ninja check-avalanche

Diff Detail

Repository
rABC Bitcoin ABC
Branch
explicit-proof-replacement-check
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 18968
Build 37698: Build Diffbuild-without-wallet · build-clang-tidy · lint-circular-dependencies · build-clang · build-diff · build-debug
Build 37697: arc lint + arc unit

Event Timeline

sdulfari requested review of this revision.May 6 2022, 00:09
deadalnix added inline comments.
src/avalanche/peermanager.cpp
262

What if it is duplicated?

src/avalanche/peermanager.cpp
262

That should not be possible since we are already in the REJECTED case of addProofIfNoConflict. If it was duplicated, we'd find ourselves in the DUPLICATED case statement (see around line 305).

src/avalanche/peermanager.cpp
262

That assumes the implementation of the function.

What if it was duplicated, would you want to run this code or not?

Attempt to handle unexpected DUPLICATED case gracefully

deadalnix requested changes to this revision.May 7 2022, 23:02

As per our discussion, I think the main problem with this is that, while the existing code indeed looks funny, not in a good way, it's not clear what the correct structure actually is. I think we should put this on hold untill we actually have a a problem that would benefit from restructuring this code, then we'll have a better idea of what it should look like.

This revision now requires changes to proceed.May 7 2022, 23:02