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

MSgCreateValidator and MsgUpdateValidator race condition #219

Closed
RiccardoM opened this issue Oct 6, 2021 · 0 comments · Fixed by #225
Closed

MSgCreateValidator and MsgUpdateValidator race condition #219

RiccardoM opened this issue Oct 6, 2021 · 0 comments · Fixed by #225
Assignees
Labels
Cat: base Related to the basics of a chain (blocks, transactions, etc) Cat: x/staking Related to the staking module Status: WIP This is currently being worked on Type: Bug Something isn't working

Comments

@RiccardoM
Copy link
Contributor

Bug description

Currently there is a possible bug while handling MsgCreateValidator and MsgUpdateValidator transactions that is due to a race condition. Suppose that a validator performs the following actions:

  1. At height X, it sends a MsgCreateValidator transaction with commission of 10%
  2. At height X+10, it sends a MsgUpdateValidator transaction updating other details but not the commission.

Suppose also that the MsgUpdateValidator is parsed before the MsgCreateValidator transaction. What would happen is the following:

  1. BDJuno parses the MsgUpdateValidator and, since the commission is not set, it stores a commission of 0%
  2. BDJuno parses the MsgCreateValidator with the correct commission, but since the latest commission as a higher height, it is never updated and the correct commission (which should be 10%) is never stored.

Expected behavior

The MsgCreateValidator should store the correct infos, and the MsgUpdateValidator should not update the wrong fields.

Possible solution

We might investigate whether is worth using some custom values to identify these cases. For example setting a commission of -1 for those validators who have not their commission set yet. Then the MsgCreateValidator will update those values accordingly.

@RiccardoM RiccardoM added Type: Bug Something isn't working Cat: x/staking Related to the staking module labels Oct 6, 2021
@RiccardoM RiccardoM self-assigned this Oct 6, 2021
@RiccardoM RiccardoM added the Cat: base Related to the basics of a chain (blocks, transactions, etc) label Oct 6, 2021
@RiccardoM RiccardoM added the Status: WIP This is currently being worked on label Oct 7, 2021
@RiccardoM RiccardoM linked a pull request Oct 8, 2021 that will close this issue
4 tasks
RiccardoM added a commit that referenced this issue Oct 8, 2021
<!-- < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < ☺
v                               ✰  Thanks for creating a PR! ✰    
v    Before smashing the submit button please review the checkboxes.
v    If a checkbox is n/a - please still include it but + a little note why
☺ > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >  -->

## Description
This PR fixes the `MsgCreateValidator` and `MsgUpdateValidator` race condition bug by changing how we handle those types of messages. Instead of storing the infos directly from them, we use gRPC to query the entire data for the height that those msgs were transmitted to, and then store such information. This way even when a `MsgUpdateValidator` is sent, the entire data will be fetched and stored avoiding partial data storage that caused the bug. 

Fix #219 

## Checklist
- [x] Targeted PR against correct branch.
- [x] Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
- [ ] Wrote unit tests.  
- [x] Re-reviewed `Files changed` in the Github PR explorer.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Cat: base Related to the basics of a chain (blocks, transactions, etc) Cat: x/staking Related to the staking module Status: WIP This is currently being worked on Type: Bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant