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

x/stake: Add global redelegation/unbonding index #1402

Closed
ValarDragon opened this issue Jun 27, 2018 · 10 comments · Fixed by #3243
Closed

x/stake: Add global redelegation/unbonding index #1402

ValarDragon opened this issue Jun 27, 2018 · 10 comments · Fixed by #3243

Comments

@ValarDragon
Copy link
Contributor

Currently this function: https://github.com/cosmos/cosmos-sdk/blob/develop/x/stake/keeper/key.go#L158 will have collisions at the same key. e.g.
Delegator has 10 steak bonded to validator A.
Delegator rebonds 3 steak to validator B
Delegator rebonds 5 steak to validator B - Issue here, the key is already in use.

One simple fix is to add an additional component to that key derivation, by appending bytes for a global delegation index, and then incrementing the global delegation index. Anything trying to access that redelegation can iterate over all the different nonces.

From a discussion with @cwgoes

@ValarDragon ValarDragon changed the title staking: Add global delegation index staking: Add global redelegation index Jun 27, 2018
@ValarDragon ValarDragon changed the title staking: Add global redelegation index x/stake: Add global redelegation index Jun 27, 2018
@ebuchman ebuchman added the T:Bug label Jun 30, 2018
@rigelrozanski rigelrozanski changed the title x/stake: Add global redelegation index x/stake: Add global redelegation/unbonding index Jul 11, 2018
@rigelrozanski
Copy link
Contributor

This also applies to unbonding

Note I don't think that the user should be required to track these nonces, the command should just pick up from the most relevant object (if possible) - going to think about this one a bit more

@rigelrozanski
Copy link
Contributor

I also wanted to note that in adding this we need some slightly more complex backend logic for processing unbonding-delegations or redelegations where there are multiple objects on the same routes. Related: #1624

@cwgoes
Copy link
Contributor

cwgoes commented Oct 16, 2018

This is not done, I only implemented a workaround.

@cwgoes cwgoes reopened this Oct 16, 2018
@jackzampolin
Copy link
Member

So does that mean we can move this out of the game-of-stakes tag? cc @cwgoes

@cwgoes
Copy link
Contributor

cwgoes commented Oct 23, 2018

So does that mean we can move this out of the game-of-stakes tag? cc @cwgoes

Yup! So moved.

@cwgoes
Copy link
Contributor

cwgoes commented Nov 26, 2018

@rigelrozanski I think we should do this fairly soon, any additional thoughts or can I start implementing?

@rigelrozanski
Copy link
Contributor

no I think this is a fairly straightforward solution. Just hmu with the R4R PR and I'll review

@cwgoes
Copy link
Contributor

cwgoes commented Nov 28, 2018

Hmm, how about we just store an array of unbonding delegations or redelegations instead? That seems simpler and requires less keyspace iteration. It doesn't seem too likely that a user would have too many unbonding delegations or redelegations to make the whole array fetch/write inefficient (and we often need to fetch all unbonding delegations / redelegations anyways).

@rigelrozanski
Copy link
Contributor

Seems like a dandy solution

@rigelrozanski
Copy link
Contributor

rigelrozanski commented Jan 6, 2019

Upon further investigation, it seems that it may increase complexity to store an array rather than use an index due to how the timestamp indexes are constructed for the auto-unbonding/redelegation maturing. - it's further unfortunate as the looping through the array would separate the queue mature logic from the single timestamp queue logic as is currently, to that and the new array loop at the bond maturing location... will think about this a bit more, but we may end up still wanting to go with an index


edit: might still be the best, just going to make the logic a bit weird


edit edit: it's not so bad, actually think it's the best solution atm lol

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants