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

reintroduce stEIP2537 tests #1366

Merged
merged 3 commits into from
May 3, 2024
Merged

reintroduce stEIP2537 tests #1366

merged 3 commits into from
May 3, 2024

Conversation

winsvega
Copy link
Collaborator

@winsvega winsvega commented Apr 11, 2024

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
Copy link

acolytec3 commented Apr 11, 2024

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
Copy link
Collaborator Author

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

@acolytec3
Copy link

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
Copy link
Collaborator Author

winsvega commented Apr 15, 2024

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
Copy link
Contributor

holiman commented Apr 15, 2024

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

@winsvega
Copy link
Collaborator Author

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

@holiman
Copy link
Contributor

holiman commented Apr 16, 2024

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

@winsvega
Copy link
Collaborator Author

winsvega commented Apr 16, 2024

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
Copy link

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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.

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.

3 participants