Add broadcast_validation to block publishing

[michaelsproul]

In light of the unbundling attacks performed against MEV relays, the relays have started verifying blocks before broadcasting them on gossip. Presently this requires custom forks of Lighthouse and Prysm, as all stock consensus clients will broadcast blocks before consensus validation is run.

• Lighthouse fork: fully validate a block before publishing sigp/lighthouse#4168
• Prysm fork: [DO NOT MERGE]: Apply consensus and equivocation check before broadcast prysmaticlabs/prysm#12335

To improve relay consensus client diversity and provide a path for un-forking Lighthouse and Prysm, this PR adds a standard query parameter that can be used to control when a block is broadcast. It is expected that relays would use ?broadcast_validation=consensus_and_equivocation to protect themselves from equivocation attacks.

For completeness we could imagine a ?broadcast_validation=full option which also checks the execution payload with the execution layer. However this would not be used by relays, as they check the execution payload separately. In the interests of simplicity I think we should avoid adding it until/unless someone thinks of a use for it.

[michaelsproul]

Note that the relays also want to prevent broadcasting of the block if an equivocation is detected. We had to add some more logic to Lighthouse to cover this, but in principle this is part of gossip validation.

[g11tech]

👍 from lodestar side, will add

[rolfyone]

Overall, I don't think we can't implement this as it stands, purely because its a breaking change. We'd have to have 100% adoption by all clients for this to be fully supported, and anyone that happens to call an older client would be expecting functionality that's not there...

Breaking changes in rest api are generally not a great plan, though we may get away with it if everyone agrees and we're careful. That being said, users may find the experience quite confusing if they happen to end up hitting an endpoint where the functionality isn't implemented...

For this to really be effective i think we'd really need to increase the version number so that users know what they're consuming and can be confident...

[dapplion]

For this to really be effective i think we'd really need to increase the version number so that users know what they're consuming and can be confident...

It's not pretty but I agree with the reasoning of needing a version bump

[terencechain]

Do we also need an equivocation check? if yes, it's better to explicitly mention in the notes

For Prysm, it's easy to apply a consensus check, and it's hard to apply an equivocation check. I don't think that's something we are willing to have in the master branch

[michaelsproul]

I'll modify the PR to bump the version to v2. As a deprecation plan, how about we keep the v1 endpoints around until the next hard fork to save some pain? Updating all the validator clients and ensuring BN compatibility with the new v2 methods could create unnecessary compatibility issues. Relays can start using the v2 methods and the rest of us can switch over when it suits us.

For Prysm, it's easy to apply a consensus check, and it's hard to apply an equivocation check

@terencechain Do you think it would make sense to have a separate parameter for the equivocation check? For Lighthouse I was initially thinking we would just enable it all the time, but if it's something that needs to be toggled maybe we do it separately like ?broadcast_equivocation_check. Or we could bundle it with broadcast_validation=consensus or broadcast_validation=consensus_and_equivocation? Does anyone have a preference amongst these?

[rolfyone]

I'll modify the PR to bump the version to v2. As a deprecation plan, how about we keep the v1 endpoints around until the next hard fork to save some pain? Updating all the validator clients and ensuring BN compatibility with the new v2 methods could create unnecessary compatibility issues. Relays can start using the v2 methods and the rest of us can switch over when it suits us.

Happy to hear suggestions about how to handle the old v1.. its been 'required' for a while so maybe yes, remove support after a hard fork, and mark it deprecated in this PR?

In terms of usage, i guess clients might need to cater for calling the v2 and it not existing, since we don't have the concept of a supported endpoints kind of function... I could foresee calling v2, getting 404, not calling it again until restart, or something like that, or until we lose and regain the connection or similar, but that's more of a client implementation detail rather than a detail for the spec to consider too... just thinking out loud...

[dapplion]

Why not keep v1 alive for foreseable future until there's social consensus all tooling supports v2? If we allow v1 to handle the new query the handler for v2 and v1 should be the same

[michaelsproul]

Why not keep v1 alive for foreseable future until there's social consensus all tooling supports v2?

My thinking exactly. No need to deprecate v1 (yet)

If we allow v1 to handle the new query the handler for v2 and v1 should be the same

I think from a spec PoV we shouldn't require v1 to have the param, but from an impl PoV this is fine

[rolfyone]

If we allow v1 to handle the new query the handler for v2 and v1 should be the same

yep i don't have the problem with v1 having the query param implemented potentially... my main problem was saying 'must' when a lot of implementations are already installed that don't have that functionality and so it cant be relied upon...

I could definitely see v1 and v2 of code being the same for anyone implementing v2, and that's fine... as long as the v2 definitely has this functionality and we can reasonably be sure it's going to be run when its requested.

[metachris]

Thanks for creating this issue and starting the discussion and standardization!

One note -- relays require the equivocation check, without it wouldn't really be usable.

[rolfyone]

LGTM