Fix matching engine

[zhangchiqing]

This PR fixes a bug in matching engine that we didn't request receipt where we should and caused sealing to halt

[codecov-commenter]

Codecov Report

Merging #1294 (07b3944) into master (6b18761) will decrease coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1294      +/-   ##
==========================================
- Coverage   54.83%   54.80%   -0.03%     
==========================================
  Files         502      502              
  Lines       31852    31848       -4     
==========================================
- Hits        17466    17455      -11     
- Misses      12029    12037       +8     
+ Partials     2357     2356       -1     

Flag	Coverage Δ
unittests	54.80% <ø> (-0.03%)	⬇️

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

Impacted Files	Coverage Δ
engine/consensus/matching/core.go	50.29% <ø> (+0.59%)	⬆️
...sus/approvals/assignment_collector_statemachine.go	40.38% <0.00%> (-6.74%)	⬇️
module/mempool/epochs/transactions.go	94.73% <0.00%> (-5.27%)	⬇️
...ngine/common/synchronization/finalized_snapshot.go	68.75% <0.00%> (-4.17%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6b18761...07b3944. Read the comment docs.

[AlexHentschel]

I think we should completely remove

• flow-go/engine/consensus/matching/core.go

  Lines 293 to 298 in 02fa1fc

  	// set of blocks for which we have a candidate seal:
  	blocksWithCandidateSeal := make(map[flow.Identifier]struct{})
  	for _, s := range c.seals.All() {
  	blocksWithCandidateSeal[s.Seal.BlockID] = struct{}{}
  	}

• flow-go/engine/consensus/matching/core.go

  Lines 317 to 325 in 02fa1fc

  	// if we have already a candidate seal, we skip any further processing
  	// CAUTION: this is not BFT, as the existence of a candidate seal
  	// does _not_ imply that all parent results are sealable.
  	// TODO: update for full BFT
  	// The follow optimization doesn't work, because sealing requires 2 matching receipts.
  	// Sealing would still halt even if there is a candidate seal but no matching receipts.
  	// if _, hasCandidateSeal := blocksWithCandidateSeal[blockID]; hasCandidateSeal {
  	// continue
  	// }

I don't think it is worth keeping the commented-out code

We have an analogous performance optimization that is compatible with our current safety-logic of requiring two consistent execution receipts:

flow-go/engine/consensus/matching/core.go

Lines 332 to 340 in 02fa1fc

	// We require at least 2 consistent receipts from different ENs to seal a block. If don't need to fetching receipts.
	// CAUTION: This is a temporary shortcut incompatible with the mature BFT protocol!
	// There might be multiple consistent receipts that commit to a wrong result. To guarantee
	// sealing liveness, we need to fetch receipts from those ENs, whose receipts we don't have yet,
	for _, receiptsForResult := range receipts.GroupByResultID() {
	if receiptsForResult.GroupByExecutorID().NumberGroups() >= 2 {
	continue HEIGHT_LOOP
	}
	}

It also includes the respective comment about BFT safety.

[zhangchiqing]

Good point