Consensus: require multiple receipts to seal #450

AlexHentschel · 2021-02-23T06:48:19Z

production version of MainNet fix: #444 (Require multiple receipts to seal)

Overview of Changes

logic for requiring two receipts from different ENs before sealing;
logic to ensure that we re-request a second receipt (as a single one is insufficient now)
additional unit tests

and

general modularization and restructuring of sealing logic (matching.Core):

in sealableResults we now have distinct methods for that implement the various sealing checks or conditions:

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

Lines 585 to 586 in a04e65d

    
           // Can we seal following the happy-path protocol? 
        
           sealingStatus, err := c.hasEnoughApprovals(incorporatedResult, sealingTracker)

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

Lines 595 to 598 in a04e65d

    
           // Emergency Sealing Fallback: only kicks in if we can't seal following the happy-path sealing 
        
           emergencySealable := false 
        
           if !happySealable { 
        
           	emergencySealable, err = c.emergencySealable(incorporatedResult, lastFinalized)

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

Line 618 in a04e65d

hasMultipleReceipts := c.resultHasMultipleReceipts(incorporatedResult)

consolidated the logic for checking the happy path sealing conditions into the single method hasEnoughApprovals and removed a lot of duplicated requests / queries

consolidated logic for tracking approvals. Specifically optimized set-membership implementation which is now using map-based lookups O(1) instead of iterating over all elements to search for a match O(n):

flow-go/model/flow/incorporated_result.go

Lines 163 to 170 in a04e65d

    
           // BySigner returns a signer's signature if it exists 
        
           func (c *SignatureCollector) BySigner(signerID Identifier) (*crypto.Signature, bool) { 
        
           	idx, found := c.signerIDSet[signerID] 
        
           	if !found { 
        
           		return nil, false 
        
           	} 
        
           	return &c.verifierSignatures[idx], true 
        
           }

Thanks @jordanschalm and @zhangchiqing for your comments on the previous version of this PR: #444. I have addressed all your comments here.

Ran this on localnet with disabled receipts broadcast for the Execution Nodes, to confirm sealing is live.

…re sealing; * fixed logic for re-requesting second receipt (as a single one is insufficient now); * added unit tests

AlexHentschel · 2021-02-23T06:48:37Z

cmd/consensus/main.go

+				requester.WithRetryInitial(2*time.Second),
+				requester.WithRetryMaximum(30*time.Second),


the default for the RetryMaximum was 2min, which is way too slow as it directly impacts time to seal , which is critical for NBA

AlexHentschel · 2021-02-23T06:52:58Z

engine/consensus/matching/core.go

+	// However, as soon as we have at least 2 receipts, the matching Engine will produce a candidate
+	// seal and put it in the mempool. To prevent spamming the logs, we log only when there is
+	// no seal in the IncorporatedResultSeals mempool:
+	_, sealAlreadyGenerated := c.seals.ByID(incorporatedResult.ID()) // IncorporatedResultSeal has the same ID as incorporatedResult


This caused some confusion in the previous version of this PR (see comments). I have revised the comment in response, specifically this part:

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

Lines 736 to 738 in 5df7490

// seal and put it in the mempool. To prevent spamming the logs, we log only when there is

// no seal in the IncorporatedResultSeals mempool:

_, sealAlreadyGenerated := c.seals.ByID(incorporatedResult.ID()) // IncorporatedResultSeal has the same ID as incorporatedResult

Additional context:

Please note the naming convention for matching.Core:

the persistent storage components have names ending in DB, such as:

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

Line 71 in 5df7490

headersDB storage.Headers // used to check sealed headers

Components without DB refer to mempools. Specifically, c.seals has the following type:

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

Line 76 in 5df7490

seals mempool.IncorporatedResultSeals // holds candidate seals for incorporated results that have acquired sufficient approvals; candidate seals are constructed without consideration of the sealability of parent results

It stores IncorporatedResultSeals, whose ID method is defined as

flow-go/model/flow/incorporated_result_seal.go

Lines 16 to 20 in 86124f6

// ID implements flow.Entity.ID for IncorporatedResultSeal to make it capable of

// being stored directly in mempools and storage.

func (s *IncorporatedResultSeal) ID() Identifier {

return s.IncorporatedResult.ID()

}

engine/consensus/matching/core.go

Co-authored-by: Yura <yuraolex@gmail.com>

…sults

engine/consensus/matching/core.go

zhangchiqing · 2021-03-04T17:26:34Z

engine/consensus/matching/core.go

+			return false
+		}
+
+		l := c.log.Info().


