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

Suppress error on duplicate blobs, track slashable blobs in blob gossip cache #4995

Merged

Conversation

realbigsean
Copy link
Member

Issue Addressed

We were getting these errors in the builder flow on devnet 12. The proposals were successful, however. This was due to relays propagating blocks before returning them via the builder API (which is expected behavior). I've made the update to treat duplicate blobs as we do blocks, suppressing the duplicate error code.

Dec 07 15:09:38.029 WARN Not publishing block - not gossip verified, error: BlobError(RepeatBlob { proposer: 2714, slot: Slot(58248), index: 0 }), slot: 58248
Dec 07 15:09:38.029 WARN Error processing HTTP API request       method: POST, path: /eth/v1/beacon/blocks, status: 400 Bad Request, elapsed: 33.891095ms

In doing this, I realized we were not considering slashable blobs in the beacon API when fulfilling broadcast_validation = consensus_and_equivocation. So I've included changes to the blob sidecar gossip cache to also track block roots.

Related: #4725

Additional Info

The gossip caches are starting to seem overly complicated to me. I think they could be made simpler if we separate out our slashability tracking into its own cache, and used this for blob and block gossip. We'd end up with 3 caches total. The gossip caches aren't really meant to be tracking slashability generally, the spec sort of makes us track it in a silo'd way, and this isn't really what we want for understanding "slashable blocks" elsewhere.

Related discussion: ethereum/consensus-specs#3561

@realbigsean realbigsean added ready-for-review The code is ready for review deneb labels Dec 8, 2023
@realbigsean realbigsean changed the title Suppress error on duplicate blobs Suppress error on duplicate blobs, track slashable blobs in blob gossip cache Dec 8, 2023
@michaelsproul michaelsproul self-requested a review December 14, 2023 00:08
Copy link
Member

@michaelsproul michaelsproul left a comment

Choose a reason for hiding this comment

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

This looks good to me, although I agree it would be nice to unify the slashing cache for blocks + blobs.

It seems like the spec might get changed to IGNORE slashable messages if a slashing has already been seen. This seems like quite a major change, so I think we should merge this PR in the meantime. I think if we go down that path we might also want to track the set of to-be-slashed validators in the same unified slashings cache that we use for block & blob broadcast.

@michaelsproul
Copy link
Member

michaelsproul commented Dec 17, 2023

Hmm, just to check my understanding, on gossip we currently don't detect the case where:

  • Block header A is gossiped as a block
  • Block header B is gossiped inside a blob
  • A & B are slashable

i.e. due to the lack of a unified cache, we don't catch cross block-blob slashings?

However, we do catch these during block publishing with broadcast_validation=consensus_and_equivocation, because this PR adds the check for the block root against observed_blob_sidecars 👌

@realbigsean
Copy link
Member Author

Hmm, just to check my understanding, on gossip we currently don't detect the case where:

* Block header A is gossiped as a block

* Block header B is gossiped inside a blob

* A & B are slashable

i.e. due to the lack of a unified cache, we don't catch cross block-blob slashings?

Right, on gossip we don't detect that scenario because that's technically what the spec says. Additionally we don't detect (due to the spec):

  • Block header A is gossiped inside a blob with index 1
  • Block header B is gossiped inside a blob with index 2
  • A & B are slashable

I raised an issue on the spec repo for this cause I thought it was weird. But that likely won't change till after deneb.

However, we do catch these during block publishing with broadcast_validation=consensus_and_equivocation, because this PR adds the check for the block root against observed_blob_sidecars 👌

Yes, and we should catch the scenario I'm describing because block roots are tracked independently of blob index

@realbigsean realbigsean merged commit c55608b into sigp:unstable Dec 18, 2023
26 checks passed
@realbigsean realbigsean deleted the builder-proposal-repeat-blobs-error branch December 18, 2023 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deneb ready-for-review The code is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants