ethereum / tests

Common tests for all Ethereum implementations
MIT License
557 stars 319 forks source link

reintroduce stEIP2537 tests #1366

Closed winsvega closed 6 months ago

winsvega commented 7 months ago

the tests are implementation of 2020, so it is in EIPTests and probably does not match the current implementation. reintroduced by the request from ethereumjs

acolytec3 commented 7 months ago

I've done an initial round of testing here and see at least 1 issue. It appears that all the precompile addresses in the tests are off by 1. For example, image g1_add_27.json calls the G1ADD precompile which is at precompile address 0x000000000000000000000000000000000000000b is being called at 0x000000000000000000000000000000000000000a in the test so I think they all need to be regenerated using the updated precompile addresses from the EIP. It's also possible gas calculations are off since I think there's been some movement there since these test were originally written.

winsvega commented 7 months ago

is there evmone/geth/numbus implementation of this EIP I can fix and refill and see if it works

acolytec3 commented 7 months ago

It appears that geth does. They merged the initial PR back in 2020 and just merged another one last week to update the precompile addresses.

winsvega commented 6 months ago

can't get the same result on latest geth as the test expects. (after changin 0x00a address to 0x00b ) I pushed filler format update. but still need to replace 0x00a addresses in fillers. the fillers contain just plain byte code which is not good. we can't really see whats happening in the tests. I suggest to rework this tests using either yul or python scripts.

holiman commented 6 months ago

How are you doing to make geth enable 2537? I'm not sure we have it exposed...?

winsvega commented 6 months ago

ah, I see, need to enable 2537. can it be plussed ?

holiman commented 6 months ago

Since it adds precompiles, unfortunately it cannot be plussed. As soon as https://github.com/ethereum/go-ethereum/pull/29552 is merged, they can be enabled using the name Prague.

winsvega commented 6 months ago

I was able to upgrade and refill 50% of the tests. I guess something went wrong in the remaining tests when trying to upgrade precompiles addresses in the bytecode. need investigation. the execution just gets reverted. and since the code is bytecode it is difficult to trace the logic.

the reliability of the tests is low. since the expect section is not strong. it checks for some hashes but I am afraid the logic of the tests could be violated. as I get the correct hash for the expect section using different precompile addresses than the one intended in the tests.

as of now I just moved all the precompile addresses +1 in the test fillers.

@acolytec3 can you also generate fillers? g1_add27 does not work for me after replacing 000..000a to 00...00b and one more 00000000000000000010 to ...00000000000000000011 or just change the addresses in already generated test. I can't get the same expected hash after execution

                    "095e7baea6a6c7c4c2dfeb977efac326af552d87" : {
                        "balance" : "1000",
                        "storage" : {
                            "0x02" : "0x99cb0c92bf4949ae75479ba813a148ceb0a9c2bf229c37a4d1e10ed60fcb880b"
                        }
                    },

should I change the second data arg from 0000000000000000000000000000000000000000000000000000000000000010 to 11 ? looks like precompile address

acolytec3 commented 6 months ago

I was able to upgrade and refill 50% of the tests. I guess something went wrong in the remaining tests when trying to upgrade precompiles addresses in the bytecode. need investigation. the execution just gets reverted. and since the code is bytecode it is difficult to trace the logic.

the reliability of the tests is low. since the expect section is not strong. it checks for some hashes but I am afraid the logic of the tests could be violated. as I get the correct hash for the expect section using different precompile addresses than the one intended in the tests.

as of now I just moved all the precompile addresses +1 in the test fillers.

@acolytec3 can you also generate fillers? g1_add27 does not work for me after replacing 000..000a to 00...00b and one

Unfortunately, our client doesn't currently have this capability though @jochem-brouwer and I were discussing if there was a simple way for us to implement the necessary code to do so.

more 00000000000000000010 to ...00000000000000000011 or just change the addresses in already generated test. I can't get the same expected hash after execution

                    "095e7baea6a6c7c4c2dfeb977efac326af552d87" : {
                        "balance" : "1000",
                        "storage" : {
                            "0x02" : "0x99cb0c92bf4949ae75479ba813a148ceb0a9c2bf229c37a4d1e10ed60fcb880b"
                        }
                    },

should I change the second data arg from 0000000000000000000000000000000000000000000000000000000000000010 to 11 ? looks like precompile address

As as far as updating the tests, I haven't looked deeply enough into the calldata to actually know what all needs to be updated. I could just see from our own logging when trying to run these tests that the referenced precompile addresses were out of sync. Will need to look into the actual construction of the tests more to determine what other changes are needed.

winsvega commented 6 months ago

yes, I can't gurantee the test is doing meaningful logic, because I don't have the bytecode source. is @shamatar (https://github.com/shamatar) still in ethereum?

winsvega commented 6 months ago

ok, I fixed all the tests now. the hashes match the expect section. please check with your specs and debug the vmtrace that it really does the bls logic.