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

BlobSideCar mutable in gossip #3103

Closed
djrtwo opened this issue Nov 14, 2022 · 7 comments
Closed

BlobSideCar mutable in gossip #3103

djrtwo opened this issue Nov 14, 2022 · 7 comments
Labels
Deneb was called: eip-4844

Comments

@djrtwo
Copy link
Contributor

djrtwo commented Nov 14, 2022

beacon_block_and_blobs_sidecar gossip validations are not sufficient to protect against any node mutating the sidecar after the proposer begins the gossip. As there is both no (1) proposer signature on the sidecar payload or (2) a cryptographic check that the commitments in the block map to the blobs in the sidecar.

EDIT: some background on gossip validations, we generally want to do minimum sufficient crypto-economic and validity checks to ensure the payload is at least "worth" gossiping. Usually this is a signature check to know it is from the expected actor and then whatever relatively cheap validity checks you can do to make sure it's generally well-formed. Here we have no crypto-economic commitment taht the adjoining payload was even made by the expected actor

So we can either sign the payload and do another sig check or we can run the full validate_blobs_sidecar.
@Inphi estimates validate_blobs_sidecar as ~12ms @ 2MB (4ms at 256kb) whereas the signature check @terencechain estimates at 1 to 4ms. If we run validate_blobs_sidecar will it be dominant wrt time? If so, we need to be careful here. If not, lets just do that.

@hwwhww hwwhww added the Deneb was called: eip-4844 label Nov 15, 2022
@kevaundray
Copy link
Contributor

Reposting here:

goos: darwin
goarch: amd64
pkg: github.com/crate-crypto/go-proto-danksharding-crypto/agg_kzg
cpu: Intel(R) Core(TM) i9-9980HK CPU @ 2.40GHz
BenchmarkAggVerify1-16 2499203 ns/op
BenchmarkAggVerify2-16 3278393 ns/op
BenchmarkAggVerify3-16 4033615 ns/op
BenchmarkAggVerify4-16 4844001 ns/op
BenchmarkAggVerify5-16 5763020 ns/op
BenchmarkAggVerify6-16 6500817 ns/op
BenchmarkAggVerify7-16 7366121 ns/op
BenchmarkAggVerify8-16 8267168 ns/op
BenchmarkAggVerify9-16 8857999 ns/op
BenchmarkAggVerify10-16 9594048 ns/op
BenchmarkAggVerify11-16 10273552 ns/op
BenchmarkAggVerify12-16 14101749 ns/op
BenchmarkAggVerify13-16 13103934 ns/op
BenchmarkAggVerify14-16 13431766 ns/op
BenchmarkAggVerify15-16 15368090 ns/op
BenchmarkAggVerify16-16 14334650 ns/op

Legend:

BenchmarkAggVerifyX-K : X denotes the amount of blobs and K denotes the number of cores on my machine

Blobs size is 4096, so each blob holds 4096*32 bytes

@realbigsean
Copy link
Contributor

I've been digging into this in lighthouse a bit more - median gossip verification times are around 8-10ms. I found validate_blobs_sidecar is pretty comparable to signature verification if we have to sign over the merkleized root of the blob, because the root calculation takes a decent amount of time. @paulhauner suggested we could sign over a "flat hash" of the blob sha2(ssz(blob)) to avoid the root calculation, and this has a more meaningful impact.

These are rough numbers:

Blobs per block Sig verification (root calculation) Flat hash sig verification
16 11ms (9.5ms) 2.8ms
8 6ms (4.5ms) 2.1ms
4 3.4 (1.9ms) 1.7ms

CPU: Ryzen 7 2700X 8 Core 16 thread w/ sha extensions

Signing blobs ends up being quite impactful in a few areas (VC, beacon API, and web3signer API) that we otherwise wouldn't need to touch, so I think it's generally preferable not to if we can afford it. I'd be interested in seeing to what degree we can we can parallelize validate_blobs_sidecar with the rest of gossip verification. If it becomes apparent that this is a bottleneck, we had previously done an implementation of blob signing that we can revive. And in that case it seems a scheme with a flat hash is what we'd want.

@dapplion
Copy link
Collaborator

Another argument against signature: In the happy case if you run validate_blobs_sidecar on gossip you are doing work you'll have to do anyway down the pipeline. With blob signature you are adding extra work per blob for its processing lifetime.

@djrtwo
Copy link
Contributor Author

djrtwo commented Nov 16, 2022

on gossip you are doing work you'll have to do anyway down the pipeline

Although this is true, the general concern isn't about added total work, imo. It's about added work on hops -- meaning you could argue for more total work in some cases if it allowed data to be disseminated faster.


Thanks @realbigsean for the analysis! Agreed to avoid signing complexity if possible, especially because it ripples all the way into the VC.

median gossip verification times are around 8-10ms

This is Block gossip today? (not attestations, right?). So in a 16 blobs-per-block world, if we don't parallelize, this more than doubles hop computation time given @kevaundray's numbers. That said, I don't see any reason this can't be parallelized against the other verifications (e.g. the proposer signature) which ideally puts us only at a marginal increase in computation.

@kevaundray -- I see you have 16 cores in your benchmarks. Is the computation actually leveraging all the cores? This might get in the way of our parallelization analysis if so.

The flat hash is a nice alternative if we hit a roadblock, especially given the "native" commitment is kzg so losing the info htr preserves doesn't really matter

@realbigsean
Copy link
Contributor

This is Block gossip today? (not attestations, right?)

Yes this is block gossip on mainnet

@terencechain
Copy link
Contributor

Blobs size is 4096, so each blob holds 4096*32 bytes

Maybe a bit off-topic here, but I'm really curious about what the 256/512KB benchmarks looks like

@dapplion
Copy link
Collaborator

Should be closed, p2p spec is already updated

-- i.e. `validate_blobs_sidecar(block.slot, hash_tree_root(block), block.body.blob_kzg_commitments, sidecar)`

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Deneb was called: eip-4844
Projects
None yet
Development

No branches or pull requests

7 participants