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

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

Fabien created this revision.Dec 21 2021, 09:12

Herald added a reviewer: Restricted Project. · View Herald TranscriptDec 21 2021, 09:12

Fabien requested review of this revision.Dec 21 2021, 09:12

Fabien added a child revision: D10712: [avalanche] Simplify the updatedBlockTip handler.Dec 21 2021, 09:24

Harbormaster completed remote builds in B17705: Diff 31502.Dec 21 2021, 09:43

Fabien planned changes to this revision.Dec 24 2021, 08:13

Fabien edited the summary of this revision. (Show Details)Jan 7 2022, 10:44

Add a mode to select between reject and invalidate

Harbormaster completed remote builds in B17826: Diff 31686.Jan 7 2022, 11:20

Fabien added a child revision: D10727: [avalanche] Poll for the new proofs.Jan 7 2022, 16:13

Rebase

Harbormaster completed remote builds in B17876: Diff 31747.Jan 11 2022, 09:13

tyler-smith added a subscriber: tyler-smith.Jan 21 2022, 07:11

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.