runtime: test that signext and saturating ops are disabled #8921
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
26e31da to
cdb5451
Compare
jakmeier
reviewed
Apr 18, 2023
Comment on lines
+404
to
+413
| #[cfg(feature = "nightly")] | ||
| tb.expect(expect![[r#" | ||
| VMOutcome: balance 4 storage_usage 12 return data None burnt gas 48450963 used gas 48450963 | ||
| Err: PrepareError: Error happened while deserializing the module. | ||
| "#]]); | ||
| #[cfg(not(feature = "nightly"))] | ||
| tb.expect(expect![[r#" | ||
| VMOutcome: balance 4 storage_usage 12 return data None burnt gas 0 used gas 0 | ||
| Err: PrepareError: Error happened while deserializing the module. | ||
| "#]]); |
There was a problem hiding this comment.
Not sure but I guess feature = "nightly" has a different gas cost because of ProtocolFeature::FixContractLoadingCost, right?
If that is the case, wouldn't it be better to use #[cfg(feature = "protocol_feature_fix_contract_loading_cost")] instead? Makes the reason why we have the flag self-descriptive.
There was a problem hiding this comment.
I sort of avoided looking too deeply into what exact reason for the difference is – when this change gets promoted to a proper feature (or is removed) the test will fail anyhow and the other branch then can be removed & I don't think it is beneficial to hold tests to the same level of rigour as we do for the regular kind of code in this instance.
There was a problem hiding this comment.
ok fair enough, I guess we can merge it then
jakmeier
approved these changes
Apr 20, 2023
Comment on lines
+404
to
+413
| #[cfg(feature = "nightly")] | ||
| tb.expect(expect![[r#" | ||
| VMOutcome: balance 4 storage_usage 12 return data None burnt gas 48450963 used gas 48450963 | ||
| Err: PrepareError: Error happened while deserializing the module. | ||
| "#]]); | ||
| #[cfg(not(feature = "nightly"))] | ||
| tb.expect(expect![[r#" | ||
| VMOutcome: balance 4 storage_usage 12 return data None burnt gas 0 used gas 0 | ||
| Err: PrepareError: Error happened while deserializing the module. | ||
| "#]]); |
There was a problem hiding this comment.
ok fair enough, I guess we can merge it then
These are not currently supported and any decision to enable them should be a intentional decision, not an accidental one.
Overall these changes are no-op since the deserialization will fail earlier when trying to deserialize with pwasm deserializer, but if and when we eventually get rid of it, it would be nice if these extensions didn't accidentally get enabled.
aborg-dev
approved these changes
Apr 20, 2023
(this required to rebase for me to reproduce locally… 🤦)
0d7ee05 to
b66db47
Compare
nikurt
pushed a commit
that referenced
this pull request
Apr 25, 2023
These are not currently supported and any decision to enable them should be a intentional decision, not an accidental one. Overall these changes are no-op since the deserialization will fail earlier when trying to deserialize with pwasm deserializer, but if and when we eventually get rid of it, it would be nice if these extensions didn't accidentally get enabled. This puts to rest the question raised in #8886 (comment).
nikurt
pushed a commit
that referenced
this pull request
Apr 28, 2023
These are not currently supported and any decision to enable them should be a intentional decision, not an accidental one. Overall these changes are no-op since the deserialization will fail earlier when trying to deserialize with pwasm deserializer, but if and when we eventually get rid of it, it would be nice if these extensions didn't accidentally get enabled. This puts to rest the question raised in #8886 (comment).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
These are not currently supported and any decision to enable them should
be a intentional decision, not an accidental one.
Overall these changes are no-op since the deserialization will fail
earlier when trying to deserialize with pwasm deserializer, but if and
when we eventually get rid of it, it would be nice if these extensions
didn't accidentally get enabled.
This puts to rest the question raised in #8886 (comment).