[avalanche] Create a conflicting proof pool
ClosedPublic
Actions

Authored by Fabien on Dec 9 2021, 16:48.

Details

Reviewers

deadalnix

Group Reviewers

Restricted Project

Maniphest Tasks

Restricted Maniphest Task

Commits

rABCbb3cc1f1db31: [avalanche] Create a conflicting proof pool

Summary

This moves away from the orphan pool the proofs that are valid and pending due to conflicts.
Since the orphans were previously rescanned, the conflicting proofs needs to do the same, but rather than looking for all the pool we only attempt to register the ones that conflicted with removed peers. The conflicting proof rescan test is updated accordingly.

Ref T1854.

Test Plan

ninja all check-all

Diff Detail

Repository

rABC Bitcoin ABC

Branch

avalanche_conflicting_pool

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 17616
Build 35058: Build Diff	lint-circular-dependencies · build-without-wallet · build-diff · build-debug · build-clang-tidy · build-clang
Build 35057: arc lint + arc unit

Event Timeline

Fabien created this revision.Dec 9 2021, 16:48

Herald added a reviewer: Restricted Project. · View Herald TranscriptDec 9 2021, 16:48

Fabien requested review of this revision.Dec 9 2021, 16:49

Fabien mentioned this in D10524: [avalanche] Select the favorite orphan in case of a conflict.Dec 9 2021, 16:56

Fabien added a child revision: D10524: [avalanche] Select the favorite orphan in case of a conflict.Dec 9 2021, 16:56

Fabien added a child revision: D10660: [avalanche] Select the favorite conflicting proof.Dec 9 2021, 17:01

Harbormaster completed remote builds in B17595: Diff 31341.Dec 9 2021, 17:04

Fabien added a child revision: D10525: [avalanche] Make it possible to replace a proof.Dec 10 2021, 10:58

Name improvement: isConflicting => hasConflictingPeer

Harbormaster completed remote builds in B17606: Diff 31352.Dec 10 2021, 13:47

Fabien added a child revision: D10663: [avalanche] Add a method to check if a proof has conflicts.Dec 10 2021, 14:30

deadalnix accepted this revision.Dec 10 2021, 16:33

This revision is now accepted and ready to land.Dec 10 2021, 16:33

deadalnix requested changes to this revision.Dec 10 2021, 16:35

deadalnix added inline comments.

src/avalanche/peermanager.h
260 ↗	(On Diff #31352)	This method name make no sense whatsoever. You can't even do what the name it does from the parameter it has. Does this proof id has a conflicting peer? How do you check that a peer conflict with a proof id? You'd need the proof, at least.

This revision now requires changes to proceed.Dec 10 2021, 16:35

Rebase.
Moved (and adapted) the test from D10660 to add test coverage for the proofs in the conflicting pool not being allowed to be replaced (yet).
I struggled to find a name I was happy with so I went with a naive one: isInConflictingPool(). Suggestions are welcome.

Harbormaster completed remote builds in B17616: Diff 31367.Dec 11 2021, 12:43

deadalnix accepted this revision.Dec 12 2021, 22:18

deadalnix added inline comments.

src/avalanche/peermanager.cpp
236	I think it would be good to keep track of the set of proof we send through this so we don't send the same one a gajilion times.

This revision is now accepted and ready to land.Dec 12 2021, 22:18

Fabien added inline comments.Dec 13 2021, 09:05

src/avalanche/peermanager.cpp
236	The proof registration will bail early if the proof is known already so it's not very costly to register several times the same proof. I agree it's better to make this not relying on the register proof implementation but I'll leave that for another diff.