Fix cyclical reference in get_context_data #335
Comments
|
Would we win something by storing the iterators in |
|
That might actually solve it, yeah. We'll have to play around with the implementation and see what the compiler thinks about it. |
|
This is a valid concern if the DB we call into is Rust. We currently check this on the cosmwasm-std side, as Rust does not allow any iterators to be open when writing to the storage. And the Go db doesn't care about this restriction (which is fine in cosmos-sdk as it is singlethreaded). I think the first step would be to ensure the iterators get closed over FFI when they are dropped (from cosmwasm-std -> vm -> go-cosmwasm -> cosmos-sdk). Then we can enforce this on the vm level, to defend against contracts not using cosmwasm-std. After those (which should shore up any real problems), we can try to fix lifetimes so rust is happy. I did fight with this for some time and the fact that iterators depend on storage but are stored separately makes this intractable. I can ensure code-wise that I close them all before releasing storage, but with all the casting to |
|
If I understand correctly, the points raised by @ethanfrey are all valid but all mostly independent of the cyclic reference.
This is avoid iterator invalidation. Good to avoid the most dangerous usages in the std. In the VM, iterators are never cleared within a contract execution. So we definitely have open iterators while storage is written. Fingers crossed those open iterators are not used anymore.
Even in single threaded envirtonments we need to be clear if an iterator is invalidated when you write to the storage in the middle of an iteration (single threaded: next, next, delete, next, add, next …). This is very different in different environments. I don't know about Go/Cosmos SDK.
I.e. call import like |
|
I don't think that there's a pressing need to add deletion of iterators from the contract itself. The way things currently work, iterators don't have a significant memory overhead, and it's faster to just collect them and close them all in one swoop after returning from the contract (which is what we do now, in CosmWasm/wasmvm#109) instead of passing through all that FFI every time. On the subject of the cyclical reference, perhaps we can resolve this by boxing the Storage field in ContextData? The solution to this issue specifically isn't that hard, just takes care that we don't break anything else. about the mutable references, Ethan is correct that we don't have to require a mutable reference (which is a unique reference) if mutating the Go objects is safe through an immutable (shared) reference. We could maybe change some of the |
|
We probably need to address this in some way for the Wasmer upgrade, where // must implement Clone
pub struct Env<S: Storage, Q: Querier> {
pub memory: wasmer::Memory,
pub context_data: Arc<RwLock<ContextData<S, Q>>>,
}instead of I tried implementing // +--->> Go pointer
// |
// Ctx ->> ContextData +-> storage: impl Storage <<-> iterators: Box<dyn Iterator + 'a> --+
// |
// +-> querier: impl Querier
//
// -> : Ownership
// ->> : Mutable borrowto move all the circular reference to Now I wonder if what we try to do is fundamentally broken: access an iterator from multiple import calls, which creates lifetime reqirements for iterators and storage that are almost impossible to get right. |
Here we have to lie to the compiler. We know the storage lives the entire ContextData, and if we make sure to clean up Iterators first (dynamically or upon cleanup), we can logically view this is right. I did some tricks with lying about lifetimes ( |
|
The types of this struct is broken right now: struct ContextData<'a, S: Storage, Q: Querier> {
gas_state: GasState,
storage: Option<S>,
storage_readonly: bool,
querier: Option<Q>,
/// A non-owning link to the wasmer instance
wasmer_instance: Option<NonNull<WasmerInstance>>,
#[cfg(feature = "iterator")]
iterators: HashMap<u32, Box<dyn StorageIterator + 'a>>,
#[cfg(not(feature = "iterator"))]
iterators: PhantomData<&'a mut ()>,
}
i.e. The only way I see to get this right is having the same lifetime for all of those three. But since |
Nope, at not that easy since the context must be created with Send+Sync only: |
|
These issues are with the Wasmer Reborn branch, correct? Not on 0.11? |
|
The issues exist on 0.11/master. The work on Wasmer Reborn forces us to think about this because there is no raw pointer operation anymore we can use to forget about all the problems. |
|
Ideally we create a struct that owns a Storage and it's iterators. This would reduce the scope of the problem to a size that you can reason about. Those two threads describe the problem and possible solutions for such a wrapper: |
|
This looks like an interesting crate that may help the storage/iterator issue (linked from the second answer above): |
As described here by @reuvenpo:
See also #331 (comment) and #331 (comment)
The text was updated successfully, but these errors were encountered: