sealing halt on exec fork

[AlexHentschel]

implements https://github.com/dapperlabs/flow-internal/issues/1294

Goals

• If sealing has been halted due to an execution fork, this halting condition should be persisted. A consensus node should not continue sealing after reboot.
• encapsulate the logic into a dedicated component
• properly log any inconsistent seals (previously we only printed them to std out)

The solution in this PR is intended to serve as an additional safety net against verification-sealing bugs:

• Until we have Missing Collection Challenges and approvals, there should only ever exist a single correct execution result for each block.
• Up to this point, we can keep the current solution (probably with minor modifications), so sealing will halt even if we have two conflicting but approved execution results.

High-level Implementation Overview

I found it an elegant solution to delegate the logic for detecting execution forks to the IncorporatedResultSeals mempool.

• thereby, we don't bloat consensus.Builder or matching.Engine with this somewhat temporary logic
• to further modularize the code, I have implemented the logic as a wrapper around the existing IncorporatedResultSeals interface. The wrapper is completely agnostic of the internal implementation

Minor infrastructure revisions contained in this PR

• Some separation in the name spaces of our unittest fixtures: For example, for many fixtures, it would be nice to set the Block the entity pertains to. However, WithBlock(blockID flow.Identifier) can only be defined once in unittest. This makes it very hard to determine what options are available for the individual fixtures. Naming becomes very convoluted and lengthy.

  flow-go/cmd/util/cmd/block_hash_by_height_test.go

  Line 118 in d61f61f

  seal := unittest.Seal.Fixture()

[AlexHentschel]

The IncorporatedResult is used to store approval signatures when they are received:

flow-go/model/flow/incorporated_result.go

Line 26 in 7c48153

aggregatedSignatures map[uint64]*AggregatedSignature

When including aggregatedSignatures in the ID computation, this changes the ID of the entity, which should not be the case.

[arrivets]

I don't think it changes the ID of the IncorporatedResult because aggregatedSignatures is a private field and is ignored in the RLP encoding which is used to calculate the ID

[AlexHentschel]

💡 thanks for pointing this out @arrivets . I was not aware that RLP did skip private fields.
Obviously, I didn't read the test you wrote:

flow-go/model/flow/incorporated_result_test.go

