Skip to content
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

Open
wants to merge 26 commits into
base: main
Choose a base branch
from
Open

Geth additions2 #298

wants to merge 26 commits into from

Conversation

ZigaMr
Copy link

@ZigaMr ZigaMr commented May 10, 2022

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

  • Read the contributing guide
  • Installed and ran pre-commit hooks
  • All tests pass with ./mev test

Copy link
Contributor

@gheise gheise left a 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(
Copy link
Contributor

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?

Copy link
Author

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

Copy link

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
Copy link
Contributor

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

Copy link
Author

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


logger = logging.getLogger(__name__)


async def inspect_block(
inspect_db_session: orm.Session,
w3: Web3,
type: RPCType,
Copy link
Contributor

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?

Copy link
Author

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 += (
Copy link
Contributor

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])?

Copy link
Author

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.

Copy link
Contributor

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?

Copy link
Author

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.

@arsenius-clbs
Copy link

So what happened to this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants