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

FEVM | Howto access cross contract pre-compile functions. #988

Closed
shamb0 opened this issue Dec 26, 2022 · 13 comments
Closed

FEVM | Howto access cross contract pre-compile functions. #988

shamb0 opened this issue Dec 26, 2022 · 13 comments
Assignees

Comments

@shamb0
Copy link
Contributor

shamb0 commented Dec 26, 2022

Issues Related

Discussion Thread ...

Bit blocked in the bring-up of test-suite push0.json. Use-case trying to invoke functions between contracts. To be specific looking for some clarity on how "CALL(0xF1)" works under FEVM context. Tried my best to mimic the test app implementation from revme, but no luck. Captured the traces from revme & FEVM context.

Use-Case

https://github.com/ethereum/tests/blob/b3d820f4488e2b49674546269a176db021ee4249/GeneralStateTests/EIPTests/stEIP3855/push0.json#LL157C18-L157C18

Draft PR

Contract-01

Test Case

 "0x0000000000000000000000000000000000000100" : {
                "balance" : "0x00",
                "code" : "0x60015f55",
                "nonce" : "0x00",
                "storage" : {
                }
            },

Decompiled ASM code

label_0000:
	0000    60  PUSH1 0x01
	0002    5F  5F
	// Stack delta = +1
	// Outputs[1] { @0000  stack[0] = 0x01 }
	// Block terminates

	0003    55    SSTORE

Contract-02

Test Case

      "0xb94f5374fce5edbc8e2a8697c15331677e6ebf0b" : {
                "balance" : "0x00",
                "code" : "0x6000600060006000600060003560601c620186a0f16000556001600155",
                "nonce" : "0x00",
                "storage" : {
                }
            }

Decompiled ASM code

label_0000:
	// Inputs[3]
	// {
	//     @000C  msg.data[0x00:0x20]
	//     @0014  memory[0x00:0x00]
	//     @0014  address(msg.data[0x00:0x20] >> 0x60).call.gas(0x0186a0)(memory[0x00:0x00])
	// }
	0000    60  PUSH1 0x00
	0002    60  PUSH1 0x00
	0004    60  PUSH1 0x00
	0006    60  PUSH1 0x00
	0008    60  PUSH1 0x00
	000A    60  PUSH1 0x00
	000C    35  CALLDATALOAD
	000D    60  PUSH1 0x60
	000F    1C  SHR
	0010    62  PUSH3 0x0186a0
	0014    F1  CALL
	0015    60  PUSH1 0x00
	0017    55  SSTORE
	0018    60  PUSH1 0x01
	001A    60  PUSH1 0x01
	001C    55  SSTORE
	// Stack delta = +0
	// Outputs[3]
	// {
	//     @0014  memory[0x00:0x00] = address(msg.data[0x00:0x20] >> 0x60).call.gas(0x0186a0)(memory[0x00:0x00])
	//     @0017  storage[0x00] = address(msg.data[0x00:0x20] >> 0x60).call.gas(0x0186a0)(memory[0x00:0x00])
	//     @001C  storage[0x01] = 0x01
	// }
	// Block terminates

Execution of testcase under revme context

https://github.com/bluealloy/revm/tree/main/bins/revme

Use-case is working fine on revme context, and the execution flow & traces looks OK.

Execution of Contract-02

Contract-02 invokes Contract-01

Complete Trace

