-
Notifications
You must be signed in to change notification settings - Fork 162
fix(l1): only rebuild payload when mempool receives new transactions (Notify) #6169
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
base: main
Are you sure you want to change the base?
Conversation
new transactions instead of rebuilding continuously. Each rebuild is CPU-intensive (EVM execution, trie operations) and contends with RPC handlers over the shared trie_cache mutex, starving the RPC server. Mempool::add_transaction() calls notify_one() after inserting a tx. build_payload_loop uses tokio::select! to wait for either a mempool notification or cancel_token cancellation before each rebuild. The first build_payload call remains unconditional.
🤖 Kimi Code ReviewReview SummaryThe PR adds transaction notification functionality to the mempool to improve payload building efficiency. Overall, the changes are well-structured and follow Rust best practices. Issues Found1. Race Condition in Payload Building (payload.rs:392-400)
Fix: Consider adding a check after waking to ensure there's actually work to do, or use a different synchronization pattern. 2. Missing Error Handling (payload.rs:394-400) Fix: Add proper error handling: tokio::select! {
_ = self.mempool.tx_added().notified() => {
match self.build_payload(payload.clone()) {
Ok(new_payload) => res = new_payload,
Err(e) => {
tracing::warn!("Failed to build payload: {}", e);
continue;
}
}
}
_ = cancel_token.cancelled() => break,
}3. Potential Deadlock (mempool.rs:132-134) {
let mut inner = self.write()?;
// ... insert operations ...
// lock is dropped here at end of scope
}
self.tx_added.notify_one();Minor Suggestions1. Documentation Needed
2. Consider Notification Coalescing
3. Test Coverage
Security AssessmentNo security vulnerabilities identified. The changes maintain proper lock ordering and don't introduce new attack vectors. Performance ImpactThe changes should improve performance by reducing busy-waiting in the payload builder, though the exact impact depends on transaction arrival patterns. Automated review by Kimi (Moonshot AI) · custom prompt |
🤖 Claude Code ReviewNow I have a comprehensive understanding of the PR. Let me write my review. Code Review: PR #6169 - Payload Building Optimization with
|
🤖 Codex Code ReviewIssue found (most important first)
Other notes
Automated review by OpenAI Codex · custom prompt |
Lines of code reportTotal lines added: Detailed view |
pre-capture the notified() future before select! for correctness, and add a slot deadline timeout so the loop exits even if no transactions arrive and the cancel token is not triggered.
🤖 Kimi Code ReviewReview SummaryThe PR introduces a notification mechanism to avoid busy-looping in the payload builder when waiting for new transactions. Overall, the implementation is sound, but there are a few issues to address: Issues Found
Suggested Changes// In payload.rs, line 396-403
// Replace:
let notified = self.mempool.tx_added().notified();
tokio::select! {
_ = notified => {}
_ = cancel_token.cancelled() => break,
_ = tokio::time::sleep(remaining) => break,
}
// With:
tokio::select! {
_ = self.mempool.tx_added().notified() => {}
_ = cancel_token.cancelled() => break,
_ = tokio::time::sleep(remaining) => break,
}# In blockchain/Cargo.toml, line 25
# Change:
tokio = { workspace = true, features = ["time", "rt", "sync", "macros"] }
# To:
tokio = { workspace = true, features = ["time", "rt", "sync"] }The implementation correctly uses Automated review by Kimi (Moonshot AI) · custom prompt |
🤖 Codex Code ReviewFound a couple of correctness/perf concerns to address.
Everything else in the diff looks reasonable: lock drop before notify is good, and the additional If you want, I can suggest a small, idiomatic Automated review by OpenAI Codex · custom prompt |
🤖 Claude Code ReviewPerfect! Now I have a comprehensive understanding. Let me create the review: Code Review: PR #6169 - Notify-based Payload Rebuild OptimizationSummaryThis PR adds ✅ Strengths
🔍 Potential Issues & Suggestions1. Missing Notification on Transaction Removal (Minor)Location: The Consideration: This might be intentional since removals don't add new revenue opportunities. However, document this behavior if it's by design. 2. Blobs Bundle Addition Doesn't Notify (Minor)Location: The pub fn add_blobs_bundle(&self, tx_hash: H256, blobs_bundle: BlobsBundle) -> Result<(), StoreError> {
self.write()?.blobs_bundle_pool.insert(tx_hash, blobs_bundle);
// No notification here - is this intentional?
Ok(())
}Question: Should blob bundle additions trigger a rebuild? If blobs can arrive after the transaction, the payload builder might build an incomplete block. 3. Notify Semantics Under Concurrent AdditionsLocation: Using
This is true because Recommendation: Add a comment explaining this permit-based behavior for future maintainers: // notify_one() stores a single permit, so if multiple txs arrive
// during a build, the next notified().await resolves immediately.
// This is optimal: one rebuild can incorporate all pending txs.
self.tx_added.notify_one();4. Race Condition: Transaction Arrives During Clone (Low Risk)Location: let payload = payload.clone();
let self_clone = self.clone();
let building_task = tokio::task::spawn_blocking(move || self_clone.build_payload(payload));If a transaction arrives between when Status: Acceptable tradeoff for code simplicity. 5. Error Handling in Build Loop (Observation)Location: If Recommendation: Consider metrics/logging for build failures to detect systematic issues. 🔒 Security Considerations✅ No Security Issues Found
🎯 Ethereum-Specific Correctness✅ Consensus Rules
|
| Aspect | This PR (Notify) | PR #6165 (AtomicU64) |
|---|---|---|
| Wake latency | Instant | Up to 100ms |
| Dependencies | Adds sync & macros features |
No new dependencies |
| Complexity | Slightly higher | Simpler |
| Performance | Better | Good enough |
Verdict: The instant wake-up is worth the additional tokio features.
🧪 Testing Recommendations
-
Add integration test for notification behavior:
- Verify payload is rebuilt when transaction arrives
- Verify loop exits after slot deadline even with no transactions
- Test cancellation token behavior
-
Stress test concurrent transaction arrivals during payload building
-
Benchmark RPC latency improvement under load
✅ Rust Best Practices
- Idiomatic async: Correct use of
tokio::select! - Lifetime management: Proper reference handling with
&self.tx_added - Error handling: Consistent
Resulttypes - Documentation: Good inline comments (could add more as suggested)
📝 Final Verdict
Approve with minor suggestions
The implementation is sound and addresses the performance issue effectively. The recent feedback incorporation (pre-capture notified, explicit drop, timeout) shows careful attention to correctness.
Required Changes: None
Recommended Improvements:
- Add comment explaining
notify_one()permit semantics - Document why
remove_transaction()andadd_blobs_bundle()don't notify - Consider metrics for build failures
- Add integration tests for notification behavior
The code is production-ready and superior to the alternative polling approach for this use case.
Automated review by Claude (Anthropic) · custom prompt
Greptile OverviewGreptile SummaryThis PR optimizes the block building loop by replacing continuous rebuilding with event-driven rebuilding using Key Changes
Technical ApproachThe implementation correctly:
Performance ImpactThis eliminates unnecessary CPU-intensive rebuilds (EVM execution, trie operations) that were previously happening continuously during the 12-second slot window, reducing contention on the shared Confidence Score: 5/5
|
| Filename | Overview |
|---|---|
| crates/blockchain/mempool.rs | Added tokio::sync::Notify field with proper initialization and notification after transaction insertion; correctly drops lock before notifying |
| crates/blockchain/payload.rs | Replaced busy-loop with tokio::select! that waits for new transactions, cancellation, or slot deadline; properly pre-captures notified() future before select |
| crates/blockchain/Cargo.toml | Added required sync and macros features to tokio dependency for Notify and select! macro support |
Sequence Diagram
sequenceDiagram
participant RPC as Engine RPC
participant Builder as Payload Builder Loop
participant Mempool as Mempool
participant P2P as P2P Network
Note over RPC,Builder: FCU arrives, payload build starts
RPC->>Builder: initiate_payload_build(payload)
activate Builder
Builder->>Builder: build_payload() (first build)
loop Until slot deadline (12s) or cancellation
Builder->>Mempool: tx_added().notified()
Note over Builder: Wait for notification or timeout
alt New transaction arrives
P2P->>Mempool: add_transaction(tx)
Mempool->>Mempool: Insert tx into pool
Mempool->>Mempool: drop(write_lock)
Mempool->>Builder: notify_one()
Builder->>Builder: build_payload() (rebuild)
else Cancellation requested
RPC->>Builder: cancel_token.cancel()
Builder->>Builder: break loop
else Slot deadline reached
Builder->>Builder: sleep(remaining) timeout
Builder->>Builder: break loop
end
end
Builder-->>RPC: PayloadBuildResult
deactivate Builder
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.
Pull request overview
This PR reduces CPU usage and contention during L1 payload building by making the payload rebuild loop event-driven: it now waits for mempool transaction insertions (via tokio::sync::Notify) instead of continuously rebuilding for the entire slot.
Changes:
- Add a
tokio::sync::NotifytoMempooland signal it on every transaction insertion. - Update
build_payload_looptoselect!on mempool notifications, cancellation, or slot deadline before triggering rebuilds. - Enable additional Tokio features (
sync,macros) required forNotifyandtokio::select!.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| crates/blockchain/mempool.rs | Adds a Notify to signal transaction insertions to the payload builder. |
| crates/blockchain/payload.rs | Makes the payload rebuild loop wait for mempool events/cancellation/deadline before rebuilding. |
| crates/blockchain/Cargo.toml | Enables Tokio sync + macros features to support Notify and select!. |
| crates/vm/levm/bench/revm_comparison/Cargo.lock | Locks additional Tokio macro dependency introduced by feature changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Benchmark Results ComparisonNo significant difference was registered for any benchmark run. Detailed ResultsBenchmark Results: BubbleSort
Benchmark Results: ERC20Approval
Benchmark Results: ERC20Mint
Benchmark Results: ERC20Transfer
Benchmark Results: Factorial
Benchmark Results: FactorialRecursive
Benchmark Results: Fibonacci
Benchmark Results: FibonacciRecursive
Benchmark Results: ManyHashes
Benchmark Results: MstoreBench
Benchmark Results: Push
Benchmark Results: SstoreBench_no_opt
|
builders, and also notify after add_blobs_bundle() so builders that woke from add_transaction() for a blob tx see the complete bundle data.
Motivation
The block building loop (
build_payload_loop) continuously rebuilds the block for 12 seconds (one slot), even when no new transactions have arrived. Each rebuild is CPU-intensive (EVM execution, trie operations) and contends with RPC handlers over the sharedtrie_cachemutex, starving the RPC server.Description
Alternative approach to #6165 — uses
tokio::sync::Notifyinstead of anAtomicU64generation counter.Add a
tokio::sync::NotifytoMempoolthat signals when new transactions or blobs bundles are inserted. The payload build loop waits on this notification (or cancellation, or the slot deadline) before rebuilding, instead of looping as fast as possible.Mempool::add_transaction()andMempool::add_blobs_bundle()both callnotify_waiters()after inserting data, so builders see complete blob tx data and all active payload builders are wokenbuild_payload_loopusestokio::select!to wait for a mempool notification,cancel_tokencancellation, or the remaining slot deadline — whichever comes firstnotified()future is pre-captured beforeselect!to avoid missing permits between loop iterationsbuild_payloadcall remains unconditional — we always build one payload immediately when FCU arrivestx_added()ispub(crate)to avoid exposing the notification primitive externallyTradeoffs vs #6165 (AtomicU64 polling):
syncandmacrostokio features to the blockchain crateChecklist
STORE_SCHEMA_VERSION(crates/storage/lib.rs) if the PR includes breaking changes to theStorerequiring a re-sync.