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

[WIP] Withdrawals pull. #2759

Closed
wants to merge 19 commits into from
Closed

[WIP] Withdrawals pull. #2759

wants to merge 19 commits into from

Conversation

djrtwo
Copy link
Contributor

@djrtwo djrtwo commented Dec 1, 2021

First of the withdrawal features to be specified. Feature metatracking issue here -- #2758

Fork has code-name -- Capella. I'm assuming Merge will have a B-star-name so putting a C-star-name placeholder for now. "Withdrawals" is too confusing of a placeholder name.

During the epoch transition, if a validator has 0x01 withdrawal credentials and is withdrawable, fully withdraw the validator -- i.e. create a withdrawal receipt and add it to the state, and zero out the validator balance

Some details to note:

  • A commitment to withdrawal_receipts (e.g. the root) to be exposed in EL for normal EL TX to consume to move the ETH into the end account
  • index (always increasing) included in WithdrawalReceipt so that a simple bitfield (of length max_successful_withdrawal_receipt_index) can be used to track consumed receipts in EL
  • A 0x00 -> 0x01 change-of-credentials operation is required to trigger a withdrawal for existing 0x00 validators (new operation to come in subsequent PR)
  • Partial withdrawals will use the saw withdraw function but not use entire balance (to be developed in subsequent PR)

todo:

  • add a withdrawn_epoch to Validator. This will be useful for any sort of validator index reuse (e.g. safe to be reused after 50k epochs)
  • don't use the just a simple SSZ list for the accumulator. With partial withdrawals this can grow at ~100MB / year. It's append only so you can send most receipts to disk but this means the WS states cant grow to cumbersome sizes.
    • main idea -- create a CommitmentContainer that just has the merkle root and right-most merkle branch that works for append-only trees (and has some merkle tree structure as a List)
    • backup idea -- use ring-buffer
      • simple
      • but puts a time limit on how long you have to claim in EL because receipts get overwritten
  • Add some epoch transition sanity tests

@zilm13
Copy link
Contributor

zilm13 commented Dec 2, 2021

I don't know if this is the right issue for it, but we should make some estimations on:

  • load on queues and BeaconChain just after withdrawal feature introduction
  • regular BeaconChain and queues load if feature "Partial withdrawal" is introduced

Copy link
Contributor

@mkalinin mkalinin left a comment

Choose a reason for hiding this comment

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

An option with a ring-buffer plus historical_roots accumulator also worth considering, IMO

specs/capella/beacon-chain.md Outdated Show resolved Hide resolved
specs/capella/beacon-chain.md Outdated Show resolved Hide resolved
specs/capella/beacon-chain.md Outdated Show resolved Hide resolved
specs/capella/beacon-chain.md Outdated Show resolved Hide resolved
@djrtwo
Copy link
Contributor Author

djrtwo commented Dec 2, 2021

Right @mkalinin, I was also considering a double batched merkle accumulator (like historic roots). One reason I would like to try this other method first (commitmnt container) is that the claim process/logic will likely be much simpler because it will just look like a merkle tree, constantly growing. Additionally, it's not immediately clear when to do the batching, but that can be solved relatively easily.

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.

Great work!

I haven't started to review the content yet. Just want to remind you of the CI setting earlier.

setup.py Outdated Show resolved Hide resolved
.circleci/config.yml Show resolved Hide resolved
@mkalinin
Copy link
Contributor

mkalinin commented Dec 3, 2021

Right @mkalinin, I was also considering a double batched merkle accumulator (like historic roots). One reason I would like to try this other method first (commitmnt container) is that the claim process/logic will likely be much simpler because it will just look like a merkle tree, constantly growing. Additionally, it's not immediately clear when to do the batching, but that can be solved relatively easily.

Actually, user may utilize historical batches to build a persistent proof with the merkle tree solution:

  1. Get a proof linking receipt to the withdrawal receipts tree root when the receipt is added to the tree
  2. Get a proof linking withdrawal receipts tree root from (1) to the historical batch once the root of the state that contains the receipts tree root from (1) is accumulated
  3. Get a proof linking the historical batch root from (2) to the beacon/state root and claim the receipt on EL side at any time

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.

some nitpicking.

the logic looks good to me with bare-eye review atm. need to add more test cases to verify it.


| Name | Value | Unit | Duration |
| - | - | :-: | :-: |
| `WITHDRAWAL_RECEIPT_LIMIT` | `uint64(2**40)` (= 1,099,511,627,776) | withdrawal receipts|
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to be added to preset files.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
| `WITHDRAWAL_RECEIPT_LIMIT` | `uint64(2**40)` (= 1,099,511,627,776) | withdrawal receipts|
| `WITHDRAWAL_RECEIPT_LIMIT` | `uint64(2**40)` (= 1,099,511,627,776) | withdrawal receipts |

Comment on lines +33 to +34
| Name | Value | Unit | Duration |
| - | - | :-: | :-: |
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
| Name | Value | Unit | Duration |
| - | - | :-: | :-: |
| Name | Value | Unit |
| - | - | :-: |

Comment on lines +34 to +39
If `state.slot % SLOTS_PER_EPOCH == 0` and `compute_epoch_at_slot(state.slot) == CAPELLA_FORK_EPOCH`,
an irregular state change is made to upgrade to Capella.

The upgrade occurs after the completion of the inner loop of `process_slots` that sets `state.slot` equal to `CAPELLA_FORK_EPOCH * SLOTS_PER_EPOCH`.
Care must be taken when transitioning through the fork boundary as implementations will need a modified [state transition function](../phase0/beacon-chain.md#beacon-chain-state-transition-function) that deviates from the Phase 0 document.
In particular, the outer `state_transition` function defined in the Phase 0 document will not expose the precise fork slot to execute the upgrade in the presence of skipped slots at the fork boundary. Instead the logic must be within `process_slots`.
Copy link
Contributor

Choose a reason for hiding this comment

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

These words seem from Altair specs. I think seeing Phase 0 here could be a little confusing. In Merge specs, we've changed the wording.

@hwwhww hwwhww added the general:enhancement New feature or request label Dec 6, 2021

## Introduction

Capella is a consensus-layer upgrade containin a number of features related
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Capella is a consensus-layer upgrade containin a number of features related
Capella is a consensus-layer upgrade containing a number of features related

@mcdee
Copy link
Contributor

mcdee commented Dec 8, 2021

Regarding partial withdrawals: there has been a fair amount of discussion on how this could be achieved with minimal additional complexity on the beacon chain. For ease of reading I'll refer to the partial withdrawal of funds above 32 Ether in a validator as "skimming".

Any early discussion regarding skimming was to allow a validator to skim at the same time they proposed a block. Although simple, it has several downsides. Most importantly, it provides no assurance as to when skimming for any given validator will take place: due to the random nature of proposer duties, one validator may skim twice in a month, whereas another may have to wait half a year. Another downside is that it requires the block to be proposed: if the validator misses the block proposal for any reason (or the block is proposed but fails to make it on-chain) then the opportunity for skimming is lost. These downsides are exacerbated with higher numbers of active validators.

Given this, it seems that a more reliable method of generating withdrawals would be nice. Manual operations to carry out skimming may appear attractive, however this requires a new operation, new p2p channels, additional block space etc. so should be avoided if possible.

One option is to have a regular, controlled skim of all eligible* validators on a periodic basis. For example, if the period was 1024 epochs then at the end of each epoch in the period 1/1024 of the validators would be processed, with each eligible validator in the set having a withdrawal receipt generated and made available to the execution layer for processing.

This removes the requirement for a new operation and all of the additional complexity and weight that goes with it, and maintains a regular schedule for rewards to be skimmed. Additional load on the beacon node should be minimal due to the small number of validators processed per epoch, and if this is a concern then the period could be increased accordingly.

Having a system for partial withdrawals is important to avoid validators churning through the exit and activation queues to retrieve rewards, but adding to complexity and weight of the beacon chain any more than necessary is not desired and the proposed mechanism achieves the former whilst avoiding the latter.

*For a validator to be eligible it would need to be ongoing_active (not slashed), have a balance > MAX_EFFECTIVE_BALANCE, and have 0x01 withdrawal credentials.

activation_epoch: Epoch
exit_epoch: Epoch
withdrawable_epoch: Epoch # When validator can withdraw funds
withdrawn_epoch: Epoch # [New in Capella]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do this, instead of just zeroing out the validator once it's withdrawn?

Copy link
Contributor

@potuz potuz Dec 23, 2021

Choose a reason for hiding this comment

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

It may be useful to later reuse the validator index. (Oh just realized it's in the PR description)

@protolambda
Copy link
Contributor

@djrtwo @ralexstokes To clarify the remaining TODO, you're looking for an implementation:

  • of an append-only merkle structure using the CommitmentContainer interface
  • similar to that of the deposit-contract merkle stack updates
  • that has the same root as a normal SSZ List would have
  • that does not store the actual list contents since you only just need an updateable merkle-root for withdrawal proofs?

I think that can be done, I can give it a try maybe tomorrow.

@protolambda
Copy link
Contributor

See protolambda@17a7065

CommitmentContainer was not the right fit here since we need to commit to all state contents, including the merkle stack contents. Simply caching the root works fine in this case.

Mirrored the deposit contract merkle-stack approach:

  • compute_withdrawal_tree_root to get the witthdrawal tree
  • withdraw updates the merkle stack in the state

Extra: since we don't have to gas-golf as much: after processing all withdrawals, cache the new latest withdrawal-tree root in the BeaconState (so light-clients can proof it easily, and everyone can access it without re-computing)

Not sure if we can carry over formal verification easily, but it's the same thing.

Besides testing the merkle-updates (or formal verification) we'll need some integration tests (e.g. produce the proof for a withdrawal)

```python
class WithdrawalReceipt(Container):
index: WithdrawalReceiptIndex
address: ExecutionAddress
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it would make sense making this Bytes32 preparing for the possibility of "address space extension"?

It has some downsides:

  1. The receipt is now packed, given 64 + 160 + 256 bits of values.
  2. The deposit mechanism is still locked in to 160-bit addresses anyway, and likely many other places on the beacon chain too.

capella: impl withdrawal tree like deposit tree
@djrtwo djrtwo changed the title (WIP) Withdraw withdrawable validators [WIP] Withdrawals pull Feb 23, 2022
@djrtwo
Copy link
Contributor Author

djrtwo commented Mar 15, 2022

closing in favor of the Push method #2836

@djrtwo djrtwo closed this Mar 15, 2022
@hwwhww hwwhww deleted the withdrawals branch May 10, 2022 15:17
@djrtwo djrtwo changed the title [WIP] Withdrawals pull [WIP] Withdrawals pull. Mar 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants