Skip to content

Conversation

@MatusKysel
Copy link

• Consensus Block Value: Now only returns an error if a non-empty value fails conversion, aligning with PR14111 that it isn’t critical for block production.
• Other Headers: No changes; the execution and blinded fields continue to be validated as before.

This update improves robustness by preventing errors when the consensus value is absent.

@mcdee
Copy link
Contributor

mcdee commented Apr 15, 2025

I'm not convinced by this change. The spec requires the value to be present, so if it is not then that suggests an error with the server. The assumption that the block value is not important to the code receiving the response is not valid in all situations, so this change could end up masking a real problem.

@MatusKysel
Copy link
Author

Hey @mcdee I know spec requires, this is more like a workaround to following code in Prysm https://github.com/OffchainLabs/prysm/blob/develop/beacon-chain/rpc/eth/validator/handlers_block.go#L122 . I’ve also opened an issue with them, but until that’s resolved, I think we could use something like this to avoid missing a block proposal.
Let me know what you think.

@mcdee
Copy link
Contributor

mcdee commented Apr 26, 2025

Although I understand the idea, I am concerned that letting through blocks that have an equivalent 0 value is going to cause problems itself by masking the error and providing incorrect information to clients.

More generally, patching around nodes that fail to adhere to the spec creates a precedent that I'm not sure we want to create. We would be stuck with these workarounds in the codebase for months or years (most likely until the next hard fork), and it creates additional work to confirm that any fix is in place prior to us being able to remove the patch. As such, I think the best we can realistically manage is to ensure we operate according to the spec.

@MatusKysel
Copy link
Author

Hey @mcdee,
I understand your point. However, missing a proposal is quite a big deal for us. Until Prysm addresses the issue themselves (relevant issue), we need a workaround.
Currently, we're running a fork of go-eth2-client with this change internally, but it would be much better for us if it could be included in the official version to avoid additional maintenance overhead.
Thanks for considering it!

@MatusKysel
Copy link
Author

Prysm fix it on theirs site with work around for now OffchainLabs/prysm#15526

@MatusKysel MatusKysel closed this Aug 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants