Transactional processing for XCM #1222

vstam1 · 2023-08-28T14:56:16Z

closes #490

update cumulus

This PR introduces transactional processing of certain xcm instructions. For the list of instructions checkout #490. The transactional processing is implemented as an xcm-executor config item. The two implementations in this PR are FrameTransactionalProcessor and (). The () implementation does no transactional processing. Each implementation of the ProcessTransaction trait has an IS_TRANSACTIONAL const that tells the XCVM if transactional processing is actually implemented. If Transactional processing is implemented, changes to touched registers should also be rolled back to prevent inconsistencies.

Note for reviewers:
Check out the following safety assumption: https://github.com/paritytech/polkadot-sdk/pull/1222/files#diff-4effad7d8c1c9de19fd27e18661cbf2128c8718f3b2420a27d2f816e0749ea53R30

polkadot/xcm/xcm-executor/src/lib.rs

gavofyork · 2023-08-30T11:42:51Z

polkadot/xcm/xcm-executor/src/lib.rs

- reduce_ticket.enact()?;
- Config::XcmSender::deliver(ticket)?;


pretty sure these two are guaranteed to succeed as long as the tickets were created recently.

gavofyork

Looks broadly ok except for the comment.

No problem with rolling back if BuyExecution fails. An attacker can do nothing worse than at present. As long as barriers and trapping filters are secure then it'll bail on the Err without the possibility of doing any work other than the asset withdrawal and failed buy (which we cannot avoid in the general case).

gavofyork · 2023-08-30T15:05:39Z

polkadot/xcm/xcm-executor/src/lib.rs

+ for asset in assets.assets_iter() {
+ Config::AssetTransactor::check_out(&dest, &asset, &self.context);
+ }
+ // Note that we pass `None` as `maybe_failed_bin` and drop any assets which
+ // cannot be reanchored because we have already checked all assets out.
+ let assets = Self::reanchored(assets, &dest, None);
+ let mut message = vec![ReceiveTeleportedAsset(assets), ClearOrigin];
+ message.extend(xcm.0.into_iter());
+ self.send(dest, Xcm(message), FeeReason::InitiateTeleport)?;


Once all the can_check_out calls pass, then the only thing which can fail is send. Therefore if that is done prior to the check_out calls, then this can avoid the need for transactionality.

We do want to keep the rollback of the holding register, correct?

polkadot/xcm/xcm-executor/src/lib.rs

gavofyork · 2023-08-30T15:10:51Z

I think with careful review there's still an opportunity to remove a bit of transactionality.

polkadot/runtime/test-runtime/src/xcm_config.rs

polkadot/xcm/xcm-builder/src/tests/pay/mock.rs

KiChjang

Little syntax nits, but should be good to go after addressing Gav's comments.

…ional-processing

paritytech-cicd-pr · 2024-01-24T13:44:29Z

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-clippy
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5018261

xlc · 2024-01-25T01:14:34Z

polkadot/xcm/xcm-builder/src/transactional.rs

+ _ => TransactionOutcome::Rollback(Ok(output)),
+ }
+ })
+ .map_err(|_| XcmError::ExceedsStackLimit)?


we got shoot by .map_err(|_| too many times so please add a comment to explain why it is acceptable to ignore the error here

The only place that can throw an error is within the processing of with_transaction, NOT the closure that is being passed in as a parameter -- if you look closely above, there's no possible way that the closure can return an Err.

Moved from: paritytech/polkadot#6951 closes #490 - [x] update cumulus --- This PR introduces transactional processing of certain xcm instructions. For the list of instructions checkout #490. The transactional processing is implemented as an xcm-executor config item. The two implementations in this PR are `FrameTransactionalProcessor` and `()`. The `()` implementation does no transactional processing. Each implementation of the `ProcessTransaction` trait has an `IS_TRANSACTIONAL` const that tells the XCVM if transactional processing is actually implemented. If Transactional processing is implemented, changes to touched registers should also be rolled back to prevent inconsistencies. Note for reviewers: Check out the following safety assumption: https://github.com/paritytech/polkadot-sdk/pull/1222/files#diff-4effad7d8c1c9de19fd27e18661cbf2128c8718f3b2420a27d2f816e0749ea53R30 --------- Co-authored-by: Keith Yeung <kungfukeith11@gmail.com> Co-authored-by: Francisco Aguirre <franciscoaguirreperez@gmail.com> Co-authored-by: command-bot <>

Moved from: paritytech/polkadot#6951 closes paritytech#490 - [x] update cumulus --- This PR introduces transactional processing of certain xcm instructions. For the list of instructions checkout paritytech#490. The transactional processing is implemented as an xcm-executor config item. The two implementations in this PR are `FrameTransactionalProcessor` and `()`. The `()` implementation does no transactional processing. Each implementation of the `ProcessTransaction` trait has an `IS_TRANSACTIONAL` const that tells the XCVM if transactional processing is actually implemented. If Transactional processing is implemented, changes to touched registers should also be rolled back to prevent inconsistencies. Note for reviewers: Check out the following safety assumption: https://github.com/paritytech/polkadot-sdk/pull/1222/files#diff-4effad7d8c1c9de19fd27e18661cbf2128c8718f3b2420a27d2f816e0749ea53R30 --------- Co-authored-by: Keith Yeung <kungfukeith11@gmail.com> Co-authored-by: Francisco Aguirre <franciscoaguirreperez@gmail.com> Co-authored-by: command-bot <>

* Setup deps * Remove Koi from account migration test * paritytech/polkadot-sdk#1495 * Bump * paritytech/polkadot-sdk#1524 * !! paritytech/polkadot-sdk#1363 * paritytech/polkadot-sdk#1492 * paritytech/polkadot-sdk#1911 * paritytech/polkadot-sdk#1900 Signed-off-by: Xavier Lau <xavier@inv.cafe> * paritytech/polkadot-sdk#1661 * paritytech/polkadot-sdk#2144 * paritytech/polkadot-sdk#2048 * paritytech/polkadot-sdk#1672 * paritytech/polkadot-sdk#2303 * paritytech/polkadot-sdk#1256 * Remove identity and vesting * Fixes * paritytech/polkadot-sdk#2657 * paritytech/polkadot-sdk#1313 * paritytech/polkadot-sdk#2331 * paritytech/polkadot-sdk#2409 part.1 * paritytech/polkadot-sdk#2767 * paritytech/polkadot-sdk#2521 Signed-off-by: Xavier Lau <xavier@inv.cafe> * paritytech/polkadot-sdk#1222 * paritytech/polkadot-sdk#1234 part.1 * Satisfy compiler * XCM V4 part.1 * paritytech/polkadot-sdk#1246 * Remove pallet-democracy part.1 * paritytech/polkadot-sdk#2142 * paritytech/polkadot-sdk#2428 * paritytech/polkadot-sdk#3228 * XCM V4 part.2 * Bump * Build all runtimes * Build node * Remove pallet-democracy Signed-off-by: Xavier Lau <xavier@inv.cafe> * Format * Fix pallet tests * Fix precompile tests * Format * Fixes * Async, remove council, common pallet config * Fix `ethtx-forward` test case (#1519) * Fix ethtx-forward tests * Format * Fix following the review * Fixes * Fixes * Use default impl * Benchmark helper * Bench part.1 * Bench part.2 * Bench part.3 * Fix all tests * Typo * Feat * Fix EVM tracing build * Reuse upstream `proof_size_base_cost()` (#1521) * Format issue * Fixes * Fix CI --------- Signed-off-by: Xavier Lau <xavier@inv.cafe> Co-authored-by: Bear Wang <boundless.forest@outlook.com>

transactional processing for xcm

775488a

paritytech-ci requested review from a team August 28, 2023 14:57

vstam1 added T6-XCM This PR/Issue is related to XCM. A3-backport Pull request is already reviewed well in another branch. labels Aug 28, 2023

vstam1 and others added 5 commits August 29, 2023 10:30

cumulus changes

a77608a

fix error

24ffca1

fix error

900b093

fmt

7f691a2

Merge branch 'master' into vstam1/xcm-transactional-processing

04c9712

gavofyork reviewed Aug 30, 2023

View reviewed changes

polkadot/xcm/xcm-executor/src/lib.rs Outdated Show resolved Hide resolved

paritytech-ci requested review from a team August 30, 2023 11:42

gavofyork reviewed Aug 30, 2023

View reviewed changes

paritytech-ci requested review from a team August 30, 2023 11:43

gavofyork approved these changes Aug 30, 2023

View reviewed changes

paritytech-ci requested a review from a team August 30, 2023 11:56

remove transactional processing for UnlockAsset and RequestUnlock

09a41c8

gavofyork changed the title ~~transactional processing for xcm~~ Transactional processing for XCM Aug 30, 2023

gavofyork reviewed Aug 30, 2023

View reviewed changes

paritytech-ci requested a review from a team August 30, 2023 15:07

gavofyork reviewed Aug 30, 2023

View reviewed changes

polkadot/xcm/xcm-executor/src/lib.rs Outdated Show resolved Hide resolved

paritytech-ci requested a review from a team August 30, 2023 15:10

KiChjang reviewed Aug 31, 2023

View reviewed changes

polkadot/runtime/test-runtime/src/xcm_config.rs Show resolved Hide resolved

KiChjang reviewed Aug 31, 2023

View reviewed changes

polkadot/xcm/xcm-builder/src/tests/pay/mock.rs Show resolved Hide resolved

KiChjang approved these changes Aug 31, 2023

View reviewed changes

address feedback

b2f75ee

Add missing config item in westend runtime

7ea3675

franciscoaguirre disabled auto-merge January 15, 2024 14:36

KiChjang added 2 commits January 16, 2024 21:53

Merge remote-tracking branch 'origin/master' into vstam1/xcm-transact…

4d1e6bb

…ional-processing

Merge remote-tracking branch 'origin/master' into vstam1/xcm-transact…

afdc4b9

…ional-processing

KiChjang enabled auto-merge January 23, 2024 18:08

KiChjang added 3 commits January 24, 2024 11:44

Fixes

f9e342f

Fixes

75403af

Fixes

311968b

Fixes

6020a71

KiChjang added this pull request to the merge queue Jan 24, 2024

Merged via the queue into master with commit 50eb12c Jan 24, 2024
123 of 124 checks passed

KiChjang deleted the vstam1/xcm-transactional-processing branch January 24, 2024 17:11

xlc reviewed Jan 25, 2024

View reviewed changes

ggwpez mentioned this pull request Jan 29, 2024

Transactional account modification #3109

Closed

KiChjang added a commit that referenced this pull request Jan 29, 2024

Backport transactional processing PR (#1222)

7bc62d9

NachoPal mentioned this pull request Jan 29, 2024

BUG: XCM transfer, that would slash account leads to total asset loss. #3050

Closed

2 tasks

acatangiu pushed a commit that referenced this pull request Jan 30, 2024

Backport transactional processing PR (#1222) (#3113)

dbb7171

franciscoaguirre pushed a commit that referenced this pull request Feb 1, 2024

Backport transactional processing PR (#1222) (#3113)

977513d

This was referenced Feb 29, 2024

Add regression tests for approval voting subsystem #3529

Closed

Add subsystem regression tests to CI #3530

Closed

Visualize subsystem regression tests results #3531

Closed

github-actions bot mentioned this pull request Mar 13, 2024

Update polkadot-sdk from v1.3.0 to v1.7.2 moonbeam-foundation/moonbeam#2703

Closed

AurevoirXavier added a commit to darwinia-network/darwinia that referenced this pull request Jun 25, 2024

paritytech/polkadot-sdk#1222

edab2d5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Transactional processing for XCM #1222

Transactional processing for XCM #1222

vstam1 commented Aug 28, 2023 •

edited

Loading

gavofyork Aug 30, 2023

gavofyork left a comment

gavofyork Aug 30, 2023

vstam1 Aug 31, 2023

gavofyork commented Aug 30, 2023

KiChjang left a comment

paritytech-cicd-pr commented Jan 24, 2024

xlc Jan 25, 2024

KiChjang Jan 25, 2024

Transactional processing for XCM #1222

Transactional processing for XCM #1222

Conversation

vstam1 commented Aug 28, 2023 • edited Loading

gavofyork Aug 30, 2023

Choose a reason for hiding this comment

gavofyork left a comment

Choose a reason for hiding this comment

gavofyork Aug 30, 2023

Choose a reason for hiding this comment

vstam1 Aug 31, 2023

Choose a reason for hiding this comment

gavofyork commented Aug 30, 2023

KiChjang left a comment

Choose a reason for hiding this comment

paritytech-cicd-pr commented Jan 24, 2024

xlc Jan 25, 2024

Choose a reason for hiding this comment

KiChjang Jan 25, 2024

Choose a reason for hiding this comment

vstam1 commented Aug 28, 2023 •

edited

Loading