engine: return -38003 for FCUv2 payloadAttributes mismatch#19779
engine: return -38003 for FCUv2 payloadAttributes mismatch#19779yperbasis merged 4 commits intoerigontech:mainfrom
Conversation
|
@yperbasis Could you please take a look when you have a chance? I’m not sure whether my changes in this PR are causing the issue, since some Hive CI tests are still failing. I suspect this may be related to Hive CI pinning a specific commit, as the failures are still there after my changes. If I got anything wrong, please let me know and I’ll fix it as soon as possible. |
yperbasis
left a comment
There was a problem hiding this comment.
Code duplication — checkPayloadAttributesWithdrawalsPresence is identical to checkWithdrawalsPresence except for the error type
50ad188 to
e49237d
Compare
e49237d to
fb86252
Compare
|
@yperbasis Thanks, updated. |
There was a problem hiding this comment.
Issues
- Unnecessary indirection in forkchoiceUpdated
The PR adds isWithdrawalsPresenceValid but then doesn't use it where it matters:
// engine_server.go:728 — calls checkWithdrawalsPresence just to discard its error
if version >= clparams.CapellaVersion {
if err := s.checkWithdrawalsPresence(timestamp, payloadAttributes.Withdrawals); err != nil {
return nil, &engine_helpers.InvalidPayloadAttributesErr
}
}
This constructs an InvalidParamsError only to throw it away. Should use the boolean predicate directly:
if version >= clparams.CapellaVersion {
if !s.isWithdrawalsPresenceValid(timestamp, payloadAttributes.Withdrawals) {
return nil, &engine_helpers.InvalidPayloadAttributesErr
}
}
That's the whole point of having extracted isWithdrawalsPresenceValid — use it.
- Tests don't test the actual behavior
Both test cases follow the same pattern:
err := srv.checkWithdrawalsPresence(1001, nil) // tests old code path, returns InvalidParamsError
require.Error(t, err)
require.Equal(t, &rpc.InvalidParamsError{...}, err) // asserts the WRONG error type
err = &engine_helpers.InvalidPayloadAttributesErr // reassigns err to a completely new value
require.Equal(t, -38003, err.(rpc.Error).ErrorCode()) // asserts error code of a constant
This proves nothing about the integration. It verifies that checkWithdrawalsPresence still returns InvalidParamsError (the old behavior) and that InvalidPayloadAttributesErr has code -38003 (a constant).
Neither assertion tests that forkchoiceUpdated actually returns -38003.
The tests should either call forkchoiceUpdated with a CapellaVersion and verify the returned error, or at minimum call isWithdrawalsPresenceValid and verify it returns false for these inputs.
- Missing analogous check for checkRequestsPresence?
The same pattern applies to execution requests with checkRequestsPresence in the Prague/Electra case. If the spec change applies to all FCU versions (not just V2), this may need a similar treatment. If it's
V2-specific, fine — but worth confirming.
Suggested approach
Keep isWithdrawalsPresenceValid as the shared predicate. Use it directly in forkchoiceUpdated. Leave checkWithdrawalsPresence unchanged for newPayload (which still needs InvalidParamsError). Write tests that
exercise the actual forkchoiceUpdated code path or at least the boolean predicate.
This PR updates `engine_forkchoiceUpdatedV2` to return `-38003: Invalid payload attributes` when the wrong `payloadAttributes` version is used. In particular, FCUv2 payload-attribute version mismatches such as: - missing `withdrawals` at or after Shanghai - unexpected `withdrawals` before Shanghai should be treated as `Invalid payload attributes`, not `Invalid params`. ## Why This change aligns the client with the latest Engine API spec update in: - ethereum/execution-apis#761 It also follows the implementation discussion and prior client-side change in: - ethereum/go-ethereum#33918 The spec was clarified so that FCUv2 now behaves consistently with newer forkchoiceUpdated versions for payloadAttributes structure/version mismatches. ## What changed - Updated FCUv2 payload attributes validation to return `-38003` for payloadAttributes version mismatches. - Added/updated regression coverage for the affected FCUv2 cases. ## Hive impact This fixes the Hive `engine-withdrawals` failure caused by returning the wrong error code for FCUv2 payloadAttributes mismatches. Relevant Hive failure: - https://hive.ethpandaops.io/#/test/generic/1773130326-4e173a80b2b6f0634fd0139743cbe0de After this change, the client returns the expected error code for the affected FCUv2 cases. If my understanding or interpretation of the spec change is incorrect, please let me know and I can adjust the implementation accordingly. --------- Co-authored-by: muzry.li <muzry.li1@ambergroup.io>
This PR updates `engine_forkchoiceUpdatedV2` to return `-38003: Invalid payload attributes` when the wrong `payloadAttributes` version is used. In particular, FCUv2 payload-attribute version mismatches such as: - missing `withdrawals` at or after Shanghai - unexpected `withdrawals` before Shanghai should be treated as `Invalid payload attributes`, not `Invalid params`. ## Why This change aligns the client with the latest Engine API spec update in: - ethereum/execution-apis#761 It also follows the implementation discussion and prior client-side change in: - ethereum/go-ethereum#33918 The spec was clarified so that FCUv2 now behaves consistently with newer forkchoiceUpdated versions for payloadAttributes structure/version mismatches. ## What changed - Updated FCUv2 payload attributes validation to return `-38003` for payloadAttributes version mismatches. - Added/updated regression coverage for the affected FCUv2 cases. ## Hive impact This fixes the Hive `engine-withdrawals` failure caused by returning the wrong error code for FCUv2 payloadAttributes mismatches. Relevant Hive failure: - https://hive.ethpandaops.io/#/test/generic/1773130326-4e173a80b2b6f0634fd0139743cbe0de After this change, the client returns the expected error code for the affected FCUv2 cases. If my understanding or interpretation of the spec change is incorrect, please let me know and I can adjust the implementation accordingly. --------- Co-authored-by: muzry.li <muzry.li1@ambergroup.io>
This PR updates
engine_forkchoiceUpdatedV2to return-38003: Invalid payload attributeswhen the wrongpayloadAttributesversion is used.In particular, FCUv2 payload-attribute version mismatches such as:
withdrawalsat or after Shanghaiwithdrawalsbefore Shanghaishould be treated as
Invalid payload attributes, notInvalid params.Why
This change aligns the client with the latest Engine API spec update in:
It also follows the implementation discussion and prior client-side change in:
The spec was clarified so that FCUv2 now behaves consistently with newer forkchoiceUpdated versions for payloadAttributes structure/version
mismatches.
What changed
-38003for payloadAttributes version mismatches.Hive impact
This fixes the Hive
engine-withdrawalsfailure caused by returning the wrong error code for FCUv2 payloadAttributes mismatches.Relevant Hive failure:
After this change, the client returns the expected error code for the affected FCUv2 cases.
If my understanding or interpretation of the spec change is incorrect, please let me know and I can adjust the implementation accordingly.