Page MenuHomePhabricator

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

Authored by Fabien on Dec 20 2021, 21:19.

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 Diffbuild-diff · lint-circular-dependencies · build-debug · build-clang-tidy · build-clang · build-without-wallet
Build 35365: arc lint + arc unit

Event Timeline

Fabien requested review of this revision.Dec 20 2021, 21:19
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
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

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.

This revision now requires changes to proceed.Jan 13 2022, 16:25
This revision is now accepted and ready to land.Jan 14 2022, 10:07