Page MenuHomePhabricator

[avalanche] Create a conflicting proof pool
ClosedPublic

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

Event Timeline

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

Name improvement: isConflicting => hasConflictingPeer

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.

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
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.

This revision was automatically updated to reflect the committed changes.