`pallet-xvm` refactor #980

shaunxw · 2023-07-20T10:31:41Z

Pull Request Summary

Refactor pallet-xvm.

API (re)design goal:

Less abstraction, easier to read and maintain
Simplify API for runtime integrations
Make calls overhead easy to benchmark
No need to loop all VMs to match the VmId to execute

In addition to API design changes, there are some notable improvements:

Benchmark & replace call overheads placeholder.
XVM call context can't be set by callers anymore, but only by the runtime, for obvious security reasons.
Calling the same VM via XVM is not allowed.
Take call overheads into consideration for weight limits.
Record cost in XVM precompile.
XvmExecutionResult::UnknownError is expanded to detailed error codes.
Trace EVM call results, instead of only tracing on success.

Design overview:

pub enum VmId {
    Evm = 0x0F,
    Wasm = 0x1F,
}

pub struct Context {
    pub source_vm_id: VmId,
    pub weight_limit: Weight,
}

pub trait XvmCall<AccountId> {
    fn call(
        context: Context,
        vm_id: VmId,
        source: AccountId,
        target: Vec<u8>,
        input: Vec<u8>,
    ) -> CallResult;
}

impl<T: Config> XvmCall<T::AccountId> for Pallet<T> {
    fn call(...) -> XvmCallResult {
        // Pallet implementation, be able to skip call execution to benchmark overhead
        Pallet::<T>::do_call(...)
    }
}

impl<T: Config> Pallet<T> {
    fn do_call(...) -> XvmCallResult {
        // ... ensure source_vm != vm_id

        // ... calls EVM or WASM depending on `vm_id`
        match vm_id {
            Vm::Evm => Pallet::<T>::evm_call(...),
            Vm::Wasm => Pallet::<T>::wasm_call(...),
        }
    }
    
    fn evm_call(...) -> ... { /* same to previous EVM impl, be able to skip execution */ }
    fn wasm_call(...) -> ... { /* same to previous WASM impl, be able to skip execution */ }
    
    // benchmark overhead
    #[cfg(feature = "runtime-benchmarks")]
    pub fn call_without_execution(...) -> ... {
        // skip execution
        Self::do_call(...)
    }
}

Check list

added or updated unit tests
updated Astar official documentation
added OnRuntimeUpgrade hook for precompile revert code registration
updated spec version
updated semver

Dinonard

I see both pros & cons here.

The pros:

should make development, testing & integration much easier
removes lot of the unnecessary code, which was just dragging on for over a year
clarity

The cons:

makes everything tightly coupled

We should be able to achieve benchmarking with the old approach too, I guess.

All that being said, IMHO we should focus on the functionality & usability instead of making various abstractions to cover whatever possible future scenarios might require but without even properly implementing support for the 2 VMs we have.

If required, we can always refactor in the future, when we have more knowledge & actually understand the problems of potential future VMs.

shaunxw · 2023-07-21T07:13:37Z

Agreed. We need to make a compromise between pragmatism and "design patterns". The previous design could be abstracted further to replace pallet-contracts / pallet-ethereum-checked deps with traits, but it doesn't really help with practicality. We're not building a common lib that is meant to be as flexible as possible, but specifically for Shibuya/Shiden/Astar.

The complexity multi-VMs brings is already quite a mental burden for devs. Personally, I prefer to make the XVM impl dead simple so that everyone can review it easily, without hopping through extra traits, macros, crate features, and runtime configs to understand what is happening.

As you mentioned, if more decoupling is required in the future, I'm always ready to refactor it again to fit the needs.

ashutoshvarma

It's much cleaner and readable than before for sure.

As I state in my previous reviews for pallet_ethereum_checked, I'm still not comfortable with extra if-else for serving runtime benchmarks only.
Is there a way to completely keep runtime-benchmarks logic behind a feature gate or something more cleaner? but if that's the only way to do this then it's fine

pallets/xvm/src/lib.rs

shaunxw · 2023-07-21T07:29:56Z

Is there a way to completely keep runtime-benchmarks logic behind a feature gate or something more cleaner? but if that's the only way to do this then it's fine

I did try feature gates, but unfortunately, they either don't work or would make the code difficult to read. One of the main issues is that if the behavior is different under different features, unit tests would fail with some features on. They can be solved with more feature gates added to unit tests, but believe me, you don't want to read that kind of code.

I'm happy to update if any doable nice clean solution is suggested.

shaunxw · 2023-07-26T09:56:26Z

/bench shibuya-dev pallet_xvm

github-actions · 2023-07-26T09:57:19Z

Benchmarks job is scheduled at https://github.com/AstarNetwork/Astar/actions/runs/5667322955.
Please wait for a while.
Branch: feat/pallet-xvm-refactor
SHA: 6b22ce6

github-actions · 2023-07-26T10:46:42Z

Benchmark job failed.
Please check https://github.com/AstarNetwork/Astar/actions/runs/5667322955.

shaunxw · 2023-07-26T12:53:58Z

/bench shibuya-dev pallet_xvm

github-actions · 2023-07-26T14:46:20Z

Benchmarks job is scheduled at https://github.com/AstarNetwork/Astar/actions/runs/5669003650.
Please wait for a while.
Branch: feat/pallet-xvm-refactor
SHA: 61220e6

github-actions · 2023-07-26T15:35:16Z

Benchmarks have been finished.
You can download artifacts if exists https://github.com/AstarNetwork/Astar/actions/runs/5669003650.

precompiles/xvm/src/lib.rs

pallets/xvm/src/lib.rs

shaunxw · 2023-07-27T10:23:20Z

/bench shibuya-dev pallet_xvm

github-actions · 2023-07-27T10:37:31Z

Benchmarks job is scheduled at https://github.com/AstarNetwork/Astar/actions/runs/5679384396.
Please wait for a while.
Branch: feat/pallet-xvm-refactor
SHA: 7482e7b

github-actions · 2023-07-27T11:27:01Z

Benchmarks have been finished.
You can download artifacts if exists https://github.com/AstarNetwork/Astar/actions/runs/5679384396.

runtime/shibuya/Cargo.toml

…ver.

shaunxw · 2023-07-27T11:49:03Z

/bench shibuya-dev pallet_xvm

github-actions · 2023-07-27T12:01:23Z

Benchmarks job is scheduled at https://github.com/AstarNetwork/Astar/actions/runs/5680132481.
Please wait for a while.
Branch: feat/pallet-xvm-refactor
SHA: ff3b41f

github-actions · 2023-07-27T12:50:48Z

Benchmarks have been finished.
You can download artifacts if exists https://github.com/AstarNetwork/Astar/actions/runs/5680132481.

github-actions · 2023-07-27T13:23:25Z

Package	Line Rate	Branch Rate	Health
pallets/dapps-staking/src	81%	0%	✔
precompiles/substrate-ecdsa/src	78%	0%	➖
precompiles/utils/src	70%	0%	➖
pallets/xc-asset-config/src	54%	0%	➖
precompiles/assets-erc20/src	76%	0%	➖
precompiles/xcm/src	84%	0%	✔
precompiles/xvm/src	67%	0%	➖
primitives/src	69%	0%	➖
primitives/src/xcm	67%	0%	➖
chain-extensions/types/dapps-staking/src	0%	0%	❌
pallets/pallet-xcm/src	53%	0%	➖
pallets/custom-signatures/src	51%	0%	➖
chain-extensions/types/assets/src	0%	0%	❌
chain-extensions/xvm/src	0%	0%	❌
precompiles/sr25519/src	79%	0%	➖
chain-extensions/pallet-assets/src	0%	0%	❌
pallets/xvm/src	0%	0%	❌
precompiles/utils/macro/src	0%	0%	❌
pallets/ethereum-checked/src	48%	0%	❌
chain-extensions/dapps-staking/src	0%	0%	❌
pallets/collator-selection/src	69%	0%	➖
pallets/block-reward/src	85%	0%	✔
pallets/contracts-migration/src	0%	0%	❌
chain-extensions/types/xvm/src	0%	0%	❌
precompiles/dapps-staking/src	93%	0%	✔
pallets/dapps-staking/src/pallet	85%	0%	✔
Summary	55% (2271 / 4108)	0% (0 / 0)	➖

Minimum allowed line rate is 50%

shaunxw added 2 commits July 20, 2023 22:10

pallet-xvm refactor.

49de894

Add TODOs.

5ea897c

shaunxw requested review from Dinonard, ashutoshvarma and gitofdeepanshu July 20, 2023 10:31

shaunxw added 3 commits July 20, 2023 22:59

Update design.

57ad0c9

Keep XVM call interface unified.

46263d2

Renaming.

5c79244

shaunxw added the other PR/issue is related to third-party, scripts or other things which don't have definite label. label Jul 20, 2023

Dinonard reviewed Jul 21, 2023

View reviewed changes

ashutoshvarma reviewed Jul 21, 2023

View reviewed changes

pallets/xvm/src/lib.rs Outdated Show resolved Hide resolved

pallets/xvm/src/lib.rs Outdated Show resolved Hide resolved

pallets/xvm/src/lib.rs Outdated Show resolved Hide resolved

shaunxw added 8 commits July 25, 2023 22:51

update & integration.

2a81a2d

Update xvm precompiles mock & tests.

e576f74

Replace 'UnknownError' with concrete errors.

401cf8c

Update CE & precompile.

e410631

Clean up.

819c7f8

Benchmarks and mock.

50c04b2

Merge branch 'master' into feat/pallet-xvm-refactor

eed8ce4

Updates for polkadot-v0.9.43.

6b22ce6

Fix benchmarks.

61220e6

Add benchmarking result and weight info.

7b92705

shaunxw marked this pull request as ready for review July 26, 2023 23:00

shaunxw added 7 commits July 27, 2023 11:11

Add license header to weight.rs.

8831ba9

Add pallet description docstring.

5666403

Record gas cost in XVM precompile.

df8e062

Less weight is available with overheads cost.

34a3942

Trace Ethereum transact result.

e9ccd4b

Handle record cost result.

cd5d2af

Bump Shibuya semver and spec versoin.

8d4f62e

Dinonard reviewed Jul 27, 2023

View reviewed changes

precompiles/xvm/src/lib.rs Show resolved Hide resolved

pallets/xvm/src/lib.rs Show resolved Hide resolved

pallets/xvm/src/lib.rs Outdated Show resolved Hide resolved

pallets/xvm/src/lib.rs Show resolved Hide resolved

This was referenced Jul 27, 2023

Forbid XVM re-entrancy #986

Closed

Respect caller's custom weight limit setting in XVM #987

Closed

Apply review suggestions.

7482e7b

Update with new benchmarking result.

3fa315e

Dinonard reviewed Jul 27, 2023

View reviewed changes

runtime/shibuya/Cargo.toml Show resolved Hide resolved

shaunxw added 2 commits July 27, 2023 23:37

Improve XVM call benchmarking.

931117c

Make local/shibuya/shiden/astar runtimes and client have the same sem…

ff3b41f

…ver.

Update with new benchmarking result.

f6a81fa

Dinonard approved these changes Jul 27, 2023

View reviewed changes

ashutoshvarma approved these changes Jul 28, 2023

View reviewed changes

shaunxw merged commit 18ff2b9 into master Jul 28, 2023

shaunxw deleted the feat/pallet-xvm-refactor branch July 28, 2023 03:23

ashutoshvarma mentioned this pull request Aug 14, 2023

chore: release v5.17.0 prep #1002

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`pallet-xvm` refactor #980

`pallet-xvm` refactor #980

shaunxw commented Jul 20, 2023 •

edited

Loading

Dinonard left a comment

shaunxw commented Jul 21, 2023

ashutoshvarma left a comment

shaunxw commented Jul 21, 2023

shaunxw commented Jul 26, 2023

github-actions bot commented Jul 26, 2023

github-actions bot commented Jul 26, 2023

shaunxw commented Jul 26, 2023

github-actions bot commented Jul 26, 2023

github-actions bot commented Jul 26, 2023

shaunxw commented Jul 27, 2023

github-actions bot commented Jul 27, 2023

github-actions bot commented Jul 27, 2023

shaunxw commented Jul 27, 2023

github-actions bot commented Jul 27, 2023

github-actions bot commented Jul 27, 2023

github-actions bot commented Jul 27, 2023

pallet-xvm refactor #980

pallet-xvm refactor #980

Conversation

shaunxw commented Jul 20, 2023 • edited Loading

Dinonard left a comment

Choose a reason for hiding this comment

shaunxw commented Jul 21, 2023

ashutoshvarma left a comment

Choose a reason for hiding this comment

shaunxw commented Jul 21, 2023

shaunxw commented Jul 26, 2023

github-actions bot commented Jul 26, 2023

github-actions bot commented Jul 26, 2023

shaunxw commented Jul 26, 2023

github-actions bot commented Jul 26, 2023

github-actions bot commented Jul 26, 2023

shaunxw commented Jul 27, 2023

github-actions bot commented Jul 27, 2023

github-actions bot commented Jul 27, 2023

shaunxw commented Jul 27, 2023

github-actions bot commented Jul 27, 2023

github-actions bot commented Jul 27, 2023

github-actions bot commented Jul 27, 2023

`pallet-xvm` refactor #980

`pallet-xvm` refactor #980

shaunxw commented Jul 20, 2023 •

edited

Loading