R4R: Genesis Vesting Accounts & Simulation #3308

alexanderbez · 2019-01-15T19:40:53Z

Fix GetVestedCoins bug where block time is past vesting end time
Update time fields on vesting account(s) to int64 from time.Time
Update constructors to directly take a *BaseAccount as this will allow vesting accounts to have the account number/sequences inherited
Update genesis logic to support vesting accounts
Add vesting account support to simulation

closes: #3305

Targeted PR against correct branch (see CONTRIBUTING.md)
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 · 2019-01-15T20:17:11Z

Codecov Report

Merging #3308 into develop will decrease coverage by 0.08%.
The diff coverage is 61.9%.

@@             Coverage Diff             @@
##           develop    #3308      +/-   ##
===========================================
- Coverage    55.53%   55.44%   -0.09%     
===========================================
  Files          134      134              
  Lines         9794     9802       +8     
===========================================
- Hits          5439     5435       -4     
- Misses        4012     4024      +12     
  Partials       343      343

alexanderbez · 2019-01-17T00:21:17Z

x/bank/simulation/msgs.go

@@ -64,7 +64,7 @@ func createSingleInputSendMsg(r *rand.Rand, ctx sdk.Context, accs []simulation.A
 		toAcc = simulation.RandomAcc(r, accs)
 	}
 	toAddr := toAcc.Address
-	initFromCoins := mapper.GetAccount(ctx, fromAcc.Address).GetCoins()
+	initFromCoins := mapper.GetAccount(ctx, fromAcc.Address).SpendableCoins(ctx.BlockHeader().Time)


Note we need to make sure we only send "sendable" amount of coins.

Ah - but sometimes (randomly) I think we should try to spend more than we can!

If we try to send any more than "sendable", then subtractCoins will panic. Did you mean something else?

If the transaction tries to spend more, it should fail - right?

yes, it'll fail and panic at subtractCoins.

alexanderbez · 2019-01-17T00:22:05Z

cmd/gaia/app/sim_test.go

+			// TODO: Do the vesting periods need to be shrunk to account for short
+			// simulations?
+			startTime := genesisTimestamp.Unix()
+			endTime := r.Int63n((startTime+(60*60*24*2))-startTime) + startTime


@cwgoes is the end time generated here reasonable? Or should it be much shorter...in the manner of seconds?

I think this is OK - a few-hundred block sim will run several days in virtual time.

Randomize it some more though - some vesting accounts should vest very quickly; others very slowly.

Updated -- lmk your thoughts.

cwgoes

On the right track, several comments. In the simulation we should randomize parameters almost always.

cmd/gaia/app/genesis.go

cwgoes · 2019-01-17T13:12:58Z

cmd/gaia/app/sim_test.go

+		// create a vesting account under the following conditions:
+		// - not an initially bonded (genesis) validator
+		// - every 10th account past the initial bonded validator set
+		if int64(i) > numInitiallyBonded && i+1%10 == 0 {


Can we use a random condition instead (each account has some chance of being vesting)?

Also there's no reason the initially bonded validators can't have vesting accounts.

Can we use a random condition instead (each account has some chance of being vesting)?

Sure, how do you suggest we do so?

Also there's no reason the initially bonded validators can't have vesting accounts.

True, but this caused me headache because the vesting accounts won't have DelegatedVesting populated and thus future delegations/undelegations will fail. I think if we want to support this, we have to add such fields to GenesisAccount which is a bit ugly imho. Perhaps we should be okay with vesting accnts never being in the initial bonded set.

cwgoes · 2019-01-17T13:14:03Z

cmd/gaia/app/sim_test.go

+			// TODO: Do the vesting periods need to be shrunk to account for short
+			// simulations?
+			startTime := genesisTimestamp.Unix()
+			endTime := r.Int63n((startTime+(60*60*24*2))-startTime) + startTime


I think this is OK - a few-hundred block sim will run several days in virtual time.

Randomize it some more though - some vesting accounts should vest very quickly; others very slowly.

cwgoes · 2019-01-17T13:14:20Z

cmd/gaia/app/sim_test.go

+			startTime := genesisTimestamp.Unix()
+			endTime := r.Int63n((startTime+(60*60*24*2))-startTime) + startTime
+
+			if r.Int()%2 == 0 {


See above comment (would prefer this to be a random condition).

docs/spec/auth/vesting.md

cwgoes · 2019-01-17T13:15:40Z

x/bank/simulation/msgs.go

@@ -64,7 +64,7 @@ func createSingleInputSendMsg(r *rand.Rand, ctx sdk.Context, accs []simulation.A
 		toAcc = simulation.RandomAcc(r, accs)
 	}
 	toAddr := toAcc.Address
-	initFromCoins := mapper.GetAccount(ctx, fromAcc.Address).GetCoins()
+	initFromCoins := mapper.GetAccount(ctx, fromAcc.Address).SpendableCoins(ctx.BlockHeader().Time)


Ah - but sometimes (randomly) I think we should try to spend more than we can!

jackzampolin

Looks like @cwgoes concerns were handled. Tested locally and looking good for me.

Aleksandr Bezobchuk added 2 commits January 15, 2019 14:35

Update vesting spec and impl time fields and constructors

22880b4

Update genesis to support vesting accounts

99dd115

alexanderbez added wip C:Simulations labels Jan 15, 2019

alexanderbez requested review from alessio, jackzampolin, rigelrozanski and cwgoes January 15, 2019 19:40

alexanderbez requested review from ebuchman and zramsay as code owners January 15, 2019 19:40

Aleksandr Bezobchuk added 3 commits January 15, 2019 14:43

More spec and godoc updates

eac2e42

Update genesis section in vesting spec

5a0d8c6

Fix bank unit tests

3c10928

Aleksandr Bezobchuk added 5 commits January 15, 2019 15:58

Add test cases to ToAccount in genesis

b3600ea

Update RegisterCodec to include vesting interface and types

f3f2ce5

Fix GetVestedCoins bug where block time is past vesting end time

22dc217

Add vesting accounts to simulation

71534af

Merge branch 'develop' into bez/vesting-acc-genesis-support

c14b0f7

cwgoes mentioned this pull request Jan 16, 2019

"Red Wedding" (first GoS hard fork) master to-do list #3079

Closed

7 tasks

alexanderbez changed the title ~~WIP: Genesis Vesting Accounts & Simulation~~ WIP: Genesis Vesting Accounts Jan 16, 2019

Aleksandr Bezobchuk added 3 commits January 16, 2019 15:23

Undo genesis changes

df24603

Add pending log entry

f29d74f

Merge branch 'develop' into bez/vesting-acc-genesis-support

ec3b99b

alexanderbez changed the title ~~WIP: Genesis Vesting Accounts~~ WIP: Genesis Vesting Accounts & Simulation Jan 16, 2019

fix simulation to support vesting accounts

ed5bbc4

alexanderbez commented Jan 17, 2019

View reviewed changes

Fix 10th account check

7fc2544

alexanderbez changed the title ~~WIP: Genesis Vesting Accounts & Simulation~~ R4R: Genesis Vesting Accounts & Simulation Jan 17, 2019

alexanderbez added ready-for-review and removed wip labels Jan 17, 2019

cwgoes suggested changes Jan 17, 2019

View reviewed changes

Aleksandr Bezobchuk added 2 commits January 17, 2019 10:00

Update vesting genesis logic to panic on invalid accounts

3e12b6b

Change randomness of vesting accounts in simulation

5692aa2

jackzampolin approved these changes Jan 17, 2019

View reviewed changes

cwgoes approved these changes Jan 17, 2019

View reviewed changes

cwgoes merged commit f2e87ad into develop Jan 17, 2019

cwgoes deleted the bez/vesting-acc-genesis-support branch January 17, 2019 16:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

R4R: Genesis Vesting Accounts & Simulation #3308

R4R: Genesis Vesting Accounts & Simulation #3308

alexanderbez commented Jan 15, 2019 •

edited

Loading

codecov bot commented Jan 15, 2019 •

edited

Loading

alexanderbez Jan 17, 2019

cwgoes Jan 17, 2019

alexanderbez Jan 17, 2019 •

edited

Loading

cwgoes Jan 17, 2019

alexanderbez Jan 17, 2019

alexanderbez Jan 17, 2019

cwgoes Jan 17, 2019

alexanderbez Jan 17, 2019

cwgoes left a comment •

edited

Loading

cwgoes Jan 17, 2019

cwgoes Jan 17, 2019

alexanderbez Jan 17, 2019 •

edited

Loading

alexanderbez Jan 17, 2019

cwgoes Jan 17, 2019

cwgoes Jan 17, 2019

alexanderbez Jan 17, 2019

cwgoes Jan 17, 2019

jackzampolin left a comment

R4R: Genesis Vesting Accounts & Simulation #3308

R4R: Genesis Vesting Accounts & Simulation #3308

Conversation

alexanderbez commented Jan 15, 2019 • edited Loading

codecov bot commented Jan 15, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderbez Jan 17, 2019 • edited Loading

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 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderbez Jan 17, 2019 • edited Loading

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

jackzampolin left a comment

Choose a reason for hiding this comment

alexanderbez commented Jan 15, 2019 •

edited

Loading

codecov bot commented Jan 15, 2019 •

edited

Loading

alexanderbez Jan 17, 2019 •

edited

Loading

cwgoes left a comment •

edited

Loading

alexanderbez Jan 17, 2019 •

edited

Loading