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 host functions to support EVM #316

Merged
merged 19 commits into from
Jun 15, 2022
Merged

Conversation

elmato
Copy link
Contributor

@elmato elmato commented May 25, 2022

No description provided.

@spoonincode
Copy link
Member

A few build related things I've noticed through a quick look over so far:

Need to add the LICENSE files for the new libraries as installed files; down in this area https://github.com/eosnetworkfoundation/mandel/blob/main/CMakeLists.txt#L235

These libraries need to be added to the libtester.cmakes.

libff as configured adds a requirement of SSE4.1. I guess this isn't a problem 🤷 but we may want to consider adjusting compile flags elsewhere if we're going to require it in one localized spot already.

libff adds back gmp as a dependency. I just got done removing that slippery dep (#128) 😞 Is there no other way? Did OpenSSL not have the needed functionality?

Will need to fork or PR a change to libff to ensure it always dynamically links to GMP otherwise it'll be a GPL violation. You can see how that was done historically over here
https://github.com/eosnetworkfoundation/mandel-fc/blob/historical-v2.0.x/CMakeModules/FindGMP.cmake#L34
But we may be pushed to do a fork regardless for install() related reasons in #277

@spoonincode
Copy link
Member

Also may need to detect and USE_ASM=Off for compatibility with #317

std::memcpy( output.data(), res.data(), copy_size );
}

int32_t interface::ecrecover( span<const char> signature, span<const char> digest, span<char> pub) const {
Copy link
Member

Choose a reason for hiding this comment

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

Feels like a missed opportunity to add this without support for EOS key types. Such functionality has long been requested.

Choose a reason for hiding this comment

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

If any of these could be useful for non-EVM specific use cases, like you're suggesting here and I suspect others would also be useful as well, then might it make sense to give this feature a more generic/feature specific name and not something that suggests it's only useful for EVM implementations?

Copy link
Member

Choose a reason for hiding this comment

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

That's a good point. These arguably should have a more generic protocol feature name because some of the new host functions are certainly not specific to EVM: anyone might want to make a sha3 hash for all sorts of purposes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any suggestion for the protocol feature name?
Alternatively we can split EVM_PRECOMPILES into many protocol features.

  • ALT_BN128
  • MODEXP
  • BLAKE2F
  • SHA3
  • SAFE_ECRECOVER

@spoonincode
Copy link
Member

We are going to need to resolve the libff installs as I think they're going to pollute the package. Worst case I can deal with it once we merge.

@arhag arhag linked an issue Jun 9, 2022 that may be closed by this pull request
CMakeModules/EosioTester.cmake.in Show resolved Hide resolved
@spoonincode spoonincode dismissed their stale review June 13, 2022 18:31

I can deal with build fixes after merge if need be

@arhag arhag mentioned this pull request Jun 14, 2022
spoonincode and others added 10 commits June 14, 2022 14:17
Added an updated configuration `deep_mind_handler` to support
special adjusted behavior for the deep mind logger to be used
specifically in the context of tests.

In particular a new config flag called `zero_elapsed` is added to this
config which if set to true will first zero out the `elapsed` fields
within a copy of the transaction trace before writing it out to the
logs.

The `zero_elapsed` flag is set to true in the deep_mind_tests in order
to guarantee a deterministically generated log to compare against the
committed log. This appears to be a more reliable solution to avoiding
non-determinism in the tests than relying on the auto-generated
`[[:xdigit:]]` masks in the committed logs. It may now be reasonable
to greatly simply that log merge and comparison logic, but that is not
addressed in this commit.
Updated to reflect the changes to logs propagated from the changes to
the initial chain startup sequence in tests (due to new protocol
features that are now also being activated at test startup by default).
This does not change anything, but it is a nice pattern to follow so
that we can easily recall by looking at the source in which order we
introduced new protocol features into newer releases of the code.
Add the two new protocol features to the list of expected protocol
feature within the script.
@arhag
Copy link
Member

arhag commented Jun 15, 2022

All jobs in https://github.com/eosnetworkfoundation/mandel/runs/6891133585?check_suite_focus=true passed except for "Ubuntu 20.04 | Contract Builder Docker". That job fails because the CICD is running for a fork and so it does not have access to the appropriate GitHub secrets needed for that job.

But since all relevant tests pass, I will be merging this PR in anyway.

@arhag arhag merged commit 1ebd100 into eosnetworkfoundation:main Jun 15, 2022
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.

Host functions to support EVM
5 participants