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.

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

Fix length limit in ExternalStorage.get #241

Merged
merged 5 commits into from Apr 8, 2020

Conversation

webmaster128
Copy link
Member

@webmaster128 webmaster128 commented Apr 8, 2020

Closes #126

  1. Length limit for the result of ExternalStorage.get was increased to 128 KiB
  2. Contracts that really really (really really111!!) need it, can use ExternalStorage.get_with_result_length. This is inconvenient by design as it is not included in the Storage trait.

The usability of this change in blockchains is limited by CosmWasm/wasmvm#59

@webmaster128
Copy link
Member Author

The gas consumption changes slightly by this. This makes sense, since every 64 KiB the Wasm code needs to grow the dynamic memory by one page, which is a Wasm instruction.

---- instance::test::contract_deducts_gas_handle stdout ----
handle used: 59498
thread 'instance::test::contract_deducts_gas_handle' panicked at 'assertion failed: `(left == right)`
  left: `59498`,
 right: `59373`', packages/vm/src/instance.rs:381:9

I don't rebuild the test contract now to avoid conflicts with other open PRs. But it works locally.

@ethanfrey
Copy link
Member

100 gas to change alloc from 2kb to 128kb seems quite cheap.
Happy with that cost so that no one can ever complain about too small data sizes.

@ethanfrey
Copy link
Member

Happy if you can merge in my iterator PR and then rebuild the test contracts, then we should be good to merge.

Copy link
Member

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

Looks good. And nice solution to the issue.

I would adjust some of the values, but otherwise good.

packages/std/src/imports.rs Show resolved Hide resolved
@@ -148,8 +155,8 @@ impl Iterator for ExternalIterator {
type Item = KV;

fn next(&mut self) -> Option<Self::Item> {
let key_ptr = alloc(MAX_READ);
let value_ptr = alloc(MAX_READ);
let key_ptr = alloc(RESULT_BUFFER_LENGTH);
Copy link
Member

Choose a reason for hiding this comment

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

Key_ptr can be FAR FAR less than 128KB.
I think making another constant for this and setting it to eg 2KB is more than overkill.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there any specific reason to make such assumptions? I mean, an application could choose to store all its data in keys with empty values. Why not? Just because this does not happen in the average case, should we make more restrictions than necessary?

In any case, I think it is good to have separate constants. Created

/// The number of bytes of the memory region we pre-allocate for the result data in ExternalIterator.next
static DB_READ_KEY_BUFFER_LENGTH: usize = 64 * KI;

now to prove the point that they are independent. Happy to change the value to something small if there is a good reason to.

@ethanfrey ethanfrey added this to In progress in Blockchain Development via automation Apr 8, 2020
@webmaster128 webmaster128 force-pushed the ExternalStorage.get-result-length branch from d0ef4a2 to 76e1972 Compare April 8, 2020 13:50
@webmaster128 webmaster128 force-pushed the ExternalStorage.get-result-length branch from 76e1972 to d704f68 Compare April 8, 2020 16:30
Copy link
Member

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

nice work

packages/std/src/imports.rs Show resolved Hide resolved
@webmaster128 webmaster128 merged commit df4c026 into master Apr 8, 2020
Blockchain Development automation moved this from In progress to Done Apr 8, 2020
@webmaster128 webmaster128 deleted the ExternalStorage.get-result-length branch April 8, 2020 20:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Retry database read when allocated memory is too small
2 participants