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

Proposer reward bug #9161

Closed
4 tasks
ebuchman opened this issue Apr 21, 2021 · 4 comments · Fixed by #11517 or #12876
Closed
4 tasks

Proposer reward bug #9161

ebuchman opened this issue Apr 21, 2021 · 4 comments · Fixed by #11517 or #12876
Assignees
Labels
C:x/distribution distribution module related T:Bug

Comments

@ebuchman
Copy link
Member

ebuchman commented Apr 21, 2021

Summary of Bug

Blocks in Tendermint include a LastCommit field, which contains a canonical commit for the previous block. That is, the commit for a block N is not included in block N but in block N+1.

The proposer reward is supposed to be based on the number of precommits included by the proposer in the LastCommit of their block. This is supposed to serve as an incentive for a proposer of block N to wait some time (ie. TimeoutCommit) after seeing a commit for block N-1 before proposing block N, in order to accumulate more precommits for block N-1 to better represent the active validators. However, currently, it appears proposers are being rewarded based on the precommits included by the next proposer. That is, the proposer for block N is being rewarded based on the behaviour of the proposer for block N+1. This opens a griefing attack where proposers can just include the minimum precommits necessary, thus reducing the reward that the previous proposer will get.

Important to recall that blocks are committed before they are executed, but that the canonical commit for a block N is only determined by block N+1 in the form of the block.LastCommit. Thus proposer for block N is supposed to be rewarded based on the commit for block N-1, which is included in block N as the LastCommit.

This was discussed in more detail here: #8928 (comment)

The code is https://github.com/cosmos/cosmos-sdk/blob/master/x/distribution/abci.go#L31-L32

Version

Latest

Steps to Reproduce

It would still be good to confirm this with a test, for instance have the first proposer make a block and include a commit for 100% of the voting power and then the second proposer makes a block with a commit that contains only 67% of the voting power and see how much the first proposer gets rewarded. It should be the maximum but with the current bug I believe it would end up being the minimum!


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@ebuchman ebuchman added T:Bug C:x/distribution distribution module related labels Apr 21, 2021
@github-actions github-actions bot added the stale label Jul 15, 2021
@ebuchman
Copy link
Member Author

Silly bot, still a bug

@ebuchman ebuchman reopened this Jul 22, 2021
@ebuchman ebuchman removed the stale label Jul 22, 2021
@github-actions github-actions bot added the stale label Sep 6, 2021
@ebuchman
Copy link
Member Author

Silly bot, still a bug!

@ebuchman ebuchman reopened this Oct 19, 2021
@ebuchman ebuchman removed the stale label Oct 19, 2021
@alexanderbez
Copy link
Contributor

This sounds correct to me. I can't recall why it was designed this way. I find it hard to believe we didn't consider this case.

@ebuchman
Copy link
Member Author

I think its just a genuine off by one error

@julienrbrt julienrbrt self-assigned this Mar 23, 2022
@amaury1093 amaury1093 moved this to In Progress in Cosmos SDK Maintenance Mar 24, 2022
@amaury1093 amaury1093 moved this from In Progress to In Review in Cosmos SDK Maintenance Apr 1, 2022
@mergify mergify bot closed this as completed in #11517 Apr 4, 2022
Repository owner moved this from In Review to Done in Cosmos SDK Maintenance Apr 4, 2022
mergify bot pushed a commit that referenced this issue Apr 4, 2022
## Description

Closes: #9161

I have added a test to confirm (or not) the proposer reward bug from the above issue.
It seems that there is no bug, and it behaves as it should.
The relevant logic that ensure that is here:

- https://github.com/cosmos/cosmos-sdk/blob/7fc7b3f6ff82eb5ede52881778114f6b38bd7dfa/x/distribution/keeper/allocation.go#L56
- https://github.com/cosmos/cosmos-sdk/blob/7fc7b3f6ff82eb5ede52881778114f6b38bd7dfa/x/distribution/keeper/allocation.go#L67

I am still opening this PR as adding this test can't do wrong.

---

### Author Checklist

*All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.*

I have...

- [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [x] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [x] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules)
- [x] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [ ] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

### Reviewers Checklist

*All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.*

I have...

- [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed 
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)
@julienrbrt julienrbrt reopened this Jul 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:x/distribution distribution module related T:Bug
Projects
No open projects
Archived in project
4 participants