Skip to content

Conversation

syntaxjak
Copy link

@syntaxjak syntaxjak commented Sep 15, 2025

Summary
Adds logging to surface P2P message sizes versus configured limits, so we can validate the 1x limits before changing enforcement (ref #2825). Also raises the stale Header size cap to 420 B (was 365 B) since headers on mainnet are consistently ~415 B.

What changed
• Inserted size logging in p2p/src/codec.rs after header decode:
• info! (or debug! for Ping/Pong) line: p2p(codec): size= (1x=, 4x=)
• warn! when msg_len > 1x (still under the existing 4× cap).
• Demoted Ping/Pong logs to debug! to avoid info-level spam in normal operation.
• Bumped Header 1× limit from 365 B → 420 B, and updated Headers batch accordingly.
• No wire format, consensus, or API changes; logging + cap maintenance only.

Why
Historically we allowed up to 4× the nominal limits for flexibility. With this patch we surface real sizes so limits can be tuned. Logs revealed that all headers today are ~415 B, making the old 365 B limit stale. Raising it to 420 B removes noisy warnings while keeping 4× safety margin unchanged.

How I tested

  • Built from source and ran a mainnet node.
  • Observed lines like:
    • p2p(codec): Ping size=16 (1x=16, 4x=64) (at DEBUG)
    • p2p(codec): CompactBlock size=1268 (1x=134803, 4x=539212) (at INFO)
    • p2p(codec): Header exceeds 1x limit (415 > 365) (at WARN)
  • Verified warn! only triggers when >1x, and that Ping/Pong no longer spam INFO.

Risks / Perf

  • Slightly more logging; minimal runtime cost. No protocol/consensus impact.

Notes

  • Builds on the intent of 4x limit on p2p msg lengths #2825 (originally opened by @antiochp), but implements logging in the codec so it covers all message types uniformly. Happy to tweak format/severity based on maintainer feedback.

@Anynomouss
Copy link

I will test this when I have time.
Sounds reasonable. All those Ping messages on the INFO level are also useful since it indicates we might have more ping messages than needed. I still want to test if syncing would speed up when decreasing the amount of ping messages to for example only ones per minute. I would prefer to reduce the amount of ping messages above demoting it do DEBUG level though.

This removes changes introducd by mimblewimble#3810 (AI-generated slop),
keeping only my own message size logging modifications.
@syntaxjak syntaxjak changed the title p2p: log message sizes; warn on >1x; demote Ping/Pong to debug (#2825) p2p: log message sizes; warn on >1x; demote Ping/Pong to debug (#2825) also fixes PIBD Sync bug Sep 26, 2025
@Anynomouss
Copy link

Anynomouss commented Oct 7, 2025

I tested this PR by syncing the node on main-net three times. The node synced just fine. One time I encountered a database error, but I think that was coinciding with me running out of HD space and after cleanup it did not reoccur.
I did not encounter any messages that exceeded the 420B threshold nor any restarts happening.

I did a 4th test run where I revert to the 365 Header size to confirm that I will indeed get Header messages that exceed the previous 365 threshold and indeed most headers messages are to large in size with the old threshold. Surprising that this was not caught earlier.

I did not measure any speed up, but most importantly I did not encounter any stalling/freezing of the node, so it appears that either A) this PR fixes the syncing stalled error or B) An attacker has stopped spamming the node network with messages that caused nodes to freeze up.

What was bothering me about the Header messaging as a whole. The threshold MAX_BLOCK_HEADERS = 512; should indicate that any peer can request a maximum of 512 Headers in one message request. So how is it possible that regular request for 32 blocks would exceed this threshold? It does not make sense to me. The limit appears to be a factor of 16 off.
=> Turns out that the message is triggered after 16x receiving 32 block headers. Perhaps I misinterpreted the MAX_BLOCK_HEADERS parameter. It means you are only allowed to consecutively get 512 Headers from a single peer, then the node is forced to switch to another peer. It is clear that Header sync is concurrent but not parallel since 16x requests are to a single peer, then switched to another peer. Instead we should have Header sync create multiple threads, delegating the headers to request to multiple peers.

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.

3 participants