Page MenuHomePhabricator

[avalanche] Make it possible to replace a proof
AbandonedPublic

Authored by Fabien on Nov 12 2021, 11:54.

Details

Reviewers
deadalnix
Group Reviewers
Restricted Project
Maniphest Tasks
Restricted Maniphest Task
Summary

This lets the peer manager decide to replace a proof with another instead of rejecting the conflict.
This feature is hidden behind a flag so the behavior is not changed by default.

Ref T1854.

Depends on D10467.

Test Plan
ninja all check-all

Diff Detail

Repository
rABC Bitcoin ABC
Branch
avalanche_peermanager_conflictingproofs
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 17283
Build 34394: Build Diffbuild-without-wallet · build-clang · lint-circular-dependencies · build-clang-tidy · build-debug · build-diff
Build 34393: arc lint + arc unit

Event Timeline

Fabien requested review of this revision.Nov 12 2021, 11:54
deadalnix requested changes to this revision.Nov 24 2021, 01:55
deadalnix added a subscriber: deadalnix.
deadalnix added inline comments.
src/avalanche/peermanager.cpp
150

The name of this method is a good tell that the abstraction don't make sense. If the caller care about manipulating the pool, then the caller should be manipulating the pool, and if it isn't, then why does this function exist?

181

So what you want here are the proof that were removed, note the one that were shifted. Which and up being the same because the proof pool has a bucket size of 1, but once again, we stumble onto something that is fragile and more complex, for reasons that are hard to justify - and are not in the diffs description.

src/avalanche/test/peermanager_tests.cpp
921

Is that a bug in the test? If so, this needs to be fixed on its own.

src/init.cpp
15

This was a good tell that this wasn't the right place to put AVALANCHE_DEFAULT_PROOF_REPLACEMENT_ENABLED in.

This revision now requires changes to proceed.Nov 24 2021, 01:55