Cosmos-sdk: Add CacheContext to Context

[mossid]

Some module (such as IBC) should delegate its task to unknown handlers. In this case, even those other modules fail, the module itself should perform some state transition. To solve this problem, Context should expose a new function called CacheContext.

someWork(ctx)
cctx := ctx.CacheContext()
err := functionThatCanFail(cctx)
if err != nil {
    return sdk.Result{}
}
cctx.Write()
return sdk.Result{}

The result of someWork will remain either case. The result of functionThatCanFail will only remain if it is succeed.

[cwgoes]

Context is immutable, why do you need to cache it?

If the unknown function needs to change the context, just have it pass back the updated one:

ctx := MakeContext() // or whatever
newCtx, err := functionThatCanFailOrUpdateContext(ctx)
if err != nil {
  // can still use original ctx
  return err.Result()
}
ctx = newCtx
// do other stuff with updated context

[mossid]

ctx.CacheContext will call ctx.WithMultiStore(ctx.multistore().cacheMultiStore()). Since Context.multistore() should not be exposed, CacheContext is cacheing internal multistore.

In that case, if functionThatCanFail returns an error, the work performed before calling it will be also reverted(and the example was bit wrong, modified it)

[rigelrozanski]

I'm a bit confused on the use case, the state is already cached..

Okay so let me get this straight - you want a transaction to be able to fail but still modify some state? Why not just return a successful IBC message (so the state will be saved) but with further result details that the handler message failed. And yeah the context is immutable - it's the cached store you want access to so you can revert any changes made by a bad handler call. ... I think this may already be achievable with the information which the ctx/keepers provide

[rigelrozanski]

slack convo w Jae below - I think I'm on board @mossid want to take on this task?

---

jaekwon
Yeah it’s cuz the IBC handling thing may have left the state in a bad state.
So you do want to WithMultiStore with me.CacheMultiStore as Mossid said.
so that you can throw away the bad state and carry on to record that it failed, like you said.
Mossid is Joon?

rige
yes

jaekwon
Cool I think he’s spot on.
But
No sounds good to me.
Does that make sense, what I wrote?
No GitHub access right now.

rige
Yeah my understanding of how the caching works is incomplete. To me it sounds like to want to extend context not cache the whole thing
Actually I guess that’s really all CacheContext is doing? Okay I think I’m onboard

jaekwon
Yeah haha
Just wrapping the multistory with a cache and slapping that on a copy of the context which wraps the original.
So both context and ms are wrapped.
In the future we might need to cache wrap more things so it makes sense to have a CacheContext function i think.

rige
awesome… going to post this convo to github for the joonster

jaekwon
Ok!

[cwgoes]

Further discussion in person with @mossid (correct me if I missed anything):

We're unclear why the multistore Write method clears the cache whenever written - https://github.com/cosmos/cosmos-sdk/blob/master/store/cachekvstore.go#L113. If a cached multistore is read from, Write is called, then read from again, the underlying store will be unnecessary re-read. Is there a strong reason to do so over exposing separate Write and Flush functions, and giving modules control over when the cache is cleared?

CC @jaekwon

[ebuchman]

I think its for the following reasons:

1) Keeps it simple to know that cache is wiped after being written. Otherwise you'd have to run through everything and remove the "removed" guys from the cache and change everything to not dirty after the Write

2) The underlying parent is a KVStore, in our case at least typically a IAVLStore, which has it's own internal caching of nodes to minimize actual reads to the disk

[cwgoes]

Closed by https://github.com/cosmos/cosmos-sdk/pull/837.