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

Incomplete testing kit EIP-2537 #11

Open
jochem-brouwer opened this issue Jun 26, 2020 · 4 comments
Open

Incomplete testing kit EIP-2537 #11

jochem-brouwer opened this issue Jun 26, 2020 · 4 comments

Comments

@jochem-brouwer
Copy link

jochem-brouwer commented Jun 26, 2020

Hey guys! We are implementing EIP-2537 (BLS12-381 precompiles) over at EthereumJS.

The tests are useful for validating input and output. However, especially for the pairing precompile, the gas cost is rather complicated here and we really want to verify that the gas used is correct. Gas usage is not in these tests.

Furthermore, it seems like some error cases are missing, like nonzero bytes in the first bytes of G1/G2 inputs (where these should be zero).

I strongly suggest creating state tests and opening a PR in the ethereum/tests repo.

@shamatar
Copy link
Member

Hello @jochem-brouwer

I'll start in a reverse order.

The main requirement for serialization of field elements in that those are "in a field" (< modulus), and since we use BE encoding then it automatically makes top 16 bytes to be zero since modulus length is just 381 bit. It will be emphasized and rephrased in a spec as soon as I polish few parts in there. There are tests for invalid Fp/Fp2 encoding, so it's expected that any sane implementation would follow a similar procedure for G1/G2 points that are just two Fp/Fp2 elements each.

It's unfortunate that people made mistakes in gas pricing formulas, so there will be explicit examples of gas cost calculation in an updated spec including logically invalid variants (like zero length). Test vectors for gas cost will also be added explicitly in a repo.

As for a state test - precompiles are stateless, so it was expected that they will be tested in such a way (it was briefly discussed in one of the calls). In any case, even while it's possible to write some state tests I do not see a way how those can be used to check gas estimations.

@jochem-brouwer
Copy link
Author

jochem-brouwer commented Jun 29, 2020

Hey @shamatar, thank you for the reply.

A very good point about the state tests. I actually thought that the return values were part of the receiptsTrie, but this is not the case. However, one could circumvent this by creating a contract and then either logging the output of the precompile or by dumping the output of the precompile as the contract code (in order to either make it turn up in receiptsTrie/logsBloom or in the stateRoot). In my eyes it is really important we have "official" tests, maybe a new type of tests, but I think it is important to normalize those tests such that every client uses the same tests and we do not get problems that all tests pass on all clients, except every client uses a different test and you thus cannot compare them.

I thus realized this is more a "problem" from the core devs / EIP protocol side and will discuss it some more there.

As a final note, you can actually use state tests for gas usage: gas used is parts of receiptsTrie so you can check these results against the trie 😄 .

Looking forward to the EIP updates!

@shamatar
Copy link
Member

@jochem-brouwer

Would it be possible to ever have a tool that will make state tests from chain of
Web3 interface interactions? At least simple state root equality assertions.

I didn’t have a problem to compare the precompile output results, but I do not think that precompile gas consumption results would be accurate and not brake immediately after e.g. 2046.

Sincerely, Alex

@jochem-brouwer
Copy link
Author

@shamatar Good point about the post-EIP hardforks, but this does not apply to just this one but would apply to other EIPs as well, so one should explicitly write state tests for Berlin (or with a post-Istanbul fork where only this EIP is activated). It gets rather complex with 2046 though, but this could be circumvented by using CALL instead of STATICCALL. A good point though, it gets more complex if you also consider the other EIPs which change gas cost. 😅

It would be easier to indeed just call these contracts directly and check against the output (and gas).

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

2 participants