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

yihuang · 2023-01-17T10:37:47Z

Description

There are always limits on size of key/values in underlying database/OS/hardware, so instead of relying on those arbitrary and indeterministic limits, we should define a deterministic and reasonable one in application.

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

so the limit is deterministicly defined, rather than some arbitrary limit decided by underlying database.

CHANGELOG.md

store/types/validity.go

yihuang · 2023-01-17T10:47:21Z

store/types/validity.go

+
+const (
+	// 32K
+	MaxKeyLength = math.MaxUint16


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

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

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

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.

👆 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.

alexanderbez · 2023-01-17T15:39:45Z

store/types/validity.go

+	// 32K
+	MaxKeyLength = math.MaxUint16
+	// 2G
+	MaxValueLength = math.MaxUint32


This should cover all cases even in CW?

@alpe do you happen to know?

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.

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.

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.

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.

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.

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.

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.

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.

store/types/validity.go

peterbourgon · 2023-01-22T03:22:08Z

store/types/validity.go

+// 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")
+	}
 }


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.

alexanderbez

So what's the status of this? @alpe @webmaster128 do you think this is good as it stands?

webmaster128 · 2023-01-24T17:10:07Z

So what's the status of this? @alpe @webmaster128 do you think this is good as it stands?

I think the length limit for the keys introduced here is too low, given that there has not been any restriction in the past and leveldb supports arbitrary key lengths. I do understand that long keys are not desirable for performance reasons and I am happy to discourage the use of long keys from now on. But since the goal here is to get a deterministic limit, any other value works as well.

I suggest ~~126~~ 128 KiB (131072 bytes) for keys. This ensure all 64 KiB keys of CosmWasm + wrapping overhead remains supported.

yihuang · 2023-01-26T14:57:12Z

So what's the status of this? @alpe @webmaster128 do you think this is good as it stands?

I think the length limit for the keys introduced here is too low, given that there has not been any restriction in the past and leveldb supports arbitrary key lengths. I do understand that long keys are not desirable for performance reasons and I am happy to discourage the use of long keys from now on. But since the goal here is to get a deterministic limit, any other value works as well.

I suggest ~~126~~ 128 KiB (131072 bytes) for keys. This ensure all 64 KiB keys of CosmWasm + wrapping overhead remains supported.

done, now it's:

key: 128K - 1
value: 2G - 1

Minus one so the max length is all ones.

webmaster128

Thank you all for considering the CosmWasm use case ❤️

webmaster128 · 2023-01-26T16:30:24Z

store/types/validity.go

+	// 128K - 1
+	MaxKeyLength = 1<<17 - 1
+	// 2G - 1
+	MaxValueLength = 1<<31 - 1


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

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

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.

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

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

I see, maybe we better to add the parentheses.

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

testinginprod · 2023-01-27T09:30:51Z

store/types/validity.go

-// AssertValidKey checks if the key is valid(key is not nil)
+const (
+	// 128K - 1
+	MaxKeyLength = (1 << 17) - 1


I would have preferred these two constants to be variables that can be modified at startup, I think the key-lengths are reasonable but we don't have full knowledge of what other protocols are doing ( I doubt even 'them' have no clear idea of the size of their own keys)

EG in cmd/appd/main.go:

func main() { storetypes.MaxKeyLength = my safe limit storetypes.MaxValueLength = my safe limit rootCmd().Execute .... }

Sounds reasonable to me. It would allow new chains to be much more restrictive on key lengths for example. Existing chains can scan their keys in usage and put a limit based on their real world data.

sounds good, done.

testinginprod

Approving since the MaxKeyLength is now a non breaking change, but something that can be customized.

We should add a big changelog entry for this!

julienrbrt · 2023-01-27T17:20:50Z

Approving since the MaxKeyLength is now a non breaking change, but something that can be customized.

We should add a big changelog entry for this!

+1, and a section in the UPGRADING.md

peterbourgon · 2023-01-27T19:21:34Z

store/types/validity.go

+	// 2G - 1
+	MaxValueLength = (1 << 31) - 1


The default size of a LevelDB log file is 2MB and compaction occurs at 4MB. It is a core assumption of LevelDB operations, like compaction, that these files will contain many key/val pairs. Many promises of the database, including but not limited to performance, rely on that assumption.

The SDK uses the default settings (edit: specifically these default settings) when creating a GoLevelDB backend. If the SDK writes a single key/val pair of larger than 4MB, then it will occupy its own unique file. This may not be a problem if there are only a handful of key/val pairs that are that large, but if there are more than just a few, then it does become an issue.

Large keys definitely hurt performance because they are usually part of the index structure to do binary search, and need to be loaded into ram to be performant. I guess no one is actually using large keys in practice.
Maybe a better thing to do is setting a smaller limit in sdk, but third-parties can enlarge it since it's a variable, it's safer than having a bigger limit in sdk while third-party setting a smaller one. But we'll have more communication to do in this way, WDYT? @webmaster128 @testinginprod

CosmWasm never allowed values greater than 128 KiB. I don't know how much wrapping overhead there is in wasmd (probably none) and iavl. But I guess any limit >= 256 KiB would not hurt us.

Large keys

We are talking about values, right?

keys ;D I think there's less worries about large values

The 2-4MB limits I discuss above are impacted by the total size of a key/val pair, not just the key size or val size in isolation. My main point is that if you want to increase either of those limits, or both of them, to something approaching those 2-4MB limits, then you would also need to increase the limits of LevelDB itself; increasing the former without changes to the latter would result in really bad outcomes.

cool-develope

looks good to me

tac0turtle · 2023-03-08T23:34:43Z

wanted to give an update: after adr 038 is merged we will tag store package then merge this pr and work on integrating iavl changes.

tac0turtle · 2023-03-17T13:54:05Z

hey we just tagged v0.1.0 of store, this will be included in the next release so we can merge this now

limit the length of key/value

7652876

so the limit is deterministicly defined, rather than some arbitrary limit decided by underlying database.

yihuang requested a review from a team as a code owner January 17, 2023 10:37

github-actions bot added the C:Store label Jan 17, 2023

github-prbot requested review from a team, aaronc and testinginprod and removed request for a team January 17, 2023 10:38

yihuang commented Jan 17, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Update CHANGELOG.md

9296c61

yihuang commented Jan 17, 2023

View reviewed changes

store/types/validity.go Outdated Show resolved Hide resolved

yihuang added 2 commits January 17, 2023 18:42

Update store/types/validity.go

13edf2b

add comment

5e6bec2

yihuang commented Jan 17, 2023

View reviewed changes

yihuang changed the title ~~limit the length of key/value~~ feat: limit the length of key/value Jan 17, 2023

yihuang changed the title ~~feat: limit the length of key/value~~ feat!: limit the length of key/value Jan 17, 2023

tac0turtle assigned alexanderbez Jan 17, 2023

alexanderbez reviewed Jan 17, 2023

View reviewed changes

peterbourgon reviewed Jan 18, 2023

View reviewed changes

store/types/validity.go Outdated Show resolved Hide resolved

peterbourgon reviewed Jan 18, 2023

View reviewed changes

store/types/validity.go Show resolved Hide resolved

yihuang commented Jan 18, 2023

View reviewed changes

store/types/validity.go Outdated Show resolved Hide resolved

yihuang added 2 commits January 18, 2023 09:41

Update store/types/validity.go

ccd0f78

comment

cf765ec

webmaster128 reviewed Jan 20, 2023

View reviewed changes

store/types/validity.go Outdated Show resolved Hide resolved

fix error message

447c365

peterbourgon reviewed Jan 22, 2023

View reviewed changes

alexanderbez reviewed Jan 24, 2023

View reviewed changes

yihuang mentioned this pull request Jan 26, 2023

Problem: no compact storage for archive node crypto-org-chain/cronos#791

Merged

13 tasks

yihuang added 2 commits January 26, 2023 22:56

Merge branch 'main' into main

35dce19

update size limit

fd4d0ac

yihuang requested review from alpe, webmaster128, alexanderbez and tac0turtle January 26, 2023 14:58

webmaster128 reviewed Jan 26, 2023

View reviewed changes

add parentheses

4c30239

alexanderbez approved these changes Jan 26, 2023

View reviewed changes

Merge branch 'main' into main

ecf2f83

testinginprod reviewed Jan 27, 2023

View reviewed changes

yihuang added 2 commits January 27, 2023 17:43

Merge branch 'main' into main

e3c2d75

const -> var

2fa8200

testinginprod self-requested a review January 27, 2023 10:06

testinginprod approved these changes Jan 27, 2023

View reviewed changes

peterbourgon reviewed Jan 27, 2023

View reviewed changes

Merge branch 'main' into main

a4b3e97

yihuang mentioned this pull request Feb 9, 2023

Problem: versiondb is not ported to main branch crypto-org-chain/cronos#851

Merged

13 tasks

cool-develope approved these changes Feb 21, 2023

View reviewed changes

Merge branch 'main' into main

b97cbfe

tac0turtle enabled auto-merge (squash) March 17, 2023 14:05

tac0turtle merged commit 8944ab6 into cosmos:main Mar 17, 2023

faddat mentioned this pull request Mar 28, 2023

update linter to 1521 ChihuahuaChain/cosmos-sdk#6

Closed

19 tasks

coderabbitai bot mentioned this pull request Jun 28, 2024

feat(store): Parallel write in the CacheMultiStore #20817

Closed

12 tasks

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

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

Conversation

yihuang commented Jan 17, 2023 • edited Loading

Description

Author Checklist

Reviewers Checklist

yihuang Jan 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yihuang Jan 18, 2023 • edited Loading

Choose a reason for hiding this comment

yihuang Jan 18, 2023 • edited Loading

Choose a reason for hiding this comment

yihuang Jan 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alpe Jan 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderbez left a comment

Choose a reason for hiding this comment

webmaster128 commented Jan 24, 2023 • edited Loading

yihuang commented Jan 26, 2023

webmaster128 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

testinginprod left a comment • edited Loading

Choose a reason for hiding this comment

julienrbrt commented Jan 27, 2023

peterbourgon Jan 27, 2023 • edited Loading

Choose a reason for hiding this comment

yihuang Jan 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yihuang Jan 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cool-develope left a comment

Choose a reason for hiding this comment

tac0turtle commented Mar 8, 2023

tac0turtle commented Mar 17, 2023

yihuang commented Jan 17, 2023 •

edited

Loading

yihuang Jan 17, 2023 •

edited

Loading

yihuang Jan 18, 2023 •

edited

Loading

yihuang Jan 18, 2023 •

edited

Loading

yihuang Jan 18, 2023 •

edited

Loading

alpe Jan 20, 2023 •

edited

Loading

webmaster128 commented Jan 24, 2023 •

edited

Loading

testinginprod left a comment •

edited

Loading

peterbourgon Jan 27, 2023 •

edited

Loading

yihuang Jan 29, 2023 •

edited

Loading

yihuang Jan 30, 2023 •

edited

Loading