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

Move non-verification misbehaviour checks to CheckForMisbehaviour #2007

Closed
3 tasks
colin-axner opened this issue Aug 15, 2022 · 11 comments · Fixed by #3046
Closed
3 tasks

Move non-verification misbehaviour checks to CheckForMisbehaviour #2007

colin-axner opened this issue Aug 15, 2022 · 11 comments · Fixed by #3046
Assignees
Labels
07-tendermint type: code hygiene Clean up code but without changing functionality or interfaces

Comments

@colin-axner
Copy link
Contributor

Summary

In 07-tendermint, we are checking whether something is misbehaviour in VerifyClientMessage, we should instead be doing this in CheckForMisbehaviour. Move this batch of code to `CheckForMisbehaviour


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged/assigned
@colin-axner colin-axner added 07-tendermint type: code hygiene Clean up code but without changing functionality or interfaces labels Aug 15, 2022
@colin-axner colin-axner added this to the 02-client refactor RC milestone Aug 15, 2022
@Daniyal98
Copy link
Contributor

Hello, I would like to start working on this issue. Thank you!

@charleenfei
Copy link
Contributor

@Daniyal98 are you still working on this?

@Daniyal98
Copy link
Contributor

@charleenfei It wasn't assigned to me so I didn't start working on it

@charleenfei
Copy link
Contributor

sorry about that! i am assigning you now :)

@colin-axner
Copy link
Contributor Author

Apologies for the delayed reply @Daniyal98. Will you be able to work on this issue now, or is it not the right time to pickup the issue? This issue is apart of our next iteration (Nov 28 - Dec 11) and we would like to complete it soon

@Daniyal98
Copy link
Contributor

@colin-axner yeah sure. I will try my best :)

@Daniyal98
Copy link
Contributor

@colin-axner the statement of this issue seems a bit misleading. The part that you mentioned is already in CheckForMisbehaviour. Am I missing something?

@colin-axner
Copy link
Contributor Author

Hi @Daniyal98 apologies again for the late reply! When I made the issue, I linked to main, but the code got moved around. The part of the code that should be moved is here, it should be moved into the switch statement case Misbehaviour in CheckForMisbehaviour

The idea is that when we check for misbehaviour, we have three cases to check for:

  • if it is a header, check if we already had a valid update for this height (and that the update isn't a duplicate)
  • if it is misbehaviour, check for time misbehaviour (produced two headers which disobey BFT time restrictions
  • if it is misbheaviour, check for fork misbehaviour (two headers at the same height)

Since CheckForMisbehaviour should check all three cases, moving the code will complete this logical layout with verifyMisbehaviour only verifying that the two headers provided were produced by the validator set

@crodriguezvega
Copy link
Contributor

@Daniyal98 will you have time to work on this one relatively soon? No worries if you can't, but please let us know, so that we can take it otherwise. Thanks!

@Daniyal98
Copy link
Contributor

Yes sure. I'll start working on it right away.
@crodriguezvega

@crodriguezvega
Copy link
Contributor

Yes sure. I'll start working on it right away.
@crodriguezvega

Much appreciated, @Daniyal98!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
07-tendermint type: code hygiene Clean up code but without changing functionality or interfaces
Projects
Status: Done 🥳
4 participants