Page MenuHomePhabricator

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

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

Details

Reviewers
Fabien
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

Event Timeline

Owners added a reviewer: Restricted Owners Package.Jun 17 2024, 17:48
roqqit requested review of this revision.Jun 17 2024, 17:48
Fabien requested changes to this revision.Jun 17 2024, 18:24
Fabien added a subscriber: Fabien.

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.Jun 17 2024, 18:24
roqqit planned changes to this revision.Jun 17 2024, 21:04

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.

Fabien requested changes to this revision.Jun 18 2024, 20:02
Fabien added inline comments.
test/functional/p2p_invalid_messages.py
295

This is always False in all the tests, so this branch is never reached

299

I'm actually surprised it works, it means that the message you send (full of garbage) do not cause a disconnect ?

352

That's the default

This revision now requires changes to proceed.Jun 18 2024, 20:02
test/functional/p2p_invalid_messages.py
299

test_oversized_msg works on the same principle. These messages fail in deserialization which does not trigger a disconnect.

352

This is leftover from a refactor where I eliminated the need to have expect_disconnect.

Cleanup refactor leftovers

This revision is now accepted and ready to land.Jun 19 2024, 21:03