migration: Add LP Gateway migration for outbound messages #1946

cdamian · 2024-08-05T09:43:07Z

Description

Add a migration that removes the LP gateway storages related to outbound messages.

Checklist:

I have added Rust doc comments to structs, enums, traits and functions
I have made corresponding changes to the documentation
I have performed a self-review of my code
I have added tests that prove my fix is effective or that my feature works

lemunozm · 2024-08-05T09:47:00Z

runtime/common/src/migrations/liquidity_pools_gateway.rs

+		for (nonce, entry) in OutboundMessageQueue::<T>::iter() {
+			log::warn!("{LOG_PREFIX}: Found outbound message:\nnonce:{nonce}\nentry:{entry:?}");
+			weight = weight.saturating_add(<T as frame_system::Config>::DbWeight::get().reads(1));
+		}
+
+		for (nonce, entry) in FailedOutboundMessages::<T>::iter() {
+			log::warn!(
+				"{LOG_PREFIX}: Found failed outbound message:\nnonce:{nonce}\nentry:{entry:?}"
+			);
+			weight = weight.saturating_add(<T as frame_system::Config>::DbWeight::get().reads(1));
+		}


I'm pretty sure if they're empty, we do not need to remove them.

cc @wischli

Yea, this is more like an extra check to ensure that they're empty. Not doing any specific removals here apart from the nonce storage.

cdamian · 2024-08-05T09:55:50Z

Testing this locally using:

try-runtime --runtime existing create-snapshot --uri [wss://fullnode.centrifuge.io:443](wss://fullnode.centrifuge.io/) cfg.snap

try-runtime --runtime target/release/wbuild/centrifuge-runtime/centrifuge_runtime.wasm on-runtime-upgrade --disable-spec-version-check snap -p cfg.snap

However, it errors out:

[2024-08-05T09:30:44Z ERROR runtime] panicked at /Users/cdamian/codebase/centrifuge-chain/runtime/common/src/migrations/liquidity_pools_gateway.rs:97:9:
    assertion `left == right` failed: OutboundMessageNonce should be empty!
      left: 55
     right: 0
thread 'main' panicked at cli/main.rs:324:10:
called `Result::unwrap()` on an `Err` value: Input("failed to execute TryRuntime_on_runtime_upgrade: Execution aborted due to trap: wasm trap: wasm `unreachable` instruction executed\nWASM backtrace:\nerror while executing at wasm backtrace:\n ...

lemunozm · 2024-08-05T09:59:14Z

That's normal. you have a ValueQuery in that storage. So if no entry is found, a default is returned. But that does not add any real entry in the storage. I think you should check that the value is 0.

cdamian · 2024-08-05T10:03:53Z

runtime/common/src/migrations/liquidity_pools_gateway.rs

+
+	#[cfg(feature = "try-runtime")]
+	fn post_upgrade(_: Vec<u8>) -> Result<(), TryRuntimeError> {
+		assert_eq!(


@lemunozm this is the assert that fails when running try-runtime.

Not sure where that panic comes from tho, that's a different story.

What's the error it tells you? "OutboundMessageNonceStore should be 0!" ?

Yes, with the latest version that's what I get in the post check. But, I guess that makes sense since the on_runtime_upgrade doesn't really get triggered or am I missing something?

Looking into this now

Mmm.. I think the assert should pass IIUC this 🤔

The previous message was failing correctly because "there is" entry if you read. But if you check the value it should be the default one, 0

wischli

Wrong prefix constructor for clearing. Apart from that I would like to add a storage value to the pallet and bump it by one because this is a breaking storage change.

Luckily, you don't need to extend the code much because you can wrap your migration into a VersionedMigration exposed from the Polkadot SDK: https://paritytech.github.io/polkadot-sdk/master/frame_support/migrations/struct.VersionedMigration.html

Unfortunately, the try-runtime will still fail because of two unsynchronized pallets:

[2024-08-06T09:58:25Z ERROR runtime::frame-support] ForeignInvestments: On chain storage version StorageVersion(1) doesn't match current storage version StorageVersion(2).
[2024-08-06T09:58:25Z ERROR runtime::frame-support] DmpQueue: On chain storage version StorageVersion(0) doesn't match current storage version StorageVersion(2).

For FI, we just need to bump and the DMP Queue can actually be removed because it is deprecated but we needed to keep it in the migration to Polkadot v1.7.2 because that included a migration to the new pallet MessageQueue / pallet_message_queue.

I am happy to open a PR with all the fixes including wrapping the gateway migration into a versioned one. WDYT @cdamian ?

wischli · 2024-08-06T09:59:39Z

runtime/common/src/migrations/liquidity_pools_gateway.rs

+{
+	fn on_runtime_upgrade() -> Weight {
+		let mut weight = Self::clear_storage(
+			OutboundMessageNonceStore::<T>::storage_prefix(),


This only represents the b"OutboundMessageNonceStore" but is missing the pallet prefix. The solution is to always use the final storage key for clearing.

Suggested change

OutboundMessageNonceStore::<T>::storage_prefix(),

OutboundMessageNonceStore::<T>::storage_value_final_key(),

Oh, I thought the storage_alias takes care of that.

wischli · 2024-08-06T10:00:13Z

runtime/common/src/migrations/liquidity_pools_gateway.rs

+		let mut weight = Self::clear_storage(
+			OutboundMessageNonceStore::<T>::storage_prefix(),
+			"OutboundMessageNonceStore",
+		);


An alternative, and my preference, would be just utilizing OutboundMessageNonceStore::<T>::kill() here as it is a simple storage value.

* chore: remove deprecated DMP Queue * chore: remove deprecated custom migrations * fix: gateway migration * fix: rm FI v1 storage entries * ci: fix try-runtime cli by adding install

wischli · 2024-08-08T09:02:33Z

The try-runtime CLI works now! It failed for Altair because in the pre_upgrade we are assuming the OutboundMessageNonceStore to be greater than 0 which obviously isn't the case for Altair. Normally, I would fix that but given that Altair is low priority, especially currently with the strict audit deadline, and is not expected to utilize LPs, we can ignore it IMO.

wischli

Approving our mutual work. Great job 😅

lemunozm

Thanks!

lemunozm reviewed Aug 5, 2024

View reviewed changes

migration: Add LP Gateway migration for outbound messages

33f81d6

cdamian force-pushed the feat/lp-v2-gateway-queue-migration branch from c635584 to 33f81d6 Compare August 5, 2024 10:02

cdamian commented Aug 5, 2024

View reviewed changes

wischli reviewed Aug 6, 2024

View reviewed changes

wischli mentioned this pull request Aug 7, 2024

fix: gateway migration et al. #1950

Merged

4 tasks

fix: gateway migration et al. (#1950)

ebd3e08

* chore: remove deprecated DMP Queue * chore: remove deprecated custom migrations * fix: gateway migration * fix: rm FI v1 storage entries * ci: fix try-runtime cli by adding install

wischli assigned cdamian and wischli Aug 8, 2024

wischli added the D8-migration Pull request touches storage and needs migration code. label Aug 8, 2024

Merge branch 'main' into feat/lp-v2-gateway-queue-migration

9e1e4a4

wischli marked this pull request as ready for review August 8, 2024 07:23

wischli requested review from mustermeiszer and hieronx as code owners August 8, 2024 07:23

wischli approved these changes Aug 8, 2024

View reviewed changes

wischli enabled auto-merge (squash) August 8, 2024 09:03

wischli requested a review from lemunozm August 8, 2024 09:03

wischli mentioned this pull request Aug 8, 2024

LPv2: Batch message logic (r2) #1949

Merged

lemunozm approved these changes Aug 8, 2024

View reviewed changes

Merge branch 'main' into feat/lp-v2-gateway-queue-migration

e6d851a

lemunozm approved these changes Aug 8, 2024

View reviewed changes

wischli merged commit 4bfeabe into main Aug 8, 2024
9 of 10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

migration: Add LP Gateway migration for outbound messages #1946

migration: Add LP Gateway migration for outbound messages #1946

cdamian commented Aug 5, 2024 •

edited

Loading

lemunozm Aug 5, 2024

cdamian Aug 5, 2024

cdamian commented Aug 5, 2024

lemunozm commented Aug 5, 2024

cdamian Aug 5, 2024

lemunozm Aug 6, 2024

cdamian Aug 6, 2024

wischli Aug 6, 2024

lemunozm Aug 6, 2024

lemunozm Aug 6, 2024

wischli left a comment

wischli Aug 6, 2024 •

edited

Loading

cdamian Aug 6, 2024

wischli Aug 6, 2024

wischli commented Aug 8, 2024

wischli left a comment

lemunozm left a comment

	OutboundMessageNonceStore::<T>::storage_prefix(),
	OutboundMessageNonceStore::<T>::storage_value_final_key(),

migration: Add LP Gateway migration for outbound messages #1946

migration: Add LP Gateway migration for outbound messages #1946

Conversation

cdamian commented Aug 5, 2024 • edited Loading

Description

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdamian commented Aug 5, 2024

lemunozm commented Aug 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wischli left a comment

Choose a reason for hiding this comment

wischli Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wischli commented Aug 8, 2024

wischli left a comment

Choose a reason for hiding this comment

lemunozm left a comment

Choose a reason for hiding this comment

cdamian commented Aug 5, 2024 •

edited

Loading

wischli Aug 6, 2024 •

edited

Loading