From f7c69c92367ceb50fa7a14d66b401758012838dd Mon Sep 17 00:00:00 2001 From: Paul Harris Date: Mon, 16 Dec 2024 10:51:21 +1000 Subject: [PATCH] Implemented expectedGasLimit from spec (#8909) Made a more complete test suite on the FN to show boundaries. Signed-off-by: Paul Harris --- CHANGELOG.md | 4 +- .../BuilderBidValidatorImpl.java | 25 +++++----- .../BuilderBidValidatorTest.java | 49 ++++++++++++++++--- 3 files changed, 57 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6558b740bfb..abd4f911a89 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,4 +11,6 @@ ### Additions and Improvements ### Bug Fixes -- Fixed an issue with the `/eth/v1/config/spec` API not returning all previously included configuration parameters. \ No newline at end of file +- Updated the gas change check for block building so that warnings only get raised if the change is off spec. +- Fixed an issue with the `/eth/v1/config/spec` API not returning all previously included configuration parameters. + diff --git a/ethereum/executionlayer/src/main/java/tech/pegasys/teku/ethereum/executionlayer/BuilderBidValidatorImpl.java b/ethereum/executionlayer/src/main/java/tech/pegasys/teku/ethereum/executionlayer/BuilderBidValidatorImpl.java index ac496f97b71..7c849a4f86a 100644 --- a/ethereum/executionlayer/src/main/java/tech/pegasys/teku/ethereum/executionlayer/BuilderBidValidatorImpl.java +++ b/ethereum/executionlayer/src/main/java/tech/pegasys/teku/ethereum/executionlayer/BuilderBidValidatorImpl.java @@ -122,20 +122,21 @@ public void validateBuilderBid( final UInt64 preferredGasLimit = validatorRegistration.getGasLimit(); final UInt64 proposedGasLimit = executionPayloadHeader.getGasLimit(); - if (parentGasLimit.equals(preferredGasLimit) && proposedGasLimit.equals(parentGasLimit)) { - return; - } - - if (preferredGasLimit.isGreaterThan(parentGasLimit) - && proposedGasLimit.isGreaterThan(parentGasLimit)) { - return; + final UInt64 expectedGasLimit = expectedGasLimit(parentGasLimit, preferredGasLimit); + if (!expectedGasLimit.equals(preferredGasLimit)) { + eventLogger.builderBidNotHonouringGasLimit( + parentGasLimit, proposedGasLimit, preferredGasLimit); } + } - if (preferredGasLimit.isLessThan(parentGasLimit) - && proposedGasLimit.isLessThan(parentGasLimit)) { - return; + static UInt64 expectedGasLimit(final UInt64 parentGasLimit, final UInt64 targetGasLimit) { + final UInt64 maxGasLimitDifference = parentGasLimit.dividedBy(1024L).minusMinZero(1); + if (targetGasLimit.isGreaterThan(parentGasLimit)) { + final UInt64 gasDiff = targetGasLimit.minusMinZero(parentGasLimit); + return parentGasLimit.plus(maxGasLimitDifference.min(gasDiff)); + } else { + final UInt64 gasDiff = parentGasLimit.minusMinZero(targetGasLimit); + return parentGasLimit.minusMinZero(maxGasLimitDifference.min(gasDiff)); } - - eventLogger.builderBidNotHonouringGasLimit(parentGasLimit, proposedGasLimit, preferredGasLimit); } } diff --git a/ethereum/executionlayer/src/test/java/tech/pegasys/teku/ethereum/executionlayer/BuilderBidValidatorTest.java b/ethereum/executionlayer/src/test/java/tech/pegasys/teku/ethereum/executionlayer/BuilderBidValidatorTest.java index c858ff89295..c77ea99efa8 100644 --- a/ethereum/executionlayer/src/test/java/tech/pegasys/teku/ethereum/executionlayer/BuilderBidValidatorTest.java +++ b/ethereum/executionlayer/src/test/java/tech/pegasys/teku/ethereum/executionlayer/BuilderBidValidatorTest.java @@ -13,6 +13,7 @@ package tech.pegasys.teku.ethereum.executionlayer; +import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.doNothing; @@ -24,12 +25,16 @@ import static tech.pegasys.teku.spec.schemas.ApiSchemas.VALIDATOR_REGISTRATION_SCHEMA; import java.util.Optional; +import java.util.stream.Stream; import org.apache.tuweni.bytes.Bytes; import org.apache.tuweni.bytes.Bytes32; import org.apache.tuweni.units.bigints.UInt256; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; import tech.pegasys.teku.bls.BLS; import tech.pegasys.teku.bls.BLSConstants; import tech.pegasys.teku.bls.BLSKeyPair; @@ -148,10 +153,21 @@ void shouldBeInvalidIfLocalPayloadWithdrawalsRootDoesNotMatchTheRootOfTheBid() { .orElseThrow()); } + @ParameterizedTest(name = "parent.{0}_target.{1}_result.{2}") + @MethodSource("expectedGasLimitPermutations") + void expectedGasLimitTestCases( + final long parentGasLimit, final long targetGasLimit, final long expectedGasLimit) { + final UInt64 computedGasLimit = + BuilderBidValidatorImpl.expectedGasLimit( + UInt64.valueOf(parentGasLimit), UInt64.valueOf(targetGasLimit)); + + assertThat(computedGasLimit).isEqualTo(UInt64.valueOf(expectedGasLimit)); + } + @Test void shouldNotLogEventIfGasLimitDecreases() throws BuilderBidValidationException { - - prepareGasLimit(UInt64.valueOf(1024_000), UInt64.valueOf(1022_000), UInt64.valueOf(1023_000)); + // 1023001 is as high as it can move in 1 shot + prepareGasLimit(UInt64.valueOf(1024_000), UInt64.valueOf(1022_000), UInt64.valueOf(1023_001)); builderBidValidatorWithMockSpec.validateBuilderBid( signedBuilderBid, validatorRegistration, state, Optional.empty()); @@ -161,8 +177,8 @@ void shouldNotLogEventIfGasLimitDecreases() throws BuilderBidValidationException @Test void shouldNotLogEventIfGasLimitIncreases() throws BuilderBidValidationException { - - prepareGasLimit(UInt64.valueOf(1024_000), UInt64.valueOf(1025_000), UInt64.valueOf(2048_000)); + // 1024999 is as high as it can move in 1 shot + prepareGasLimit(UInt64.valueOf(1024_000), UInt64.valueOf(1025_000), UInt64.valueOf(1024_999)); builderBidValidatorWithMockSpec.validateBuilderBid( signedBuilderBid, validatorRegistration, state, Optional.empty()); @@ -184,27 +200,44 @@ void shouldNotLogEventIfGasLimitStaysTheSame() throws BuilderBidValidationExcept @Test void shouldLogEventIfGasLimitDoesNotDecrease() throws BuilderBidValidationException { - prepareGasLimit(UInt64.valueOf(1024_000), UInt64.valueOf(1024_000), UInt64.valueOf(1023_100)); + prepareGasLimit(UInt64.valueOf(1024_000), UInt64.valueOf(1024_000), UInt64.valueOf(1020_000)); builderBidValidatorWithMockSpec.validateBuilderBid( signedBuilderBid, validatorRegistration, state, Optional.empty()); verify(eventLogger) .builderBidNotHonouringGasLimit( - UInt64.valueOf(1024_000), UInt64.valueOf(1024_000), UInt64.valueOf(1023_100)); + UInt64.valueOf(1024_000), UInt64.valueOf(1024_000), UInt64.valueOf(1020_000)); } @Test void shouldLogEventIfGasLimitDoesNotIncrease() throws BuilderBidValidationException { - prepareGasLimit(UInt64.valueOf(1024_000), UInt64.valueOf(1020_000), UInt64.valueOf(1024_100)); + prepareGasLimit(UInt64.valueOf(1024_000), UInt64.valueOf(1020_000), UInt64.valueOf(1028_000)); builderBidValidatorWithMockSpec.validateBuilderBid( signedBuilderBid, validatorRegistration, state, Optional.empty()); verify(eventLogger) .builderBidNotHonouringGasLimit( - UInt64.valueOf(1024_000), UInt64.valueOf(1020_000), UInt64.valueOf(1024_100)); + UInt64.valueOf(1024_000), UInt64.valueOf(1020_000), UInt64.valueOf(1028_000)); + } + + static Stream expectedGasLimitPermutations() { + return Stream.of( + // same, expect no change + Arguments.of(36_000_000L, 36_000_000L, 36_000_000L), + Arguments.of(1024_000L, 1024_000L, 1024_000L), + // down inside bounds + Arguments.of(1024_000L, 1023_500L, 1023_500L), + // down outside bounds - results in a partial move + Arguments.of(36_000_000L, 35_000_000L, 35_964_845L), + Arguments.of(1024_000L, 1020_000L, 1023_001L), + // increase outside bounds - results in a partial move + Arguments.of(1024_000L, 1025_000L, 1024_999L), + Arguments.of(30_000_000L, 36_000_000L, 30_029_295L), + // increase inside bounds + Arguments.of(1024_000L, 1024_500L, 1024_500L)); } private void prepareValidSignedBuilderBid() {