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

Switch to compounding when consolidating with source==target #8646

Merged
merged 11 commits into from
Oct 7, 2024
Prev Previous commit
Next Next commit
Switch to compounding when consolidating with source==target
  • Loading branch information
lucassaldanha committed Oct 7, 2024
commit 674c47de723ce61188b1eaecda5e891b45ff0450
Original file line number Diff line number Diff line change
Expand Up @@ -930,6 +930,14 @@ public void processConsolidationRequests(
// No Consolidations until Electra
}

@Override
public boolean isValidSwitchToCompoundingRequest(
final BeaconState beaconState, final ConsolidationRequest consolidationRequest)
throws BlockProcessingException {
// No Consolidations until Electra
return false;
}

// Catch generic errors and wrap them in a BlockProcessingException
protected void safelyProcess(final BlockProcessingAction action) throws BlockProcessingException {
try {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,10 @@ void processConsolidationRequests(
MutableBeaconState state, List<ConsolidationRequest> consolidationRequests)
throws BlockProcessingException;

boolean isValidSwitchToCompoundingRequest(
BeaconState beaconState, ConsolidationRequest consolidationRequest)
throws BlockProcessingException;

ExpectedWithdrawals getExpectedWithdrawals(BeaconState preState);

default Optional<BlockProcessorAltair> toVersionAltair() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -377,7 +377,7 @@ public void processDepositRequests(
public void processConsolidationRequests(
final MutableBeaconState state, final List<ConsolidationRequest> consolidationRequests) {
LOG.debug(
"process_consolidation_request: {} consolidation request to process from block at "
"process_consolidation_request: {} consolidation requests to process from block at "
+ "slot {}",
consolidationRequests.size(),
state.getSlot());
Expand All @@ -392,6 +392,23 @@ private void processConsolidationRequest(
final UInt64 slot = state.getSlot();
final UInt64 currentEpoch = miscHelpers.computeEpochAtSlot(slot);

if (isValidSwitchToCompoundingRequest(state, consolidationRequest)) {
LOG.debug("process_consolidation_request: valid switching validator to compounding address");
Copy link
Contributor

Choose a reason for hiding this comment

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

this would probably be more valuable with context, so that we can see the validator id or abbreviated key?

validatorsUtil
.getValidatorIndex(state, consolidationRequest.getSourcePubkey())
.ifPresent(
sourceValidatorIndex ->
beaconStateMutatorsElectra.switchToCompoundingValidator(
state, sourceValidatorIndex));
return;
}

// Verify that source != target, so a consolidation cannot be used as an exit
if (consolidationRequest.getSourcePubkey().equals(consolidationRequest.getTargetPubkey())) {
LOG.debug("process_consolidation_request: source_pubkey and target_pubkey must be different");
Copy link
Contributor

Choose a reason for hiding this comment

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

probably would add the abbreviated pubkey here too...

return;
}

// If the pending consolidations queue is full, consolidation requests are ignored
if (state.getPendingConsolidations().size()
== specConfigElectra.getPendingConsolidationsLimit()) {
Expand Down Expand Up @@ -428,14 +445,10 @@ private void processConsolidationRequest(
return;
}

// Verify that source != target, so a consolidation cannot be used as an exit.
if (maybeSourceValidatorIndex.get().equals(maybeTargetValidatorIndex.get())) {
LOG.debug("process_consolidation_request: source_pubkey and target_pubkey must be different");
return;
}

final Validator sourceValidator = state.getValidators().get(maybeSourceValidatorIndex.get());
final Validator targetValidator = state.getValidators().get(maybeTargetValidatorIndex.get());
final int sourceValidatorIndex = maybeSourceValidatorIndex.get();
final Validator sourceValidator = state.getValidators().get(sourceValidatorIndex);
final int targetValidatorIndex = maybeTargetValidatorIndex.get();
final Validator targetValidator = state.getValidators().get(targetValidatorIndex);

// Verify source withdrawal credentials
final boolean sourceHasExecutionWithdrawalCredentials =
Expand Down Expand Up @@ -487,24 +500,75 @@ private void processConsolidationRequest(
state
.getValidators()
.update(
maybeSourceValidatorIndex.get(),
sourceValidatorIndex,
v -> v.withExitEpoch(exitEpoch).withWithdrawableEpoch(withdrawableEpoch));
LOG.debug(
"process_consolidation_request: updated validator {} with exit_epoch = {}, withdrawable_epoch = {}",
maybeSourceValidatorIndex.get(),
sourceValidatorIndex,
exitEpoch,
withdrawableEpoch);

final PendingConsolidation pendingConsolidation =
new PendingConsolidation(
schemaDefinitionsElectra.getPendingConsolidationSchema(),
SszUInt64.of(UInt64.valueOf(maybeSourceValidatorIndex.get())),
SszUInt64.of(UInt64.valueOf(maybeTargetValidatorIndex.get())));
SszUInt64.of(UInt64.valueOf(sourceValidatorIndex)),
SszUInt64.of(UInt64.valueOf(targetValidatorIndex)));
state.getPendingConsolidations().append(pendingConsolidation);

// Churn any target excess active balance of target and raise its max
if (predicatesElectra.hasEth1WithdrawalCredential(targetValidator)) {
beaconStateMutatorsElectra.switchToCompoundingValidator(state, targetValidatorIndex);
}

LOG.debug("process_consolidation_request: created {}", pendingConsolidation);
}

@Override
public boolean isValidSwitchToCompoundingRequest(
final BeaconState state, final ConsolidationRequest consolidationRequest) {

// Switch to compounding requires source and target be equal
if (!consolidationRequest.getSourcePubkey().equals(consolidationRequest.getTargetPubkey())) {
return false;
}

// Verify source_pubkey exists
final Optional<Integer> maybeSourceValidatorIndex =
validatorsUtil.getValidatorIndex(state, consolidationRequest.getSourcePubkey());
if (maybeSourceValidatorIndex.isEmpty()) {
return false;
}

final int sourceValidatorIndex = maybeSourceValidatorIndex.get();
final Validator sourceValidator = state.getValidators().get(sourceValidatorIndex);

// Verify request has been authorized
final Eth1Address sourceValidatorExecutionAddress =
Predicates.getExecutionAddressUnchecked(sourceValidator.getWithdrawalCredentials());
if (!sourceValidatorExecutionAddress.equals(
Eth1Address.fromBytes(consolidationRequest.getSourceAddress().getWrappedBytes()))) {
return false;
}

// Verify source withdrawal credentials
if (!predicatesElectra.hasEth1WithdrawalCredential(sourceValidator)) {
return false;
}

// Verify the source is active
final UInt64 currentEpoch = miscHelpers.computeEpochAtSlot(state.getSlot());
if (!predicatesElectra.isActiveValidator(sourceValidator, currentEpoch)) {
return false;
}

// Verify exit for source has not been initiated
if (!sourceValidator.getExitEpoch().equals(FAR_FUTURE_EPOCH)) {
return false;
}

return true;
}

@Override
public void applyDeposit(
final MutableBeaconState state,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -184,19 +184,16 @@ public UInt64 computeConsolidationEpochAndUpdateChurn(
* @param index validatorIndex
*/
public void switchToCompoundingValidator(final MutableBeaconStateElectra state, final int index) {
if (PredicatesElectra.isEth1WithdrawalCredential(
state.getValidators().get(index).getWithdrawalCredentials())) {
final byte[] withdrawalCredentialsUpdated =
state.getValidators().get(index).getWithdrawalCredentials().toArray();
withdrawalCredentialsUpdated[0] = COMPOUNDING_WITHDRAWAL_BYTE;
state
.getValidators()
.update(
index,
validator ->
validator.withWithdrawalCredentials(Bytes32.wrap(withdrawalCredentialsUpdated)));
queueExcessActiveBalance(state, index);
}
final byte[] withdrawalCredentialsUpdated =
state.getValidators().get(index).getWithdrawalCredentials().toArray();
withdrawalCredentialsUpdated[0] = COMPOUNDING_WITHDRAWAL_BYTE;
state
.getValidators()
.update(
index,
validator ->
validator.withWithdrawalCredentials(Bytes32.wrap(withdrawalCredentialsUpdated)));
queueExcessActiveBalance(state, index);
}
Comment on lines +187 to 197
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: probably not super important but this is super unsafe if the index doesn't exist, i wonder if we should put a comment in the description
PRE: validator must exist
It's checked up in the process, but this is also public, and we probably want to be super careful where we attempt this...


/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -394,8 +394,6 @@ public void processPendingConsolidations(final MutableBeaconState state) {
break;
}

stateMutatorsElectra.switchToCompoundingValidator(
stateElectra, pendingConsolidation.getTargetIndex());
final UInt64 activeBalance =
stateAccessorsElectra.getActiveBalance(state, pendingConsolidation.getSourceIndex());
beaconStateMutators.decreaseBalance(
Expand Down