[Consensus] Hotfix: Fix synchornization handling response #1001

zhangchiqing · 2021-07-20T18:21:57Z

This PR fixes a bug that when there is network connectivity issue when sending the sync request, the node will crash. The fix will simply log the error.

jordanschalm

I'm OK to merge this hotfix, but in general I think we should be keeping the error return, but capturing these expected error cases explicitly and wrapping the error with type information so that the higher-up layer is able to handle it appropriately rather than causing a crash.

codecov-commenter · 2021-07-20T19:05:00Z

Codecov Report

Merging #1001 (586f0f1) into master (208bb5e) will decrease coverage by 0.09%.
The diff coverage is 8.10%.

@@            Coverage Diff             @@
##           master    #1001      +/-   ##
==========================================
- Coverage   53.38%   53.28%   -0.10%     
==========================================
  Files         318      318              
  Lines       21487    21510      +23     
==========================================
- Hits        11471    11462       -9     
- Misses       8450     8483      +33     
+ Partials     1566     1565       -1

Flag	Coverage Δ
unittests	`53.28% <8.10%> (-0.10%)`	⬇️

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

Impacted Files	Coverage Δ
engine/consensus/ingestion/engine.go	`47.09% <0.00%> (-6.19%)`	⬇️
engine/consensus/sealing/engine.go	`49.00% <0.00%> (-1.78%)`	⬇️
engine/errors.go	`15.78% <ø> (ø)`
engine/common/synchronization/engine.go	`64.21% <12.50%> (-1.07%)`	⬇️
engine/enqueue.go	`66.66% <25.00%> (-9.34%)`	⬇️
module/mempool/epochs/transactions.go	`94.73% <0.00%> (-5.27%)`	⬇️
cmd/util/ledger/migrations/storage_v4.go	`41.56% <0.00%> (-0.61%)`	⬇️

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 208bb5e...586f0f1. Read the comment docs.

huitseeker · 2021-07-20T19:32:20Z

engine/common/synchronization/engine.go


 	final := e.finalSnapshot().head
 	e.core.HandleHeight(final, res.Height)
-	return nil


What's the logic for not inspecting errors and at least logging them here?

I remove this line, because HandleHeight doesn't return error, and there isn't a case that would return a non-nil error. So I just removed this non-existent case

huitseeker · 2021-07-20T19:32:40Z

engine/common/synchronization/engine.go

@@ -610,7 +603,6 @@ func (e *Engine) onBlockResponse(originID flow.Identifier, res *messages.BlockRe
 		}
 		e.comp.SubmitLocal(synced)
 	}
-	return nil


What's the logic for not inspecting errors and at least logging them here?

same here. there is no error ever returned

AlexHentschel

Echoing Jordan's comment, I think just logging errors and continuing on a best-effort basis is fundamentally incompatible with working towards a BFT implementation:

flow-go/engine/common/synchronization/engine.go

Lines 431 to 433 in 214746d

    
           if err != nil { 
        
           	engine.LogError(e.log, err) 
        
           }

(please see TPM from February 11th for reasoning)

I am ok with this as a hot fix, but I think this is not a viable approach for master. I would like to propose the revisions in #1008 (PR is targeting this branch for now).

…error-handling-in-synchronization # Conflicts: # engine/common/synchronization/engine.go

Co-authored-by: Leo Zhang <zhangchiqing@gmail.com>

…ronization proposal for handling networking errors in `synchronization.Engine`

synzhu · 2021-07-26T22:42:45Z

engine/common/synchronization/engine.go

