Skip to content
This repository was archived by the owner on Nov 27, 2024. It is now read-only.

Geth support for inspect#112

Open
supragya wants to merge 12 commits into
flashbots:mainfrom
marlinprotocol:geth_additions
Open

Geth support for inspect#112
supragya wants to merge 12 commits into
flashbots:mainfrom
marlinprotocol:geth_additions

Conversation

@supragya

Copy link
Copy Markdown

Added support for geth nodes by introducing a translation layer at RPC call sites. Translates geth traces and geth receipts to parity like traces and parity like receipts. Works for analyzing arbitrages on polygon chain.

Comment thread cli.py Outdated
@supragya supragya requested a review from taarushv October 27, 2021 13:49
Comment thread cli.py Outdated
@click.option("--rpc", default=lambda: os.environ.get(RPC_URL_ENV, ""))
@click.option("--cache/--no-cache", default=True)
def inspect_block_command(block_number: int, rpc: str, cache: bool):
@click.option("--geth/--no-geth", default=False)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a way for us to derive this from the RPC directly?

would be great to plug in any RPC and have it "just work"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just talked to @sragss who had some ideas for doing that in supporting OpenEthereum

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, it seems to be possible to check whether the first RPC query failed or not and decide which method to use based on that. Then, if RPC endpoint client changes and request fails again, we could do the same thing just checking the response code every time.

OpenEthereum also supports block tracing btw: https://openethereum.github.io/JSONRPC-trace-module#trace_block

Comment thread mev_inspect/block.py Outdated
Comment thread mev_inspect/block.py Outdated
Comment thread mev_inspect/block.py Outdated
Comment thread mev_inspect/block.py Outdated
Comment thread mev_inspect/block.py Outdated
Comment thread mev_inspect/block.py
)
)
except Exception:
return []

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what causes this to get hit

@supragya supragya Dec 7, 2021

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from my experience, CREATE2. Not used in analysis

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If that's the case, want to move this to the same as you have for STATICCALL where it checks for it explicitly?

It's better on our side to have it just blow up with an exception for now. We'd rather know if we're missing cases.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Added create2
  2. Added logger warning.
    Haven't yet made it to throw exception when stuff happens since it helps us back-fill in "best effort" form. Yet, person who wants to work on this further for translation to be "exact" can remove this. Can add a comment around here though. What do you think? @lukevs

@lukevs lukevs Dec 13, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For 1, should there be a line like this?

        if tx_trace["type"] == "CREATE2":
            return []

For 2, is there anything that still makes it throw now that create2 is supported?

I'm a little hesitant to blanket accept all Exception - if there are known failures I'm ok with skipping those blocks for now, but I think we should at least limit the scope of exceptions to what we know

Otherwise for geth users they could be unknowingly missing blocks

Comment thread mev_inspect/block.py Outdated
Comment thread mev_inspect/block.py Outdated
Comment thread mev_inspect/block.py
traces = await geth_get_tx_traces_parity_format(base_provider, block_json[0])
geth_tx_receipts = await geth_get_tx_receipts_async(
base_provider.endpoint_uri, block_json[0]["transactions"]
)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these can be grouped with a gather to make in parallel

@supragya supragya Dec 7, 2021

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have seen extra load while tracing is being done hamper tracer in polygon. Sometimes the tracer runs out of time (internal timeout) and errors. Better to keep separate.

Comment thread mev_inspect/block.py Outdated
Comment thread mev_inspect/geth_poa_middleware.py
Comment thread cli.py Outdated
Comment thread mev_inspect/block.py Outdated
base_provider.endpoint_uri, block_json[0]["transactions"]
)
receipts = geth_receipts_translator(block_json[0], geth_tx_receipts)
base_fee_per_gas = 0

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there no way to fetch base fee for geth?

@supragya supragya Dec 7, 2021

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be, however, we don't use in polygon. Could be taken up in a separate PR/GH issue since it will need to be tested for the concerned chain before merge.

Comment thread cli.py Outdated
Comment thread mev_inspect/utils.py

class RPCType(Enum):
parity = 0
geth = 1

@lukevs lukevs Dec 13, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Elsewhere we do like:

class RPCType(Enum):
    parity = "parity"
    geth = "geth"

which then allows you to instantiate from the string directly

>>> RPCType("parity")
<RPCType.parity: 'parity'>

instead of custom conversion logic

@CodeForcer

CodeForcer commented May 1, 2022

Copy link
Copy Markdown

Is there any appetite for getting this merged into main? I think it's a really crucial addition in order to make inspect multichain.

EDIT: I just checked this out and the divergence from main goes deep. Maybe the crucial stuff can be pulled out, or someone more familiar with the repo than me can attempt the rebase?

@ZigaMr ZigaMr mentioned this pull request May 10, 2022
3 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants