Set a protocol-wide mempool gossip transaction size (CON-420) - #4139
Set a protocol-wide mempool gossip transaction size (CON-420)#4139shemnon wants to merge 5 commits into
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
PR SummaryMedium Risk Overview Mempool admission and config: P2P reactor: Channel receive capacity is derived from the encoded max-gossip envelope (not config), with Mux layer: Tests cover mismatched Reviewed by Cursor Bugbot for commit fd5e6c9. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4139 +/- ##
==========================================
- Coverage 61.38% 60.41% -0.97%
==========================================
Files 2192 2075 -117
Lines 192289 178843 -13446
==========================================
- Hits 118038 108054 -9984
+ Misses 62752 60450 -2302
+ Partials 11499 10339 -1160
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Decoupling the mempool P2P receive capacity from local max-tx-bytes and discarding oversized envelopes instead of killing the connection is the right shape, and the new mux tests cover the discard state machine well. However, the checkTx-error blacklist still counts locally-oversized gossiped txs, so a node with a raised max-tx-bytes is still evicted by default-config peers after 50 txs — the connectivity break the PR sets out to remove, just delayed.
Findings: 1 blocking | 8 non-blocking | 5 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- [suggestion]
max-tx-bytesmay now exceedtypes.MaxGossipTxByteswith no validation or startup warning;MempoolConfig.ValidateBasiconly rejects negative values. Since a tx above the gossip cap is admitted locally but silently never propagated, a warning (or a hard error) at config validation would make the new coupling between the local knob and the protocol cap discoverable. - [suggestion] The new
len(tx) > types.MaxGossipTxBytesguard inhandleMempoolMessageis unreachable through the mux:RecvMessageCapacityis exactly the encoded size of aMessagecarrying oneMaxGossipTxBytestx, so any single tx above the cap already overflows the channel capacity and is discarded inpushMsg. It is reasonable defence-in-depth, but it is untested and cannot be exercised end-to-end — worth a direct unit test onhandleMempoolMessageif the branch is meant to be load-bearing. - [suggestion] During a mixed-version rollout, a node whose
max-tx-bytesis raised above 1MiB will gossip txs that exceed an un-upgraded peer's config-derivedRecvMessageCapacity, and that peer (lackingDiscardOversized) closes the multiplexed connection as before. Defaults keep this dormant, but it is worth calling out in the PR description as a rollout ordering constraint: raisemax-tx-bytesonly after the network has the new binary. - 4 suggestion(s)/nit(s) flagged inline on specific lines.
- 1 non-blocking pre-existing issue(s) listed below under pre-existing issues.
Pre-existing issues
- [suggestion]
recvChannelinsei-tendermint/internal/p2p/conn/oldmux.gosizes its byte accumulator withdesc.RecvBufferCapacity, which is documented as a message-queue depth, not a byte count (mempool passes 128). Harmless today since the slice grows, but the two units are conflated and the new discard path copies the same expression.
Oversized mempool envelopes are a handled path: skip them at debug with hash and peer, warn on mux discard with connection identity, and fold the mismatched max-tx-bytes fixture into the shared reactor setup. Co-authored-by: Cursor <cursoragent@cursor.com>
wen-coding
left a comment
There was a problem hiding this comment.
Looks good to me, the blocker flagged by seidroid seems worth addressing.
7386202 to
db5df2f
Compare
max-tx-bytes stays in toml but is unused for admission; a different value logs a warning. Gossip, recv, and CheckTx share MaxGossipTxBytes (1MiB, the former default).
db5df2f to
a36d25d
Compare
CheckTx cannot admit them, so the gossip list is seeded directly and a following legal tx proves the peer never received the oversized one.
|
@seidroid review |
There was a problem hiding this comment.
Moving the mempool gossip cap to a protocol constant (types.MaxGossipTxBytes) and making the mux discard oversized envelopes instead of tearing down the connection is a sound design, and it resolves the eviction-on-config-mismatch blocker from the previous review. Remaining notes are test-isolation/coverage and operator-facing documentation; no blocking defects found.
Findings: 0 blocking | 6 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion] The PR description no longer matches the diff: it says the gossip cap is 2MiB (the constant is 1MiB,
sei-tendermint/types/params.go) and that "Localmax-tx-bytesstill governs CheckTx admission" (admission now hard-codestypes.MaxGossipTxBytes). Since this silently changes admission behaviour for any operator with a non-defaultmax-tx-bytes, the description/release note should state the actual new semantics. - [suggestion] Rollout ordering note worth recording: an operator who lowered
max-tx-bytesbelow 1MiB now admits and gossips txs up to 1MiB after upgrading, while a not-yet-upgraded peer with the same lowered setting derivesRecvMessageCapacityfrom its own config and has noDiscardOversized, so it still closes the multiplexed connection. Default-configured networks are unaffected (1MiB before and after), but the constraint should be called out. - 3 suggestion(s)/nit(s) flagged inline on specific lines.
- 1 non-blocking pre-existing issue(s) listed below under pre-existing issues.
Pre-existing issues
- [suggestion]
newRecvChannel(sei-tendermint/internal/p2p/conn/oldmux.go:485) sizes the byte accumulator withdesc.RecvBufferCapacity, which is documented as a message-queue depth rather than a byte count (mempool passes 128), and it reads it off the pre-withDefaults()descriptor whilech.descstores the defaulted one. Harmless because the slice grows, but the two units are conflated.
|
|
||
| // Test: broadcast walks an oversized tx then a gossip-legal tx. | ||
| // Verify: the receiver gets only the legal tx. | ||
| require.Eventually(t, func() bool { |
There was a problem hiding this comment.
[suggestion] This test does not isolate the branch it names. Every assertion is receiver-side, and they all hold identically if the len(tx) > types.MaxGossipTxBytes skip in broadcastTxRoutine is deleted: Channel.Send has no send-side size check (sei-tendermint/internal/p2p/channel.go:81), so the oversized envelope goes out, the receiver's mux discards it because DiscardOversized is set, the connection survives (exactly what TestMConnectionDiscardOversized proves), and okTx still arrives. I removed the skip mentally and every require here still passes.
To make it load-bearing, assert on the sender side — e.g. open the receiver's channel with DiscardOversized: false so the un-skipped send would kill the connection, or count outbound mempool-channel messages/bytes — otherwise the only guard on this branch is the log statement.
| MaxGas: -1, | ||
| }, nil | ||
| }) | ||
| rts := setupReactorsWithConfig(ctx, t, 2, cfg, mempool.NopTxConstraintsFetcher) |
There was a problem hiding this comment.
[suggestion] Swapping the TxConstraints fetcher for NopTxConstraintsFetcher here (and at lines 295, 326, 508) drops the last reactor-level coverage of the consensus-derived ErrTxTooLarge from constraints.MaxDataBytes (mempool.go:315). That error still feeds accountFailedCheckTx and can evict a peer, and it is the path that survives real gossip — the protocol-oversize path this test now uses is unreachable over a live connection, since a tx above MaxGossipTxBytes always overflows RecvMessageCapacity and is discarded by the mux before handleMempoolMessage runs.
Keeping one case with a small MaxDataBytes (as before) alongside the new protocol-oversize case would preserve both. It also avoids allocating a 1MiB buffer per bad tx in a table-driven test.
|
|
||
| # Maximum size of a single transaction. | ||
| # NOTE: the max size of a tx transmitted over the network is {max-tx-bytes}. | ||
| # XXX: Unused. Admission uses the protocol gossip limit. A different value logs a warning. |
There was a problem hiding this comment.
[suggestion] This is operator-facing generated config, and the two lines contradict each other: "Maximum size of a single transaction." immediately followed by "Unused." It also never says what the effective limit actually is, so an operator reading their config.toml cannot tell whether their value matters or what replaced it.
Suggest stating the behaviour outright, e.g.: "Ignored. Transaction admission and gossip both use the protocol limit of 1MiB; setting a different value here logs a warning at startup." The same rewrite applies to the MaxTxBytes doc comments in sei-tendermint/config/config.go:887 and sei-tendermint/internal/mempool/mempool.go:56.
Superseded: latest AI review found no blocking issues.
Summary
types.MaxGossipTxBytes, 2MiB), not from localmax-tx-bytes.max-tx-bytesstill governs CheckTx admission. Autobahn/Giga is unchanged.Test plan
go test ./sei-tendermint/internal/p2p/conn/ ./sei-tendermint/internal/mempool/reactor/ ./sei-tendermint/types/