Skip to content

voter: only primary voter proposes and only once#57

Merged
rphmeier merged 4 commits intomasterfrom
andre/primary-proposer-fix
Mar 29, 2019
Merged

voter: only primary voter proposes and only once#57
rphmeier merged 4 commits intomasterfrom
andre/primary-proposer-fix

Conversation

@andresilva
Copy link
Copy Markdown
Contributor

No description provided.

@rphmeier
Copy link
Copy Markdown
Contributor

We discussed this in the original PR and I was against including the VoterId in the voter at all. We haven't done it for any other messages either

@andresilva
Copy link
Copy Markdown
Contributor Author

Sorry I missed that discussion. What if the local id is exposed from the Environment? I don't really want to spam consensus gossip with useless messages, and we either filter here or on substrate (which means the logic for primary election leaks there).

Copy link
Copy Markdown

@marcio-diaz marcio-diaz 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 @rphmeier wanted to filter them at Substrate level. Except for that lgtm.

@rphmeier
Copy link
Copy Markdown
Contributor

Yeah, although it does seem like a somewhat debatable point. I think we either go all-the-way with VoterId in finality-grandpa or not at all. So if Id is None we don't send messages at all, for instance.

@rphmeier
Copy link
Copy Markdown
Contributor

@andresilva

we either filter here or on substrate (which means the logic for primary election leaks there).

The idea is that with #55 we will just call voter_state.round_data(round).primary == self.id for filtering -- the logic for primary election doesn't need to leak.

@rphmeier rphmeier closed this Mar 29, 2019
@rphmeier rphmeier reopened this Mar 29, 2019
@andresilva
Copy link
Copy Markdown
Contributor Author

I agree that doing the filtering on substrate is more inline with what we have now, especially with #55. I'll update this PR shortly to remove this.

@andresilva
Copy link
Copy Markdown
Contributor Author

After discussing this with @rphmeier we've decided to go "all-in" on using the voter_id and move the logic for filtering messages to this crate. I've updated the PR so that no votes are sent out if we're not a current voter in the voter set. The local voter id is given per-round through Environment::round_data. The protocol state machine still transitions as if the votes were cast, the only difference being that we don't push the messages to the sink. Commits are still pushed out and should be filtered on substrate side (although it may be beneficial in some cases for observers to broadcast commits).

@rphmeier rphmeier merged commit e565fc3 into master Mar 29, 2019
@rphmeier rphmeier deleted the andre/primary-proposer-fix branch March 29, 2019 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants