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

EVM-C: Remove factory class #131

Merged
merged 6 commits into from Oct 2, 2017

Conversation

Projects
None yet
3 participants
@chfast
Collaborator

chfast commented Sep 22, 2017

This is final change to EVM-C API +/- documentation fixes and small refactorings.

I reviewed the previous version by drawing the class diagram of it. I managed to reduce it from 4 classes: Host, Context, EVM, Factory to only 2: EVM and Context. Now it matches very simple OOP polymorphic desing, but it is hard to see in under big C overhead.

I'm not sure we should keep the ABI_VERSION field. It does not seem to be useful in near future.

The still missing piece is tracing.

chfast added some commits Sep 22, 2017

@chfast chfast requested review from axic and gcolvin Sep 22, 2017

chfast added some commits Sep 22, 2017

@gcolvin

This comment has been minimized.

Show comment
Hide comment
@gcolvin

gcolvin Oct 1, 2017

@chfast After reading through the code I decided to build the library from the readme instructions. That seemed to go fine until I got to step 5. It doesn't say what directory the build directory should go in, and searching my hard drive for CMakeLists.txt didn't find anyplace that looked right.

  1. Install llvm-3.7-dev package
  2. For Ubuntu 14.04 using LLVM deb packages source: http://llvm.org/apt
  3. For Ubuntu 14.10 using Ubuntu packages
  4. Build library with cmake
  5. mkdir build && cd $_
  6. cmake .. && make
  7. Install library
  8. sudo make install
  9. sudo ldconfig

gcolvin commented Oct 1, 2017

@chfast After reading through the code I decided to build the library from the readme instructions. That seemed to go fine until I got to step 5. It doesn't say what directory the build directory should go in, and searching my hard drive for CMakeLists.txt didn't find anyplace that looked right.

  1. Install llvm-3.7-dev package
  2. For Ubuntu 14.04 using LLVM deb packages source: http://llvm.org/apt
  3. For Ubuntu 14.10 using Ubuntu packages
  4. Build library with cmake
  5. mkdir build && cd $_
  6. cmake .. && make
  7. Install library
  8. sudo make install
  9. sudo ldconfig
@gcolvin

gcolvin approved these changes Oct 1, 2017

Very nice!

/// FIXME: We should rethink reporting internal errors. One of the options
/// it to allow using any negative value to represent internal errors.
/// @todo We should rethink reporting internal errors. One of the options
/// it to allow using any negative value to represent internal errors.
EVM_INTERNAL_ERROR = -1,

This comment has been minimized.

@gcolvin

gcolvin Oct 1, 2017

Handing over all the negative numbers seems a good idea. An optional string might be good if the memory management isn't too much trouble.

@gcolvin

gcolvin Oct 1, 2017

Handing over all the negative numbers seems a good idea. An optional string might be good if the memory management isn't too much trouble.

This comment has been minimized.

@chfast

chfast Oct 2, 2017

Collaborator

Ok, I will clarify that negative values are allowed and are related to implementation-defined errors.

Yes, additional error message is possible. We actually had it in evm_result before, but it was removed. Memory management is not a problem, because evm_result have "virtual destruktor". However, I'd like to add it the moment we have any practical use case for it. What do you think @axic? Do you need it in your eWASM enabled fork of cpp-ethereum?

@chfast

chfast Oct 2, 2017

Collaborator

Ok, I will clarify that negative values are allowed and are related to implementation-defined errors.

Yes, additional error message is possible. We actually had it in evm_result before, but it was removed. Memory management is not a problem, because evm_result have "virtual destruktor". However, I'd like to add it the moment we have any practical use case for it. What do you think @axic? Do you need it in your eWASM enabled fork of cpp-ethereum?

Show outdated Hide outdated include/evm.h Outdated
@@ -437,6 +428,15 @@ typedef void (*evm_prepare_code_fn)(struct evm_instance* instance,
///

This comment has been minimized.

@gcolvin

gcolvin Oct 1, 2017

So CACHED code doesn't have to be on-disk, but won't become UNKNOWN? And READY code might become CACHED, or might go back to being UNKNOWN?

I probably missed it, but what are the flags? Implementation defined? They show up in few places.

It might be good to allow for implementations that support preparation to execute ready or cached code based on the code's hash.

@gcolvin

gcolvin Oct 1, 2017

So CACHED code doesn't have to be on-disk, but won't become UNKNOWN? And READY code might become CACHED, or might go back to being UNKNOWN?

I probably missed it, but what are the flags? Implementation defined? They show up in few places.

It might be good to allow for implementations that support preparation to execute ready or cached code based on the code's hash.

This comment has been minimized.

@chfast

chfast Oct 2, 2017

Collaborator

I needed this to have the same capabilities in API for JIT as before going EVM-C. This is needed for SmartJIT strategy - it has to decide where to delegate the execution - to JIT or to interpreter.

READY means the pre-processed code is in memory and there is not overhead of starting execution in JIT.
CACHED means the pre-processed code is on-disk and the overhead of executing in JIT will be to load the code from disk.

We should think about generalizing this API more... but it is not critical at the moment, and not required to implement interpreter.

@chfast

chfast Oct 2, 2017

Collaborator

I needed this to have the same capabilities in API for JIT as before going EVM-C. This is needed for SmartJIT strategy - it has to decide where to delegate the execution - to JIT or to interpreter.

READY means the pre-processed code is in memory and there is not overhead of starting execution in JIT.
CACHED means the pre-processed code is on-disk and the overhead of executing in JIT will be to load the code from disk.

We should think about generalizing this API more... but it is not critical at the moment, and not required to implement interpreter.

This comment has been minimized.

@gcolvin

gcolvin Oct 2, 2017

Thanks, but I'm still confused. Let me try again.

  • Are READY files guaranteed to be ready for subsequent calls?
  • Are CACHED files guaranteed to be cached or ready for subsequent calls?
    This matters for a JIT, as I understand that a JIT that does not cache its output and keep it cached can be exploited.
    (Whether the cache is on disk or in memory is an implementation detail.)
@gcolvin

gcolvin Oct 2, 2017

Thanks, but I'm still confused. Let me try again.

  • Are READY files guaranteed to be ready for subsequent calls?
  • Are CACHED files guaranteed to be cached or ready for subsequent calls?
    This matters for a JIT, as I understand that a JIT that does not cache its output and keep it cached can be exploited.
    (Whether the cache is on disk or in memory is an implementation detail.)

This comment has been minimized.

@chfast

chfast Oct 2, 2017

Collaborator

No and no.
In terms of JIT, READY means that the code is in memory. Memory is limited, you have to delete some codes over time.
The same for CACHED. Your disk space is limited, so you may want to remove some codes from on-disk cache.
So there are no guarantees that any code will preserve its current status.

@chfast

chfast Oct 2, 2017

Collaborator

No and no.
In terms of JIT, READY means that the code is in memory. Memory is limited, you have to delete some codes over time.
The same for CACHED. Your disk space is limited, so you may want to remove some codes from on-disk cache.
So there are no guarantees that any code will preserve its current status.

This comment has been minimized.

@gcolvin

gcolvin Oct 2, 2017

OK. The first no I expected. But I do worry about code falling out of the cache. In discussions with @Arachnid he was quite clear that caches that can spill are exploitable. I'll hope he shows up and explains.

@gcolvin

gcolvin Oct 2, 2017

OK. The first no I expected. But I do worry about code falling out of the cache. In discussions with @Arachnid he was quite clear that caches that can spill are exploitable. I'll hope he shows up and explains.

This comment has been minimized.

@Arachnid

Arachnid Oct 3, 2017

Yes - any cache with eviction cannot be relied on as a performance optimisation in a blockchain environment. The attacker can simply devise an attack that round-robins a larger number of entries than the cache can hold, guaranteeing every single action is a cache miss.

@Arachnid

Arachnid Oct 3, 2017

Yes - any cache with eviction cannot be relied on as a performance optimisation in a blockchain environment. The attacker can simply devise an attack that round-robins a larger number of entries than the cache can hold, guaranteeing every single action is a cache miss.

This comment has been minimized.

@chfast

chfast Oct 3, 2017

Collaborator

Thanks to explaining this to me. Then, the CACHED code SHOULD be maintained the same way the Ethereum state is. I will add this clarification in comments.

@chfast

chfast Oct 3, 2017

Collaborator

Thanks to explaining this to me. Then, the CACHED code SHOULD be maintained the same way the Ethereum state is. I will add this clarification in comments.

This comment has been minimized.

@gcolvin

gcolvin Oct 3, 2017

And as I think we discussed, a good strategy might be to compile and cache contracts when they are first created so an interpreter isn't even needed.

@gcolvin

gcolvin Oct 3, 2017

And as I think we discussed, a good strategy might be to compile and cache contracts when they are first created so an interpreter isn't even needed.

@chfast

Sorry about build instructions. They are always out-dated.

Anyway, If you want to play with EVM-C you only need the header evm.h.

/// FIXME: We should rethink reporting internal errors. One of the options
/// it to allow using any negative value to represent internal errors.
/// @todo We should rethink reporting internal errors. One of the options
/// it to allow using any negative value to represent internal errors.
EVM_INTERNAL_ERROR = -1,

This comment has been minimized.

@chfast

chfast Oct 2, 2017

Collaborator

Ok, I will clarify that negative values are allowed and are related to implementation-defined errors.

Yes, additional error message is possible. We actually had it in evm_result before, but it was removed. Memory management is not a problem, because evm_result have "virtual destruktor". However, I'd like to add it the moment we have any practical use case for it. What do you think @axic? Do you need it in your eWASM enabled fork of cpp-ethereum?

@chfast

chfast Oct 2, 2017

Collaborator

Ok, I will clarify that negative values are allowed and are related to implementation-defined errors.

Yes, additional error message is possible. We actually had it in evm_result before, but it was removed. Memory management is not a problem, because evm_result have "virtual destruktor". However, I'd like to add it the moment we have any practical use case for it. What do you think @axic? Do you need it in your eWASM enabled fork of cpp-ethereum?

@@ -437,6 +428,15 @@ typedef void (*evm_prepare_code_fn)(struct evm_instance* instance,
///

This comment has been minimized.

@chfast

chfast Oct 2, 2017

Collaborator

I needed this to have the same capabilities in API for JIT as before going EVM-C. This is needed for SmartJIT strategy - it has to decide where to delegate the execution - to JIT or to interpreter.

READY means the pre-processed code is in memory and there is not overhead of starting execution in JIT.
CACHED means the pre-processed code is on-disk and the overhead of executing in JIT will be to load the code from disk.

We should think about generalizing this API more... but it is not critical at the moment, and not required to implement interpreter.

@chfast

chfast Oct 2, 2017

Collaborator

I needed this to have the same capabilities in API for JIT as before going EVM-C. This is needed for SmartJIT strategy - it has to decide where to delegate the execution - to JIT or to interpreter.

READY means the pre-processed code is in memory and there is not overhead of starting execution in JIT.
CACHED means the pre-processed code is on-disk and the overhead of executing in JIT will be to load the code from disk.

We should think about generalizing this API more... but it is not critical at the moment, and not required to implement interpreter.

Show outdated Hide outdated include/evm.h Outdated

@chfast chfast merged commit 2490f6d into develop Oct 2, 2017

1 of 5 checks passed

ci/circleci CircleCI is running your tests
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
wercker/build-release Wercker pipeline executing
Details
wercker/build-debug Wercker pipeline passed
Details

@chfast chfast deleted the evmc-no-factory branch Oct 2, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment