Skip to content

wire: use maxprotocol message len on msg reject#2518

Merged
Roasbeef merged 2 commits intobtcsuite:masterfrom
kcalvinalvin:2026-04-06-use-maxprotocol-message-len-on-msg-reject
Apr 8, 2026
Merged

wire: use maxprotocol message len on msg reject#2518
Roasbeef merged 2 commits intobtcsuite:masterfrom
kcalvinalvin:2026-04-06-use-maxprotocol-message-len-on-msg-reject

Conversation

@kcalvinalvin
Copy link
Copy Markdown
Collaborator

@kcalvinalvin kcalvinalvin commented Apr 6, 2026

Change Description

As we introduced a new MaxProtocolMessageLength, MsgReject max size
should also be lowered to MaxProtocolMessageLength.

If this isn't lowered and if we ever have a MsgReject message that is
bigger than MaxProtocolMessageLength, then the MaxPayloadLength() check
on the message will pass but it won't serialize in functions
WriteMessageWithEncodingN() and WriteV2MessageN() as both of these
functions have a separate check that each message isn't greater than
MaxProtocolMessageLength.

I really doubt that we'll ever trigger this but the code is technically incorrect and we should change this.

Follow up to #2504

Steps to Test

cd wire; go test

Pull Request Checklist

Testing

  • Your PR passes all CI checks.
  • Tests covering the positive and negative (error paths) are included.
  • Bug fixes contain tests triggering the bug to prevent regressions.

Code Style and Documentation

📝 Please see our Contribution Guidelines for further guidance.

erickcestari and others added 2 commits March 24, 2026 09:26
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).
As we introduced a new MaxProtocolMessageLength, MsgReject max size
should also be lowered to MaxProtocolMessageLength.

If this isn't lowered and if we ever have a MsgReject message that is
bigger than MaxProtocolMessageLength, then the MaxPayloadLength() check
on the message will pass but it won't serialize in functions
WriteMessageWithEncodingN() and WriteV2MessageN() as both of these
functions have a separate check that each message isn't greater than
MaxProtocolMessageLength.
Copy link
Copy Markdown
Contributor

@erickcestari erickcestari left a comment

Choose a reason for hiding this comment

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

LGTM!

By the way, is it worth keeping the reject message? The Core team removed support for it in 2019 (bitcoin/bitcoin#15437). It looks like it adds a lot of complexity, and other nodes don't listen for it either. So we may remove it later.

@saubyk saubyk added this to the v0.25.1 milestone Apr 6, 2026
@Roasbeef
Copy link
Copy Markdown
Member

Roasbeef commented Apr 6, 2026

By the way, is it worth keeping the reject message?

It's worth keeping for light clients IMO. As otherwise, you have no idea why your transaction wasn't accepted. Light clients don't have a mempool, and mempool policy is somewhat fast moving lately.

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 🫐

@Roasbeef Roasbeef merged commit 51e9b53 into btcsuite:master Apr 8, 2026
3 checks passed
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.

4 participants