[avalanche] Correctly check if a contender is associated with a remote proof
ClosedPublic
Actions

Authored by roqqit on Mon, Apr 28, 23:18.

Details

Reviewers

Fabien

Group Reviewers

Restricted Project

Commits

rABC4a35b7c33298: [avalanche] Correctly check if a contender is associated with a remote proof

Summary

Contenders incorrectly checks if there is any remote proof status set for a proof but the intent was to check if the remote proof is present. This materializes into incorrect behavior when a node is restarted with persisted avapeers. The contender will improperly be promoted despite no remote proof being present.

Test coverage is not complete on the functional test side but that will be improved in a near future diff. For now we can be comfortable knowing the existing tests pass since they were written with the assumption that remote presence was being checked.

Depends on D18010

Test Plan

ninja check
./test/functional/test_runner.py abc_p2p_avalanche_contender_voting

Diff Detail

Repository

rABC Bitcoin ABC

Branch

sc-isremote

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 33164
Build 65811: Build Diff	lint-circular-dependencies · build-without-wallet · build-diff · build-debug · build-clang-tidy · build-clang
Build 65810: arc lint + arc unit

Event Timeline

roqqit created this revision.Mon, Apr 28, 23:18

Herald added a reviewer: Restricted Project. · View Herald TranscriptMon, Apr 28, 23:18

roqqit requested review of this revision.Mon, Apr 28, 23:18

Harbormaster completed remote builds in B33141: Diff 53737.Mon, Apr 28, 23:29

roqqit added a child revision: D18013: [avalanche] Only re-add contenders to the cache if the remote proof is present.Mon, Apr 28, 23:53

OK with the behavior fix. Not sure about the clarity of name. Maybe isRemoteProofPresent or hasRemoteProof would be clearer? Or maybe a line of doc for hasRemoteProofStatus and isRemoteProof would clarify things.

Fabien requested changes to this revision.Tue, Apr 29, 07:24

Fabien added a subscriber: Fabien.

Fabien added inline comments.

src/avalanche/peermanager.cpp
723 ↗	(On Diff #53737)	As per Pierre feedback, what about this ?
726–731 ↗	(On Diff #53737)	not tested
1469 ↗	(On Diff #53737)	Do you really need the isDangling() here ?