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

Comments

Harden XCM v1 for Recursions#3586

Merged
42 commits merged intomasterfrom
shawntabrizi-harden-xcm-master
Aug 7, 2021
Merged

Harden XCM v1 for Recursions#3586
42 commits merged intomasterfrom
shawntabrizi-harden-xcm-master

Conversation

@shawntabrizi
Copy link
Member

Taking: #3555

And making it work for XCM v1

KiChjang and others added 30 commits August 2, 2021 07:25
Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
Co-authored-by: Gavin Wood <gavin@parity.io>
@shawntabrizi shawntabrizi added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. C1-low PR touches the given topic and has a low impact on builders. labels Aug 6, 2021
pub type LocalOriginToLocation = xcm_builder::SignedToAccountId32<Origin, AccountId, AnyNetwork>;

impl pallet_xcm::Config for Runtime {
// The config types here are entirely configurable, since the only one that is sorely needed
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// The config types here are entirely configurable, since the only one that is sorely needed
// The config types here are entirely configurable, since the only one that is surely needed

?

}
}

pub type Barrier = AllowUnpaidExecutionFrom<All<MultiLocation>>;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is better than struct YesItShould that I put for benchmarking mock, perhaps use this there as well: https://github.com/paritytech/polkadot/pull/3563/files#diff-4956fec0e6b5a0cf3ad1a1980b05285ac79c5a91d5edb15863d19b1519e8ae40R21


block_builder.push_polkadot_extrinsic(execute).expect("pushes extrinsic");

let block = block_builder.build().expect("Finalizes the block").block;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let block = block_builder.build().expect("Finalizes the block").block;
let block = block_builder.build().expect("finalizes the block").block;

futures::executor::block_on(client.import(sp_consensus::BlockOrigin::Own, block))
.expect("imports the block");

client
Copy link
Contributor

Choose a reason for hiding this comment

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

why so complicated? why not just use the runtime primitives to test this?

Copy link
Contributor

Choose a reason for hiding this comment

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

You can test the decode limit (which is checked at client-runtime boundary) with manual encode/decode. Also the execution limit can be checked here as well via the xcm-executor.

I recall that it had something to do with a strange rustc issue. Now's is a good time to rethink why we're doing it this way.

Copy link
Contributor

Choose a reason for hiding this comment

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

We had to run the test in WASM in order to simulate a real-ish environment. Local testing with a native client would blow the stack with a depth of just 6, while on WASM, it began to OOM at around 84.

Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

Looks correct to me. Not sure why the tests need to be so verbose.

@gavofyork gavofyork mentioned this pull request Aug 7, 2021
47 tasks
@KiChjang
Copy link
Contributor

KiChjang commented Aug 7, 2021

bot merge

@ghost
Copy link

ghost commented Aug 7, 2021

Trying merge.

@ghost ghost merged commit 659911f into master Aug 7, 2021
@ghost ghost deleted the shawntabrizi-harden-xcm-master branch August 7, 2021 19:29
@chevdor chevdor added this to the v0.9.10 milestone Sep 2, 2021
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A0-please_review Pull request needs code review. 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. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants