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 initial precompile configs and handle them #226

Merged
merged 39 commits into from
Sep 22, 2022

Conversation

ceyonur
Copy link
Collaborator

@ceyonur ceyonur commented Aug 24, 2022

Adds initial precomple configs to existing precompile so they can be activated and perform their operations through configs, without requiring an interaction with actual contracts.

@ceyonur ceyonur marked this pull request as ready for review August 26, 2022 09:27
precompile/allow_list.go Outdated Show resolved Hide resolved
return false
}

return areEqualAddressLists(c.EnabledAddresses, other.EnabledAddresses)
Copy link
Collaborator

Choose a reason for hiding this comment

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

doesn't this make it impossible to change the order of addresses?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes. do you think we should flex this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just trying to think about how this interacts with the hashing / canonical representation.
So if we wanted to make the canonical order sorted, fixing it here would make this backwards incompatible.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We could also add a function to the interface that returns a canonical representation that can be used in hashing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think we need to sort these lists. I think it's ok to expect the same order every time since hash(list[b,c,a]) != hash(list[a,b,c]). This is because the byte representation of those lists are not equal. JSON arrays are also ordered so users should intuitively know that providing any different list can cause issues.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since Equal will be used by the node locally to determine the validity of an upgrade config, if we change this in the future to be "looser" or to sort these lists, then the upgrade process should still be straight forward ie.

we push an upgrade that changes the Equal check to to define equality to ensure that the sorted list is equal and an upgrade that would have previously been invalid (ie a list in a different order, but containing the same set of addresses) will become valid, which is fine since it's just for the node's upgrade config.

We should also make sure that we do not allow duplicates in the list. Created a ticket for this here, since I think this PR is ready to merge: #269.

precompile/allow_list.go Outdated Show resolved Hide resolved
@aaronbuchwald aaronbuchwald added this to the subnet-evm@v0.3.1 milestone Sep 22, 2022
@aaronbuchwald aaronbuchwald merged commit 8e7895d into master Sep 22, 2022
@aaronbuchwald aaronbuchwald deleted the trustless-precompile-configs branch September 22, 2022 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants