[stable2412] pallet-revive: remove related crates from umbrella

[iulianbarbu]

Description

When building polkadot-sdk-parachain-template workspace (cargo build --workspace --all-features), based on stable2412 dependencies (paritytech/polkadot-sdk-parachain-template#26), it fails with an error like below.

error message

error[E0080]: evaluation of constant value failed --> /home/ubuntu/.cargo/registry/src/index.crates.io-6f17d22bba15001f/sc-network-0.47.0/src/protocol/message.rs:79:40 | 79 | #[derive(Debug, PartialEq, Eq, Clone, Encode, Decode)] | ^^^^^^ the evaluated program panicked at 'Found variants that have duplicate indexes. Both `Consensus` and `RemoteCallResponse` have the index `6`. Use different indexes for each variant.', /home/ubuntu/.cargo/registry/src/index.crates.io-6f17d22bba15001f/sc-network-0.47.0/src/protocol/message.rs:79:43 | = note: this error originates in the macro `$crate::panic::panic_2021` which comes from the expansion of the macro `::core::panic` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0080]: evaluation of constant value failed
--> /home/ubuntu/.cargo/registry/src/index.crates.io-6f17d22bba15001f/sc-network-0.47.0/src/protocol/message.rs:79:48
|
79 | #[derive(Debug, PartialEq, Eq, Clone, Encode, Decode)]
| ^^^^^^ the evaluated program panicked at 'Found variants that have duplicate indexes. Both Consensus and RemoteCallResponse have the index 6. Use different indexes for each variant.', /home/ubuntu/.cargo/registry/src/index.crates.io-6f17d22bba15001f/sc-network-0.47.0/src/protocol/message.rs:79:51
|
= note: this error originates in the macro $crate::panic::panic_2021 which comes from the expansion of the macro ::core::panic (in Nightly builds, run with -Z macro-backtrace for more info)

For more information about this error, try rustc --explain E0080.
error: could not compile sc-network (lib) due to 2 previous errors

The root cause is sc-network 0.47.0 (which builds fine when >=0.48.0), a dependency of pallet-revive-eth-rpc 0.2.0, which is a dependency of polkadot-sdk 0.12.1, used for various things in polkadot-sdk-parachain-template repo. While investigating how to fix the issue on stable2412, I discovered a few things:

1. pallet-revive 0.3.1 can not build with runtime-benchmarks based on latest stable2412, and it is also a dependency of pallet-revive-eth-rpc 0.2.0, and pulled by polkadot-sdk 0.12.1 for its runtime-benchmarks feature.

2. Based on some chats started here: [pallet-revive] fixture as dev dep #7844 (comment), we identified that pallet-revive/pallet-revive-eth-rpc were set with publish = false for a good reason and it shouldn't have changed here: 31b5923. We also identified as a potential fix what this PR does, meaning removing the pallet-revive/pallet-revive-eth-rpc dependency of polkadot-sdk. I removed the other dependencies as well since I feel it does not make sense to still keep them once pallet-revive is out.

3. Releasing correct crates for pallet-revive* and use them in polkadot-sdk isn't yet necessary IMO, since some things still need to be polished, and given their builds fail when building certain features, they are not usable by users fully, so maybe not worth keeping partial working functionality around just for the sake of having the crates in polkadot-sdk - please challenge this if you think differently. Ideally, we will make these pallets available for developers to use via polkadot-sdk when all builds correctly with all features.

Integration

Runtime/Node devs will not be able to reference pallet-revive* pallets anymore via polkadot-sdk, so this is a breaking change, but if their usage is close to zero by polkadot devs we can tune it down and consider it a minor change for polkadot-sdk.

Review Notes

1. If we think this requires a major bump for polkadot-sdk.. then we can't look at the PR as a fix for stable2412 and associated published polkadot-sdk version. If we consider pallet-revive logic not being necessarily used by polkadot devs so not very breaking from usage perspective, then we can go with a minor bump and finally have a working polkadot-sdk for `stable2412.

2. We also considered fixing pallet-revive & pallet-revive-eth-rpc on stable2412, and publish correct versions on crates.io, and then follow with a polkadot-sdk publish that depends on them, and in the end polkadot-sdk-parachain-template workspace would build successfully based on stable2412 crates, but this is more work which could've been done by now if it was necessary - e.g. in a scenario where actual developers were using them (instead of setting the crates with publish = false), so I think it makes sense to remove them for now from polkadot-sdk.

3. IMO this PR should be backported to stable2503 too. Releasing polkadot-sdk with pallet-revive* dependencies can be done with the June release if things are more stable by then. We actually want to fix pallet-revive* crates for stable2503, so must be kept in master and 2503 branch.

[bkchr]

@iulianbarbu #7446 this is the pr you are searching for. This is not related to pallet-revive. The problem is scale codec or better broken code in this crate.

[bkchr]

Linked the wrong pr 🙈 #7437 is the correct one.

[iulianbarbu]

@iulianbarbu #7446 this is the pr you are searching for. This is not related to pallet-revive. The problem is scale codec or better broken code in this crate.

Nice, thanks for the lead @bkchr ! Will take a look to clarify it on my end.

I thought that the underlying reason for why sc-network 0.47.0 fails is important, but I also noticed sc-network 0.48.0 builds successfully (part of stable2412 too), so I assumed things got fixed in between. It was more important to release a working polkadot-sdk version for stable2412, that could be used with the parachain-template repository. My experiments resulted in failures of building polkadot-sdk due to pallet-revive-eth-rpc crate, which needed to be published with a dependency on sc-network 0.48.0, and that attracted all sorts of things like here: 31b5923, and then pallet-revive issues too, which eventually led to this PR which I still think it makes sense for master and stable2503 if we'll walk into stable2503 with publish = false for some pallet-revive* crates, but let me know if you think differently.

[bkchr]

My experiments resulted in failures of building polkadot-sdk due to pallet-revive-eth-rpc crate, which needed to be published with a dependency on sc-network 0.48.0, and that attracted all sorts of things like here: 31b5923

This looks really horrible! :D If these crates are not publishable, yes we should not add them, but with 2503 this should change, as we want to put these crates onto the production networks.

[iulianbarbu]

My experiments resulted in failures of building polkadot-sdk due to pallet-revive-eth-rpc crate, which needed to be published with a dependency on sc-network 0.48.0, and that attracted all sorts of things like here: 31b5923

This looks really horrible! :D If these crates are not publishable, yes we should not add them, but with 2503 this should change, as we want to put these crates onto the production networks.

I see. If that's the case, maybe this PR should be applied only on top of stable2412, and then ensure that pallet-revive* crates are publishable with 2503. @EgorPopelyaev , tagging you here because I think #7844 is related to a publishing issue of pallet-revive-fixtures for the 2503 rc, which should unblock it, but have you noticed issues with the publishing of pallet-revive or the other pallet-revive* crates that need to be published for 2503?

[pgherveou]

This looks really horrible! :D If these crates are not publishable, yes we should not add them, but with 2503 this should change, as we want to put these crates onto the production networks.

Yes that make sense to me, ideally we want to keep pallet-revive in the umbrella crate in master as well as publishing eth-rpc to crates.io

[iulianbarbu]

I rebased the branch on top of stable2412.

[iulianbarbu]

Lots of tests are failing due to various reasons which seem unrelated to the changes. I think we don't expect the CI to be green for backports like this.

[athei]

There are actual hard build errors here. They exist because the kitchensink-runtime still depends on pallet-revive. pallet-revive-{*} should have been set back to publish = false. Otherwise I guess it will try to release them.

• Remove revive from the kitchensink runtime
• Set publish = false

[iulianbarbu]

Built polkadot-sdk-parachain-template against this branch and all goes well, compared to latest stable2412.

[iulianbarbu]

Multiple kitchensink tests fail to build because pallet-revive is not around anymore. I might be able to fix them, but I am not sure how valuable this is. Build failure: https://github.com/paritytech/polkadot-sdk/actions/runs/13838785891/job/38720764717?pr=7894. Other CI workflows fail but I am not sure if they are supposed to pass on the stable branch. All in all I wouldn't invest that much time in fixing what is red so far, but happy to reconsider if you think differently.

cc @athei @pgherveou, I would love another look on the PR.

[athei]

The build failures now are due to the fact that the UncheckedExtrinsic is no longer wrap. The fix is to remove the .0 everywhere where it is failing.

[iulianbarbu]

The build failures now are due to the fact that the UncheckedExtrinsic is no longer wrap. The fix is to remove the .0 everywhere where it is failing.

Yep, will do that. Not sure if the tests will pass though, hopefully yes.

[iulianbarbu]

I think the feature propagation done by zepter for most of the crates is mostly done for tests to compile successfully.
I will have to double check all updated crates to clarify if this applies to all of them.

TLDR, above is correct. We can tune down to minor the major bumps from the prdoc. I think it makes sense to tune down from major to minor for polkadot-sdk as well, since we did not have a fully working version for stable2412, and we can yank the versions before next patch release.

More context on what's failing before zepter fix:

1. snowbridge/pallets/inbound-queue & snowbridge/pallets/outbound-queue fail to build for runtime-benchmarks, but that's because of snowbridge/primitives/core missing runtime-benchmarks feature set, which gets fixed down the line by zepter.
2. polkadot-parachain has all its runtime-benchmarks feature set removed, the build is successful, but of course when running a benchmark command on any embedded runtime it complains. Zepter fixes it.

[iulianbarbu]

Now the short benchmarks workflows fail after the zepter fix: https://github.com/paritytech/polkadot-sdk/actions/runs/13970718623/job/39111667946?pr=7894. It looks like some runtime API is missing. Maybe a missing backport? cc @franciscoaguirre

2025-03-20T14:03:04.951823Z  INFO polkadot_sdk_frame::benchmark::pallet: [ 87 % ] Starting benchmark: pallet_xcm_benchmarks::fungible::initiate_teleport    
2025-03-20T14:03:04.952446Z ERROR polkadot_sdk_frame::benchmark::pallet: Error executing and verifying runtime benchmark: Invalid input: Could not call runtime API to dispatch a benchmark: Execution aborted due to trap: wasm trap: wasm `unreachable` instruction executed
WASM backtrace:
error while executing at wasm backtrace:
    0: 0x6ca923 - asset_hub_rococo_runtime.wasm!rust_begin_unwind
    1: 0x6695c1 - asset_hub_rococo_runtime.wasm!core::panicking::panic_fmt::hf6b8e339784efd02
    2: 0x6694ca - asset_hub_rococo_runtime.wasm!core::result::unwrap_failed::h960229ea1632482b
    3: 0x5ef3f4 - asset_hub_rococo_runtime.wasm!<cumulus_primitives_utility::ToParentDeliveryHelper<XcmConfig,ExistentialDeposit,PriceForDelivery> as staging_xcm_builder::routing::EnsureDelivery>::ensure_successful_delivery::he649bb33ded0ce5f
    4: 0x625cb8 - asset_hub_rococo_runtime.wasm!<pallet_xcm_benchmarks::fungible::benchmarking::SelectedBenchmark as frame_benchmarking::utils::BenchmarkingSetup<T,I>>::instance::h16d20334a8f3ac92
    5: 0x45bef7 - asset_hub_rococo_runtime.wasm!pallet_xcm_benchmarks::fungible::benchmarking::<impl frame_benchmarking::utils::Benchmarking for pallet_xcm_benchmarks::fungible::pallet::Pallet<T,I>>::run_benchmark::hc366916f1462368f
    6: 0x8a473 - asset_hub_rococo_runtime.wasm!<asset_hub_rococo_runtime::Runtime as frame_benchmarking::utils::runtime_decl_for_benchmark::BenchmarkV1<sp_runtime::generic::block::Block<sp_runtime::generic::header::Header<u32,sp_runtime::traits::BlakeTwo256>,sp_runtime::generic::unchecked_extrinsic::UncheckedExtrinsic<sp_runtime::multiaddress::MultiAddress<<<sp_runtime::MultiSignature as sp_runtime::traits::Verify>::Signer as sp_runtime::traits::IdentifyAccount>::AccountId,()>,asset_hub_rococo_runtime::RuntimeCall,sp_runtime::MultiSignature,(frame_system::extensions::check_non_zero_sender::CheckNonZeroSender<asset_hub_rococo_runtime::Runtime>,frame_system::extensions::check_spec_version::CheckSpecVersion<asset_hub_rococo_runtime::Runtime>,frame_system::extensions::check_tx_version::CheckTxVersion<asset_hub_rococo_runtime::Runtime>,frame_system::extensions::check_genesis::CheckGenesis<asset_hub_rococo_runtime::Runtime>,frame_system::extensions::check_mortality::CheckMortality<asset_hub_rococo_runtime::Runtime>,frame_system::extensions::check_nonce::CheckNonce<asset_hub_rococo_runtime::Runtime>,frame_system::extensions::check_weight::CheckWeight<asset_hub_rococo_runtime::Runtime>,pallet_asset_conversion_tx_payment::ChargeAssetTxPayment<asset_hub_rococo_runtime::Runtime>,cumulus_primitives_storage_weight_reclaim::StorageWeightReclaim<asset_hub_rococo_runtime::Runtime>,frame_metadata_hash_extension::CheckMetadataHash<asset_hub_rococo_runtime::Runtime>)>>>>::dispatch_benchmark::ha1520ead727c03f4
    7: 0x336578 - asset_hub_rococo_runtime.wasm!Benchmark_dispatch_benchmark    

[franciscoaguirre]

Looks like it. These things should've been backported but I'll check which one is missing

[franciscoaguirre]

Weird, I'm able to execute this locally:

frame-omni-bencher v1 benchmark pallet \
        --runtime asset_hub_westend_runtime.wasm \
        --pallet "pallet_xcm_benchmarks::fungible" \
        --extrinsic "initiate_teleport"

Built the runtime with cargo build --release -p asset-hub-westend-runtime --features runtime-benchmarks

[iulianbarbu]

Weird, I'm able to execute this locally:

frame-omni-bencher v1 benchmark pallet \
        --runtime asset_hub_westend_runtime.wasm \
        --pallet "pallet_xcm_benchmarks::fungible" \
        --extrinsic "initiate_teleport"

Built the runtime with cargo build --release -p asset-hub-westend-runtime --features runtime-benchmarks

Same, trying the CI without forklift. 🤔

[iulianbarbu]

I had to do this on this branch to make stuff compile: a965960#diff-ca8e59f5a23b8cade9cf116fb3d9f30c56d174691d11d971b70f7251ea4c21d4R21. At the same time, master seems to use something differently: https://github.com/paritytech/polkadot-sdk/blob/master/polkadot/xcm/pallet-xcm-benchmarks/src/generic/benchmarking.rs#L22.

Blaming on this line shows up a PR from Nov 28 2024: github.com/paritytech/polkadot-sdk/pull/6618. Should've this been backported too? Doesn't seem related to initiate_teleport directly, but seems related to pallet-xcm-benchmarks overall. Nore sure if there might be some non-obvious dependency coming from the PR. Any ideas @franciscoaguirre ?

[franciscoaguirre]

Did your fix work @iulianbarbu? Can you push it?

[iulianbarbu]

Did your fix work @iulianbarbu? Can you push it?

No fix 😞 . My trials were related only to running benchmarks without forklift, which resulted in same failures.

[iulianbarbu]

I am now able to reproduce this on my dev machine on scaleway. cc @franciscoaguirre (not sure how this happens - e.g. last time I tried to reproduce it I was running ony my personal desktop machine, and for some reason all benchmark tests passed). What's more interesting is that the test started to fail on the CI after merging the zepter/missing backport commit from you.

Ways to reproduce:

1. cd cumulus/parachains/runtimes/assets/asset-hub-westend && cargo clean
2. cargo build --release --features runtime-benchmarks
3. cd ../../../../..
4. cargo run --locked --release -p frame-omni-bencher --quiet -- v1 benchmark pallet --runtime target/release/wbuild/asset-hub-westend-runtime/asset_hub_westend_runtime.compact.compressed.wasm --all --steps 2 --repeat 1

I am able to consistently reproduce the initiate_teleport failure. Can you try again the above?
FYI, I get some more logs than we can see on the CI (hope this helps):

g benchmark: pallet_xcm_benchmarks::fungible::initiate_teleport
2025-03-24T09:42:12.634191Z ERROR runtime: panicked at /home/ubuntu/repos/polkadot-sdk/cumulus/primitives/utility/src/lib.rs:862:71:
called `Result::unwrap()` on an `Err` value: FailedToTransactAsset("AccountIdConversionFailed")

[franciscoaguirre]

Awesome! I'll try to reproduce it and check out that error

[iulianbarbu]

Thanks for the fix @franciscoaguirre ! Now xcm benchmarks pass again. I think we're good to go with this. cc @athei @ggwpez

[franciscoaguirre]

There seem to be some issues with the umbrella crate and the toml format?

[iulianbarbu]

There seem to be some issues with the umbrella crate and the toml format?

Fixed taplo, and I considered umbrella check not a must to fix with this PR (it fails on most of the release branches since inception). The fix is to continously update the script call to use the version from umbrella/Cargo.toml. I think we can do a proper fix for this in master, and backport it where necessary. Opened #8029.

At the same time, fixing taplo dismissed @ggwpez review again. Sorry, I'll need another approve.

[acatangiu]

@EgorPopelyaev looks like it is good. Are you able to merge this?

[EgorPopelyaev]

Nice! Yes, will do it.