Differential D11571

Extract the compact block processing into a generic helper class
ClosedPublic
Actions

Authored by Fabien on Jun 4 2022, 01:12.

Details

Reviewers

sdulfari

Group Reviewers

Restricted Project

Commits

rABCeb09c72ebc66: Extract the compact block processing into a generic helper class

Summary

This moves the logic out of the compact block into a new ShortIdProcessor that can be reused for the compact proofs. This also makes the code a bit easier to follow.

Depends on D11569.

Test Plan

ninja all check-all

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

Fabien created this revision.Jun 4 2022, 01:12

Herald added a reviewer: Restricted Project. · View Herald TranscriptJun 4 2022, 01:12

Fabien requested review of this revision.Jun 4 2022, 01:12

Fabien mentioned this in D11545: [avalanche] Request missing proofs from short ids.

Fabien added a child revision: D11545: [avalanche] Request missing proofs from short ids.Jun 4 2022, 01:13

Harbormaster completed remote builds in B19258: Diff 33820.Jun 4 2022, 01:27

deadalnix added a subscriber: deadalnix.Jun 6 2022, 12:32

deadalnix added inline comments.

src/shortidprocessor.h
18–19 ↗	(On Diff #33820)	I was guilty of not doing that myself, but you might want to use private inheritance to benefit from empty base optimization.

Fabien planned changes to this revision.Jun 7 2022, 07:52

Fabien added inline comments.Jun 7 2022, 09:09

src/shortidprocessor.h
18–19 ↗	(On Diff #33820)	TIL

Leverage EBO

Harbormaster completed remote builds in B19281: Diff 33855.Jun 7 2022, 09:41

Fabien mentioned this in D11574: Leverage Empty Base Optimization for the radix tree Adapter.Jun 7 2022, 09:48

The tests aren't comprehensive enough.

src/test/shortidprocessor_tests.cpp
53 ↗	(On Diff #33855)	Using 0 as the bucket size seems wrong for both of the above cases since it's not intuitive, even though it certainly makes writing it easy. It raises questions like: What does a size 0 mean? Is size 0 related to any real world case that is conceivable? I would also consider disallowing size 0 entirely but that should not block this diff.
73 ↗	(On Diff #33855)	This tests one specific case where the first index is out of bounds, but does not test for invalid indices following valid ones and vice versa.
78 ↗	(On Diff #33855)	What about shortid out of bounds indices?

This revision now requires changes to proceed.Jun 7 2022, 18:37

Fabien added inline comments.Jun 7 2022, 20:49

src/test/shortidprocessor_tests.cpp
53 ↗	(On Diff #33855)	0 is not a realistic value but is the only value that is guaranteed to trigger the flag all the time, since the number of bucket is set to the number of shortids. Other values depends on statistics and would make the test flaky. I'll add a comment for that.
78 ↗	(On Diff #33855)	I will not create a vector with max(uint32_t) elements to check this case :)