adr-038 state listener could observe uncommitted writes

[yihuang]

Summary of Bug

adr-038 state listener get feed with uncommitted writes.

adr-038 state listener is expect to only listens to the writes that get committed into chain state at the end of block, but it currently listens to kv store interface, which could includes writes that are not committed eventually, for example write calls happens in a grpc query.

We should only listen to the state writes on the consensus state machine.

Version

all the versions that support adr-038 state listening

Steps to Reproduce

• Setup state listener
• Simulate a cache write in grpc query like this:

      cacheCtx, write := ctx.CacheContext()
      store := cacheCtx.KVStore(k.storeKey)
      store.Set([]byte("debug set"), []byte("value"))
      write()

• Observe that the writes happen in grpc query are observed by state listener, although the writes will be discarded eventually.

[yihuang]

Even in tx delivery, if we write to a nested cache context, commit the underlying cache context, then discard that cache, the write is also propagated to the listener I think.
And if nested cache context all committed, the writes will be observed multiple times.

[yihuang]

This is more tricky than I thought, the most inner cache only commits in abci commit event, so we actually need to listen to the second layer of the cache, and this layer only, which is assuming all the writes happens on this layer, we need to structure the code like this:

BeginBlocker:
  deliverState.ms = app.cms.CacheMultiStore()

  cacheMS = deliverState.ms.CacheMultiStore()
  ... execute begin blocker ...
  cacheMS.Write()
DeliverTx:
  cacheMS = deliverState.ms.CacheMultiStore()
  ... execute ante handler ...
  cacheMS.Write()

  cacheMS = deliverState.ms.CacheMultiStore()
  ... execute msgs ...
  cacheMS.Write()
EndBlocker:
  cacheMS = deliverState.ms.CacheMultiStore()
  ... execute end blocker ...
  cacheMS.Write()
Commit:
  deliverState.ms.Write()
  app.cms.Commit()

Alternatively, we only listen to the uncached rootmulti, in that case, we only observe the writes at commit event, and can't associate the writes with tx, which is ok for our use case, but different from the design purpose of adr-038.

[yihuang]

I find the current api is very tricky to use, for example, set a listener on rootmulti.Store means you get notification when user do store.GetKVStore().Set(...), but set a listener on cachemulti.Store, you don't get notification when store.GetKVStore().Set(...), but get notification when store.Write().
And when you call AddListeners on cachemulti.Store, it don't have effect because the listenkv.Store wiring is done at construction only.

[alexanderbez]

I honestly don't have much context on the ADR 038 API -- but my intuition tells me there should be and only one place where we set listeners, and that should probably be on the root MS only. Even then, only emit writes on commit/write. WDYT?

[yihuang]

I honestly don't have much context on the ADR 038 API -- but my intuition tells me there should be and only one place where we set listeners, and that should probably be on the root MS only. Even then, only emit writes on commit/write. WDYT?

that works for my use case perfectly, I only want states versioned by block numbers.
But the original design seems to be able to associate the writes to abci events, you can tell a certain change set is emitted by begin blocker or delivery of the first tx, that's not possible if we observe the root ms which is only get written to at commit event. That may be useful for some use case.
For example, tx0 set a -> 1, tx1 set a -> 2, observe most inner layer only sees a -> 2, while observe higher level can see both.

[alexanderbez]

We should really aim to keep this as dead simple as possible:

1. Write ABCI Begin and EndBlock requests/responses
   2. Write reads
2. Write writes (on commit/write) (these will already encapsulate DeliverTx)

[yihuang]

I think we don't need to care about reads here.

[yihuang]

We should really aim to keep this as dead simple as possible:

1. Write ABCI Begin and EndBlock requests/responses
   2. Write reads
2. Write writes (on commit/write) (these will already encapsulate DeliverTx)

I guess a minimal fix would be:

• notify listeners of rootmulti with writes coming from the cache store, so we only need to listen rootmulti, rather than propagate the listeners to cache store.
• add ListenCommit to ABCIListener.
• update file streamer:
  • save into single file per block, with all the metadata and writes.
  • I'll use atomic file writing, so the files can be replicated atomically.

Listener Reliability

To make sure reliability of listener commit, we need to treat listener commit error as the same as state commit error, which is panic.

Batch Call

Since we only listen to the end result on commit, we can go one step further to notify listener in batch, which would be more efficient, but that means a bigger interface change, what do you think?

[peterbourgon]

For example, tx0 set a -> 1, tx1 set a -> 2, observe most inner layer only sees a -> 2, while observe higher level can see both.

The individual set operations are not guaranteed to arrive in a deterministic order, and are not necessarily going to be represented in on-chain state. Is the state listening system supposed to guarantee either of those properties? If not, you can hook in wherever, and do whatever you like. But if so, then you actually have to emit only the final a -> 2 event. In which case there's no reason any of this code should be touching any of the caching layers at all.

[yihuang]

For example, tx0 set a -> 1, tx1 set a -> 2, observe most inner layer only sees a -> 2, while observe higher level can see both.

The individual set operations are not guaranteed to arrive in a deterministic order, and are not necessarily going to be represented in on-chain state. Is the state listening system supposed to guarantee either of those properties? If not, you can hook in wherever, and do whatever you like. But if so, then you actually have to emit only the final a -> 2 event. In which case there's no reason any of this code should be touching any of the caching layers at all.

make sense, in my use case I only care about the end state of each block, I'm just not sure about the intension of original design, and if there are other use cases that do need to associating writes to specific txs.