-
Notifications
You must be signed in to change notification settings - Fork 4
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
Use tests in BlockchainTests
#35
Comments
Hmm yeah I agree. The tests under |
I think one difference is the absence of variant cases on the BlockchainTests side. For example comparing the Shanghai version for BlockchainTests/Call1MB1024Calldepth.json vs GeneralStateTests/Call1MB1024Calldepth.json, there's only 1 txn vs 2 (the second one is the same). |
Hmm yeah. I think what actually ends up happening during the test expansion is each of these variants gets expanded into separate "test sections" (for lack of a better word). So like in this case, the two txns are there, but they have their own sections "Shanghai" : [
{
"hash" : "0x5de8ccc907a2cf4b21c0ee18f6ccd171f7b6a517b149cc7fb5398ec40d619e0f",
"indexes" : {
"data" : 0,
"gas" : 1,
"value" : 0
},
"logs" : "0x1dcc4de8dec75d7aab85b567b6ccd41ad312451b948a7413f0a142fd40d49347",
"txbytes" : "0xf861800a840ee6b28094bbbf5374fce5edbc8e2a8697c15331677e6ebf0b0a801ba0462186579a4be0ad8a63224059a11693b4c0684b9939f6c2394d1fbe045275f2a059d73f99e037295a5f8c0e656acdb5c8b9acd28ec73c320c277df61f2e2d54f9"
},
{
"hash" : "0x485947362d7a26bfe59efa9a2581036f48cbdb09f9b704130ecd0dc2bf969ec7",
"indexes" : {
"data" : 0,
"gas" : 0,
"value" : 0
},
"logs" : "0x1dcc4de8dec75d7aab85b567b6ccd41ad312451b948a7413f0a142fd40d49347",
"txbytes" : "0xf860800a830249f094bbbf5374fce5edbc8e2a8697c15331677e6ebf0b0a801ba06843bbbe573744c46cf47f1c126c11a53cffa1bcc3eeb0e0d328e6a73ecc5447a02062c2fbb34bfea606f3cf268b5a94a81d68e748d4d896c514061c03e8acb774"
}
] |
Ah good point, I had missed this! All good then 😄 |
In addition to the added support for the receipt trie, the latest PRs have also added a few fields to the
some of which seem kind of cumbersome to parse ahead of proving |
Thanks for the headsup @Nashtare. Sorry that I haven't finished this yet. I have something in-progress, but I keep getting pulled to work on other tasks. I'm aiming to have this PR done by the end of tomorrow. |
And actually another topic regarding recent updates on plonky2 side. Now that we observe public values in the challenger, we can't process tests that have a I know that @wborgeaud wasn't inclined in supporting 2-limb gaslimit as kinda needless in practice, though that means there would be now many tests that aren't supported natively (we could manually alter their Do you have any opinion on the matter? |
Hmm... So I'm a bit naive when it comes to a lot of the plonky2 internals, but maybe could we clamp the gas limit in these cases to |
Hmm, we could test on a case-by-case basis but for the few of which I actually tried setting this to
hence I couldn't find a really satisfactory solution. |
@wborgeaud I was wondering if your opinion had changed on the matter:
I am currently working on updating the runner to latest plonky2, but the fact that so many tests are uncheckable (altering block_gaslimit leads to failure anyway in most cases) kind of concerns me, especially with all the heavy changes that happened on the zkEVM the past few months. |
Yeah sounds good, I agree the performance overhead is pretty negligible. |
Should we change
|
Yes sounds good too. Just one question, do you have an idea if most of these tests will run in a reasonable time? Like I just want to make sure these tests are not stupidly large and will make the runner run out of memory anyways. |
Most of them are runnable on non-beefy machines yeah. They are already run with the current instance, we just "Ignore" them if the prover fails with GasLimit error. |
We currently use the tests in the GeneralStateTests folder of the Ethereum tests repo.
Each such test seems to be replicated in the BlockchainTests/GeneralStateTests folder, in a different format. This format includes useful information like the roots of the receipts and transactions tries, the Bloom filter, and the withdrawals root.
It would be helpful to modify the runner to use these tests instead, so that we can verify these data. An immediate application would be to verify the receipts trie root produced by 0xPolygonZero/plonky2#1097.
The text was updated successfully, but these errors were encountered: