fix(anvil, evm): gas accounting in debug_traceTransaction #3230
Conversation
shekhirin
force-pushed
the
geth-debug-trace-gas
branch
from
2312bab to
f51fc49
Compare
shekhirin
force-pushed
the
geth-debug-trace-gas
branch
from
f51fc49 to
5f72aa3
Compare
|
ok, only jump opcodes gas is due to be fixed |
|
ok, gas seems to be fixed in bluealloy/revm#222 + I refactored the gas counting into inspector, so we (and other users) could re-use it in other places (e.g. https://github.com/foundry-rs/foundry/blob/master/evm/src/executor/inspector/debugger.rs) since gas blocks make it a bit complicated.
|
shekhirin
changed the title
|
@mattsse this is ready for review but bluealloy/revm#222 isn't merged yet, so I marked this PR as |
|
|
||
| /// The [`revm::Inspector`] used when transacting in the evm | ||
| #[derive(Debug, Clone, Default)] | ||
| pub struct Inspector { | ||
| pub gas: Option<Arc<RwLock<GasInspector>>>, |
There was a problem hiding this comment.
I see this was added in the revm PR
why do we need to guard this via RwLock here?
all Inspector functions are mut
ah is this because GasInspector is itself a Inspector, that is also used by the Tracer inspector?
since all of them are executed in sequence, there can be no race condition, so perhaps this could be a Rc<Refcell<>>?
or we move the GasInspector inside the Tracer, but then we don't have a standalone GasInspector
there's probably not much overhead in gas_inspector.write but could add up since this is called per step
There was a problem hiding this comment.
but then we don't have a standalone GasInspector
yep, I wanted to call it in a more explicitly way without behaviour hidden in Tracer + that way it can be shared between Inspector (I suspect we might need it in the Debugger later)
since all of them are executed in sequence, there can be no race condition, so perhaps this could be a Rc<Refcell<>>?
agree, fixed
shekhirin
force-pushed
the
geth-debug-trace-gas
branch
from
6703e39 to
227d4e7
Compare
shekhirin
force-pushed
the
geth-debug-trace-gas
branch
from
227d4e7 to
9d49e39
Compare
shekhirin
changed the title
|
@shekhirin should we be using the new GasInspector in other areas of the foundry code? |
|
gotcha! |
…#3230) * fix(anvil, evm): gas accounting in debug_traceTransaction * checked sub for previous pc * use revm gas inspector * bump ethers * bump revm * Arc<RwLock<_>> -> Rc<RefCell<_>> for GasInspector * bump revm
Motivation
#3206
Solution
GasInspectorwas introduced to revm, so we can use it to count gas easier and in a less error-prone way.