EVM runtime: implement GetBytecode and GetStorageAt getters. #631

raulk · 2022-09-09T19:50:45Z

GetBytecode returns the CID of the bytecode block. In the future, the FVM's reachability analysis should add this CID to the reachable set of the caller. GetBytecode is publicly callable.
GetStorageAt returns the value stored in the supplied EVM storage key. This method is private and only intended for calls from instrumentation and client functionality, such as the eth_getStorageAt JSON-RPC API operation. In order to make it private, it validates that the caller is the zero address. It needs to be private to preserve EVM guarantees: no contract is able to arbitrarily access storage keys from another contract. The client must form a message from the 0x0 sender address for this method to work.

Supports filecoin-project/ref-fvm#838
Supports filecoin-project/ref-fvm#854

GetBytecode returns the CID of the bytecode block. In the future, the FVM's reachability analysis should add this CID to the reachable set of the caller. GetBytecode is publicly callable. GetStorageAt returns the value stored in the supplied EVM storage key. This method is private and only intended for calls from instrumentation and client functionality, such as the eth_getStorageAt JSON-RPC API operation. In order to make it private, it validates that the caller is the zero address. It needs to be private to preserve EVM guarantees: no contract is able to arbitrarily access storage keys from another contract. The client must form a message from the 0x0 sender address for this method to work.

raulk · 2022-09-09T19:52:07Z

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

+    let output_usize = (output_size.bits() < 32)
+        .then(|| output_size.as_usize())
+        .ok_or(StatusCode::InvalidMemoryAccess)?;
+


This was a nit from a previous review that stuck in my local copy. This is more idiomatic.

Putting on my pedantic hat...

then is usually used in place of:

if foo { Some(1+2) } else { None } // easier to read as `foo.then(||1+2)`

In cases like this, I'd usually write:

if output_size.bits() >= 32 { return Err(StatusCode::InvalidMemoryAccess); }

... taking off my pedantic hat, this code is perfectly fine any which way.

i think the original code was cleaner here, you ar going overboard with scala-like code!

I am gonna rewrite this to not be like scale, but like steven.

fixed in 69462dd, the scala-like incantation hurt my eyes. Let's not declare war on if!

Stebalien

Very clean (with a test!).

Stebalien · 2022-09-10T01:48:33Z

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

+    let output_usize = (output_size.bits() < 32)
+        .then(|| output_size.as_usize())
+        .ok_or(StatusCode::InvalidMemoryAccess)?;
+


Putting on my pedantic hat...

then is usually used in place of:

if foo { Some(1+2) } else { None } // easier to read as `foo.then(||1+2)`

In cases like this, I'd usually write:

if output_size.bits() >= 32 { return Err(StatusCode::InvalidMemoryAccess); }

... taking off my pedantic hat, this code is perfectly fine any which way.

Stebalien · 2022-09-10T01:50:59Z

actors/evm/src/lib.rs

+        // load the storage HAMT
+        let mut hamt =
+            Hamt::<_, _, U256>::load(&state.contract_state, blockstore).map_err(|e| {
+                ActorError::illegal_state(format!(
+                    "failed to load storage HAMT on invoke: {e:?}, e"
+                ))
+            })?;
+
+        let mut system = System::new(rt, &mut hamt).map_err(|e| {
+            ActorError::unspecified(format!("failed to create execution abstraction layer: {e:?}"))
+        })?;


Looks like we need to do this a lot. Helper?

we should probably make one, but that's out of scope here.

I would like to do a general cleanup/beatify pass at some point.

btw, feel free to go wild and apply bigclaw magic in the code when there is a lull.

vyzo

nice tests!

vyzo · 2022-09-10T05:00:06Z

actors/evm/src/lib.rs

+    {
+        // This method cannot be called on-chain; other on-chain logic should not be able to
+        // access arbitrary storage keys from a contract.
+        rt.validate_immediate_caller_is([&fvm_shared::address::Address::new_id(0)])?;


This very funky, we have an effectively uncallable method.

Thats one extra reason to expect the client to directly read the state, or it should be a non method entry point.

Having said that, it is totally fine for now, but we should think about this general pattern.

maciejwitowski · 2022-09-10T14:22:13Z

actors/evm/src/lib.rs

+        BS: Blockstore + Clone,
+        RT: Runtime<BS>,
+    {
+        // Any caller can fetch the bytecode of a contract; this is now EXT* opcodes work.


nit: this is now -> this is how

…n-project#631) * EVM runtime: implement GetBytecode and GetStorageAt getters. GetBytecode returns the CID of the bytecode block. In the future, the FVM's reachability analysis should add this CID to the reachable set of the caller. GetBytecode is publicly callable. GetStorageAt returns the value stored in the supplied EVM storage key. This method is private and only intended for calls from instrumentation and client functionality, such as the eth_getStorageAt JSON-RPC API operation. In order to make it private, it validates that the caller is the zero address. It needs to be private to preserve EVM guarantees: no contract is able to arbitrarily access storage keys from another contract. The client must form a message from the 0x0 sender address for this method to work. * it's not scala! Co-authored-by: vyzo <vyzo@hackzen.org>

raulk force-pushed the raulk/evm-getters-ext branch from a18021d to f73b364 Compare September 9, 2022 19:54

raulk force-pushed the raulk/evm-getters-ext branch from f73b364 to 46314db Compare September 9, 2022 19:56

raulk requested review from vyzo, mriise and Stebalien September 9, 2022 20:03

raulk commented Sep 9, 2022

View reviewed changes

Stebalien approved these changes Sep 10, 2022

View reviewed changes

vyzo approved these changes Sep 10, 2022

View reviewed changes

it's not scala!

69462dd

vyzo merged commit bf3f20b into next Sep 10, 2022

vyzo deleted the raulk/evm-getters-ext branch September 10, 2022 06:41

maciejwitowski reviewed Sep 10, 2022

View reviewed changes

maciejwitowski added this to the M2.1 (r3) Copper milestone Sep 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EVM runtime: implement GetBytecode and GetStorageAt getters. #631

EVM runtime: implement GetBytecode and GetStorageAt getters. #631

raulk commented Sep 9, 2022 •

edited

Loading

raulk Sep 9, 2022

Stebalien Sep 10, 2022

vyzo Sep 10, 2022

vyzo Sep 10, 2022

vyzo Sep 10, 2022

raulk Sep 11, 2022

Stebalien left a comment

Stebalien Sep 10, 2022

Stebalien Sep 10, 2022

vyzo Sep 10, 2022

vyzo Sep 10, 2022

vyzo left a comment

vyzo Sep 10, 2022

maciejwitowski Sep 10, 2022

EVM runtime: implement GetBytecode and GetStorageAt getters. #631

EVM runtime: implement GetBytecode and GetStorageAt getters. #631

Conversation

raulk commented Sep 9, 2022 • edited Loading

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

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

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

raulk commented Sep 9, 2022 •

edited

Loading