Details

Reviewers

deadalnix

Group Reviewers

Restricted Project

Maniphest Tasks

Restricted Maniphest Task

Commits

rABC1ea5404c2027: [avalanche] Add a mode to the proof registration to force accept it as a peer

Summary

This diff adds a new mode parameter to the proof registration function. This allows for reusing most of the registration code and add the specific behavior that will be requires to account for the votes. There is no change in behavior by default. The new force accept mode will be used when the network accepts the proof during a vote.

Ref T1854.

Test Plan

ninja all check-avalanche

Diff Detail

Repository

rABC Bitcoin ABC

Branch

avalanche_force_accept_mode

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 17770
Build 35366: Build Diff	build-diff · lint-circular-dependencies · build-debug · build-clang-tidy · build-clang · build-without-wallet
Build 35365: arc lint + arc unit

Event Timeline

Fabien created this revision.Dec 20 2021, 21:19

Herald added a reviewer: Restricted Project. · View Herald TranscriptDec 20 2021, 21:19

Fabien requested review of this revision.Dec 20 2021, 21:19

Harbormaster completed remote builds in B17695: Diff 31485.Dec 20 2021, 21:34

Fabien added a child revision: D10714: [avalanche] Limit the conflicting proof rate.Dec 21 2021, 12:38

Use an enum class

Harbormaster completed remote builds in B17770: Diff 31598.Jan 3 2022, 14:11

deadalnix requested changes to this revision.Jan 5 2022, 17:15

deadalnix added a subscriber: deadalnix.

deadalnix added inline comments.

src/avalanche/peermanager.cpp
205	This needs to happen in case of success, and you need a test case for this, as removing it didn't break anything. If it isn't reachable, why? Should we assert this?

This revision now requires changes to proceed.Jan 5 2022, 17:15

Fabien added inline comments.Jan 5 2022, 20:50

src/avalanche/peermanager.cpp
205	This was unreachable before this diff, because the first thing the registration did is checking that the proof does not exist. After this diff the initial check is changed, but obviously at this stage we are creating the peer so the proof should never end up in the conflicting pool, and the added code path makes sure that this never happens. It's a good idea to assert it, I'll add this with a comment.

Assert the proof is not in the conflicting pool at the time the peer is getting created

Harbormaster completed remote builds in B17799: Diff 31656.Jan 5 2022, 21:08

Fabien added a child revision: D10773: [avalanche] Output a proof registration state.Jan 6 2022, 13:39

Fabien removed a child revision: D10714: [avalanche] Limit the conflicting proof rate.Jan 7 2022, 09:39

deadalnix requested changes to this revision.Jan 13 2022, 16:25

deadalnix added inline comments.

src/avalanche/peermanager.cpp
172–173 ↗	(On Diff #31656)	if ((mode != RegistrationMode::FORCE_ACCEPT \|\| !isInConflictingPool(proofid) && exists(proofid)) Would short circuit more often.
236 ↗	(On Diff #31656)	I don't really see what we win turning the remove into an assert. It appear stricly more risky, considering the former reestablish the invariant we are expecting.