[Dynamic Protocol State] Protocol State Mutator

[durkmurder]

https://github.com/dapperlabs/flow-go/issues/6802

Context

This PR implements protocol state mutator. It's a dedicated component that will be used by compliance layer to create protocol state updater and later commit protocol state into storage. To maintain a correct index of blockID -> protocol state previously described logic has to be executed for EACH block.

[jordanschalm]

Suppose we have two protocol state changes, $\delta_1$, $\delta_2$, and applying them sequentially to an initial state $\Delta$ results in $\Delta$ again.

$\Delta + \delta_1 = \Delta'$
$\Delta' + \delta_2 = \Delta$

For example, if the state changes subtract, then add, a constant value to Weight.

In that case, we will have an unhandled storage.ErrAlreadyExists error here, treated as irrecoverable. I don't think we should make the assumption at this layer that protocol states can never repeat, even if it happens to be true.

Suggestion: Handle ErrAlreadyExists here, and consider as a no-op.

[AlexHentschel]

+1

We should certainly document possible error returns in detail for the interface ProtocolState. See my comment below for a detailed suggestion

[jordanschalm]

⚠️ Approving the PR - but please address this before merging.

[durkmurder]

but in such case hasChanges has to be false, so it shouldn't happen. Nevertheless I will make it accept such case. 7266c8f (#4597)

[jordanschalm]

We should document this as an expected error, or wrap as irrecoverable.

[AlexHentschel]

I think from the perspective of the Mutator the error should be benign, as it allows (under certain conditions, e.g. access node trying to serve an API request) to continue.

See my comment below for a detailed suggestion on documenting error returns of interface ProtocolState.

[jordanschalm]

⚠️ Approving the PR - but please address this before merging.

[durkmurder]

I've updated godoc so we expect ErrNotFound a1969a0 (#4597)

[AlexHentschel]

Not sure if I mentioned this in one of the preceding PR reviews:

• I feel the goDoc of interface ProtocolState could be extended
• Regarding

  flow-go/storage/protocol_state.go

  Lines 10 to 11 in ddd1a1c

  	// StoreTx allows us to store protocol state as part of a DB tx, while still going through the caching layer.
  	StoreTx(id flow.Identifier, protocolState *flow.ProtocolStateEntry) func(*transaction.Tx) error

  
  If I understand correctly, the implementation does not (contrary to the interface documentation) go through the caching layer, right?

What do you think about extending the goDoc as follows:

// ProtocolState represents persistent storage for protocol state entries.
type ProtocolState interface {
	// StoreTx returns an anonymous function (intended to be executed as part of a badger transaction),
	// which persists the given protocol state as part of a DB tx.
	// Expected errors of the returned anonymous function:
	//   - storage.ErrAlreadyExists if a protocol state snapshot has already been stored under the given id.
	StoreTx(id flow.Identifier, protocolState *flow.ProtocolStateEntry) func(*transaction.Tx) error

	// Index returns an anonymous function that is intended to be executed as part of a database transaction.
	// In a nutshell, we want to maintain a map from `blockID` to `protocolStateID`. Upon call, the
	// anonymous function persists the specific map entry in the node's database.
	// Protocol convention:
	//   - Consider block B, whose ingestion might potentially lead to an updated protocol state. For example,
	//     the protocol state changes if we seal some execution results emitting service events.
	//   - For the key `blockID`, we use the identity of block B. As value, the hash of the resulting protocol
	//     state at the end of processing B is to be used.
	// Expected errors during normal operations:
	//   - storage.ErrAlreadyExists if the key already exists in the database.
	Index(blockID flow.Identifier, protocolStateID flow.Identifier) func(*transaction.Tx) error

	// ByID returns the protocol state by its ID.
	// Expected errors during normal operations:
	//  * `storage.ErrNotFound` if no protocol state snapshot with the given Identifier is known.
	ByID(id flow.Identifier) (*flow.RichProtocolStateEntry, error)

	// ByBlockID returns the protocol state by block ID.
	// Expected errors during normal operations:
	//  * `storage.ErrNotFound` if no snapshot of the protocol state has been indexed for the given block.
	ByBlockID(blockID flow.Identifier) (*flow.RichProtocolStateEntry, error)
}

[AlexHentschel]

+1

We should certainly document possible error returns in detail for the interface ProtocolState. See my comment below for a detailed suggestion

[jordanschalm]

Commented on my outstanding points which we should address before merging.

[codecov-commenter]

Codecov Report

Patch coverage: 70.83% and project coverage change: +7.67% 🎉

Comparison is base (b361d16) 54.90% compared to head (a1969a0) 62.58%.

Additional details and impacted files

@@                        Coverage Diff                         @@
##           feature/dynamic-protocol-state    #4597      +/-   ##
==================================================================
+ Coverage                           54.90%   62.58%   +7.67%     
==================================================================
  Files                                 923      386     -537     
  Lines                               86103    33982   -52121     
==================================================================
- Hits                                47277    21267   -26010     
+ Misses                              35227    11005   -24222     
+ Partials                             3599     1710    -1889     

Flag	Coverage Δ
unittests	62.58% <70.83%> (+7.67%)	⬆️

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

Files Changed	Coverage Δ
state/protocol/protocol_state/mutator.go	70.83% <70.83%> (ø)

... and 542 files with indirect coverage changes

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