Page MenuHomePhabricator

[avalanche] Add a method to reject a proof
ClosedPublic

Authored by Fabien on Dec 21 2021, 09:12.

Details

Reviewers
tyler-smith
Group Reviewers
Restricted Project
Maniphest Tasks
Restricted Maniphest Task
Commits
rABC1e85695588dd: [avalanche] Add a method to reject a proof
Summary

This rejects the proof and attempt to bring back conflicting proofs if a peer was removed in the process. This will be used during the vote as well as when the tip is updated to remove newly invalidated proofs.
In case a peer is rejected, the mode selects whether the proof should be moved to the conflicting pool or removed entirely. First case will make it possible to change our position while the lasts makes sense after a vote is finalized: if a proof is confirmed rejected by the network there is no point keeping it around.

Ref T1854.

Test Plan
ninja check-avalanche

Diff Detail

Event Timeline

Fabien requested review of this revision.Dec 21 2021, 09:12
Fabien planned changes to this revision.Dec 24 2021, 08:13

Add a mode to select between reject and invalidate

tyler-smith added inline comments.
src/avalanche/peermanager.h
248 ↗(On Diff #31747)

Should INVALIDATE not keep the proof marked in a rejected pool or something?

258 ↗(On Diff #31747)

I'm curious why it was decided to pass in a behavior-modifying mode instead of having different functions that implement the different policies.

Fabien marked 2 inline comments as done.Jan 21 2022, 11:33
Fabien added inline comments.
src/avalanche/peermanager.h
248 ↗(On Diff #31747)

There is already a rejected proofs bloom filter in the network layer (callsite), which I wanted to use for that purpose but it is missing in D10788, I'll fix that.

258 ↗(On Diff #31747)

Most of the code is the same and it was just simpler to have a single mode param rather than a lot of tiny not very useful functions and 2 wrapping methods. This is also more consistent with the registerProof method.

One small additional assertion that I think would be ideal, other than that this behavior, implementation, and test look good.

src/avalanche/test/peermanager_tests.cpp
1425 ↗(On Diff #31747)

Should we have another BOOST_CHECK(pm.exists(proofid)); after calling rejectProof? That seems to be a critical difference between the INVALIDATE mode so I think it would be good to make clear here.

This revision now requires changes to proceed.Jan 26 2022, 09:27

One small additional assertion that I think would be ideal, other than that this behavior, implementation, and test look good.

Fabien marked 2 inline comments as done.

Assert the proof still exists if it was not an orphan

Now that the assertion is fixed, this looks correct and ready to go.

This revision is now accepted and ready to land.Jan 28 2022, 18:06
This revision was automatically updated to reflect the committed changes.