[Dynamic Protocol State] Storage layer for KV store

[durkmurder]

Context

This PR implements storage capabilities for persisting KV store in the database.
It's implemented following the design doc.
Implementation is similar to the protocol state with the exception that it's a bit simpler and performs cache on store as well.

[jordanschalm]

Suggested change

	// - CAUTION: The updated state requires confirmation by a QC and will only become active at the child block,
	// _after_ validating the QC.

This is not true from protocol state machine, shouldn't be true for the kv store either. I'm guessing this is a copy-paste from some outdated comments.

[AlexHentschel]

We make the same statement in the storage abstraction for the protocol state:

flow-go/storage/protocol_state.go

Lines 27 to 28 in 81266cf

	// - CAUTION: The protocol state requires confirmation by a QC and will only become active at the child block,
	// _after_ validating the QC.

For sure, this is a simplified statement skipping over some details. Nevertheless, I feel the statement is still applicable, and a similarly worded statement is also in our notion doc Identity-Changing Operations:

My interpretation of your comment is: "the consensus nodes don't need to wait for a QC, as they validate the correctness of the state transition by re-computing it locally" I am happy to discuss how we could reword the sentence, to make it more clear.

Nevertheless, I feel there is a super-important aspect in here:

The updated state [...] only become active at the child block, after [...] valid QC

This avoids some subtle but safety-critical misusage:

1. using the resulting protocol state (kv-store) and apply it to the payload of the block (circular dependency)
2. some off-chain logic uses the protocol state to determine signing authority. Without waiting for a valid QC, byzantine consensus participant could give a malicious block to a client where it tempered with the epoch participation information. Thereby, a client could be easily tricked into accepting malicious messages from unauthorized parties, or rejecting messages from honest parties.

On the one hand, the consensus follower handles this safely and protects higher-level business logic. On the other hand, I feel we should emphasize this safety-critical requirement because not all applications might be using the consensus follower and additionally we want to be reminded of this requirement when we work on the consensus follower in the future.

[AlexHentschel]

how about we write (?)

Suggested change

	// - CAUTION: The updated state requires confirmation by a QC and will only become active at the child block,
	// _after_ validating the QC.
	// - CAUTION: The updated state will only become active at the child block,
	// _after_ its correctness has been affirmed by a valid QC.

Does that address your wording concerns?

[jordanschalm]

Suggested change

	// CAUTION: this store state requires confirmation by a QC and will only become active at the child block,
	// _after_ validating the QC. Protocol convention:
	// - Consider block B, whose ingestion might potentially lead to an updated KV store state.
	// For example, the state changes if we seal some execution results emitting specific service events.
	// - For the key `blockID`, we use the identity of block B which _proposes_ this updated KV store. As value,
	// the hash of the resulting state at the end of processing B is to be used.
	// - CAUTION: The updated state requires confirmation by a QC and will only become active at the child block,
	// _after_ validating the QC.
	// Protocol convention:
	// - Consider block B, whose ingestion might potentially lead to an updated KV store state.
	// For example, the state changes if we seal some execution results emitting specific service events.
	// - For the key `blockID`, we use the identity of block B which _proposes_ this updated KV store. As value,
	// the hash of the resulting state at the end of processing B is to be used.

[jordanschalm]

I'm wondering if it will be a clearer separation of concerns for this storage interface to keep KeyValueStoreData private, and accept/return instances of kvstore.API when storing and retrieving. My thinking is to keep the surface area where we need to worry about versioned types as small as possible (ideally only kvstore and storage to differing extents).

Not sure though, we can definitely merge this as-is and sort out the exact separation when hooking up to #5371.

[jordanschalm]

Looks good, most comments are around documentation -- there are a few places where ProtocolStateMachine docs are leftover.

[codecov-commenter]

Codecov Report

Attention: 22 lines in your changes are missing coverage. Please review.

Comparison is base (7d7a9ab) 56.00% compared to head (230f56a) 56.00%.
Report is 1 commits behind head on feature/protocol-state-kvstore.

Files	Patch %	Lines
storage/badger/protocol_kv_store.go	59.25%	18 Missing and 4 partials ⚠️

Additional details and impacted files

@@                        Coverage Diff                         @@
##           feature/protocol-state-kvstore    #5383      +/-   ##
==================================================================
- Coverage                           56.00%   56.00%   -0.01%     
==================================================================
  Files                                1021     1025       +4     
  Lines                               98524    98623      +99     
==================================================================
+ Hits                                55183    55229      +46     
- Misses                              39211    39253      +42     
- Partials                             4130     4141      +11     

Flag	Coverage Δ
unittests	56.00% <64.51%> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[AlexHentschel]

first batch of comments:

[AlexHentschel]

Overall, there is some ambiguity in the term "model". In my opinion, a data model is very similar to a schema: it specifies the data fields and their types. I would equate our golang type definition with the data model (if we had a protobuf representation of the same struct, it would be the same data model, just in a different language). For example, in Jordan's PR #5371, he defines the v0-data-model as

flow-go/state/protocol/protocol_state/kvstore/models.go

Line 16 in 5707616

type modelv0 struct{}

and the v1-data model has has the single key-value pair InvalidEpochTransitionAttempted.

What we are storing here is not the data model in my opinion. For example all entries we store could be the same data model (e.g. they are all instances of modelv0). I would call the thing we are storing either 'snapshots' (consistent with snapshots of the protocol state) or instances of a data model.

Suggested change

	// - storage.ErrAlreadyExists if a model with the given id is already stored
	// - storage.ErrAlreadyExists if a KV-store snapshot with the given id is already stored

[AlexHentschel]

Great work. Largely documentation questions.

I feel the only possibly controversial aspect is this sentence in the documentation:

CAUTION: The updated state requires confirmation by a QC and will only become active at the child block, after validating the QC.

See Jordan's comment. If you want to move ahead with this PR, I think it would be totally fine to merge when all other comments are addressed. We discuss on Tuesday and add a follow-up PR fixing the few lines of documentation if necessary.