-
Notifications
You must be signed in to change notification settings - Fork 131
fix(l1): handle missing payloads in engine_getPayloadBodiesByRange #5408
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
Conversation
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 fixes a spec deviation in the engine_getPayloadBodiesByRange RPC method by changing batch-GET methods in the Store API to return None for missing payloads instead of returning database errors.
Key changes:
- Modified
read_bulk_asyncto returnVec<Option<V>>instead of failing on missing keys - Updated
get_block_bodies,get_block_bodies_by_hash, andread_fullsync_batchmethods across the storage trait and implementations - Adjusted RPC handler to work with the new signature (removing double-wrapping of Options)
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| crates/storage/api.rs | Updated trait method signatures to return Vec<Option<T>> instead of Vec<T> |
| crates/storage/store.rs | Updated method signatures to match the trait changes |
| crates/storage/store_db/in_memory.rs | Implemented the new behavior to return None for missing values instead of skipping them |
| crates/storage/store_db/rocksdb.rs | Modified read_bulk_async to return None for missing keys and updated get_block_bodies with complex logic to handle missing hashes/bodies |
| crates/networking/rpc/engine/payload.rs | Simplified the RPC handler by removing the double-wrapping of Options since the storage layer now returns the correct format |
| crates/networking/p2p/sync.rs | Added error handling to convert None values to MissingFullsyncBatch errors where missing data is unexpected during sync |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Lines of code reportTotal lines added: Detailed view |
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.
LGTM, this has been running for 3 hours without any internal error. That said we should address Copilot's comment
Oppen
left a comment
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.
Fun fact: I've been working in something related the last few days, though I didn't know if it would be correct to change the read_fullsync_batch part. Can you do the same for the GetBlockBodies RLPx message? With this change it should have the same behavior, and batching the reads should help its performance (we can use batched_multiget, BTW).
…5408) **Motivation** When we added RPC metrics for success/error rates in #5335, we found that we were returning "Internal error" in the `engine_getPayloadBodiesByRange` method. This was related to changes done in #2430, which turned missing payloads into a DB error. The spec says we should return `null`s for any missing payloads, so this deviates from the spec. **Description** This PR fixes the spec deviation by changing the batch-GET methods from the Store API to return `None` on missing values instead of failing. Closes #5403 --------- Co-authored-by: Rodrigo Oliveri <[email protected]>
Motivation
When we added RPC metrics for success/error rates in #5335, we found that we were returning "Internal error" in the
engine_getPayloadBodiesByRangemethod. This was related to changes done in #2430, which turned missing payloads into a DB error. The spec says we should returnnulls for any missing payloads, so this deviates from the spec.Description
This PR fixes the spec deviation by changing the batch-GET methods from the Store API to return
Noneon missing values instead of failing.Closes #5403