Page MenuHomePhabricator

Extract the compact block processing into a generic helper class

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



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

rABC Bitcoin ABC
Lint Not Applicable
Tests Not Applicable

Event Timeline

deadalnix added inline comments.
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
18–19 ↗(On Diff #33820)


sdulfari requested changes to this revision.Jun 7 2022, 18:37
sdulfari added a subscriber: sdulfari.

The tests aren't comprehensive enough.

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
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 :)

Add a comment for the uneven bucket case, expand on the prefilled tx index out of bounds

53 ↗(On Diff #33855)

I got confused on the bucket size vs number of buckets. This makes sense.

78 ↗(On Diff #33855)


This revision is now accepted and ready to land.Jun 7 2022, 21:50