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 785: enable and disable access lists in the runtime #1839

Merged

Conversation

Nemanja0x
Copy link
Contributor

@Nemanja0x Nemanja0x commented Aug 21, 2023

Description

  1. Being able to enable/disable access list for contracts/txs during runtime
  2. Access list does not have to be defined during genesis but it can be enabled later on if access lists owner is defined during genesis.
  3. Access lists owner can only be defined during genesis creation (--access-lists-owner flag).
    Genesis command example:
./polygon-edge genesis 
   --consensus polybft \
  --block-gas-limit 10000000 \
  --premine 0x85da99c8a7c2c95964c8efd687e95e632fc533d6:1000000000000000000000 \
  --premine 0x0000000000000000000000000000000000000000 \
  --epoch-size 10 \
  --reward-wallet 0xDEADBEEF:1000000 \
  --access-lists-owner 0xF8fC8D3D271ACACfcA412984f76e18Ad616BdCBa
  --burn-contract 0:0x0000000000000000000000000000000000000000
  1. Access lists owner (new role which will be introduced) and admin can enable/disable an access list
  2. Access lists owner or admin can modify an access list before it was enabled
  3. Smart contract that uses bridge allow/block lists has its own logic for enabling/disabling those lists and that logic is still the same as it was before.
  4. If access lists owner is specified all access lists smart contracts will be deployed during the genesis phase and access list owner will be owner/creator of all of them
    7.1 That will be the case even if bridge allow list and bridge block list are empty
  5. E2E test to support this feature

Changes include

  • Bugfix (non-breaking change that solves an issue)
  • Hotfix (change that solves an urgent issue, and requires immediate attention)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (change that is not backwards-compatible and/or changes current functionality)

Checklist

  • I have assigned this PR to myself
  • I have added at least 1 reviewer
  • I have added the relevant labels
  • I have updated the official documentation
  • I have added sufficient documentation in code

Testing

  • I have tested this code with the official test suite
  • I have tested this code manually

Documentation update

Please link the documentation update PR in this section if it's present, otherwise delete it

@Nemanja0x Nemanja0x added the feature New update to Polygon Edge label Aug 21, 2023
@igorcrevar igorcrevar force-pushed the feature/EVM-785_Enable_and_disable_access_lists_n_the_runtime branch from b651de0 to 0a17c51 Compare August 21, 2023 10:45
@igorcrevar igorcrevar requested a review from a team August 21, 2023 11:00
@igorcrevar igorcrevar force-pushed the feature/EVM-785_Enable_and_disable_access_lists_n_the_runtime branch from 0a17c51 to 741c3a5 Compare August 21, 2023 17:51
Copy link
Contributor

@rachit77 rachit77 left a comment

Choose a reason for hiding this comment

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

LGTM

chain/params.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@Stefan-Ethernal Stefan-Ethernal left a comment

Choose a reason for hiding this comment

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

Generally looking good to me. However I'd consider renaming SuperAdminAllowBlock to SuperAdminAccessLists everywhere.

chain/params.go Outdated Show resolved Hide resolved
state/runtime/addresslist/addresslist.go Outdated Show resolved Hide resolved
state/runtime/addresslist/addresslist.go Outdated Show resolved Hide resolved
state/runtime/addresslist/addresslist.go Outdated Show resolved Hide resolved
state/executor.go Outdated Show resolved Hide resolved
@igorcrevar igorcrevar marked this pull request as ready for review August 22, 2023 07:58
@igorcrevar igorcrevar force-pushed the feature/EVM-785_Enable_and_disable_access_lists_n_the_runtime branch from 8a61025 to 0387a8b Compare August 22, 2023 08:39
@igorcrevar igorcrevar self-requested a review August 22, 2023 10:27
chain/params.go Outdated Show resolved Hide resolved
state/executor.go Show resolved Hide resolved
Copy link
Contributor

@vcastellm vcastellm left a comment

Choose a reason for hiding this comment

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

Where's the superadmin stored?

@igorcrevar igorcrevar force-pushed the feature/EVM-785_Enable_and_disable_access_lists_n_the_runtime branch from a48f746 to 7a759df Compare August 22, 2023 11:14
@igorcrevar
Copy link
Contributor

igorcrevar commented Aug 22, 2023

Where's the superadmin stored?

In genesis file in configuration. If we need to store that info in storage I think it would be easier to put superadmin for each list (otherwise we need to create another storage address just for that superadmin)

Copy link
Contributor

@vcastellm vcastellm left a comment

Choose a reason for hiding this comment

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

Overall LGTM but there's some consistency details:

  1. Bridge SCs function naming is different see https://github.com/0xPolygon/core-contracts/blob/main/contracts/lib/AccessList.sol#L20, this can be confusing, we can adapt it in both sides, even though I like this naming better
  2. Consider emitting the event too
  3. Now that we're introducing the concept of super admin would be great to make him the owner of the bridge ACL, I have doubt on this tho.

@vcastellm
Copy link
Contributor

vcastellm commented Aug 23, 2023

Some of the considerations can be implemented in follow-up PRs but the minimum set of changes to be implemented here are:

  1. If there's a superAdmin defined, we deploy Predicate ACLs contracts with both lists enabled (useBridgeAllowList/useBridgeBlockList) hardcoded to true, and superAdmin as owner. https://github.com/0xPolygon/polygon-edge/blob/develop/consensus/polybft/polybft.go#L182-L191 as we will control the usage using the precompile interface.
  2. Assign the superAdmin to be the lists admin here https://github.com/0xPolygon/polygon-edge/blob/develop/consensus/polybft/polybft.go#L168-L178
  3. Include new ACLs interface in the docs https://wiki.polygon.technology/docs/supernets/operate/deploy/access-control/supernets-allowlist-add-remove it's currently missing, and usage examples cc/ @DannyS03

@igorcrevar igorcrevar force-pushed the feature/EVM-785_Enable_and_disable_access_lists_n_the_runtime branch from bda2c97 to 4e072ae Compare August 23, 2023 09:12
Copy link
Contributor

@vcastellm vcastellm left a comment

Choose a reason for hiding this comment

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

After latest modifications and several conversations with Igor, we've implemented the minimum set of changes that makes sense from the UX perspective, fixed several issues and refactored some parts, this PR should be ready to be merged.

Some extra improvements will be described in an RFC and implemented in the following weeks.

LGTM

@igorcrevar igorcrevar merged commit 85af1b3 into develop Aug 25, 2023
6 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Aug 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature New update to Polygon Edge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants