Bwilson/proxyd fix headers process response#1698
Bwilson/proxyd fix headers process response#1698optimisticben wants to merge 2 commits intodevelopfrom
Conversation
🦋 Changeset detectedLatest commit: dbbb36e The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codecov Report
@@ Coverage Diff @@
## develop #1698 +/- ##
========================================
Coverage 76.52% 76.52%
========================================
Files 82 82
Lines 3041 3041
Branches 466 466
========================================
Hits 2327 2327
Misses 714 714
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
| if err != nil { | ||
| return nil, wrapErr(err, "error unmarshaling response from forward") | ||
| } | ||
| go processReponse(responseBody) |
There was a problem hiding this comment.
I think we should process the response on the existing goroutine - the overhead of spawning the new goroutine is likely higher the overhead of processing the response itself.
There was a problem hiding this comment.
My concern was slowing down the request response to the client.
The processing is pretty light right now, but I could imagine it getting heavier.
Premature optimization 😅
|
|
||
| return resB, nil | ||
| } | ||
| func processReponse(response RPCResponse) { |
There was a problem hiding this comment.
*RPCResponse to avoid copying the struct.
There was a problem hiding this comment.
I did a copy on purpose because of the go routine. Does that matter, or nah?
There was a problem hiding this comment.
Nah. You can safely pass around pointers without race conditions as long as you don't modify the data the pointer points to without a mutex.
| noneTooLowRegex := regexp.MustCompile(`nonce too low`) | ||
| gasPriceTooHighRegex := regexp.MustCompile(`gas price too high`) | ||
| gasPriceTooLowRegex := regexp.MustCompile(`gas price too low`) | ||
| invalidParametersRegex := regexp.MustCompile(`invalid parameters`) |
There was a problem hiding this comment.
This will recompile the regexes on every request. Instead, create these regexes as variables at the top of the file.
That said - if all we're doing is a substring match, then strings.Contains will likely be more efficient.
| i int64 | ||
| } | ||
|
|
||
| type RPCResponse struct { |
There was a problem hiding this comment.
I think this struct already exists - see RPCRes.
|
I recommend we merge #1679 followed by this PR. The metrics collected are different there, and we'll need to wire up the new error metrics defined here with the WS handler as well. |
## Overview Adds a path for finalizing L2 blocks, based off of the L1 block they were derived from. To accomplish this: 1. The derivation pipeline now attaches the L1 block that the L2 block was derived from, within `OpAttributesWithParent`. 2. The DA watcher actor now watches a stream of finalized L1 blocks, polled every 1m. 3. The engine actor now holds onto a map of `l1_block_number => highest_l2_block_in_epoch`, saturated when it receives attributes from the derivation actor. - When a new finalized L1 block is observed by the engine actor, it will find the highest L2 block whose batch data is contained within the finalized L1 chain, and finalize it (if it knows of any.) This approach is different than suggested by the tickets, but results in a more slim outcome that takes advantage of existing actors. ### Periphery changes - Some refactoring of the `OpAttributesWithParent` type. - The sync start algorithm now stops safe block traversal at the finalized head, to ensure it never assigns a safe block past it. - The `L1WatcherRpc` now uses a custom `BlockStream`, since alloy's `watch_blocks` stream doesn't dedup nor allow for observing anything other than head block updates. ### Result Finalized blocks are correctly streaming in 😄 <img width="1228" alt="Screenshot 2025-05-24 at 5 09 01 PM" src="https://github.com/user-attachments/assets/609184a5-35b8-4b04-a124-eeece8ef53e4" /> ### Meta closes #1693 closes #1694 closes #1695 closes #1696 closes #1698
## Overview Adds a path for finalizing L2 blocks, based off of the L1 block they were derived from. To accomplish this: 1. The derivation pipeline now attaches the L1 block that the L2 block was derived from, within `OpAttributesWithParent`. 2. The DA watcher actor now watches a stream of finalized L1 blocks, polled every 1m. 3. The engine actor now holds onto a map of `l1_block_number => highest_l2_block_in_epoch`, saturated when it receives attributes from the derivation actor. - When a new finalized L1 block is observed by the engine actor, it will find the highest L2 block whose batch data is contained within the finalized L1 chain, and finalize it (if it knows of any.) This approach is different than suggested by the tickets, but results in a more slim outcome that takes advantage of existing actors. ### Periphery changes - Some refactoring of the `OpAttributesWithParent` type. - The sync start algorithm now stops safe block traversal at the finalized head, to ensure it never assigns a safe block past it. - The `L1WatcherRpc` now uses a custom `BlockStream`, since alloy's `watch_blocks` stream doesn't dedup nor allow for observing anything other than head block updates. ### Result Finalized blocks are correctly streaming in 😄 <img width="1228" alt="Screenshot 2025-05-24 at 5 09 01 PM" src="https://github.com/user-attachments/assets/609184a5-35b8-4b04-a124-eeece8ef53e4" /> ### Meta closes #1693 closes #1694 closes #1695 closes #1696 closes #1698
4 participants
Description
Should have been 2 PRs, my bad.
Fix:
We're striping all headers from client requests, this is probably a good thing!
Geth requires the
Content-typeheader is present though (infra providers seem to let this slide, lol)Added:
Process RPC responses and create metrics based on errors.