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 use address instead of bond height / intratxcounter for deduplication #3055

Merged
merged 4 commits into from
Dec 10, 2018

Conversation

jaekwon
Copy link
Contributor

@jaekwon jaekwon commented Dec 10, 2018

See: #2909 (comment)
Addresses #3049
Related to #3051 (better alternative)

This includes the (inverted) address of the validator in the power key.
This also completely removes the IntraTxCounter, simplifying the code.

Before, some of the tests were testing the bug condition, e.g. the tests were previously broken. This fixes everything AFAIK.

I'm confident enough about this PR that I believe strongly that we should merge this in for GoS.

@jaekwon jaekwon changed the title WIP use address instead of bond height / intratxcounter for deduplication R4R use address instead of bond height / intratxcounter for deduplication Dec 10, 2018
require.True(t, got.IsOK())
validatorUpdates, _ = stake.EndBlocker(ctx, sk)
require.Equal(t, 2, len(validatorUpdates))
validator, _ = sk.GetValidator(ctx, addr)
require.Equal(t, sdk.Bonded, validator.Status)
newAmt = int64(102)
newAmt = int64(103)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only reason why it worked before with 102, after getting slashed down to 100.98, which is less than the alternative validator, is because of the bug where power key indices are getting trampled.

require.Equal(t, 0, len(keeper.ApplyAndReturnValidatorSetUpdates(ctx)))
validators[0] = TestingUpdateValidator(keeper, ctx, validators[0], false)
validators[1] = TestingUpdateValidator(keeper, ctx, validators[1], false)
require.Equal(t, 2, len(keeper.ApplyAndReturnValidatorSetUpdates(ctx)))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Notice that by passing in false, the next line is more useful.

validators[0] = TestingUpdateValidator(keeper, ctx, validators[0])
validators[1] = TestingUpdateValidator(keeper, ctx, validators[1])
validators[0] = TestingUpdateValidator(keeper, ctx, validators[0], false)
validators[1] = TestingUpdateValidator(keeper, ctx, validators[1], false)

updates := keeper.ApplyAndReturnValidatorSetUpdates(ctx)
require.Equal(t, 2, len(updates))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Notice that before, ApplyAndReturnValidatorSetUpdates() shouldn't have returned anything at all. That it was returning 2 and the update orders were inverted is super weird. This addresses both issues.

keeper.SetValidator(ctx, validator)
{ // Remove any existing power key for validator.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before, none of the old power key entries were being removed. This properly cleans any old entries and also asserts a sanity check if there are more than 1 duplicate.

@codecov
Copy link

codecov bot commented Dec 10, 2018

Codecov Report

Merging #3055 into develop will decrease coverage by <.01%.
The diff coverage is 69%.

@@             Coverage Diff             @@
##           develop    #3055      +/-   ##
===========================================
- Coverage    52.19%   52.18%   -0.01%     
===========================================
  Files          136      136              
  Lines         9605     9613       +8     
===========================================
+ Hits          5013     5017       +4     
  Misses        4261     4261              
- Partials       331      335       +4

@jaekwon jaekwon merged commit 133134c into develop Dec 10, 2018
@jaekwon
Copy link
Contributor Author

jaekwon commented Dec 10, 2018

Merging this in early for sake of release. Lets still discuss here though.

@alexanderbez
Copy link
Contributor

alexanderbez commented Dec 10, 2018

@jaekwon a good chunk of these quick/forceful merges fail linting just FYI. Might want to run that prior to merging.

validator, found := keeper.GetValidator(ctx, validator.OperatorAddr)
if !found {
panic("validator expected but not found")
if apply {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could be a bit cleaner always creating ctx.CacheContext() and calling write() if apply is true

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.

Code changes look OK. Address grinding incentives if validators have the same stake / bond height, but in most real networks that seems very unlikely to be a problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants