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
2 changes: 2 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions frame/ethereum/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,7 @@ impl pallet_evm::Config for Test {
type OnChargeTransaction = ();
type OnCreate = ();
type FindAuthor = FindAuthorTruncated;
type PrecompileModifierOrigin = frame_system::EnsureRoot<Self::AccountId>;
}

parameter_types! {
Expand Down
2 changes: 2 additions & 0 deletions frame/evm/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ log = { version = "0.4.17", default-features = false }
rlp = { workspace = true }
scale-codec = { package = "parity-scale-codec", workspace = true }
scale-info = { workspace = true }
serde = { workspace = true, default-features = false }
# Substrate
frame-benchmarking = { workspace = true, optional = true }
frame-support = { workspace = true }
Expand All @@ -40,6 +41,7 @@ pallet-evm-precompile-simple = { workspace = true }
[features]
default = ["std"]
std = [
"serde/std",
"environmental?/std",
"evm/std",
"evm/with-serde",
Expand Down
1 change: 1 addition & 0 deletions frame/evm/precompile/dispatch/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,7 @@ impl pallet_evm::Config for Test {
type OnChargeTransaction = ();
type OnCreate = ();
type FindAuthor = FindAuthorTruncated;
type PrecompileModifierOrigin = frame_system::EnsureRoot<Self::AccountId>;
}

pub(crate) struct MockHandle {
Expand Down
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
70 changes: 70 additions & 0 deletions frame/evm/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,8 @@ use frame_support::{
};
use frame_system::RawOrigin;
use impl_trait_for_tuples::impl_for_tuples;
use scale_codec::{Decode, Encode};
use scale_info::TypeInfo;
use sp_core::{Hasher, H160, H256, U256};
use sp_runtime::{
traits::{BadOrigin, Saturating, UniqueSaturatedInto, Zero},
Expand Down Expand Up @@ -155,6 +157,9 @@ pub mod pallet {
/// Find author for the current block.
type FindAuthor: FindAuthor<H160>;

/// Origin allowed to modify Precompiles
type PrecompileModifierOrigin: EnsureOrigin<Self::RuntimeOrigin>;

/// EVM config used in the module.
fn config() -> &'static EvmConfig {
&LONDON_CONFIG
Expand Down Expand Up @@ -404,6 +409,37 @@ pub mod pallet {
pays_fee: Pays::No,
})
}

/// Add a precompile to storage
#[pallet::call_index(4)]
#[pallet::weight(T::DbWeight::get().reads_writes(0, 1))]
pub fn add_precompile(
origin: OriginFor<T>,
address: H160,
label: PrecompileLabel,
) -> DispatchResult {
T::PrecompileModifierOrigin::ensure_origin(origin)?;

Self::do_add_precompile(&address, label);

Ok(())
}

/// Remove a precompile from storage
#[pallet::call_index(5)]
#[pallet::weight(T::DbWeight::get().reads_writes(0, 1))]
pub fn remove_precompile(origin: OriginFor<T>, address: H160) -> DispatchResult {
T::PrecompileModifierOrigin::ensure_origin(origin)?;

ensure!(
Precompiles::<T>::contains_key(address),
Error::<T>::PrecompileDoesNotExist
);

Self::do_remove_precompile(&address);

Ok(())
}
}

#[pallet::event]
Expand Down Expand Up @@ -445,6 +481,8 @@ pub mod pallet {
Reentrancy,
/// EIP-3607,
TransactionMustComeFromEOA,
/// Precompile does not exist in storage
PrecompileDoesNotExist,
}

impl<T> From<InvalidEvmTransactionError> for Error<T> {
Expand All @@ -467,6 +505,7 @@ pub mod pallet {
#[cfg_attr(feature = "std", derive(Default))]
pub struct GenesisConfig {
pub accounts: std::collections::BTreeMap<H160, GenesisAccount>,
pub precompiles: Vec<(H160, PrecompileLabel)>,
}

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

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

Expand All @@ -508,6 +551,23 @@ 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.

/// `key`: precompile `H160` address
/// `value`: precompile label, e.g.: `PrecompileLabel { label: b"Sha3FIPS256".to_vec() }`

/// Please note that adding a new precompile label here is not enough to guarantee its execution
/// It is also required to list new labels on the implementation of `PrecompileSet::execute()`
#[pallet::storage]
#[pallet::getter(fn precompiles)]
pub type Precompiles<T: Config> =
StorageMap<_, Blake2_128Concat, H160, PrecompileLabel, ValueQuery>;
}

#[derive(Decode, Encode, Default, TypeInfo, Clone, PartialEq, Debug)]
#[cfg_attr(feature = "std", derive(serde::Serialize, serde::Deserialize))]
pub struct PrecompileLabel {
bernardoaraujor marked this conversation as resolved.
Show resolved Hide resolved
pub label: Vec<u8>,
}

/// Type alias for currency balance.
Expand Down Expand Up @@ -671,6 +731,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 do_add_precompile(address: &H160, label: PrecompileLabel) {
Precompiles::<T>::set(address, label);
}

/// Remove a precompile from storage
pub fn do_remove_precompile(address: &H160) {
Precompiles::<T>::remove(address);
}

/// Check whether an account is empty.
pub fn is_account_empty(address: &H160) -> bool {
let (account, _) = Self::account_basic(address);
Expand Down
1 change: 1 addition & 0 deletions frame/evm/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,7 @@ impl crate::Config for Test {
type OnChargeTransaction = ();
type OnCreate = ();
type FindAuthor = FindAuthorTruncated;
type PrecompileModifierOrigin = frame_system::EnsureRoot<Self::AccountId>;
}

/// Exemple PrecompileSet with only Identity precompile.
Expand Down
36 changes: 35 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 Expand Up @@ -611,3 +620,28 @@ fn eip3607_transaction_from_precompile_should_fail() {
}
});
}

#[test]
fn precompile_storage_works() {
new_test_ext().execute_with(|| {
let origin = RuntimeOrigin::root();
let address = H160::from_low_u64_be(1);

let mut read_precompile = EVM::precompiles(address);
assert_eq!(read_precompile, PrecompileLabel::default());

let label = PrecompileLabel {
label: b"ECRecover".to_vec(),
};

assert_ok!(EVM::add_precompile(origin.clone(), address, label.clone()));

read_precompile = EVM::precompiles(address);
assert_eq!(read_precompile, label);

assert_ok!(EVM::remove_precompile(origin, address));

read_precompile = EVM::precompiles(address);
assert_eq!(read_precompile, PrecompileLabel::default());
});
}
1 change: 1 addition & 0 deletions template/node/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ fp-dynamic-fee = { workspace = true, features = ["default"] }
fp-evm = { workspace = true, features = ["default"] }
fp-rpc = { workspace = true, features = ["default"] }
frontier-template-runtime = { workspace = true, features = ["default"] }
pallet-evm = { workspace = true }

[build-dependencies]
substrate-build-script-utils = { workspace = true }
Expand Down
48 changes: 48 additions & 0 deletions template/node/src/chain_spec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ use frontier_template_runtime::{
AccountId, EnableManualSeal, GenesisConfig, Signature, WASM_BINARY,
};

use pallet_evm::PrecompileLabel;

// The URL for the telemetry server.
// const STAGING_TELEMETRY_URL: &str = "wss://telemetry.polkadot.io/submit/";

Expand Down Expand Up @@ -250,6 +252,52 @@ fn testnet_genesis(
);
map
},
precompiles: vec![
// Ethereum precompiles :
(
H160::from_low_u64_be(1),
PrecompileLabel {
label: b"ECRecover".to_vec(),
},
),
(
H160::from_low_u64_be(2),
PrecompileLabel {
label: b"Sha256".to_vec(),
},
),
(
H160::from_low_u64_be(3),
PrecompileLabel {
label: b"Ripemd160".to_vec(),
},
),
(
H160::from_low_u64_be(4),
PrecompileLabel {
label: b"Identity".to_vec(),
},
),
(
H160::from_low_u64_be(5),
PrecompileLabel {
label: b"Modexp".to_vec(),
},
),
// Non-Frontier specific nor Ethereum precompiles :
(
H160::from_low_u64_be(1024),
PrecompileLabel {
label: b"Sha3FIPS256".to_vec(),
},
),
(
H160::from_low_u64_be(1025),
PrecompileLabel {
label: b"Sha3FIPS256".to_vec(),
},
),
],
},
ethereum: Default::default(),
dynamic_fee: Default::default(),
Expand Down
1 change: 1 addition & 0 deletions template/runtime/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -343,6 +343,7 @@ impl pallet_evm::Config for Runtime {
type OnChargeTransaction = ();
type OnCreate = ();
type FindAuthor = FindAuthorTruncated<Aura>;
type PrecompileModifierOrigin = frame_system::EnsureRoot<Self::AccountId>;
}

parameter_types! {
Expand Down
Loading