R4R: Unbonding and Redelegations Queue #2405

sunnya97 · 2018-09-26T07:18:37Z

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 · 2018-09-26T07:29:14Z

Codecov Report

Merging #2405 into develop will increase coverage by 0.01%.
The diff coverage is 65.81%.

@@            Coverage Diff             @@
##           develop   #2405      +/-   ##
==========================================
+ Coverage    59.69%   59.7%   +0.01%     
==========================================
  Files          136     136              
  Lines         8405    8428      +23     
==========================================
+ Hits          5017    5032      +15     
- Misses        3055    3060       +5     
- Partials       333     336       +3

cwgoes

Thanks @sunnya97 - mostly looks good, left comments on spec correctness, BeginBlock/EndBlock, and iterator ordering.

I think these changes make it pretty easy to allow delegators to have multiple simultaneous unbonding delegations from the same validator / redelegations from one validator to another validator. Just need some simple ID for the struct store keys - maybe the counter we're now using for validator ordering. (that can be a separate PR though)

cwgoes · 2018-09-26T11:35:29Z

docs/spec/staking/end_block.md

+
+Note that unlike CompleteUnbonding slashing of redelegating shares does not
+take place during completion. Slashing on redelegated shares takes place
+actively as a slashing occurs.


Then explain why we need this, e.g. "The redelegation completion queue serves simply to clean up state, as redelegations older than an unbonding period need not be kept."

cwgoes · 2018-09-26T11:37:24Z

docs/spec/staking/end_block.md

+
+## CompleteUnbonding
+
+Complete the unbonding and transfer the coins to the delegate. Perform any


How about "realize" any slashing, "perform" is a bit confusing since the slashing already happened. Let's explain how the slashing already occurred - namely, that the unbonding delegation struct will be updated at that point to reflect the reduced later withdrawal amount.

cwgoes · 2018-09-26T11:37:48Z

docs/spec/staking/end_block.md

+unbondingQueue(currTime time.Time):
+    for all unbondings whose CompleteTime < currTime:
+        validator = GetValidator(unbonding.ValidatorAddr)
+        returnTokens = ExpectedTokens * unbonding.startSlashRatio/validator.SlashRatio


I think this is wrong, we now track Balance explicitly.

docs/spec/staking/end_block.md

x/stake/handler.go

cwgoes · 2018-09-26T11:45:43Z

x/stake/handler.go

@@ -37,6 +33,46 @@ func NewHandler(k keeper.Keeper) sdk.Handler {

 // Called every block, process inflation, update validator set
 func EndBlocker(ctx sdk.Context, k keeper.Keeper) (ValidatorUpdates []abci.Validator) {
+	cdc := types.MsgCdc


Is there a reason the queue logic is run in the EndBlocker instead of the BeginBlocker? I guess it doesn't matter much but the latter would seem a bit more accurate since it means state during execution of transactions within the block will always be consistent (any old redelegations / unbonding delegations will have been deleted).

Idk, I did the governance one during EndBlock, so just did this one in EndBlock as well. Don't mind moving it though, cause I don't think it really matters either way.

What's the reason for doing it in BeginBlock instead of EndBlock? Also, why would the state of transactions in a block not be consistent if it happens in EndBlock?

any old redelegations / unbonding delegations will have been deleted

This is also true if you move if move it to the endblock of the previous block - so I'd say it's still just as arbitrary.

The general trend which I've seen in our development is to include more state machine logic in EndBlock - and more special block prep logic in BeginBlock. Although the more I think about it, the more I want to just Merge the two

I'm in favour of leaving in endblock - however I also think it's totally arbitrary

I don't think it's arbitrary, if we put the logic in BeginBlock instead we now have the invariant that all transactions will see a consistent view of the unbonding delegations / redelegations (where expired ones have been deleted), so we don't need to e.g. check timestamps on slashing.

I don't see why it changes the consistency? All the txs in this block will still see it in the queue, and all the txs in the next block, won't. You can already get rid of the check in slashing. Basically, it gives one extra block for evidence to be submitted through txs (which no evidence types are submitted in txs rn anyways, so kinda irrelevant).

alexanderbez

Thanks @sunnya97 -- I left some initial feedback 👍

Also second what @cwgoes suggested with BeginBlocker (if of course this is still valid to do).

alexanderbez · 2018-09-26T14:50:32Z

x/stake/keeper/delegation.go

@@ -155,6 +155,41 @@ func (k Keeper) RemoveUnbondingDelegation(ctx sdk.Context, ubd types.UnbondingDe
 	store.Delete(GetUBDByValIndexKey(ubd.DelegatorAddr, ubd.ValidatorAddr))
 }

+// Gets a specific unbonding queue timeslice. A timeslice is a slice of keys to an unbonding delegation


Missing period. Also, the latter is a bit confusing no? Are we getting a list of delegator/validator address pairs for a certain time?

I would also prefix this and the methods below with Get, Set, and Insert respectively to make it more clear and easier to read.

alexanderbez · 2018-09-26T14:54:37Z

x/stake/keeper/delegation.go

+
+// Insert an unbonding delegation to the appropriate timeslice in the unbonding queue
+func (k Keeper) UnbondingQueueInsert(ctx sdk.Context, ubd types.UnbondingDelegation) {
+	timeSlice := k.UnbondingQueueGetTimeSlice(ctx, ubd.MinTime)


I think we can put types.DVPair{ubd.DelegatorAddr, ubd.ValidatorAddr} into a variable atop and use that in the cases. It'll shorten the code to read and DRY it up.

alexanderbez · 2018-09-26T14:55:05Z

x/stake/keeper/delegation.go

@@ -241,6 +276,41 @@ func (k Keeper) RemoveRedelegation(ctx sdk.Context, red types.Redelegation) {
 	store.Delete(GetREDByValDstIndexKey(red.DelegatorAddr, red.ValidatorSrcAddr, red.ValidatorDstAddr))
 }

+// Gets a specific redelegation queue timeslice. A timeslice is a slice of keys to an redelegation delegation
+func (k Keeper) RedelegationQueueGetTimeSlice(ctx sdk.Context, timestamp time.Time) (dvvTriplets []types.DVVTriplet) {


Same feedback as above.

alexanderbez · 2018-09-26T15:01:10Z

x/stake/keeper/key.go

@@ -154,6 +157,12 @@ func GetUBDsByValIndexKey(valAddr sdk.ValAddress) []byte {
 	return append(UnbondingDelegationByValIndexKey, valAddr.Bytes()...)
 }

+// gets the prefix for all unbonding delegations from a delegator
+func GetUnbondingDelegationTimeKey(timestamp time.Time) []byte {
+	bz, _ := timestamp.MarshalBinary()


Do you think we should panic here and in GetRedelegationTimeKey to be defensive/safe?

alexanderbez · 2018-09-26T15:01:51Z

x/stake/types/delegation.go

@@ -9,6 +9,19 @@ import (
 	sdk "github.com/cosmos/cosmos-sdk/types"
 )

+// struct that just has a delegator validator pair with no other data. To be used to marshal keys


Let's be consistent with the punctuation/use of full sentences. i.e. pick one or the other 👍

alexanderbez · 2018-09-26T15:01:56Z

x/stake/types/delegation.go

+	ValidatorAddr sdk.ValAddress
+}
+
+// struct that just has a delegator validator validator triplet with no other data. To be used to marshal keys


x/stake/types/delegation.go

rigelrozanski · 2018-09-27T07:32:41Z

x/stake/handler.go

+	endBlockerTags := sdk.EmptyTags()
+
+	unbondingTimesliceIterator := k.UnbondingQueueIterator(ctx, ctx.BlockHeader().Time)
+	for ; unbondingTimesliceIterator.Valid(); unbondingTimesliceIterator.Next() {


could we add a short comment here describing how this iterator actually works? - it's a big confusing just looking at the code. - for instance, it's not immediately apparent to me how the iterator breaks out once it's gone past the appropriate time for an unbond

rigelrozanski

Generally looks well done - couple comments above - the only important request I'd like to make is that we test the "inserted" queue. By inserted queue test I mean, write a test for EndBlock functionality testing an redelegation/unbond transaction from a validator which is already in the unbonding state. The existing tests didn't cover this case because it was relevant previous to the queue (triggered by transaction).

This test should probably be located in x/stake/handler_test.go and look kinda like something between TestRedelegationPeriod from x/stake/handler_test.go and TestRedelegateFromUnbondingValidator from x/stake/keeper/delegation_test.go

x/stake/handler_test.go

rigelrozanski · 2018-10-01T04:32:27Z

x/stake/keeper/delegation.go

@@ -449,6 +457,8 @@ func (k Keeper) getBeginInfo(ctx sdk.Context, params types.Params, valSrcAddr sd
 	minTime time.Time, height int64, completeNow bool) {

 	validator, found := k.GetValidator(ctx, valSrcAddr)
+	fmt.Println(validator.Status)


please delete all the debugging code in this file (see 6ae62f6)

rigelrozanski · 2018-10-01T04:33:35Z

Also test_cover failing

alexanderbez · 2018-10-01T14:34:37Z

x/stake/keeper/delegation.go

+}
+
+// Returns a concatenated list of all the timeslices before currTime, and deletes the timeslices from the queue
+func (k Keeper) GetAllMatureUnbondingQueue(ctx sdk.Context, currTime time.Time) (matureUnbonds []types.DVPair) {


Doesn't seem like GetAllMatureUnbondingQueue and GetRedelegationQueueTimeSlice are idempotent. Based on the Get* nomenclature, I'd expect it to be so.

How do we feel about that? If we're going to keep it as non-idempotent. May I suggest changing the name?

If I understand this correctly, during this Get we are actually also deleting the records from the queue after they are retrieved (aka modifying the state, aka non-idempotent) - Yeah, for that reason I'd agree we should consider a different function-name-prefix. Seems like the most technically correct term to use here would be Dequeue - ideally we should also prefix the function which adds elements to these queues with Enqueue to stay consistent

alexanderbez

utACK pending CI fixes -- thanks @sunnya97 👍

cwgoes · 2018-10-02T10:31:00Z

@sunnya97 Do you know why the simulator is failing?

sunnya97 · 2018-10-02T16:38:44Z

Isn't the simulator failing on develop as well?

cwgoes · 2018-10-02T17:27:55Z

Isn't the simulator failing on develop as well?

No.

rigelrozanski · 2018-10-02T17:29:22Z

Let's rebase onto staking refactor

cwgoes · 2018-10-03T16:41:19Z

Staking refactor now merged, this just needs conflict resolution.

rigelrozanski · 2018-10-03T16:43:50Z

RE: as a part of this fix we should use the method unbondingToUnbonded in the state transition stuff now

sunnya97 requested review from cwgoes, ebuchman, rigelrozanski and zramsay as code owners September 26, 2018 07:18

sunnya97 assigned rigelrozanski and cwgoes Sep 26, 2018

sunnya97 added C:x/staking ready-for-review labels Sep 26, 2018

cwgoes suggested changes Sep 26, 2018

View reviewed changes

alexanderbez suggested changes Sep 26, 2018

View reviewed changes

sunnya97 force-pushed the sunny/staking-unbonding-queue branch from aed49da to 766dc94 Compare September 26, 2018 22:25

rigelrozanski reviewed Sep 27, 2018

View reviewed changes

rigelrozanski suggested changes Sep 27, 2018

View reviewed changes

sunnya97 force-pushed the sunny/staking-unbonding-queue branch from 3eac436 to 2baada9 Compare September 28, 2018 17:32

sunnya97 mentioned this pull request Sep 28, 2018

Add Querier for getting all unbonds and redelegations that end at a certain time #2418

Closed

rigelrozanski reviewed Sep 28, 2018

View reviewed changes

x/stake/handler_test.go Show resolved Hide resolved

rigelrozanski reviewed Oct 1, 2018

View reviewed changes

sunnya97 force-pushed the sunny/staking-unbonding-queue branch from 9b5c60c to b192b1d Compare October 1, 2018 08:35

alexanderbez reviewed Oct 1, 2018

View reviewed changes

alexanderbez approved these changes Oct 1, 2018

View reviewed changes

rigelrozanski approved these changes Oct 2, 2018

View reviewed changes

unbonding redelegation queue

12fbe44

sunnya97 added 8 commits October 5, 2018 17:09

address some of bez and chris review

d0f49f1

delete old timeslices from queue

f690712

added Rigel's test case

49f1670

added end-time to tags

89af16e

fixed bug in staking

96b4c4d

removed prints

b0f29a1

Get -> Queue

92770ee

called Endblocker in test

668cfef

sunnya97 force-pushed the sunny/staking-unbonding-queue branch from 74ef6c5 to 668cfef Compare October 6, 2018 19:14

cwgoes approved these changes Oct 8, 2018

View reviewed changes

cwgoes merged commit cd21427 into develop Oct 8, 2018

cwgoes deleted the sunny/staking-unbonding-queue branch October 17, 2018 03:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

R4R: Unbonding and Redelegations Queue #2405

R4R: Unbonding and Redelegations Queue #2405

sunnya97 commented Sep 26, 2018 •

edited

Loading

codecov bot commented Sep 26, 2018 •

edited

Loading

cwgoes left a comment

cwgoes Sep 26, 2018

cwgoes Sep 26, 2018

cwgoes Sep 26, 2018

cwgoes Sep 26, 2018

sunnya97 Sep 26, 2018

rigelrozanski Sep 27, 2018

cwgoes Sep 27, 2018

sunnya97 Sep 27, 2018

alexanderbez left a comment •

edited

Loading

alexanderbez Sep 26, 2018

alexanderbez Sep 26, 2018

alexanderbez Sep 26, 2018

alexanderbez Sep 26, 2018

alexanderbez Sep 26, 2018

alexanderbez Sep 26, 2018

rigelrozanski Sep 27, 2018 •

edited

Loading

rigelrozanski left a comment

rigelrozanski Oct 1, 2018

rigelrozanski commented Oct 1, 2018

alexanderbez Oct 1, 2018

rigelrozanski Oct 1, 2018

alexanderbez Oct 1, 2018

alexanderbez left a comment •

edited

Loading

cwgoes commented Oct 2, 2018

sunnya97 commented Oct 2, 2018

cwgoes commented Oct 2, 2018

rigelrozanski commented Oct 2, 2018

cwgoes commented Oct 3, 2018

rigelrozanski commented Oct 3, 2018


		## CompleteUnbonding

		Complete the unbonding and transfer the coins to the delegate. Perform any

R4R: Unbonding and Redelegations Queue #2405

R4R: Unbonding and Redelegations Queue #2405

Conversation

sunnya97 commented Sep 26, 2018 • edited Loading

codecov bot commented Sep 26, 2018 • edited Loading

Codecov Report

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

alexanderbez 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

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 Sep 27, 2018 • edited Loading

Choose a reason for hiding this comment

rigelrozanski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rigelrozanski commented Oct 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderbez left a comment • edited Loading

Choose a reason for hiding this comment

cwgoes commented Oct 2, 2018

sunnya97 commented Oct 2, 2018

cwgoes commented Oct 2, 2018

rigelrozanski commented Oct 2, 2018

cwgoes commented Oct 3, 2018

rigelrozanski commented Oct 3, 2018

sunnya97 commented Sep 26, 2018 •

edited

Loading

codecov bot commented Sep 26, 2018 •

edited

Loading

alexanderbez left a comment •

edited

Loading

rigelrozanski Sep 27, 2018 •

edited

Loading

alexanderbez left a comment •

edited

Loading