Add test coverage for oversized block messages
ClosedPublic
Actions

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

Details

Reviewers

Fabien
PiRK

Group Reviewers

Restricted Owners Package	(Owns No Changed Paths)
Restricted Project

Commits

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

Repository

rABC Bitcoin ABC

Branch

oversized-block-tests

Lint

Lint Errors

Severity	Location	Code	Message
Error	test/functional/p2p_invalid_messages.py:296	E306	flake8 E306

Unit

No Test Coverage

Build Status

Buildable 29335
Build 58206: Build Diff	build-without-wallet · build-diff · build-debug · build-clang · build-clang-tidy
Build 58205: arc lint + arc unit

Owners added a reviewer: Restricted Owners Package.Jun 17 2024, 21:01

roqqit requested review of this revision.Jun 17 2024, 21:01

Fabien requested changes to this revision.Jun 18 2024, 06:40

Fabien added a subscriber: Fabien.

Fabien added inline comments.

test/functional/p2p_invalid_messages.py
315 ↗	(On Diff #48286)	I still don't think you need blockmaxsize, which is only used for mining

This revision now requires changes to proceed.Jun 18 2024, 06:40

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

test/functional/p2p_invalid_messages.py
294–295 ↗	(On Diff #48286)
296 ↗	(On Diff #48286)	Maybe add a comment that `size - 5` works for this particular compact size range `0x10000 <= size < 0x100000000`.

PiRK added inline comments.Jun 18 2024, 07:11

test/functional/p2p_invalid_messages.py
315 ↗	(On Diff #48286)	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 ↗	(On Diff #48286)	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.