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 1 commit
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 @@ -154,6 +154,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* (x/group) [#13876](https://github.com/cosmos/cosmos-sdk/pull/13876) Fix group MinExecutionPeriod that is checked on execution now, instead of voting period end.
* (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.
* (store)[#]() Add limit to the length of key and value.
yihuang marked this conversation as resolved.
Show resolved Hide resolved

### API Breaking Changes

Expand Down
13 changes: 13 additions & 0 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

import math "math"

const (
MaxKeyLength = math.MaxUint16
MaxValueLength = math.MaxUint32
yihuang marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

This should cover all cases even in CW?

Copy link
Member

Choose a reason for hiding this comment

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

@alpe do you happen to know?

Copy link
Collaborator Author

@yihuang yihuang Jan 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The comment is wrong, the limit is 4G, since it's uint32, I think there was no limit before? we are collecting opinions on this.

Copy link
Collaborator Author

@yihuang yihuang Jan 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

MaxUint32 is 4G, yeah, it's a breaking change, but I believe it's a necessary one and need to be done sooner or later.

Copy link
Collaborator Author

@yihuang yihuang Jan 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

That's fine, but breaking changes have enormous and wide-ranging impact, and so must be motivated by something stronger than belief 😃 What concrete rationale supports this breaking change across all Cosmos networks?

deterministic state machine, as I explained in PR description. the goal is set a deterministic limit which are supported by all supported db backends, and can cover hopefully all existing use cases.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually we need to be a little bit smaller than hard limit, because iavl leaf node encode both key and value and a few other bytes together as value in low level db, I'd prefer 2G(aka. MaxInt32) if it's enough.

Copy link
Contributor

@alpe alpe Jan 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I got feedback from Mauro on CosmWasm limits for contracts. They are much lower than the suggested values: https://github.com/CosmWasm/cosmwasm/blob/main/packages/vm/src/imports.rs#L32-L35
Please note all contract data is stored in prefix store so the actual key/value length can be a bit bigger.

Copy link
Member

Choose a reason for hiding this comment

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

Note there is no good reason for having different values in MAX_LENGTH_DB_KEY and MAX_LENGTH_DB_VALUE in CosmWasm. It was a gut feeling from the early days and if contract developers exceed those they are most likely doing something wrong. For the next iteration of that code, I'd give them both the same value.

Copy link
Member

Choose a reason for hiding this comment

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

Good to know, thanks @peterbourgon.

In this case my only concern is that the current key limit is smaller than the limit that CosmWasm set before (64 KiB + wasmd prefix + iavl wrapper?) and we have no way to know who is using values close to this limit.

Copy link
Member

Choose a reason for hiding this comment

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

Congratulations to this clever analysis. It's not helping though. You asked for empirical data, worried about breaking and other people have been asking for our insights. And yes, in a smart contract scenario we cannot control what people are using right now within the currently enforced range.

)

// AssertValidKey checks if the key is valid(key is not nil)
func AssertValidKey(key []byte) {
if len(key) == 0 {
panic("key is nil")
webmaster128 marked this conversation as resolved.
Show resolved Hide resolved
}
if len(key) > MaxKeyLength {
panic("key is too large")
}
tac0turtle marked this conversation as resolved.
Show resolved Hide resolved
}

// AssertValidValue checks if the value is valid(value is not nil)
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
}