MessageQueue pallet does not process message #2319

claravanstaden · 2023-11-14T13:54:38Z

Is there an existing issue?

I have searched the existing issues

Experiencing problems? Have you tried our Stack Exchange first?

This is not a support question.

Description of bug

When using the MessageQueue pallet to enqueue a message, and the process_message implementation of the enqueued message enqueues another message of a different origin, the other message is never processed.

To illustrate:

enqueue message (origin: Here message: first message)
when ☝🏻 is processed using trait ProcessMessage (method processed_message), enqueue another message (origin: There message: another message)
another message never gets processed

This happens specifically when the other message is added to the neighbour.prev pointer in the message queue ready_ring_unknit method (when the first message is linked to the other message).

Steps to reproduce

Here are tests that replicate the bug:

master...claravanstaden:master

The text was updated successfully, but these errors were encountered:

acatangiu · 2023-11-14T14:08:24Z

cc @ggwpez

vgeddes · 2023-11-14T15:26:09Z

Some further background information - Clara discovered the bug while updating Snowbridge to use a bleeding-edge version of the SDK. This version includes #1246, which changes the XCMP subsystem to use MessageQueue as a message buffer.

This results in the behavior Clara described in general terms. In our case specifically, when MessageQueue services a message with a Sibling(ParaId) origin, and this message contains an ExportMessage(dest=Ethereum) XCM instruction, then our handler for ExportMessage ends up enqueuing another message.

gavofyork · 2023-11-14T16:21:05Z

would be good if you could add a test to demonstrate the issue. @vgeddes @claravanstaden

claravanstaden · 2023-11-14T16:30:25Z

would be good if you could add a test to demonstrate the issue. @vgeddes @claravanstaden

@gavofyork I added a test over here: master...claravanstaden:master

ggwpez · 2023-11-14T16:44:45Z

I will fix it

ggwpez · 2023-11-14T17:24:12Z

It can easily be fixed by re-loading the ready neighbours before unknitting, to not accidentally unknit a recursively knitted book. However, i would like to write some more recursive tests first. After looking at the code i realize that there are other caveats that we seem to lucky have dodged so far.

ggwpez · 2023-11-15T20:52:39Z

Fix is here #2356. Now waiting for feedback on the approach, then hopefully going to audit/merge.

claravanstaden · 2023-11-16T15:56:09Z

@ggwpez I cherry-picked your fixes into Snowbridge to test. It resolves the problem we had, thank you! 🙏

Closes #2319 Changes: - Ensure that only `enqueue_message(s)` is callable from within the message processor. This prevents messed up storage that can currently happen when the pallet is called into recursively. - Use `H256` instead of `[u8; 32]` for clearer API. ## Details The re-entracy check is done with the `environmental` crate by adding a `with_service_mutex(f)` function that runs the closure exclusively. This works since the MQ pallet is not instantiable. --------- Signed-off-by: Oliver Tale-Yazdi <[email protected]> Co-authored-by: Francisco Aguirre <[email protected]>

Closes paritytech#2319 Changes: - Ensure that only `enqueue_message(s)` is callable from within the message processor. This prevents messed up storage that can currently happen when the pallet is called into recursively. - Use `H256` instead of `[u8; 32]` for clearer API. ## Details The re-entracy check is done with the `environmental` crate by adding a `with_service_mutex(f)` function that runs the closure exclusively. This works since the MQ pallet is not instantiable. --------- Signed-off-by: Oliver Tale-Yazdi <[email protected]> Co-authored-by: Francisco Aguirre <[email protected]>

* impl backpressure in the XcmBlobHaulerAdapter * LocalXcmQueueManager + more adapters * OnMessageDelviered callback * forbid mesage delivery transactions when the channel between target bridge hub and target asset hub is suspended * pallet-xcm-bridge-hub-router * removed commented code * improvements and tests for palle-xcm-bridge-router * use LocalXcmChannel in XcmBlobMessageDispatch * new tests for logic changes in messages pallet * tests for LocalXcmQueueSuspender * tests for LocalXcmQueueMessageProcessor * tests for new logic in the XcmBlobHaulerAdapter * fix other tests in the bridge-runtime-common * extension_reject_call_when_dispatcher_is_inactive * benchmarks for pallet-xcm-bridge-hub-router * get rid of redundant storage value * add new pallet to verify-pallets-build.sh * fixing spellcheck, clippy and rustdoc * trigger CI * Revert "trigger CI" This reverts commit 48f1ba032334e3c6d8470436483736988aa060ac. * change log target for xcm bridge router pallet * Update modules/xcm-bridge-hub-router/src/lib.rs Co-authored-by: Branislav Kontur <[email protected]> * use saturated_len where possible * fmt * (Suggestion) Ability to externalize configuration for `ExporterFor` (paritytech#2313) * Ability to externalize configuration for `ExporterFor` (Replaced `BridgedNetworkId/SiblingBridgeHubLocation` with `Bridges: ExporterFor`) * Fix millau * Compile fix * Return back `BridgedNetworkId` but as optional filter * Replaced `BaseFee` with fees from inner `Bridges: ExporterFor` * typo * Clippy * Rename LocalXcmChannel to XcmChannelStatusProvider (paritytech#2319) * Rename LocalXcmChannel to XcmChannelStatusProvider * fmt * added/fixed some docs * Dynamic fees v1: report congestion status to sending chain (paritytech#2318) * report congestion status: changes at the sending chain * OnMessagesDelivered is back * report congestion status: changes at the bridge hub * moer logging * fix? benchmarks * spelling * tests for XcmBlobHaulerAdapter and LocalXcmQueueManager * tests for messages pallet * fix typo * rustdoc * Update modules/messages/src/lib.rs * apply review suggestions * ".git/.scripts/commands/fmt/fmt.sh" * Added `XcmBridgeHubRouterCall::report_bridge_status` encodings for AHK/P (paritytech#2350) * Added `XcmBridgeHubRouterCall::report_bridge_status` encodings for AHK/P * Spellcheck * Added const for `XcmBridgeHubRouterTransactCallMaxWeight` * Cargo.lock * Introduced base delivery fee constants * Congestion messages as Optional to turn on/off `supports_congestion_detection` * Spellcheck * Ability to externalize dest for benchmarks * Ability to externalize dest for benchmarks --------- Co-authored-by: Branislav Kontur <[email protected]> Co-authored-by: command-bot <>

* impl backpressure in the XcmBlobHaulerAdapter * LocalXcmQueueManager + more adapters * OnMessageDelviered callback * forbid mesage delivery transactions when the channel between target bridge hub and target asset hub is suspended * pallet-xcm-bridge-hub-router * removed commented code * improvements and tests for palle-xcm-bridge-router * use LocalXcmChannel in XcmBlobMessageDispatch * new tests for logic changes in messages pallet * tests for LocalXcmQueueSuspender * tests for LocalXcmQueueMessageProcessor * tests for new logic in the XcmBlobHaulerAdapter * fix other tests in the bridge-runtime-common * extension_reject_call_when_dispatcher_is_inactive * benchmarks for pallet-xcm-bridge-hub-router * get rid of redundant storage value * add new pallet to verify-pallets-build.sh * fixing spellcheck, clippy and rustdoc * trigger CI * Revert "trigger CI" This reverts commit 48f1ba032334e3c6d8470436483736988aa060ac. * change log target for xcm bridge router pallet * Update modules/xcm-bridge-hub-router/src/lib.rs Co-authored-by: Branislav Kontur <[email protected]> * use saturated_len where possible * fmt * (Suggestion) Ability to externalize configuration for `ExporterFor` (#2313) * Ability to externalize configuration for `ExporterFor` (Replaced `BridgedNetworkId/SiblingBridgeHubLocation` with `Bridges: ExporterFor`) * Fix millau * Compile fix * Return back `BridgedNetworkId` but as optional filter * Replaced `BaseFee` with fees from inner `Bridges: ExporterFor` * typo * Clippy * Rename LocalXcmChannel to XcmChannelStatusProvider (#2319) * Rename LocalXcmChannel to XcmChannelStatusProvider * fmt * added/fixed some docs * Dynamic fees v1: report congestion status to sending chain (#2318) * report congestion status: changes at the sending chain * OnMessagesDelivered is back * report congestion status: changes at the bridge hub * moer logging * fix? benchmarks * spelling * tests for XcmBlobHaulerAdapter and LocalXcmQueueManager * tests for messages pallet * fix typo * rustdoc * Update modules/messages/src/lib.rs * apply review suggestions * ".git/.scripts/commands/fmt/fmt.sh" * Added `XcmBridgeHubRouterCall::report_bridge_status` encodings for AHK/P (#2350) * Added `XcmBridgeHubRouterCall::report_bridge_status` encodings for AHK/P * Spellcheck * Added const for `XcmBridgeHubRouterTransactCallMaxWeight` * Cargo.lock * Introduced base delivery fee constants * Congestion messages as Optional to turn on/off `supports_congestion_detection` * Spellcheck * Ability to externalize dest for benchmarks * Ability to externalize dest for benchmarks --------- Co-authored-by: Branislav Kontur <[email protected]> Co-authored-by: command-bot <>

claravanstaden added I10-unconfirmed Issue might be valid, but it's not yet known. I2-bug The node fails to follow expected behavior. labels Nov 14, 2023

claravanstaden changed the title ~~MessageQueue pallet does not process message enqueued within another processed enqueue message~~ MessageQueue pallet does not process message Nov 14, 2023

gavofyork assigned ggwpez Nov 14, 2023

ggwpez mentioned this issue Nov 15, 2023

[FRAME] Make MQ pallet re-entrancy safe #2356

Merged

claravanstaden mentioned this issue Nov 24, 2023

Message queue reentrancy issue fix Snowfork/polkadot-sdk#37

Merged

gavofyork closed this as completed in #2356 Dec 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MessageQueue pallet does not process message #2319

MessageQueue pallet does not process message #2319

claravanstaden commented Nov 14, 2023 •

edited

Loading

acatangiu commented Nov 14, 2023

vgeddes commented Nov 14, 2023 •

edited

Loading

gavofyork commented Nov 14, 2023 •

edited

Loading

claravanstaden commented Nov 14, 2023 •

edited

Loading

ggwpez commented Nov 14, 2023

ggwpez commented Nov 14, 2023 •

edited

Loading

ggwpez commented Nov 15, 2023 •

edited

Loading

claravanstaden commented Nov 16, 2023 •

edited

Loading

MessageQueue pallet does not process message #2319

MessageQueue pallet does not process message #2319

Comments

claravanstaden commented Nov 14, 2023 • edited Loading

Is there an existing issue?

Experiencing problems? Have you tried our Stack Exchange first?

Description of bug

Steps to reproduce

acatangiu commented Nov 14, 2023

vgeddes commented Nov 14, 2023 • edited Loading

gavofyork commented Nov 14, 2023 • edited Loading

claravanstaden commented Nov 14, 2023 • edited Loading

ggwpez commented Nov 14, 2023

ggwpez commented Nov 14, 2023 • edited Loading

ggwpez commented Nov 15, 2023 • edited Loading

claravanstaden commented Nov 16, 2023 • edited Loading

claravanstaden commented Nov 14, 2023 •

edited

Loading

vgeddes commented Nov 14, 2023 •

edited

Loading

gavofyork commented Nov 14, 2023 •

edited

Loading

claravanstaden commented Nov 14, 2023 •

edited

Loading

ggwpez commented Nov 14, 2023 •

edited

Loading

ggwpez commented Nov 15, 2023 •

edited

Loading

claravanstaden commented Nov 16, 2023 •

edited

Loading