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

Add rust binding client #527

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

CaptainVincent
Copy link

@CaptainVincent CaptainVincent commented Jun 26, 2020

Part of #476.

  1. Add rust client binding base on EVMC 7.4.0.
  2. f8466c7 is a temporary workaround for evmc_host_context cannot be instantiated. cherry-pick b0c40df from rust: turn evmc_host_context into a void type #524
  3. Update all files' license to Apache 2.0.

I am not yet add any unit test for this module but it work fine with Hera.
https://github.com/second-state/rust-ssvm/tree/upgrade-evmc-client
Use this branch follow original example section.
(I add hera as submodule, replace build.rs to build and link libhera.so and update dummy host function in example)
but it already applied on https://github.com/second-state/rust-ssvm/tree/evmc-7 could run example with SSVM.

@CaptainVincent CaptainVincent force-pushed the rust-binding-client branch 2 times, most recently from 8a81f02 to b87db19 Compare June 26, 2020 07:26
@CaptainVincent
Copy link
Author

CaptainVincent commented Jun 26, 2020

I saw some ci issues come from my evmc_host_context workaround as below.

  1. /home/circleci/project/include/evmc/evmc.h:164:8: error: struct has no members [-Werror=pedantic]
  2. error: empty struct is a GNU extension [-Werror,-Wgnu-empty-struct]

struct evmc_host_context {};

  1. Code format
-struct evmc_host_context {};
+struct evmc_host_context
+{
+};
  1. bindings-go-latest fail reason unknown.

I think I need some suggestions for pass these tests. Thanks.

pub type CallKind = ffi::evmc_call_kind;
pub type Revision = ffi::evmc_revision;
pub type StatusCode = ffi::evmc_status_code;
pub type Capabilities = ffi::evmc_capabilities;
Copy link
Member

Choose a reason for hiding this comment

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

Why not use evmc-vm::types instead of duplicating? We could later refactor to pull them out somewhere.

Copy link
Author

@CaptainVincent CaptainVincent Jun 29, 2020

Choose a reason for hiding this comment

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

Yes, there is needless. I don't notice main line is already added alias types after I fork it.
Fixed in f726610

host_interface: ::std::option::Option<Box<dyn HostInterface>>,
}

static mut CALLBACK: Callback = Callback {
Copy link
Member

Choose a reason for hiding this comment

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

This makes this not thread-safe. Why did you choose to use this and not use evmc_host_context, which by design is there to avoid having these globals.

Copy link
Author

@CaptainVincent CaptainVincent Jun 29, 2020

Choose a reason for hiding this comment

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

Yes, you are right. The original implementation was limited by how much I familiar with Rust. I have rewritten it more like go-binding implementation in 5b2cc1a.

value: &Bytes32,
code: &Bytes,
create2_salt: &Bytes32,
) -> (&Bytes, i64, StatusCode) {
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to use evmc-vm::ExecutionResult here.

Copy link
Author

Choose a reason for hiding this comment

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

The struct is incompatible with my idea. I try to hide ffi layer's nest struct to user make it more user friendly.
ex. in bindings.rs

pub struct evmc_address {
    #[doc = " The 20 bytes of the hash."]
    pub bytes: [u8; 20usize],
}

evmc-vm expose those nest structures directly like Address, Bytes32 ...etc and ExecutionResult include one Address field.
But I usually prefer use the simply types that re-defined in emc-client as below.

pub const ADDRESS_LENGTH: usize = 20;
pub const BYTES32_LENGTH: usize = 32;
pub type Address = [u8; ADDRESS_LENGTH];
pub type Bytes32 = [u8; BYTES32_LENGTH];

I need confirm which way is better.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for the delay, but evmc-vm::ExecutionResult is not ffi, it has high-level types.

Copy link
Member

Choose a reason for hiding this comment

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

Also regarding the type aliases, the goal was to eventually replace them with nicer types, but to do it incrementally.

EvmcLoaderInvalidOptionValue = 7,
}

pub fn evmc_load_and_create(fname: &str) -> (*mut ffi::evmc_vm, EvmcLoaderErrorCode) {
Copy link
Member

Choose a reason for hiding this comment

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

Why did you chose to reimplement this in Rust and not just use the C loader which already exists?

Copy link
Author

Choose a reason for hiding this comment

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

I reimplement it is because I use this function (loader) outside evmc module at first. I want to avoid move partial evmc's C code to my codebase. But I agree it's better and more consistent to use the C loader after I move my evmc relative code into evmc-client now. I will try to replace it use the C loader, but maybe this won't finish too soon.

Copy link
Author

Choose a reason for hiding this comment

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

Finish 5b2b4f2, btw I need cmake in ci environment.

use std::ffi::CStr;

extern "C" {
fn evmc_create() -> *mut ffi::evmc_vm;
Copy link
Member

Choose a reason for hiding this comment

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

I think as a first step it would be better to only support dynamic loading.

Are you using this static linking in SSVM?

Copy link
Author

Choose a reason for hiding this comment

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

I am not sure this is relative with static linking. I still use dynamic link to lib.so and the statement just tell rustc how to resolve symbol after cargo link the library.
https://github.com/second-state/rust-ssvm/blob/master/build.rs#L25
This is useful in my case that I don't need to know where is my evmc dynamic-link library (especially the target path is not always the same).

Copy link
Member

Choose a reason for hiding this comment

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

If it does an extern import of evmc_create that is statically linking against an EVMC VM, IIRC, otherwise this would be a lingering reference.

@CaptainVincent CaptainVincent requested a review from axic July 4, 2020 06:36
Turn evmc_host_context instantiable.
1. remove duplicate types
2. add an alias type Capabilities in evmc-vm
Refine this module make it more like geth client binding implementation.
1. A global static variable requires static lifetime.
2. Box with a trait object requires static lifetime.

Avoid host context contain a outside variable with conflict lifetime issue.

# Conflicts:
#	bindings/rust/evmc-client/src/host.rs
#	bindings/rust/evmc-client/src/lib.rs
) -> ffi::evmc_result {
let msg = *msg;
let (output, gas_left, create_address, status_code) =
(*(context as *mut ExtendedContext)).hctx.call(
Copy link
Member

Choose a reason for hiding this comment

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

This entire function could be reduced to hctx.call().into() is using ExecutionResult from evmc-vm/lib.rs because that implements the translation and memory handling already.

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

Successfully merging this pull request may close these issues.

2 participants