fix(l1): return -38003 for FCUv2 payloadAttributes mismatch#6353
Conversation
Greptile SummaryThis PR aligns Key changes:
Minor observation:
Confidence Score: 4/5
|
| Filename | Overview |
|---|---|
| crates/networking/rpc/engine/fork_choice.rs | FCUv2 payload attribute validation updated: validate_attributes_v2 now returns -38003 instead of -32602 when withdrawals is missing post-Shanghai; new validate_attributes_v2_pre_shanghai returns -38003 when withdrawals is unexpectedly present pre-Shanghai; unit tests added for both cases. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[FCUv2 handle - payloadAttributes present] --> B{is_cancun_activated?}
B -- Yes --> C[Return UnsupportedFork error]
B -- No --> D{is_shanghai_activated?}
D -- Yes --> E[validate_attributes_v2]
D -- No --> F[validate_attributes_v2_pre_shanghai]
E --> G{withdrawals is None?}
G -- Yes --> H[Return -38003 InvalidPayloadAttributes]
G -- No --> I[validate_timestamp]
F --> J{withdrawals is Some?}
J -- Yes --> K[Return -38003 InvalidPayloadAttributes]
J -- No --> L[validate_timestamp]
I --> M{timestamp <= head_block.timestamp?}
L --> M
M -- Yes --> N[Return -38003 InvalidPayloadAttributes - invalid timestamp]
M -- No --> O[build_payload]
Last reviewed commit: 559aa86
| fn validate_attributes_v2_pre_shanghai( | ||
| attributes: &PayloadAttributesV3, | ||
| head_block: &BlockHeader, | ||
| ) -> Result<(), RpcErr> { | ||
| if attributes.withdrawals.is_some() { | ||
| return Err(RpcErr::InvalidPayloadAttributes("withdrawals".to_string())); | ||
| } | ||
| validate_timestamp(attributes, head_block) | ||
| } |
There was a problem hiding this comment.
Ambiguous error message for pre-Shanghai withdrawals case
Both validate_attributes_v2 (missing withdrawals) and validate_attributes_v2_pre_shanghai (unexpected withdrawals) produce RpcErr::InvalidPayloadAttributes("withdrawals".to_string()). This makes it harder to distinguish the two failure modes when debugging. A more descriptive message would help differentiate them.
| fn validate_attributes_v2_pre_shanghai( | |
| attributes: &PayloadAttributesV3, | |
| head_block: &BlockHeader, | |
| ) -> Result<(), RpcErr> { | |
| if attributes.withdrawals.is_some() { | |
| return Err(RpcErr::InvalidPayloadAttributes("withdrawals".to_string())); | |
| } | |
| validate_timestamp(attributes, head_block) | |
| } | |
| fn validate_attributes_v2_pre_shanghai( | |
| attributes: &PayloadAttributesV3, | |
| head_block: &BlockHeader, | |
| ) -> Result<(), RpcErr> { | |
| if attributes.withdrawals.is_some() { | |
| return Err(RpcErr::InvalidPayloadAttributes( | |
| "unexpected withdrawals before Shanghai".to_string(), | |
| )); | |
| } | |
| validate_timestamp(attributes, head_block) | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/networking/rpc/engine/fork_choice.rs
Line: 381-389
Comment:
**Ambiguous error message for pre-Shanghai `withdrawals` case**
Both `validate_attributes_v2` (missing `withdrawals`) and `validate_attributes_v2_pre_shanghai` (unexpected `withdrawals`) produce `RpcErr::InvalidPayloadAttributes("withdrawals".to_string())`. This makes it harder to distinguish the two failure modes when debugging. A more descriptive message would help differentiate them.
```suggestion
fn validate_attributes_v2_pre_shanghai(
attributes: &PayloadAttributesV3,
head_block: &BlockHeader,
) -> Result<(), RpcErr> {
if attributes.withdrawals.is_some() {
return Err(RpcErr::InvalidPayloadAttributes(
"unexpected withdrawals before Shanghai".to_string(),
));
}
validate_timestamp(attributes, head_block)
}
```
How can I resolve this? If you propose a fix, please make it concise.
ElFantasma
left a comment
There was a problem hiding this comment.
LGTM, Greptile suggestion is a nice to have, but not blocking. Thanks for the contribution!
|
Hey, @Muzry! Your commits are showing as Unverified, which is blocking the merge. To fix this, you need to set up commit signature verification on GitHub. Quickest fix (SSH signing — no GPG needed): # 1. Tell git to sign commits with your SSH key
git config --global gpg.format ssh
git config --global user.signingkey ~/.ssh/id_ed25519.pub # or whatever key you use with GitHub
git config --global commit.gpgsign true
# 2. Add the same key as a **Signing Key** on GitHub:
# → https://github.com/settings/keys → "New SSH Key" → Key type: **Signing Key**
# 3. Re-sign your commits on this branch (rebase onto the base branch)
git rebase main --exec 'git commit --amend --no-edit -S'
git push --force-with-lease Make sure the email in git config user.email matches your GitHub account (or is added in https://github.com/settings/emails). Full docs: https://docs.github.com/en/authentication/managing-commit-signature-verification/about-commit-signature verification |
ee28d75 to
e3171a5
Compare
b2f857e to
8810591
Compare
8810591 to
6052034
Compare
|
@Arkenan Thanks. I’ve updated the branch and re-signed the commits with my SSH signing key. The commits are now verified on GitHub. Please take another look when you have a moment. |
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.