-
Notifications
You must be signed in to change notification settings - Fork 886
fix(sei-tendermint): restore proposal tx-key decode bounds (CON-334) #4011
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
55047a7
94149ba
3cd075a
40cc54f
afe5642
2d768fa
8471c91
23fe5f8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| package types | ||
|
|
||
| import ( | ||
| "crypto/sha256" | ||
| "testing" | ||
|
|
||
| "github.com/sei-protocol/sei-chain/sei-tendermint/libs/utils/require" | ||
| tmproto "github.com/sei-protocol/sei-chain/sei-tendermint/proto/tendermint/types" | ||
| ) | ||
|
|
||
| func TestTxHashFromProtoValidatesLength(t *testing.T) { | ||
| testCases := []struct { | ||
| name string | ||
| size int | ||
| }{ | ||
| {name: "empty", size: 0}, | ||
| {name: "short", size: sha256.Size - 1}, | ||
| {name: "long", size: sha256.Size + 1}, | ||
| } | ||
| for _, tc := range testCases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| _, err := TxHashFromProto(&tmproto.TxKey{TxKey: make([]byte, tc.size)}) | ||
| require.Error(t, err) | ||
| }) | ||
| } | ||
|
|
||
| _, err := TxHashFromProto(nil) | ||
| require.Error(t, err) | ||
|
|
||
| key := make([]byte, sha256.Size) | ||
| for i := range key { | ||
| key[i] = byte(i) | ||
| } | ||
| hash, err := TxHashFromProto(&tmproto.TxKey{TxKey: key}) | ||
| require.NoError(t, err) | ||
| require.Equal(t, TxHash(key), hash) | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,12 +6,16 @@ import ( | |
| gogoproto "github.com/gogo/protobuf/proto" | ||
| "github.com/stretchr/testify/require" | ||
|
|
||
| "github.com/sei-protocol/sei-chain/sei-tendermint/crypto" | ||
| "github.com/sei-protocol/sei-chain/sei-tendermint/internal/protoutils" | ||
| tmproto "github.com/sei-protocol/sei-chain/sei-tendermint/proto/tendermint/types" | ||
| "github.com/sei-protocol/sei-chain/sei-tendermint/types" | ||
| ) | ||
|
|
||
| const maxCommitSignatures = types.MaxVotesCount | ||
| const ( | ||
| maxCommitSignatures = types.MaxVotesCount | ||
| txKeySize = crypto.HashSize | ||
| ) | ||
|
|
||
| func marshal(t *testing.T, m gogoproto.Message) []byte { | ||
| t.Helper() | ||
|
|
@@ -54,6 +58,37 @@ func lcaeEvidence(n int) *tmproto.Evidence { | |
| return &ev | ||
| } | ||
|
|
||
| func txKeyWith(n int) *tmproto.TxKey { | ||
| return &tmproto.TxKey{TxKey: make([]byte, n)} | ||
| } | ||
|
|
||
| func proposalSizeWithTxKeys(t *testing.T, n int) int { | ||
| t.Helper() | ||
| txKeys := make([]*tmproto.TxKey, n) | ||
| for i := range txKeys { | ||
| txKeys[i] = txKeyWith(txKeySize) | ||
| } | ||
| return len(marshal(t, &tmproto.Proposal{TxKeys: txKeys})) | ||
| } | ||
|
|
||
| // MaxTxKeysPerProposal must sit just above the largest tx key count the | ||
| // consensus channel can carry. The transport enforces that budget while | ||
| // reassembling a message, before the wireguard scan runs, so a cap at or above | ||
| // this point can only reject proposals that were never deliverable. The lower | ||
| // 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tx-key cap test asserts inverted boundMedium Severity
Additional Locations (1)Reviewed by Cursor Bugbot for commit 23fe5f8. Configure here.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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”
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the bot meant the tests should be:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| } | ||
|
|
||
| func TestSchemaForTxKey_AcceptsAtCap(t *testing.T) { | ||
| require.NoError(t, protoutils.Scan[*tmproto.TxKey](marshal(t, txKeyWith(txKeySize)))) | ||
| } | ||
|
|
||
| func TestSchemaForTxKey_RejectsOverCap(t *testing.T) { | ||
| require.Error(t, protoutils.Scan[*tmproto.TxKey](marshal(t, txKeyWith(txKeySize+1)))) | ||
| } | ||
|
|
||
| func TestSchemaForBlock_AcceptsLastCommitAtCap(t *testing.T) { | ||
| require.NoError(t, protoutils.Scan[*tmproto.Block](marshal(t, | ||
| consensusAssembledBlock(commitWith(maxCommitSignatures))))) | ||
|
|
||


There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still need this check if we are going wireguard?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, wireguard has no min size or exact size constraint. If there was a min_size we could drop the in-code check.