Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
staketia migration - staketia accounting #1214
staketia migration - staketia accounting #1214
Changes from all commits
e7913c3
761f16d
6c78140
8d5c03f
e1be294
a68981c
f7955d5
ea2b9d7
b61055a
3173670
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is that the ordering of refreshing the RR and sending delegations and undelegations matters. The comments here suggest the same.
If we move the update RR step to stakeibc, but keep the delegation and undelegation txs in staketia, we just need to make sure the update RR is run at the same epochly cadence.
Just to triple check - it is, right? I.e. the cadence at which we ran it here matches the cadence at which we run it in stakeibc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GREAT question! The cadence will be different in stakeibc (it runs every stride epoch instead of every day epoch)
i think how we have it right now, the RR will be ~6 hours stale when its used for unbondings here. But imo, since that's pretty negligible, I'd prefer to keep it as is, rather than mess with the ordering. Wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I see.
So just to map out the worst case: some packets may be are stuck or reinvestment would be otherwise delayed for a long time, then we'd see a large RR jump that triggers within the "stale period". In that case, we'd use the stale RR for unbonding and the users who receive those unbondings would not receive their yield from the "stuck packet" period. Is that right?
Just to explore syncing up the epochs.... what if we had staketia epochs run every 6 hours? I imagine not much would change in the 6-hourly logic until the operator advances the state. But in that case, would this edge case or the "stale period" be solved?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think that worst case is righ!, But I think it's pretty unlikely that we have some multi-day bottleneck that just happens to clear in this 6 hour window. And even in that case, the impact is low - they just get the RR at the time of redemption (similar to how it used to be done).
Running the epochs every 6 hours could work, but I'd be a bit worried we have some implicit invariant about the length and not sure it's worth the effort to investigate based on the above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah good point that it'd revert to how it used to be done. We did get some complaints about that, but as long as we don't have a long packet backlog we're good. And ultimately this migration period is temporary so the time window within which we could see backlogs is bounded.
Fwiw I'm mostly convinced to leave things as is, especially because of "we [may] have some implicit invariant about the length" could lead to a serious accounting issue if overlooked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah good point about this being temporary!
also I assume the complaints were more in the case where the unbondings were delayed right? I’d imagine w/o a delay, getting the same RR that’s shown in the FE at time of redemption wouldnt upset anyone!