perf(l1): defer KZG blob proof verification from P2P to mempool insertion#6150
Conversation
…rtion eBPF profiling on main showed ~38% of CPU time spent in BLS12-381/KZG proof verification during P2P blob transaction handling. The expensive verify_kzg_proof_batch/verify_cell_kzg_proof_batch calls in validate_requested() run for every blob tx received from every peer, before the mempool dedup check. Add validate_cheap() to BlobsBundle that performs all structural checks (max blobs, version/fork, lengths, versioned hash SHA256) but skips the KZG proof verification. Use it in validate_requested() so the P2P layer only does cheap validation. Full KZG verification still happens in add_blob_transaction_to_pool() after dedup, ensuring correctness while avoiding redundant cryptographic work for the same blob seen from multiple peers.
🤖 Kimi Code ReviewReview SummaryThis PR introduces a Issues Found1. Missing KZG Feature Check (Security/Correctness)File: 2. Incomplete Validation (Security)File:
The current implementation only checks structural consistency, which is appropriate for the "cheap" validation, but the method name could be clearer about what it does/doesn't validate. 3. Redundant Validation (Performance)File: 4. Error Handling Consistency (Code Quality)File: match (self.version, fork) {
(0, fork) if fork >= Fork::Osaka => return Err(BlobsBundleError::InvalidBlobVersionForFork),
(v, fork) if v != 0 && fork < Fork::Osaka => return Err(BlobsBundleError::InvalidBlobVersionForFork),
_ => {}
}Suggestions
The change in Automated review by Kimi (Moonshot AI) · custom prompt |
🤖 Claude Code ReviewNow I have a complete understanding of the PR. Let me compile my review: PR Review: Defer KZG blob proof verification from P2P to mempool insertionSummaryThis optimization defers expensive KZG proof verification from the P2P layer to mempool insertion, achieving an 85.5% reduction in KZG-related CPU samples. The approach is sound and the implementation is correct. Positive Aspects✅ Solid Performance Improvement: The profiling data shows clear impact - reducing redundant KZG verification for duplicate blob transactions received from multiple peers. ✅ Security Architecture: The validation flow is correct:
✅ Code Quality: The Issues & Suggestions1. Missing Operator Precedence Parentheses (Critical for correctness)Location: The condition has ambiguous operator precedence: if self.version == 0 && fork >= Fork::Osaka || self.version != 0 && fork < Fork::Osaka {This evaluates as: While this appears correct, it's error-prone and hard to verify. The same issue exists in the original Recommendation: Add explicit parentheses for clarity: if (self.version == 0 && fork >= Fork::Osaka) || (self.version != 0 && fork < Fork::Osaka) {2. Code Duplication (Maintainability concern)Location: The Recommendation: Consider refactoring to reduce duplication: pub fn validate(&self, tx: &super::EIP4844Transaction, fork: super::Fork)
-> Result<(), BlobsBundleError> {
// Call cheap validation first
self.validate_cheap(tx, fork)?;
// Then do expensive KZG verification
if self.version != 0 {
use ethrex_crypto::kzg::verify_cell_kzg_proof_batch;
if !verify_cell_kzg_proof_batch(&self.blobs, &self.commitments, &self.proofs)? {
return Err(BlobsBundleError::BlobToCommitmentAndProofError);
}
} else {
use ethrex_crypto::kzg::verify_kzg_proof_batch;
if !verify_kzg_proof_batch(&self.blobs, &self.commitments, &self.proofs)? {
return Err(BlobsBundleError::BlobToCommitmentAndProofError);
}
}
Ok(())
}This ensures they stay in sync and makes the optimization explicit. 3. Missing Test Coverage (Nice to have)Location: No tests for While existing tests cover the full Recommendation: Add tests to verify:
4. Minor: Comment ClarityLocation: The comment is good but could mention that this is a performance optimization specifically. Suggestion: /// Performance optimization: Validates blob bundle structure without expensive
/// KZG cryptographic verification. Used in P2P validation where full KZG is
/// deferred to mempool insertion (after dedup check), avoiding redundant proof
/// verification for the same blob tx received from multiple peers.
Ethereum-Specific Considerations✅ EIP-4844 Compliance: The structural validation correctly checks all EIP-4844 requirements except cryptographic proof verification, which is intentionally deferred. ✅ Fork Handling: Properly handles Osaka fork blob version transition (version 0 vs version 1). ✅ Versioned Hash Verification: Correctly validates SHA256-based versioned hashes in VerdictApprove with suggestions. The optimization is sound and achieves significant performance improvements. The security model is correct - all transactions eventually undergo full KZG verification before entering the mempool. The main concern is code duplication which should be addressed to improve maintainability. Priority fixes:
Automated review by Claude (Anthropic) · custom prompt |
🤖 Codex Code ReviewFindings
If the above is intentional (especially the peer-penalty change), then the core logic looks consistent and the performance win is clear. Automated review by OpenAI Codex · custom prompt |
Lines of code reportTotal lines added: Detailed view |
Greptile OverviewGreptile SummaryThis PR optimizes blob transaction handling by deferring expensive KZG cryptographic proof verification from P2P receipt to mempool insertion, reducing redundant computation when the same blob transaction is received from multiple peers. Key changes:
Performance impact: Security analysis:
The implementation is clean, minimal, and well-documented with clear comments explaining the optimization strategy. Confidence Score: 5/5
|
| Filename | Overview |
|---|---|
| crates/common/types/blobs_bundle.rs | Added validate_cheap() method that performs structural validation without expensive KZG cryptographic verification - correctly implements all checks except KZG proof verification |
| crates/networking/p2p/rlpx/eth/transactions.rs | Changed P2P validation to use validate_cheap() instead of validate() - defers expensive KZG proof verification to mempool insertion |
Sequence Diagram
sequenceDiagram
participant Peer
participant P2P as P2P Layer<br/>(validate_requested)
participant Mempool
participant Validation as Full Validation<br/>(add_blob_transaction_to_pool)
Note over Peer,Validation: Before this PR: KZG runs N times for N peers
Peer->>P2P: Send blob transaction
P2P->>P2P: validate_cheap()<br/>(structural checks only)
Note right of P2P: Max blobs check<br/>Empty check<br/>Fork compatibility<br/>Length checks<br/>SHA256 hash verification<br/>❌ Skip KZG proof
P2P->>Mempool: Add to mempool
Mempool->>Mempool: Check if tx already exists
alt Transaction already in pool
Mempool-->>P2P: Return early (no KZG!)
else New transaction
Mempool->>Validation: validate() full KZG check
Note right of Validation: ✅ KZG proof verification<br/>(expensive!)
Validation-->>Mempool: Valid
Mempool->>Mempool: Add transaction
end
Note over Peer,Validation: Result: KZG runs exactly once per unique blob tx
There was a problem hiding this comment.
Pull request overview
This PR reduces redundant CPU-heavy KZG verification during P2P blob-transaction handling by deferring full proof verification until mempool insertion, where duplicates are filtered first.
Changes:
- Add
BlobsBundle::validate_cheap()to perform structural/versioned-hash checks while skipping KZG proof verification. - Switch P2P
PooledTransactions::validate_requested()to usevalidate_cheap()instead of fullvalidate()for EIP-4844 txs.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| crates/networking/p2p/rlpx/eth/transactions.rs | Uses cheap blob bundle validation during P2P “response matches request” validation. |
| crates/common/types/blobs_bundle.rs | Introduces validate_cheap() to validate blob bundle structure without running KZG verification. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for tx in &self.pooled_transactions { | ||
| if let P2PTransaction::EIP4844TransactionWithBlobs(itx) = tx { | ||
| itx.blobs_bundle.validate(&itx.tx, fork)?; | ||
| itx.blobs_bundle.validate_cheap(&itx.tx, fork)?; | ||
| } |
There was a problem hiding this comment.
Switching P2P validation from validate() to validate_cheap() means invalid KZG proofs will no longer cause validate_requested() to error, so the peer will not be disconnected in server.rs for sending bad proofs. Those invalid blobs will instead be rejected later in add_blob_transaction_to_pool() and silently ignored here, which can weaken DoS protection (a peer can repeatedly send unique/invalid blob txs and keep the connection). Consider either (a) disconnecting/penalizing the peer when mempool insertion fails specifically due to KZG verification, and/or (b) caching recently-seen invalid blob tx hashes (TTL) to avoid repeated expensive KZG work for the same invalid tx.
| let max_blobs = max_blobs_per_block(fork); | ||
| let blob_count = self.blobs.len(); | ||
|
|
||
| if blob_count > max_blobs { | ||
| return Err(BlobsBundleError::MaxBlobsExceeded); | ||
| } | ||
|
|
||
| if blob_count == 0 { | ||
| return Err(BlobsBundleError::BlobBundleEmptyError); | ||
| } | ||
|
|
||
| if self.version == 0 && fork >= Fork::Osaka || self.version != 0 && fork < Fork::Osaka { | ||
| return Err(BlobsBundleError::InvalidBlobVersionForFork); | ||
| } | ||
|
|
||
| if blob_count != self.commitments.len() | ||
| || (self.version == 0 && blob_count != self.proofs.len()) | ||
| || (self.version != 0 && blob_count * CELLS_PER_EXT_BLOB != self.proofs.len()) | ||
| || blob_count != tx.blob_versioned_hashes.len() | ||
| { | ||
| return Err(BlobsBundleError::BlobsBundleWrongLen); | ||
| }; |
There was a problem hiding this comment.
validate_cheap() duplicates most of the structural validation logic in validate(). This increases the risk that future changes update one path but not the other (e.g., fork/version rules, length checks, max blob count). Consider factoring the shared structural checks into a single internal helper (or have validate() call validate_cheap() first, then run the KZG verification) so both paths stay consistent.
There was a problem hiding this comment.
Agreed, maybe call validate_cheap inside validate?
| /// Validates blob bundle structure without expensive KZG cryptographic verification. | ||
| /// Used in P2P validation where full KZG is deferred to mempool insertion | ||
| /// (after dedup check), avoiding redundant proof verification for the same | ||
| /// blob tx received from multiple peers. | ||
| #[cfg(feature = "c-kzg")] | ||
| pub fn validate_cheap( | ||
| &self, | ||
| tx: &super::EIP4844Transaction, | ||
| fork: super::Fork, | ||
| ) -> Result<(), BlobsBundleError> { | ||
| use super::CELLS_PER_EXT_BLOB; | ||
|
|
||
| let max_blobs = max_blobs_per_block(fork); | ||
| let blob_count = self.blobs.len(); | ||
|
|
||
| if blob_count > max_blobs { | ||
| return Err(BlobsBundleError::MaxBlobsExceeded); | ||
| } | ||
|
|
||
| if blob_count == 0 { | ||
| return Err(BlobsBundleError::BlobBundleEmptyError); | ||
| } | ||
|
|
||
| if self.version == 0 && fork >= Fork::Osaka || self.version != 0 && fork < Fork::Osaka { | ||
| return Err(BlobsBundleError::InvalidBlobVersionForFork); | ||
| } | ||
|
|
||
| if blob_count != self.commitments.len() | ||
| || (self.version == 0 && blob_count != self.proofs.len()) | ||
| || (self.version != 0 && blob_count * CELLS_PER_EXT_BLOB != self.proofs.len()) | ||
| || blob_count != tx.blob_versioned_hashes.len() | ||
| { | ||
| return Err(BlobsBundleError::BlobsBundleWrongLen); | ||
| }; | ||
|
|
||
| self.validate_blob_commitment_hashes(&tx.blob_versioned_hashes)?; | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
validate_cheap() is new behavior that is now relied on by the P2P path, but there are no unit tests covering it (existing tests only exercise validate()). Add tests that assert (1) validate_cheap() rejects the same structural issues as validate() (empty bundle, wrong lengths, wrong fork/version), and (2) validate_cheap() succeeds for a bundle with correct structure but invalid proofs/blobs where validate() fails (to ensure KZG is actually skipped).
…cated structural checks. Add explicit parentheses for operator precedence clarity on the version/fork condition.
version-0 and version-1 KZG verification paths
Benchmark Block Execution Results Comparison Against Main
|
azteca1998
left a comment
There was a problem hiding this comment.
Approved, but please review copilot's comments since they address potential security concerns.
Previously, when add_blob_transaction_to_pool failed due to invalid KZG proofs, the error was silently logged and swallowed — the peer stayed connected and could keep sending invalid blobs (DoS vector). Now, BlobsBundleError is returned from PooledTransactions::handle() and caught in handle_incoming_message(), which disconnects the peer with SubprotocolError, matching the existing pattern for validate_requested and BlockRangeUpdate validation failures.
Cover the new validate_cheap() method with 6 test cases: - accepts valid blobs - rejects empty bundles - rejects wrong lengths (commitment/blob count mismatch) - rejects version/fork mismatch (version-0 on Osaka) - rejects wrong versioned hashes - passes with invalid KZG proofs (key test: structural checks pass while full validate() correctly fails on KZG verification)
Motivation
eBPF profiling on Hoodi steady-state (main branch, 5-minute capture at 49 Hz) showed ~51% of CPU samples in BLS12-381/KZG blob proof verification during P2P transaction handling. The expensive
verify_kzg_proof_batch/verify_cell_kzg_proof_batchcalls inPooledTransactions::validate_requested()run for every blob tx received from every peer, before the mempool deduplication check. When the same blob transaction is received from N peers, KZG runs N times — all but the first are redundant.Description
Add
validate_cheap()toBlobsBundlethat performs all structural checks (max blobs per block, version/fork compatibility, lengths, versioned hash SHA256 verification) but skips the KZG proof verification. Use it invalidate_requested()so the P2P layer only does cheap validation.Full KZG verification still happens in
add_blob_transaction_to_pool(), which checks for duplicates before validating KZG:This means KZG runs exactly once per unique blob transaction, regardless of how many peers send it.
eBPF profiling results (simultaneous 5-min captures, same block range)
Both servers running Hoodi on Ryzen 9 9950X3D, profiled during the same 5-minute window (same blocks, same P2P traffic):
KZG is pure CPU (BLS12-381 pairings), so the RAM difference between servers (126 GB vs 62 GB) does not affect this comparison. The remaining ~27% KZG is the irreducible minimum: each unique blob tx must be validated once at mempool insertion.
EXPB benchmark results (no regression)
Synthetic
engine_newPayloadbenchmarks on ethrex-office-5, comparing against fresh main baselines at the same commit:Fast scenario (main #253 vs ours #247):
Gigablocks scenario (main #254 vs ours #248):
All metrics within noise (<2.5%), confirming no regression in block execution performance. This is expected since the optimization only affects the P2P blob validation path, not
engine_newPayloadprocessing.Test plan