Skip to content

Prevent precompiles removal at pallet-evm-system#1519

Merged
dmitrylavrenov merged 15 commits intomasterfrom
properly-manage-precompiles-at-evm-system
Apr 11, 2025
Merged

Prevent precompiles removal at pallet-evm-system#1519
dmitrylavrenov merged 15 commits intomasterfrom
properly-manage-precompiles-at-evm-system

Conversation

@dmitrylavrenov
Copy link
Contributor

@dmitrylavrenov dmitrylavrenov commented Apr 7, 2025

Issue

By default, precompiled accounts are created at the pallet-evm-system <- pallet-evm <- pallet-dummy-precompiles-code:

The issue is that a precompiled account is removed at pallet-evm-system in case withdrawing full balance from this account if the balance is not empty.

Later if some tokens are transferred to the account -> the account is created again. Withdrawing full tokens -> the account is removed again. It's repeated again and again.

So, it means we should prevent precompiled accounts removal.

Proposed solution

It sounds reasonable to introduce and use additional IsPrecompile at pallet config that allows checking whether a given account is a precompile or not. As a result, prevent removal if the account is a precompiled one.

@MOZGIII
Copy link
Contributor

MOZGIII commented Apr 7, 2025

  1. I don't like using a specific type here, much better would be to use a trait that allows checking whather a given address is a precompile or not.
  2. I don't particularly like a BTree for this; it looks like we could implement a custom type that would not have any allocations and would instead just have a match under the hood.
  3. I don't think we need to solve the precompiles set issue properly at this time, as there might be a a solution for this in the later frontier updates, or in the moonbeam branch of frontier - I'd look into that first to see if we can use some preexisting code - mainly to check if they did it properly already (maybe they have something but it is not good)

@dmitrylavrenov
Copy link
Contributor Author

dmitrylavrenov commented Apr 8, 2025

  1. I don't like using a specific type here, much better would be to use a trait that allows checking whather a given address is a precompile or not.
  2. I don't particularly like a BTree for this; it looks like we could implement a custom type that would not have any allocations and would instead just have a match under the hood.
  3. I don't think we need to solve the precompiles set issue properly at this time, as there might be a a solution for this in the later frontier updates, or in the moonbeam branch of frontier - I'd look into that first to see if we can use some preexisting code - mainly to check if they did it properly already (maybe they have something but it is not good)

Thanks

  1. Got it, will introduce the trait.
  2. Got it.
  3. Not sure that we are on the same page here. Let me describe the issue that I wanted to highlight. At the current moment, there is some logic that depends on pallet-evm-balances and pallet-evm-system explicitly, not any other EVM logic from pallet-evm. As an example, when we do swap, the balances are updated for precompiles and we got such confusing events like KilledAccount and NewAccount for precompiles for each swap from EVM to native tokens. As we already investigated, moonbeam and frontier use sufficients to track the precimpiles nature. Here I propose to introduce additional type into config that allows checking whether a given address is a precompile or not based on our logic without sufficients.

@dmitrylavrenov dmitrylavrenov force-pushed the properly-manage-precompiles-at-evm-system branch 2 times, most recently from e11487a to 93335c7 Compare April 8, 2025 15:00
@MOZGIII
Copy link
Contributor

MOZGIII commented Apr 8, 2025

What I meant regarding (3) is that the type itself that we use to represent the PrecompilesSet (instead of BTreeMap) can be quite rudimentary. I can imagine a "proper" solution powered by macros and etc - but I'd avoid doing something too complex for this ourselves. Let others do it.

No that we don't want to solve the events issue.

@dmitrylavrenov
Copy link
Contributor Author

What I meant regarding (3) is that the type itself that we use to represent the PrecompilesSet (instead of BTreeMap) can be quite rudimentary. I can imagine a "proper" solution powered by macros and etc - but I'd avoid doing something too complex for this ourselves. Let others do it.

No that we don't want to solve the events issue.

I see, thanks, now it's clear!

@dmitrylavrenov dmitrylavrenov force-pushed the properly-manage-precompiles-at-evm-system branch 2 times, most recently from d60b793 to 8bf5bdf Compare April 10, 2025 07:00
@dmitrylavrenov dmitrylavrenov force-pushed the properly-manage-precompiles-at-evm-system branch 3 times, most recently from ce88b73 to 9864d20 Compare April 10, 2025 08:46
@dmitrylavrenov dmitrylavrenov force-pushed the properly-manage-precompiles-at-evm-system branch from 9864d20 to c8bb381 Compare April 10, 2025 08:48
@dmitrylavrenov dmitrylavrenov changed the title Properly manage precompiles at pallet-evm-system Prevent precompiles removal at pallet-evm-system Apr 10, 2025
@dmitrylavrenov dmitrylavrenov marked this pull request as ready for review April 10, 2025 13:04
@dmitrylavrenov dmitrylavrenov requested a review from MOZGIII April 10, 2025 14:53
@dmitrylavrenov dmitrylavrenov added this pull request to the merge queue Apr 11, 2025
Merged via the queue into master with commit 5ea3318 Apr 11, 2025
21 checks passed
@dmitrylavrenov dmitrylavrenov deleted the properly-manage-precompiles-at-evm-system branch April 11, 2025 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants