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

Add function for checking misbehaviors in Vetomint #495

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Jeongseup
Copy link
Contributor

@Jeongseup Jeongseup commented Aug 11, 2023

implement for the issue, #473

@Jeongseup Jeongseup force-pushed the vetomint/add-function-for-checking-misbehaviors branch from e51d3a0 to def5873 Compare August 11, 2023 16:23
@Jeongseup Jeongseup changed the title Feature: add function for checking misbehaviors in Vetomint Add function for checking misbehaviors in Vetomint Aug 17, 2023
Copy link
Member

@junha1 junha1 left a comment

Choose a reason for hiding this comment

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

I think the overall structure should be revised.

  1. Reporting misbehaviors should be done on the same basis of feeding ConsensusEvents. However, the current interface of check_misbehavior() is confusing. Why does it take check_round and check_proposal? When is the function supposed to be called? Why did you take those two very particular parameters?
  2. Don't use println!().It seems you just printed whenever there is no misbehavior. This is just so random.
  3. Please add some comments about important code. For example, I'd put doc comments for every check_~ functions.
  4. The local variable valid_prevotes is named very misleadingly. Why is it valid? You just filter prevotes by their rounds.

@Jeongseup
Copy link
Contributor Author

Jeongseup commented Aug 27, 2023

I modified with your review. but, I don't fully understand the vetomint. so I'm not sure which place i called the check~ function is correct. and some check_invalid_~ functions is in unimplement.

ans1) I deleted the check_misbehavior function. because it didn't seem necessary when I looked at it again.
Instead of, I add some check misbehavior functions for responsing ConsensusResponse::ViolationReport enum which will be used in (https://github.com/postech-dao/simperby/blob/main/consensus/src/state.rs#L225C7-L225C7)

ans2) I removed all println!() macro in my PR

ans3) I already know what comment I should put for evenry check~ function. but I'm not sure what to say. so, Can you give me some guideline for me?

ans4) I changed unimplement state at some functions which had valid_prevotes local variables.


And also, I have a question for this PR. I added some local variable like that (https://github.com/Jeongseup/simperby/blob/vetomint%2Fadd-function-for-checking-misbehaviors/vetomint/src/misbehavior.rs#L82)
Because that's better to see codes by others. Is it alright?

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.

2 participants