fix(sei-tendermint): restore proposal tx-key decode bounds (CON-334) - #4011
Conversation
|
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}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4011 +/- ##
==========================================
- Coverage 61.30% 58.36% -2.95%
==========================================
Files 2163 2170 +7
Lines 188732 182469 -6263
==========================================
- Hits 115704 106493 -9211
- Misses 62305 66259 +3954
+ Partials 10723 9717 -1006
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.
| // 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.
There was a problem hiding this comment.
yes the original intent was to mitigate long proposals.
Either approach works. I would use wireguard if possible.
There was a problem hiding this comment.
Do you know what limit would work in production? Is 1000 far from what we will normally see?
There was a problem hiding this comment.
I went with 10_000, which is the wireguard limit for commit signatures (txes are not validator sigs). But TXKey is just a hash, so we have the same level of memory risk as with validator signatures.
There was a problem hiding this comment.
1K is plenty just now (thanks will for confirming).
Refuse more than 10,000 tx_keys on a gossiped proposal at scan, matching Commit.signatures, so a 4MB consensus message cannot force an unbounded TxKey slice. Native conversion still rejects hashes that are not 32 bytes. Co-authored-by: Cursor <cursoragent@cursor.com>
3684663 to
55047a7
Compare
|
@seidroid review |
There was a problem hiding this comment.
The approach was reworked from a config-driven limit to a wireguard max_count annotation plus a 32-byte length check in TxHashFromProto, which is a cleaner choke point and well covered by the new tests. The remaining blocker is that the 10,000 tx-key cap is not derived from any enforced block-level transaction limit, so an honest oversized proposal is rejected — and the sender disconnected — by every peer.
Findings: 1 blocking | 2 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- [suggestion] The PR title/description no longer match the diff: they describe
consensus.max-tx-keys-per-proposal, aConsensusConfigfield, theconfix.CheckValidchange, and a test plan over./sei-tendermint/config/and./sei-tendermint/scripts/confix/— none of which exist in the current changes (the limit is now awireguard.max_countproto annotation). Since this becomes the squash commit message, please rewrite it to describe the wire-schema approach and the actual test plan (./sei-tendermint/types/,./sei-tendermint/internal/consensus/). - [suggestion]
TxKey.tx_keystill has no size bound in the wire schema (types.wireguard.goregisters*TxKeyas1: {MaxCount: 1}), so the pre-decode layer accepts 10,000 arbitrarily-sized entries and the 32-byte invariant is only discovered after gogo unmarshals the whole message. Adding[(wireguard.max_size) = 32]toTxKey.tx_keywould put both bounds at the same choke point and matches the convention already used for 32-byte hash fields insei-tendermint/internal/autobahn/autobahn.proto(parent_hash,payload_hash,app_hash).
| if dp == nil { | ||
| return TxHash{}, errors.New("nil data") | ||
| } | ||
| if len(dp.TxKey) != sha256.Size { |
There was a problem hiding this comment.
Do we still need this check if we are going wireguard?
There was a problem hiding this comment.
It was in the source PR, and since the linear issue was about porting missed fixes and it was the same source file, it fits.
If we take it out I think short TX Bytes will leave trailing zeros, and the ToProto always creates a full size key.
Since the only way it would ordinarily be mismatched is via corruption or someone else writing it, I consider it defense in depth, and hence prefer it stays.
There was a problem hiding this comment.
also, wireguard has no min size or exact size constraint. If there was a min_size we could drop the in-code check.
PR SummaryMedium Risk Overview Proposal
Reviewed by Cursor Bugbot for commit 23fe5f8. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
@seidroid review |
There was a problem hiding this comment.
The 32-byte max_size bound on TxKey.tx_key and the length check in TxHashFromProto are correct and well tested, but the max_count = 2000 cap on Proposal.tx_keys is borrowed from Autobahn's MaxTxsPerBlock, which governs a code path that never emits this message — nothing bounds tx count on the classic consensus path, and the default mempool size (5000) is 2.5x the new cap, so an honest proposal can be rejected and its sender disconnected by every peer.
Findings: 1 blocking | 2 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- [suggestion] The PR description is out of sync with the diff: it says
(wireguard.max_count) = 10000(the value is now 2000, sourced fromatypes.MaxTxsPerBlock) and does not mention the new(wireguard.max_size) = 32bound onTxKey.tx_key. Since this becomes the squash commit message, please update both the summary and the test plan. - [suggestion] The new
max_size = 32bound onTxKey.tx_keyhas no wire-level test.consensusProposalMessageWithTxKeysbuilds&tmproto.TxKey{}with a nilTxKey, so every entry serializes to zero bytes and onlyMaxCountis exercised;mempool_test.gocovers the nativeTxHashFromProtoconversion, not the pre-decode layer. ATestSchemaForMessage_RejectsOversizedTxKeypopulating one entry with 33 bytes would pin the bound where it is actually enforced (protoutils/runtime/runtime.go:103).
…/con-334-auditor-feedback-immunefi-80676-regression-of-txkeys
Derive the max proposal numbers from other fixed constants.
|
@seidroid review |
There was a problem hiding this comment.
The tx-key cap is now derived from the consensus channel's own 4MB reassembly budget (MaxConsensusMsgBytes / txKeyEntryBytes = 116508) and pinned by tests, which makes it unreachable-by-construction and closes the previous blocker; the 32-byte max_size bound and the TxHashFromProto length check are correct and now covered at both the wire and native layers. The only remaining issue is the stale PR description.
Findings: 0 blocking | 1 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion] The PR description is out of sync with the diff for the third revision running. The summary still claims
(wireguard.max_count) = 10000, while the diff uses116508; it also omits the newtypes.MaxConsensusMsgBytes/types.MaxTxKeysPerProposalconstants insei-tendermint/types/proposal.go, themaxMsgSizeredefinition ininternal/consensus/reactor.go, and the(wireguard.max_size) = 32bound onTxKey.tx_key. The test plan also predatesTestMaxTxKeysPerProposalExceedsEveryDeliverableProposalandTestSchemaForTxKey_{AcceptsAtCap,RejectsOverCap}. Since this becomes the squash commit message, please rewrite both sections to describe the transport-derived cap that actually landed.
Superseded: latest AI review found no blocking issues.
…676-regression-of-txkeys
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 23fe5f8. Configure here.
| // bound keeps the cap tight enough to still bound decode work. | ||
| func TestMaxTxKeysPerProposalExceedsEveryDeliverableProposal(t *testing.T) { | ||
| require.Greater(t, proposalSizeWithTxKeys(t, types.MaxTxKeysPerProposal), types.MaxConsensusMsgBytes) | ||
| require.LessOrEqual(t, proposalSizeWithTxKeys(t, types.MaxTxKeysPerProposal-1), types.MaxConsensusMsgBytes) |
There was a problem hiding this comment.
Tx-key cap test asserts inverted bound
Medium Severity
TestMaxTxKeysPerProposalExceedsEveryDeliverableProposal requires MaxTxKeysPerProposal keys to encode larger than MaxConsensusMsgBytes, but MaxTxKeysPerProposal is MaxConsensusMsgBytes / txKeyEntryBytes (integer division). That product is 16 bytes under the 4MB budget, so the new tightness test fails.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 23fe5f8. Configure here.
There was a problem hiding this comment.
The largest proposal permitted by these changes is indeed 16 bytes under, which means it will be correctly delivered,. As each tx hash is 36 bytes (including protobuf overhead) adding one more makes the message too large. Hence we meet the constraint of “scan cannot reject a proposal the transport would have delivered”
There was a problem hiding this comment.
I think the bot meant the tests should be:
require.LessOrEqual(....(Max))
require.Greater(...(Max+1))
?
There was a problem hiding this comment.
It's off by (at least) one tx hash because of the protobuf overhead of the proposal, which is what the limits are demonstrating. Maybe 19 bytes?
But the core question of "can anything sent over consensus be unmarshaled within the wireguard limit" errs to max being less than one hash larger than 4 Gib.
3 participants
Summary
Proposal.tx_keysat decode with a wireguard maxCount derived from protocol maximums.TxHashFromProtoso truncated or padded keys are refused at native conversion. Also adds wireguard check for max size on tx_keysconsensus.Messageand length tests forTxHashFromProto.Test plan
go test ./sei-tendermint/internal/consensus/ -run 'TestSchemaForMessage_(AcceptsProposalTxKeysAtCap|RejectsProposalTxKeysOverCap)' -count=1go test ./sei-tendermint/types/ -run TestTxHashFromProtoValidatesLength -count=1go test ./sei-tendermint/internal/consensus/ ./sei-tendermint/types/ -count=1