Skip to content

[Engine-API] Exclude empty requests in requests list #8859

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Nov 28, 2024
Merged

[Engine-API] Exclude empty requests in requests list #8859

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions ...ech/pegasys/teku/spec/datastructures/execution/versions/electra/ConsolidationRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

package tech.pegasys.teku.spec.datastructures.execution.versions.electra;

import org.apache.tuweni.bytes.Bytes;
import tech.pegasys.teku.bls.BLSPublicKey;
import tech.pegasys.teku.infrastructure.bytes.Bytes20;
import tech.pegasys.teku.infrastructure.ssz.collections.SszByteVector;
Expand All @@ -25,6 +26,7 @@ public class ConsolidationRequest
extends Container3<ConsolidationRequest, SszByteVector, SszPublicKey, SszPublicKey> {

public static final byte REQUEST_TYPE = 0x2;
public static final Bytes REQUEST_TYPE_PREFIX = Bytes.of(REQUEST_TYPE);

protected ConsolidationRequest(
final ConsolidationRequestSchema schema,
Expand Down
2 changes: 2 additions & 0 deletions ...java/tech/pegasys/teku/spec/datastructures/execution/versions/electra/DepositRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

package tech.pegasys.teku.spec.datastructures.execution.versions.electra;

import org.apache.tuweni.bytes.Bytes;
import org.apache.tuweni.bytes.Bytes32;
import tech.pegasys.teku.bls.BLSPublicKey;
import tech.pegasys.teku.bls.BLSSignature;
Expand All @@ -30,6 +31,7 @@ public class DepositRequest
DepositRequest, SszPublicKey, SszBytes32, SszUInt64, SszSignature, SszUInt64> {

public static final byte REQUEST_TYPE = 0x0;
public static final Bytes REQUEST_TYPE_PREFIX = Bytes.of(REQUEST_TYPE);

DepositRequest(
final DepositRequestSchema schema,
Expand Down
90 changes: 53 additions & 37 deletions ...gasys/teku/spec/datastructures/execution/versions/electra/ExecutionRequestsDataCodec.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,18 +13,16 @@

package tech.pegasys.teku.spec.datastructures.execution.versions.electra;

import java.util.ArrayList;
import java.util.List;
import org.apache.tuweni.bytes.Bytes;
import tech.pegasys.teku.infrastructure.ssz.SszList;
import tech.pegasys.teku.spec.datastructures.execution.ExecutionRequestsBuilder;

/*
Implement the rules for decoding and hashing execution requests according to https://eips.ethereum.org/EIPS/eip-7685
*/
public class ExecutionRequestsDataCodec {

private static final int EXPECTED_REQUEST_DATA_ELEMENTS = 3;

private final ExecutionRequestsSchema executionRequestsSchema;

public ExecutionRequestsDataCodec(final ExecutionRequestsSchema executionRequestsSchema) {
Expand All @@ -34,44 +32,47 @@ public ExecutionRequestsDataCodec(final ExecutionRequestsSchema executionRequest
/**
* Decodes the execution requests received from the EL.
*
* @param executionRequestData list of encoded execution requests from the EL
* @param executionRequests list of encoded execution requests from the EL
* @return an ExecutionRequests object with the requests
*/
public ExecutionRequests decode(final List<Bytes> executionRequestData) {
if (executionRequestData.size() != EXPECTED_REQUEST_DATA_ELEMENTS) {
throw new IllegalArgumentException(
"Invalid number of execution request data elements: expected "
+ EXPECTED_REQUEST_DATA_ELEMENTS
+ ", received "
+ executionRequestData.size());
}

public ExecutionRequests decode(final List<Bytes> executionRequests) {
final ExecutionRequestsBuilder executionRequestsBuilder =
new ExecutionRequestsBuilderElectra(executionRequestsSchema);

for (int index = 0; index < executionRequestData.size(); index++) {
// The request type is implicitly defined as the index of the element in executionRequestData
switch ((byte) index) {
byte previousRequestType = -1;
for (final Bytes request : executionRequests) {
if (request.isEmpty()) {
throw new IllegalArgumentException("Execution request data must not be empty");
}
final byte requestType = request.get(0);
if (requestType <= previousRequestType) {
throw new IllegalArgumentException(
"Execution requests are not in strictly ascending order");
}
final Bytes requestData = request.slice(1);
Comment on lines +44 to +52
Copy link
Contributor

Choose a reason for hiding this comment

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

This will allow requests with empty request data.

For example, this will pass when it should throw an exception.

  @Test
  public void decodeExecutionRequestsDataWithEmptyRequestData() {
    final ExecutionRequests executionRequests =
        codec.decode(List.of(Bytes.of(DepositRequest.REQUEST_TYPE)));
    assertThat(executionRequests.getDeposits()).isEmpty();
  }

The spec is a little unclear about this distinction.

  • The first assert checks that the request type (request[0:1]) is valid.
    • If the request is empty, the type will be b"" and this will throw.
  • The second assert ensures the request data (request[1:]) is not empty.
request_type, request_data = request[0:1], request[1:]

# Check that the request type is valid
assert request_type in request_types
# Check that the request data is not empty
assert len(request_data) != 0

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @jtraglia !

Copy link
Member

Choose a reason for hiding this comment

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

Fixed on #8880

switch (requestType) {
case DepositRequest.REQUEST_TYPE ->
executionRequestsBuilder.deposits(
executionRequestsSchema
.getDepositRequestsSchema()
.sszDeserialize(executionRequestData.get(index))
.sszDeserialize(requestData)
.asList());
case WithdrawalRequest.REQUEST_TYPE ->
executionRequestsBuilder.withdrawals(
executionRequestsSchema
.getWithdrawalRequestsSchema()
.sszDeserialize(executionRequestData.get(index))
.sszDeserialize(requestData)
.asList());
case ConsolidationRequest.REQUEST_TYPE ->
executionRequestsBuilder.consolidations(
executionRequestsSchema
.getConsolidationRequestsSchema()
.sszDeserialize(executionRequestData.get(index))
.sszDeserialize(requestData)
.asList());
default -> throw new IllegalArgumentException("Invalid execution request type: " + index);
default ->
throw new IllegalArgumentException("Invalid execution request type: " + requestType);
}
previousRequestType = requestType;
}

return executionRequestsBuilder.build();
Expand All @@ -84,22 +85,37 @@ public ExecutionRequests decode(final List<Bytes> executionRequestData) {
* @return list of encoded execution requests
*/
public List<Bytes> encode(final ExecutionRequests executionRequests) {
final SszList<DepositRequest> depositRequestsSszList =
executionRequestsSchema
.getDepositRequestsSchema()
.createFromElements(executionRequests.getDeposits());
final SszList<WithdrawalRequest> withdrawalRequestsSszList =
executionRequestsSchema
.getWithdrawalRequestsSchema()
.createFromElements(executionRequests.getWithdrawals());
final SszList<ConsolidationRequest> consolidationRequestsSszList =
executionRequestsSchema
.getConsolidationRequestsSchema()
.createFromElements(executionRequests.getConsolidations());

return List.of(
depositRequestsSszList.sszSerialize(),
withdrawalRequestsSszList.sszSerialize(),
consolidationRequestsSszList.sszSerialize());
final List<Bytes> executionRequestsData = new ArrayList<>();
final List<DepositRequest> deposits = executionRequests.getDeposits();
if (!deposits.isEmpty()) {
final Bytes depositRequestsData =
executionRequestsSchema
.getDepositRequestsSchema()
.createFromElements(deposits)
.sszSerialize();
executionRequestsData.add(
Bytes.concatenate(DepositRequest.REQUEST_TYPE_PREFIX, depositRequestsData));
}
final List<WithdrawalRequest> withdrawals = executionRequests.getWithdrawals();
if (!withdrawals.isEmpty()) {
final Bytes withdrawalsRequestsData =
executionRequestsSchema
.getWithdrawalRequestsSchema()
.createFromElements(withdrawals)
.sszSerialize();
executionRequestsData.add(
Bytes.concatenate(WithdrawalRequest.REQUEST_TYPE_PREFIX, withdrawalsRequestsData));
}
final List<ConsolidationRequest> consolidations = executionRequests.getConsolidations();
if (!consolidations.isEmpty()) {
final Bytes consolidationRequestsData =
executionRequestsSchema
.getConsolidationRequestsSchema()
.createFromElements(consolidations)
.sszSerialize();
executionRequestsData.add(
Bytes.concatenate(ConsolidationRequest.REQUEST_TYPE_PREFIX, consolidationRequestsData));
}
return executionRequestsData;
}
}
2 changes: 2 additions & 0 deletions ...a/tech/pegasys/teku/spec/datastructures/execution/versions/electra/WithdrawalRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

package tech.pegasys.teku.spec.datastructures.execution.versions.electra;

import org.apache.tuweni.bytes.Bytes;
import tech.pegasys.teku.bls.BLSPublicKey;
import tech.pegasys.teku.infrastructure.bytes.Bytes20;
import tech.pegasys.teku.infrastructure.ssz.collections.SszByteVector;
Expand All @@ -27,6 +28,7 @@ public class WithdrawalRequest
extends Container3<WithdrawalRequest, SszByteVector, SszPublicKey, SszUInt64> {

public static final byte REQUEST_TYPE = 0x1;
public static final Bytes REQUEST_TYPE_PREFIX = Bytes.of(REQUEST_TYPE);

protected WithdrawalRequest(
final WithdrawalRequestSchema schema,
Expand Down
57 changes: 24 additions & 33 deletions ...s/teku/spec/datastructures/execution/versions/electra/ExecutionRequestsDataCodecTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,20 +70,18 @@ public void decodeExecutionRequestData() {
}

@Test
public void decodeExecutionRequestDataWithAllRequestTypesEmpty() {
final List<Bytes> executionRequestsData = List.of(Bytes.EMPTY, Bytes.EMPTY, Bytes.EMPTY);

final ExecutionRequests executionRequests = codec.decode(executionRequestsData);
public void decodeExecutionRequestsDataWithNoRequests() {
final ExecutionRequests executionRequests = codec.decode(List.of());

assertThat(executionRequests.getDeposits()).isEmpty();
assertThat(executionRequests.getWithdrawals()).isEmpty();
assertThat(executionRequests.getConsolidations()).isEmpty();
}

@Test
public void decodeExecutionRequestDataWithOneRequestTypeEmpty() {
public void decodeExecutionRequestsDataWithOneRequestMissing() {
final List<Bytes> executionRequestsData =
List.of(depositRequestListEncoded, Bytes.EMPTY, consolidationRequestsListEncoded);
List.of(depositRequestListEncoded, consolidationRequestsListEncoded);

final ExecutionRequests executionRequests = codec.decode(executionRequestsData);

Expand All @@ -93,46 +91,39 @@ public void decodeExecutionRequestDataWithOneRequestTypeEmpty() {
}

@Test
public void decodeExecutionRequestDataWithMoreElementsThanExpected() {
public void decodeExecutionRequestsDataWithInvalidRequestType() {
final List<Bytes> invalidExecutionRequestsData =
List.of(
depositRequestListEncoded,
withdrawalRequestsListEncoded,
consolidationRequestsListEncoded,
Bytes.random(10));
List.of(depositRequestListEncoded, withdrawalRequestsListEncoded, Bytes.of(9));

assertThatThrownBy(() -> codec.decode(invalidExecutionRequestsData))
.isInstanceOf(IllegalArgumentException.class);
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Invalid execution request type: 9");
}

@Test
public void decodeExecutionRequestDataWithLessThanExpectedElements() {
public void decodeExecutionRequestDataWithRequestsNotOrderedInAscendingOrder() {
final List<Bytes> invalidExecutionRequestsData =
List.of(depositRequestListEncoded, withdrawalRequestsListEncoded);
List.of(
depositRequestListEncoded,
consolidationRequestsListEncoded,
withdrawalRequestsListEncoded);

assertThatThrownBy(() -> codec.decode(invalidExecutionRequestsData))
.isInstanceOf(IllegalArgumentException.class);
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Execution requests are not in strictly ascending order");
}

@Test
public void decodeExecutionRequestDataWithMoreThanExpectedElements() {
public void decodeExecutionRequestDataWithRepeatedRequestsOfSameType() {
final List<Bytes> invalidExecutionRequestsData =
List.of(
depositRequestListEncoded,
withdrawalRequestsListEncoded,
consolidationRequestsListEncoded,
depositRequestListEncoded);

assertThatThrownBy(() -> codec.decode(invalidExecutionRequestsData))
.isInstanceOf(IllegalArgumentException.class);
}

@Test
public void decodeExecutionRequestDataWithZeroElements() {
final List<Bytes> invalidExecutionRequestsData = List.of();
consolidationRequestsListEncoded);

assertThatThrownBy(() -> codec.decode(invalidExecutionRequestsData))
.isInstanceOf(IllegalArgumentException.class);
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Execution requests are not in strictly ascending order");
}

@Test
Expand Down Expand Up @@ -165,7 +156,7 @@ public void encodeWithWithOneEmptyRequestList() {
final List<Bytes> encodedRequests = codec.encode(executionRequests);

assertThat(encodedRequests)
.containsExactly(depositRequestListEncoded, Bytes.EMPTY, consolidationRequestsListEncoded);
.containsExactly(depositRequestListEncoded, consolidationRequestsListEncoded);
}

@Test
Expand All @@ -179,14 +170,14 @@ public void encodeWithAllEmptyRequestLists() {

final List<Bytes> encodedRequests = codec.encode(executionRequests);

assertThat(encodedRequests).containsExactly(Bytes.EMPTY, Bytes.EMPTY, Bytes.EMPTY);
assertThat(encodedRequests).isEmpty();
}

// Examples taken from
// https://github.com/ethereum/execution-apis/blob/main/src/engine/openrpc/methods/payload.yaml
private final Bytes depositRequestListEncoded =
Bytes.fromHexString(
"0x96a96086cff07df17668f35f7418ef8798079167e3f4f9b72ecde17b28226137cf454ab1dd20ef5d924786ab3483c2f9003f5102dabe0a27b1746098d1dc17a5d3fbd478759fea9287e4e419b3c3cef20100000000000000b1acdb2c4d3df3f1b8d3bfd33421660df358d84d78d16c4603551935f4b67643373e7eb63dcb16ec359be0ec41fee33b03a16e80745f2374ff1d3c352508ac5d857c6476d3c3bcf7e6ca37427c9209f17be3af5264c0e2132b3dd1156c28b4e9f000000000000000a5c85a60ba2905c215f6a12872e62b1ee037051364244043a5f639aa81b04a204c55e7cc851f29c7c183be253ea1510b001db70c485b6264692f26b8aeaab5b0c384180df8e2184a21a808a3ec8e86ca01000000000000009561731785b48cf1886412234531e4940064584463e96ac63a1a154320227e333fb51addc4a89b7e0d3f862d7c1fd4ea03bd8eb3d8806f1e7daf591cbbbb92b0beb74d13c01617f22c5026b4f9f9f294a8a7c32db895de3b01bee0132c9209e1f100000000000000");
"0x0096a96086cff07df17668f35f7418ef8798079167e3f4f9b72ecde17b28226137cf454ab1dd20ef5d924786ab3483c2f9003f5102dabe0a27b1746098d1dc17a5d3fbd478759fea9287e4e419b3c3cef20100000000000000b1acdb2c4d3df3f1b8d3bfd33421660df358d84d78d16c4603551935f4b67643373e7eb63dcb16ec359be0ec41fee33b03a16e80745f2374ff1d3c352508ac5d857c6476d3c3bcf7e6ca37427c9209f17be3af5264c0e2132b3dd1156c28b4e9f000000000000000a5c85a60ba2905c215f6a12872e62b1ee037051364244043a5f639aa81b04a204c55e7cc851f29c7c183be253ea1510b001db70c485b6264692f26b8aeaab5b0c384180df8e2184a21a808a3ec8e86ca01000000000000009561731785b48cf1886412234531e4940064584463e96ac63a1a154320227e333fb51addc4a89b7e0d3f862d7c1fd4ea03bd8eb3d8806f1e7daf591cbbbb92b0beb74d13c01617f22c5026b4f9f9f294a8a7c32db895de3b01bee0132c9209e1f100000000000000");

private final DepositRequest depositRequest1 =
new DepositRequest(
Expand Down Expand Up @@ -218,7 +209,7 @@ public void encodeWithAllEmptyRequestLists() {

private final Bytes withdrawalRequestsListEncoded =
Bytes.fromHexString(
"0xa94f5374fce5edbc8e2a8697c15331677e6ebf0b85103a5617937691dfeeb89b86a80d5dc9e3c9d3a1a0e7ce311e26e0bb732eabaa47ffa288f0d54de28209a62a7d29d0000000000000000000000000000000000000000000000000000010f698daeed734da114470da559bd4b4c7259e1f7952555241dcbc90cf194a2ef676fc6005f3672fada2a3645edb297a75530100000000000000");
"0x01a94f5374fce5edbc8e2a8697c15331677e6ebf0b85103a5617937691dfeeb89b86a80d5dc9e3c9d3a1a0e7ce311e26e0bb732eabaa47ffa288f0d54de28209a62a7d29d0000000000000000000000000000000000000000000000000000010f698daeed734da114470da559bd4b4c7259e1f7952555241dcbc90cf194a2ef676fc6005f3672fada2a3645edb297a75530100000000000000");

private final WithdrawalRequest withdrawalRequest1 =
new WithdrawalRequest(
Expand All @@ -240,7 +231,7 @@ public void encodeWithAllEmptyRequestLists() {

private final Bytes consolidationRequestsListEncoded =
Bytes.fromHexString(
"0xa94f5374fce5edbc8e2a8697c15331677e6ebf0b85103a5617937691dfeeb89b86a80d5dc9e3c9d3a1a0e7ce311e26e0bb732eabaa47ffa288f0d54de28209a62a7d29d098daeed734da114470da559bd4b4c7259e1f7952555241dcbc90cf194a2ef676fc6005f3672fada2a3645edb297a7553");
"0x02a94f5374fce5edbc8e2a8697c15331677e6ebf0b85103a5617937691dfeeb89b86a80d5dc9e3c9d3a1a0e7ce311e26e0bb732eabaa47ffa288f0d54de28209a62a7d29d098daeed734da114470da559bd4b4c7259e1f7952555241dcbc90cf194a2ef676fc6005f3672fada2a3645edb297a7553");

private final ConsolidationRequest consolidationRequest1 =
new ConsolidationRequest(
Expand Down