Change requests to use flat encoding #7771

jframe · 2024-10-15T05:01:31Z

PR description

Update generalised request encoding for changes introduced in devnet4

EIP-7251: change request to flat encoding ethereum/EIPs#8857
EIP-6110: change request to flat encoding ethereum/EIPs#8856
EIP-7002: change request to flat encoding ethereum/EIPs#8855
EIP-7685: change requests hash to flat hash ethereum/EIPs#8854
EIP-7685: group requests into request-data ethereum/EIPs#8924
Make execution requests a sidecar, take 2 ethereum/execution-apis#591

Fixed Issue(s)

#7744

Thanks for sending a pull request! Have you done the following?

Checked out our contribution guidelines?
Considered documentation and added the doc-change-required label to this PR if updates are required.
Considered the changelog and included an update if required.
For database changes (e.g. KeyValueSegmentIdentifier) considered compatibility and performed forwards and backwards compatibility tests

Locally, you can run these tests to catch failures early:

unit tests: ./gradlew build
acceptance tests: ./gradlew acceptanceTest
integration tests: ./gradlew integrationTest
reference tests: ./gradlew ethereum:referenceTests:referenceTests

Signed-off-by: Jason Frame <[email protected]>

Signed-off-by: Jason Frame <[email protected]> don't return request type in getPayload result Signed-off-by: Jason Frame <[email protected]>

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/BodyValidation.java

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/chain/GenesisState.java

.../src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/response/RpcErrorType.java

...rc/main/java/org/hyperledger/besu/ethereum/mainnet/requests/ProhibitedRequestsValidator.java

...eum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/requests/RequestsValidator.java

Gabriel-Trintinalia · 2024-10-16T03:19:08Z

I would love to have Request renamed to ExecutionRequest everywhere but can be done later

Signed-off-by: Jason Frame <[email protected]>

…equests Signed-off-by: Jason Frame <[email protected]>

Signed-off-by: Jason Frame <[email protected]>

…ve specific processors Signed-off-by: Jason Frame <[email protected]>

Signed-off-by: Jason Frame <[email protected]>

…quests in new structure and contract code Signed-off-by: Jason Frame <[email protected]>

…rt of the comparison Signed-off-by: Jason Frame <[email protected]>

Signed-off-by: Jason Frame <[email protected]>

...src/test/java/org/hyperledger/besu/tests/acceptance/ethereum/PragueAcceptanceTestHelper.java

...merge/src/main/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeCoordinator.java

Gabriel-Trintinalia · 2024-10-24T01:10:13Z

datatypes/src/main/java/org/hyperledger/besu/datatypes/Hash.java

+  /**
+   * Hash of empty requests or "0x6036c41849da9c076ed79654d434017387a88fb833c2856b32e18218b3341c5f"
+   */
+  public static final Hash EMPTY_REQUESTS_HASH =


Assuming this will change for devnet5?

Yep this will change for devnet5. Will create another PR for that change though

Gabriel-Trintinalia · 2024-10-24T01:38:19Z

...e/src/main/java/org/hyperledger/besu/ethereum/mainnet/requests/MainnetRequestsValidator.java

+    if (!allRequestTypesAreContained(maybeRequests.get())) {
+      LOG.warn("Requests must contain all request types");
+      return false;
+    }


Will this work if we add a new request type for an futere fork?

No it won't work as the list of request types will be different for each fork. This will also need to change for upcoming execution request changes, as there could be empty requests.

Maybe we don't even need this check? Think only the first two validations are actually necessary

Removed this size check since we won't be able do this soon with the upcoming changes anyway

Gabriel-Trintinalia · 2024-10-24T01:51:16Z

...rc/main/java/org/hyperledger/besu/ethereum/mainnet/requests/RequestProcessorCoordinator.java

+      if (requests == null) {
+        requests = new ArrayList<>();
      }
+      requests.add(r);


With this change, the only way to return Optional.Empty is a processor coordinator with no processors. We can throw an exception in the builder if trying to build with empty processors and simplify this method to return List

nice suggestion. I've done that and also simplified the process method to use a stream because doesn't need to consider nulls now

...src/main/java/org/hyperledger/besu/ethereum/mainnet/requests/SystemCallRequestProcessor.java

...e/src/main/java/org/hyperledger/besu/ethereum/mainnet/requests/MainnetRequestsValidator.java

Signed-off-by: Jason Frame <[email protected]>

siladu · 2024-10-24T05:29:04Z

@jframe fyi, I am in the middle of reviewing

siladu · 2024-10-24T05:32:46Z

I would love to have Request renamed to ExecutionRequest everywhere but can be done later

Maybe even EngineExecutionRequest to further refine the scope?

ExecutionRequest still pretty generic, unless understood within the context of the Engine API.

If we need to cross a boundary from engine API to core domain, then does something like SystemCallRequest make sense?

Gabriel-Trintinalia · 2024-10-24T05:38:10Z

I would love to have Request renamed to ExecutionRequest everywhere but can be done later

