Make builders non-validating staked actors #4788
Conversation
jtraglia
force-pushed
the
builders-no-updated-withdrawal-types
branch
from
2e93bc6 to
f709f7d
Compare
…4789) I should have done this in the previous refactoring PR. * Rename `get_sweep_withdrawals` to `get_validators_sweep_withdrawals`. * Remove `validator_index` parameter from function. * Update comments & variables to reflect this change. If/when we merge #4788 which adds a builders sweep, this distinction will be necessary.
specs/gloas/beacon-chain.md
Outdated
| pubkey: BLSPubkey | ||
| withdrawal_credentials: Bytes32 | ||
| balance: Gwei | ||
| exit_epoch: Epoch |
There was a problem hiding this comment.
I think builder should also have a withdrawable_epoch. Once the exit_epoch is reached the builder becomes inactive, i.e. it cannot bid, and then until withdrawable_epoch the balance is frozen on the beacon chain. Without that freeze the DoS protection isn’t as strong as it can be
There was a problem hiding this comment.
Right now, builders cannot submit bids after initiating their exit. So there is a freeze. I chatted with @fradamt about this some and he has some concerns with not allowing builders to submit bids during this period. So maybe we'll change it & add a withdrawable_epoch field.
consensus-specs/specs/gloas/beacon-chain.md
Lines 1049 to 1050 in 4284aec
jtraglia
changed the title
| if builder_balance < min_balance: | ||
| return False | ||
| return builder_balance - min_balance >= bid_amount |
There was a problem hiding this comment.
oh you're modifying p2p as well, alright it makes sense, but it's a bit confusing to interleave p2p validation with core functions because of this. It also ties p2p validation to having a particular state, the balance of the builder can change state by state and typically when validating p2p objects we are a little lax into which state you use to validate them otherwise it could lead to peering issues. I think that even if you insist in having this validation here, the p2p one should be more lax and be about the builder's balance (in whatever state the node is using to validate it) to be less than the min balance + bid_amount, regardless of pending payments in the pipeline.
| def get_expected_withdrawals( | ||
| state: BeaconState, | ||
| ) -> Tuple[Sequence[Withdrawal], uint64, uint64, uint64]: | ||
| ) -> Tuple[Sequence[Withdrawal], uint64, uint64, uint64, uint64]: |
There was a problem hiding this comment.
At this stage the return object of this function probably needs to be a named type
| ###### New `get_index_for_new_builder` | ||
|
|
||
| ```python | ||
| def get_index_for_new_builder(state: BeaconState) -> BuilderIndex: |
There was a problem hiding this comment.
Flagging this function as a possible problem. Don't have it clear yet but it's something that clients may want to discuss in the breakout. We typically have a global pubkey -> index (or inverse) cache for signature validation and this type of reusage would make this cache a little more sensitive to be in sync with the head view.
Not against this, but just flagging as a possible shot in our feet.
| current_epoch = compute_epoch_at_slot(state.slot) | ||
| return not builder.slashed or current_epoch >= builder.withdrawable_epoch | ||
| ``` | ||
| def get_builder_withdrawals( |
There was a problem hiding this comment.
Not sure if it is just me but I've found get_builder_withdrawals a bit confusing as it sounds like general withdrawals although it's actually builder payments. Perhaps we could use something like get_builder_payment_withdrawals.
This comment also applies to state.builder_pending_withdrawals as well. It's a pair of builder_pending_withdrawals and builder_pending_payments, but perhaps it's better to distinguish it via something like payment withdrawals as we now introduce sweep withdrawals for builders.
There was a problem hiding this comment.
Yes, I also find this confusing. Let's rename this in a separate PR though.
This PR makes a new
Buildertype, tracked instate.builders.Some key improvements:
Note: This PR does not fully test everything. We can handle that in separate PRs.