EVM: fix EXTCODE_ ops #870

mriise · 2022-11-24T02:15:57Z

codecov-commenter · 2022-11-24T02:38:08Z

Codecov Report

Merging #870 (95d4db2) into next (5a4b15b) will increase coverage by 0.12%.
The diff coverage is 93.91%.

❗ Current head 95d4db2 differs from pull request most recent head 9026da0. Consider uploading reports for the commit 9026da0 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             next     #870      +/-   ##
==========================================
+ Coverage   87.10%   87.23%   +0.12%     
==========================================
  Files         126      127       +1     
  Lines       23278    23677     +399     
==========================================
+ Hits        20276    20654     +378     
- Misses       3002     3023      +21

Impacted Files	Coverage Δ
actors/market/src/ext.rs	`100.00% <ø> (ø)`
actors/paych/src/state.rs	`100.00% <ø> (ø)`
actors/verifreg/src/types.rs	`100.00% <ø> (ø)`
runtime/src/actor_error.rs	`76.74% <ø> (+2.32%)`	⬆️
actors/power/src/state.rs	`83.49% <77.27%> (+4.73%)`	⬆️
actors/evm/src/interpreter/instructions/mod.rs	`81.48% <81.25%> (-0.96%)`	⬇️
actors/miner/src/lib.rs	`83.26% <84.94%> (-0.16%)`	⬇️
runtime/src/test_utils.rs	`81.35% <85.71%> (+0.19%)`	⬆️
actors/evm/src/interpreter/execution.rs	`86.36% <87.50%> (-2.17%)`	⬇️
actors/evm/src/interpreter/instructions/call.rs	`83.25% <87.50%> (-0.32%)`	⬇️
... and 37 more

mriise · 2022-11-28T23:45:09Z

just need to fix tests

mriise · 2022-11-29T03:01:00Z

tests implemented! review time :)

actors/evm/src/interpreter/instructions/ext.rs

Stebalien · 2022-11-29T04:58:22Z

actors/evm/src/interpreter/instructions/ext.rs

    let actor_id = rt.resolve_address(&addr).ok_or_else(|| {
-        StatusCode::InvalidArgument("failed to resolve address".to_string())
+        StatusCode::InvalidArgument("EVM EXT opcode failed to resolve address".to_string())
        // TODO better error code
    })?;


This should return empty bytecode.

That is, by definition, all actors that don't exist have empty bytecode (in the EVM).

actors/evm/src/interpreter/instructions/ext.rs

vyzo

What are all these unrelated changes?

Please remove them from this pr.

vyzo

the relevant parts look ok at first glance however.

actors/evm/src/interpreter/instructions/call.rs

vyzo · 2022-11-30T10:29:26Z

Also, and this is important, please dont change toolchain like this in a totally irrelevant pr.

This is an important change that should be its own pr.

…sserting for better debugging help

- address todo - use the new get_cid_type

Stebalien

Couple of small nits remaining. I made some larger changes because it was difficult to review this patch due to the added indentation and deep indentation (see the changes I pushed).

Stebalien · 2022-12-01T04:11:50Z

actors/evm/src/interpreter/instructions/ext.rs

    let raw_bytecode = rt
        .store()
-        .get(cid) // TODO this is inefficient; should call stat here.
+        .get(&cid) // TODO this is inefficient; should call stat here.


This is fine. Note: we don't actually ever expect this error as it should be impossible.

Stebalien · 2022-12-01T04:11:59Z

actors/evm/src/interpreter/instructions/ext.rs

+pub fn get_cid_type(rt: &impl Runtime, addr: U256) -> CodeCid {
+    let addr: EthAddress = addr.into();
+
+    addr.try_into()


I flattened this to make it easier to read.

actors/evm/src/interpreter/instructions/ext.rs

Stebalien · 2022-12-01T04:18:21Z

actors/evm/src/interpreter/instructions/call.rs

+                        )
+                    }
+                    // If we're calling an account or a non-existent actor, return nothing because
+                    // this is how the EVM behaves.


I changed this to return "ok" in these cases.

Stebalien · 2022-12-01T04:19:22Z

actors/evm/src/interpreter/instructions/call.rs

-                        TokenAmount::from(&value),
-                    )
-                }
+                CallKind::DelegateCall => match get_cid_type(system.rt, dst) {


I removed the closure/return. We didn't need them.

Generally speaking, jumping like that is a "last resort" kind of thing as it can make code really hard to follow. It also increases diffs by changing indent.

yeah diff was gross for sure. In all honesty this method ended up a lot cleaner than i was thinking (idk what i was thinking tbh). I probably should've given it at least a few minutes of poking before dismissing it as unfeasible.

addressed

vyzo

The important parts look good now, but we probably want to address steven's remarks before merging.

vyzo · 2022-12-01T06:15:28Z

actors/evm/src/interpreter/instructions/ext.rs

-        .map(|bytecode| bytecode.len())?;
+    let len = match get_cid_type(system.rt, addr) {
+        CodeCid::EVM(addr) => get_evm_bytecode(system.rt, &addr).map(|bytecode| bytecode.len())?,
+        CodeCid::Native(_) => 1,


ah, 0xfe below.

yes, but has more to do with playing nicely with solidity's behavior of isContract

actors/evm/src/interpreter/instructions/ext.rs

vyzo · 2022-12-01T15:54:46Z

Can we wait for #885 before merging?

vyzo · 2022-12-01T15:55:33Z

I mean we already have conflicts, and it will certainly create more with 885.

* wip fixes * cleanup implementation * return 0 for non existient address in extcodesize * use enum instead of result result maddness * fmt & cleanup some logic * update tests for the remaining two ext opcodes * redo logic to make things a bit more clearn * fix bugs & keep bytecode logic seperate, verify expectations before asserting for better debugging help * add another test case for ext op * use closure for early return * nit: reduce indentation * fix: formatting * remove unnecessary closure * fix: make delegatecall behave more like the EVM - address todo - use the new get_cid_type * fix some review nits Co-authored-by: Steven Allen <steven@stebalien.com>

mriise changed the title ~~wip fixes~~ EVM: fix EXTCODE_ ops Nov 24, 2022

mriise requested review from Stebalien and raulk November 28, 2022 23:44

mriise marked this pull request as ready for review November 29, 2022 00:29

Stebalien requested changes Nov 29, 2022

View reviewed changes

Stebalien requested a review from vyzo November 30, 2022 02:29

mriise force-pushed the fix/codesize-op branch from 3df8736 to abe04a7 Compare November 30, 2022 03:09

vyzo previously requested changes Nov 30, 2022

View reviewed changes

vyzo reviewed Nov 30, 2022

View reviewed changes

actors/evm/src/interpreter/instructions/call.rs Outdated Show resolved Hide resolved

mriise added 10 commits November 30, 2022 13:23

wip fixes

d351a09

cleanup implementation

e0951d2

return 0 for non existient address in extcodesize

0701568

use enum instead of result result maddness

c3b7bcf

fmt & cleanup some logic

a5bfb4e

update tests for the remaining two ext opcodes

ea3f6e2

redo logic to make things a bit more clearn

c383f1d

fix bugs & keep bytecode logic seperate, verify expectations before a…

2e7529a

…sserting for better debugging help

add another test case for ext op

107e2a7

use closure for early return

4952b9b

mriise force-pushed the fix/codesize-op branch from 89bf664 to 4952b9b Compare November 30, 2022 22:16

Stebalien added 4 commits November 30, 2022 19:34

nit: reduce indentation

95d4db2

fix: formatting

9026da0

remove unnecessary closure

93ee901

fix: make delegatecall behave more like the EVM

499a329

- address todo - use the new get_cid_type

Stebalien approved these changes Dec 1, 2022

View reviewed changes

vyzo reviewed Dec 1, 2022

View reviewed changes

fix some review nits

363222e

Stebalien approved these changes Dec 1, 2022

View reviewed changes

Merge branch 'next' into fix/codesize-op

dd16451

Stebalien enabled auto-merge (squash) December 1, 2022 18:28

Stebalien merged commit 49f927c into next Dec 1, 2022

Stebalien deleted the fix/codesize-op branch December 1, 2022 18:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EVM: fix EXTCODE_ ops #870

EVM: fix EXTCODE_ ops #870

mriise commented Nov 24, 2022

codecov-commenter commented Nov 24, 2022 •

edited

Loading

mriise commented Nov 28, 2022

mriise commented Nov 29, 2022

Stebalien Nov 29, 2022

Stebalien Nov 29, 2022

vyzo left a comment •

edited

Loading

vyzo left a comment

vyzo commented Nov 30, 2022 •

edited

Loading

Stebalien left a comment

Stebalien Dec 1, 2022

Stebalien Dec 1, 2022

mriise Dec 1, 2022

Stebalien Dec 1, 2022

Stebalien Dec 1, 2022

mriise Dec 1, 2022

vyzo left a comment

vyzo Dec 1, 2022

vyzo Dec 1, 2022

mriise Dec 1, 2022

vyzo commented Dec 1, 2022

vyzo commented Dec 1, 2022

EVM: fix EXTCODE_ ops #870

EVM: fix EXTCODE_ ops #870

Conversation

mriise commented Nov 24, 2022

codecov-commenter commented Nov 24, 2022 • edited Loading

Codecov Report

mriise commented Nov 28, 2022

mriise commented Nov 29, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vyzo left a comment • edited Loading

Choose a reason for hiding this comment

vyzo left a comment

Choose a reason for hiding this comment

vyzo commented Nov 30, 2022 • edited Loading

Stebalien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vyzo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vyzo commented Dec 1, 2022

vyzo commented Dec 1, 2022

codecov-commenter commented Nov 24, 2022 •

edited

Loading

vyzo left a comment •

edited

Loading

vyzo commented Nov 30, 2022 •

edited

Loading