Maybe even EngineExecutionRequest to further refine the scope?

ExecutionRequest still pretty generic, unless understood within the context of the Engine API.

If we need to cross a boundary from engine API to core domain, then does something like SystemCallRequest make sense?

We have the deposit request that is not a a system call

siladu · 2024-10-24T05:39:21Z

We have the deposit request that is not a a system call

ContractRequest?? 😅

Gabriel-Trintinalia · 2024-10-24T05:42:40Z

We have the deposit request that is not a a system call

ContractRequest?? 😅

CLRequest? ConsensusRequest? EngineRequest?

jframe · 2024-10-24T05:52:06Z

My preference is for ExecutionRequest and it also matches with what it's called in the engine API spec. In EIP 7685 it's just called Requests but easy enough to know it means ExecutionRequests.

Also rather do that change in a separate PR once we agree on a name.

siladu

Sooo much deleted code, love it! +1,099 −5,067 Kudos to the spec authors too🏅

A few nits and questions - happy to approve if you think there's nothing further to change based on my comments.

consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/PayloadWrapper.java

siladu · 2024-10-24T04:55:40Z

...rc/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/execution/TracedJsonRpcProcessor.java

@@ -106,6 +106,7 @@ public JsonRpcResponse process(
        case INVALID_PROPOSAL_PARAMS:
        case INVALID_REMOTE_CAPABILITIES_PARAMS:
        case INVALID_REWARD_PERCENTILES_PARAMS:
+        case INVALID_REQUESTS_PARAMS:


nit: a little close to INVALID_REQUEST, maybe we could namespace with something, e.g. INVALID_EXECUTION_REQUESTS_PARAMS or
INVALID_ENGINE_NEW_PAYLOAD_REQUESTS_PARAMS (others already use this prefix)

~~General point: "requests" is a very overloaded name~~
Saw Gabriel mentioned changing to executionRequests.

good idea, changed it to INVALID_EXECUTION_REQUESTS_PARAMS

siladu · 2024-10-24T05:12:24Z

...e/src/main/java/org/hyperledger/besu/ethereum/mainnet/requests/MainnetRequestsValidator.java

+    return IntStream.range(0, requests.size() - 1)
+        .allMatch(i -> requests.get(i).getType().compareTo(requests.get(i + 1).getType()) <= 0);


nit: guava has quite a neat way of doing this

Suggested change

return IntStream.range(0, requests.size() - 1)

.allMatch(i -> requests.get(i).getType().compareTo(requests.get(i + 1).getType()) <= 0);

return Ordering.natural().onResultOf(Request::getType).isOrdered(requests);

nice! much less code

siladu · 2024-10-24T05:15:00Z

.../hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/AbstractEngineNewPayload.java

Enjoying the removal of lots of code, esp in this class :)

siladu · 2024-10-24T05:43:54Z

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/BlockProcessingResult.java

+    if (yield.isEmpty()) {
+      return Optional.empty();
+    } else {
+      return yield.get().getRequests();
+    }


Suggested change

if (yield.isEmpty()) {

return Optional.empty();

} else {

return yield.get().getRequests();

}

return yield.flatMap(BlockProcessingOutputs::getRequests);

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/SystemCallProcessor.java

siladu · 2024-10-24T06:05:40Z

...re/src/main/java/org/hyperledger/besu/ethereum/mainnet/requests/DepositRequestProcessor.java

+                .flatMap(receipt -> receipt.getLogsList().stream())
+                .filter(log -> address.equals(log.getLogger()))
+                .map(DepositLogDecoder::decodeFromLog)
+                .reduce(Bytes::concatenate));


a nice fix for java stream junkies here ;)

siladu

My remaining unresolved comments are only nits, nice to haves or namings

Signed-off-by: Jason Frame <[email protected]>

jframe added 10 commits October 9, 2024 14:24

rename requestsRoot to requestsHash

49a4411

Signed-off-by: Jason Frame <[email protected]>

generalised requests flat encoding and engine api changes

cdc9bd5

Signed-off-by: Jason Frame <[email protected]>

javadoc

cee6abc

Signed-off-by: Jason Frame <[email protected]>

get tests passing

8f346f4

Signed-off-by: Jason Frame <[email protected]>

get tests passing

4e12780

Signed-off-by: Jason Frame <[email protected]>

Merge remote-tracking branch 'upstream/main' into 7685_flat_encoding

42e6865

clean code

f0d5a24

Signed-off-by: Jason Frame <[email protected]>

change requests to single requestData for each requestType

0424317

Signed-off-by: Jason Frame <[email protected]>

fix PoWBlockCreatorTest after requests data type change

86f5049

Signed-off-by: Jason Frame <[email protected]>

don't return request type in getPayload result

722823e

Signed-off-by: Jason Frame <[email protected]> don't return request type in getPayload result Signed-off-by: Jason Frame <[email protected]>

lucassaldanha reviewed Oct 15, 2024

View reviewed changes

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/BodyValidation.java Show resolved Hide resolved