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

Only log a block voting error if top version is higher and isn't ideal fork version #3683

Merged
merged 1 commit into from
Apr 28, 2018

Conversation

thaerkh
Copy link
Contributor

@thaerkh thaerkh commented Apr 23, 2018

This is less likely to occur in Monero since it has a much larger hash rate, but it's possible that there are nodes mining a higher version but is forked from somewhere it's known to be otherwise (i.e. at a checkpoint that's known to be v6).
The behaviour this avoids is logging a false positive indicating a potential protocol upgrade.
I speculate that without this PR, the error may trigger if a chain fork like MoneroV doesn't properly configure their network and are broadcasting to Monero daemons.

Copy link
Collaborator

@moneromooo-monero moneromooo-monero left a comment

Choose a reason for hiding this comment

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

It also filters outs true positives though.

@thaerkh
Copy link
Contributor Author

thaerkh commented Apr 23, 2018

When ideal version at a given height is equal to the ideal version overall, being less than hshd.top_version implies that it's unknown and bigger than fork versions known (i.e. if I was running v6, and are getting v7 top versions from broadcasting nodes).
The intended behaviour for ensuring the log occurs has been confirmed on a testnet, and omission on a mainnet that has somebody broadcasting v7 blocks in v2 checkpointed heights (ideal fork version there is 4).
I might be missing an edge case though, can you elaborate on what you mean?

@moneromooo-monero
Copy link
Collaborator

In the past, we've brought a fork forward slightly when we had a planned fork, then needed to make an additional change to consensus rules. I think this would silence that one. It's a rare case I guess since it'd apply to someone who's running a master build, so these people should know what they're doing.

Copy link
Collaborator

@moneromooo-monero moneromooo-monero left a comment

Choose a reason for hiding this comment

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

Could you please also PR this to the release-0.12 branch ? It'd be nice to get it active for the september fork.

@thaerkh
Copy link
Contributor Author

thaerkh commented Apr 23, 2018

You'll find a copy of the PR referenced above, as requested

Copy link
Contributor

@fluffypony fluffypony left a comment

Choose a reason for hiding this comment

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

Reviewed

@fluffypony fluffypony merged commit dad1077 into monero-project:master Apr 28, 2018
fluffypony added a commit that referenced this pull request Apr 28, 2018
dad1077 Only log an error if fork version is higher AND is not known. (Thaer Khawaja)
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

3 participants