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

eth: cancel DAO challenge on peer drop (annoying log) #2833

Merged
merged 1 commit into from
Jul 22, 2016

Conversation

karalabe
Copy link
Member

@karalabe karalabe commented Jul 19, 2016

When peers are disconnected for some reason other than the DAO challenge (either locally or remotely), but the challenge is still running, after the 15 sec timeout a log will appear warning of a disconnect (of the already disconnected peer). Although it's fully harmless, it's annoying. This PR makes sure the challenge timer is aborted when the peer drops.

@robotally
Copy link

robotally commented Jul 19, 2016

Vote Count Reviewers
👍 0
👎 0

Updated: Tue Jul 19 09:43:07 UTC 2016

@@ -295,6 +295,13 @@ func (pm *ProtocolManager) handle(p *peer) error {
glog.V(logger.Warn).Infof("%v: timed out DAO fork-check, dropping", p)
pm.removePeer(p.id)
})
// Make sure it's cleaned up if the peer dies off
defer func() {
if p.forkDrop != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

What may set this to nil other than the line below?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

PS: They set it to nil so those parts of the code that check headers for challenges is completely disabled after the challenge passed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could this therefor not be racey?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's all in a single thread: the defer is in the same method that calls handleMsg for every network packet.

@obscuren
Copy link
Contributor

The frequency in which the messages were churned out before has been halted to a minimum. I still get it occasionally but assume that it actually does timeout for those instances.

@karalabe
Copy link
Member Author

We can hide the message completely after a while (i.e. debug log) but I'd keep it around for a bit just to see that everything behaves well.

@karalabe karalabe merged commit c646d28 into ethereum:develop Jul 22, 2016
@obscuren obscuren removed the review label Jul 22, 2016
@fjl fjl modified the milestone: 1.4.11 Aug 5, 2016
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.

4 participants