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

abci: remove Evidence#TotalVotingPower #4581

Closed
melekes opened this issue Mar 18, 2020 · 5 comments
Closed

abci: remove Evidence#TotalVotingPower #4581

melekes opened this issue Mar 18, 2020 · 5 comments
Labels
C:evidence Component: Evidence T:breaking Type: Breaking Change T:bug Type Bug (Confirmed)

Comments

@melekes
Copy link
Contributor

melekes commented Mar 18, 2020

Power: ev.TotalVotingPower - 1,

by not using VotingPower of byzantine validator (and using total voting power of entire validator set), we're essentially increasing byzantine validator voting power => promoting it (instead of punishing).

@melekes melekes added T:bug Type Bug (Confirmed) C:evidence Component: Evidence T:breaking Type: Breaking Change and removed T:breaking Type: Breaking Change C:evidence Component: Evidence labels Mar 18, 2020
@melekes
Copy link
Contributor Author

melekes commented Mar 18, 2020

Why do we need total_voting_power anyway?

@melekes melekes changed the title abci: remove Evidence#TotalVotingPower, add Evidence#ValidatorVotingPower abci: remove Evidence#TotalVotingPower Mar 19, 2020
@melekes
Copy link
Contributor Author

melekes commented Apr 13, 2020

cc @ebuchman

@melekes melekes added the T:breaking Type: Breaking Change label May 11, 2020
@ebuchman
Copy link
Contributor

ebuchman commented Jun 5, 2020

Very good question. I can't seem to find a good answer. I suspect it had something to do with letting the app know what percentage of the voting power was byzantine in case they weren't maintaining their own historical validator set or something, but I'm not totally sure and unfortunately I can't find a reference. From what I can tell Cosmos-SDK is not using it, but probably because it keeps historical validators ... not sure if other apps are.

Here's the PR on the abci repo where it was introduced: tendermint/abci#245 but I can't find any supporting justification for why.

It could be related to issues like cosmos/cosmos-sdk#1348 ? Maybe @cwgoes knows/remembers.

Without it, you'd have to pull up the old validator set to know how much to slash, since the offending validator might be gone by now. But I can't really tell if this is actually solving for something since I'd expect any app doing slashing to maintain historical validator sets probably anyways.

Would be good to find out if anyone else is using this field ...

@cwgoes
Copy link
Contributor

cwgoes commented Jun 6, 2020

The SDK cannot currently calculate past voting power of individual validators or past total voting power, so if we were to implement cosmos/gaia#30 (moved from cosmos/cosmos-sdk#1348) we would need this. This is in the category of proof-of-stake issues that I think would be nice to implement if we want a rigorous formal model with exact penalties, but probably don't make too much difference in most practical use-cases.

I do not know if any non-Cosmos-SDK users are using this field.

Without it, you'd have to pull up the old validator set to know how much to slash, since the offending validator might be gone by now. But I can't really tell if this is actually solving for something since I'd expect any app doing slashing to maintain historical validator sets probably anyways.

I'm not sure about this - it's fairly expensive to keep all of this information in state (twice, if Tendermint is already storing it), at least for individual validators, although I suppose it wouldn't be so bad to just store the total voting power for each of the past blocks within the unbonding period. Recalculating it is possible in principle but problematic because it is likely to require iteration over user-controlled fields (e.g. delegations), which opens up DoS vectors.

Arguably, unless Tendermint needs this information, it should be stored by the application instead though, and if the application doesn't need it perhaps it shouldn't be stored at all.

@melekes
Copy link
Contributor Author

melekes commented Jul 3, 2020

The SDK cannot currently calculate past voting power of individual validators or past total voting power, so if we were to implement cosmos/gaia#30 (moved from cosmos/cosmos-sdk#1348) we would need this.

Sounds like we should leave it then. @ebuchman and @cwgoes thanks for your input 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:evidence Component: Evidence T:breaking Type: Breaking Change T:bug Type Bug (Confirmed)
Projects
None yet
Development

No branches or pull requests

3 participants