From 145b1fda07eb8b7cc22d49284035bfe8cc61122c Mon Sep 17 00:00:00 2001 From: Paul Harris Date: Tue, 21 Jan 2025 11:34:06 +1000 Subject: [PATCH 1/4] Added block production restrictions for electra voluntary exits should not be included in a block if - they have a withdrawal pending (thats invalid) - they have a pending deposit (looking to make that invalid) The pending withdrawal was pointed out in devnet5, so this resolves that. The pending deposit is less cheap than i'd like ,but im fairly sure it'd potentially lead to some bad state data that we don't really want (eg. potentially depositing onto a withdrawn validator, so a withdrawn validator ending up with non 0 balance). Signed-off-by: Paul Harris --- .../BlockOperationSelectorFactory.java | 32 ++++++- .../BlockOperationSelectorFactoryTest.java | 84 +++++++++++++++++++ .../teku/spec/util/DataStructureUtil.java | 9 ++ 3 files changed, 124 insertions(+), 1 deletion(-) diff --git a/beacon/validator/src/main/java/tech/pegasys/teku/validator/coordinator/BlockOperationSelectorFactory.java b/beacon/validator/src/main/java/tech/pegasys/teku/validator/coordinator/BlockOperationSelectorFactory.java index 7eff3b34f13..afaab3389bb 100644 --- a/beacon/validator/src/main/java/tech/pegasys/teku/validator/coordinator/BlockOperationSelectorFactory.java +++ b/beacon/validator/src/main/java/tech/pegasys/teku/validator/coordinator/BlockOperationSelectorFactory.java @@ -57,6 +57,8 @@ import tech.pegasys.teku.spec.datastructures.operations.SignedVoluntaryExit; import tech.pegasys.teku.spec.datastructures.state.beaconstate.BeaconState; import tech.pegasys.teku.spec.datastructures.state.beaconstate.BeaconStateCache; +import tech.pegasys.teku.spec.datastructures.state.beaconstate.versions.electra.BeaconStateElectra; +import tech.pegasys.teku.spec.datastructures.state.versions.electra.PendingPartialWithdrawal; import tech.pegasys.teku.spec.datastructures.type.SszKZGCommitment; import tech.pegasys.teku.spec.datastructures.type.SszKZGProof; import tech.pegasys.teku.spec.executionlayer.ExecutionLayerBlockProductionManager; @@ -146,7 +148,7 @@ public Function> createSelector( final SszList voluntaryExits = voluntaryExitPool.getItemsForBlock( blockSlotState, - exit -> !exitedValidators.contains(exit.getMessage().getValidatorIndex()), + exit -> voluntaryExitPredicate(blockSlotState, exitedValidators, exit), exit -> exitedValidators.add(exit.getMessage().getValidatorIndex())); bodyBuilder @@ -204,6 +206,34 @@ public Function> createSelector( }; } + private boolean voluntaryExitPredicate( + final BeaconState blockSlotState, + final Set exitedValidators, + final SignedVoluntaryExit exit) { + final UInt64 validatorIndex = exit.getMessage().getValidatorIndex(); + if (exitedValidators.contains(validatorIndex)) { + return false; + } + final Optional electraState = blockSlotState.toVersionElectra(); + // if there is a pending withdrawal, the exit is not valid for inclusion in a block. + if (electraState.isPresent()) { + if (electraState.get().getPendingPartialWithdrawals().stream() + .map(PendingPartialWithdrawal::getValidatorIndex) + .anyMatch(i -> i.equals(exit.getValidatorId()))) { + return false; + } + // if there is a pending deposit in the state, then it should not be included in a block. + if (electraState.get().getPendingDeposits().stream() + .map(p -> spec.getValidatorIndex(blockSlotState, p.getPublicKey())) + .filter(Optional::isPresent) + .map(Optional::get) + .anyMatch(i -> i.equals(exit.getValidatorId()))) { + return false; + } + } + return true; + } + private SafeFuture setExecutionData( final Optional executionPayloadContext, final BeaconBlockBodyBuilder bodyBuilder, diff --git a/beacon/validator/src/test/java/tech/pegasys/teku/validator/coordinator/BlockOperationSelectorFactoryTest.java b/beacon/validator/src/test/java/tech/pegasys/teku/validator/coordinator/BlockOperationSelectorFactoryTest.java index 35c58e2a331..a0732a58182 100644 --- a/beacon/validator/src/test/java/tech/pegasys/teku/validator/coordinator/BlockOperationSelectorFactoryTest.java +++ b/beacon/validator/src/test/java/tech/pegasys/teku/validator/coordinator/BlockOperationSelectorFactoryTest.java @@ -42,6 +42,8 @@ import tech.pegasys.teku.infrastructure.metrics.StubMetricsSystem; import tech.pegasys.teku.infrastructure.ssz.SszData; import tech.pegasys.teku.infrastructure.ssz.SszList; +import tech.pegasys.teku.infrastructure.ssz.primitive.SszBytes32; +import tech.pegasys.teku.infrastructure.ssz.primitive.SszUInt64; import tech.pegasys.teku.infrastructure.unsigned.UInt64; import tech.pegasys.teku.spec.Spec; import tech.pegasys.teku.spec.TestSpecFactory; @@ -76,10 +78,15 @@ import tech.pegasys.teku.spec.datastructures.operations.SignedBlsToExecutionChange; import tech.pegasys.teku.spec.datastructures.operations.SignedVoluntaryExit; import tech.pegasys.teku.spec.datastructures.operations.versions.altair.SignedContributionAndProof; +import tech.pegasys.teku.spec.datastructures.state.Validator; import tech.pegasys.teku.spec.datastructures.state.beaconstate.BeaconState; import tech.pegasys.teku.spec.datastructures.state.beaconstate.BeaconStateCache; +import tech.pegasys.teku.spec.datastructures.state.beaconstate.versions.electra.MutableBeaconStateElectra; +import tech.pegasys.teku.spec.datastructures.state.versions.electra.PendingDeposit; import tech.pegasys.teku.spec.datastructures.type.SszKZGCommitment; import tech.pegasys.teku.spec.datastructures.type.SszKZGProof; +import tech.pegasys.teku.spec.datastructures.type.SszPublicKey; +import tech.pegasys.teku.spec.datastructures.type.SszSignature; import tech.pegasys.teku.spec.executionlayer.ExecutionLayerBlockProductionManager; import tech.pegasys.teku.spec.logic.versions.capella.operations.validation.BlsToExecutionChangesValidator.BlsToExecutionChangeInvalidReason; import tech.pegasys.teku.spec.logic.versions.deneb.helpers.MiscHelpersDeneb; @@ -89,6 +96,7 @@ import tech.pegasys.teku.spec.schemas.SchemaDefinitions; import tech.pegasys.teku.spec.schemas.SchemaDefinitionsBellatrix; import tech.pegasys.teku.spec.schemas.SchemaDefinitionsDeneb; +import tech.pegasys.teku.spec.schemas.SchemaDefinitionsElectra; import tech.pegasys.teku.spec.util.DataStructureUtil; import tech.pegasys.teku.statetransition.OperationPool; import tech.pegasys.teku.statetransition.SimpleOperationPool; @@ -264,6 +272,82 @@ void shouldNotSelectOperationsWhenNoneAreAvailable() { assertThat(bodyBuilder.blsToExecutionChanges).isEmpty(); } + @Test + void shouldNotSelectVoluntaryExitWhenValidatorHasPendingWithdrawal() { + final UInt64 slot = UInt64.ONE; + final MutableBeaconStateElectra blockSlotState = + dataStructureUtil.randomBeaconState(slot).toVersionElectra().get().createWritableCopy(); + blockSlotState + .getPendingPartialWithdrawals() + .append(dataStructureUtil.randomPendingPartialWithdrawal(1)); + final SignedVoluntaryExit voluntaryExit = + dataStructureUtil.randomSignedVoluntaryExit(UInt64.valueOf(1)); + final ExecutionPayload randomExecutionPayload = dataStructureUtil.randomExecutionPayload(); + final UInt256 blockExecutionValue = dataStructureUtil.randomUInt256(); + + addToPool(voluntaryExitPool, voluntaryExit); + prepareBlockProductionWithPayload( + randomExecutionPayload, + executionPayloadContext, + blockSlotState, + Optional.of(blockExecutionValue)); + + safeJoin( + factory + .createSelector( + parentRoot, + blockSlotState, + randaoReveal, + Optional.of(defaultGraffiti), + Optional.empty(), + BlockProductionPerformance.NOOP) + .apply(bodyBuilder)); + assertThat(bodyBuilder.voluntaryExits).isEmpty(); + } + + @Test + void shouldNotSelectVoluntaryExitWhenValidatorHasPendingDeposit() { + final UInt64 slot = UInt64.ONE; + final MutableBeaconStateElectra blockSlotState = + dataStructureUtil.randomBeaconState(slot).toVersionElectra().get().createWritableCopy(); + final SchemaDefinitionsElectra schemaDefinitionsElectra = + spec.getGenesisSchemaDefinitions().toVersionElectra().get(); + final Validator validator = blockSlotState.getValidators().get(1); + final PendingDeposit deposit = + schemaDefinitionsElectra + .getPendingDepositSchema() + .create( + new SszPublicKey(validator.getPublicKey().toBytesCompressed()), + SszBytes32.of(validator.getWithdrawalCredentials()), + SszUInt64.of(UInt64.ONE), + new SszSignature(dataStructureUtil.randomSignature()), + SszUInt64.of(dataStructureUtil.randomUInt64())); + blockSlotState.getPendingDeposits().append(deposit); + final SignedVoluntaryExit voluntaryExit = + dataStructureUtil.randomSignedVoluntaryExit(UInt64.valueOf(1)); + final ExecutionPayload randomExecutionPayload = dataStructureUtil.randomExecutionPayload(); + final UInt256 blockExecutionValue = dataStructureUtil.randomUInt256(); + + addToPool(voluntaryExitPool, voluntaryExit); + prepareBlockProductionWithPayload( + randomExecutionPayload, + executionPayloadContext, + blockSlotState, + Optional.of(blockExecutionValue)); + + safeJoin( + factory + .createSelector( + parentRoot, + blockSlotState, + randaoReveal, + Optional.of(defaultGraffiti), + Optional.empty(), + BlockProductionPerformance.NOOP) + .apply(bodyBuilder)); + assertThat(bodyBuilder.voluntaryExits).isEmpty(); + } + @Test void shouldIncludeValidOperations() { final UInt64 slot = UInt64.valueOf(2); diff --git a/ethereum/spec/src/testFixtures/java/tech/pegasys/teku/spec/util/DataStructureUtil.java b/ethereum/spec/src/testFixtures/java/tech/pegasys/teku/spec/util/DataStructureUtil.java index 7e312c58cf7..13349943cf1 100644 --- a/ethereum/spec/src/testFixtures/java/tech/pegasys/teku/spec/util/DataStructureUtil.java +++ b/ethereum/spec/src/testFixtures/java/tech/pegasys/teku/spec/util/DataStructureUtil.java @@ -2641,6 +2641,15 @@ public PendingPartialWithdrawal randomPendingPartialWithdrawal() { SszUInt64.of(randomUInt64())); } + public PendingPartialWithdrawal randomPendingPartialWithdrawal(final long validatorIndex) { + return getElectraSchemaDefinitions(randomSlot()) + .getPendingPartialWithdrawalSchema() + .create( + SszUInt64.of(UInt64.valueOf(validatorIndex)), + SszUInt64.of(randomUInt64()), + SszUInt64.of(randomUInt64())); + } + public UInt64 randomBlobSidecarIndex() { return randomUInt64( spec.forMilestone(spec.getForkSchedule().getHighestSupportedMilestone()) From 6134a7af174bd7c48c6c7c1b6fd4f2c7af1dcb74 Mon Sep 17 00:00:00 2001 From: Paul Harris Date: Tue, 21 Jan 2025 15:25:16 +1000 Subject: [PATCH 2/4] nothing appeared broken when electra got enabled, it should probably be on. Signed-off-by: Paul Harris --- .../tech/pegasys/teku/ethtests/finder/ReferenceTestFinder.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/eth-tests/src/main/java/tech/pegasys/teku/ethtests/finder/ReferenceTestFinder.java b/eth-tests/src/main/java/tech/pegasys/teku/ethtests/finder/ReferenceTestFinder.java index 4dbcf5ac10a..d58be7186fa 100644 --- a/eth-tests/src/main/java/tech/pegasys/teku/ethtests/finder/ReferenceTestFinder.java +++ b/eth-tests/src/main/java/tech/pegasys/teku/ethtests/finder/ReferenceTestFinder.java @@ -35,7 +35,8 @@ public class ReferenceTestFinder { TestFork.ALTAIR, TestFork.BELLATRIX, TestFork.CAPELLA, - TestFork.DENEB); // TODO: Add Electra fork tests back + TestFork.DENEB, + TestFork.ELECTRA); @MustBeClosed public static Stream findReferenceTests() throws IOException { From a51615fa4f9327401dfd4c211890526f122b3495 Mon Sep 17 00:00:00 2001 From: Paul Harris Date: Tue, 21 Jan 2025 18:40:15 +1000 Subject: [PATCH 3/4] remove the deposits check as i was convinced that it's not going to be a problem. Signed-off-by: Paul Harris --- .../BlockOperationSelectorFactory.java | 8 --- .../BlockOperationSelectorFactoryTest.java | 50 ------------------- 2 files changed, 58 deletions(-) diff --git a/beacon/validator/src/main/java/tech/pegasys/teku/validator/coordinator/BlockOperationSelectorFactory.java b/beacon/validator/src/main/java/tech/pegasys/teku/validator/coordinator/BlockOperationSelectorFactory.java index afaab3389bb..9f13b8035a0 100644 --- a/beacon/validator/src/main/java/tech/pegasys/teku/validator/coordinator/BlockOperationSelectorFactory.java +++ b/beacon/validator/src/main/java/tech/pegasys/teku/validator/coordinator/BlockOperationSelectorFactory.java @@ -222,14 +222,6 @@ private boolean voluntaryExitPredicate( .anyMatch(i -> i.equals(exit.getValidatorId()))) { return false; } - // if there is a pending deposit in the state, then it should not be included in a block. - if (electraState.get().getPendingDeposits().stream() - .map(p -> spec.getValidatorIndex(blockSlotState, p.getPublicKey())) - .filter(Optional::isPresent) - .map(Optional::get) - .anyMatch(i -> i.equals(exit.getValidatorId()))) { - return false; - } } return true; } diff --git a/beacon/validator/src/test/java/tech/pegasys/teku/validator/coordinator/BlockOperationSelectorFactoryTest.java b/beacon/validator/src/test/java/tech/pegasys/teku/validator/coordinator/BlockOperationSelectorFactoryTest.java index a0732a58182..331e4ac3832 100644 --- a/beacon/validator/src/test/java/tech/pegasys/teku/validator/coordinator/BlockOperationSelectorFactoryTest.java +++ b/beacon/validator/src/test/java/tech/pegasys/teku/validator/coordinator/BlockOperationSelectorFactoryTest.java @@ -42,8 +42,6 @@ import tech.pegasys.teku.infrastructure.metrics.StubMetricsSystem; import tech.pegasys.teku.infrastructure.ssz.SszData; import tech.pegasys.teku.infrastructure.ssz.SszList; -import tech.pegasys.teku.infrastructure.ssz.primitive.SszBytes32; -import tech.pegasys.teku.infrastructure.ssz.primitive.SszUInt64; import tech.pegasys.teku.infrastructure.unsigned.UInt64; import tech.pegasys.teku.spec.Spec; import tech.pegasys.teku.spec.TestSpecFactory; @@ -78,15 +76,11 @@ import tech.pegasys.teku.spec.datastructures.operations.SignedBlsToExecutionChange; import tech.pegasys.teku.spec.datastructures.operations.SignedVoluntaryExit; import tech.pegasys.teku.spec.datastructures.operations.versions.altair.SignedContributionAndProof; -import tech.pegasys.teku.spec.datastructures.state.Validator; import tech.pegasys.teku.spec.datastructures.state.beaconstate.BeaconState; import tech.pegasys.teku.spec.datastructures.state.beaconstate.BeaconStateCache; import tech.pegasys.teku.spec.datastructures.state.beaconstate.versions.electra.MutableBeaconStateElectra; -import tech.pegasys.teku.spec.datastructures.state.versions.electra.PendingDeposit; import tech.pegasys.teku.spec.datastructures.type.SszKZGCommitment; import tech.pegasys.teku.spec.datastructures.type.SszKZGProof; -import tech.pegasys.teku.spec.datastructures.type.SszPublicKey; -import tech.pegasys.teku.spec.datastructures.type.SszSignature; import tech.pegasys.teku.spec.executionlayer.ExecutionLayerBlockProductionManager; import tech.pegasys.teku.spec.logic.versions.capella.operations.validation.BlsToExecutionChangesValidator.BlsToExecutionChangeInvalidReason; import tech.pegasys.teku.spec.logic.versions.deneb.helpers.MiscHelpersDeneb; @@ -96,7 +90,6 @@ import tech.pegasys.teku.spec.schemas.SchemaDefinitions; import tech.pegasys.teku.spec.schemas.SchemaDefinitionsBellatrix; import tech.pegasys.teku.spec.schemas.SchemaDefinitionsDeneb; -import tech.pegasys.teku.spec.schemas.SchemaDefinitionsElectra; import tech.pegasys.teku.spec.util.DataStructureUtil; import tech.pegasys.teku.statetransition.OperationPool; import tech.pegasys.teku.statetransition.SimpleOperationPool; @@ -305,49 +298,6 @@ void shouldNotSelectVoluntaryExitWhenValidatorHasPendingWithdrawal() { assertThat(bodyBuilder.voluntaryExits).isEmpty(); } - @Test - void shouldNotSelectVoluntaryExitWhenValidatorHasPendingDeposit() { - final UInt64 slot = UInt64.ONE; - final MutableBeaconStateElectra blockSlotState = - dataStructureUtil.randomBeaconState(slot).toVersionElectra().get().createWritableCopy(); - final SchemaDefinitionsElectra schemaDefinitionsElectra = - spec.getGenesisSchemaDefinitions().toVersionElectra().get(); - final Validator validator = blockSlotState.getValidators().get(1); - final PendingDeposit deposit = - schemaDefinitionsElectra - .getPendingDepositSchema() - .create( - new SszPublicKey(validator.getPublicKey().toBytesCompressed()), - SszBytes32.of(validator.getWithdrawalCredentials()), - SszUInt64.of(UInt64.ONE), - new SszSignature(dataStructureUtil.randomSignature()), - SszUInt64.of(dataStructureUtil.randomUInt64())); - blockSlotState.getPendingDeposits().append(deposit); - final SignedVoluntaryExit voluntaryExit = - dataStructureUtil.randomSignedVoluntaryExit(UInt64.valueOf(1)); - final ExecutionPayload randomExecutionPayload = dataStructureUtil.randomExecutionPayload(); - final UInt256 blockExecutionValue = dataStructureUtil.randomUInt256(); - - addToPool(voluntaryExitPool, voluntaryExit); - prepareBlockProductionWithPayload( - randomExecutionPayload, - executionPayloadContext, - blockSlotState, - Optional.of(blockExecutionValue)); - - safeJoin( - factory - .createSelector( - parentRoot, - blockSlotState, - randaoReveal, - Optional.of(defaultGraffiti), - Optional.empty(), - BlockProductionPerformance.NOOP) - .apply(bodyBuilder)); - assertThat(bodyBuilder.voluntaryExits).isEmpty(); - } - @Test void shouldIncludeValidOperations() { final UInt64 slot = UInt64.valueOf(2); From 097415c3968cb6eaae729d034dfdf1587517b45e Mon Sep 17 00:00:00 2001 From: Enrico Del Fante Date: Tue, 21 Jan 2025 13:10:54 +0100 Subject: [PATCH 4/4] simplification --- .../BlockOperationSelectorFactory.java | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/beacon/validator/src/main/java/tech/pegasys/teku/validator/coordinator/BlockOperationSelectorFactory.java b/beacon/validator/src/main/java/tech/pegasys/teku/validator/coordinator/BlockOperationSelectorFactory.java index 9f13b8035a0..6b71266f7bf 100644 --- a/beacon/validator/src/main/java/tech/pegasys/teku/validator/coordinator/BlockOperationSelectorFactory.java +++ b/beacon/validator/src/main/java/tech/pegasys/teku/validator/coordinator/BlockOperationSelectorFactory.java @@ -57,7 +57,6 @@ import tech.pegasys.teku.spec.datastructures.operations.SignedVoluntaryExit; import tech.pegasys.teku.spec.datastructures.state.beaconstate.BeaconState; import tech.pegasys.teku.spec.datastructures.state.beaconstate.BeaconStateCache; -import tech.pegasys.teku.spec.datastructures.state.beaconstate.versions.electra.BeaconStateElectra; import tech.pegasys.teku.spec.datastructures.state.versions.electra.PendingPartialWithdrawal; import tech.pegasys.teku.spec.datastructures.type.SszKZGCommitment; import tech.pegasys.teku.spec.datastructures.type.SszKZGProof; @@ -214,16 +213,15 @@ private boolean voluntaryExitPredicate( if (exitedValidators.contains(validatorIndex)) { return false; } - final Optional electraState = blockSlotState.toVersionElectra(); // if there is a pending withdrawal, the exit is not valid for inclusion in a block. - if (electraState.isPresent()) { - if (electraState.get().getPendingPartialWithdrawals().stream() - .map(PendingPartialWithdrawal::getValidatorIndex) - .anyMatch(i -> i.equals(exit.getValidatorId()))) { - return false; - } - } - return true; + return blockSlotState + .toVersionElectra() + .map( + beaconStateElectra -> + beaconStateElectra.getPendingPartialWithdrawals().stream() + .map(PendingPartialWithdrawal::getValidatorIndex) + .noneMatch(index -> index == validatorIndex.intValue())) + .orElse(true); } private SafeFuture setExecutionData(