R4R: Allow the undelegation of more coins than were delegated; More validity checks. #3717

jaekwon · 2019-02-22T20:33:30Z

WIP

…y checks

This reverts commit 2f9a0da.

codecov · 2019-02-22T22:15:52Z

Codecov Report

Merging #3717 into develop will decrease coverage by 0.11%.
The diff coverage is 47.69%.

@@             Coverage Diff             @@
##           develop    #3717      +/-   ##
===========================================
- Coverage    61.46%   61.34%   -0.12%     
===========================================
  Files          189      189              
  Lines        14057    14094      +37     
===========================================
+ Hits          8640     8646       +6     
- Misses        4876     4894      +18     
- Partials       541      554      +13

cwgoes · 2019-02-23T12:03:40Z

x/slashing/keeper.go

-		panic(fmt.Sprintf("Validator consensus-address %v not found", consAddr))
+		// ignore evidence that cannot be handled.
+		return
+		// NOTE:


We aren't talking about having the validators in storage here, we're talking about just the public keys associated with the consensus addresses. Why would Tendermint ever send the application a consensus address for which the application hadn't stored a public key?

it's in the comment below. because the app deletes the validator after the unbonding period is over (say), and the simulator or tendermint doesn't yet know exactly when this happens. so currently the simulator is breaking the app because it's not very intelligent with inserting evidence. same could happen w/ tendermint.

Indeed the app does delete the validator after the unbonding period is over (x/slashing/hooks.go:35), and the simulation could still send evidence - which I would consider a bug in the simulation, but I guess the simulation's job is to model whatever the actual Tendermint might do - does Tendermint submit evidence past the unbonding period (if max evidence age is set equal to it)?

It doesn't seem likely that programmers will get it all right, even if Tendermint were to support evidence... it just seems like a difficult thing to do, or even to prove from the perspective of a developer, even if the criterion for evidence correctness were to be defined by the app in another ABCI message (say). So let's go with defensive here, which we can undo later easily (since it's a single if statement). Later apps can upgrade to become less permissive, and that's fine once Tendermint+SDK's been upgraded to support evidence at only allowable heights.

Notifying the programmer/operator of these issues can be done via WARN logging/notification of some sort, which should be built out on the Tendermint side.

cwgoes · 2019-02-25T12:15:17Z

x/distribution/keeper/allocation.go

+		k.AllocateTokensToValidator(ctx, proposerValidator, proposerReward)
+		remaining = feesCollected.Sub(proposerReward)
+	} else {
+		// proposer can be unknown if say, the unbonding period is 1 block, so


If the unbonding period is greater than 1 block, the proposer should never be unknown, right? Should we panic in that case (we can fetch the unbonding period from the params) - or at least log a warning?

ditto on at least a log here.

docs/spec/auth/05_vesting.md

cwgoes · 2019-02-25T12:19:14Z

x/mock/simulation/rand_util.go

@@ -38,37 +37,33 @@ 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.
+// Note: The range of RandomAmount includes max, and is, in fact, biased to return max as well as 0.
 func RandomAmount(r *rand.Rand, max sdk.Int) sdk.Int {


Suggested change

func RandomAmount(r *rand.Rand, max sdk.Int) sdk.Int {

func BiasedRandomAmount(r *rand.Rand, max sdk.Int) sdk.Int {

I don't think this is necessary to differentiate... RandomAmount might as well be biased all the time intelligently...

IMHO it's not intuitive. Can the same be said for the native go library's rand functions?

cwgoes · 2019-02-25T12:19:27Z

x/mock/simulation/rand_util.go

 }

 // RandomDecAmount generates a random decimal amount
-// Note: The range of RandomDecAmount includes max, and is, in fact, biased to return max.
+// Note: The range of RandomDecAmount includes max, and is, in fact, biased to return max as well as 0.
 func RandomDecAmount(r *rand.Rand, max sdk.Dec) sdk.Dec {


Suggested change

func RandomDecAmount(r *rand.Rand, max sdk.Dec) sdk.Dec {

func BiasedRandomDecAmount(r *rand.Rand, max sdk.Dec) sdk.Dec {

cwgoes

A few minor notes and a question about whether we should panic if the proposer is unknown and the unbonding period is greater than 1 block (which seems likely to be the normal case); otherwise LGTM.

alexanderbez · 2019-02-25T14:22:41Z

x/slashing/keeper.go

-		panic(fmt.Sprintf("Validator consensus-address %v not found", consAddr))
+		// ignore evidence that cannot be handled.
+		return
+		// NOTE:


I would put all documentation above the return statement.

alexanderbez · 2019-02-25T14:50:31Z

x/distribution/keeper/allocation.go

+		k.AllocateTokensToValidator(ctx, proposerValidator, proposerReward)
+		remaining = feesCollected.Sub(proposerReward)
+	} else {
+		// proposer can be unknown if say, the unbonding period is 1 block, so


ditto on at least a log here.

PENDING.md

jackzampolin · 2019-02-27T19:04:24Z

This looks like it needs some comments addressed.

jackzampolin

LGTM 👍

cwgoes and others added 2 commits February 22, 2019 13:02

Add altered random number generation

da12165

WIP Allow undelegation of more coins than delegated; Add more validit…

b7d1d69

…y checks

jaekwon requested review from cwgoes, ebuchman and rigelrozanski as code owners February 22, 2019 20:33

Fix tests

2f9a0da

jaekwon changed the base branch from develop to cwgoes/debug-vesting-finding February 22, 2019 21:02

jaekwon added 4 commits February 22, 2019 13:10

Revert "Fix tests"

c938039

This reverts commit 2f9a0da.

fix tests

066e11f

Add comment on new behavior

f7c3cc6

Call AddCoins conditionally

637902d

jaekwon added 2 commits February 22, 2019 17:45

Do not panic with unknown evidence

71774b8

update comments from bez's branch

d9b5071

cwgoes reviewed Feb 23, 2019

View reviewed changes

jaekwon added 2 commits February 23, 2019 16:23

fixing more sim issues

0c0b142

Ignore unknown proposer

aca59ac

jaekwon changed the title ~~WIP: Allow the undelegation of more coins than were delegated; More validity checks.~~ R4R: Allow the undelegation of more coins than were delegated; More validity checks. Feb 25, 2019

Add bias to return 0 to RandomAmount

002e49f

jaekwon mentioned this pull request Feb 25, 2019

WIP: Altered RNG / vesting debugging #3712

Closed

5 tasks

cwgoes reviewed Feb 25, 2019

View reviewed changes

docs/spec/auth/05_vesting.md Outdated Show resolved Hide resolved

cwgoes reviewed Feb 25, 2019

View reviewed changes

cwgoes mentioned this pull request Feb 25, 2019

Cap Undelegation for Vesting Accounts #3718

Closed

5 tasks

alexanderbez reviewed Feb 25, 2019

View reviewed changes

alexanderbez suggested changes Feb 25, 2019

View reviewed changes

jaekwon changed the base branch from cwgoes/debug-vesting-finding to develop February 25, 2019 16:56

cwgoes added this to the v0.33.0 (Launch) milestone Feb 25, 2019

cwgoes mentioned this pull request Feb 25, 2019

DNM: launch rc jae #3736

Closed

5 tasks

rigelrozanski mentioned this pull request Feb 25, 2019

Insufficient Outstanding Rewards Simulation Edge Case #3735

Closed

Address review comments

c80e4ae

jackzampolin approved these changes Feb 27, 2019

View reviewed changes

docs updates

2925cc8

alexanderbez approved these changes Feb 27, 2019

View reviewed changes

Merge branch 'develop' into jae/undelegate_more

46e9d4b

cwgoes merged commit 10bd98e into develop Feb 27, 2019

cwgoes deleted the jae/undelegate_more branch February 27, 2019 21:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

R4R: Allow the undelegation of more coins than were delegated; More validity checks. #3717

R4R: Allow the undelegation of more coins than were delegated; More validity checks. #3717

jaekwon commented Feb 22, 2019

codecov bot commented Feb 22, 2019 •

edited

Loading

cwgoes Feb 23, 2019

jaekwon Feb 23, 2019

cwgoes Feb 23, 2019

jaekwon Feb 23, 2019

cwgoes Feb 25, 2019

alexanderbez Feb 25, 2019

cwgoes Feb 25, 2019

jaekwon Feb 27, 2019

alexanderbez Feb 27, 2019

cwgoes Feb 25, 2019

cwgoes left a comment

alexanderbez Feb 25, 2019

alexanderbez Feb 25, 2019

jackzampolin commented Feb 27, 2019

jackzampolin left a comment

	func RandomAmount(r *rand.Rand, max sdk.Int) sdk.Int {
	func BiasedRandomAmount(r *rand.Rand, max sdk.Int) sdk.Int {

	func RandomDecAmount(r *rand.Rand, max sdk.Dec) sdk.Dec {
	func BiasedRandomDecAmount(r *rand.Rand, max sdk.Dec) sdk.Dec {

R4R: Allow the undelegation of more coins than were delegated; More validity checks. #3717

R4R: Allow the undelegation of more coins than were delegated; More validity checks. #3717

Conversation

jaekwon commented Feb 22, 2019

codecov bot commented Feb 22, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwgoes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackzampolin commented Feb 27, 2019

jackzampolin left a comment

Choose a reason for hiding this comment

codecov bot commented Feb 22, 2019 •

edited

Loading