2022-12-25T14:56:51.336398Z  INFO revme::statetest::cmd: Start running tests on: "tests/GeneralStateTests/EIPTests/stEIP3855/push0.json"
2022-12-25T14:56:51.336671Z  WARN revme::statetest::runner: Processing 1/1
2022-12-25T14:56:51.336745Z  WARN revme::statetest::runner: Processing 1/1
2022-12-25T14:56:51.336809Z  WARN revme::statetest::runner: Processing 1/1
2022-12-25T14:56:51.336845Z  WARN revme::statetest::runner: Processing 1/1
2022-12-25T14:56:51.336873Z  WARN revme::statetest::runner: Processing 1/1
2022-12-25T14:56:51.336907Z  WARN revme::statetest::runner: Processing 1/1
2022-12-25T14:56:51.336942Z  WARN revme::statetest::runner: Processing 1/1
2022-12-25T14:56:51.336973Z  WARN revme::statetest::runner: Processing 1/1
2022-12-25T14:56:51.336997Z  WARN revme::statetest::runner: Processing 1/1
2022-12-25T14:56:51.339193Z DEBUG revme::statetest::runner: Pre Processing => "push0"
2022-12-25T14:56:51.340249Z DEBUG revme::statetest::runner: Executing Spec => Merge
2022-12-25T14:56:51.340349Z  INFO revm::evm_impl: TransactTo::Call()=>
2022-12-25T14:56:51.340370Z  INFO revm::evm_impl: Host::code => 0xb94f5374fce5edbc8e2a8697c15331677e6ebf0b
2022-12-25T14:56:51.340390Z  INFO revm::evm_impl: Host::load_account => 0xb94f5374fce5edbc8e2a8697c15331677e6ebf0b
2022-12-25T14:56:51.340433Z  INFO revm::evm_impl: Host::step =>
2022-12-25T14:56:51.340445Z TRACE revm::interpreter: run=>PC:[0x0], OC:[0x60]
2022-12-25T14:56:51.340457Z  INFO revm::evm_impl: Host::step_end =>
2022-12-25T14:56:51.340463Z  INFO revm::evm_impl: Host::step =>
2022-12-25T14:56:51.340470Z TRACE revm::interpreter: run=>PC:[0x2], OC:[0x60]
2022-12-25T14:56:51.340476Z  INFO revm::evm_impl: Host::step_end =>
2022-12-25T14:56:51.340482Z  INFO revm::evm_impl: Host::step =>
2022-12-25T14:56:51.340489Z TRACE revm::interpreter: run=>PC:[0x4], OC:[0x60]
2022-12-25T14:56:51.340496Z  INFO revm::evm_impl: Host::step_end =>
2022-12-25T14:56:51.340502Z  INFO revm::evm_impl: Host::step =>
2022-12-25T14:56:51.340508Z TRACE revm::interpreter: run=>PC:[0x6], OC:[0x60]
2022-12-25T14:56:51.340514Z  INFO revm::evm_impl: Host::step_end =>
2022-12-25T14:56:51.340520Z  INFO revm::evm_impl: Host::step =>
2022-12-25T14:56:51.340526Z TRACE revm::interpreter: run=>PC:[0x8], OC:[0x60]
2022-12-25T14:56:51.340532Z  INFO revm::evm_impl: Host::step_end =>
2022-12-25T14:56:51.340538Z  INFO revm::evm_impl: Host::step =>
2022-12-25T14:56:51.340544Z TRACE revm::interpreter: run=>PC:[0xa], OC:[0x60]
2022-12-25T14:56:51.340552Z  INFO revm::evm_impl: Host::step_end =>
2022-12-25T14:56:51.340558Z  INFO revm::evm_impl: Host::step =>
2022-12-25T14:56:51.340564Z TRACE revm::interpreter: run=>PC:[0xc], OC:[0x35]
2022-12-25T14:56:51.340575Z  INFO revm::evm_impl: Host::step_end =>
2022-12-25T14:56:51.340582Z  INFO revm::evm_impl: Host::step =>
2022-12-25T14:56:51.340588Z TRACE revm::interpreter: run=>PC:[0xd], OC:[0x60]
2022-12-25T14:56:51.340595Z  INFO revm::evm_impl: Host::step_end =>
2022-12-25T14:56:51.340602Z  INFO revm::evm_impl: Host::step =>
2022-12-25T14:56:51.340608Z TRACE revm::interpreter: run=>PC:[0xf], OC:[0x1c]
2022-12-25T14:56:51.340617Z  INFO revm::evm_impl: Host::step_end =>
2022-12-25T14:56:51.340624Z  INFO revm::evm_impl: Host::step =>
2022-12-25T14:56:51.340630Z TRACE revm::interpreter: run=>PC:[0x10], OC:[0x62]
2022-12-25T14:56:51.340638Z  INFO revm::evm_impl: Host::step_end =>
2022-12-25T14:56:51.340644Z  INFO revm::evm_impl: Host::step =>
2022-12-25T14:56:51.340651Z TRACE revm::interpreter: run=>PC:[0x14], OC:[0xf1]
2022-12-25T14:56:51.340664Z  INFO revm::evm_impl: Host::load_account => 0x0000000000000000000000000000000000000100
2022-12-25T14:56:51.340686Z  INFO revm::evm_impl: Host::call =>
2022-12-25T14:56:51.340693Z  INFO revm::evm_impl: Host::code => 0x0000000000000000000000000000000000000100
2022-12-25T14:56:51.340705Z  INFO revm::evm_impl: Host::load_account => 0x0000000000000000000000000000000000000100
2022-12-25T14:56:51.340735Z  INFO revm::evm_impl: Host::step =>
2022-12-25T14:56:51.340744Z TRACE revm::interpreter: run=>PC:[0x0], OC:[0x60]
2022-12-25T14:56:51.340752Z  INFO revm::evm_impl: Host::step_end =>
2022-12-25T14:56:51.340758Z  INFO revm::evm_impl: Host::step =>
2022-12-25T14:56:51.340765Z TRACE revm::interpreter: run=>PC:[0x2], OC:[0x5f]
2022-12-25T14:56:51.340772Z  INFO revm::evm_impl: Host::step_end =>
2022-12-25T14:56:51.340788Z  INFO revm::evm_impl: Host::step_end =>
2022-12-25T14:56:51.340796Z  INFO revm::evm_impl: Host::step =>
2022-12-25T14:56:51.340803Z TRACE revm::interpreter: run=>PC:[0x15], OC:[0x60]
2022-12-25T14:56:51.340810Z  INFO revm::evm_impl: Host::step_end =>
2022-12-25T14:56:51.340816Z  INFO revm::evm_impl: Host::step =>
2022-12-25T14:56:51.340823Z TRACE revm::interpreter: run=>PC:[0x17], OC:[0x55]
2022-12-25T14:56:51.340839Z  INFO revm::evm_impl: Host::sstore =>
2022-12-25T14:56:51.340856Z  INFO revm::evm_impl: Host::step_end =>
2022-12-25T14:56:51.340864Z  INFO revm::evm_impl: Host::step =>
2022-12-25T14:56:51.340870Z TRACE revm::interpreter: run=>PC:[0x18], OC:[0x60]
2022-12-25T14:56:51.340877Z  INFO revm::evm_impl: Host::step_end =>
2022-12-25T14:56:51.340884Z  INFO revm::evm_impl: Host::step =>
2022-12-25T14:56:51.340890Z TRACE revm::interpreter: run=>PC:[0x1a], OC:[0x60]
2022-12-25T14:56:51.340896Z  INFO revm::evm_impl: Host::step_end =>
2022-12-25T14:56:51.340902Z  INFO revm::evm_impl: Host::step =>
2022-12-25T14:56:51.340908Z TRACE revm::interpreter: run=>PC:[0x1c], OC:[0x55]
2022-12-25T14:56:51.340915Z  INFO revm::evm_impl: Host::sstore =>
2022-12-25T14:56:51.340926Z  INFO revm::evm_impl: Host::step_end =>
2022-12-25T14:56:51.340932Z  INFO revm::evm_impl: Host::step =>
2022-12-25T14:56:51.340939Z TRACE revm::interpreter: run=>PC:[0x1d], OC:[0x0]
2022-12-25T14:56:51.340945Z  INFO revm::evm_impl: Host::step_end =>
2022-12-25T14:56:51.340991Z  INFO revme::statetest::runner: exit_reason::[Stop], gas_used::[148031], gas_refunded::[0], logs::[[]]
2022-12-25T14:56:51.342130Z  WARN revme::statetest::runner: Ignoring the spec => MergePush0
2022-12-25T14:56:51.342154Z DEBUG revme::statetest::runner: TestDone => 0/"tests/GeneralStateTests/EIPTests/stEIP3855/push0.json"

