Validation: don't detect STDIN closing when running in process #1695

cecton · 2020-09-10T06:22:08Z

I had an issue in paritytech/cumulus#201 because the test was working locally but not on CI and I couldn't figure out why. I finally found out why: when running in a CI, the STDIN is actually closed. Because of that, the exit detection mechanism was triggered and the validation was never run.

This PR proposes to fix this by adding a flag that activates this exit mechanism. So when the validation is ran "in process", it will disable the flag and let the validation run indefinitely.

Related to #1622

arkpar · 2020-09-10T08:53:56Z

when running in a CI, the STDIN is actually closed. Because of that, the exit detection mechanism was triggered and the validation was never run.

It should not matter. The validation subprocess is spawned with StdIo::piped(). It does not inherit it from the parent process. So the worker should always have stdin opened. The problem is that now there might not be an actual worker process when this code is called.

I failed to spot when reviewing #1622 how ValidationExecutionMode::InProcess was implemented. I'd expect ValidationExecutionMode::InProcess to be the same as ExecutionMode::Local, I.e. to bypass validation host entirely and simply perform validation in the current thread without using any shared memory. I was actually suggesting merging ValidationExecutionMode and ExecutionMode into a single enum, which matches current version of ValidationExecutionMode.

bkchr

Yeah, I see it as @arkpar and I already told you this in Element. The thread does not even care about stdin.

bkchr · 2020-09-10T09:14:54Z

runtime/westend/src/lib.rs

@@ -84,7 +84,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
 	spec_name: create_runtime_str!("westend"),
 	impl_name: create_runtime_str!("parity-westend"),
 	authoring_version: 2,
-	spec_version: 43,
+	spec_version: 44,


Revert please

cecton · 2020-09-10T13:03:57Z

I failed to spot when reviewing #1622 how ValidationExecutionMode::InProcess was implemented. I'd expect ValidationExecutionMode::InProcess to be the same as ExecutionMode::Local, I.e. to bypass validation host entirely and simply perform validation in the current thread without using any shared memory. I was actually suggesting merging ValidationExecutionMode and ExecutionMode into a single enum, which matches current version of ValidationExecutionMode.

OK!! I get it now. Sorry I missed completely the point in the previous PR.

I just pushed something that is more like you said. This is what you had in mind? (but with the pool in the enum would be better XD)

(quick response would be appreciated)

cc @arkpar

cecton · 2020-09-10T16:55:11Z

@arkpar @bkchr ok this is ready for review

PLEASE READ CAREFULLY THANKS 🤣

ordian · 2020-09-10T20:28:59Z

parachain/src/wasm_executor/mod.rs

@@ -166,7 +184,7 @@ pub fn validate_candidate_internal(
 ) -> Result<ValidationResult, ValidationError> {
 	let executor = sc_executor::WasmExecutor::new(
 		#[cfg(not(any(target_os = "android", target_os = "unknown")))]
-		sc_executor::WasmExecutionMethod::Compiled,
+		sc_executor::WasmExecutionMethod::Interpreted,


may I ask why it was changed?

~~not sure how much of that code will be retained given #1609~~

I had compilation issue. Will revert it to see

In the previous backport I had to remove the feature wasmtime because the CI failed like this: https://gitlab.parity.io/parity/polkadot/-/jobs/635768

And now it's failing because of:

error[E0599]: no variant or associated item named `Compiled` found for enum `WasmExecutionMethod` in the current scope --> parachain/src/wasm_executor/mod.rs:187:37 | 187 | sc_executor::WasmExecutionMethod::Compiled, | ^^^^^^^^ variant or associated item not found in `WasmExecutionMethod` error: aborting due to previous error For more information about this error, try `rustc --explain E0599`. error: could not compile `polkadot-parachain`.

So I talked to @bkchr and apparently I can ignore the failing test for now

parachain/src/wasm_executor/mod.rs

parachain/src/wasm_executor/validation_host.rs

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

Forked at: e916423 Parent branch: origin/rococo-branch

…rocess (#1695) (#1703) * Initial commit Forked at: d04e449 Parent branch: origin/master * Validation: don't detect STDIN closing when running in process (#1695)

* master: Companion PR for #6984 (#1661) Update some dependencies. (#1718) Add a specific memory requirements (#1716) Companion PR for #7039: grandpa-rpc dont share subscription manager, only executor (#1687) Update bytes. (#1715) Add a note about memory requirements (#1714) Update parity-multiaddr. (#1700) typo in proxy tests (#1713) Companion PR for ` Add a `build-sync-spec` subcommand and remove the CHT roots from the light sync state.` (#1670) Forwardport: Validation: don't detect STDIN closing when running in process (#1695) (#1703)

cecton requested review from bkchr and arkpar September 10, 2020 06:22

github-actions bot added the A0-please_review Pull request needs code review. label Sep 10, 2020

paritytech-ci added the D2-breaksapi label Sep 10, 2020

cecton added C1-low PR touches the given topic and has a low impact on builders. B0-silent Changes should not be mentioned in any release notes labels Sep 10, 2020

bkchr reviewed Sep 10, 2020

View reviewed changes

Merge enums

66a8654

cecton force-pushed the cecton-fix-validation-close-with-stdin branch from 7a7dca9 to 66a8654 Compare September 10, 2020 12:57

cecton marked this pull request as draft September 10, 2020 12:57

cecton changed the base branch from master to rococo-branch September 10, 2020 12:58

cecton added 5 commits September 10, 2020 15:40

Move validation pool to execution mode

0cea17a

Done todos

f3620a5

Fix tests

6682600

fix for wasm

4efe84b

missing )

9197463

cecton marked this pull request as ready for review September 10, 2020 16:54

arkpar approved these changes Sep 10, 2020

View reviewed changes

ordian reviewed Sep 10, 2020

View reviewed changes

bkchr approved these changes Sep 10, 2020

View reviewed changes

parachain/src/wasm_executor/mod.rs Outdated Show resolved Hide resolved

parachain/src/wasm_executor/validation_host.rs Outdated Show resolved Hide resolved

cecton and others added 4 commits September 11, 2020 09:54

Update parachain/src/wasm_executor/mod.rs

a3cecf4

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

Update parachain/src/wasm_executor/validation_host.rs

23fcec3

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

WIP

737bee1

Forked at: e916423 Parent branch: origin/rococo-branch

Re-enable wasmtime

a6e238d

cecton merged commit 26f1fa4 into rococo-branch Sep 11, 2020

cecton deleted the cecton-fix-validation-close-with-stdin branch September 11, 2020 08:48

cecton added a commit that referenced this pull request Sep 11, 2020

Validation: don't detect STDIN closing when running in process (#1695)

7f8e86c

cecton mentioned this pull request Sep 11, 2020

Forwardport: Validation: don't detect STDIN closing when running in process (#1695) #1703

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validation: don't detect STDIN closing when running in process #1695

Validation: don't detect STDIN closing when running in process #1695

cecton commented Sep 10, 2020 •

edited

Loading

arkpar commented Sep 10, 2020

bkchr left a comment

bkchr Sep 10, 2020

cecton commented Sep 10, 2020 •

edited

Loading

cecton commented Sep 10, 2020 •

edited

Loading

ordian Sep 10, 2020 •

edited

Loading

cecton Sep 11, 2020

cecton Sep 11, 2020

Validation: don't detect STDIN closing when running in process #1695

Validation: don't detect STDIN closing when running in process #1695

Conversation

cecton commented Sep 10, 2020 • edited Loading

arkpar commented Sep 10, 2020

bkchr left a comment

Choose a reason for hiding this comment

bkchr Sep 10, 2020

Choose a reason for hiding this comment

cecton commented Sep 10, 2020 • edited Loading

cecton commented Sep 10, 2020 • edited Loading

ordian Sep 10, 2020 • edited Loading

Choose a reason for hiding this comment

cecton Sep 11, 2020

Choose a reason for hiding this comment

cecton Sep 11, 2020

Choose a reason for hiding this comment

cecton commented Sep 10, 2020 •

edited

Loading

cecton commented Sep 10, 2020 •

edited

Loading

cecton commented Sep 10, 2020 •

edited

Loading

ordian Sep 10, 2020 •

edited

Loading