Page MenuHomePhabricator

[avalanche] Remember rejected proofs
Needs RevisionPublic

Authored by PiRK on Thu, Apr 15, 13:43.

Details

Reviewers
Fabien
deadalnix
Group Reviewers
Restricted Project
Summary

Add a rolling bloom filter to remember rejected proofs and avoid verifying them multiple times.

Add an accessor method to test this filter, and add the corresponding unit test.

This is a piece of D9370 with improvements.

Test Plan

ninja && ninja check

Diff Detail

Event Timeline

PiRK requested review of this revision.Thu, Apr 15, 13:44

rebase onto master (this diff has no dependency)

Fabien requested changes to this revision.Mon, Apr 19, 09:31
Fabien added a subscriber: Fabien.

What makes them recent and why are they different from other rejected proofs ?

This revision now requires changes to proceed.Mon, Apr 19, 09:31

rename recentProofRejects -> rejectedProofs.
I used the recent word to mean that the filter has a limited size and will drop the oldest entries when full. But that is an implementation detail. A proof that enters this pool will be bad forever, there is no reset of the filter done on tip updates, like there is for transactions.

Rename the test method PeerManager::addRecentReject -> PeerManager::rejectProof

Also add a comment to explain the choice of parameters for the bloom filter (same params as for transactions).

rephrase the misleading comment.

deadalnix requested changes to this revision.Tue, Apr 20, 00:01
deadalnix added a subscriber: deadalnix.
deadalnix added inline comments.
src/avalanche/peermanager.cpp
179

Why is this checking for this specific state?

208

I'm not sure this makes sense. This check is probabilistic, it it shouldn't be there, but somewhere between the network on here.

this definitively raises the question: should this filter be in the PeerManager at all?

src/avalanche/peermanager.h
130

How does CRollingBloomFilter ends up being declared here?

195

This is the section you are in, this comment is redundant.

This revision now requires changes to proceed.Tue, Apr 20, 00:01
PiRK retitled this revision from [avalanche] Remember recently rejected proofs to [avalanche] Remember rejected proofs.Tue, Apr 20, 09:57
src/avalanche/peermanager.cpp
179

Because the orphan proofs will go into another pool.

208

Having the filter in the PeerManager is an easy way to fit it in the existing codebase and avoid having the network layer deal with proof verifications. The PeerManager is where proofs are currently verified and stored (if they are good).

I thought about putting it elsewhere, but it requires some clumsy forwarding of ProofValidationState, if we want to avoid checking a proof twice (this is a previous attempt at doing that : https://reviews.bitcoinabc.org/D9417).