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.

Sign up for GitHub

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

Jump to bottom

VM: Custom precompiles #1813

Merged
merged 20 commits into from
Apr 13, 2022
Merged

VM: Custom precompiles #1813

merged 20 commits into from
Apr 13, 2022

Conversation

jochem-brouwer
Copy link
Member

Cherry-picked from #1806

@codecov
Copy link

codecov bot commented Mar 24, 2022 •
edited
Loading

Codecov Report

Merging #1813 (66686c1) into master (1f44637) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

Flag Coverage Δ
block 85.57% <ø> (ø)
blockchain 83.82% <ø> (ø)
client 76.20% <ø> (ø)
common 94.19% <ø> (ø)
devp2p 82.47% <ø> (ø)
ethash 90.76% <ø> (ø)
trie 80.02% <ø> (ø)
tx 88.20% <ø> (ø)
util 92.62% <ø> (ø)
vm 81.59% <100.00%> (+<0.01%) ⬆️

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

@jochem-brouwer jochem-brouwer marked this pull request as draft March 31, 2022 16:29
@jochem-brouwer
Copy link
Member Author

Hardhat tests are failing and should be fixed

@holgerd77
Copy link
Member

Can this get a short analysis if a fix can be applied quickly or not (and then optimally also an update on the fix 😋, but first task would also already bring this further, slowly starting to think about the next small release round, mainly to bring EIP-1153 in on request)?

@holgerd77 holgerd77 mentioned this pull request Apr 5, 2022
Closed
@holgerd77
Copy link
Member

Have moved this back to WIP (see thread above), would be great if this can be tackled soon-ish, so that this can optimally get included into eventual next-week releases.

@jochem-brouwer
Copy link
Member Author

This should fix it, but let's see. I am also suddenly getting local lint errors but that might be because I'm on a different version (the develop version?)

This can definitely get into the next release, will fix today.

@jochem-brouwer
Copy link
Member Author

jochem-brouwer commented Apr 7, 2022 •
edited
Loading

OK, this fixes the hardhat e2e tests, but in c168dd7 it created an issue on CI on client which we cannot reproduce locally. Thoughts?

Note that in the commit before that: da1f81b this error was not there, and all tests passed except hardhat e2e. The test failed because upon setting up each test, npm i failed, because client reported an error when building:

image

@jochem-brouwer
Copy link
Member Author

Tests pass, ready for re-review

@jochem-brouwer jochem-brouwer marked this pull request as ready for review April 7, 2022 18:45
acolytec3
acolytec3 previously requested changes Apr 8, 2022
View reviewed changes
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.

Looks good except you have an open TODO so wanted to understand if we need to wrap that up before merging?

packages/vm/src/index.ts Outdated Show resolved Hide resolved
holgerd77
holgerd77 requested changes Apr 8, 2022
View reviewed changes
packages/vm/src/state/baseStateManager.ts Outdated
const check1 = getActivePrecompiles(this._common).find((a) => a.equals(address))
const check1 = getActivePrecompiles(this._common, this._customPrecompiles).has(
address.buf.toString('hex')
)
Copy link
Member

Choose a reason for hiding this comment

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

This very much feels like the wrong abstraction to me to bake the precompiles even more into StateManager, I guess this was suboptimal from the beginning to have this getActivePrecompiles call in here.

This becomes even more apparent regarding the upcoming StateManager extraction.

Would it be a way to generally remove this check1 and instead add the precompiles (e.g.) to the passed in addressesRemoved array? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah right, I don't know why I added this, it should not be there. I think I had in the original PR also the remark that we should probably wait for the state manager to be extracted.

@jochem-brouwer jochem-brouwer marked this pull request as draft April 8, 2022 14:03
@jochem-brouwer
Copy link
Member Author

I have blocked the PR, I wanted to say let's wait for the extracted state manager but we can't do that since we are pointed to master while that PR #1817 is pointed to develop.

The reason precompiles are added is because they are used to generate an access list. You do not need to add precompiles since these are in the access list by default (they are always warm addresses). We could disable this on master but then it would break things. The alternative is to move this getAccessList from StateManager but then the rebase on develop would probably get a bit nasty... Thoughts? 🤔

@holgerd77
Copy link
Member

Can't we just pass in the precompiles from the outside to the method as I suggested above? 🤔

@jochem-brouwer
Copy link
Member Author

@holgerd77 I somehow misread that, you are right, that'll definitely work and is really clean. Will do 😄

@jochem-brouwer jochem-brouwer added type: test all hardforks This special label enables VM state and blockchain tests for all hardforks on the respective PR. and removed PR state: do-not-merge PR/Issue state: blocked labels Apr 8, 2022
holgerd77
holgerd77 requested changes Apr 11, 2022
View reviewed changes
packages/vm/src/state/baseStateManager.ts Outdated Show resolved Hide resolved
packages/vm/src/state/stateManager.ts Outdated Show resolved Hide resolved
@jochem-brouwer
Copy link
Member Author

Done @holgerd77!

ryanio
ryanio reviewed Apr 11, 2022
View reviewed changes
packages/vm/src/runTx.ts Outdated Show resolved Hide resolved
@ryanio ryanio changed the title Custom precompiles VM: Custom precompiles Apr 11, 2022
holgerd77
holgerd77 approved these changes Apr 13, 2022
View reviewed changes
Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

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

LGTM, great, thanks Jochem! 😄

@holgerd77 holgerd77 marked this pull request as ready for review April 13, 2022 16:52
@holgerd77 holgerd77 merged commit 1d7d419 into master Apr 13, 2022
@holgerd77 holgerd77 deleted the custom-precompiles-bnjs branch April 13, 2022 16:53
@holgerd77 holgerd77 mentioned this pull request May 4, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ryanio ryanio ryanio left review comments

@holgerd77 holgerd77 holgerd77 approved these changes

@acolytec3 acolytec3 acolytec3 left review comments

Assignees
No one assigned
Labels
package: vm PR state: needs review type: feature type: test all hardforks This special label enables VM state and blockchain tests for all hardforks on the respective PR.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jochem-brouwer @holgerd77 @ryanio @acolytec3