Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

test: add test for unint64 overflow on limit #5713

Merged
merged 6 commits into from
Jan 24, 2024
Merged
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions modules/core/04-channel/keeper/keeper_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package keeper_test

import (
"fmt"
"math"
"reflect"
"testing"

Expand Down Expand Up @@ -811,6 +812,28 @@ func (suite *KeeperTestSuite) TestPruneAcknowledgements() {
},
nil,
},
{
"success: limit wraps around due to uint64 overflow",
func() {
// Send 10 packets from B -> A, creating 10 packet receipts and 10 packet acks on A.
suite.sendMockPackets(path, 10, true)
},
func() {
limit = math.MaxUint64
},
func(pruned, left uint64) {
// Nothing should be pruned, by passing in a limit of math.MaxUint64, overflow occurs
// when initializing end to pruningSequenceStart + limit. This results in end always being
// equal to start - 1 and thereby not entering the for loop.
// We expect 10 acks, 10 receipts and pruningSequenceStart == 1 (loop not entered).
Comment on lines +825 to +828
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

crazy, so it doesn't panic, it just wraps around. I guess by construction this is safe. Should we add an in-line comment in the code in case someone tries to refactor it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

absolutely! Added a small note, feel free (you or anyone else) to expand on my dry-ness 😅

postPruneExpState(10, 10, 1)

// We expect 0 to be pruned and 10 left.
suite.Require().Equal(uint64(0), pruned)
suite.Require().Equal(uint64(10), left)
},
nil,
},
{
"failure: packet sequence start not set",
func() {},
Expand Down
Loading