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

Allow for precompiles to have arbitrary addresses, potentially more than one. #1016

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
10 changes: 8 additions & 2 deletions frame/evm/precompile/dispatch/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,14 @@ pub fn new_test_ext() -> sp_io::TestExternalities {
}
.assimilate_storage(&mut t)
.expect("Pallet balances storage can be assimilated");
GenesisBuild::<Test>::assimilate_storage(&pallet_evm::GenesisConfig { accounts }, &mut t)
.unwrap();
GenesisBuild::<Test>::assimilate_storage(
&pallet_evm::GenesisConfig {
accounts,
precompiles: vec![],
},
&mut t,
)
.unwrap();
t.into()
}

Expand Down
26 changes: 26 additions & 0 deletions frame/evm/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -467,6 +467,7 @@ pub mod pallet {
#[cfg_attr(feature = "std", derive(Default))]
pub struct GenesisConfig {
pub accounts: std::collections::BTreeMap<H160, GenesisAccount>,
pub precompiles: Vec<(Vec<u8>, H160)>,
}

#[pallet::genesis_build]
Expand Down Expand Up @@ -497,6 +498,10 @@ pub mod pallet {
<AccountStorages<T>>::insert(address, index, value);
}
}

for (label, address) in &self.precompiles {
Pallet::<T>::add_precompile(label, address);
}
}
}

Expand All @@ -508,6 +513,17 @@ pub mod pallet {
#[pallet::getter(fn account_storages)]
pub type AccountStorages<T: Config> =
StorageDoubleMap<_, Blake2_128Concat, H160, Blake2_128Concat, H256, H256, ValueQuery>;

/// Allows for precompiles to have arbitrary addresses, potentially more than one.
Copy link
Collaborator

Choose a reason for hiding this comment

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

In which cases do you use the multiple addresses for a particular precompile? If the address of the a precompile changes, is it enough to just update the address is enough?

Copy link
Member

Choose a reason for hiding this comment

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

I think mostly something like ERC20/asset. You can have lots of contracts of the same code.

If the address of the a precompile changes, is it enough to just update the address is enough?

No, because in this case the address will have their associated storages, which differentiate them.

And this is supposed not to happen -- a precompile's label is to remain static.

Copy link
Contributor Author

@bernardoaraujor bernardoaraujor Mar 9, 2023

Choose a reason for hiding this comment

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

In which cases do you use the multiple addresses for a particular precompile?

This change is in the context of a new feature that will potentially be upstreamed later. We want to have specific precompiles that use traits like frame_support::traits::tokens::fungibles::*. For example, an ERC20-compatible precompile that uses pallet-assets to manage the tokens. In this case, different precompile addresses (that are associated with the Fungibles label) will map to different AssetIds.

But for most use-cases where a single precompile address is sufficient, this won't be really useful. In those cases, just associate a single address (potentially hardcoded) to the precompile label and that's it.

If the address of the a precompile changes, is it enough to just update the address is enough?

I'm not sure I fully understand the question. With this implementation, you can't really "change" the address of a precompile. You can add a new precompile address with the same label, which would now make two instances of the same precompile. You can later remove the first address. Whatever dApp that expects the old precompile address would stop working at this point.

Copy link
Contributor Author

@bernardoaraujor bernardoaraujor Mar 9, 2023

Choose a reason for hiding this comment

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

Another point worth mentioning is that using this feature (on-chain precompile addresses) is totally optional.

You could still implement your own PrecompileSet::execute() so that it only executes precompiles with hardcoded addresses, the same way FrontierPrecompiles<R> does in its current form:
https://github.com/paritytech/frontier/blob/58c568964dc32dd96153b7d113ac2df58d27de2b/template/runtime/src/precompiles.rs#L30-L47

A hybrid solution mixing both approaches (hardcoded + on-chain precompile addresses) is also possible.

The only breaking changes that this PR imposes is in case some runtime eventually decides to opt-in to on-chain precompile addresses via pallet-evm-precompile, a storage migration is necessary to populate storage with the previously hardcoded addresses.

/// `k1`: precompile label, e.g.: `b"Sha3FIPS256".to_vec()`
/// `k2`: precompile address
///
/// Please note that adding a new precompile label here is not enough to guarantee its execution
/// It is also required to list the new label on the implementation of `PrecompileSet::execute()`
#[pallet::storage]
#[pallet::getter(fn precompiles)]
pub type Precompiles<T: Config> =
StorageDoubleMap<_, Blake2_128Concat, Vec<u8>, Blake2_128Concat, H160, (), OptionQuery>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this pr is finally acceptable, consider changing the () to bool?
This way, you can choose to enable/disable one of the precompile's addresses.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm why do we need double storage map here? I think just a storage map mapping H160 to label would be enough.

Copy link
Contributor Author

@bernardoaraujor bernardoaraujor Mar 9, 2023

Choose a reason for hiding this comment

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

@sorpaas I switched to StorageMap.

But then I was thinking: wouldn't it be useful to use StorageDoubleMap and have an index for each H160 address?

  • k1: H160
  • k2: PrecompileLabel
  • v: PrecompileIndex

where PrecompileIndex is a Config type, with same traits as AssetId

that would avoid the need for using #[precompiles::precompile_set] & #[precompile::discriminant] to distinguish between precompiles within the same label (which btw is Moonbeam's approach for their assets-erc20 precompiles cc @nanocryk )

Copy link
Member

@sorpaas sorpaas Mar 10, 2023

Choose a reason for hiding this comment

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

That can be useful, but I don't see why we need double storage map. We can have H160 -> (PrecompileLabel, PrecompileIndex) map, and that should be sufficient. Instead of PrecompileIndex, we should allow arbitrary data to be passed to the executor with the type controllable by the config.

I honestly start to think that all this probably belongs to a new pallet. Apart from all the scaffolding, it should be a trivial change. We have a new pallet pallet-evm-precompile and move the following to it: a) this precompile storage map, b) the add/remove precompile dispatchable. Then we implement PrecompileSet on Pallet struct. Any users who have imported the new pallet can then set the PrecompileSet config in pallet-evm to this new pallet.

