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

Vetomint additional review #289

Open
P1n3tree opened this issue Jan 3, 2023 · 0 comments · May be fixed by #352
Open

Vetomint additional review #289

P1n3tree opened this issue Jan 3, 2023 · 0 comments · May be fixed by #352
Assignees

Comments

@P1n3tree
Copy link
Contributor

P1n3tree commented Jan 3, 2023

1.

ConsensusEvent::Prevote {
proposal,
signer,
round,
} => {
state.prevotes.insert(Vote {
proposal,
signer,
round,
});
let mut response = Vec::new();
if let Some(proposal) = proposal {
response.extend(on_4f_non_nil_prevote_in_propose_step(
state, round, proposal,
));
response.extend(on_4f_non_nil_prevote_in_prevote_step(
state, round, proposal,
));

I think that when Prevote event comes, we do not need to call on_4f_non_nil_prevote_in_propose_step

→ parameter "round" in this function means target_round. However, on_4f_non_nil_prevote_in_propose_step’s 4f+1 condition is for vr(valid round, i.e some previous round).

So I think nothing will happen if we call this function.

2.

if let Some(proposal) = proposal {
response.extend(on_4f_non_nil_prevote_in_propose_step(
state, round, proposal,
));
response.extend(on_4f_non_nil_prevote_in_prevote_step(
state, round, proposal,
));
} else {
response.extend(on_4f_nil_prevote(state, round));
}
response.extend(on_5f_prevote(state, round, proposal));
response
}

When I have received 4f+1 non-nil & f nil votes, Both of on_4f_non_nil_prevote_in_prevote_step & on_5f_prevote will return "non-nil precommit" and response.extend() works twice.

I'm not sure how "extend" works, so is it okay to use "extend" like this?

3.

In tendermint paper,

Some of the rules ends with ”for the first time” constraint to denote that it is triggered only the first time a corresponding condition evaluates to true.
This is because those rules do not always change the state of algorithm variables so without this constraint, the algorithm could keep executing those rules forever.

So, on_4f_non_nil_prevote_in_prevote_step should only be called once.

However, I think “executing forever” only applied for updating validValue & validRound. As vetomint’s performance does not important, I think we can leave it as it is, what do you think?
(→ Actually, I think this is not a safety issue, only performance issue.)

@hataehyeok hataehyeok linked a pull request Feb 25, 2023 that will close this issue
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 a pull request may close this issue.

2 participants