Skip to content

wire: separate protocol message limit from serialization bound#2504

Open
erickcestari wants to merge 1 commit intobtcsuite:masterfrom
erickcestari:fix-max-message-payload
Open

wire: separate protocol message limit from serialization bound#2504
erickcestari wants to merge 1 commit intobtcsuite:masterfrom
erickcestari:fix-max-message-payload

Conversation

@erickcestari
Copy link
Copy Markdown
Contributor

Revert MaxMessagePayload to 32MiB and introduce MaxProtocolMessageLength (~4MB) for p2p network message size enforcement. This mirrors Bitcoin Core's separation between MAX_SIZE (32MiB serialization bound) and MAX_PROTOCOL_MESSAGE_LENGTH (~4MB network limit) introduced in bitcoin/bitcoin#5843.

The previous commit reduced MaxMessagePayload to 4MB, but that constant is also used as a serialization bound for deriving maxTxInPerMessage, maxTxOutPerMessage, and variable-length string limits in contexts beyond network messages (e.g. database deserialization via MsgTx.Deserialize). While consensus limits keep real values well below the 4MB-derived bounds, conflating the two constants is architecturally incorrect and diverges from Bitcoin Core's design.

The new MaxProtocolMessageLength is now enforced in all four network read/write paths: WriteMessageN, WriteMessageWithEncodingN, ReadMessageWithEncodingN, and ReadV2MessageN (which previously had no overall message size check).

closes #2503

@saubyk saubyk requested a review from kcalvinalvin March 23, 2026 15:16
@saubyk saubyk added this to the v0.25.1 milestone Mar 23, 2026
@saubyk saubyk requested a review from Roasbeef March 24, 2026 00:13
Copy link
Copy Markdown
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🥦

Copy link
Copy Markdown
Collaborator

@kcalvinalvin kcalvinalvin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we also replace with MaxMessagePayload here:

btcd/wire/msgreject.go

Lines 172 to 180 in b950e5b

// The reject message did not exist before protocol version
// RejectVersion.
if pver >= RejectVersion {
// Unfortunately the bitcoin protocol does not enforce a sane
// limit on the length of the reason, so the max payload is the
// overall maximum message payload.
plen = MaxMessagePayload
}

wire/message.go Outdated
if len(plaintext) > MaxProtocolMessageLength {
return nil, nil, fmt.Errorf("payload exceeds max protocol "+
"message length - %d bytes, max %d", len(plaintext),
MaxProtocolMessageLength)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Everywhere else we return messageError() but here we return fmt.Errof(). Might be worth aligning to avoid error type check errors later down the road

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ReadV2MessageN function it's only using fmt.Errorf(), but I'll refactor to return messageError() instead.

Revert MaxMessagePayload to 32MB and introduce MaxProtocolMessageLength
(~4MB) for p2p network message size enforcement. This mirrors Bitcoin
Core's separation between MAX_SIZE (32MB serialization bound) and
MAX_PROTOCOL_MESSAGE_LENGTH (~4MB network limit) introduced in
bitcoin/bitcoin#5843.

The previous commit reduced MaxMessagePayload to 4MB, but that constant
is also used as a serialization bound for deriving maxTxInPerMessage,
maxTxOutPerMessage, and variable-length string limits in contexts beyond
network messages (e.g. database deserialization via MsgTx.Deserialize).
While consensus limits keep real values well below the 4MB-derived
bounds, conflating the two constants is architecturally incorrect and
diverges from Bitcoin Core's design.

The new MaxProtocolMessageLength is now enforced in all four network
read/write paths: WriteMessageN, WriteMessageWithEncodingN,
ReadMessageWithEncodingN, and ReadV2MessageN (which previously had no
overall message size check).
@erickcestari erickcestari force-pushed the fix-max-message-payload branch from 9c64dec to 5e96c5b Compare March 24, 2026 12:35
@erickcestari
Copy link
Copy Markdown
Contributor Author

Shouldn't we also replace with MaxMessagePayload here:

Sure, but I thinks is work for a follow-up PR to audit all MaxPayloadLength implementations and ensure none exceed MaxProtocolMessageLength. The MsgReject case isn't a bug since the protocol-level check in the read/write paths already caps messages at 4MB, but aligning per-message limits is worth its own review since it touches many files.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MaxMessagePayload incorrectly lowered to 4MB for all serialization.

4 participants