[Dynamic Protocol State] Block payload contains protocol state ID

[durkmurder]

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

Context

This PR adds protocol state as part of block payload. Essentially a valid block now needs to have a correct protocol state ID which will be validated by replicas, to support that leader runs calculation of expected protocol state ID in block building process.
Lots of changes were made to the testing code since we have increased requirements on what is a valid block.
Essentially main and the most important changes are in:

• state/protocol/protocol_state/mutator.go - allows to mutate protocol state and apply service events
• state/protocol/badger/mutator.go - uses mutator and validates block proposals
• module/builder/consensus/builder.go - uses mutator to build a valid proposal

⚠️ I also had to modify our sporking process. Previously we have generated root block in rootblock command and then called finalize to finish root snapshot creation. With the new changes rootblock wasn't able to produce a valid block anymore so I had to extract generation of epoch setup and commit events from finalize and move it to rootblock. In new implementation rootblock generates epoch setup, epoch commit events and creates a valid root block. It isn't a huge change but now rootblock is not deterministic anymore(it generates assignments and in process of generating assignments we randomly shuffle elements, additionally we generate a random "random source").

I will be adding a few more tests for mutator to cover all scenarios but in general it's ready for a review.

[AlexHentschel]

Would suggest to introduce a dedicated type for better readability:

Suggested change

	// DeferredDBUpdate is a shorthand notation for an anonymous function. The function takes
	// a badger transaction as input to runs some database updates as part of that transaction.
	type DeferredDBUpdate = []func(tx *transaction.Tx) error

I explicitly used a type alias here to codify that is literally just a shorthand notation and not meant as a standalone type.

I would use the DeferredDBUpdate everywhere here and in the test.

[durkmurder]

Moved it into transaction package so we can use it in other places.
8264c2a

[AlexHentschel]

In line 84 below

flow-go/state/protocol/protocol_state/mutator_test.go

Line 84 in 2e59e75

require.Len(s.T(), dbUpdates, 1)

we check that there is one element in the dbUpdates but whether that is the correct one is not asserted. But I think we can easily check that. Essentially we are expecting that exactly the object returned by the StoreTx call is in dbUpdates

Suggested change

	s.setupsDB.On("StoreTx", epochSetup).Return(func(*transaction.Tx) error { return nil }).Once()
	deferredEpochSetupDBStore := func(*transaction.Tx) error { return nil }
	s.setupsDB.On("StoreTx", epochSetup).Return(deferredEpochSetupDBStore).Once()

and in line 84 we would write:

assert.ElementsMatch(s.T(), []func(tx *transaction.Tx) error{deferredEpochSetupDBStore}, dbUpdates)

[durkmurder]

I don't think you can compare first-class functions in golang, there is a discussion about it but the whole approach seems hacky to me. It doesn't work with Equal, ElementsMatch or EqualValues

[durkmurder]

I have taken slightly different approach to perform assertions in that function: 38b3210

[AlexHentschel]

do we still need to explicitly call AssertExpectations on stateMachine like this:

Suggested change

	require.NoError(s.T(), err)
	s.stateMachine.AssertExpectations(s.T())
	require.NoError(s.T(), err)

to explicitly enforce that SetInvalidStateTransitionAttempted() was called at all? This tutorial from 2023 talks about calling AssertExpectations:

Use the AssertExpectations method Testify’s AssertExpectations method can be used to check that the expected methods were called on the mock object. Always use this method to ensure that the expected behavior was observed during the test.

Not sure ... maybe testify is now doing that automatically. If not, my comment applies to various places in the test.

[durkmurder]

I am using "smart" constructor which automatically registers mocks and performs assertions:

flow-go/state/protocol/protocol_state/mock/protocol_state_machine.go

Lines 147 to 154 in 9fc2f66

	func NewProtocolStateMachine(t mockConstructorTestingTNewProtocolStateMachine) *ProtocolStateMachine {
	mock := &ProtocolStateMachine{}
	mock.Mock.Test(t)
	t.Cleanup(func() { mock.AssertExpectations(t) })
	return mock
	}

If you are using those then you can be safe that all expected calls will be executed and no unexpected calls will be called.

[AlexHentschel]

If I understood correctly, in TestHappyPathWithDbChanges we test the state transition from Staking to Epoch Setup phase. I don't think we test the happy path for EpochCommit event, right? I.e. the transition from Epoch Setup to Epoch Commit Phase.

[durkmurder]

Purpose of that test was to ensure that successfully processing a service event creates a deferred DB update to store that event in database, since we cache db updates in stateMutator we need to ensure that we don't drop them and get the exact set as return values of Build. To summarize it's not about the transition but more about what happens after successfully processing a service event.
I will update the documentation and add processing of commit event as well.

[durkmurder]

38b3210

[AlexHentschel]

Amazing PR. Thanks for putting all this work in. The structure is becoming really clear and clean. All my remaining comments are stylistic, like function names, etc.

There is this conceptual point around epoch fallback triggers and finalization, where it isn't clear to me that we are on the same page. Its probably most efficient to discuss in person. I added a corresponding todo to issue #4649 and tried to give a some details.
It could easily be that we are on the same page and I am just misunderstanding a comment.