Skip to content
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.

Sign up for GitHub

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

feat!: limit the length of key/value #14645

Merged
merged 15 commits into from
Mar 17, 2023
Merged

feat!: limit the length of key/value #14645

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
7652876
limit the length of key/value
yihuang Jan 17, 2023
9296c61
Update CHANGELOG.md
yihuang Jan 17, 2023
13edf2b
Update store/types/validity.go
yihuang Jan 17, 2023
5e6bec2
add comment
yihuang Jan 17, 2023
ccd0f78
Update store/types/validity.go
yihuang Jan 18, 2023
cf765ec
comment
yihuang Jan 18, 2023
447c365
fix error message
yihuang Jan 22, 2023
35dce19
Merge branch 'main' into main
yihuang Jan 26, 2023
fd4d0ac
update size limit
yihuang Jan 26, 2023
4c30239
add parentheses
yihuang Jan 26, 2023
ecf2f83
Merge branch 'main' into main
alexanderbez Jan 26, 2023
e3c2d75
Merge branch 'main' into main
yihuang Jan 27, 2023
2fa8200
const -> var
yihuang Jan 27, 2023
a4b3e97
Merge branch 'main' into main
yihuang Feb 7, 2023
b97cbfe
Merge branch 'main' into main
tac0turtle Mar 17, 2023
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* (x/feegrant) [#14294](https://github.com/cosmos/cosmos-sdk/pull/14294) Moved the logic of rejecting duplicate grant from `msg_server` to `keeper` method.
* (store) [#14378](https://github.com/cosmos/cosmos-sdk/pull/14378) The `CacheKV` store is thread-safe again, which includes improved iteration and deletion logic. Iteration is on a strictly isolated view now, which is breaking from previous behavior.
* (x/staking) [#14590](https://github.com/cosmos/cosmos-sdk/pull/14590) `MsgUndelegateResponse` now includes undelegated amount. `x/staking` module's `keeper.Undelegate` now returns 3 values (completionTime,undelegateAmount,error) instead of 2.
* (store)[#14645](https://github.com/cosmos/cosmos-sdk/pull/14645) Add limit to the length of key and value.

### API Breaking Changes

Expand Down
19 changes: 16 additions & 3 deletions store/types/validity.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,28 @@
package types

// AssertValidKey checks if the key is valid(key is not nil)
const (
// 128K - 1
MaxKeyLength = 1<<17 - 1
// 2G - 1
MaxValueLength = 1<<31 - 1
Copy link
Member

Choose a reason for hiding this comment

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

Both of those values are off. Can we just write 128 * 1024 and 2 * 1024 * 1024 * 1024 and live a happy life?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was just think about how many bits are needed to encode the length, that's why I prefer all 1s value.

Copy link
Member

Choose a reason for hiding this comment

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

Cool, but 1<<17 is not 2^17 so I wonder if this notation is helpful.

Anyways, leave it up to you folks. But right now the comment and the values do not fit together.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

https://go.dev/play/p/8pC3XGTDRZw
I think they are the same values?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, my bad sorry! I take everything back.

But nice TIL: In python the operator precedence is different:

>>> # What I tried
>>> 1<<17 - 1
65536

>>> # What Go does
>>> (1<<17) - 1
131071

>>> # What Python does
>>> 1<<(17 - 1)
65536

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see, maybe we better to add the parentheses.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that's helpful, thanks. Python, Rust and JavaScript evaluate one way. Go and Swift evaluate a different way.

)

// AssertValidKey checks if the key is valid(key is not nil, not empty and within length limit)
func AssertValidKey(key []byte) {
if len(key) == 0 {
panic("key is nil")
panic("key is nil or empty")
}
if len(key) > MaxKeyLength {
panic("key is too large")
}
}

// AssertValidValue checks if the value is valid(value is not nil)
// AssertValidValue checks if the value is valid(value is not nil and within length limit)
func AssertValidValue(value []byte) {
if value == nil {
panic("value is nil")
}
if len(value) > MaxValueLength {
panic("value is too large")
}
tac0turtle marked this conversation as resolved.
Show resolved Hide resolved
}
Comment on lines +10 to 28
Copy link

Choose a reason for hiding this comment

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

Valid functions like these need to return errors rather than panicking, so that callers can detect when the validation checks have failed. Otherwise, there's no reason to have these functions at all, because any access of invalid values would trigger panics, anyway.