The advantage of this is that anyone who wants not to use precompile label can stop worrying about it, and don't need to care about an unused precompile storage map in the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cool, I'll refactor the changes into a separate pallet-evm-precompile

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do we leave template using the old FrontierPrecompiles as PrecompileSet, or the new one (pallet-evm-precompile)?

Copy link
Contributor Author

@bernardoaraujor bernardoaraujor Mar 10, 2023

Choose a reason for hiding this comment

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

I did some experiments and it turns out having an on-chain PrecompileIndex is not really useful.

impl<R> PrecompileSet for OnChainPrecompiles<R>
where
	R: pallet::Config + pallet_evm::Config,
{
	fn execute(&self, handle: &mut impl PrecompileHandle) -> Option<PrecompileResult> {
		match handle.code_address() {
			// Ethereum precompiles :
			a if &Precompiles::<R>::get(a).0[..] == b"PrecompileX" => {
				let precompile_index = Precompiles::<R>::get(a).1; // this is useless
				Some(PrecompileX::execute(handle))
			}
...

even though I can read precompile_index, I have no means to pass it to PrecompileX::execute(handle).

It seems the only available approach is using Moonbeam's #[precompiles::precompile_set] & #[precompile::discriminant].

Copy link
Member

Choose a reason for hiding this comment

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

We can modify that trait to take on precompile_index though.

But if you want, let's do that in a separate PR. This precompile_index is best added as a separate storage item (because large storage, which can happen with arbitrary data, takes longer to retrieve).

Copy link
Member

Choose a reason for hiding this comment

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

do we leave template using the old FrontierPrecompiles as PrecompileSet, or the new one (pallet-evm-precompile)?

Please make the template use the new one. One of the use for the template is to test our all pallets within this project, so it should include as many pallets as possible.

}

/// Type alias for currency balance.
Expand Down Expand Up @@ -671,6 +687,16 @@ impl<T: Config> GasWeightMapping for FixedGasWeightMapping<T> {
static LONDON_CONFIG: EvmConfig = EvmConfig::london();

impl<T: Config> Pallet<T> {
/// Add a precompile to storage
pub fn add_precompile(label: &Vec<u8>, address: &H160) {
Precompiles::<T>::set(label, address, Some(()));
}

/// Remove a precompile from storage
pub fn remove_precompile(label: &Vec<u8>, address: &H160) {
Precompiles::<T>::remove(label, address);
}

/// Check whether an account is empty.
pub fn is_account_empty(address: &H160) -> bool {
let (account, _) = Self::account_basic(address);
Expand Down
11 changes: 10 additions & 1 deletion frame/evm/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@ pub fn new_test_ext() -> sp_io::TestExternalities {
},
);

let precompiles = vec![];

pallet_balances::GenesisConfig::<Test> {
// Create the block author account with some balance.
balances: vec![(
Expand All @@ -76,7 +78,14 @@ pub fn new_test_ext() -> sp_io::TestExternalities {
}
.assimilate_storage(&mut t)
.expect("Pallet balances storage can be assimilated");
GenesisBuild::<Test>::assimilate_storage(&crate::GenesisConfig { accounts }, &mut t).unwrap();
GenesisBuild::<Test>::assimilate_storage(
&crate::GenesisConfig {
accounts,
precompiles,
},
&mut t,
)
.unwrap();
t.into()
}

Expand Down
11 changes: 11 additions & 0 deletions template/node/src/chain_spec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -250,6 +250,17 @@ fn testnet_genesis(
);
map
},
precompiles: vec![
// Ethereum precompiles :
(b"ECRecover".to_vec(), H160::from_low_u64_be(1)),
(b"Sha256".to_vec(), H160::from_low_u64_be(2)),
(b"Ripemd160".to_vec(), H160::from_low_u64_be(3)),
(b"Identity".to_vec(), H160::from_low_u64_be(4)),
(b"Modexp".to_vec(), H160::from_low_u64_be(5)),
// Non-Frontier specific nor Ethereum precompiles :
(b"Sha3FIPS256".to_vec(), H160::from_low_u64_be(1024)),
(b"Sha3FIPS256".to_vec(), H160::from_low_u64_be(1025)),
],
},
ethereum: Default::default(),
dynamic_fee: Default::default(),
Expand Down
55 changes: 32 additions & 23 deletions template/runtime/src/precompiles.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ use pallet_evm::{Precompile, PrecompileHandle, PrecompileResult, PrecompileSet};
use sp_core::H160;
use sp_std::marker::PhantomData;

use pallet_evm::Precompiles;
use pallet_evm_precompile_modexp::Modexp;
use pallet_evm_precompile_sha3fips::Sha3FIPS256;
use pallet_evm_precompile_simple::{ECRecover, ECRecoverPublicKey, Identity, Ripemd160, Sha256};
Expand All @@ -15,17 +16,6 @@ where
pub fn new() -> Self {
Self(Default::default())
}
pub fn used_addresses() -> [H160; 7] {
[
hash(1),
hash(2),
hash(3),
hash(4),
hash(5),
hash(1024),
hash(1025),
]
}
}
impl<R> PrecompileSet for FrontierPrecompiles<R>
where
Expand All @@ -34,23 +24,42 @@ where
fn execute(&self, handle: &mut impl PrecompileHandle) -> Option<PrecompileResult> {
match handle.code_address() {
// Ethereum precompiles :
a if a == hash(1) => Some(ECRecover::execute(handle)),
a if a == hash(2) => Some(Sha256::execute(handle)),
a if a == hash(3) => Some(Ripemd160::execute(handle)),
a if a == hash(4) => Some(Identity::execute(handle)),
a if a == hash(5) => Some(Modexp::execute(handle)),
a if Precompiles::<R>::contains_key(b"ECRecover".to_vec(), a) => {
Some(ECRecover::execute(handle))
}
a if Precompiles::<R>::contains_key(b"Sha256".to_vec(), a) => {
Some(Sha256::execute(handle))
}
a if Precompiles::<R>::contains_key(b"Ripemd160".to_vec(), a) => {
Some(Ripemd160::execute(handle))
}
a if Precompiles::<R>::contains_key(b"Identity".to_vec(), a) => {
Some(Identity::execute(handle))
}
a if Precompiles::<R>::contains_key(b"Modexp".to_vec(), a) => {
Some(Modexp::execute(handle))
}
// Non-Frontier specific nor Ethereum precompiles :
a if a == hash(1024) => Some(Sha3FIPS256::execute(handle)),
a if a == hash(1025) => Some(ECRecoverPublicKey::execute(handle)),
a if Precompiles::<R>::contains_key(b"Sha3FIPS256".to_vec(), a) => {
Some(Sha3FIPS256::execute(handle))
}
a if Precompiles::<R>::contains_key(b"ECRecoverPublicKey".to_vec(), a) => {
Some(ECRecoverPublicKey::execute(handle))
}
_ => None,
}
}

fn is_precompile(&self, address: H160) -> bool {
Self::used_addresses().contains(&address)
match address {
a if Precompiles::<R>::contains_key(b"ECRecover".to_vec(), a) => true,
a if Precompiles::<R>::contains_key(b"Sha256".to_vec(), a) => true,
a if Precompiles::<R>::contains_key(b"Ripemd160".to_vec(), a) => true,
a if Precompiles::<R>::contains_key(b"Identity".to_vec(), a) => true,
a if Precompiles::<R>::contains_key(b"Modexp".to_vec(), a) => true,
a if Precompiles::<R>::contains_key(b"Sha3FIPS256".to_vec(), a) => true,
a if Precompiles::<R>::contains_key(b"ECRecoverPublicKey".to_vec(), a) => true,
_ => false,
}
}
}

fn hash(a: u64) -> H160 {
H160::from_low_u64_be(a)
}