Cosmos-sdk: Capability State Does Not Revert on Failed Transaction

[AdityaSripal]

Writes to the CapabilityStore map do not get rolled back on a failed transaction. This issue shows up whenever a transaction writes to the CapabilityStore (i.e. calls ClaimCapability, NewCapability, ReleaseCapability, etc.) and then fails later in transaction execution. All other state gets rolled back since the cached multistore gets reset. However, we don't have a way to reset the capability maps back to their state before tx execution.

Found this issue while debugging with @jackzampolin. In our case, it prevented successful handshake from being completed. Since we were failing on a callback that ran after creating the channel capability on ChanOpenInit. Since we failed to initialize the channel but still created the channel capability, all future ChanOpenInit calls failed because it tried to create a new channel capability; but the channel capability was already created so an error CapabilityAlreadyTaken was returned.

Thus the handshake was frozen in place. It was impossible to move further in the handshake process.

We need to think of a way to preserve the property of rolling back all state on tx failure with this new CapabilityStore

cc: @alexanderbez @cwgoes @fedekunze

[fedekunze]

can you release the capability if an error is found?

[alexanderbez]

Indeed. We really need to think about this before the next fix attempt. We need something that is a plain ole' map but that has KVStore semantics.

One idea that comes to mind is to re-introduce the notion of the mem-KVStore, but we update the KVStore API/interface to include methods to get/set a raw value (interface{}) instead of bytes. The capability module would then use this store and call these new get/set APIs which return an interface{} and then it can cast to Capability.

[alexanderbez]

can you release the capability if an error is found?

No. The BaseApp has no notion of state-machine specifics.

[alexanderbez]

Another approach is to reintroduce the mem-KVStore _without_ any new APIs and _still_ use the map. We first query the mem-kvstore to find a UID where that UID is a key in the map. If the KVStore doesn't exist, we delete from the map.

mem-kvstore <=> map <=> caps

[jackzampolin]

That sounds like a pretty good approach @alexanderbez and it makes good reuse of understood primitives in the system.

[cwgoes]

Writes to the CapabilityStore map do not get rolled back on a failed transaction. This issue shows up whenever a transaction writes to the CapabilityStore (i.e. calls ClaimCapability, NewCapability, ReleaseCapability, etc.) and then fails later in transaction execution. All other state gets rolled back since the cached multistore gets reset. However, we don't have a way to reset the capability maps back to their state before tx execution.

Ah yes, this makes sense, I think this was why the ADR didn't use a map in the first place (but then I forgot about it - sorry about that).

Another approach is to reintroduce the mem-KVStore without any new APIs and still use the map. We first query the mem-kvstore to find a UID where that UID is a key in the map. If the KVStore doesn't exist, we delete from the map.

I think this will work, but what is the benefit compared to just altering the interface as you describe in the above comment? We don't care too much about using the map as a cache.

[alexanderbez]

I think this will work, but what is the benefit compared to just altering the interface as you describe in the above comment? We don't care too much about using the map as a cache.

The map is less of a cache and more of an ephemeral store. I'd rather avoid modifying interfaces in the store package just for this purpose. Hence, I propose we reintroduce the mem-kvstore and still use the map which actually stores the capabilities.

[cwgoes]

I think this will work, but what is the benefit compared to just altering the interface as you describe in the above comment? We don't care too much about using the map as a cache.

The map is less of a cache and more of an ephemeral store. I'd rather avoid modifying interfaces in the store package just for this purpose. Hence, I propose we reintroduce the mem-kvstore and still use the map which actually stores the capabilities.

Ah, I see - if that allows us to avoid a major interface modification, that makes sense.