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.

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

Jump to bottom

Forkchoice v3 #5768

Merged
merged 17 commits into from
Aug 24, 2023
Merged

Forkchoice v3 #5768

merged 17 commits into from
Aug 24, 2023

Conversation

jflo
Copy link
Contributor

@jflo jflo commented Aug 17, 2023

PR description

Fixed Issue(s)

fixes #5735

@github-actions
Copy link

  • I thought about documentation and added the doc-change-required label to this PR if updates are required.
  • I thought about the changelog and included a changelog update if required.
  • If my PR includes database changes (e.g. KeyValueSegmentIdentifier) I have thought about compatibility and performed forwards and backwards compatibility tests

@jflo jflo force-pushed the forkchoiceV3 branch 2 times, most recently from c84b85c to ab1eded Compare August 21, 2023 14:43
@jflo jflo marked this pull request as ready for review August 21, 2023 14:47
jflo added 7 commits August 21, 2023 16:23
@jflo
adds new rpc method for fcu3
6c942c1 
Signed-off-by: Justin Florentine <justin+github@florentine.us>
@jflo
pulls up fork checking for reuse across engine api methods
12fccf1 
Signed-off-by: Justin Florentine <justin+github@florentine.us>
@jflo
more reuse of timestamp/fork checks, getPayload only needs 1 return type
fe0878b 
Signed-off-by: Justin Florentine <justin+github@florentine.us>
@jflo
error handling in fcu3, spotless
f56a391 
Signed-off-by: Justin Florentine <justin+github@florentine.us>
@jflo
we really need to factor out version specific logic down from the abs…
f219ec4 
…tract parent

Signed-off-by: Justin Florentine <justin+github@florentine.us>
@jflo
spotless
11819f2 
Signed-off-by: Justin Florentine <justin+github@florentine.us>
@jflo
v2 engine new doesn't eval forks
c9d7c2e 
Signed-off-by: Justin Florentine <justin+github@florentine.us>
@jflo jflo requested a review from macfarla August 21, 2023 20:51
@jflo jflo enabled auto-merge (squash) August 21, 2023 21:03
@macfarla
fix: merge 4788
60aad83 
Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>
pinges
pinges reviewed Aug 22, 2023
View reviewed changes
...org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/ExecutionEngineJsonRpcMethod.java Show resolved Hide resolved
...org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/ExecutionEngineJsonRpcMethod.java Outdated Show resolved Hide resolved
...va/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineNewPayloadV3.java
if (payloadParameter.getTimestamp() < cancunTimestamp) {
return ValidationResult.invalid(RpcErrorType.UNSUPPORTED_FORK, "Fork not supported");
} else {
return ValidationResult.valid();
Copy link
Contributor

Choose a reason for hiding this comment

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

we have lost some of the checks here. We need to check that the versioned hash params and the parent beacon block root are available when we are calling the v3 RPC.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are checked in the parent abstract class, via the validateParams method.

Copy link
Contributor

Choose a reason for hiding this comment

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

syncResponse is calling the much more robust validateBlobs, but it doesn't look like validateBlobs enforces that the parameters are present. Does the spec for v3 require expectedBlobVersionedHashes to be present even if the list is empty?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it can't be null. We should return invalid params if expectedBlobVersionedHashes is null. The current validateBlobs will eventually find a mismatch if the transactions have blobs and return an error but we have to check for null first. We have 2 hive tests failing because of this. I am addressing this issue in a different PR.

jflo added 2 commits August 22, 2023 15:42
@jflo
adopts pr suggestions
fe1be21 
Signed-off-by: Justin Florentine <justin+github@florentine.us>
@jflo
adopts pr suggestion to push fork checking down into versioned impls …
e402f81 
…and leave default case to valid

Signed-off-by: Justin Florentine <justin+github@florentine.us>
garyschulte
garyschulte reviewed Aug 22, 2023
View reviewed changes
Copy link
Contributor

@garyschulte garyschulte left a comment

Choose a reason for hiding this comment

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

nice refactor. just a couple questions

...va/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineNewPayloadV3.java
if (payloadParameter.getTimestamp() < cancunTimestamp) {
return ValidationResult.invalid(RpcErrorType.UNSUPPORTED_FORK, "Fork not supported");
} else {
return ValidationResult.valid();
Copy link
Contributor

Choose a reason for hiding this comment

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

syncResponse is calling the much more robust validateBlobs, but it doesn't look like validateBlobs enforces that the parameters are present. Does the spec for v3 require expectedBlobVersionedHashes to be present even if the list is empty?

macfarla and others added 5 commits August 23, 2023 10:58
@macfarla
added test for empty list
45ba585 
Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>
@macfarla
validate all required parameters together
2adccb0 
Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>
@macfarla
Merge branch 'main' of github.com:hyperledger/besu into forkchoiceV3
0a74208
@Gabriel-Trintinalia
Merge branch 'main' into forkchoiceV3
fc849c4
@jflo
adopts pr suggestion to push more milestone awareness down into speci…
780a656 
…fic versions

Signed-off-by: Justin Florentine <justin+github@florentine.us>
Gabriel-Trintinalia
Gabriel-Trintinalia approved these changes Aug 23, 2023
View reviewed changes
Copy link
Contributor

@Gabriel-Trintinalia Gabriel-Trintinalia left a comment

Choose a reason for hiding this comment

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

LGTM

@jflo jflo merged commit 2fc8682 into hyperledger:main Aug 24, 2023
garyschulte pushed a commit to garyschulte/besu that referenced this pull request Aug 28, 2023
@jflo @garyschulte
Forkchoice v3 (hyperledger#5768)
8862f6b 
* adds new rpc method for fcu3
* pulls up fork checking for reuse across engine api methods
* more reuse of timestamp/fork checks, getPayload only needs 1 return type
---------

Signed-off-by: Justin Florentine <justin+github@florentine.us>
garyschulte pushed a commit to garyschulte/besu that referenced this pull request Aug 28, 2023
@jflo @garyschulte
Forkchoice v3 (hyperledger#5768)
5a270de 
* adds new rpc method for fcu3
* pulls up fork checking for reuse across engine api methods
* more reuse of timestamp/fork checks, getPayload only needs 1 return type
---------

Signed-off-by: Justin Florentine <justin+github@florentine.us>
garyschulte pushed a commit to garyschulte/besu that referenced this pull request Aug 28, 2023
@jflo @garyschulte
Forkchoice v3 (hyperledger#5768)
897fe93 
* adds new rpc method for fcu3
* pulls up fork checking for reuse across engine api methods
* more reuse of timestamp/fork checks, getPayload only needs 1 return type
---------

Signed-off-by: Justin Florentine <justin+github@florentine.us>
Signed-off-by: garyschulte <garyschulte@gmail.com>
eum602 pushed a commit to lacchain/besu that referenced this pull request Nov 3, 2023
@jflo @eum602
Forkchoice v3 (hyperledger#5768)
dcbc04f 
* adds new rpc method for fcu3
* pulls up fork checking for reuse across engine api methods
* more reuse of timestamp/fork checks, getPayload only needs 1 return type
---------

Signed-off-by: Justin Florentine <justin+github@florentine.us>
NickSneo pushed a commit to NickSneo/besu that referenced this pull request Nov 12, 2023
@jflo @NickSneo
Forkchoice v3 (hyperledger#5768)
db60309 
* adds new rpc method for fcu3
* pulls up fork checking for reuse across engine api methods
* more reuse of timestamp/fork checks, getPayload only needs 1 return type
---------

Signed-off-by: Justin Florentine <justin+github@florentine.us>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pinges pinges pinges left review comments

@macfarla macfarla macfarla left review comments

@garyschulte garyschulte garyschulte left review comments

@Gabriel-Trintinalia Gabriel-Trintinalia Gabriel-Trintinalia approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

engine_forkchoiceUpdateV3
5 participants
@jflo @garyschulte @macfarla @Gabriel-Trintinalia @pinges