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

Updated sweep calculation #577

Open
wants to merge 10 commits into
base: feat/oracle-v5
Choose a base branch
from
Open

Updated sweep calculation #577

wants to merge 10 commits into from

Conversation

madlabman
Copy link
Contributor

@madlabman madlabman commented Dec 13, 2024

Thing to consider: the spec-like logic is heavy, but has almost the same results as the current approach with no pending withdrawals; pending withdrawals have meaningful effect when there are thousands of them which doesn't seem to me as a case for mainnet.

It seems the old algo has an error due to rounding down via int().

Next steps: I would decide to go or not to go with the new algo. From my perspective it's better to abandon it and use the old approach with an updated multiplier to cover the case with pending withdrawals.

@madlabman madlabman force-pushed the electra-sweep branch 3 times, most recently from 70897c6 to d0cf8f4 Compare December 13, 2024 17:49
@madlabman madlabman marked this pull request as ready for review December 13, 2024 17:58
@madlabman madlabman force-pushed the electra-sweep branch 2 times, most recently from fb335f7 to 0160038 Compare December 18, 2024 08:35
@madlabman madlabman added the V5 label Dec 22, 2024
@@ -286,19 +279,31 @@ def _get_latest_exit_epoch(self, blockstamp: ReferenceBlockStamp) -> tuple[Epoch
def _get_sweep_delay_in_epochs(self, blockstamp: ReferenceBlockStamp) -> int:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe just split function into two
_get_sweep_delay_in_epochs_v1
_get_sweep_delay_in_epochs_v3

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, done.

return [
v
for v in self.w3.cc.get_validators(blockstamp)
if is_partially_withdrawable_validator(v) or is_fully_withdrawable_validator(v, blockstamp.ref_epoch)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would we like to tweak the is_partially_withdrawable_validator function a bit to take into account validators with balances 32 ETH exactly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. In the current approach it looks like it's better to not add such an assumption for prediction.

@madlabman madlabman requested a review from F4ever January 7, 2025 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants