fix(sei-tendermint): restore proposal tx-key decode bounds (CON-334) - #4011
fix(sei-tendermint): restore proposal tx-key decode bounds (CON-334)#4011shemnon wants to merge 2 commits into
Conversation
Reject truncated or padded hashes and cap the list at consensus.max-tx-keys-per-proposal (default 1000), loaded through the existing config path rather than a package-level env read. The check runs at decode, before per-entry conversion, so a 4MB gossip message cannot force an unbounded allocation. Co-authored-by: Cursor <cursoragent@cursor.com>
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 34f83ae. Configure here.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4011 +/- ##
==========================================
- Coverage 59.67% 58.61% -1.07%
==========================================
Files 2261 2162 -99
Lines 194324 182736 -11588
==========================================
- Hits 115967 107112 -8855
+ Misses 67631 65840 -1791
+ Partials 10726 9784 -942
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
The restored tx-key validation (32-byte hashes, per-proposal count cap) is wired correctly through every decode path and well tested, but the 1000 default is far below the number of transactions a valid block can carry, so an honest large proposal would be rejected by every peer. Smaller notes on WAL-replay coupling to the live limit, the unregistered flag/env claim, and godoc style.
Findings: 1 blocking | 3 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- [suggestion] The PR description says the limit resolves through "defaults, file, env, flags", but no
consensus.max-tx-keys-per-proposalflag is registered inAddNodeFlags(sei-tendermint/cmd/tendermint/commands/run_node.go), and viper'sAutomaticEnvdoes not surface a key duringUnmarshalunless it is bound or present in the file — so for a pre-existing config.toml that omits the key, neither the env var nor a flag can override it. Either register the flag (asconsensus.gossip-tx-key-onlyandconsensus.double-sign-check-heightalready are) or drop the env/flag claim. - [suggestion] The new godocs carry rationale rather than description, which
AGENTS.md("Godoc") rules out: the second paragraph ofTxHashesListFromProto(sei-tendermint/types/mempool.go) explains why the count is checked first, and the added paragraph onconfix.CheckValidexplains the mechanism and the rejected alternative. Both belong as inline comments at the lines they justify; keep the godocs to what the function is. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
| DoubleSignCheckHeight: int64(0), | ||
| // Sei Configurations | ||
| GossipTransactionKeyOnly: true, | ||
| MaxTxKeysPerProposal: 1000, |
There was a problem hiding this comment.
[blocker] A default of 1000 makes this an effective cap on transactions per block, and it is well below what a valid block can hold. Proposal.TxHashes is block.GetTxHashes() for the whole block (internal/consensus/state.go:1263), set unconditionally, and it is part of the signed proposal — so a receiver can only reject, never trim.
Failure scenario: a proposer builds a legitimate block with 1200 transactions. Every peer's MsgFromProto returns "too many tx hashes in proposal", handleDataMessage drops the proposal, nobody prevotes for the block, and the round fails; the same happens for the next proposer under the same load, so the chain stalls until an operator edits config.toml on every node. This is reachable today: app/genesis/chains/atlantic-2.json sets block.max_gas = -1, leaving the 21MB max_bytes as the only cap (tens of thousands of small txs), and the codebase's own per-block tx bound elsewhere is 2000 (sei-tendermint/autobahn/types/block.go: MaxTxsPerBlock).
As the field comment above notes, the 4MB channel cap (RecvMessageCapacity, internal/consensus/reactor.go:89) already bounds the list at ~116k entries, i.e. a few MB of conversion — so sizing the default at or near that bound keeps the DoS guard while making it impossible for a valid proposal to trip it. A per-node value that gates acceptance of an otherwise-valid signed proposal is also a divergence knob: a validator set with heterogeneous values will disagree on whether a block is proposable at all.
| cs.replayMode = true | ||
| defer func() { cs.replayMode = false }() | ||
| gotHeight, msgs, err := cs.wal.ReadLastHeightMsgs() | ||
| gotHeight, msgs, err := cs.wal.ReadLastHeightMsgs(cs.config.MaxTxKeysPerProposal) |
There was a problem hiding this comment.
[suggestion] Replay applies the current limit to WAL entries that were written under the previous one. Every proposal in the WAL already passed this check when it was recorded (or was produced locally), so re-checking it adds no protection and converts a config edit into a start failure: lower max-tx-keys-per-proposal below a recorded proposal's key count and catchupReplay returns an error, so the node cannot start until the WAL is cleared. Consider replaying with an unbounded (or math.MaxInt) limit and keeping the bound on the peer-message path in Reactor.handle*Message, where the untrusted input actually arrives.
Change default to 2000 (autobahn max) and replay to infinite (blocks are already valid, so don't mark as invalid)
| GossipTransactionKeyOnly bool `mapstructure:"gossip-tx-key-only"` | ||
| // Largest number of transaction hashes a proposal received from a peer may carry. | ||
| // The consensus channels cap a message at 4MB, which admits roughly 116,000 hashes, | ||
| // so the message size does not bound the decode on its own. |
There was a problem hiding this comment.
nit: how much of the original comment in sei-protocol/sei-tendermint#292 apply here?
| // Largest number of transaction hashes a proposal received from a peer may carry. | ||
| // The consensus channels cap a message at 4MB, which admits roughly 116,000 hashes, | ||
| // so the message size does not bound the decode on its own. | ||
| MaxTxKeysPerProposal int `mapstructure:"max-tx-keys-per-proposal"` |
There was a problem hiding this comment.
@masih is the original intent just refusing a super long proposal from a peer? We recently added wireguard, should we use that?
I'm a bit worried that people can add different limits in their local nodes, then the consensus proposal may be accepted at some places but not others, we are back to "people may override consensus params" situation.
Maybe a better solution to set a limit using wireguard so people at least use the same limit. But then we need to make the limit reasonably large so we never hit it in reality.
2 participants
Summary
consensus.max-tx-keys-per-proposal(default 1000).ConsensusConfigfield resolved through the existing config path (defaults, file, env, flags). Decode (TxHashesListFromProtoviaProposalFromProto/MsgFromProto/ WAL replay) refuses a longer list before converting entries.confix.CheckValidnow unmarshals overDefaultConfig(), matching node load, so a migratedconfig.tomlthat omits the new key still validates.Test plan
go test ./sei-tendermint/types/ ./sei-tendermint/config/ ./sei-tendermint/scripts/confix/go test ./sei-tendermint/internal/consensus/ -count=1[consensus]section withoutmax-tx-keys-per-proposalstill starts (default 1000)max-tx-keys-per-proposal = 0fails config validationMade with Cursor