Eth-Consensus-version header now passed for json block production from VC #8754
Conversation
…m VC When producing a block, JSON payloads require the Eth-Consensus-version header. Generally using SSZ this was correct, but there was no test case for JSON block production, and it wasn't providing the header correctly. Because probably 99% of our block production is via SSZ, this was not really visible. This would not be a problem for local BN+VC, but if the teku VC ever falls back to json (or is instructed to produce JSON), block production could potentially fail if the BN is following spec. Changed an AT to check both SSZ and JSON block production. fixes Consensys#8753 Signed-off-by: Paul Harris <[email protected]>
… and elaborated on changelog. Signed-off-by: Paul Harris <[email protected]>
There was a problem hiding this comment.
LGTM
AT is a good way to cover it but I'd also be inclined to add an integration test like we do for the other APIs. A good example is the PostAttestationsV2IntegrationTest where we have a test for the header specifically.
We can go with the current changes and add that in a separate PR though
|
Just an addition: If we'd want off spec VCs (doesn't send the header) to work any way, we could use the header with slot fall back selector. We could live with that before completely relying on the header. But tbh it's in the spec now and it's legitimate to expect the consensus version header |
yes I have a version with that locally working but I think potentially being spec compliant and updating the changelog is probably a good option. |
When producing a block, JSON payloads require the Eth-Consensus-version header.
Generally using SSZ this was correct, but there was no test case for JSON block production, and it wasn't providing the header correctly.
Because probably 99% of our block production is via SSZ, this was not really visible.
This would not be a problem for local BN+VC, but if the teku VC ever falls back to json (or is instructed to produce JSON), block production could potentially fail if the BN is following spec.
Changed an AT to check both SSZ and JSON block production.
fixes #8753
Documentation
doc-change-requiredlabel to this PR if updates are required.Changelog