Execution of testcase under fevm context

FEVM Error

Complete Trace

2022-12-25T14:41:45.676742Z  WARN test_fevm_eth_compliance::statetest::runner: Processing status sender:false to:true
2022-12-25T14:41:45.676757Z  INFO test_fevm_eth_compliance::statetest::runner: Pre Processing TestCase "push0"::9::0xb94f5374fce5edbc8e2a8697c15331677e6ebf0b
2022-12-25T14:41:45.677142Z TRACE fil_actor_init: called exec; params.code_cid: Cid(bafkqaelgnfwc65dfon2c63lvnr2gs43jm4)    
2022-12-25T14:41:45.677169Z TRACE fil_actor_init: caller code CID: Cid(bafkqad3gnfwc65dfon2c643zon2gk3i)    
2022-12-25T14:41:45.677188Z TRACE fil_actor_init: robust address: Address { payload: Actor([83, 79, 152, 179, 173, 99, 8, 25, 210, 132, 40, 123, 100, 114, 131, 161, 213, 219, 207, 144]) }    
2022-12-25T14:41:45.678144Z TRACE fil_actor_init: delegated address: Address { payload: Delegated(DelegatedAddress { namespace: 10, length: 20, buffer: [54, 30, 204, 175, 91, 188, 185, 119, 30, 170, 64, 66, 213, 189, 192, 126, 103, 122, 13, 118, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0] }) }    
2022-12-25T14:41:45.678177Z TRACE fil_actor_init: robust address: Address { payload: Actor([85, 148, 66, 220, 206, 30, 3, 194, 148, 67, 169, 132, 24, 251, 193, 115, 118, 21, 218, 203]) }    
2022-12-25T14:41:45.678310Z TRACE fil_actor_evm: Entering::constructor=>
2022-12-25T14:41:45.678351Z TRACE fil_actor_evm::interpreter::execution::opcodes: Entering=>PC:[0x0], [0x60]
2022-12-25T14:41:45.678369Z TRACE fil_actor_evm::interpreter::execution::opcodes: Entering=>PC:[0x2], [0x60]
2022-12-25T14:41:45.678382Z TRACE fil_actor_evm::interpreter::execution::opcodes: Entering=>PC:[0x4], [0x60]
2022-12-25T14:41:45.678394Z TRACE fil_actor_evm::interpreter::execution::opcodes: Entering=>PC:[0x6], [0x60]
2022-12-25T14:41:45.678407Z TRACE fil_actor_evm::interpreter::execution::opcodes: Entering=>PC:[0x8], [0x60]
2022-12-25T14:41:45.678421Z TRACE fil_actor_evm::interpreter::execution::opcodes: Entering=>PC:[0xa], [0x60]
2022-12-25T14:41:45.678438Z TRACE fil_actor_evm::interpreter::execution::opcodes: Entering=>PC:[0xc], [0x35]
2022-12-25T14:41:45.678456Z TRACE fil_actor_evm::interpreter::execution::opcodes: Entering=>PC:[0xd], [0x60]
2022-12-25T14:41:45.678469Z TRACE fil_actor_evm::interpreter::execution::opcodes: Entering=>PC:[0xf], [0x1c]
2022-12-25T14:41:45.678479Z TRACE fil_actor_evm::interpreter::execution::opcodes: Entering=>PC:[0x10], [0x62]
2022-12-25T14:41:45.678492Z TRACE fil_actor_evm::interpreter::execution::opcodes: Entering=>PC:[0x14], [0xf1]
thread '<unnamed>' panicked at 'address is a precompile: BadAddress("cannot convert precompile 0 to an f4 address")', /home/popoyi/dscbox/sun/ws-020-blocks/ws-030-filecoin-project/dev-030-01-fvm/builtin-actors/actors/evm/src/interpreter/instructions/call.rs:216:65
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Error: Statetest(SystemError)

Some Highlights for Discussion.

Q1.1 | As part of ConstructorParams, Is it good idea to split contract bytecode, into optional initcode & runtimecode ?

Most of the bytecodes in Eth test vectors, have only runtime codes,
I hope this approach helps to deploy the actor as it is. and manage the state & invokation of contract runtime function from other actors.

#[derive(Serialize_tuple, Deserialize_tuple)]
pub struct ConstructorParams {
    /// The actor's "creator" (specified by the EAM).
    pub creator: EthAddress,
    /// The initcode that will construct the new EVM actor.
    pub initcode: Option<RawBytes>,
    /// The runtime code of the solidity contract.
    pub runtimecode: RawBytes,    
}

Q1.2 | Is it possible to use pre-compiled bytecode as it is from ethereum/tests: Common tests for all Ethereum implementations, or do we need any adoptation to make Eth & f4 address for compatibility. Under FEVM context, hit with error message of "BadAddress("cannot convert precompile 0 to an f4 address")", looks like I am missing some thing on state initialization. Can I have some more clarity over here?

@mriise
Copy link
Contributor

mriise commented Jan 5, 2023

Starting off with the error message:

This is attempting to CALL with destination 0x0000000000000000000000000000000000000000, which skips past CALL's precompile check by not being an actual precompile, but at the same time fails when trying to convert the address to an f4 address since the check it does is incomplete. We should be letting it pass if address all zeros.

if addr.0[..19] == [0; 19] {

Null address is used commonly by burn tokens, so this is definitely a good issue to catch. I've opened an issue to fix it.


Now to the assembly:

Contract 1 - First push 0x01 to the stack, then PUSH0 (0x5f), then SSTORE an item. Final result ends up being that storage of the contract is changed by putting value 0x01 at key 0x00.

Contract 2 - First push a bunch of 0s (used later). Push 32 bytes of input data starting at 0x00 onto stack, shifts input 32 bytes right by 0x60 bits (this will be the destination of CALL), then does CALL with 3,946,720 gas, destination (calculated from input), with value, input offset/size, and output offset/size all being 0x00. Store exit code at key 0x00 and value 0x01 at 0x01.

Comments in the code already have some good pseudo-code of what is going on. Which side note - I really like, is this part of a dissasembler somewhere or something you wrote in?

Contract 1 will always fail as since we dont have support for PUSH0, but its a simple enough add so I've opened an issue to add it.


Q1.1

We don't want add runtime bytecode as part of the ConstructorParams since this usage is specific to tests, on network contract will only be created with initcode. The solution to use the constructor method is to wrap the bytecode with some simple initcode that just returns the bytecode. e.g.

push4 0x60015f55 # bytecode
push1 0x00 # memory offset
mstore
push 0x04 # bytecode length
push 0x00 # offset
return

This will deploy the contract as-is, waiting to be ran with InvokeContract method.

Q1.2

We support all Eth Precompiles at their normal addresses! I believe what happened was since it was called in the constructor it had no input data so it used 0x00 as the address for call. We have a bug (as mentioned above) so sending to the null address fails for us with a misleading error.

Thanks for the great issue!

Issues created from this:
filecoin-project/ref-fvm#1399
filecoin-project/ref-fvm#1400

@shamb0
Copy link
Contributor Author

shamb0 commented Jan 6, 2023

Thanks, @mriise. I got your suggestion for Q1.1. But the requirement for the test tool is to re-use the test vector as it is from https://github.com/ethereum/tests.git. So any other possibility to deploy the contract with invalid bytecodes ?

@mriise
Copy link
Contributor

mriise commented Jan 7, 2023

Yes, though it's a fair bit more of a pain to do.

  1. If you wish, you should be able to skip the call to the Init actor directly and put the actor in state directly. Be careful that you are replicating everything the init actor would do though. (so not recommended)

  2. Alternatively you should be able to mutate the state of an existing evm actor directly with the test vm. Essentially put the bytecode into the blockstore, fill in the rest of the values (again be careful here), serialize and put the new state into the blockstore. Finally update the state root of the target actor and then invoke it, it should load the new bytecode from the new state before executing.

Both options require stepping carefully though, so I recommend double checking to make sure that not changing the test bytecode (like mentioned) in any way is a hard requirement.

@mriise
Copy link
Contributor

mriise commented Jan 9, 2023

@shamb0 Address rejection fix just got merged into next 🚀

@maciejwitowski
Copy link
Contributor

maciejwitowski commented Jan 10, 2023

@shamb0 can you share your plan for next steps so I know what to expect?
Context: we'd like to declare a code freeze on Jan 26th so we're super interested in finding and fixing as many bugs as possible prior to that.

@maciejwitowski
Copy link
Contributor

btw we are still not sure whether to add Push0 to the scope of this milestone filecoin-project/ref-fvm#1400

@shamb0
Copy link
Contributor Author

shamb0 commented Jan 10, 2023

Hello @maciejwitowski,

Have some good news, cross contract call is working
as expected, Just completed the solution suggested by @mriise.

As next step.

  • Planning to take support of @mriise, to review the implementation & changes.

  • Can understand the milestone priorities, I hope covering all the testsuites under GeneralStateTests within Jan-26 is bit difficult. Just exploring the best possibilities, is it good idea to shortlist the practical use-case based test-suites ( may be roughly some 250 testsuites ) for coming next 15 days ? I hope it helps to improve the code coverage as expected.

@maciejwitowski
Copy link
Contributor

Thanks for the update!

Yes, I like the suggestion about shortlisting the tests!

What do you think about these rules for work?

  • FEVM supports EVM till Paris fork, only EIP1559 transactions and it doesn't support any deprecated opcodes. Focus only on what is supported.
  • Security has the highest priority, then functional items, then usability - so let's look at the prioritisation from this perspective
  • We should do it iteratively and communicate as frequently as possible. This means reporting the issues after every new test, to make sure we get the feedback asap and have more time to fix bugs.

If we do this, and we report whether particular test pass or not every day, then we build a solid confidence day by day. We could create a new issue eg in ref-fvm repo and post updates there as they come. Does it make sense?

Thanks for your hard work! This is an absolutely crucial time for us and we really appreciate your enthusiasm to help! ❤️

@shamb0
Copy link
Contributor Author

shamb0 commented Jan 11, 2023

Thanks @maciejwitowski :-).

Proposed Way-of-work sounds good. I agree to that.

One suggestion is, Instead of updating the issues to repo link, like to update the test execution status to tracking sheet in the PR branch (at least for first few days). Once the team agrees, the failure or bug will move to repo issue tracker. The idea is not to flood with spam issues on the repo issue tracker.

https://github.com/shamb0/builtin-actors/blob/shamb0/dev-v1-evm-eth-compliance-test/test_evm_eth_compliance/test-vectors/01_wk2250_skip_test_tracking.md

@maciejwitowski
Copy link
Contributor

Sounds good, thank you!
Just to confirm I understand correctly:

  • skip test tracking contains test cases we know we don't support, like legacy transactions, Shanghai upgrades changes, right? For these, we don't even need screenshots or detail information, since we know straightaway that they are not supported.
  • GeneralStateTests_status contains the actual report of test cases we know we want to support. We need to make sure the report is very clear and it's really easy to see eg which opcode fails at which particular scenario. I'd suggest to mark a suite as OK even though some skipped tests aren't passing (as we don't care about the at this time).

Correct?

@shamb0
Copy link
Contributor Author

shamb0 commented Jan 11, 2023

Hello @maciejwitowski,

Eth Compliance test, intermediate progress.

Progress Count
Total Completed 534
Skipped 63
Use-Cases Number of Testsuites
EIPTests 18
stArgsZeroOneBalance 46
stAttackTest 2
stBadOpcode 121
stBugs 5
stCallCodes 80
stCallCreateCallCodeTest 43
stCallDelegateCodesCallCodeHomestead 58
stCallDelegateCodesHomestead 58
stChainId 2
stCodeCopyTest 2
stCodeSizeLimit 5
stCreate2 51
stCreateTest 43

Raised Issues

@mriise
Copy link
Contributor

mriise commented Jan 11, 2023

@shamb0 is this issue good to be closed since initial issue has been fixed?

@shamb0
Copy link
Contributor Author

shamb0 commented Jan 12, 2023

Thanks, @mriise, for great support. :-)

@shamb0 shamb0 closed this as completed Jan 12, 2023
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

No branches or pull requests

3 participants