Make clearer that the distribution iterates over all active validators

[hjorthjort]

Description

We found that the current distribution code for distributing to all bonded validators wasn't perfectly clear, and it confused both me and @sunnya97. Here's a suggestion on how to fix that.

This fact is already reflected in the docs, but the wording in the code made us suspect there was a mismatch. I think @sunnya97 can look into and double check that req.LastCommitInfo.GetVotes() actually returns all bonded validators.

---

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

• Targeted PR against correct branch (see CONTRIBUTING.md)
• Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
• Code follows the module structure standards.
• Wrote unit and integration tests
• Updated relevant documentation (docs/) or specification (x/<module>/spec/)
• Added relevant godoc comments.
• Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
• Re-reviewed Files changed in the Github PR explorer
• Review Codecov Report in the comment section below once CI passes

[ValarDragon]

I don't think there is a bug at least, since req.LastCommitInfo.GetVotes() is all validators, see:
https://github.com/tendermint/tendermint/blob/0d0181856b3ec54946cb1fa7ce56651606de8910/state/execution.go#L366-L372

I think this could be improved by changing the naming of this, and making it not be called GetVotes to instead perhaps GetVoteStatuses? (I completely agree that the current naming makes it unclear if you're iterating over just the validators who voted, or all validators)

[hjorthjort]

Thanks @ValarDragon for checking this. We suspected that there wasn't a bug, but it's nice to get that verified.

Do you know if this is tested with a unit test somewhere? That GetVotes supplies a list of all bonded validators, regardless of if they voted or not? That would be good to have since the distribution crucially relies on it, but the description and name doesn't really make that clear.

[robert-zaremba]

Please move the argument doc comment to the function doc comment. Also left a suggestion to use more concise name.

[robert-zaremba]

Suggested change

	previousProposer sdk.ConsAddress, bondedValidatorsVoteInfo []abci.VoteInfo,
	previousProposer sdk.ConsAddress, bondedVotes []abci.VoteInfo,

[robert-zaremba]

if we don't prefer concise names and we don't need to repeat a type name in the variable name.

[robert-zaremba]

Do you know if this is tested with a unit test somewhere?

Maybe it would be good to add a test?

[lgtm-com]

This pull request introduces 4 alerts when merging 41f7693 into deaee53 - view on LGTM.com

new alerts:

• 2 for Useless assignment to local variable
• 2 for Unreachable statement

[hjorthjort]

Do you know if this is tested with a unit test somewhere?

Maybe it would be good to add a test?

Looks lite the function GetVotes is auto-generated by the protobufs. So this may better be a test on the Tendermint end?

[robert-zaremba]

Do you know if this is tested with a unit test somewhere?

Maybe it would be good to add a test?

Looks lite the function GetVotes is auto-generated by the protobufs. So this may better be a test on the Tendermint end?

OK, could you check on Tendermint sit and create issue there if needed?

[orijbot]

Benchmark finished. See the result: https://github.orijtech.com/benchmark/result?id=ce9041e3f14e43ebaf8890b487970de4