-
Notifications
You must be signed in to change notification settings - Fork 648
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
Partial fix for issue 303 #336
Conversation
tested and works good in the witness node, nice output:
However in the tests running a random one as:
We could just remove the exception log at https://github.com/pmconrad/graphene/blob/165c914308b8b6df915b9fbd79a3279a1bf2ae2e/libraries/chain/db_management.cpp#L171 It will not be the first catch with no log in the code, there is actually one just above at https://github.com/pmconrad/graphene/blob/165c914308b8b6df915b9fbd79a3279a1bf2ae2e/libraries/chain/db_management.cpp#L164 Output with that change in the same test case will be:
I will approve it as it is with a commit to remove the exception wlog is that is not a problem. adding @abitmore so he can take another look. good job :) |
The message still appears in those unit tests that don't create a sufficient number of blocks. In #303 I identified 2 reasons for the message, and this PR fixes only the first. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is good enough, i am merging it.
This PR gets rid of the "no blocks to pop" message in witness_node and some unit tests.
IMO a better fix would be to define a clear distinction between "open" and "closed" states in the database, and handle state transitions within the database code. Feel free to reject this PR if you agree. :-)