-
Notifications
You must be signed in to change notification settings - Fork 263
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
Geth additions2 #298
base: main
Are you sure you want to change the base?
Geth additions2 #298
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.
Awesome work! Looking forward to your response, some suggestions above.
|
||
|
||
def get_base_provider(rpc: str, request_timeout: int = 500) -> Web3.AsyncHTTPProvider: | ||
def get_base_provider( |
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.
Here, rather than checking for the RPC type that's been passed as a variable, it would be beneficial to write a function somewhere that infers the RPC type, maybe from the result of a create_block attempt?
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.
I agree, will commit new changes
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.
I believe there is a cleaner way to do this - namely the web3_clientVersion RPC Call
mev
Outdated
@@ -43,7 +43,7 @@ case "$1" in | |||
redis | |||
;; | |||
listener) | |||
kubectl exec -ti deploy/mev-inspect -- ./listener $2 | |||
kubectl exec -ti deploy/mev-inspect -- ./listener $2 $3 |
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.
Having to pass the RPC type to the user will become burdensome on every command
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.
I agree, will commit new changes
mev_inspect/inspect_block.py
Outdated
|
||
logger = logging.getLogger(__name__) | ||
|
||
|
||
async def inspect_block( | ||
inspect_db_session: orm.Session, | ||
w3: Web3, | ||
type: RPCType, |
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.
rather than passing the rpc type as an argument, is it possible to retrieve a list of added middlewares from the w3 object itself, and determine the type from the presence/absence of the geth_poa_middleware?
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.
I agree, will commit new changes
base_provider = AsyncHTTPProvider(rpc, request_kwargs={"timeout": request_timeout}) | ||
base_provider.middlewares += (http_retry_with_backoff_request_middleware,) | ||
if type is RPCType.geth: | ||
base_provider.middlewares += ( |
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.
what is the advantage of adding middlewares at the provider, rather than using Web3.middleware_onion.add
or Web3(middlewares=[geth_middleware])
?
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.
What do you propose, to set middlewares at MevInspector constructor?
I'm not sure why it's set up like that, but for some reason when trying to set it up:
base_provider = AsyncHTTPProvider(rpc, request_kwargs={"timeout": request_timeout})
Web3(base_provider, modules={"eth": (AsyncEth,)}, middlewares=[geth_poa_middleware, http_retry_with_backoff_request_middleware,])
The function call
w3.provider.middlewares
returns empty set, so I'm assuming it has to be done at the provider.
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.
Interesting, not to picky about it. Were you able to get this working with a geth node and inspect any blocks?
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.
Hey, yeah it works when enabling debug "--http.api debug" on geth. With a full node I was able to start the listener at few days old blocks (had to update latest_block_update before running listener).
Listener now infers rpc type from "trace_block" rpc call, but inspect and inspect-many still require the rpc_type parameter.
So what happened to this PR? |
What does this PR do?
Continuation of Supragya Raj's work. This PR adds support for geth nodes by introducing a translation layer between geth and parity traces. Makes use of debug_traceBlockByHash RPC call so make sure to enable debug mode when starting Geth "--http.api debug".
Related issue
#112
Testing
Tested with local node on host machine, used minikube instead of kind. Tested for use with .listener (./mev listener start geth).
Checklist before merging
./mev test