ADR 038: State Listening

[i-norden]

ADR-038 State Listening

This PR introduces an ADR for the state listening features discussed in #7888 and #7889, and is foundational to syncing state to an external database (#7099)

Tentative implementation branch

closes: #7888 and #7889

---

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

• Targeted PR against correct branch (see CONTRIBUTING.md)
• Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
• Code follows the module structure standards. N/A
• Wrote unit and integration tests. N/A
• Updated relevant documentation (docs/) or specification (x/<module>/spec/)
• Added relevant godoc comments. N/A
• Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
• Re-reviewed Files changed in the Github PR explorer
• Review Codecov Report in the comment section below once CI passes

[aaronc]

Thanks @i-norden !

Left a few comments. I think we could simplify things a bit. Why not just have an interface that listens to writes and receives key and value simply as []byte (with nil value for deletion)? The base64 and json marshaling seems like unnecessary overhead and should be the responsibility of the listener not the multi-store.

[aaronc]

Also I would argue that listeners should do their own filtering too. Instead of calling Allow and then WriteString, we just make a single call with both the key and value and then let the listener decide what to do. So I would propose this instead:

type WriteListener interface {
  // if value is nil then it was deleted
  OnWrite(key []byte, value []byte)
}

[i-norden]

@aaronc thank you! Responding to the individual comments, will make those adjustments.

New interface makes sense to me, and will simplify things a lot. Does the allowance/permissioning approach make sense otherwise? E.g. the stock WriteListener implementation that we propose will use the same blacklisting/whitelisting logic (aside from no longer filtering on operation) inside OnWrite.

[i-norden]

@alexanderbez thank you for the feedback, will make these changes!

[i-norden]

This might be out of scope, but I think we want to introduce the ability to update the block context (e.g. height, hashes) of a streaming service so that it can wrap/delineate the key-value pairs with that information. We could do this by defining a simple interface for any streaming service e.g.

type ContextStreaming interface {
	UpdateBlockContext(req abci.RequestBeginBlock)
}

And call this directly from within an ABCI application's BeginBlocker.

Or we could restructure the streaming service as a module itself, registering it with the module manager and letting BeginBlocker to call down through there. Curious what people's thoughts are on either approach (or other ideas). The streaming service won't have any meaningful fulfillment for the rest of the AppModule interface.

[aaronc]

So in general, we need a way to coordinate store changes with blocks and transactions and also a way to prune files.

I think likely what we need to do is batch up writes into a single file on disk per block or per transaction. That will make it easy to coordinate the sync process with the block/transaction data from tendermint and for the sync process to prune the files.

Probably what makes the most sense is to have a single file per stateful ABCI method:

• one file per tx with a name like tx-{tx-hash}
• one file per begin block with a name like begin-block-{block-height}
• one file per end block with a name like end-block-{block-height}

Not sure the indexer needs tx level granularity, but doesn't hurt to have it either.

Then the indexer process would basically work like this:

1. listen for new block data from tendermint
2. read the corresponding tx and begin/end block state changes for that block
3. delete each file once it has been synced

So this means that the state listener will also have to listen to the BeginBlock, EndBlock and DeliverTx ABCI methods in addition to the state. Likely we want to integrate this into BaseApp, although it could also be a wrapper around BaseApp.

[aaronc]

Actually here's a better approach, name the files like this:

• block-{N}-begin where N is the block number
• block-{N}-tx-{M} where M is the tx number in the block (i.e. 0, 1, 2...)
• block-{N}-end

then the indexer doesn't need for the block to even finish to start indexing.

And the ABIC request/response data could be included in the same of separate files so there's no need to fetch that info from tendermint separately.

[amaury1093]

My main question is what is the scope of this ADR? It seems to me that ABCI listeners could warrant a separate ADR.

[amaury1093]

We're adding more computation to be run on each ABCI call. It would be good to have an idea of the performance overhead.

[i-norden]

Completely agree and I admittedly do not have a great feel for this yet, I could do some preliminary benchmarking. These methods would be updating the StreamingService with the latest ABCI message, so that it can e.g. generate a new file which retains that context and to which we write out the state changes that are caused by that particular message.

[alessio]

Makes sense and looks good to me

Bez mentioned on Discord that he wouldn't have time to re-review this week, and said we can move forward with an ACK from Alessio & Aaron.