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

(WIP) Call fixes precompiles #168

Merged
merged 28 commits into from
Nov 12, 2018
Merged

(WIP) Call fixes precompiles #168

merged 28 commits into from
Nov 12, 2018

Conversation

mratsim
Copy link
Contributor

@mratsim mratsim commented Oct 5, 2018

Fix #122, required by #121

I'm opening it the PR to start discussion implementation (or things not implemented) and tests.


A change in eth_trie file structure broke the CI: https://travis-ci.org/status-im/nimbus/jobs/437522149#L1050

@@ -85,7 +85,7 @@ proc prepareChildMessage*(
code,
childOptions)

proc applyMessage(computation: var BaseComputation) =
proc applyMessage(computation: var BaseComputation, opCode: static[Op]) =
Copy link
Contributor

Choose a reason for hiding this comment

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

If this proc is generic now (due to the static param), it would make sense to extract the parts of it that don't depend on the opCode param in helper procs (the goal would be the reduce the overall code size).

@tersec
Copy link
Contributor

tersec commented Oct 6, 2018

When the GeneralStateTests.md changes show up, it would also be good to add the newly passing tests to test_generalstate_failing.nim to prevent regressions.

Copy link
Contributor

@tersec tersec left a comment

Choose a reason for hiding this comment

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

Where do the PrecompileTests come from? I don't see them in https://github.com/ethereum/tests

Copy link
Contributor

@tersec tersec left a comment

Choose a reason for hiding this comment

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

Do you have any insight as to why these PrecompileTests mostly work, but the state tests in stPreCompiledContracts2 don't?

@coffeepots
Copy link
Contributor

@tersec Calling for review for this work to be merged before continuing on the state tests.
In response to your query, I'm not sure why the stPreCompiledContracts2 don't pass yet but it's worth noting there is currently an issue in the modexp precompile that needs fixing. However this branch still pushes forward more passing tests so I believe it's worth doing these fixes in another PR to prevent more merge conflicts going forward.

@mratsim
Copy link
Contributor Author

mratsim commented Nov 12, 2018

Regarding modExp are some tests passing and other failings or are all failing?

@coffeepots
Copy link
Contributor

coffeepots commented Nov 12, 2018

@mratsim ModExp currently fails all tests except one. Current issues are:

1. Consuming Data

Fails assertion when consuming data here: https://github.com/status-im/nimbus/blob/0366dd47d94c07e7d32606343adfda6043b7a59d/nimbus/vm/precompiles.nim#L117

base = rawMsg.rangeToPaddedUint256(96, start_exp - 1) fails an assertion check in io.nim because some tests use a base_len that is >32.

base_len values in the tests are 32, 64, 128, 256, 512 and 1024.

2. Results

The remaining tests that don't have the above error are eip_example1 and eip_example2

eip_example1:
Output : 0000000000000000000000000000000000000000000000000000000000000000
Expected: 0000000000000000000000000000000000000000000000000000000000000001

eip_example2: Is the only test that passes, but is expecting zero output anyway.

@tersec
Copy link
Contributor

tersec commented Nov 12, 2018

@tersec Calling for review for this work to be merged before continuing on the state tests.
In response to your query, I'm not sure why the stPreCompiledContracts2 don't pass yet but it's worth noting there is currently an issue in the modexp precompile that needs fixing. However this branch still pushes forward more passing tests so I believe it's worth doing these fixes in another PR to prevent more merge conflicts going forward.

I agree.

@coffeepots coffeepots merged commit 0366dd4 into master Nov 12, 2018
@coffeepots coffeepots deleted the call-fixes-precompiles branch December 5, 2018 13:48
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.

5 participants