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

WIP: Altered RNG / vesting debugging #3712

Closed
wants to merge 4 commits into from

Conversation

cwgoes
Copy link
Contributor

@cwgoes cwgoes commented Feb 22, 2019

cc @rigelrozanski

Reproduce with:

go test ./cmd/gaia/app -run TestFullGaiaSimulation -SimulationEnabled=true -SimulationNumBlocks=400 -SimulationVerbose=true -SimulationCommit=true -SimulationSeed=20 -v -timeout 24h
  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
  • Wrote tests
  • Updated relevant documentation (docs/)
  • Added entries in PENDING.md with issue #
  • rereviewed Files changed in the github PR explorer

For Admin Use:

  • Added appropriate labels to PR (ex. wip, ready-for-review, docs)
  • Reviewers Assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@codecov
Copy link

codecov bot commented Feb 22, 2019

Codecov Report

Merging #3712 into develop will decrease coverage by 0.01%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #3712      +/-   ##
===========================================
- Coverage    61.18%   61.17%   -0.02%     
===========================================
  Files          190      190              
  Lines        14008    14008              
===========================================
- Hits          8571     8569       -2     
- Misses        4901     4903       +2     
  Partials       536      536

@cwgoes
Copy link
Contributor Author

cwgoes commented Feb 22, 2019

... if you undelegate fractional shares, tokens are truncated, so the exchange rate can exceed 1.

(x/stake/types/validator.go:385)

@cwgoes
Copy link
Contributor Author

cwgoes commented Feb 22, 2019

Why do we allow fractional share withdrawals? Not obvious. edit: see #3712 (comment)

Would preventing them fix this? Could there still be a case when a validator is slashed and withdrawal-truncations result in a > 1 exchange rate?

@cwgoes
Copy link
Contributor Author

cwgoes commented Feb 22, 2019

For the exchange rate to be >1:

R = T / S ~ rate = tokens / shares

1 < (T - floor(s*T/S)) / (S - s) ~ inequality (rate greater than one) after undelegation

S - s < T - floor(s * T / S)

S - T < s - floor(s * T / S)

@rigelrozanski
Copy link
Contributor

@cwgoes we need fractional share withdrawals if you're to be allowed to fully withdraw your shares from a validator. Let's say I delegate 100 atoms to a validator with a non-integer exchange rate and I receive 123.457 shares in return. If I am only able to withdraw 123 shares then I can only ever get back 99 atoms

@cwgoes
Copy link
Contributor Author

cwgoes commented Feb 22, 2019

@cwgoes we need fractional share withdrawals if you're to be allowed to fully withdraw your shares from a validator. Let's say I delegate 100 atoms to a validator with a non-integer exchange rate and I receive 123.457 shares in return. If I am only able to withdraw 123 shares then I can only ever get back 99 atoms

I see, that makes sense.

@cwgoes
Copy link
Contributor Author

cwgoes commented Feb 22, 2019

Let's donate to the community pool (credit to @alexanderbez / @rigelrozanski).

func RandomDecAmount(r *rand.Rand, max sdk.Dec) sdk.Dec {
randInt := big.NewInt(0).Rand(r, max.Int)
return sdk.NewDecFromBigIntWithPrec(randInt, sdk.Precision)
// randInt := big.NewInt(0).Rand(r, max.Int)
Copy link
Contributor

Choose a reason for hiding this comment

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

Make this call RandomAmount so that we can tweak that one function...
in the future we could make RandomAmount's probability to include bias for 0 and small numbers as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

i.e. keeps the code dry in a good way.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we actually want to keep said changes as it's biased?

}
result := sdk.NewIntFromBigInt(randInt)
// Sanity
if result.GT(max) {
Copy link
Contributor

Choose a reason for hiding this comment

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

result should be exclusive of max, so this is incorrect... will add pseudocode 1 min...

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that everywhere RandomAmount and RandomDecAmount are used, it makes sense to include the upper bound. (Though, I agree that leaving out the upper bound is more common and intuitive.)

@@ -37,14 +38,37 @@ func RandStringOfLength(r *rand.Rand, n int) string {
}

// Generate a random amount
// Note: The range of RandomAmount includes max, and is, in fact, biased to return max.
Copy link
Contributor

Choose a reason for hiding this comment

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

it shouldn't include max, it should be exclusive.

func RandomAmount(r *rand.Rand, max sdk.Int) sdk.Int {
return sdk.NewInt(int64(r.Intn(int(max.Int64()))))
// return sdk.NewInt(int64(r.Intn(int(max.Int64()))))
Copy link
Contributor

Choose a reason for hiding this comment

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

var randInt = big.NewInt(0)
switch r.Intn(3) {
case 0:
	// randInt = big.NewInt(0)
case 1:
	randInt = max.BigInt() - 1 // max - 1 
case 2:
	randInt = big.NewInt(0).Rand(r, max) // up to max - 1
}
return randInt

Copy link
Contributor

Choose a reason for hiding this comment

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

This makes the various cases laid out better, is easier to extend and tweak.

@jaekwon
Copy link
Contributor

jaekwon commented Feb 25, 2019

everything incorporated here: #3717

@cwgoes
Copy link
Contributor Author

cwgoes commented Feb 25, 2019

Closing in favor of #3717.

@cwgoes cwgoes closed this Feb 25, 2019
@cwgoes cwgoes deleted the cwgoes/debug-vesting-finding branch February 25, 2019 18:21
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.

5 participants