would this generate lots of info log?

Happy to change the log level. Let me know what you prefer.
The logging should essentially be almost identical to the initial version you built: #363 (comment) (including logging on info Level). The only noteworthy differences between your version and the revised behaviour as of this PR:

FirstUnmatchedChunkIndex: -1 is now omitted.

SealedByEmergency is only included when we actually evaluated the emergency sealing condition (as opposed to before, where the default value was false even if we didn't check whether the result qualified for emergency sealing)

we also have information whether 2 receipts for the result are known

I would suggest to use debug log.

The log I built only print logs for next unsealed result, whereas the added log here will print log for all unsealed results.

Since you've added extensive logging for the next unsealed result, I think this logging could be debug level

… result has sufficient approvals (for all chunks)

# Conflicts: # engine/consensus/matching/indexer.go # storage/badger/receipts.go # storage/mock/execution_receipts.go # storage/receipts.go

model/flow/execution_receipt.go

durkmurder · 2021-03-15T17:51:06Z

engine/consensus/matching/sealingtracker/sealing_record.go

+	ExecutedBlock      *flow.Header             // block the incorporated Result is for
+	IncorporatedResult *flow.IncorporatedResult // the incorporated result
+
+	// SufficientApprovalsForSealing: True iff all chunks in the result have


Suggested change

// SufficientApprovalsForSealing: True iff all chunks in the result have

// SufficientApprovalsForSealing: True if all chunks in the result have

this is intentional and not a typo 😉 :

[Wiki]: In logic and related fields such as mathematics and philosophy, "if and only if" (shortened as "iff") is a biconditional logical connective between statements, where either both statements are true or both are false.

engine/consensus/matching/sealingtracker/sealing_record.go

durkmurder · 2021-03-15T17:52:05Z

engine/consensus/matching/sealingtracker/sealing_record.go

+	// FirstUnmatchedChunkIndex: Index of first chunk that hasn't received
+	// sufficient approval (ordered by chunk index). Optional value: only set
+	// if SufficientApprovalsForSealing == False and nil otherwise.
+	FirstUnmatchedChunkIndex *uint64


why do we have pointers here?

We don't have a proper value for FirstUnmatchedChunkIndex in some cases. It only has a well-defined value if one of the chunks is missing approvals. If all chunks have sufficient approvals, what should we set FirstUnmatchedChunkIndex to? Previously, we used to set it to -1. Personally, I prefer to have this expressed more explicitly.

In Java, I would have implemented this using Optional. Did some looking around, and it seems people tend to use pointer types for this in Go (-> stackoverflow) ... open to alternative suggestions.

To main purpose of the SealingTracker is to provide logging information. We don't include the values if they haven't been set by the sealing logic:

flow-go/engine/consensus/matching/sealingtracker/sealing_tracker.go

Lines 86 to 94 in bedd704

if record.firstUnmatchedChunkIndex != nil {

kvps["first_unmatched_chunk_index"] = *record.firstUnmatchedChunkIndex

}

if record.qualifiesForEmergencySealing != nil {

kvps["qualifies_for_emergency_sealing"] = *record.qualifiesForEmergencySealing

}

if record.hasMultipleReceipts != nil {

kvps["has_multiple_receipts"] = *record.hasMultipleReceipts

}

engine/consensus/matching/core_test.go

durkmurder · 2021-03-15T18:16:06Z

Looks good to me, just a few comments related to tests and some minor stuff.

zhangchiqing · 2021-03-15T18:15:41Z

engine/consensus/matching/sealingtracker/sealing_record.go

+	if rs == nil {
+		return
+	}


if rs is nil, then rs.SetHasMultipleReceipts would hit nil pinter first

Suggested change

if rs == nil {

return

}

You are right, having nil as legitimate value for SealingRecord is just too confusing and error-prone. Now working only with non-nil values SealingRecords. Hope that is clearer now 😅

engine/consensus/matching/sealingtracker/sealing_record.go

engine/consensus/matching/sealingtracker/sealing_tracker.go

zhangchiqing · 2021-03-15T18:28:45Z

engine/consensus/matching/sealingtracker/sealing_tracker.go

+		ExecutedBlock:      executedBlock,
+		IncorporatedResult: incorporatedResult,
+	}
+	st.records = append(st.records, record)


when do we remove the record from it, would the memory keeps growing as we record and hold more?

zhangchiqing · 2021-03-15T19:03:33Z

engine/consensus/matching/core.go

+//     have a child yet. Then, the chunk assignment cannot be computed.
+//   - All other errors are unexpected and symptoms of internal bugs, uncovered edge cases,
+//     or a corrupted internal node state. These are all fatal failures.
+func (c *Core) hasEnoughApprovals(incorporatedResult *flow.IncorporatedResult, sealingTracker *sealingtracker.SealingTracker) (*sealingtracker.SealingRecord, error) {


This function is too complex as it takes in a stateful sealingTracker type and returns another sealingRecord type.

I would suggest not to depend on those types in order to keep it simple.

This caller seems only care about whether there is enough approvals for the given result, and the FirstUnmatchedChunkIndex if not enough. In other words, we don't care about the chunk that had enough approvals, right?

So we could let this function return (FirstUnmatchedChunkIndex int, hasEnoughApprovals bool).

If we had enough approvals, we return (-1, true); if we the 3rd chunk didn't have enough approval, we return (2, false)

👍 Agree. I was concerned about the complexity here as well.

I have refactored hasEnoughApprovals to:

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

Line 639 in 9fd1bbb

func (c *Core) hasEnoughApprovals(incorporatedResult *flow.IncorporatedResult) (*sealingtracker.SealingRecord, error) {

While I still return the SealingRecord for the incorporatedResult, I have made several modifications which hopefully address the heart of your concern:

removed the stateful sealingTracker from the arguments

Regarding the usage of SealingRecord, I have applied significant simplifications:

SealingRecord is now directly generated by hasEnoughApprovals (i.e. I removed SealingTracker as a factory for SealingRecords)

SealingRecord is never nil and always reflects accurately the sealing status of the incorporated Result

Essentially, the SealingRecord holds the values (FirstUnmatchedChunkIndex int, hasEnoughApprovals bool) plus some other flags for later sealing checks.

Please let me know whether the code is clear enough with these revisions.

engine/consensus/matching/core.go

zhangchiqing · 2021-03-15T19:10:56Z

engine/consensus/matching/core.go

 		Int("sealable_incorporated_results", len(sealedBlockIDs)).
-		Int64("duration_ms", time.Since(startTime).Milliseconds()).
+		Str("next_unsealed_results", sealingTracker.String()).


Does sealingTracker.String() include all records, or just the next unsealed result?

Could you provide a sample log?

Better not all records, because there will be too much noise, as we only care about the next unsealed when debugging.

we only track the sealing status of the IncorporatedResult(s) for the next unsealed finalized block:

flow-go/engine/consensus/matching/sealingtracker/sealing_tracker.go

Lines 58 to 65 in 3b33804

// Track tracks the given SealingRecord, provided the it should be tracked

// according to the SealingTracker's internal policy.

func (st *SealingTracker) Track(sealingRecord *SealingRecord) {

executedBlockID := sealingRecord.IncorporatedResult.Result.BlockID

if st.isRelevant(executedBlockID) {

st.records = append(st.records, sealingRecord)

}

}

The logging behaviour should be almost identical to the initial version you built: #363 (comment)

When missing approval for the next unsealed block

[{ "executed_block_height":2190957487, "executed_block_id":"9efa9526c0ab51db9c5a904bfc589f95955c1659f6b439196c0146dc8e2a3694", "first_unmatched_chunk_index":2, "incorporated_result_id":"b159f2623bd9d660fa1125d05487d7658e5725fcba479568da139e880df8422e", "number_chunks":17, "qualifies_for_emergency_sealing":false, "result_id":"53c8d0dd81bf4aef407331c2388bcdd53f4bd5ae5cb81ad5242a9ee6655706db", "sufficient_approvals_for_sealing":false}]

When sealed by emergency

The SealedByEmergency becomes true

[{ "executed_block_height":2190957487, "executed_block_id":"9efa9526c0ab51db9c5a904bfc589f95955c1659f6b439196c0146dc8e2a3694", "first_unmatched_chunk_index":2, "has_multiple_receipts":true, "incorporated_result_id":"b159f2623bd9d660fa1125d05487d7658e5725fcba479568da139e880df8422e", "number_chunks":17, "qualifies_for_emergency_sealing":true, "result_id":"53c8d0dd81bf4aef407331c2388bcdd53f4bd5ae5cb81ad5242a9ee6655706db", "sufficient_approvals_for_sealing":false}]

Looks good. Thanks for providing the log

zhangchiqing · 2021-03-15T19:28:10Z

engine/consensus/matching/core.go

-		// contains a receipt that commits to this result.
-		assignment, err := c.assigner.Assign(incorporatedResult.Result, incorporatedResult.IncorporatedBlockID)
+		// Can we seal following the happy-path protocol?
+		sealingStatus, err := c.hasEnoughApprovals(incorporatedResult, sealingTracker)


⚠️ This behavior is different from what I expected.

What I expected was that:

we will require multiple receipts to produce a seal

in other words, we will never produce a seal if there is less two receipts.

Now the new behavior is that:

we will require multiple receipts and either enough approvals or emergency sealing to produce a seal

in other words, we won't produce a seal even if we've received multiple receipts, but haven't received enough approvals yet, and emergency sealing has not been triggered.

I'm not comfortable to have the sealing logic requiring enough approvals, because we still have one unsolved issue, which is verification node can't resume the verification on results which are produced when it's off. So I'm afraid the emergency sealing will often be triggered, and makes the sealing sometimes fast and sometimes slow, making it hard to debug.

I would suggest to keep the original plan to just seal with multiple receipts until we fix the sealing liveness issue

You are right that the logic seals under the condition

we will require multiple receipts and either enough approvals or emergency sealing to produce a seal

This is also stated here in the code comment:

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

Lines 605 to 612 in 34a14af

// Determine sealability:

// (i) the incorporatedResult must qualify for happy path sealing

// or qualify for emergency sealing

// AND

// (ii) there must be at least 2 receipts from _different_ ENs

// committing to the result

// comment: we evaluate condition (ii) only if (i) is true

if happySealable || emergencySealable {

Essentially, the consensus node already implements the general case, where we could require some approvals to seal (or wait until emergency sealing kicks in). With the parametrization DefaultRequiredApprovalsForSealConstruction = 0, the function hasEnoughApprovals always returns true (for any incorporatedResult). Therefore, (i) is always true and the sealing condition

(i) the incorporatedResult must qualify for happy path sealing or qualify for emergency sealing AND (ii) there must be at least 2 receipts from _different_ ENs committing to the result

reduces to

there must be at least 2 receipts from _different_ ENs committing to the result

which is exactly what you expected.

Note:

From my perspective, the current logic should be the same as what we are currently running on Mainnet (PR #444):

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

Lines 812 to 817 in c4ea872

if matched || emergencySealed {

if e.resultHasMultipleReceipts(incorporatedResult) {

// add the result to the results that should be sealed

results = append(results, incorporatedResult)

}

}

I have added the following explicit shortcut in hasEnoughApprovals

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

Lines 646 to 649 in 4bdb072

// shortcut: if we don't require any approvals, any incorporatedResult has enough approvals

if c.requiredApprovalsForSealConstruction == 0 {

return sealingTracker.SufficientApprovals(incorporatedResult), nil

}

which hopefully makes the logic clearer

it shortcuts some work of determining assignments if we don't require any approvals to seal

Co-authored-by: Yura <yuraolex@gmail.com>

…ow-go into alex/5341_two-receipts-to-seal

• removed nil as a valid instance of SealingRecord • moved String generation from SealingRecord to SealingTracker

…ex/5341_two-receipts-to-seal

zhangchiqing

Looks good, nice work!

zhangchiqing · 2021-03-16T16:38:08Z

engine/consensus/matching/core.go

+			return false
+		}
+
+		l := c.log.Info().


I would suggest to use debug log.

The log I built only print logs for next unsealed result, whereas the added log here will print log for all unsealed results.

Since you've added extensive logging for the next unsealed result, I think this logging could be debug level

zhangchiqing · 2021-03-16T16:40:00Z

engine/consensus/matching/core.go

+		// (ii) there must be at least 2 receipts from _different_ ENs
+		//      committing to the result
+		// comment: we evaluate condition (ii) only if (i) is true
+		if sealableWithEnoughApprovals || emergencySealable {


[optional] there are quite a few nested if and || conditions in this function, would be great to keep the logic flat, however I haven't found a cleaner way. Maybe something for later in the next phase.

flattened the logic a bit:

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

Lines 598 to 613 in c72ae1e

// Determine sealability:

// (i) the incorporatedResult must qualify for happy path sealing

// or qualify for emergency sealing

// AND

// (ii) there must be at least 2 receipts from _different_ ENs

// committing to the result

// comment: we evaluate condition (ii) only if (i) is true

if !(sealableWithEnoughApprovals || emergencySealable) { // condition (i) is false

continue

}

hasMultipleReceipts := c.resultHasMultipleReceipts(incorporatedResult)

sealingStatus.SetHasMultipleReceipts(hasMultipleReceipts)

if !hasMultipleReceipts { // condition (ii) is false

continue

}

results = append(results, incorporatedResult) // add the result to the results that should be sealed

not sure whether this is simpler or just as complex (only in a different way) 😅

# Conflicts: # engine/access/rpc/backend/backend_test.go

zhangchiqing · 2021-03-16T19:20:18Z

engine/consensus/matching/core.go

+		// (ii) there must be at least 2 receipts from _different_ ENs
+		//      committing to the result
+		// comment: we evaluate condition (ii) only if (i) is true
+		if !(sealableWithEnoughApprovals || emergencySealable) { // condition (i) is false


Suggested change

if !(sealableWithEnoughApprovals || emergencySealable) { // condition (i) is false

if sealableWithEnoughApprovals == false && emergencySealable == false { // condition (i) is false

Alexander Hentschel added 2 commits February 22, 2021 21:46

* polished logic for requiring two receipts from _different_ ENs befo…

7e52970

…re sealing; * fixed logic for re-requesting second receipt (as a single one is insufficient now); * added unit tests

updated default parameters for Consensus nodes and Localnet

5df7490

AlexHentschel requested review from zhangchiqing, Kay-Zee, durkmurder and jordanschalm February 23, 2021 06:48

AlexHentschel self-assigned this Feb 23, 2021

AlexHentschel commented Feb 23, 2021

View reviewed changes

AlexHentschel mentioned this pull request Feb 23, 2021

MAINNET FIX v0.14: require multiple receipts to seal V3 #444

Merged

AlexHentschel commented Feb 23, 2021

View reviewed changes

durkmurder reviewed Feb 23, 2021

View reviewed changes

engine/consensus/matching/core.go Outdated Show resolved Hide resolved

durkmurder approved these changes Feb 23, 2021

View reviewed changes

Alexander Hentschel and others added 2 commits February 23, 2021 14:43

Update engine/consensus/matching/core.go

946e188

Co-authored-by: Yura <yuraolex@gmail.com>

Merge branch 'master' into alex/5341_two-receipts-to-seal

70c922d

AlexHentschel mentioned this pull request Feb 26, 2021

[Verification] Yahya/assigner-engine #427

Merged

Alexander Hentschel added 5 commits February 27, 2021 13:03

Merge branch 'master' into alex/5341_two-receipts-to-seal

40a5b1d

Merge branch 'master' into alex/5341_two-receipts-to-seal

b8523da

updated tests

cd50cf3

new mock

ffcd011

added GroupBy functionality to Execution Receipts and Incorporated Re…

4c0b22d

…sults

zhangchiqing approved these changes Mar 4, 2021

View reviewed changes

Alexander Hentschel added 8 commits March 4, 2021 12:25

added godocs; removed unncessary loops from sealing logic

44e7983

consolidated and optimized logic to determine whether an incorporated…

ab64075

… result has sufficient approvals (for all chunks)

updated goDoc

99d6048

wip

6c467f0

matching core: sealing logic modularization (work in progress)

ab5aa25

first complete draft

cdf738b

Merge branch 'master' into alex/5341_two-receipts-to-seal

a04e65d

# Conflicts: # engine/consensus/matching/indexer.go # storage/badger/receipts.go # storage/mock/execution_receipts.go # storage/receipts.go

linted code

34a14af

durkmurder reviewed Mar 15, 2021

View reviewed changes

model/flow/execution_receipt.go Outdated Show resolved Hide resolved

durkmurder reviewed Mar 15, 2021

View reviewed changes

engine/consensus/matching/sealingtracker/sealing_record.go Outdated Show resolved Hide resolved

durkmurder reviewed Mar 15, 2021

View reviewed changes

engine/consensus/matching/core_test.go Outdated Show resolved Hide resolved

durkmurder reviewed Mar 15, 2021

View reviewed changes

engine/consensus/matching/core_test.go Outdated Show resolved Hide resolved

durkmurder reviewed Mar 15, 2021

View reviewed changes

engine/consensus/matching/core_test.go Outdated Show resolved Hide resolved

zhangchiqing reviewed Mar 15, 2021

View reviewed changes

Alexander Hentschel and others added 11 commits March 15, 2021 13:25

fixed some of the test

4bdb072

cleanup

7199450

Update engine/consensus/matching/sealingtracker/sealing_record.go

643b37f

Co-authored-by: Yura <yuraolex@gmail.com>

updated comment for clarity

9dd4c99

Merge branch 'alex/5341_two-receipts-to-seal' of github.com:onflow/fl…

cce6055

…ow-go into alex/5341_two-receipts-to-seal

some trial

b40fbb7

restructured Sealing Tracker:

9873c2e

• removed nil as a valid instance of SealingRecord • moved String generation from SealingRecord to SealingTracker

Merge branch 'alex/5341_two-receipts-to-seal_-_exploration01' into al…

3b33804

…ex/5341_two-receipts-to-seal

updated matching.Core unit tests

bedd704

fixed remaining unit tests

9fd1bbb

attempt to fix integration test

0341d6f

zhangchiqing approved these changes Mar 16, 2021

View reviewed changes

Alexander Hentschel added 4 commits March 16, 2021 11:45

last round of final polishing

56c648c

Merge branch 'master' into alex/5341_two-receipts-to-seal

14002a5

# Conflicts: # engine/access/rpc/backend/backend_test.go

linted code

9783727

fixed typo

c72ae1e

zhangchiqing reviewed Mar 16, 2021

View reviewed changes

AlexHentschel merged commit f6e12e8 into master Mar 16, 2021

AlexHentschel deleted the alex/5341_two-receipts-to-seal branch March 16, 2021 19:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consensus: require multiple receipts to seal #450

Consensus: require multiple receipts to seal #450

AlexHentschel commented Feb 23, 2021 •

edited

AlexHentschel Feb 23, 2021

AlexHentschel Feb 23, 2021 •

edited

zhangchiqing Mar 4, 2021

AlexHentschel Mar 16, 2021 •

edited

zhangchiqing Mar 16, 2021

durkmurder Mar 15, 2021

AlexHentschel Mar 15, 2021

durkmurder Mar 15, 2021

AlexHentschel Mar 15, 2021 •

edited

AlexHentschel Mar 16, 2021

durkmurder commented Mar 15, 2021

zhangchiqing Mar 15, 2021

AlexHentschel Mar 16, 2021 •

edited

zhangchiqing Mar 15, 2021

zhangchiqing Mar 15, 2021

AlexHentschel Mar 16, 2021 •

edited

zhangchiqing Mar 15, 2021

AlexHentschel Mar 15, 2021 •

edited

AlexHentschel Mar 16, 2021 •

edited

zhangchiqing Mar 16, 2021

zhangchiqing Mar 15, 2021

AlexHentschel Mar 15, 2021 •

edited

AlexHentschel Mar 15, 2021

zhangchiqing left a comment

zhangchiqing Mar 16, 2021

zhangchiqing Mar 16, 2021

AlexHentschel Mar 16, 2021 •

edited

zhangchiqing Mar 16, 2021

	// Can we seal following the happy-path protocol?
	sealingStatus, err := c.hasEnoughApprovals(incorporatedResult, sealingTracker)

	// Emergency Sealing Fallback: only kicks in if we can't seal following the happy-path sealing
	emergencySealable := false
	if !happySealable {
	emergencySealable, err = c.emergencySealable(incorporatedResult, lastFinalized)

	// BySigner returns a signer's signature if it exists
	func (c SignatureCollector) BySigner(signerID Identifier) (crypto.Signature, bool) {
	idx, found := c.signerIDSet[signerID]
	if !found {
	return nil, false
	}
	return &c.verifierSignatures[idx], true
	}

		requester.WithRetryInitial(2*time.Second),
		requester.WithRetryMaximum(30*time.Second),

	// seal and put it in the mempool. To prevent spamming the logs, we log only when there is
	// no seal in the IncorporatedResultSeals mempool:
	_, sealAlreadyGenerated := c.seals.ByID(incorporatedResult.ID()) // IncorporatedResultSeal has the same ID as incorporatedResult

	// ID implements flow.Entity.ID for IncorporatedResultSeal to make it capable of
	// being stored directly in mempools and storage.
	func (s *IncorporatedResultSeal) ID() Identifier {
	return s.IncorporatedResult.ID()
	}

	// SufficientApprovalsForSealing: True iff all chunks in the result have
	// SufficientApprovalsForSealing: True if all chunks in the result have

	if record.firstUnmatchedChunkIndex != nil {
	kvps["first_unmatched_chunk_index"] = *record.firstUnmatchedChunkIndex
	}
	if record.qualifiesForEmergencySealing != nil {
	kvps["qualifies_for_emergency_sealing"] = *record.qualifiesForEmergencySealing
	}
	if record.hasMultipleReceipts != nil {
	kvps["has_multiple_receipts"] = *record.hasMultipleReceipts
	}

	// Track tracks the given SealingRecord, provided the it should be tracked
	// according to the SealingTracker's internal policy.
	func (st SealingTracker) Track(sealingRecord SealingRecord) {
	executedBlockID := sealingRecord.IncorporatedResult.Result.BlockID
	if st.isRelevant(executedBlockID) {
	st.records = append(st.records, sealingRecord)
	}
	}

	// Determine sealability:
	// (i) the incorporatedResult must qualify for happy path sealing
	// or qualify for emergency sealing
	// AND
	// (ii) there must be at least 2 receipts from _different_ ENs
	// committing to the result
	// comment: we evaluate condition (ii) only if (i) is true
	if happySealable \|\| emergencySealable {

	if matched \|\| emergencySealed {
	if e.resultHasMultipleReceipts(incorporatedResult) {
	// add the result to the results that should be sealed
	results = append(results, incorporatedResult)
	}
	}

	// shortcut: if we don't require any approvals, any incorporatedResult has enough approvals
	if c.requiredApprovalsForSealConstruction == 0 {
	return sealingTracker.SufficientApprovals(incorporatedResult), nil
	}

	if !(sealableWithEnoughApprovals \|\| emergencySealable) { // condition (i) is false
	if sealableWithEnoughApprovals == false && emergencySealable == false { // condition (i) is false

Consensus: require multiple receipts to seal #450

Consensus: require multiple receipts to seal #450

Conversation

AlexHentschel commented Feb 23, 2021 • edited

Overview of Changes

Choose a reason for hiding this comment

AlexHentschel Feb 23, 2021 • edited

Choose a reason for hiding this comment

Additional context:

Choose a reason for hiding this comment

AlexHentschel Mar 16, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexHentschel Mar 15, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

durkmurder commented Mar 15, 2021

Choose a reason for hiding this comment

AlexHentschel Mar 16, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexHentschel Mar 16, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexHentschel Mar 15, 2021 • edited

Choose a reason for hiding this comment

AlexHentschel Mar 16, 2021 • edited

Choose a reason for hiding this comment

When missing approval for the next unsealed block

When sealed by emergency

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexHentschel Mar 15, 2021 • edited

Choose a reason for hiding this comment

Note:

Choose a reason for hiding this comment

zhangchiqing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexHentschel Mar 16, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexHentschel commented Feb 23, 2021 •

edited

AlexHentschel Feb 23, 2021 •

edited

AlexHentschel Mar 16, 2021 •

edited

AlexHentschel Mar 15, 2021 •

edited

AlexHentschel Mar 16, 2021 •

edited

AlexHentschel Mar 16, 2021 •

edited

AlexHentschel Mar 15, 2021 •

edited

AlexHentschel Mar 16, 2021 •

edited

AlexHentschel Mar 15, 2021 •

edited

AlexHentschel Mar 16, 2021 •

edited