Fix message size check for max block sizes much smaller than max protocol message size
ClosedPublic
Actions

Authored by roqqit on Mon, Jun 17, 17:48.

Details

Reviewers

Group Reviewers

Restricted Owners Package	(Owns No Changed Paths)
Restricted Project

Commits

rABCb138ee23c41b: Fix message size check for max block sizes much smaller than max protocol…

Summary

This fixes a subtle bug: if max block size * 2 is less than max protocol message size, messages could be rejected.
This should not be an issue unless or until MAX_PROTOCOL_MESSAGE_LENGTH is made much larger.

Test Plan

ninja check check-functional

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

roqqit created this revision.Mon, Jun 17, 17:48

Owners added a reviewer: Restricted Owners Package.Mon, Jun 17, 17:48

Herald added a reviewer: Restricted Project. · View Herald TranscriptMon, Jun 17, 17:48

roqqit requested review of this revision.Mon, Jun 17, 17:48

Harbormaster completed remote builds in B29326: Diff 48278.Mon, Jun 17, 18:07

It's a nice code readability improvement and test coverage increase.

But I don't see the bug, if any. The test passes without the changes. It's normal for the block message to be larger than other messages in the protocol, hence the special case. But if the protocol size is increased more than twice the block size, then there is no reason to allow for a larger block message anyway (that would be a rejected block).

test/functional/p2p_invalid_messages.py
300 ↗	(On Diff #48278)
315 ↗	(On Diff #48278)	blockmaxsize is for mining only, not needed here
324 ↗	(On Diff #48278)	Dito

This revision now requires changes to proceed.Mon, Jun 17, 18:24

Addressed feedback and separated passing tests into its own diff: D16345

The actual behavior change was not captured by these tests. My mistake.

test/functional/p2p_invalid_messages.py
315 ↗	(On Diff #48278)	It's needed because blockmaxsize cannot be smaller than excessiveblocksize

Rebased on D16345 and introduce test coverage that breaks without the fix. It may look a bit strange at first blush to test non-oversized messages in this test, but checking boundaries is clearly a good idea here.

Harbormaster completed remote builds in B29334: Diff 48290.Tue, Jun 18, 16:31

Fabien requested changes to this revision.Tue, Jun 18, 20:02

Fabien added inline comments.

test/functional/p2p_invalid_messages.py
295 ↗	(On Diff #48290)	This is always False in all the tests, so this branch is never reached
299 ↗	(On Diff #48290)	I'm actually surprised it works, it means that the message you send (full of garbage) do not cause a disconnect ?
352 ↗	(On Diff #48290)	That's the default

This revision now requires changes to proceed.Tue, Jun 18, 20:02

roqqit added inline comments.Wed, Jun 19, 16:37

test/functional/p2p_invalid_messages.py
299 ↗	(On Diff #48290)	test_oversized_msg works on the same principle. These messages fail in deserialization which does not trigger a disconnect.
352 ↗	(On Diff #48290)	This is leftover from a refactor where I eliminated the need to have expect_disconnect.