-
Notifications
You must be signed in to change notification settings - Fork 975
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
Add blob sidecar inclusion proof #3531
Conversation
Do we have it written down what these concerns are, concretely? |
not too sure about this one - how would we slash someone that provided a rejected / ignored blob for an otherwise accepted block? |
If someone equivocates the blob by sending different headers we can prepare a proposer slashing with these conflicting headers |
well, but this is not really different from today - ie you can always withhold blobs from everyone except the next producer if you're privately connected to them - if you're not privately connected / colluding and instead use gossip to send both block and blob, you cannot stop the correct blob from propagating (assuming correctly implemented caches that are cleared when a blob is found to be invalid). ergo, the only way to get slashed both today and after this change is to send two different proposals. |
This seems like a much more correct way to do this -- leverage the existing proposer signature (simplify signing) and relate the payload to the proposer's actual crypto-economic commitment (the header/block) |
@arnetheduck The point is that with this -- you cannot propagate two conflicting sets of blobs on the p2p network for a slot-N with out creating an equivocating proposal (thus the slashing). |
Co-authored-by: Mikhail Kalinin <noblesse.knight@gmail.com>
i also agree that this is the better/preferred way to link the blobs with the block |
This seems like the way to go.
|
there are other patches to alleviate these problems. I've opened the following to see if they can help: |
I'm going to merge this and then handle the byRoot and byRange condition tightening in another PR |
`v1.4.0-beta.4` added `KZG_COMMITMENT_INCLUSION_PROOF_DEPTH` to preset: - Spec PR: ethereum/consensus-specs#3531 - Gnosis PR: gnosischain/configs#17
- _[REJECT]_ The current finalized_checkpoint is an ancestor of the sidecar's block -- i.e. `get_checkpoint_block(store, block_header.parent_root, store.finalized_checkpoint.epoch) == store.finalized_checkpoint.root`. | ||
- _[REJECT]_ The sidecar's inclusion proof is valid as verified by `verify_blob_sidecar_inclusion_proof(blob_sidecar)`. | ||
- _[REJECT]_ The sidecar's blob is valid as verified by `verify_blob_kzg_proof(blob_sidecar.blob, blob_sidecar.kzg_commitment, blob_sidecar.kzg_proof)`. | ||
- _[IGNORE]_ The sidecar is the first sidecar for the tuple (block_header.slot, block_header.proposer_index, blob_sidecar.index) with valid header signature, sidecar inclusion proof, and kzg proof. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't work if there is a reorg and the same proposer gets lucky and is selected at the same slot on both branches of the reorg.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's actually even worse, because the blobs from the wrong branch may end up being assumed to be valid on the other branch. As the cache is populated after validation, and validation is not re-done on each reorg.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No proposer should propose blocks on both sides of the reorg, and no two blocks on different sides of the reorg can have the same root so what will happen is that validators will take one side of the equivocation in this case, which is what we do with all equivocations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is slashable, but if we accept blobs on one branch, and then reorg to the other branch, we may still end up with that situation where we have the incorrect blobs matched with a block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can't cause the blockroot is different, they can't be matched
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The block root is no longer checked, since beta 4. the check is now based on (Slot, ProposerIndex)
.
+- [IGNORE] The sidecar is the first sidecar for the tuple (block_header.slot, block_header.proposer_index, blob_sidecar.index) with valid header signature, sidecar inclusion proof, and kzg proof.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The inclusion proof is against the block root
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only body_root
not block root, but the problem is still the same: At the time when we receive the BlobSidecar
, we are on the old branch, so the checks all pass, and we cache it for tuple (block_header.slot, block_header.proposer_index, blob_sidecar.index)
. Then, there is a reorg and we switch to a new branch, with an equivocating block. For that one, we now reject any valid blobs because we already have (block_header.slot, block_header.proposer_index, blob_sidecar.index)
cached, and we assume that the ones that we originally cached are actually valid for the new branch as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This gossip validation rule doesn't apply to RPC. If nodes are able to retrieve the missing sidecars for the new branch via BlobSidecarsByRoot
, they would be able to verify DA and move forward.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we assume that the ones that we originally cached are actually valid for the new branch as well.
I think this is not true. As @potuz said, the block root will be different and the inclusion proof will not be valid anymore because the block root in the new branch is different from the one in the old branch.
`BlobSidecar` are no longer signed, but instead are linked with the signed block via merkle proof. Also renamed `signed_blob_sidecars` and `signed_blinded_blob_sidecars` in `BlockContents` envelopes to match the new types (without signature). - ethereum/consensus-specs#3531
`BlobSidecar` are no longer signed, but instead are linked with the signed block via merkle proof. Also renamed `signed_blob_sidecars` and `signed_blinded_blob_sidecars` in `BlockContents` envelopes to match the new types (without signature). - ethereum/consensus-specs#3531
`BlindedBlobSidecar` are no longer signed, but instead are linked with signed block via merkle proof. Renamed `signed_blinded_blob_sidecars` in `SignedBlindedBlockContents` envelopes to match the new types. - ethereum/consensus-specs#3531 - ethereum/beacon-APIs#370
`v1.4.0-beta.4` added `KZG_COMMITMENT_INCLUSION_PROOF_DEPTH` to preset: - Spec PR: ethereum/consensus-specs#3531 - Gnosis PR: gnosischain/configs#17
`BlobSidecar` is no longer signed, instead use Merkle proof to link blobs with block. - ethereum/consensus-specs#3531 Associated beacon-API / builder-specs still TBD; minimal changes done to compile in similar style to previous spec, but not standardized yet. - ethereum/beacon-APIs#370 - ethereum/builder-specs#92
`BlobSidecar` is no longer signed, instead use Merkle proof to link blobs with block. - ethereum/consensus-specs#3531 Associated beacon-API / builder-specs still TBD; minimal changes done to compile in similar style to previous spec, but not standardized yet. - ethereum/beacon-APIs#369 - ethereum/builder-specs#90
`BlobSidecar` is no longer signed, instead use Merkle proof to link blobs with block. - ethereum/consensus-specs#3531 Associated beacon-API / builder-specs still TBD; minimal changes done to compile in similar style to previous spec, but not standardized yet. - ethereum/beacon-APIs#369 - ethereum/builder-specs#90
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
l thank you pure did one's. am I getting close friends.
To alleviate the concerns around blob equivocations, @fradamt suggested the idea to attach the block header and an inclusion proof on each blob sidecar.
@potuz mentioned that @dankrad may had some concerns with this approach in the past.
TODO:
Merkle proof test vectors: