Move service events to execution receipt

[jordanschalm]

This PR removes service events from seals so they live only in execution results.

Changes

• Convert flow.Event (ie. Cadence) typed service events immediately to flow.ServiceEvent in execution node, so we only deal with one representation as much as possible.
  • Change type of ExecutionResult.ServiceEvent to flow.ServiceEvent
• Update bootstrapping to add service event to root result, rather than root seal
• Change when protocol state change caused by a service take place
  • For the minimal sealing chain (A <- B(resultA) <- C(sealA) <- D) where a (e.g.) EpochSetup event is emitted in block A, the epoch would previously become accessible at block C, when the seal is incorporated. Now the epoch becomes accessible at block D, after we have a QC for the block containing the seal.
  • Protocol events are still emitted at the same time (ie. when C is finalized)
• Update EpochBuilder test utility
  • Expose an API for querying information about epochs built by the utility

[durkmurder]

Very clean, great work!

[zhangchiqing]

Nice work! Just have 1 comment

[zhangchiqing]

1. [optional] since we are handling events for its parent, what about passing the parent to this function instead of letting this function query the parent?
2. ⚠️ we might handle events for the same parent again when inserting a second child of that parent, would that raise error? Are we able to exit earlier?

[jordanschalm]

1 since we are handling events for its parent, what about passing the parent to this function

Would this be for performance? The parent is effectively guaranteed to be cached , I don't think there would be much performance impact by passing it as an argument. We also don't already have the parent at the callsite

flow-go/state/protocol/badger/mutator.go

Lines 469 to 485 in 9c91dc2

	blockID := candidate.ID()
	m.tracer.StartSpan(blockID, trace.ProtoStateMutatorExtendDBInsert)
	defer m.tracer.FinishSpan(blockID, trace.ProtoStateMutatorExtendDBInsert)
	// SIXTH: epoch transitions and service events
	// (i) Determine protocol state for block's _current_ Epoch.
	// As we don't have slashing yet, the protocol state is fully
	// determined by the Epoch Preparation events.
	// (ii) Determine protocol state for block's _next_ Epoch.
	// In case any of the payload seals includes system events,
	// we need to check if they are valid and must apply them
	// to the protocol state as needed.
	ops, err := m.handleServiceEvents(candidate)
	if err != nil {
	return fmt.Errorf("could not handle service events: %w", err)
	}

2 we might handle events for the same parent again when inserting a second child of that parent

Good point 🙇 . For a sealing chain like A <- B(RA) <- C(SA) <- D, when inserting conflicting children of C we always need to insert the epoch status for the child D. This will not cause conflicts because it is keyed by the block ID of D. We also need to insert the service events, which may have conflicts. (They can have conflicts beyond this particular case of multiple children of block C, since there may be multiple ERs containing the same service event in different forks, as well as different seals.)

I think the best way to handle all these cases is to allow duplicates directly in the storage layer of EpochSetups and EpochCommits -- I'll add this and a test case.

[zhangchiqing]

Would this be for performance?

Not for performance. Just to note that I was suggesting this:

	parent, err := m.blocks.ByID(candidate.Header.ParentID)
	if err != nil {
		return nil, fmt.Errorf("could not get parent (id=%x): %w", block.Header.ParentID, err)
	}
 	// SIXTH: epoch transitions...
 	ops, err := m.handleServiceEvents(parent) 
 	if err != nil { 
 	 	return fmt.Errorf("could not handle service events: %w", err) 
  	} 

when inserting conflicting children of C

I'm not sure if we are talking about the same case. The case I'm talking about is this:

A <- B(RA) <- C(SA) <- D
              ^------- E

Block C has 2 children: D and E. When we receive D first, we will call handleServiceEvents(D) which store the service events for its parent block (C). And then when we receive E later, we will call handleServiceEvents(E) again which will store the service events for its parent block (C) again. And storing the service events will store the epoch setup event, which seems doesn't allow SkipDuplication, which means, it will fail, right?

[jordanschalm]

I see. We can't only pass in the parent block, because we insert the epoch status for the block being inserted (ie. block D), not the parent block.

flow-go/state/protocol/badger/mutator.go

Lines 858 to 859 in 9c91dc2

	// we always index the epoch status, even when there are no service events
	ops = append(ops, m.epoch.statuses.StoreTx(block.ID(), epochStatus))

Block C has 2 children: D and E

That is the same case I'm thinking of 👍 , added SkipDuplicate to the service event storage APIs in 132e243 and test case in 9e5bdaa.

[zhangchiqing]

This comment seems is duplicated with the TestExtendConflictingEpochEvents

[jordanschalm]

It's very close but it says two duplicate instances rather than two different instances

[zhangchiqing]

is the following case same as this one? Or has any test case covered it already?

ROOT <--B1 <- B2(R1) <-- B3(S1) <--B4
                            ^----- B5

This is to test we are able to process S1 twice.

[jordanschalm]

TestExtendDuplicateEpochEvents will cover processing the same service event twice, since we create the same service event on two different forks, and handle service events on both forks.