Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add try_state and integrity_test to XCM simulator fuzzer #3222

Merged

Conversation

louismerlin
Copy link
Contributor

@louismerlin louismerlin commented Feb 6, 2024

This adds try_state() and integrity_test() to the four runtimes of the XCM-simulator fuzzer.

With this, we are able to stress-test message-queue's try_state.

This also adds the Transact block-listing from #2424 to avoid false-positives.

Thank you @ggwpez for the help with the runtime configurations.

@louismerlin louismerlin requested a review from a team as a code owner February 6, 2024 10:04
TransferReserveAsset { xcm, .. } |
SetErrorHandler(xcm) |
SetAppendix(xcm) => Vec::from(xcm.inner()).iter_mut().flat_map(matches_recursive).collect(),
_ => vec![message.clone()],
Copy link
Member

@ggwpez ggwpez Feb 6, 2024

Choose a reason for hiding this comment

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

Hm, this function is called matches_recursive, but it dies not call the matches_blocklisted_messages?
To me it looks like it unpacks them.
You can also probably do both functions in one and just add a Transact { } here, then there are no vector instructions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be addressed

Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

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

Thanks! Left some nits, and then afterwards please look for the CI.

polkadot/xcm/xcm-simulator/fuzzer/src/fuzz.rs Show resolved Hide resolved
// runtime in substitute for the on-chain Wasm runtime unless all of `spec_name`,
// `spec_version`, and `authoring_version` are the same between Wasm and native.
// This value is set to 100 to notify Polkadot-JS App (https://polkadot.js.org/apps) to use
// the compatible custom types.
Copy link
Member

Choose a reason for hiding this comment

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

I think you can delete these comments 😆 Maybe there is also a default or a mock for testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@ggwpez ggwpez added the T10-tests This PR/Issue is related to tests. label Feb 6, 2024
@ggwpez
Copy link
Member

ggwpez commented Feb 6, 2024

It looks like the cargo test does not work on MacOS, probably because of how the fuzzer works. Maybe need to mark to only build for linux.

@louismerlin louismerlin added the R0-silent Changes should not be mentioned in any release notes label Feb 6, 2024
polkadot/xcm/xcm-simulator/fuzzer/src/fuzz.rs Outdated Show resolved Hide resolved

#[sp_version::runtime_version]
pub const VERSION: RuntimeVersion = RuntimeVersion {
spec_name: create_runtime_str!("node-template"),
Copy link
Contributor

Choose a reason for hiding this comment

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

this doesn't sound like the right name to me.

Suggested change
spec_name: create_runtime_str!("node-template"),
spec_name: create_runtime_str!("xcm-simulator-node"),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was removed in 3281a1f.

@@ -259,3 +265,53 @@ construct_runtime!(
MessageQueue: pallet_message_queue,
}
);

#[sp_version::runtime_version]
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see anywhere else in this PR that this runtime apis are needed. It seems like you just want to call into the methods exposed from Executive. Are you sure you need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was removed in 3281a1f.

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.

I might be wrong because I don't know the context, but I think the impl_runtime_apis is not needed.

@louismerlin
Copy link
Contributor Author

I might be wrong because I don't know the context, but I think the impl_runtime_apis is not needed.

Just verified with coverage, and the fuzzer is able to access try_runtime without impl_runtime_apis. Thank you very much for catching this!

@paritytech-cicd-pr
Copy link

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

@louismerlin louismerlin added this pull request to the merge queue Feb 8, 2024
Merged via the queue into paritytech:master with commit 84d89e3 Feb 8, 2024
125 of 126 checks passed
@louismerlin louismerlin deleted the xcm-simulator-fuzzer-try-runtime branch February 8, 2024 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
R0-silent Changes should not be mentioned in any release notes T10-tests This PR/Issue is related to tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants