Move flat-storage-reads/fix-contract-loading-cost/implicit-accounts features to runtime configuration #9364

nagisa · 2023-07-27T14:51:47Z

This PR is built on top of a previous PR (see the base branch). Here the noted protocol features have been replaced with runtime configuration via parameters instead.

This would be one solution/option to getting rid of compile-time features in contract runtime which is interfering with limited replayability (compile-time feature control means that all of the crates still need to be built as a single compilation unit with consistent options.)

cc @jakmeier

cc #8197

jakmeier

Lots of changes in tests... Didn't expect it to be that invasive but I'm also not surprised.

But the approach makes sense to me and I would argue it results in cleaner code than we have today. Happy to discuss alternatives though, if you think it's worth doing.

jakmeier · 2023-07-27T14:59:51Z

core/primitives/src/runtime/config_store.rs

+    #[test]
+    fn all_configs_are_specified() {


ah yes we should have had such a test from the beginning 👍

jakmeier · 2023-07-27T16:41:13Z

runtime/near-vm-runner/src/tests/runtime_errors.rs

@@ -445,7 +426,7 @@ fn test_stack_instrumentation_protocol_upgrade() {
        .opaque_error()
        .expects(&[
            expect![[r#"
-                VMOutcome: balance 4 storage_usage 12 return data None burnt gas 6789985365 used gas 6789985365
+                VMOutcome: balance 4 storage_usage 12 return data None burnt gas 18136872021 used gas 18136872021


Hm, why would the gas values change here? Is there an issue with the new way of keeping versions? (Which I wonder, what's the reason for changing it?)

I believe this change is actually indicative of the previous way not correctly using the old stack limiting mechanism. You will notice that previously the gas was exactly the same before (the first expect! and after (the 2nd expect!) the ProtocolFeature::CorrectStackLimit feature, which makes no sense given the nature of this test. That said, I haven’t looked too deeply into why this exact test was behaving the way it did before…

Ok, that's a reasonable explanation. :)

Maybe for final PR, it would make sense to do the refactor of the TestBuilder in a separate PR to make it obvious it has nothing to do with code changes outside the testing framework.

We made a single letter mistake and passed values larger than `i32::MAX` to a function that did only supports `0..=i32::MAX`. This fix corrects the problem, introduces the necessary protocol versioning, a regression test and an additional assert to validate the inputs in the aforementioned function. I ended up using the parameters for the behaviour flag in anticipation that I wouldn’t need to do this later as part of #9364.

This is a no-op change that's been split out from #9364. In principle this is not a strict improvement in isolation since the functions now get access to more information than they strictly need to work with, but in practice this should make our lives easier as now we won’t need to redo this work if we want to make some fees dependent on some configuration option :)

jakmeier

Looks great!

core/primitives-core/src/version.rs

This is a no-op change that's been split out from #9364. In principle this is not a strict improvement in isolation since the functions now get access to more information than they strictly need to work with, but in practice this should make our lives easier as now we won’t need to redo this work if we want to make some fees dependent on some configuration option :)

Here the noted protocol features have been replaced with runtime configuration via parameters instead. This would be one solution/option to getting rid of compile-time features in contract runtime which is interfering with limited replayability (compile-time feature control means that all of the crates still need to be built as a single compilation unit with consistent options.) cc #8197

We made a single letter mistake and passed values larger than `i32::MAX` to a function that did only supports `0..=i32::MAX`. This fix corrects the problem, introduces the necessary protocol versioning, a regression test and an additional assert to validate the inputs in the aforementioned function. I ended up using the parameters for the behaviour flag in anticipation that I wouldn’t need to do this later as part of #9364.

We made a single letter mistake and passed values larger than `i32::MAX` to a function that did only supports `0..=i32::MAX`. This fix corrects the problem, introduces the necessary protocol versioning, a regression test and an additional assert to validate the inputs in the aforementioned function. I ended up using the parameters for the behaviour flag in anticipation that I wouldn’t need to do this later as part of near#9364.

This is a no-op change that's been split out from near#9364. In principle this is not a strict improvement in isolation since the functions now get access to more information than they strictly need to work with, but in practice this should make our lives easier as now we won’t need to redo this work if we want to make some fees dependent on some configuration option :)

@jakmeier

…eatures to runtime configuration (near#9364) This PR is built on top of a previous PR (see the base branch). Here the noted protocol features have been replaced with runtime configuration via parameters instead. This would be one solution/option to getting rid of compile-time features in contract runtime which is interfering with limited replayability (compile-time feature control means that all of the crates still need to be built as a single compilation unit with consistent options.) cc @jakmeier cc near#8197

This is a no-op change that's been split out from #9364. In principle this is not a strict improvement in isolation since the functions now get access to more information than they strictly need to work with, but in practice this should make our lives easier as now we won’t need to redo this work if we want to make some fees dependent on some configuration option :)

@jakmeier

…eatures to runtime configuration (#9364) This PR is built on top of a previous PR (see the base branch). Here the noted protocol features have been replaced with runtime configuration via parameters instead. This would be one solution/option to getting rid of compile-time features in contract runtime which is interfering with limited replayability (compile-time feature control means that all of the crates still need to be built as a single compilation unit with consistent options.) cc @jakmeier cc #8197

nagisa requested a review from a team as a code owner July 27, 2023 14:51

nagisa requested review from akhi3030 and removed request for a team July 27, 2023 14:51

nagisa marked this pull request as draft July 27, 2023 14:51

nagisa removed the request for review from akhi3030 July 27, 2023 14:52

jakmeier reviewed Jul 27, 2023

View reviewed changes

nagisa force-pushed the nagisa/action-exorcism branch from 205e340 to c0513e8 Compare July 31, 2023 17:56

This was referenced Aug 3, 2023

[2023-08] Make near-vm-runner crate independent #9383

Closed

Fix #9393 #9395

Merged

nagisa mentioned this pull request Aug 9, 2023

Fix #9393 (#9395) – backport to 1.36.0 #9401

Closed

Base automatically changed from nagisa/action-exorcism to master August 10, 2023 09:09

nagisa force-pushed the nagisa/runtime-features-experiment branch 3 times, most recently from 3c3dc00 to 4d60111 Compare August 11, 2023 11:38

nagisa mentioned this pull request Aug 14, 2023

test_builder: more flexibility and control #9426

Merged

nagisa mentioned this pull request Aug 16, 2023

runtime: pass around the RuntimeConfig rather than just fees #9439

Merged

nagisa force-pushed the nagisa/runtime-features-experiment branch from 1c4b142 to b6f151f Compare August 16, 2023 10:32

nagisa changed the title ~~experiment: configure flat-storage-reads/fix-contract-loading-cost/implicit-accounts features to runtime configuration~~ Move flat-storage-reads/fix-contract-loading-cost/implicit-accounts features to runtime configuration Aug 16, 2023

nagisa changed the base branch from master to nagisa/runtime-config-over-fees August 16, 2023 10:33

nagisa marked this pull request as ready for review August 16, 2023 10:34

jakmeier approved these changes Aug 16, 2023

View reviewed changes

core/primitives-core/src/version.rs Outdated Show resolved Hide resolved

Base automatically changed from nagisa/runtime-config-over-fees to master August 18, 2023 13:44

nagisa added 3 commits August 18, 2023 16:45

runtime: remove unnecessary passing of the protocol version

b81a22b

Normalize formatting of ProtocolVersion enum

f545a34

nagisa force-pushed the nagisa/runtime-features-experiment branch from 135d52e to f545a34 Compare August 18, 2023 13:45

nagisa added the S-automerge label Aug 18, 2023

Merge refs/heads/master into nagisa/runtime-features-experiment

1e0004d

near-bulldozer bot merged commit e4bd885 into master Aug 18, 2023

near-bulldozer bot deleted the nagisa/runtime-features-experiment branch August 18, 2023 16:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move flat-storage-reads/fix-contract-loading-cost/implicit-accounts features to runtime configuration #9364

Move flat-storage-reads/fix-contract-loading-cost/implicit-accounts features to runtime configuration #9364

nagisa commented Jul 27, 2023 •

edited

Loading

jakmeier left a comment

jakmeier Jul 27, 2023

jakmeier Jul 27, 2023

nagisa Jul 28, 2023

jakmeier Jul 28, 2023

jakmeier left a comment

Move flat-storage-reads/fix-contract-loading-cost/implicit-accounts features to runtime configuration #9364

Move flat-storage-reads/fix-contract-loading-cost/implicit-accounts features to runtime configuration #9364

Conversation

nagisa commented Jul 27, 2023 • edited Loading

jakmeier left a comment

Choose a reason for hiding this comment

jakmeier Jul 27, 2023

Choose a reason for hiding this comment

jakmeier Jul 27, 2023

Choose a reason for hiding this comment

nagisa Jul 28, 2023

Choose a reason for hiding this comment

jakmeier Jul 28, 2023

Choose a reason for hiding this comment

jakmeier left a comment

Choose a reason for hiding this comment

nagisa commented Jul 27, 2023 •

edited

Loading