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 6 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 @@ -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)[#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: 17 additions & 2 deletions store/types/validity.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,30 @@
package types

// AssertValidKey checks if the key is valid(key is not nil)
import math "math"

const (
// 32K
MaxKeyLength = math.MaxUint16
Copy link
Collaborator Author

@yihuang yihuang Jan 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

MaxUint16 is picked so the length can be encoded in 2bytes with fixed length, should be more than enough for all use cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

2^16 bytes (64 KiB) is definitely not enough for CosmWasm values

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

value is 2^32, 2^16 is for keys.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are there any length limits for keys on DBs used that are shorter than 2^32? Personally, I don't see any reasons why we should have a shorter limit for keys than for values. The original motivation was deterministic behaviour.

Copy link
Member

Choose a reason for hiding this comment

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

👆 Data structures like sets (i.e. a map from key to nothing) store all its data in keys. So treating keys and value differently should be avoided if possible IMO.

// 4G
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 and within length limit)
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")
}
}

// 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
}