Enable Miri in CI

[KaiserKarel]

This PR does not fix the possible UB in storage (#850), but does run miri in CI and disables tests using/depending on asm!, which is not supported by Miri.

[HCastano]

I don't think we should make failing this check a hard error for the CI (yet), can you allow build failures?

[HCastano]

Why's this behind a not(miri) flag?

[KaiserKarel]

These examples call into functions using asm! and thus do not compile under Miri

[HCastano]

Ah, yeah you mentioned that in the PR description, but I didn't see any asm! blocks. A short comment explaining that in the code would be useful imo

[KaiserKarel]

I don't think we should make failing this check a hard error for the CI (yet), can you allow build failures?

I can do that, however I am hesitant. I haven't looked into the actual failure yet, but UB inside storage may mean a severe risk to contracts.

[HCastano]

I can do that, however I am hesitant. I haven't looked into the actual failure yet, but UB inside storage may mean a severe risk to contracts.

Sure, but I say this for two reasons.

1. The checks are still experimental

If we look at the CI logs we'll see the following message:

= help: this indicates a potential bug in the program: it performed an invalid operation, but the rules it violated are still experimental

I don't think it's quite "fair" to block a developer's PR on a check that's experimental.

That said, I think there are compile time flags we can use to ignore some of these experimental checks, so that might be one approach to use here.

2. Allows us to fix potential UB in follow up PRs

If we make it a hard check we'll have to fix the potential issues in this PR. I'd prefer to fix them in follow ups so that the review area is smaller and we can ensure that the fixes are indeed correct.

[KaiserKarel]

I don't think it's quite "fair" to block a developer's PR on a check that's experimental.

I agree with that. I could enable those compile time flags, or currently annotate the failing test with an ignore attribute and create a related issue; that way we'll still have these checks on PR, but if a developer submits something which is correct, but unsupported by Miri, they can also annotate it with a comment.

Of course allowing failures there is possible too; but in my experience once you have a check that regularly fails, reviewers (including me) stop paying attention to it.

[HCastano]

Of course allowing failures there is possible too; but in my experience once you have a check that regularly fails, reviewers (including me) stop paying attention to it.

Yeah, I totally agree here. We should definitely avoid having a check which is always failing.

I agree with that. I could enable those compile time flags, or currently annotate the failing test with an ignore attribute and create a related issue; that way we'll still have these checks on PR, but if a developer submits something which is correct, but unsupported by Miri, they can also annotate it with a comment.

I've been looking through the Miri README and it mentions that despite the stacked borrow rule being experimental, disabling the check is "unsound". Playing it safe then, let's go with your second suggestion and ignore things for now, but ensure that issues are created so we can track them and address them in follow up PRs.

[KaiserKarel]

Miri seems to error at the end of the test run during some cleanup routine. I'm unsure where to look to fix this, or if that is even the cause.

Gotcha I think rust-lang/miri#1360 ;)

[KaiserKarel]

Cheered too soon. Still unsure why the thread_local panics.

[HCastano]

@KaiserKarel Hey, do you need any help with this, or have you just not had time to get around to looking at this again?

[KaiserKarel]

@KaiserKarel Hey, do you need any help with this, or have you just not had time to get around to looking at this again?

Been a bit busy, but could use some help here as well. The thread_local panics at the end of the test run, not during the tests.

[HCastano]

@KaiserKarel Hey, do you need any help with this, or have you just not had time to get around to looking at this again?

Been a bit busy, but could use some help here as well. The thread_local panics at the end of the test run, not during the tests.

I haven't had time to look at this either. I'll try (🤞) to make some time this week to help

[HCastano]

I'm a bit stumped about this too actually.

At first I thought it could be the ink_storage doctests causing the problem, since from the miri output those aren't run. However, removing them didn't change anything.

I find it curious that all 320 storage tests pass (well, 315 pass and 5 are ignored, which we expect) and then the failure happens 🤔

[KaiserKarel]

I'm a bit stumped about this too actually.

At first I thought it could be the ink_storage doctests causing the problem, since from the miri output those aren't run. However, removing them didn't change anything.

I find it curious that all 320 storage tests pass (well, 315 pass and 5 are ignored, which we expect) and then the failure happens

I think it happens in the Drop of the thread_local, which causes the env to drop with the associated Engine.

[HCastano]

I think it happens in the Drop of the thread_local, which causes the env to drop with the associated Engine.

Yeah, I agree with you here.

I'll go a step further and say that I'm pretty sure it happens when we Drop the DynamicAllocator. I found that I can reproduce this by doing: cargo miri test -p ink_storage alloc (although nothing more specific than that unfortunately).

[KaiserKarel]

So the docs for LocalKey::with state: "(panics) If the key has been destroyed (which may happen if this is called in a destructor)". Perhaps we are hitting that case? Although abort is called not on the AccessError being returned.

[KaiserKarel]

cargo miri test -- --Z unstable-options --exclude-should-panic still triggers the abort.

[Robbepop]

What is the state of this PR? It has been stale for quite a while now.

[HCastano]

I wasn't able to figure out how to get this working, but I agree that it would be nice to include this as part of the CI.

You're more knowledgeable around unsafe and Miri, maybe you can take a look @Robbepop 😅

[Robbepop]

I wasn't able to figure out how to get this working, but I agree that it would be nice to include this as part of the CI.

You're more knowledgeable around unsafe and Miri, maybe you can take a look @Robbepop sweat_smile

Generally the PR looks alright but I am concerned about the failing miri CI job.

[KaiserKarel]

I can't figure out what the panic at the end of the test run is. I think it is not UB; however we shouldn't ignore of course. Seems to be caused during test teardown?

[HCastano]

@KaiserKarel This would be nice to have, but tbh I was stumped back then and nobody on the team has time to circle back to this. Also, with #852 being closed this isn't strictly required anymore.

Let us know if you disagree with this, and thanks again for your time!

[Robbepop]

The main problem with miri in our CI is that is is super slow unless we disable most tests which makes this less useful.
For example none of our extensive UI tests can be run with miri and most of our examples cannot be run either - any example that uses more elaborate off-chain features to be precise.