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

evm: verkle testing extraction #3817

Merged
merged 7 commits into from
Dec 20, 2024
Merged

Conversation

gabrocheleau
Copy link
Contributor

This PR extracts some verkle work from #3716 so that the other PR can remain scoped on running verkle tests.

Copy link

codecov bot commented Dec 18, 2024

Codecov Report

Attention: Patch coverage is 50.27624% with 90 lines in your changes missing coverage. Please review.

Project coverage is 75.56%. Comparing base (4da1f03) to head (15ece2b).
Report is 1 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

Flag Coverage Δ
block 73.74% <ø> (ø)
blockchain 83.23% <ø> (ø)
client 73.66% <ø> (ø)
common 89.82% <50.00%> (-0.12%) ⬇️
devp2p 71.95% <ø> (ø)
evm 64.60% <56.66%> (-0.12%) ⬇️
genesis 100.00% <ø> (ø)
mpt 52.05% <ø> (-0.08%) ⬇️
rlp 95.11% <ø> (ø)
statemanager 67.76% <0.00%> (-0.05%) ⬇️
tx 76.56% <ø> (ø)
util 72.81% <ø> (ø)
vm 57.02% <17.39%> (-0.28%) ⬇️
wallet 79.67% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

historyServeWindow: 8192, // The amount of blocks to be served by the historical blockhash contract
systemAddress: '0xfffffffffffffffffffffffffffffffffffffffe', // The system address
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to have both of these parameters since they are identical?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will admit that I've been a bit confused with regards to those two addresses. I've played around when troubleshooting so this is a remnant of that. I've adjusted it so that it matches the EIP: https://eips.ethereum.org/EIPS/eip-2935

// config
historyStorageAddress: '0xfffffffffffffffffffffffffffffffffffffffe', // The address where the historical blockhashes are stored
historyStorageAddress: '0x0aae40965e6800cd9b1f4b05ff21581047e3f91e', // The address where the historical blockhashes are stored
Copy link
Contributor

Choose a reason for hiding this comment

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

Why doesn't this match the EIP 2935 address?

Copy link
Contributor Author

@gabrocheleau gabrocheleau Dec 19, 2024

Choose a reason for hiding this comment

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

Left ts as is but adjusted the eip 2935 one from evm

Copy link
Contributor

@acolytec3 acolytec3 left a comment

Choose a reason for hiding this comment

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

Few questions/clean up items we should address before merging.

@gabrocheleau
Copy link
Contributor Author

Few questions/clean up items we should address before merging.

I think all has been addressed! Ready for re-review/merge

acolytec3
acolytec3 previously approved these changes Dec 19, 2024
Copy link
Contributor

@acolytec3 acolytec3 left a comment

Choose a reason for hiding this comment

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

LGTM

@gabrocheleau
Copy link
Contributor Author

LGTM

Ty! It seems like the PR wasn't approved, can you re-approve? thanks!

@acolytec3 acolytec3 merged commit cd4afd0 into master Dec 20, 2024
40 of 41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants