Conversation
| let requested_envelopes = request.beacon_block_roots.len(); | ||
| let mut envelope_stream = match self | ||
| .chain | ||
| .get_payload_envelopes_checking_caches(request.beacon_block_roots.to_vec()) |
There was a problem hiding this comment.
Why is it necessary to have this stream gadget instead of a simple for loop?
for block_root in block_roots {
if Some(payload) = chain.get_payload_checking_caches(block_root) {
send_to_peer(payload)
}
}There was a problem hiding this comment.
We're going to use getPayloadBodiesByRange which allows us to fetch a batch of payloads in one engine api request. If we did the simple for loop we'd have to make an engine api request for each payload
…oas-serve-envelope-rpc
| rpc_block_limits_by_fork(fork_context.current_fork_name()) | ||
| } | ||
| Protocol::PayloadEnvelopesByRoot => { | ||
| rpc_block_limits_by_fork(fork_context.current_fork_name()) |
There was a problem hiding this comment.
we may need a rpc_payload_envelope_limits function and update the rpc_block_limits_by_fork to include Gloas block size limit.
There was a problem hiding this comment.
updated rpc_block_limits_by_fork and introduced rpc_payload_envelope_limits
There was a problem hiding this comment.
We still need to set rpc_block_limits_by_fork max to be bellatrix_max for historical sync compatibility
| // TODO(gloas) we'll want to use the execution layer directly to call | ||
| // the engine api method eth_getBlockByHash() | ||
| self.store | ||
| .get_payload_envelope(&beacon_block_root) |
There was a problem hiding this comment.
synchronous db read inside async task here, use a blocking task (spawn_blocking)
There was a problem hiding this comment.
added spawn_blocking. I had to introduce a new span_blocking_and_await fn inside task executor. its the same spawn_blocking wrapper we have in beacon chain. I did this because I didnt want envelope streamer to rely on beacon chain.
beacon_node/beacon_chain/src/execution_payload_envelope_streamer.rs
Outdated
Show resolved
Hide resolved
| RequestType::PayloadEnvelopesByRoot(PayloadEnvelopesByRootRequest { | ||
| beacon_block_roots: RuntimeVariableList::from_ssz_bytes( | ||
| decoded_buffer, | ||
| spec.max_request_blocks(current_fork), |
There was a problem hiding this comment.
This should be spec.max_request_payloads() to be consistent with the other fixes
jimmygchen
left a comment
There was a problem hiding this comment.
The new protocols need count validation here too, otherwise a peer can make us iterate the full chain instead of the expected max 128 slots:
| let results = match self.load_envelopes(&beacon_block_roots).await { | ||
| Ok(results) => results, | ||
| Err(e) => { | ||
| send_errors(beacon_block_roots, sender, e).await; |
There was a problem hiding this comment.
may be worth logging some errors/warn here?
…oas-serve-envelope-rpc
|
This is ready for a review. We're skipping envelope verification here, since we dont have fork choice yet. |
|
Suggestion: avoid wrapping Implement See: dapplion@3e6afae31 |
|
Also, Removed it along with 9 unused imports it pulled in: dapplion@ed8c5495f |
…oas-serve-envelope-rpc
|
Some required checks have failed. Could you please take a look @eserilev? 🙏 |
|
@dapplion your changes make sense, i went ahead and merged them. thanks! |
| use tree_hash_derive::TreeHash; | ||
|
|
||
| #[derive(Debug, Clone, Serialize, Encode, Decode, Deserialize, TestRandom, TreeHash, Educe)] | ||
| #[derive( |
There was a problem hiding this comment.
Can we create an explicit empty() method for the default case?
|
|
||
| /// Returns the maximum SSZ-encoded size. | ||
| #[allow(clippy::arithmetic_side_effects)] | ||
| pub fn max_size() -> usize { |
There was a problem hiding this comment.
might be better to have a max_size on the ExecutionPayloadEnvelope as well and use it here instead so we don't forget to update it if the spec changes
common/task_executor/src/lib.rs
Outdated
|
|
||
| /// Error type for spawning a blocking task and awaiting its result. | ||
| #[derive(Debug)] | ||
| pub enum SpawnBlockingError { |
There was a problem hiding this comment.
I'm not a fan of doing this without refactoring the spawn_blocking_handle function in beacon_chain.rs to also use this.
We'll just end up having multiple functions that do the same thing for no reason.
There was a problem hiding this comment.
this is no longer needed due to the adapter pattern change and has been removed
| // TODO(gloas) remove expect when execution layer field | ||
| // is no longer dead. | ||
| #[expect(dead_code)] | ||
| execution_layer: ExecutionLayer<T::EthSpec>, |
There was a problem hiding this comment.
Why don't we start with using the beacon chain here instead and do beacon_chain.execution_layer when we need to access it? that way we get to keep pretty similar logic to the beaocn block streamer.
We need the beacon_chain to access the db anyway
There was a problem hiding this comment.
If testing is the concern, we can take a similar approach to the fetch_blobs adapter.
Here, it seems like we need ExecutionLayer, CanonicalHead and the store all of which are present in the BeaconChain
There was a problem hiding this comment.
ive went with the adapter pattern
| Ok(Some(cached_envelope)) | ||
| } else { | ||
| // TODO(gloas) we'll want to use the execution layer directly to call | ||
| // the engine api method eth_getBlockByHash() |
There was a problem hiding this comment.
do you mean getPayloadBodiesByRange or some equivalent thing that returns in the right format for the CL?
There was a problem hiding this comment.
yes, i've updated the comment
|
went ahead with the adapter pattern, i think its the cleaner approach. thanks pawan for suggesting it. theres a bit of code churn from those changes, sorry about that... this should be ready for another round of reviews |
|
This pull request has merge conflicts. Could you please resolve them @eserilev? 🙏 |
|
🤖 Note on memory amplification (inherited pattern, not new to this PR):
(lion) However in practice the Beacon processor has limited worker threads, so not all 1024 queued items run simultaneously, on high CPU machine it may be a slight concern. The |
Issue Addressed
Serves envelope by range and by root requests. Added PayloadEnvelopeStreamer so that we dont need to alter upstream code when we introduce blinded payload envelopes.