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

Partial withdrawals #2862

Merged
merged 9 commits into from
Jun 8, 2022
Merged

Partial withdrawals #2862

merged 9 commits into from
Jun 8, 2022

Conversation

djrtwo
Copy link
Contributor

@djrtwo djrtwo commented Mar 24, 2022

Partial withdrawals for 0x01 validators. Utilizes a method that just sweeps a maximum number of partially withdrawable validators on a per-epoch basis

  • On each epoch transition -- cycle through the validator set (remembering where you last left off (via next_partial_withdrawal_validator_index)
    • if the validator is partially withdrawable (0x01 creds and balance in excess of 32 eth -- is_partially_withdrawable_validator), withdraw the excess balance
    • when cycle through whole list OR reach max withdrawals per epoch, abort process and mark where you left off for next time

Additionally, cleaned up some naming

See discussion below for rationale on config values. Take tuning these values to an issue if desired

discussion around if this should be all validators or just "active and not slashed" here -- #2905

@djrtwo djrtwo changed the base branch from dev to 00-to-01 March 24, 2022 16:00
@djrtwo djrtwo force-pushed the partial-withdrawals branch from f5c967f to 2dbafa2 Compare March 24, 2022 16:00
@djrtwo djrtwo mentioned this pull request Mar 24, 2022
5 tasks
@djrtwo djrtwo force-pushed the partial-withdrawals branch from 2dbafa2 to d84000e Compare March 24, 2022 16:11
@mcdee
Copy link
Contributor

mcdee commented Mar 24, 2022

Thinking about MAX_PARTIAL_WITHDRAWALS_PER_EPOCH, and the related MAX_WITHDRAWALS_PER_PAYLOAD.

On the current value of MAX_PARTIAL_WITHDRAWALS_PER_EPOCH (256), with the current number of active validators it would work through the entire set every ~10 days, which seems fast. Given that we're adding weight to the consensus and execution chains with this data I'd be inclined to bring it down to 128, which means that even if we add another 50% to the active validator set then validators can still skim off approximately once a month. Frankly, I don't think that once a quarter is unreasonable but we don't know the size of the active validator set in future so this seems like a decent compromise.

If we did this, then we could reduce MAX_WITHDRAWALS_PER_PAYLOAD from 16 to 8 with no harm (with all Ether staked the churn limit would be ~50 so there would be space for all withdrawals and all partial withdrawals generated within the epoch even if we only proposed 70% of the blocks). Although as the struct is a variable size list we could just leave this alone.

@dapplion
Copy link
Member

dapplion commented Mar 25, 2022

So this is a "non-voluntary" partial withdrawal right? This can do a partial withdrawal for 1 Gwei if the validator balance is 32_000_000_001, should there be a minimum above MAX_EFFECTIVE_BALANCE? Looking at current mainnet balances this mechanism will forever create withdrawals maxing MAX_PARTIAL_WITHDRAWALS_PER_EPOCH since when it loops to index 0 all balances are already above MAX_EFFECTIVE_BALANCE

EDIT: From @mcdee comment I see that's the idea. Agree then that MAX_PARTIAL_WITHDRAWALS_PER_EPOCH should be tuned

@djrtwo
Copy link
Contributor Author

djrtwo commented Mar 25, 2022

Regarding the number per epoch and the number per slot to be dequeued, there's a few considerations I'm taking

  • Picking a number for MAX_PARTIAL_WITHDRAWALS_PER_EPOCH that is larger than would be possible given a block-proposal-led method. e.g. 256 being 8x per epoch than if we just did one per proposal
  • Then, picking a number of MAX_WITHDRAWALS_PER_PAYLOAD such that if MAX_PARTIAL_WITHDRAWALS_PER_EPOCH and the full get_validator_churn_limit() for each epoch is hit, that the queue does not back up in the long run if approximately 50% of slots are non-empty on average.
    • get_validator_churn_limit() can theoretically get up to ~50 in the event that all ETH in existence is staked (~100M ETH or about 3M validators).
    • In this extreme case (and assuming the max exits per epoch), you'd need about 19 non-empty slots (60%) per epoch on average to not have a backup in the queue with the current 16 withdrawals per payload. In more modest staked amounts (e.g. 30M ETH), you clear the queue at more like 17 non-empty slots per epoch (53%)
  • Finally, we also must consider load on the execution layer (both in balance updates -- which induces hashing -- and in the payload data costs)
    • withdrawals are likely to be approximately 60 bytes {amount: 32 bytes, address: 20 bytes, index: 8 bytes}. so at max 16 per payload, that's ~1kb (60 * 16) which is about +1% to +1.5% on current block data sizes (and way less in the event of 4844 or calldata repricing schemes
    • withdrawals also induce up to a maximum number of balance updates which induce some amount of hashing (this looks akin to a max 16 COINBASE updates in current config) ... I suspect on this order for this not to be a large consideration for fixed system overhead. But it's worth asking an expert.

I'm totally fine re-configuring some of these values, but given the low cost of 16 as MAX_WITHDRAWALS_PER_PAYLOAD and with this 256 number cleaning clearing the queue with 50% to 60% non-empty slots, I don't see the value in reducing the number yet. Especially considering that it is "free" for the validator -- they don't have to send an L1 transaction to claim this partial amount so that can accumulate happily over time.

Another thing to note is that less access to profits for solo/small stakers can be a centralizing pressure. So if we can provide a higher partial withdrawal rate at very low cost, it is by default a good thing unless shown unnecessarily costly on some other front.

specs/capella/beacon-chain.md Outdated Show resolved Hide resolved
specs/capella/beacon-chain.md Show resolved Hide resolved
specs/capella/beacon-chain.md Show resolved Hide resolved
@djrtwo djrtwo force-pushed the partial-withdrawals branch from dc15780 to 4d10a79 Compare March 25, 2022 16:02
@mcdee
Copy link
Contributor

mcdee commented Mar 26, 2022

On the technical front, to grab some quick numbers there are around 210 balance changes per block (quick sample of the last 10,000 blocks at time of writing), so adding another 8 would be a ~4% increase. Not sure if this is considered significant, but thought I'd put it out there.

One of my initial concerns (and apologies for not putting this in there to begin with, but I didn't run the numbers) would be the % of withdrawal that may be used as gas. If we take 50Gewi as the effective gas price (48Gwei average bass fee over the last 10,000 blocks and 2Gwei tip) and pick a relatively normal transaction such as converting to USDC, then the cost of doing so is ~10% of the median withdrawal amount. This feels high.

I'm fully aware that rather than halve the number of withdrawals per epoch stakers could just wait for two cycles to transfer their funds and there would be no difference in reality, but given the (even minor) additional cost to the network to calculate, store and process the partial withdrawals it feels like we could halve it without any major downsides, and in doing so save a few cycles and possibly avoid some complaints about the relative cost of moving withdrawals in the future.

But given the main point of my argument is around how this may feel to stakers, which isn't the hardest of positions, perhaps take this more as an observation for consideration than a strong reason to halve MAX_PARTIAL_WITHDRAWALS_PER_EPOCH

@djrtwo
Copy link
Contributor Author

djrtwo commented Mar 29, 2022

The balance update count is great! thank you @mcdee. I do wonder though -- do balance updates matter as the comparison here or more so all of the writes

If we take 50Gewi as the effective gas price (48Gwei average bass fee over the last 10,000 blocks and 2Gwei tip) and pick a relatively normal transaction such as converting to USDC, then the cost of doing so is ~10% of the median withdrawal amount. This feels high.

yeah, I see that. "is there value in having this granularity of amounts available"

Copy link

@Ikaberry202 Ikaberry202 left a comment

Choose a reason for hiding this comment

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

Good to go

Copy link

@Ikaberry202 Ikaberry202 left a comment

Choose a reason for hiding this comment

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

Good to go

Base automatically changed from 00-to-01 to dev May 5, 2022 13:31
@djrtwo djrtwo changed the title [WIP] Partial withdrawals Partial withdrawals Jun 3, 2022
@djrtwo djrtwo force-pushed the partial-withdrawals branch 3 times, most recently from 5595953 to 333d25b Compare June 3, 2022 20:13
@djrtwo djrtwo force-pushed the partial-withdrawals branch from 333d25b to 3f37d1b Compare June 3, 2022 20:24
Copy link

@Ikaberry202 Ikaberry202 left a comment

Choose a reason for hiding this comment

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

Good to go

Comment on lines 392 to 394
assert len(dequeued_withdrawals) == len(payload.withdrawals)
for dequeued_withdrawal, withdrawal in zip(dequeued_withdrawals, payload.withdrawals):
assert dequeued_withdrawal == withdrawal
Copy link
Contributor

Choose a reason for hiding this comment

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

Can these three lines be simplified to assert dequeue_withdrawals == payload.withdrawals?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

@hwwhww hwwhww left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@ralexstokes ralexstokes left a comment

Choose a reason for hiding this comment

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

nice work! spec additions look good, I'll make another pass on the testing

specs/capella/beacon-chain.md Outdated Show resolved Hide resolved
Comment on lines 392 to 394
assert len(dequeued_withdrawals) == len(payload.withdrawals)
for dequeued_withdrawal, withdrawal in zip(dequeued_withdrawals, payload.withdrawals):
assert dequeued_withdrawal == withdrawal
Copy link
Member

Choose a reason for hiding this comment

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

@djrtwo djrtwo merged commit 74489d5 into dev Jun 8, 2022
@djrtwo djrtwo deleted the partial-withdrawals branch June 8, 2022 19:16
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.

8 participants