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

regenerate all state tests #511

Merged
merged 11 commits into from
Oct 5, 2018
Merged

regenerate all state tests #511

merged 11 commits into from
Oct 5, 2018

Conversation

winsvega
Copy link
Collaborator

No description provided.

"filledwith" : "cpp-1.3.0+commit.619a6438.Linux.g++",
"lllcversion" : "Version: 0.4.20-develop.2018.1.8+commit.2548228b.Linux.g++",
"filledwith" : "testeth 1.5.0.dev2-48+commit.413e4077.dirty",
"lllcversion" : "Version: 0.4.25-develop.2018.8.9+commit.63d071d6.mod.Linux.g++",
Copy link
Member

Choose a reason for hiding this comment

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

Can you please not use an unstable nightly version of solidity/lll? :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I use custom version because it had wrong opcode for create2 I had to fix it manually. Need to check what current release version is.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@axic, sorry, I cant use solidity develop. it has incorrect create2 being translated into 0xfb
the create2 should be translated into 0xf5 https://eips.ethereum.org/EIPS/eip-1014

Copy link
Member

Choose a reason for hiding this comment

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

Fixed in ethereum/solidity#5013.

However using a random nightly version can cause a variety of problems. I'd to either hold off refilling or take the release 0.4.25 and changing that single line in libevmasm/Instruction.h.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

smth changed. this lll translated into smth that cpp now dont understand.
{ (seq (CREATE2 0 0 (lll (seq (mstore 0 0x112233) (revert 0 32)) 0) 0) (SSTORE 0 (RETURNDATASIZE)) )}

it was working before. Now I get invalid instruction

@axic
Copy link
Member

axic commented Sep 18, 2018

@gumb0 @chfast do you plan to make a "final" 1.5.x release of aleth soon? Do you think it makes sense holding back this PR until that is done?

I have the feeling it is a bad idea refilling all tests with alpha software (i.e. aleth 1.5 and solidity 0.5.0 develop).

cc @holiman @pipermerriam

@winsvega
Copy link
Collaborator Author

The main change affecting lots of tests is sstore change. Other then that it should be possible to refill tests without issues very fast

@holiman
Copy link
Contributor

holiman commented Sep 19, 2018

The main change affecting lots of tests is sstore change. Other then that it should be possible to refill tests without issues very fast

Yes, currently the implementation of sstore change causes a lot of tests to start failing. Would be good to get that sorted out. Not sure what's the best way -- is it even possible to regenerate some tests without recompiling the solidity code?

At some point we should change the way we create tests. IMO, only the manually written tests should be versioned, along with scripts that produce the executable (filled) tests. And then we should have build servers that generate stateTests, blockchainTests etc. And clients could download test-bundles, and the build servers could display info about which tests have been changed in a particular bundle etc.

@holiman
Copy link
Contributor

holiman commented Sep 19, 2018

In current develop on tests, the rlp here is missing the mixHash and nonce, and fails rlp decoding.

Just checked, it's still wrong on this PR.

build/bin/rlpdump -hex 0xf9028ff901d0a036750291f1a8429aeb553a790dc2d149d04dbba0ca4cfc7fd5eb12d478117c9fa01dcc4de8dec75d7aab85b567b6ccd41ad312451b948a7413f0a142fd40d49347948888f1f195afa192cfee860698584c030f4c9db1a0337e81b561c683320e7ee9c4c9364560a3c5fd2b85bfc166d4c110d734fdc856a0e43a3d032e8c63ea45d7237fb886a5c009fd50277c3a6d5f4ff96801497dd87da0e8e2ce84e12ec67439103ffa365e13ffea2c6f10c90b783e34d033b3ce56752eb90100000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000008302000001832fefd88301b6f3845b7dd29b80f8b9f8608001830249f094ec0e71ad0a90ffe1909d27dac207f7680abba42d80801ba03d666b40cc93bea89201058674c2f1eb50ff5e50eeea2dca60f9ce367a736d7ea0760ee620e6a73d7ea93f00664a108aedb9dd0c0bd6a57ee9b5bbce0bd76606b2f8550101830249f0800a896000600060006000f51ba048eadf8a79c2f5f001f58745dafc4cda11460e7f40661fbae3f48b7ef055134ca049a83ce358e2d8c1b2e10772a6a23e939305032138aca1e4c40c6b41ccfdc3f9c0
[
  [
    36750291f1a8429aeb553a790dc2d149d04dbba0ca4cfc7fd5eb12d478117c9f,
    1dcc4de8dec75d7aab85b567b6ccd41ad312451b948a7413f0a142fd40d49347,
    8888f1f195afa192cfee860698584c030f4c9db1,
    337e81b561c683320e7ee9c4c9364560a3c5fd2b85bfc166d4c110d734fdc856,
    e43a3d032e8c63ea45d7237fb886a5c009fd50277c3a6d5f4ff96801497dd87d,
    e8e2ce84e12ec67439103ffa365e13ffea2c6f10c90b783e34d033b3ce56752e,
    00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000,
    020000,
    01,
    2fefd8,
    01b6f3,
    5b7dd29b,
    "",
  ],

The 5b7dd29b is the timestamp, then follows an empty extradata, which should be followed by MixDigest and Nonce.

@gumb0
Copy link
Member

gumb0 commented Sep 19, 2018

I have the feeling it is a bad idea refilling all tests with alpha software (i.e. aleth 1.5 and solidity 0.5.0 develop).

I have even worse feeling about generating tests with unknown commit of aleth.
The new tests have

            "filledwith" : "testeth 1.5.0.dev2-48+commit.413e4077.dirty",

@winsvega Can you clarify what is this commit? I can't find it in the repo.
Also it should better not be "dirty"

@winsvega
Copy link
Collaborator Author

winsvega commented Sep 19, 2018

it is origin/fixtesthash rebased on develop
what dirty means for aleth and how to fix it?

@axic
Copy link
Member

axic commented Sep 19, 2018

dirty, just like mod on solidity, means you have a local change (not committed to git) in the build

@winsvega winsvega changed the title regenerate all tests regenerate all state tests Sep 24, 2018
test.py Outdated
# fillerSource = jsonTest[test]["_info"]["source"]
# fillerHash = jsonTest[test]["_info"]["sourceHash"]
# if fillerHash != hashFile(fillerSource):
# _logerror("Filler hash is different:", jsonFile)
Copy link
Member

Choose a reason for hiding this comment

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

Why not fix broken test cases? Disabling the validator script seems to be a very bad idea. @ehildenb has put in a lot of effort to do validation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't know how to write json_spirit::write_string(jsdata, true) in python.
Jared did not respond to my mail.

Copy link
Member

@pipermerriam pipermerriam Sep 26, 2018

Choose a reason for hiding this comment

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

what does json_spirit::write_string(jsdata, true) do, I'll give you a python equivalent. (assuming this is still an issue)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this issue is fixed. now the issue is how to calculate hash of json object of .yml test in python.
in cpp this function is used to construct json out of .yml

json_spirit::mValue convertYamlNodeToJson(YAML::Node _node)
{
    if (_node.IsNull())
        return json_spirit::mValue();

    if (_node.IsScalar())
    {
        if (_node.Tag() == "tag:yaml.org,2002:int")
            return _node.as<int>();
        else
            return _node.as<string>();
    }

    if (_node.IsMap())
    {
        json_spirit::mObject jObject;
        for (auto const& i : _node)
            jObject.emplace(i.first.as<string>(), convertYamlNodeToJson(i.second));
        return jObject;
    }

    if (_node.IsSequence())
    {
        json_spirit::mArray jArray;
        for (size_t i = 0; i < _node.size(); i++)
            jArray.emplace_back(convertYamlNodeToJson(_node[i]));
        return jArray;
    }

    BOOST_ERROR("Error parsing YAML node. Element type not defined!");
    return json_spirit::mValue();
}

Copy link
Collaborator Author

@winsvega winsvega Sep 27, 2018

Choose a reason for hiding this comment

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

however when I use this in python, the hash is different:

        fparsed = json.dump(yaml.load(f), sys.stdout, indent=4)
        s = json.dumps(fparsed, separators=(',', ':'))
        k.update(s.encode('utf-8'))
        return k.hexdigest()

s = json.dumps(fparsed, separators=(',', ':')) is equivalent to json_spirit::write_string(jsdata, false)

Copy link
Collaborator Author

@winsvega winsvega Sep 28, 2018

Choose a reason for hiding this comment

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

ok the current issue is that json.dumps when converting yml to json
thread indexes in a different way:

'0x01': '0x00'  ->   '1': '0x00'

instead of putting it as is 0x01 it converts the index into 1
@pipermerriam if you could help with this. it should fix filler hash check in travis

@winsvega
Copy link
Collaborator Author

winsvega commented Sep 24, 2018

@axic
filler hash calculation alg is changed. to :

1) construct json object out of *Filler.json
2) put hash(json_spirit::write_string(jsonObjectOfTheFiller, false))  as a Filler hash

I am ready to merge this PR.

@5chdn
Copy link

5chdn commented Sep 30, 2018

uh, how is anyone supposed to review that?

@winsvega
Copy link
Collaborator Author

winsvega commented Oct 1, 2018

try running Constantinople enabled client using tests from this branch

@winsvega winsvega force-pushed the refill branch 3 times, most recently from c9bfc8e to 2db5438 Compare October 2, 2018 21:59
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.

6 participants