[avalanche] Remember rejected proofs
AbandonedPublic
Actions

Authored by PiRK on Apr 15 2021, 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

Repository

rABC Bitcoin ABC

Branch

recentProofRejects

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 15532
Build 30984: Build Diff	lint-circular-dependencies · build-diff · build-without-wallet · build-clang · build-clang-tidy · build-debug
Build 30983: arc lint + arc unit

Event Timeline

PiRK created this revision.Apr 15 2021, 13:43

Herald added a reviewer: Restricted Project. · View Herald TranscriptApr 15 2021, 13:43

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

PiRK mentioned this in D9370: [avalanche] orphan pool for proofs, make AlreadyHave support proofs.Apr 15 2021, 13:44

Harbormaster completed remote builds in B15521: Diff 28195.Apr 15 2021, 13:56

rebase onto master (this diff has no dependency)

Harbormaster completed remote builds in B15530: Diff 28206.Apr 19 2021, 08:17

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

This revision now requires changes to proceed.Apr 19 2021, 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.

Harbormaster completed remote builds in B15532: Diff 28208.Apr 19 2021, 12:15

Harbormaster completed remote builds in B15533: Diff 28209.Apr 19 2021, 12:20

PiRK edited the summary of this revision. (Show Details)Apr 19 2021, 19:36

deadalnix requested changes to this revision.Apr 20 2021, 00:01

deadalnix added a subscriber: deadalnix.

deadalnix added inline comments.

src/avalanche/peermanager.cpp
179 ↗	(On Diff #28209)	Why is this checking for this specific state?
208 ↗	(On Diff #28209)	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 ↗	(On Diff #28209)	How does `CRollingBloomFilter` ends up being declared here?
195 ↗	(On Diff #28209)	This is the section you are in, this comment is redundant.

This revision now requires changes to proceed.Apr 20 2021, 00:01

PiRK retitled this revision from [avalanche] Remember recently rejected proofs to [avalanche] Remember rejected proofs.Apr 20 2021, 09:57

PiRK added inline comments.Apr 20 2021, 13:31

src/avalanche/peermanager.cpp
179 ↗	(On Diff #28209)	Because the orphan proofs will go into another pool.
208 ↗	(On Diff #28209)	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).