remove another potential abort in staking_pool #7571
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 4 Ignored Deployments
|
emmazzz
force-pushed
the
reward_withdraw
branch
from
7778577 to
9e28536
Compare
| @@ -352,6 +352,11 @@ module sui::staking_pool { | |||
| if (total_sui_withdraw_amount >= principal_withdraw_amount) | |||
| total_sui_withdraw_amount - principal_withdraw_amount | |||
| else 0; | |||
| // This may happen when we are withdrawing everything from the pool and | |||
| // the rewards pool balance may be 1 less than reward_withdraw_amount. | |||
| if (balance::value(&pool.rewards_pool) < reward_withdraw_amount) { | |||
There was a problem hiding this comment.
The exact reason why this happened needs to be investigated.
I think I would feel better if we did the investigation rather than adding this code--any case where we might be creating money (even if it's just 1 MIST) has to be avoided because it will make our reconciliation of the total SUI supply impossible.
There was a problem hiding this comment.
However: if we think running in testnet with this code added + emitting an event that will help us debug the issue, I'm ok with that. But I don't think we should plan to keep this code around once we've fixed the root cause.
| @@ -352,6 +352,11 @@ module sui::staking_pool { | |||
| if (total_sui_withdraw_amount >= principal_withdraw_amount) | |||
| total_sui_withdraw_amount - principal_withdraw_amount | |||
| else 0; | |||
| // This may happen when we are withdrawing everything from the pool and | |||
| // the rewards pool balance may be 1 less than reward_withdraw_amount. | |||
There was a problem hiding this comment.
Might make sense to assert that the leftover amount is 1 to check this assumption.
|
If this can be reproduced entirely locally, we probably can just investigate by adding a ton of prints (locally)? Not sure if events are really needed |
emmazzz
force-pushed
the
reward_withdraw
branch
from
a6ae13c to
35500c9
Compare
| // the rewards pool balance may be 1 less than reward_withdraw_amount. | ||
| // TODO: FIGURE OUT EXACTLY WHY THIS CAN HAPPEN. | ||
| if (balance::value(&pool.rewards_pool) < reward_withdraw_amount) { | ||
| reward_withdraw_amount = balance::value(&pool.rewards_pool); |
There was a problem hiding this comment.
I think you can use math::min
| @@ -352,6 +352,12 @@ module sui::staking_pool { | |||
| if (total_sui_withdraw_amount >= principal_withdraw_amount) | |||
| total_sui_withdraw_amount - principal_withdraw_amount | |||
| else 0; | |||
| // This may happen when we are withdrawing everything from the pool and | |||
| // the rewards pool balance may be 1 less than reward_withdraw_amount. | |||
There was a problem hiding this comment.
It doesn't necessarily have to be 1 less though.
There was a problem hiding this comment.
right although I've only seen 1 in test output, we don't know if it can only be 1. changing the comments
| let reward_withdraw_amount = staking_pool::process_pending_delegation_withdraws( | ||
| &mut self.delegation_staking_pool, ctx); | ||
| self.metadata.next_epoch_delegation = self.metadata.next_epoch_delegation - reward_withdraw_amount; | ||
| // assert!(delegate_amount(self) == self.metadata.next_epoch_delegation, 0); | ||
| staking_pool::process_pending_delegations(&mut self.delegation_staking_pool, ctx); |
There was a problem hiding this comment.
Do we want to add a TODO here so that one day we could add the assert back? Or is it never going to be guaranteed down the road?
There was a problem hiding this comment.
Will add a TODO here. I think this assert does hold today but I don't want to risk having it here and failing advance_epoch for next_epoch_delegation which doesn't matter too much now.
emmazzz
force-pushed
the
reward_withdraw
branch
from
35500c9 to
8a5a7eb
Compare
This showed up when stress unit testing delegation code with random amounts, random operations, etc. The rewards pool balance is 1 less than the rewards we are trying to withdraw, possibly due to rounding. The exact reason why this happened needs to be investigated. This PR removes the possibility of aborting at `balance::split` when withdrawing rewards from rewards pool. UPDATE: switching the order in which we process new delegations and delegation withdraws seems to fix the issue. It's still not super clear whether the original order was the only trigger of the abort though.
This showed up when stress unit testing delegation code with random amounts, random operations, etc. The rewards pool balance is 1 less than the rewards we are trying to withdraw, possibly due to rounding. The exact reason why this happened needs to be investigated. This PR removes the possibility of aborting at `balance::split` when withdrawing rewards from rewards pool. UPDATE: switching the order in which we process new delegations and delegation withdraws seems to fix the issue. It's still not super clear whether the original order was the only trigger of the abort though.
This showed up when stress unit testing delegation code with random amounts, random operations, etc. The rewards pool balance is 1 less than the rewards we are trying to withdraw, possibly due to rounding. The exact reason why this happened needs to be investigated. This PR removes the possibility of aborting at
balance::splitwhen withdrawing rewards from rewards pool.UPDATE: switching the order in which we process new delegations and delegation withdraws seems to fix the issue. It's still not super clear whether the original order was the only trigger of the abort though.