+			Str("origin", originID.String()).
+			Logger()
+		if errors.Is(err, engine.IncompatibleInputTypeError) {
+			lg.Error().Msg("received message with incompatible type")


@AlexHentschel Do we actually want to return after this line? If I understand correctly, this if statement captures the case where we get a message with incompatible input type from the network, right? In that case, we probably shouldn't crash? Without the return after this line we will proceed to the lg.Fatal which comes after.

Good question.

I think throwing fatal errors here is intended, as explained here and here

added new sentinel error engine.IncompatibleInputTypeError, which engines can raise, if they receive an input that does not have any of the expected types (previously, engines just errored with an unspecific error)
this allows us to reject inputs from the networking layer with incompatible types
In contrast, an engine should generally not receive an incompatible input from a trusted internal component within the node. This would likely be an implementation bug and should crash the node

an engine should generally not receive an incompatible input from a trusted internal component within the node. This would likely be an implementation bug and should crash the node

I believe this second case is reflected in the code here. However, my concern about the code above is that it seems to represent the first case:

this allows us to reject inputs from the networking layer with incompatible types

The code above is in Submit and not SubmitLocal, which means it is processing a message which came from the network, right? So if we receive an invalid input from the network layer, shouldn't we just log the error, reject the input, and continue? We only want to throw fatal error if the invalid input is from an internal component.

thanks. Good catch. Yes, this would be from external source, where we could expect that they feed us with invalid inputs. I intended to return here (hence logging the error), but I messed this up.

Cool. Thanks for the catch @smnzhu

synzhu · 2021-07-26T22:43:10Z

engine/consensus/ingestion/engine.go

+			Str("origin", originID.String()).
+			Logger()
+		if errors.Is(err, engine.IncompatibleInputTypeError) {
+			lg.Error().Msg("received message with incompatible type")


Yes. Thanks

synzhu · 2021-07-26T22:43:32Z

engine/consensus/sealing/engine.go

+			Str("origin", originID.String()).
+			Logger()
+		if errors.Is(err, engine.IncompatibleInputTypeError) {
+			lg.Error().Msg("received message with incompatible type")


… type.

AlexHentschel · 2021-07-27T01:40:28Z

@smnzhu thanks for noticing the missing error returns for external inputs of incompatible type. You are right, these should not crash the node.

@zhangchiqing I took the liberty to add the three missing returns and commit (74f6395) directly to your branch, because I screwed this up originally in my PR. Hope that was ok 😅 (?)

AlexHentschel

🎸

fix error handling

5386370

zhangchiqing requested review from jordanschalm, vishalchangrani and AlexHentschel July 20, 2021 18:23

zhangchiqing force-pushed the leo/fix-synchornization-handle-response branch from afc6f75 to 113bf1b Compare July 20, 2021 18:25

jordanschalm approved these changes Jul 20, 2021

View reviewed changes

fix synchornization handling response

214746d

zhangchiqing force-pushed the leo/fix-synchornization-handle-response branch from 113bf1b to 214746d Compare July 20, 2021 18:51

huitseeker mentioned this pull request Jul 20, 2021

Retry unit-tests & integration-tests twice #1004

Closed

huitseeker reviewed Jul 20, 2021

View reviewed changes

Alexander Hentschel added 3 commits July 20, 2021 16:49

removed LogError from all consensus engines

1866782

handling networking errors in sync engine

f3e9391

cleanup

3f865c1

AlexHentschel requested changes Jul 21, 2021

View reviewed changes

Merge branch 'leo/fix-synchornization-handle-response' into alex/fix-…

1f0439f

…error-handling-in-synchronization # Conflicts: # engine/common/synchronization/engine.go

This was referenced Jul 21, 2021

proposal for handling networking errors in synchronization.Engine #1008

Merged

fix error handling in synchornization engine #999

Closed

Alexander Hentschel and others added 6 commits July 26, 2021 10:47

removed error returns that were always nil

cabc8ac

Update engine/common/synchronization/engine.go

b75f322

Co-authored-by: Leo Zhang <zhangchiqing@gmail.com>

Update engine/common/synchronization/engine.go

5bbe2ea

Co-authored-by: Leo Zhang <zhangchiqing@gmail.com>

Merge pull request #1008 from onflow/alex/fix-error-handling-in-synch…

7db785a

…ronization proposal for handling networking errors in `synchronization.Engine`

fix lint

dbf9cae

Merge branch 'master' into leo/fix-synchornization-handle-response

cb6e2a6

synzhu reviewed Jul 26, 2021

View reviewed changes

zhangchiqing and others added 2 commits July 26, 2021 16:18

fix lint

586f0f1

added missing return statements for _external_ inputs of incompatible…

74f6395

… type.

AlexHentschel approved these changes Jul 27, 2021

View reviewed changes

synzhu approved these changes Jul 27, 2021

View reviewed changes

zhangchiqing merged commit c3d239e into master Jul 27, 2021

zhangchiqing deleted the leo/fix-synchornization-handle-response branch July 27, 2021 02:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Consensus] Hotfix: Fix synchornization handling response #1001

[Consensus] Hotfix: Fix synchornization handling response #1001

zhangchiqing commented Jul 20, 2021 •

edited

jordanschalm left a comment

codecov-commenter commented Jul 20, 2021 •

edited

huitseeker Jul 20, 2021

zhangchiqing Jul 20, 2021

huitseeker Jul 20, 2021

zhangchiqing Jul 20, 2021

AlexHentschel left a comment •

edited

synzhu Jul 26, 2021 •

edited

zhangchiqing Jul 26, 2021

synzhu Jul 27, 2021 •

edited

AlexHentschel Jul 27, 2021

zhangchiqing Jul 27, 2021

synzhu Jul 26, 2021

AlexHentschel Jul 27, 2021

synzhu Jul 26, 2021

AlexHentschel Jul 27, 2021

AlexHentschel commented Jul 27, 2021 •

edited

AlexHentschel left a comment

[Consensus] Hotfix: Fix synchornization handling response #1001

[Consensus] Hotfix: Fix synchornization handling response #1001

Conversation

zhangchiqing commented Jul 20, 2021 • edited

jordanschalm left a comment

Choose a reason for hiding this comment

codecov-commenter commented Jul 20, 2021 • edited

Codecov Report

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 left a comment • edited

Choose a reason for hiding this comment

synzhu Jul 26, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

synzhu Jul 27, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexHentschel commented Jul 27, 2021 • edited

AlexHentschel left a comment

Choose a reason for hiding this comment

zhangchiqing commented Jul 20, 2021 •

edited

codecov-commenter commented Jul 20, 2021 •

edited

AlexHentschel left a comment •

edited

synzhu Jul 26, 2021 •

edited

synzhu Jul 27, 2021 •

edited

AlexHentschel commented Jul 27, 2021 •

edited