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

Add new precompile PrecompileRegistry #2138

Merged
merged 8 commits into from Mar 28, 2023
Merged

Conversation

nanocryk
Copy link
Contributor

What does it do?

Implements this proposal: https://forum.moonbeam.foundation/t/proposal-status-idea-precompile-to-check-active-precompiles/384

It also allows for any user to set the "dummy code" of precompiles to make them callable from Solidity (which adds an empty code check). This is currently done through democracy with set_storage, which is far from ideal.

For audit

Ensure that it is not possible to change the code of non-precompile addresses.

@nanocryk nanocryk added A0-pleasereview Pull request needs code review. B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes C1-low Does not elevate a release containing this beyond "low priority". D9-needsaudit👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited not-breaking Does not need to be mentioned in breaking changes labels Feb 28, 2023
Copy link
Contributor

@notlesh notlesh 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, I think... I am unclear about a couple things:

  • Anyone can insert the dummy code as long as the precompile is active? I thought we sometimes used this bug feature to control whether a contract could call a precompile, allowing cases where only an EOA can call.
  • There is no removing the dummy code, but a precompile could be modified (via runtime upgrade) to become inactive, which would cause it to always revert when called -- right?

@nanocryk
Copy link
Contributor Author

Anyone can insert the dummy code as long as the precompile is active? I thought we sometimes used this bug feature to control whether a contract could call a precompile, allowing cases where only an EOA can call.

This only prevents code written in Solidity that uses interfaces, which automatically appends codesize checks. address(X).call(...) or assembly will not perform the size-check. This is why we have proper capabilities list with PrecompileSetBuilder.

There is no removing the dummy code, but a precompile could be modified (via runtime upgrade) to become inactive, which would cause it to always revert when called -- right?

Precompile should be replaced with RemovedPrecompileAt, but otherwise yes if it is really removed from the precompile set it will use the "always revert" code.

@nanocryk nanocryk merged commit a3e75b4 into master Mar 28, 2023
18 checks passed
@nanocryk nanocryk deleted the jeremy-precompile-registry branch March 28, 2023 11:58
@crystalin crystalin changed the title PrecompileRegistry Add new precompile PrecompileRegistry Apr 13, 2023
@notlesh notlesh added D1-audited👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited and removed D9-needsaudit👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Apr 26, 2023
imstar15 pushed a commit to OAK-Foundation/moonbeam that referenced this pull request May 16, 2023
* add IsActivePrecompile trait in utils

* PrecompileRegistry precompile

* toml-sort

* record 1 db read cost

* add registry to moonbase

* typo

* update precompiles tests in basebase

* separate RevertPrecompile and RemovedPrecompileAt
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A0-pleasereview Pull request needs code review. B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes C1-low Does not elevate a release containing this beyond "low priority". D1-audited👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited not-breaking Does not need to be mentioned in breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants