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.

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

core: remove withdrawal length check for state processor #30286

Merged

Conversation

islishude
Copy link
Contributor

I thought the withdrawal length check should be placed in BlockValidator.ValidateBody

if header.WithdrawalsHash != nil {
// Withdrawals list must be present in body after Shanghai.
if block.Withdrawals() == nil {
return errors.New("missing withdrawals in block body")
}
if hash := types.DeriveSha(block.Withdrawals(), trie.NewStackTrie(nil)); hash != *header.WithdrawalsHash {
return fmt.Errorf("withdrawals root hash mismatch (header value %x, calculated %x)", *header.WithdrawalsHash, hash)
}
} else if block.Withdrawals() != nil {
// Withdrawals are not allowed prior to Shanghai fork
return errors.New("withdrawals present in block body")
}

the BlockValidator has the chain config field, and should be feasible to move the check to the ValidateBody.

but there is only a withdrawal root hash check.

and the verifyHeader func in the consensus package ensures a block that before shanghai fork doesn't have a withdrawal hash.

shanghai := chain.Config().IsShanghai(header.Number, header.Time)
if shanghai && header.WithdrawalsHash == nil {
return errors.New("missing withdrawalsHash")
}
if !shanghai && header.WithdrawalsHash != nil {
return fmt.Errorf("invalid withdrawalsHash: have %x, expected nil", header.WithdrawalsHash)
}

so I think the check in the processor is not required and can be removed.

@rjl493456442
Copy link
Member

Personally I think the change is good, but i would prefer to discuss with team tomorrow as it's a very core part.

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

I can think of no reason to have this check here, that would not be covered already by the other checks. LGTM

@lightclient lightclient merged commit 09d889d into ethereum:master Aug 16, 2024
3 checks passed
@lightclient lightclient added this to the 1.14.9 milestone Aug 16, 2024
@islishude islishude deleted the remove-withdrawal-check-in-processor branch August 16, 2024 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants