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

Blake2f test vectors #619

Merged
merged 20 commits into from
Sep 23, 2019
Merged

Blake2f test vectors #619

merged 20 commits into from
Sep 23, 2019

Conversation

shemnon
Copy link
Contributor

@shemnon shemnon commented Aug 5, 2019

Add CALL and CALLCODE tests for the standard unit test vectors for Blake2F, targeting Istanbul.

"expect": [
{
"indexes": {
"data": [0],
Copy link
Collaborator

Choose a reason for hiding this comment

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

also a comment about what is the case on data 0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

described the outputs in the "// data" section, they are all hash values so they are quite opaque.

@shemnon
Copy link
Contributor Author

shemnon commented Aug 6, 2019

Is there some sort fo a standard published as to what is expected from a community contributed test case? These objections seem arbitrary when I can point to other tests cases that don't have these features. While it is valid to say "past bad behavior does not set a precedent" without a clear document describing what is expected all I have are the past behaviors and I don't know which are bad, which are acceptable, and which are good.

@winsvega
Copy link
Collaborator

winsvega commented Aug 6, 2019

hm. no such standard created yet. good point this notes is well to be published in test creation wiki.
we have a lot of tests for the past 4 years, it would be good to have more comments in tests, but not enough hands to write both tests, manuals and tools

and also non of the external contributors mostly ever wrote any .json tests to the repo. so thats why no manuals. now when other clients could write their own tests the manuals are to be created from precedents

#620

* add input data descriptions
* add new tests for input length and round extremes (0, max)
@shemnon
Copy link
Contributor Author

shemnon commented Aug 7, 2019

Also added a few more test cases.

Also, we should have cross comparison against another before committing. Some details are not settled yet that are covered by these test cases.

@winsvega
Copy link
Collaborator

winsvega commented Aug 7, 2019

Stay in touch. Once geth have the implementation you could run this test against two clients

@holiman
Copy link
Contributor

holiman commented Aug 7, 2019

This may not be the best discussion forum, but one thing that struck me is that it would be neat if all[*] tests use the sender account as coinbase - in this case 0xa94f5374fce5edbc8e2a8697c15331677e6ebf0b. If we do that, then the postStates (and poststate dumps) become smaller, and we get less questions about "how did account X (coinbase) get into the poststate?"

[*] all meaning all cases where there isn't some explicit test involving the coinbase address

@shemnon
Copy link
Contributor Author

shemnon commented Aug 7, 2019

It totally demolishes gas calculation comparison because all the fees go right back where they started. Perhaps the contract becomes the coinbase? But then you would have to explain why the contract's balance goes up in the same way the coinbase would go up. "gasPrice": "0" may be more what we are looking for.

@holiman
Copy link
Contributor

holiman commented Aug 7, 2019 via email

@winsvega
Copy link
Collaborator

@holiman
Copy link
Contributor

holiman commented Aug 23, 2019

Geth doesn't pass these tests...

[user@work go-ethereum]$ build/bin/evm statetest /home/user/workspace/tests/GeneralStateTests/stPreCompiledContracts2/CALLBlake2f.json 
[
  {
    "name": "CALLBlake2f",
    "pass": false,
    "fork": "Istanbul",
    "error": "post state root mismatch: got 56c0c7bd91cff99f993ede0733fa3a38e194d01f5c87a9b882305f61c3a4c338, want b46f92edbed07baacf55c64dc3c9169e1f04966f9ce87e81cc107eebd7ce94af"
  },
...

Question: is this PR up to date?

@shemnon
Copy link
Contributor Author

shemnon commented Aug 23, 2019

Should be up to date. What does a retesteth run at --verbosity 6 say? Given that this is a state root mismatch and not off the filler it could be many things.

@winsvega
Copy link
Collaborator

Also to see the post state
'--fullstate --poststate'

@shemnon
Copy link
Contributor Author

shemnon commented Aug 23, 2019

Is this off of develop/head? I don't think the contract is firing as there is nothing stored in storage - https://gist.github.com/shemnon/adf5b5d24f05aadbbcd2272405b1c3e0

@winsvega
Copy link
Collaborator

The precompiled must be supported by the client implementation. You use Pantheon ?

@shemnon
Copy link
Contributor Author

shemnon commented Aug 23, 2019

Yes, Pantheon (as it currently is named). it's an internal build off of head.

@shemnon
Copy link
Contributor Author

shemnon commented Aug 23, 2019

The failing vectors are d = {0,1,2,6}, which are all the bad data failure cases. All the "good data" cases are passing. Geth has ending balance of 999999999999901559711 for 0,1, and 2 whereas pantheon has multiple balances (caused by intrinsic tx cost changes) of around 999999999999999954832 (at gas 0). Pantheon charges 45,168 wei for this bad call and Geth 98,440,289 at gas 0 and the full limit at gas 1, 2, and 3.

When the precompile fails due to validation is the correct failure mode to fail the call or to fail the whole tx and charge the gas limit?

@winsvega
Copy link
Collaborator

I am afraid geth didn't implement the contract yet. If so then its just a call to empty address. If the precompile is implemented but fails then it just eat the whole gas. If the precompile works need to check the results againt other clients. (By putting the results into storage)

Copy link
Collaborator

@winsvega winsvega left a comment

Choose a reason for hiding this comment

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

please add expect section for uncovered transactions
and refill with latest retesteth (ethereum/retesteth#48)
and with latest lllc version

rebase on tests:develop to fetch the retesteth config first

@shemnon
Copy link
Contributor Author

shemnon commented Sep 16, 2019

I think the no TXes complaint is from the lack of the Istanbul config. I pulled over the one I had in place for Martin's changes.

@winsvega
Copy link
Collaborator

hmm. how you created this tests? did you get any errors?
I see this errors on that retesteth version:

Error: A transaction was specified, but there is no execution results in a test! CALLBlake2f, fork: Istanbul, TrInfo: d: 9, g: 0, v: 0 (Errored TR: d: 0, g: 1, v: 0 (CALLBlake2f)
Error: A transaction was specified, but there is no execution results in a test! CALLBlake2f, fork: Istanbul, TrInfo: d: 9, g: 0, v: 0 (Errored TR: d: 0, g: 2, v: 0 (CALLBlake2f)
Error: A transaction was specified, but there is no execution results in a test! CALLBlake2f, fork: Istanbul, TrInfo: d: 9, g: 0, v: 0 (Errored TR: d: 0, g: 3, v: 0 (CALLBlake2f)
Error: A transaction was specified, but there is no execution results in a test! CALLBlake2f, fork: Istanbul, TrInfo: d: 9, g: 0, v: 0 (Errored TR: d: 1, g: 1, v: 0 (CALLBlake2f)
Error: A transaction was specified, but there is no execution results in a test! CALLBlake2f, fork: Istanbul, TrInfo: d: 9, g: 0, v: 0 (Errored TR: d: 1, g: 2, v: 0 (CALLBlake2f)
Error: A transaction was specified, but there is no execution results in a test! CALLBlake2f, fork: Istanbul, TrInfo: d: 9, g: 0, v: 0 (Errored TR: d: 1, g: 3, v: 0 (CALLBlake2f)
Error: A transaction was specified, but there is no execution results in a test! CALLBlake2f, fork: Istanbul, TrInfo: d: 9, g: 0, v: 0 (Errored TR: d: 2, g: 1, v: 0 (CALLBlake2f)
Error: A transaction was specified, but there is no execution results in a test! CALLBlake2f, fork: Istanbul, TrInfo: d: 9, g: 0, v: 0 (Errored TR: d: 2, g: 2, v: 0 (CALLBlake2f)
Error: A transaction was specified, but there is no execution results in a test! CALLBlake2f, fork: Istanbul, TrInfo: d: 9, g: 0, v: 0 (Errored TR: d: 2, g: 3, v: 0 (CALLBlake2f)
Error: A transaction was specified, but there is no execution results in a test! CALLBlake2f, fork: Istanbul, TrInfo: d: 9, g: 0, v: 0 (Errored TR: d: 3, g: 1, v: 0 (CALLBlake2f)
Error: A transaction was specified, but there is no execution results in a test! CALLBlake2f, fork: Istanbul, TrInfo: d: 9, g: 0, v: 0 (Errored TR: d: 3, g: 2, v: 0 (CALLBlake2f)
Error: A transaction was specified, but there is no execution results in a test! CALLBlake2f, fork: Istanbul, TrInfo: d: 9, g: 0, v: 0 (Errored TR: d: 3, g: 3, v: 0 (CALLBlake2f)
Error: A transaction was specified, but there is no execution results in a test! CALLBlake2f, fork: Istanbul, TrInfo: d: 9, g: 0, v: 0 (Errored TR: d: 9, g: 1, v: 0 (CALLBlake2f)
Error: A transaction was specified, but there is no execution results in a test! CALLBlake2f, fork: Istanbul, TrInfo: d: 9, g: 0, v: 0 (Errored TR: d: 9, g: 2, v: 0 (CALLBlake2f)
Error: A transaction was specified, but there is no execution results in a test! CALLBlake2f, fork: Istanbul, TrInfo: d: 9, g: 0, v: 0 (Errored TR: d: 9, g: 3, v: 0 (CALLBlake2f)
Error: Test has transaction uncovered with expect sections!  (CALLBlake2f)

meaning there are some transactions that does not have post condition checks

# Conflicts:
#	Retesteth/default/genesis/Istanbul.json
@shemnon
Copy link
Contributor Author

shemnon commented Sep 17, 2019

dannos-mbp:build dannoferrin$ PATH+=":../../../ethereum/solidity/build/lllc/" ./retesteth/retesteth -t  GeneralStateTests/stPreCompiledContracts2 -- --testpath ../../../shemnon/ethereum-tests/ --clients besu --singletest CALLBlake2f --filltests --fillblockchain
Unknown option: --fillblockchain
dannos-mbp:build dannoferrin$ PATH+=":../../../ethereum/solidity/build/lllc/" ./retesteth/retesteth -t  GeneralStateTests/stPreCompiledContracts2 -- --testpath ../../../shemnon/ethereum-tests/ --clients besu --singletest CALLBlake2f --filltests --fillchain
Running tests using path: "../../../shemnon/ethereum-tests/"
Running 1 test case...
Active client configurations: 'besu '
Filter: 'CALLBlake2f'
Running tests for config 'Hyperledger Besu on TCP' 2
Test Case "stPreCompiledContracts2": 
100%

*** No errors detected
dannos-mbp:build dannoferrin$ PATH+=":../../../ethereum/solidity/build/lllc/" ./retesteth/retesteth -t  GeneralStateTests/stPreCompiledContracts2 -- --testpath ../../../shemnon/ethereum-tests/ --clients besu --singletest CALLCODEBlake2f --filltests --fillchain
Running tests using path: "../../../shemnon/ethereum-tests/"
Running 1 test case...
Active client configurations: 'besu '
Filter: 'CALLCODEBlake2f'
Running tests for config 'Hyperledger Besu on TCP' 2
Test Case "stPreCompiledContracts2": 
100%

*** No errors detected
dannos-mbp:build dannoferrin$ PATH+=":../../../ethereum/solidity/build/lllc/" ./retesteth/retesteth -t  GeneralStateTests/stTimeConsuming -- --testpath ../../../shemnon/ethereum-tests/ --clients besu --singletest CALLBlake2f_MaxRounds --filltests --fillchain
Running tests using path: "../../../shemnon/ethereum-tests/"
Running 1 test case...
Skipping stTimeConsuming because --all option is not specified.

*** No errors detected
dannos-mbp:build dannoferrin$ PATH+=":../../../ethereum/solidity/build/lllc/" ./retesteth/retesteth -t  GeneralStateTests/stTimeConsuming -- --testpath ../../../shemnon/ethereum-tests/ --clients besu --singletest CALLBlake2f_MaxRounds --filltests --fillchain --all
Running tests using path: "../../../shemnon/ethereum-tests/"
Running 1 test case...
Active client configurations: 'besu '
Filter: 'CALLBlake2f_MaxRounds'
Running tests for config 'Hyperledger Besu on TCP' 2
Test Case "stTimeConsuming": 
100%

@winsvega
Copy link
Collaborator

winsvega commented Sep 17, 2019

No. I don't understand why. the tests state that retesteth used commit is from master branch. but the behaviour of your retesteth is way outdated. It is filling the blockchain tests into a wrong folder name. and it does not perform the state test validity check.

could you double check that you running the tool on latest commit from the master branch?
ethereum/retesteth@eeb4805

@shemnon
Copy link
Contributor Author

shemnon commented Sep 18, 2019

at @eeb4805 I get a whole lot of WARNING: Error getting LLLC Version

@shemnon
Copy link
Contributor Author

shemnon commented Sep 18, 2019

lllc version is hand fed because the lllc version logic is broken on retesteth on a mac.

@winsvega
Copy link
Collaborator

this is very strange. you must at least see this errors:

Error: A transaction was specified, but there is no execution results in a test! CALLBlake2f, fork: Istanbul, TrInfo: d: 9, g: 0, v: 0 (Errored TR: d: 0, g: 1, v: 0 (CALLBlake2f)
Error: A transaction was specified, but there is no execution results in a test! CALLBlake2f, fork: Istanbul, TrInfo: d: 9, g: 0, v: 0 (Errored TR: d: 0, g: 2, v: 0 (CALLBlake2f)
Error: A transaction was specified, but there is no execution results in a test! CALLBlake2f, fork: Istanbul, TrInfo: d: 9, g: 0, v: 0 (Errored TR: d: 0, g: 3, v: 0 (CALLBlake2f)
Error: A transaction was specified, but there is no execution results in a test! CALLBlake2f, fork: Istanbul, TrInfo: d: 9, g: 0, v: 0 (Errored TR: d: 1, g: 1, v: 0 (CALLBlake2f)

how you deal with lllc version problem ?

@shemnon
Copy link
Contributor Author

shemnon commented Sep 19, 2019

I don't see those errors, never have.

I hand edited the LLLC versions into the JSON. I think there is some problems with the way you detect if LLLC is present on mac.

@winsvega
Copy link
Collaborator

winsvega commented Sep 19, 2019

I've just compiled lllc and retesteth on mac and created a test for it:
https://circleci.com/gh/ethereum/retesteth/665?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

the build went through just fine. smth is wrong on your end.

I hand edited the LLLC versions into the JSON. I think there is some problems with the way you detect if LLLC is present on mac.

You can't just edit those fields. You provide misinformation about the test. If smth went wrong with lllc then the test is not filled correctly. lllc version is only a warning if when you fill the tests there is no lllc code in the test.

@shemnon
Copy link
Contributor Author

shemnon commented Sep 20, 2019

LLLC version - when I put the LLLC setting in the exported environment instead of as a shell parameter the error stops occuring. You will note the version is exactly the same as was copied in by hand.

ethereum/retesteth#52 version includes the v,s,r values that were complained about in another thread. So I re-ran the tests to get those in the blockchain tests.

The transaction errors are flakey, so I think it's something inside of retesteth. I think it is a timing issue as I didn't see them until today and I see them less than 10% or the time and only under load (running a compile in another thread or some ad-filled chrome webpage). The JSON-RPC traces are mostly the same, except that a success will continue with more JSON-RPCs to do the fill. https://gist.github.com/shemnon/72cd75520148dc546e995e63a5ecd938

Have you tried to local test against geth or besu and do you see the same errors consistantly?

@winsvega
Copy link
Collaborator

yes the process is fully deterministic. how can I build pantheon/besu on ubuntu?

@shemnon
Copy link
Contributor Author

shemnon commented Sep 20, 2019

The full version is here.

The short version for this case is

  • Install Java JDK 11+ (make sure javac and java are on the path)
  • clone the repo
  • ./gradlew installdist
  • ./build/install/besu/bin/besu retesteth

"9 - RFC 7693 Appendix-A, 8 million rounds"
],
"data": [
"0x",
Copy link
Member

Choose a reason for hiding this comment

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

Another test suggestion: non-empty data, but fewer than 4 bytes, to catch possible errors of trying to calculate the precompile cost out of it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added as data 10

@shemnon
Copy link
Contributor Author

shemnon commented Sep 23, 2019

Two things

(a) the docs and errors were not particularly clear that all permutations must be covered. The error message could be re-tooled to communicate this better

Missing transaction execution results. All permutations for a test transaction must be covered. Please add the validation data for CALLBlake2f, fork: Istanbul, TrInfo: d: 9, g: 0, v: 0

(b) This error must be produced all the time on all platforms. I only rarely see it on my macbook pro, and then only when I put the system under load. This feels like a memory fencing or threading issues, like data is getting overwritten before it is moved to a more permanent location.

@winsvega
Copy link
Collaborator

winsvega commented Sep 23, 2019

(b)
how is this possible?

the error is added to error list upon test execution. then all errors from the list are stdeerored once threads are merged. so the output is done at the main thread.

@winsvega winsvega merged commit 86b8a27 into ethereum:develop Sep 23, 2019
@shemnon
Copy link
Contributor Author

shemnon commented Sep 24, 2019

I don't know how it's possible, but I have observed that it does occur. Perhaps a memory fence needs to be raised prior to the data check?

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