Skip to content
This repository was archived by the owner on Nov 6, 2020. It is now read-only.

Stop breaking out of loop if a non-canonical hash is found#10566

Closed
HCastano wants to merge 1 commit into
openethereum:masterfrom
HCastano:hc-non-canon-block-logging
Closed

Stop breaking out of loop if a non-canonical hash is found#10566
HCastano wants to merge 1 commit into
openethereum:masterfrom
HCastano:hc-non-canon-block-logging

Conversation

@HCastano
Copy link
Copy Markdown
Contributor

@HCastano HCastano commented Apr 4, 2019

Currently investigating #10085 with the help of @tomusdrw. We have a couple theories floating around, but we aren't sure how to reproduce them on our own.

We've narrowed down the cause to one of three function calls within Blockchain::epoch_transition_for. This PR handles the None case of what we think is the most suspect function call better, as well as adds some additional logging so we can track where we are the loop.

@parity-cla-bot
Copy link
Copy Markdown

It looks like @HCastano signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

@soc1c soc1c added this to the 2.6 milestone Apr 4, 2019
@soc1c soc1c added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Apr 4, 2019
@soc1c soc1c added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A0-pleasereview 🤓 Pull request needs code review. labels Apr 7, 2019
Copy link
Copy Markdown
Collaborator

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

Code looks fine and collecting more data is warranted. Do we want the logging to remain indefinitely though? Or make a note to return to the previous code when we've figured this one out?

@HCastano
Copy link
Copy Markdown
Contributor Author

HCastano commented Apr 9, 2019

I don't think we need all the logging added here, but I think some additions in this area of the code wouldn't hurt (especially if they're at the trace level).

@dvdplm
Copy link
Copy Markdown
Collaborator

dvdplm commented May 24, 2019

@HCastano @soc1c What is the plan for this PR? To merge it or is it used for debugging only? If the latter, perhaps we can close it and leave the branch for those who need it?

@HCastano
Copy link
Copy Markdown
Contributor Author

@dvdplm Hey, sorry for the late reply, I haven't been checking Github all that often recently. It's basically just for debugging. It didn't end up solving the issue, but did provide some extra logs to look at. I think its fine if we close it without deleting it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. M4-core ⛓ Core client code / Rust.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants