feat: Add the ability to read the cl_node_urls from env variables #59
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
MoeMahhouk
requested review from
dvush,
ZanCorDX,
metachris and
jakubhruby7
as code owners
Benchmark results for
|
| Date (UTC) | 2024-07-29T09:26:47+00:00 |
| Commit | d72ead8479f636064bd4ba5fe0cd817c98538029 |
| Base SHA | bda8468f17a459d7238a237f6a71c28c13ca1c24 |
Significant changes
| Benchmark | Mean | Status |
|---|---|---|
| MEV-Boost SubmitBlock serialization/JSON encoding | 11.49% | Performance has degraded. |
| MEV-Boost Sign block for relay/Capella | 10.48% | Performance has degraded. |
| MEV-Boost Sign block for relay/Deneb | 11.56% | Performance has degraded. |
MoeMahhouk
force-pushed
the
env-var-cl-node
branch
from
4319bfd
to
1d80972
Compare
|
I have ran the make tests too and all green besides one of the tests is failing. |
|
This looks like a copy/paste from EnvOrInplaceValue, can you try to refactor it? |
ZanCorDX
approved these changes
Jul 17, 2024
|
@MoeMahhouk could you rebase please? |
## 📝 Summary More comments on: - bin - backtest - utils - primitives More will come in following PRs.
#38) ## 📝 Summary The current use of Mutex on `nonce_cache` in `filter_order_by_nonces` is not correct. It causes the function to run sequentially because every other task will wait for prev one ( both read and write ) complete in order to do its work. I change to use RwLock instead. Before the change it takes 18m to fetch a block with 2k4 orders, and now it takes 18s with 400 concurrent request. - And lint the file ## 💡 Motivation and Context <!--- (Optional) Why is this change required? What problem does it solve? Remove this section if not applicable. --> --- ## ✅ I have completed the following steps: * [x] Run `make lint` * [x] Run `make test` * [ ] Added tests (if applicable)
This PR adds a small abstraction over the beacon client api and implements the methods required by the rbuilder. There are a couple of unit tests that require a running beacon node. They are ignore right now but we will enable them in a followup PR. <!--- (Optional) Why is this change required? What problem does it solve? Remove this section if not applicable. --> --- * [x] Run `make lint` * [x] Run `make test` * [x] Added tests (if applicable)
## 📝 Summary Builds on top of #13. This PR moves the initialisation of the mempool dumpster to the mempool dumpster datasource itself. Before, it was on the fetch command. This also enables a unit test that was ignored before. --- ## ✅ I have completed the following steps: * [x] Run `make lint` * [x] Run `make test` * [x] Added tests (if applicable)
## 📝 Summary This PR replaces the custom types in the MevBoost client with the counterparts in the alloy library. --- ## ✅ I have completed the following steps: * [x] Run `make lint` * [x] Run `make test` * [x] Added tests (if applicable)
## 📝 Summary Builds on top of #34. This PR removes the flag `cl_network_config_dir` which is used to load the consensus network spec to derive the builder signing domain. Instead, it uses the beacon clients to retrieve the required fields. --- ## ✅ I have completed the following steps: * [x] Run `make lint` * [x] Run `make test` * [x] Added tests (if applicable)
## 📝 Summary For merged sbundles, we were wrongly reporting as included failed or fully dropped subbundles. A fully dropped sub bundle is a bundle with all droppable children (TxRevertBehavior::AllowedExcluded) that when executed dropped everything so no tx was included. ## 💡 Motivation and Context We saw some weird stuff on our DB. ## ✅ I have completed the following steps: * [x] Run `make lint` * [x] Run `make test` * [x] Added tests (if applicable) --------- Co-authored-by: Ryan Schneider <[email protected]>
## 📝 Summary The test has some race condition and fails from time to time. ## 💡 Motivation and Context I love making PRs and want to break the Guinness record of PR making.
## 📝 Summary Missing corrections I missed to add on a previous PR (#53). --------- Co-authored-by: Alejo Salles <[email protected]>
## 📝 Summary alloy dependencies changed from 0.1 to 0.2 ## 💡 Motivation and Context Newer dependencies help to build around the rbuilder with fewer problems. Also 0.2 comes on blue which highlights my eyes. ## ✅ I have completed the following steps: * [x] Run `make lint` * [x] Run `make test` * [ ] Added tests (if applicable)
## 📝 Summary Have `rbuilder` correctly return the transaction hash in response to `eth_sendRawTransaction`. ## 💡 Motivation and Context <!--- (Optional) Why is this change required? What problem does it solve? Remove this section if not applicable. --> I noticed in local testing that `cast send` always returns null. NOTE: the same is true of the other RPC methods `eth_sendBundle` etc. but I figured I'd leave those as a Good First Issue for others. --- ## ✅ I have completed the following steps: * [x] Run `make lint` * [x] Run `make test` * [ ] Added tests (if applicable)
There were situations where we had merged sbundles that looked like this bundle1: user_tx, allow_revert: no backrun1 bundle 2: user_tx: allow_revert yes backrun merged sbundle is concat of these two and consistency check would think that user_tx wat not allowed to revert enven though there is a situation where we include bundle2
metachris
reviewed
Jul 29, 2024
| /// We ONLY use this special flow for orders with some particular structure (see [`ShareBundleMerger::break_down_bundle`]). | ||
| /// SBundle not complying with that will pass through and use the standard handling. | ||
| /// @Pending evaluate if we can avoid to send changes no every order update and "flush" changes all together (since orders are usually processed on batches) | ||
| /// Merging example: SBundle(Bundle(tx)+br1) + SBundle(Bundle(tx)+br2) will become SBundle(Bundle(Bundle(tx)+br1),Bundle(Bundle(tx)+br2)). |
There was a problem hiding this comment.
why adding these spaces?
There was a problem hiding this comment.
When I rebased and solved the conflicts. The lint pipeline blocked the merging due to those errors.
Here you can see them
https://github.com/flashbots/rbuilder/actions/runs/10141455345/job/28038732480
PS. interesting though that those files were not introduced by my PR but still blocked my PR to merge.
Also the make lint locally didn't show me to fix them. But I did them manually just to make the PR to progress forward
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
📝 Summary
This PR adds the ability to parse the cl_node_url passed from an environment variable too.
It would help with the automatic deployment in the TDX yocto images.
This addition does not affect the original behaviour of accepting a single value or an array of values. It adds extra a way to pass the values from an env variable where the values are separated by a comma from a single string.
💡 Motivation and Context
The motivation behind this change was triggered when preparing the TDX demo with rbuilder and the rbuilder was crashing because we were passing the CL_NODE_URL as environment variables during deployment from the cloud init conf.
This lead us to hardcode it first just for the sake of the demo but I believe it would be better to allow this to be passed and fetched from an environment parameter too similar to the secrets configs.
✅ I have completed the following steps:
make lintmake test