Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Introduce transactional processing for xcm instructions #6951

Open
wants to merge 26 commits into
base: master
Choose a base branch
from

Conversation

vstam1
Copy link
Contributor

@vstam1 vstam1 commented Mar 24, 2023

@vstam1 vstam1 added A3-in_progress Pull request is in progress. No review needed at this stage. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Mar 24, 2023
xcm/xcm-builder/src/transactional.rs Outdated Show resolved Hide resolved
xcm/xcm-executor/src/config.rs Show resolved Hide resolved
@jakoblell jakoblell added the D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited. label Mar 29, 2023
Merge branch 'master' into vstam1/xcm-transactional-processing
@vstam1
Copy link
Contributor Author

vstam1 commented Jun 13, 2023

bot rebase

@paritytech-processbot
Copy link

Rebased

@vstam1 vstam1 marked this pull request as ready for review August 3, 2023 09:43
@paritytech-ci paritytech-ci requested review from a team August 3, 2023 09:43
@vstam1
Copy link
Contributor Author

vstam1 commented Aug 3, 2023

bot rebase

@paritytech-processbot
Copy link

Rebased

xcm/xcm-executor/src/traits/process_transaction.rs Outdated Show resolved Hide resolved
xcm/xcm-executor/src/traits/process_transaction.rs Outdated Show resolved Hide resolved
xcm/xcm-executor/src/traits/process_transaction.rs Outdated Show resolved Hide resolved
xcm/xcm-executor/src/lib.rs Outdated Show resolved Hide resolved
xcm/xcm-builder/src/transactional.rs Show resolved Hide resolved
@vstam1
Copy link
Contributor Author

vstam1 commented Aug 7, 2023

bot rebase

@paritytech-processbot
Copy link

Rebased

@@ -314,6 +336,7 @@ impl<Config: config::Config> XcmExecutor<Config> {
for (i, instr) in xcm.0.into_iter().enumerate() {
match &mut result {
r @ Ok(()) => {
let vm_clone = self.clone();
Copy link
Member

Choose a reason for hiding this comment

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

This is a bit dangerous and makes correct weighing rather difficult.

@paritytech-ci paritytech-ci requested review from a team August 10, 2023 15:52
Copy link
Member

@gavofyork gavofyork left a comment

Choose a reason for hiding this comment

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

Not that many instructions make sense to make transactional. In fact the need for transactional processing is limited to just a few. I would suggest we instead target transactionalism to those instructions and contain potentially costly state-cloning and transactional overhead to just the few instructions which actually need it.

For what it's worth, I already wrote this in the issue which this PR references but it seems to have been disregarded.

When rewriting this PR, do ensure there are no unnecessary self.clone()s. In particular cloning the Error Handler and Appendix is totally unnecessary since the only instructions which alter them are infallible after the point they get altered.

Also pay close attention to the whether a particular instruction's implementation actually needs transactionalism. It's entirely possible that some of the instructions detailed in the issue are infallible after the point any state changes are made. The only time we need transactive storage is when an instruction can fail after (possibly) making changes in storage.

@paritytech-ci paritytech-ci requested review from a team August 10, 2023 15:54
@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable
Logs: https://gitlab.parity.io/parity/mirrors/polkadot/-/jobs/3435238

@vstam1
Copy link
Contributor Author

vstam1 commented Aug 24, 2023

@gavofyork I updated the PR to only update the necessary registers. I also checks the instructions for infallibility after state changes.
I am wondering if we should rollback the holding/trader registers for the BuyExecution instruction. Aren't we allowing free execution if we enable rollbacks on BuyExecution?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A3-in_progress Pull request is in progress. No review needed at this stage. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited.
Projects
Development

Successfully merging this pull request may close these issues.

XCM: Transactional Processing
8 participants