Page MenuHomePhabricator

Extract the compact block processing into a generic helper class
ClosedPublic

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

Details

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
Branch
factorize_compact_proofs_blocks
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 19258
Build 38263: Build Difflint-circular-dependencies · build-clang · build-clang-tidy · build-without-wallet · build-debug · build-diff
Build 38262: arc lint + arc unit

Event Timeline

deadalnix added inline comments.
src/shortidprocessor.h
18–19

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
src/shortidprocessor.h
18–19

TIL

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

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

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

src/test/shortidprocessor_tests.cpp
53 ↗(On Diff #33855)

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

78 ↗(On Diff #33855)

touche

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