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

Update to SDK stable2409 #490

Open
wants to merge 53 commits into
base: main
Choose a base branch
from

Conversation

pandres95
Copy link
Contributor

@pandres95 pandres95 commented Oct 19, 2024

Closes #457
Closes #469

Updating the runtimes to SDK version stable2409. CHANGELOG mentions all relevant changes for UI and Walletbuilders.

Checklist

  • Update SDK to stable2409
  • Update configuration for relay chains.
  • Update configuration for system parachains.
  • Update configuration for emulated chains.
  • Update configuration for Encointer (see Help wanted).
  • Update CHANGELOG.

Dependencies

This PR depends on certain PRs to be merged before it can properly work.

Impacts

The success of this PR impacts directly on the feasibility of executing certain issues, or closing certain PRs.

Help wanted

Initially, I request the help of:

  • @franciscoaguirre to help me correcting the potential conflicts that may arise when foreign assets change to XCMv4.
  • @acatangiu to help me setting up the Bridge Hubs, as well as adding the respective transition layer (a.k.a. preparing the migrations from fixed Lanes to LegacyLane-based lanes storage).
  • @clangenb to help me upgrading Encointer.
  • @Szegoo to check the parameters that changed in Coretime.

@pandres95 pandres95 changed the title Upgrade to Polkadot SDK stable2409 Update to Polkadot SDK stable2409 Oct 19, 2024
@pandres95 pandres95 changed the title Update to Polkadot SDK stable2409 Update to SDK stable2409 Oct 19, 2024
@pandres95 pandres95 force-pushed the upgrade-polkadot-stable2409 branch 3 times, most recently from f08d159 to 4b4a252 Compare October 21, 2024 02:23
- Rename `assigner_on_demand` to `on_demand` (SDK #4706)
- [BEEFY] Add runtime support for reporting fork voting (#4522)
- `SchedulerParams` moved from `polkadot_primitives::vstaging`, to `polkadot_primitives`
- `XcmFeeToAccount` -> `SendXcmFeeToAccount`
- Mock Weights
- Rename `assigner_on_demand` to `on_demand` (SDK #4706)
- [BEEFY] Add runtime support for reporting fork voting (SDK #4522)
- `SchedulerParams` moved from `polkadot_primitives::vstaging`, to `polkadot_primitives`
- `XcmFeeToAccount` -> `SendXcmFeeToAccount`
- Mock Weights
- [Assets] Call implementation for `transfer_all` (SDK #4527)
- [bridges-v2] Permissionless lanes (SDK #4949)
- add possibility to inject non-authorities session-keys in genesis (SDK #5078)
- Tx Payment: drop ED requirements for tx payments with exchangeable asset (SDK #4488)
- relax XcmFeeToAccount trait bound on AccountId (SDK #4959)
- [Assets] Call implementation for `transfer_all` (SDK #4527)
- [bridges-v2] Permissionless lanes (SDK #4949)
- add possibility to inject non-authorities session-keys in genesis (SDK #5078)
- Tx Payment: drop ED requirements for tx payments with exchangeable asset (SDK #4488)
- relax XcmFeeToAccount trait bound on AccountId (SDK #4959)
- Bridges V2 refactoring backport and pallet_bridge_messages simplifications (SDK #4935)
- [bridges-v2] Permissionless lanes (SDK #4949)
- add possibility to inject non-authorities session-keys in genesis (SDK #5078)
- Transfer Polkadot-native assets to Ethereum (SDK #5546)
- [stable2049] Backport #5546 (SDK #5710)
- `XcmFeeToAccount` -> `SendXcmFeeToAccount`
- Bridges V2 refactoring backport and pallet_bridge_messages simplifications (SDK #4935)
- [bridges-v2] Permissionless lanes (SDK #4949)
- add possibility to inject non-authorities session-keys in genesis (SDK #5078)
- Transfer Polkadot-native assets to Ethereum (SDK #5546)
- [stable2049] Backport #5546 (SDK #5710)
- `XcmFeeToAccount` -> `SendXcmFeeToAccount`
- [bh polkadot] minor: replace `DOLLARS` with `UNITS` on `BridgeDeposit` storage type
- `XcmFeeToAccount` -> `SendXcmFeeToAccount`
- add possibility to inject non-authorities session-keys in genesis (SDK #5078)
- `XcmFeeToAccount` -> `SendXcmFeeToAccount`
- add possibility to inject non-authorities session-keys in genesis (SDK #5078)
- Coretime auto-renew (SDK #4424)
- Mock Weights
- `XcmFeeToAccount` -> `SendXcmFeeToAccount`
- add possibility to inject non-authorities session-keys in genesis (SDK #5078)
- Coretime auto-renew (SDK #4424)
- Mock Weights
- `XcmFeeToAccount` -> `SendXcmFeeToAccount`
- add possibility to inject non-authorities session-keys in genesis (SDK #5078)
- `XcmFeeToAccount` -> `SendXcmFeeToAccount`
- add possibility to inject non-authorities session-keys in genesis (SDK #5078)
- add possibility to inject non-authorities session-keys in genesis (SDK #5078)
@pandres95 pandres95 marked this pull request as ready for review October 21, 2024 07:38
@clangenb
Copy link
Contributor

I can probably squeeze it in later this week. 👍

@pandres95
Copy link
Contributor Author

Opened for review with a small caveat: tests will keep failing, until #472 is merged and can be merged back to this branch.

Comment on lines 136 to 137
// TODO: What's the correct value?
pub storage BridgeDeposit: Balance = constants::currency::UNITS;
Copy link
Contributor

Choose a reason for hiding this comment

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

for testnets I used values related to the HRMP deposits from live testnets, we could use those or add some utility adapter, which would read HRMP deposit from HostConfiguration

Suggested change
// TODO: What's the correct value?
pub storage BridgeDeposit: Balance = constants::currency::UNITS;
// TODO: What's the correct value? - FAIL-CI
pub storage BridgeDeposit: Balance = constants::currency::UNITS;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have a question: is there a specific place in runtimes where I can get the HRMP deposit value? Or somewhere in the testnet (on polkadot-sdk), maybe?

…ge_pallet_ethereum_client::WeightInfo::submit_with_sync_committee` to assert tests are passing
@pandres95
Copy link
Contributor Author

/cmd bench --runtime bridge-hub-kusama --continue-on-fail

Copy link

Command "--runtime bridge-hub-kusama --continue-on-fail" has started 🚀 See logs here

Copy link

Command "--runtime bridge-hub-kusama --continue-on-fail" has failed ❌! See logs here

@acatangiu
Copy link
Contributor

@acatangiu I found that in kah integration tests, the chain 11155111 is being used as the origin chain for wETH. So, I made this change: https://github.com/pandres95/runtimes/blob/f8adb35e0efda35c96b5e3913e60f7f201aecfcb/system-parachains/asset-hubs/asset-hub-kusama/src/xcm_config.rs#L516-L521

Is this alright? Or should we change that value in the tests and point to the chain 1?

The Rococo/Westend testnets are bridged to Ethereum Sepolia testnet which is using chain_id = 11155111.

But Ethereum chain_id should be 1 throughout this repo, as that's Ethereum Mainnet which Polkadot is bridged to.

use bp_bridge_hub_polkadot::snowbridge::{CreateAssetCall, InboundQueuePalletInstance, Parameters};
pub use bp_bridge_hub_polkadot::snowbridge::{EthereumLocation, EthereumNetwork};
Copy link
Contributor

Choose a reason for hiding this comment

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

@yrong @alistair-singh @vgeddes I have compared the PNA runtime changes required and all looks correct, from what I can see. Please double check :)

@claravanstaden
Copy link
Contributor

@pandres95 I added the Snowbridge Polkadot-native token transfer to Ethereum integration tests here: pandres95#2

@acatangiu we need to set the ReserveAssetDeposited weight in PBH to something realistic for PNA to work.

@acatangiu
Copy link
Contributor

@acatangiu we need to set the ReserveAssetDeposited weight in PBH to something realistic for PNA to work.

why? where would BridgeHub receive/execute a ReserveAssetDeposited from? BH only handles DOT which cannot be transferred into BH as a reserve transfer with a remote reserve.

@claravanstaden
Copy link
Contributor

why? where would BridgeHub receive/execute a ReserveAssetDeposited from? BH only handles DOT which cannot be transferred into BH as a reserve transfer with a remote reserve.

Our EthereumBlobExporter expects a message that contains ReserveAssetDeposited. The exporter then parses the xcm payload to send a command to Ethereum. So it does not actually execute ReserveAssetDeposited with the XCM executor, but I think the message is still weighed per instruction.

@acatangiu
Copy link
Contributor

acatangiu commented Oct 29, 2024

Our EthereumBlobExporter expects a message that contains ReserveAssetDeposited. The exporter then parses the xcm payload to send a command to Ethereum. So it does not actually execute ReserveAssetDeposited with the XCM executor, but I think the message is still weighed per instruction.

AFAIK, the XCM is weighed as a first step of execution. If BridgeHub is not executing the XCM, why would it weigh it?

I was expecting the XCM is captured, parsed and consumed by the Snowbridge exporter before being weighed or executed. Ideally it should be captured even before trying to go through the BH XCM barriers, but I'm not sure about that, needs checking. L.E.: barriers are used during execution, so they should also not be a problem.

@claravanstaden
Copy link
Contributor

claravanstaden commented Oct 29, 2024

@acatangiu you are right, its not the weight that is a problem. It is related to withdrawing the fee asset it seems:

2024-10-29T10:24:55.407179Z TRACE xcm::bridge-hub-router: Router with bridged_network_id Kusama does not support bridging to network Ethereum { chain_id: 1 }!    
2024-10-29T10:24:55.407185Z TRACE xcm::bridge-hub-router: validate - ViaBridgeHubExporter - error: NotApplicable    
2024-10-29T10:24:55.407318Z TRACE xcm::pallet_xcm::note_unknown_version: XCM version is unknown for destination: Location { parents: 1, interior: X1([Parachain(1002)]) }    
2024-10-29T10:24:55.407504Z TRACE xcm::contains: AllSiblingSystemParachains location: Location { parents: 0, interior: X1([AccountId32 { network: Some(Polkadot), id: [212, 53, 147, 199, 21, 253, 211, 28, 97, 20, 26, 189, 4, 169, 159, 214, 130, 44, 133, 88, 133, 76, 205, 227, 154, 86, 132, 231, 165, 109, 162, 125] }]) }    
2024-10-29T10:24:55.408142Z TRACE xcm::fungible_adapter: withdraw_asset what: Asset { id: AssetId(Location { parents: 1, interior: Here }), fun: Fungible(340282366920938463463374607431768211455) }, who: Location { parents: 0, interior: X1([AccountId32 { network: Some(Polkadot), id: [212, 53, 147, 199, 21, 253, 211, 28, 97, 20, 26, 189, 4, 169, 159, 214, 130, 44, 133, 88, 133, 76, 205, 227, 154, 86, 132, 231, 165, 109, 162, 125] }]) } 

@claravanstaden
Copy link
Contributor

@pandres95 @acatangiu test fixes over here: pandres95#4 Thanks @alistair-singh for the BridgeHubEthereumBaseFee tip, you were right. 🏆

@pandres95
Copy link
Contributor Author

@bkontur
Copy link
Contributor

bkontur commented Oct 29, 2024

@acatangiu @bkontur any clue about this error?

https://github.com/polkadot-fellows/runtimes/actions/runs/11574125246/job/32217719672#step:11:244

I think it should be related to the frame-omni-bencher bug, let me check that and try it tomorrow

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

upgrade to polkadot sdk stable2409 chore: upgrade to polkadot sdk stable2407
7 participants