Conversation
thinkAfCod
force-pushed
the
support_hildr
branch
from
cfc2a8a to
030b191
Compare
thinkAfCod
force-pushed
the
support_hildr
branch
from
eea6e65 to
6d51e3e
Compare
norswap
left a comment
norswap
left a comment
There was a problem hiding this comment.
Nice work! Super happy we'll get to have multiple node options :)
I think the biggest thing we need to change is to not introduce a copy of every single option, and also simply reuse the engine code as-is.
An open question: do we want to handle spinning multiple nodes / engines via a single roll-op invocation? If we do, we'll need to find a way to make the port config multi-dimensional at least.
I think this scenario is tricky enough that it makes sense to run multiple roll-op commands with slightly different config files instead.
| """ | ||
| Address to use to connect to the hildr-node RPC server ("http://127.0.0.1:11545" by default). | ||
| """ | ||
|
|
There was a problem hiding this comment.
We should not introduce separate config option, but reuse the existing one wherever possible (so after #67 is merged, l2_engine_rpc_url for instance).
| Address the L2 hildr engine metrics server should bind to ("0.0.0.0" by default). | ||
| Ignored if :py:attribute:`node_metrics` is False. | ||
| """ | ||
|
|
There was a problem hiding this comment.
Same comment as above, reuse existing options whenever possible. Totally fine to introduce new config option that Hildr has but the OP Labs version doesn't however (mention that in the comment for those options).
| self.l2_hildr_enabled = False | ||
| """ | ||
| Whether to enable Hildr service (hildr node + hildr engine — False by default). | ||
| """ |
There was a problem hiding this comment.
I think we should probably have one config option per component where there are multiple options, and enums for the options.
So for instance, l2_engine_type and l2_node_type(orl2_engine_implem`? not sure what's better).
Is Hildr node interoperable with op-geth, and op-node with Hildr engine?
|
|
||
| # Check if Node is installed and is the correct version. | ||
| if shutil.which("java") is not None: | ||
| version = lib.run("get jdk version", "java -version").replace("version", "").replace("\"", "") |
There was a problem hiding this comment.
This will throw an exception if java doesn't exist. You can get around it with the check=False option, or put this is a try statement.
There was a problem hiding this comment.
Since this just uses op-geth, do we need this at all? We should just be able to run the normal op-geth code, right?
| if config.l2_hildr_enabled: | ||
| hildr_engine.start(config) | ||
| hildr_node.start(config) | ||
|
|
There was a problem hiding this comment.
Ah, I see, you're running everything side-by-side.
We can still reuse the code (e.g. for op-geth/engine) but we'll need to setup multiple ports at least.
I wonder if a better option is not to run roll-op multiple times with different config files.
It can be run once for the OP Labs version, then a second time for the hildr version (with different ports and Hildr turned on).
Can Hildr sequence at the moment?
There was a problem hiding this comment.
Hildr not support sequencer right now, we use it as a validator with self engine.
| lib.clone_repo(github_url, "clone the hildr repository") | ||
|
|
||
| # lib.run("checkout stable version", f"git checkout --detach {git_tag}", | ||
| # cwd="hildr") |
|
|
||
| log_file = "logs/build_hildr.log" | ||
| print( | ||
| f"Starting to build the optimism repository. Logging to {log_file}\n" |
There was a problem hiding this comment.
update comment to say "hildr" instead of "optimism"
3 participants
done for #64