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

Think about broadcast_validation in the context of blobs #4725

Closed
michaelsproul opened this issue Sep 12, 2023 · 4 comments
Closed

Think about broadcast_validation in the context of blobs #4725

michaelsproul opened this issue Sep 12, 2023 · 4 comments

Comments

@michaelsproul
Copy link
Member

Description

The broadcast_validation flag exists during block publishing so that block builders/relays can guard against invalid and equivocating blocks, and halt publishing.

The addition of blobs in Deneb requires careful consideration to ensure that a malicious proposer does not gain any more avenues for tricking the relay into publishing an invalid/equivocating block.

I think it's probably sufficient to just return the same kind of duplicate error that we return for blocks whenever the gossip cache for blobs is tripped, but want to just make a note of this so that we can revisit it properly.

@jimmygchen
Copy link
Member

jimmygchen commented Nov 16, 2023

@realbigsean if I understand correctly, this is no longer an issue as blobs are no longer signed after the blob inclusion proof changes (spec)?

@pawanjay176
Copy link
Member

@jimmygchen Not sure. Since each blob contains a SignedBeaconBlockHeader, it is possible that a builder can embed an equivocating block header into the sidecar.
It's also possible that the inclusion proof is invalid.
So there's still scope of the blobs being invalid/equivocating.

@michaelsproul
Copy link
Member Author

I think our decision in #4995 is sensible. When broadcast_validation=consensus_and_equivocation:

  • Do not publish if the block's header is slashable wrt to any block header that has been seen on gossip on either the block topic or a blob topic.
  • Do not publish if the block's header is slashable wrt to a block fetched via RPC. Accomplished by this observation:
    chain
    .observed_block_producers
    .write()
    .observe_proposal(block_root, block.message())
    .map_err(|e| BlockError::BeaconChainError(e.into()))?;

I think the only case we're missing is blobs received via RPC. As far as I can tell we never write these to observed_blob_sidecars because the only call to observe_sidecar is in validate_blob_sidecar_for_gossip. I also don't think we can guarantee that any blobs received on RPC have already had their block header added to observed_block_producers or observed_blob_sidecars? We trigger single blob RPC alongside single block RPC for events like unknown parent, etc, and can't guarantee that the blobs we get back necessarily match the block's header because A) we may not know the block header B) the peer may send us blobs with a mismatched block header (which is invalid, but still slashable).

@realbigsean
Copy link
Member

I think slashability is now well covered with #5033

So I'm going to close this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants