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

Add range to ReadonlyStorage #181

Merged
merged 41 commits into from Mar 27, 2020
Merged

Add range to ReadonlyStorage #181

merged 41 commits into from Mar 27, 2020

Conversation

ethanfrey
Copy link
Member

@ethanfrey ethanfrey commented Mar 24, 2020

Closes #53

This is working pretty well now. All tests pass and I am happy enough with the code structure.

TODO:

  • Add CHANGELOG entries

Do #188 later

Copy link
Member Author

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

Reponded

@ethanfrey ethanfrey marked this pull request as ready for review March 24, 2020 19:46
@ethanfrey
Copy link
Member Author

This is not done, but close (the wasm integration is left).

But before I get into C types and all, I would like a look over the API I am creating. (And yes, I want this with a feature flag for more general blockchain support, even if this ends up as a default-on flag).

@ethanfrey
Copy link
Member Author

Interesting... it seems that only the extern "C" functions that are reachable by rust code in the contract are required in the wasm build. I enabled the iterator feature and it didn't change the imports. I then modified code and it did quite a bit.

$ wasm-nm -i ./target/wasm32-unknown-unknown/release/queue.wasm 
i scan
i write_db
i next
$ wasm-nm -i ./target/wasm32-unknown-unknown/release/hackatom.wasm 
i read_db
i write_db
i canonicalize_address
i humanize_address

The feature flag seems unnecessary for building the wasm contracts, and we can always enable it there (default).

However, when working on the cosmwasm-vm and the functions it needs from the runtime, we need to clearly define this (as it is exposed or not exposed at link time, not when a contract is loaded)

Copy link
Member

@webmaster128 webmaster128 left a comment

Choose a reason for hiding this comment

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

Some random thoughts for now

.circleci/config.yml Outdated Show resolved Hide resolved
.circleci/config.yml Outdated Show resolved Hide resolved
contracts/queue/Cargo.toml Show resolved Hide resolved
contracts/queue/examples/schema.rs Show resolved Hide resolved
contracts/queue/tests/integration.rs Outdated Show resolved Hide resolved
contracts/queue/src/contract.rs Outdated Show resolved Hide resolved
packages/std/Cargo.toml Outdated Show resolved Hide resolved
packages/std/src/storage.rs Show resolved Hide resolved
packages/std/src/traits.rs Outdated Show resolved Hide resolved
packages/std/src/traits.rs Outdated Show resolved Hide resolved
@ethanfrey ethanfrey force-pushed the iterator branch 2 times, most recently from bd4a876 to 38b7ed5 Compare March 25, 2020 09:29
@ethanfrey
Copy link
Member Author

I think I've done all the plumbing, but now a SIGABRT when using the scan/next callbacks. I need to get into serious debug mode. (playing with unsafe code here).

One larger issue I found is when I write to two buffers, there is no clear way to return the length of both of them. In the write_db, we only wrote to one buffer and returned a single i32 as the length or error code.

Relevant lines to explain what I mean:

@webmaster128 would it make sense to "upgrade" the buffer from {offset, len} to {offset, len, cap} like we do on the Go FFI boundary? Then we can always write the proper length in the Region object while writing data back from rust to wasm.

If you like the idea and have time, I'd be happy if you could work on a PR on that. Otherwise I look tomorrow

@webmaster128
Copy link
Member

@webmaster128 would it make sense to "upgrade" the buffer from {offset, len} to {offset, len, cap} like we do on the Go FFI boundary? Then we can always write the proper length in the Region object while writing data back from rust to wasm.

If you like the idea and have time, I'd be happy if you could work on a PR on that. Otherwise I look tomorrow

That makes a lot of sense to me. Having two separate paths (data at pointer and return value) that need to be brought together in order to read data smaller than the whole region is no the nicest thing I can imagine.

The structure we end of with is by the way almost the same as a Rust vector

Most fundamentally, Vec is and always will be a (pointer, capacity, length) triplet. No more, no less.

https://doc.rust-lang.org/std/vec/struct.Vec.html#guarantees

I'm looking into this right now to understand the problem. Will at least start with something right now and push later.

@ethanfrey ethanfrey force-pushed the iterator branch 2 times, most recently from 436eeca to 798ca12 Compare March 26, 2020 10:16
contracts/queue/src/contract.rs Outdated Show resolved Hide resolved
contracts/queue/src/contract.rs Outdated Show resolved Hide resolved
contracts/queue/src/contract.rs Outdated Show resolved Hide resolved
contracts/queue/src/contract.rs Outdated Show resolved Hide resolved
contracts/queue/src/contract.rs Outdated Show resolved Hide resolved
contracts/queue/tests/integration.rs Outdated Show resolved Hide resolved
packages/std/src/imports.rs Outdated Show resolved Hide resolved
packages/std/src/imports.rs Outdated Show resolved Hide resolved
packages/std/src/imports.rs Show resolved Hide resolved
packages/vm/src/context.rs Outdated Show resolved Hide resolved
@webmaster128
Copy link
Member

Great stuff. Looking forward to finish this.

Do you have any idea what happens when you delete a database entry in the middle of a range query?

@ethanfrey
Copy link
Member Author

Thank you, I'm glad to get this in and add a few follow up PRs, for lifetimes, renamings, and multiple iterators.

Do you have any idea what happens when you delete a database entry in the middle of a range query?

You cannot delete an entry in the middle of a range query.
The iterator holds &storage for the lifetime of the iterator.
Removing an item requires &mut storage, which requires that all other references have been released.

In this case the over-eager compiler helps us 😄

@ethanfrey
Copy link
Member Author

Actually in this version (before #188), the iterators loads all data into memory, and you can delete while iterating, but the iterator will keep giving you a snapshot of previous data. I prefer the compiler prohibiting it.

Copy link
Member

@webmaster128 webmaster128 left a comment

Choose a reason for hiding this comment

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

🥇

Time to merge the big chunk

@webmaster128 webmaster128 merged commit c2ebb1a into master Mar 27, 2020
Blockchain Development automation moved this from In progress to Done Mar 27, 2020
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.

Add DB.Iterator to callbacks
2 participants