Lines 12 to 14 in 49fb4a1

	// TestIncorporatedResultID checks that the ID and Checksum of the Incorporated
	// Result do not depend on the aggregatedSignatures placeholder.
	func TestIncorporatedResultID(t *testing.T) {

🤦 Apologies.

Rolled back my changes to incorporated_result.go

[AlexHentschel]

@zhangchiqing I went back to using a list of ejection callbacks. After trying around a bit, I found this solution to be much more general compared to a single callback. Trying to give a high-level summary here:

• some mempools don't store the entities directly in the backend, but use composite structure, e.g.:

  flow-go/module/mempool/stdmap/incorporated_results.go

  Line 45 in 7c48153

  var incResults map[flow.Identifier]*flow.IncorporatedResult

• To not expose this internal details to an externally-provided ejection callback, the callback needs to be wrapped before it is injected into the Backend.
• By allowing the backend to serve multiple callbacks:
  • it is trivial for a mempool to wrap any externally-provided callback with the necessary logic before registering the (wrapped) ejection callback with the Backend
  • In contrast, when the Backend only accepts a single callback, the situation is much more complex for a mempool that has an internal ejection callback as well as accepts external ejection callbacks (like IncorporatedResults):
  • the mempool must implement a multiplexer which serves its internal mempool as well as an external mempool

To me, it seemed much more straightforward, to include the functionality to serve multiple callbacks into the Backend, specifically so as it is only adds two lines of code here. If you have other ideas, lets sync Leo. I tried for quite some time and this seemed to be by far the easiest (and least error-prone) solution.

[zhangchiqing]

can we say if no value is persisted, then there is no fork detected? so that we don't need to insert the default value.

[AlexHentschel]

👍 implemented

[zhangchiqing]

Instead of storing a bool value, what about storing the "evidence", which is the two conflicting seals. Because otherwise, we will forget about the evidence after a restart, and only remembered the conclusion without knowing how it leads to it.

We could store the two evidence under the same key codeExecutionForkDetected . On startup, to check whether we saw conflicting seals before, we could just check if the key exists . if the key exists, then we will log the conflicting seals in the value, and then crash the node.

[AlexHentschel]

storing the "evidence"

👍 implemented

if the key exists [...] crash the node.

👍 implemented. For context:

• what we do in case we detect conflicting seals is now encapsulated in

  flow-go/module/mempool/consensus/exec_fork_actor.go

  Lines 12 to 14 in 33de9d9

  	type ExecForkActor func([]*flow.IncorporatedResultSeal)
  	func LogForkAndCrash(log zerolog.Logger) ExecForkActor {

  This allows us to easily replace the logic (and potentially keep going, just without sealing)

[zhangchiqing]

seems incomplete

[AlexHentschel]

thanks for noticing. Fixed the test's doc.

[zhangchiqing]

seems incomplete

[AlexHentschel]

Sorry, this entire test was an outdated artifact from work-in-progress. Removed it

[zhangchiqing]

I'm not quite clear how this tests work.
But if you need help creating such forks in tests, I have an example here.
https://github.com/onflow/flow-go/blob/master/engine/execution/ingestion/engine_test.go#L816-L831

[AlexHentschel]

thanks for the reference. The test for the Builder already contained its own implementation:

flow-go/module/builder/consensus/builder_test.go

Lines 87 to 92 in addcb4b

	// createAndRecordBlock creates a new block chained to the previous block (if it
	// is not nil). The new block contains a receipt for a result of the previous
	// block, which is also used to create a seal for the previous block. The seal
	// and the result are combined in an IncorporatedResultSeal which is a candidate
	// for the seals mempool.
	func (bs *BuilderSuite) createAndRecordBlock(parentBlock *flow.Block) *flow.Block {

it has some specialized functionality for testing the payload Builder:

• creates execution receipts and seals for the blocks and includes them into the block's payloads
• tracks the created seals and blocks in the mempool mocks

I am inclined to keep the current test structure as I just added a test. 😅

[zhangchiqing]

I was looking for a test case that verifies this error will be raised, but didn't find.

There are different ways to test, the simplest would be a unittest that test this function. Or a test that tests the Add function.

[AlexHentschel]

thanks for flagging this. The ExecutionForkErr is only used internally within ExecForkSuppressor:

flow-go/module/mempool/consensus/exec_fork_suppressor.go

Lines 136 to 140 in 49fb4a1

	err := s.enforceConsistentStateTransitions(newSeal, otherSeal)
	if errors.Is(err, executionForkErr) {
	s.onExecFork([]*flow.IncorporatedResultSeal{newSeal, otherSeal})
	return false, nil
	}

I changed the error to be non-exported to avoid confusion.

For context:

• When implementing ExecForkSuppressor, I decided not to error on Add in case a fork is detected. Instead, the ExecForkSuppressor will internally clear the wrapped mempool and drop any subsequent seals which are added. In my view, this provides the following advantage:
  • The higher-level business logic can be oblivious to the existence of the ExecForkSuppressor wrapper. Otherwise, we would have to include additional logic to treat the specific errors from the wrapper (which is only semi-temporary).
  • Not erroring allows the higher-level business logic to potentially continue (just with halted sealing)

[AlexHentschel]

Thanks for the insightful review comments @zhangchiqing . I have implemented most of your suggestions.

Please note that I have split up the logic a bit more:

• the ExecForkSuppressor will enforce that sealing halts when an execution fork is detected
• whether the consensus node crashes or not is now encapsulated in a separate function (injected into ExecForkSuppressor):

  flow-go/module/mempool/consensus/exec_fork_actor.go

  Lines 12 to 14 in 33de9d9

  	type ExecForkActor func([]*flow.IncorporatedResultSeal)
  	func LogForkAndCrash(log zerolog.Logger) ExecForkActor {

  • This allows us to easily change whether or not we want to crash the consensus node or keep going (with halted sealing)
  • Though, based on our sync today, I only implemented a ExecForkActor that crashes the consensus node

[arrivets]

I think it should return false at this point. But if we've made it this far, added is true

[AlexHentschel]

Totally agree with your thinking and would prefer a different return value in this particular case. Before we decide what the return value should be, lets take a look at the larger picture:

• ExecForkSuppressor is a wrapper a around mempool.IncorporatedResultSeals. So how does the implementation behave when you Add an entity and the mempool overflows?
  • mempool.IncorporatedResultSeals is implemented by

    flow-go/module/mempool/stdmap/incorporated_result_seals.go

    Lines 21 to 24 in d14a1e7

    	// Add adds an IncorporatedResultSeal to the mempool
    	func (ir *IncorporatedResultSeals) Add(seal *flow.IncorporatedResultSeal) (bool, error) {
    	return ir.Backend.Add(seal), nil
    	}

    which calls into Backend

    flow-go/module/mempool/stdmap/backend.go

    Lines 131 to 138 in d14a1e7

    	// Add adds the given item to the pool.
    	func (b *Backend) Add(entity flow.Entity) bool {
    	b.Lock()
    	defer b.Unlock()
    	added := b.Backdata.Add(entity)
    	b.reduce()
    	return added
    	}

    and subsequently into Backdata

    flow-go/module/mempool/stdmap/backend.go

    Lines 31 to 40 in d14a1e7

    	// Add adds the given item to the pool.
    	func (b *Backdata) Add(entity flow.Entity) bool {
    	entityID := entity.ID()
    	_, exists := b.entities[entityID]
    	if exists {
    	return false
    	}
    	b.entities[entityID] = entity
    	return true
    	}

  • Specifically, the Backend always stores the element in Backdata. The return value indicates
    • false: entity is already present in the mempool and was deduplicated
    • true: entity is new from the mempool's perspective and can be added (ignoring mempool's capacity limitations)
  • The return value makes no assertion about whether or not an element is subsequently ejected
• Essentially, IncorporatedResultSeals.Add(seal) returns true if the entity is addable. Even if the mempool overflows and decides to eject the seal right away, the seal was still "addable". Note that this convention applies to all mempools backed by Backend.

ExecForkSuppressor is only a wrapper, and higher-level business logic should be agnostic to whether or not the wrapper exists. Therefore, I concluded that the wrapper should implement the same convention as the wrapped mempool.

Nevertheless, @arrivets you are raising a valid point:

• Given that a mempool has the ability to eject elements, can we base some algorithmic decisions on the return value?
• If the mempool returns false, it is telling us that the entity is already stored. However, this might change any moment, as another routine might concurrently add another entity leading to the ejection of our entity.
• Likewise, if the mempool returns true, the entity might be ejected right away as well.

[arrivets]

I don't think it changes the ID of the IncorporatedResult because aggregatedSignatures is a private field and is ignored in the RLP encoding which is used to calculate the ID

[zhangchiqing]

Had some optional suggestions. Other than that looks great 👍

[zhangchiqing]

Do we need to lock? I think seals itself is concurrent-safe.

[AlexHentschel]

Interesting suggestion. 🤔
You are probably right: we don't need to lock for methods that only do an atomic read from the wrapped mempool. But I am not 100% sure.

• We certainly need to lock ExecForkSuppressor for modifying operations such as Add or Rem as they interact with the wrapped mempool multiple times.
• So the question is then: can anything bad happen if we read from the wrapped mempool, while Add or Rem are modifying it at the same time?

As long as I am not sure, I am hesitant to remove the lock here. With the lock, we are sure that we don't have any concurrency edge cases.

[zhangchiqing]

Optional suggestion:

If we are committing to the approach that we will crash as soon as we detect forks, then we could simply the logic here.

For instance, we could use the default log and crash as the onExecFork, and execForkDetected is not need, because we will crash if detected, and if we haven't crashed, then it means we haven't detected execFork yet. And we don't need sealSet either, because there won't be 3 conflicting seals for the same block. As soon as there are 2 conflicting, then we will log them, restore them as evidence and crash.

[AlexHentschel]

Agree, it is complexity we don't utilize at this point and thereby a risk for additional bugs. Though, the code is quite well tested so I feel this risk is relatively low. I still think the benefit of being able to boot up consensus nodes with only halted sealing but otherwise functioning block production outweighs the small risk.