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

R4R: Assign validator initial bonded height #2910

Closed
wants to merge 1 commit into from
Closed

R4R: Assign validator initial bonded height #2910

wants to merge 1 commit into from

Conversation

HaoyangLiu
Copy link

Closes: #2909

@alexanderbez alexanderbez changed the title Assign validator initial bonded height R4R: Assign validator initial bonded height Nov 27, 2018
@@ -104,6 +104,7 @@ func handleMsgCreateValidator(ctx sdk.Context, msg types.MsgCreateValidator, k k
}

validator := NewValidator(msg.ValidatorAddr, msg.PubKey, msg.Description)
validator.BondHeight = ctx.BlockHeight()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the best place for the change given that a validator may still not enter the bonded set here? Just thinking about semantics...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't make sense - bond height is supposed to be set when the validator is bonded, not when it is created. This code is erroneous if you are creating a validator with "tiny" stake for chain where the validator group is already full

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah. I think we can close this PR and another one be opened that addresses the deeper problem.

Copy link
Contributor

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the bug report. I don't think this addresses the root of the problem.

See #2909 (comment) and let's discuss on the issue first.

@rigelrozanski
Copy link
Contributor

rigelrozanski commented Nov 27, 2018

This PR doesn't address the provided issue sufficiently and is also missing a bunch of requirements from the Contributing Guidelines... Let's come to consensus in the issue before opening PRs in the future.

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.

4 participants