R4R: blockly minting #2825

rigelrozanski · 2018-11-15T03:52:03Z

closes #2763

REF #2846

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 · 2018-11-16T18:03:54Z

Codecov Report

Merging #2825 into develop will increase coverage by 0.04%.
The diff coverage is 45.23%.

@@             Coverage Diff             @@
##           develop    #2825      +/-   ##
===========================================
+ Coverage    56.84%   56.89%   +0.04%     
===========================================
  Files          120      120              
  Lines         8298     8321      +23     
===========================================
+ Hits          4717     4734      +17     
- Misses        3263     3268       +5     
- Partials       318      319       +1

x/mint/params.go

jackzampolin · 2018-11-16T22:18:06Z

Tests run well locally for me, but I'm prob not the best one to review the core logic here. Looks like the implementation follows whats in #2763 however. LGTM

cwgoes

General structure LGTM, a few nits / questions.

docs/spec/mint/begin_block.md

docs/spec/mint/state.md

x/mint/abci_app.go

x/mint/minter.go

x/mint/minter_test.go

x/mint/params.go

cmd/gaia/app/sim_test.go

docs/spec/mint/begin_block.md

docs/spec/mint/state.md

x/mint/abci_app.go

alexanderbez

Left some initial minor feedback, otherwise great work @rigelrozanski ⚡️

x/mint/minter.go

x/mint/minter_test.go

x/mint/params.go

alexanderbez · 2018-11-20T00:56:55Z

Looks like we need to come to consensus on when to do decimal truncation and address the broken CI

…-sdk into rigel/blockly-mint

x/distribution/types/delegator_info.go

x/stake/keeper/delegation.go

cwgoes

Tested ACK

A few more minor comments which might be worth addressing.

x/distribution/keeper/validator.go

cwgoes · 2018-11-23T14:00:31Z

x/distribution/simulation/invariants.go

@@ -48,3 +52,81 @@ func ValAccumInvariants(k distr.Keeper, sk distr.StakeKeeper) simulation.Invaria
 		return nil
 	}
 }
+
+// DelAccumInvariants checks that each validator del accum == sum all delegators' accum
+func DelAccumInvariants(k distr.Keeper, sk distr.StakeKeeper) simulation.Invariant {


This is a great invariant but it is very slow, I wonder if we should make it periodic by default.

I think we should leave it at every block until we upgrade for more capable feature set in the simulator - From a debugging perspective it's nice to not have to be an expert and just "know" that we should reset an invariant to be running each block when it fails to determine the location of the bug. I'm also not so worried about the speed of the simulator - this doesn't seem to be an issue as I've been running things locally.

To improve the speed let's focus on things like the ability to load the simulator from a simulated state etc.

As per running this every block on chain - yeah that seems like a performance issue - not sure how to adjust that (is this per block invariants stuff already merged? - feel free to push here to make that fix, or we could add another PR)

I think we should run this on-chain and not run it for blockchain syncing for new nodes... I think it would be nice to halt the chain as soon as we know that something is off, at least in the beginning.

💯 - we can do this very easily, just edit the runtime invariants in cmd/gaia/app/invariants.go

(right now we aren't running it on-chain @rigelrozanski, you have to add it explicitly)

I'm also not so worried about the speed of the simulator - this doesn't seem to be an issue as I've been running things locally.

OK, maybe it's just my slow laptop - but we should ensure we can still run 500/1000-block simulations as they do catch issues.

Well, depends on how fast it is... checking...

ahhhhhhs didn't realize how much slower this is

I think that we should probably add an option to just "enable" or "disable" slow invar checks for the simulation here... maybe a flag

Hmm I much prefer periodic invariants, which achieve essentially the same speed benefit while still ensuring errors are caught (and once caught, you can disable periodicity to figure out the exact operation).

x/distribution/types/delegator_info.go

x/mint/minter_test.go

x/stake/keeper/delegation.go

cwgoes · 2018-11-23T14:05:14Z

x/stake/simulation/invariants.go

+// DelegatorSharesInvariant checks whether all the delegator shares which persist
+// in the delegator object add up to the correct total delegator shares
+// amount stored in each validator
+func DelegatorSharesInvariant(k stake.Keeper) simulation.Invariant {


Also a computationally expensive invariant, maybe it should be periodic by default

x/distribution/types/dec_coin.go

x/distribution/types/delegator_info.go

x/mint/abci_app.go

x/mint/minter.go

x/mint/minter_test.go

x/stake/keeper/delegation.go

Co-Authored-By: rigelrozanski <[email protected]>

jaekwon · 2018-11-23T23:05:31Z

x/distribution/types/delegator_info.go

@@ -42,16 +50,49 @@ func (di DelegationDistInfo) WithdrawRewards(wc WithdrawContext, vi ValidatorDis
 	vi = vi.UpdateTotalDelAccum(wc.Height, totalDelShares)

 	if vi.DelAccum.Accum.IsZero() {


I don't understand why this code block is necessary. It's a slight optimization maybe, but it increases surface area for bugs. Also, if vi.DelAccum.Accum.IsZero(), di.DelPoolWithdrawHeight I don't think would matter anyways (even though it would be correct to update the height here).

hmmm - yeah maybe this is an “over-optimization” and we should just delete this whole block instead of adding this defensive check

Another note... when adding more shares to a delegation we were assuming that the withdrawal height was set previously.
so we should add a panic check when we modify the delegation shares, that the withdrawal height is set to the current height.

I’m not saying we should add the panic after it was clearly updated. there’s a timeline of dependencies here… withdrawal <-------> further-add-to-delegation-shares currently we made the bug fix where we update the withdrawal height on the left hand side. we want the very far right hand side, which is not the highest level of code… it’s a bit deeper in the call stack.

Refining my thinking...

I was trying to refer to (I think) in x/stake/keeper/distribution.go,

validator types.Validator, subtractAccount bool) (newShares sdk.Dec, err sdk.Error) { ... // Get or create the delegator delegation delegation, found := k.GetDelegation(ctx, delAddr, validator.OperatorAddr) ... // call the appropriate hook if present if found { k.OnDelegationSharesModified(ctx, delAddr, validator.OperatorAddr) } else { k.OnDelegationCreated(ctx, delAddr, validator.OperatorAddr) } ... // Update delegation delegation.Shares = delegation.Shares.Add(newShares) delegation.Height = ctx.BlockHeight() k.SetDelegation(ctx, delegation) }

Was the problem that we were calling OnDelegateSharesModified here, but because the shares was zero, we didn't update the height?

Some thoughts... OnDelegatorSharesModified here should be called WillModifyDelegatorShares maybe, otherwise it appears to be a misnomer. WillCreateDelegation as well... since the delegation hasn't been created or modified yet. Or, maybe we can keep the old info and pass it into OnDelegationSharesModified. Not a big deal ATM, just pointing out an inconsistency.

If we had an DidModifyDelegationShares, we could add the check in the distribution keeper to check that the withdrawal height was modified. But also, maybe this isn't necessary.

Agreed - ref #2538

Clip withdrawal tokens

untested core minting updates

93242a2

rigelrozanski requested review from cwgoes and ebuchman as code owners November 15, 2018 03:52

fix initialMinter

d9a53e5

jackzampolin mentioned this pull request Nov 15, 2018

Game of Stakes / gaia-9002 release checklist #2754

Closed

27 tasks

rigelrozanski added 3 commits November 15, 2018 14:14

fix existing tests

1cc058f

working writing test

32e652f

NextProvision test, some fixes

b9b979f

rigelrozanski requested a review from zramsay as a code owner November 16, 2018 18:03

rigelrozanski added 4 commits November 16, 2018 15:11

update mechanism to use average block time, PENDING, wip docs

f31a7e8

1 line compile fix

3dc8ab1

finish updating docs

5b0f3c9

compile fix

5f70992

rigelrozanski added ready-for-review C:x/mint labels Nov 16, 2018

rigelrozanski changed the title ~~WIP: hourly minting~~ R4R: hourly minting Nov 16, 2018

jackzampolin reviewed Nov 16, 2018

View reviewed changes

x/mint/params.go Show resolved Hide resolved

rigelrozanski changed the title ~~R4R: hourly minting~~ R4R: blockly minting Nov 17, 2018

cwgoes suggested changes Nov 19, 2018

View reviewed changes

alexanderbez reviewed Nov 19, 2018

View reviewed changes

cmd/gaia/app/sim_test.go Show resolved Hide resolved

docs/spec/mint/begin_block.md Show resolved Hide resolved

docs/spec/mint/state.md Outdated Show resolved Hide resolved

docs/spec/mint/state.md Outdated Show resolved Hide resolved

x/mint/abci_app.go Show resolved Hide resolved

alexanderbez reviewed Nov 19, 2018

View reviewed changes

x/mint/minter.go Outdated Show resolved Hide resolved

x/mint/minter_test.go Outdated Show resolved Hide resolved

x/mint/params.go Outdated Show resolved Hide resolved

rigelrozanski added 2 commits November 19, 2018 16:08

address @cwgoes comments

01a9246

uint fixes

bb8c8ed

rigelrozanski added 3 commits November 20, 2018 01:30

benchmark using Int

fa469c2

use decimal

e1af6c6

Merge remote-tracking branch 'origin/develop' into rigel/blockly-mint

58d34b7

rigelrozanski added 5 commits November 21, 2018 20:23

PENDING.md

d41eeb1

Merge branch 'develop' into rigel/blockly-mint

b678bcc

test cover fix

fffc100

Merge branch 'rigel/blockly-mint' of https://github.com/cosmos/cosmos…

0da12f8

…-sdk into rigel/blockly-mint

...

ccfbfcf

rigelrozanski mentioned this pull request Nov 22, 2018

Debug import/export sim distribution leftover coins issue #2872

Closed

cwgoes mentioned this pull request Nov 22, 2018

R4R: Add a flag to export for zero-height start #2827

Merged

5 tasks

rigelrozanski commented Nov 22, 2018

View reviewed changes

x/distribution/types/delegator_info.go Outdated Show resolved Hide resolved

rigelrozanski commented Nov 22, 2018

View reviewed changes

x/stake/keeper/delegation.go Show resolved Hide resolved

rigelrozanski added 3 commits November 22, 2018 13:28

demonstrate defensive accum height for zero-delegations

b466775

add back in the zero-delegation invar

05ddb7b

note on defensive code

607eaca

cwgoes approved these changes Nov 23, 2018

View reviewed changes

cwgoes requested a review from sunnya97 November 23, 2018 14:18

alexanderbez reviewed Nov 23, 2018

View reviewed changes

alexanderbez and others added 2 commits November 23, 2018 14:36

Update x/distribution/types/delegator_info.go

d2f59fc

Co-Authored-By: rigelrozanski <[email protected]>

Update x/distribution/types/delegator_info.go

c0239e0

Co-Authored-By: rigelrozanski <[email protected]>

jaekwon reviewed Nov 23, 2018

View reviewed changes

jaekwon and others added 8 commits November 23, 2018 19:28

Update Decimal Format()

4959289

Clip withdrawal tokens

9a66906

oops

0088c22

bez comments, lint

50e119a

Merge pull request #2895 from cosmos/jae/blockly-mint

5ddd549

Clip withdrawal tokens

Merge remote-tracking branch 'origin/develop' into rigel/blockly-mint

807a4fd

lint, bez comments

4605acb

Fix typo in errors.go

94717fa

jaekwon merged commit 7cb1ba6 into develop Nov 26, 2018

cwgoes deleted the rigel/blockly-mint branch November 26, 2018 12:28

ValarDragon mentioned this pull request Nov 29, 2018

Decimal Multiplication/Quo Bug #2888

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

R4R: blockly minting #2825

R4R: blockly minting #2825

rigelrozanski commented Nov 15, 2018 •

edited

Loading

codecov bot commented Nov 16, 2018 •

edited

Loading

jackzampolin commented Nov 16, 2018

cwgoes left a comment

alexanderbez left a comment

alexanderbez commented Nov 20, 2018

cwgoes left a comment

cwgoes Nov 23, 2018

rigelrozanski Nov 23, 2018

jaekwon Nov 23, 2018

cwgoes Nov 24, 2018

cwgoes Nov 24, 2018

cwgoes Nov 24, 2018

jaekwon Nov 24, 2018

rigelrozanski Nov 24, 2018

cwgoes Nov 26, 2018

cwgoes Nov 23, 2018

jaekwon Nov 23, 2018

rigelrozanski Nov 24, 2018

jaekwon Nov 24, 2018

rigelrozanski Nov 25, 2018

jaekwon Nov 25, 2018

cwgoes Nov 26, 2018

		@@ -42,16 +50,49 @@ func (di DelegationDistInfo) WithdrawRewards(wc WithdrawContext, vi ValidatorDis
		vi = vi.UpdateTotalDelAccum(wc.Height, totalDelShares)

		if vi.DelAccum.Accum.IsZero() {

R4R: blockly minting #2825

R4R: blockly minting #2825

Conversation

rigelrozanski commented Nov 15, 2018 • edited Loading

codecov bot commented Nov 16, 2018 • edited Loading

Codecov Report

jackzampolin commented Nov 16, 2018

cwgoes left a comment

Choose a reason for hiding this comment

alexanderbez left a comment

Choose a reason for hiding this comment

alexanderbez commented Nov 20, 2018

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

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

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

rigelrozanski commented Nov 15, 2018 •

edited

Loading

codecov bot commented Nov 16, 2018 •

edited

Loading