Page MenuHomePhabricator

Add test coverage for oversized block messages
ClosedPublic

Authored by roqqit on Mon, Jun 17, 21:01.

Details

Reviewers
Fabien
PiRK
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Commits
rABCf6dc88d5a2f5: Add test coverage for oversized block messages
Summary

Oversized blocks are treated differently from bitcoin core, so extra test coverage near boundaries is warranted.

Test Plan
./test/functional/test_runner.py p2p_invalid_messages

Diff Detail

Repository
rABC Bitcoin ABC
Branch
oversized-block-tests
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 29331
Build 58198: Build Diffbuild-without-wallet · build-debug · build-clang · build-diff · build-clang-tidy
Build 58197: arc lint + arc unit

Event Timeline

Owners added a reviewer: Restricted Owners Package.Mon, Jun 17, 21:01
roqqit requested review of this revision.Mon, Jun 17, 21:01
Fabien requested changes to this revision.Tue, Jun 18, 06:40
Fabien added a subscriber: Fabien.
Fabien added inline comments.
test/functional/p2p_invalid_messages.py
315

I still don't think you need blockmaxsize, which is only used for mining

This revision now requires changes to proceed.Tue, Jun 18, 06:40
PiRK added a subscriber: PiRK.

The code looks goods. For future maintenance and ease of review, some additional comments would help.

test/functional/p2p_invalid_messages.py
294–295
296

Maybe add a comment that size - 5 works for this particular compact size range 0x10000 <= size < 0x100000000.

test/functional/p2p_invalid_messages.py
315

The node won't start if excessiveblocksize < DEFAULT_MAX_BLOCK_SIZE.
Here also a comment would be useful for future maintenance.

test/functional/p2p_invalid_messages.py
296

I am considering a patch that bumps this arbitrary value to str_data=b'\xff' everywhere because I ran into the limit when writing the new tests. I can include the comment in that patch.

Added comments and log message

Fabien requested changes to this revision.Tue, Jun 18, 17:48

Please fix the linter warning, otherwise LGTM

This revision now requires changes to proceed.Tue, Jun 18, 17:48
This revision is now accepted and ready to land.Wed, Jun 19, 06:19