Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

skip connecting to invalid identities in the identity table and log error instead of fatal #1138

Merged
merged 5 commits into from Aug 19, 2021

Conversation

vishalchangrani
Copy link
Contributor

Found one more place where it was log.fatal on a bad identity.

@codecov-commenter
Copy link

codecov-commenter commented Aug 13, 2021

Codecov Report

Merging #1138 (c5abe58) into master (e74ecca) will increase coverage by 0.03%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1138      +/-   ##
==========================================
+ Coverage   55.67%   55.71%   +0.03%     
==========================================
  Files         481      481              
  Lines       29391    29387       -4     
==========================================
+ Hits        16364    16373       +9     
+ Misses      10797    10781      -16     
- Partials     2230     2233       +3     
Flag Coverage Δ
unittests 55.71% <0.00%> (+0.03%) ⬆️

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

Impacted Files Coverage Δ
network/p2p/libp2pConnector.go 0.00% <0.00%> (ø)
network/p2p/peerManager.go 75.75% <0.00%> (+2.78%) ⬆️
cmd/util/ledger/migrations/storage_v4.go 42.16% <0.00%> (+0.60%) ⬆️
module/mempool/stdmap/identifier_map.go 84.37% <0.00%> (+3.12%) ⬆️
...sus/approvals/assignment_collector_statemachine.go 50.00% <0.00%> (+7.69%) ⬆️

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 e74ecca...c5abe58. Read the comment docs.

Copy link
Contributor

@huitseeker huitseeker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch!

return
if err != nil {
// one of more identities in the identity table could not be connected to
pm.logger.Error().Err(err).Msg("failed to connect to one or more peers")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems this can only be an NewUnconvertableIdentitiesError in our implementation. Could we unpack this case and thereby maybe get a more usable error message?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so UnconvertableIdentitiesError is logged as multiple individual errors (multi-error) which will list out the Identity and the error reason.
Will that suffice?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, all good!

Copy link
Contributor

@huitseeker huitseeker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

@vishalchangrani vishalchangrani mentioned this pull request Aug 19, 2021
8 tasks
@huitseeker
Copy link
Contributor

@vishalchangrani you could use bors to merge this

@vishalchangrani
Copy link
Contributor Author

bors merge

bors bot added a commit that referenced this pull request Aug 19, 2021
1138: skip connecting to invalid identities in the identity table and log error instead of fatal r=vishalchangrani a=vishalchangrani

Found one more place where it was `log.fatal` on a bad identity.


1163: [network] Check the OriginID of a libp2p message corresponds to its authenticated source r=huitseeker a=huitseeker

Contributes to #1115.
Follow-up of #1129.

Edit: there was no problem with the integration tests, 🤦 

However, TestMiddlewareTestSuit/TestUnsubscribe is still flaky based on the delay in the last step (sending the message once a mesh has formed). But it is problematic on master, see [[1]](https://github.com/onflow/flow-go/runs/3357056741) [[2]](https://github.com/onflow/flow-go/runs/3356508175), so this is not relevant to this PR per se.

Co-authored-by: Vishal <1117327+vishalchangrani@users.noreply.github.com>
Co-authored-by: François Garillot <francois.garillot@dapperlabs.com>
@bors
Copy link
Contributor

bors bot commented Aug 19, 2021

Build failed (retrying...):

@bors
Copy link
Contributor

bors bot commented Aug 19, 2021

@bors bors bot merged commit 85dc4a6 into master Aug 19, 2021
@bors bors bot deleted the vishal/ignore_bad_identities branch August 19, 2021 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants