"[move] Enabled Sui bytecode verification for test code"

[awelc]

trafficstars

This resolves https://github.com/MystenLabs/sui/issues/5712 which was a problem reported on Discord. The issue was that Sui bytecode verifier would not detect a problem with the struct having the key ability but no UID field in the following code (which is what this PR fixes):

module verifier_test::test {
    #[test_only]
    struct NotObject has key {f: u64}

    #[test]
    fun bad_share() {
        sui::transfer::share_object(NotObject{f: 42});
    }
}

In order to address the issue, we need to enable test_mode option in MoveBuildConfig when running sui move build/test commands. After this is done, in order for the Sui framework and Move stdlib version validation check to succeed, we need a version of Sui framework and Move stdlib that also includes tests - otherwise comparing code bundled with Sui binary and code pulled as dependencies of the user's code will always fail.

Enabling test_mode option in MoveBuildConfig when running sui move build/test commands means that our own test code (in the framework and otherwise) will be subject to Sui bytecode verification, hence some changes that were required in the code that previously violated Sui bytecode verification invariants. . The test_scenario package is the one that does a bit of magic to emulate transactional execution and thus, I believe, it's not that easy to make it pass bytecode verification. For example, freezing object of the generic type is illegal (though I believe necessary) in the following test_scenario function:

    public fun return_immutable<T: key>(t: T) {
        let id = object::id(&t);
        assert!(was_taken_immutable(id), ECantReturnObject);
        sui::transfer::freeze_object(t)
    }

If we do not exclude test_scenario module from verification, trying to build the above code will result in the following error:

error: failed to run custom build command for `sui-framework v0.1.0 (/Users/adamwelc/sui/crates/sui-framework)`

Caused by:
  process didn't exit successfully: `/Users/adamwelc/sui/target/debug/build/sui-framework-8a61be598d43b21e/build-script-build` (exit status: 101)
  --- stderr
  thread '<unnamed>' panicked at 'called `Result::unwrap()` on an `Err` value: ExecutionError("ExecutionError: ExecutionError { inner: ExecutionErrorInner { kind: SuiMoveVerificationError, source: Some(\"0000000000000000000000000000000000000002::test_scenario::return_immutable. Invalid call to '0000000000000000000000000000000000000002::transfer::freeze_object' on an object of type 'T0'. The transferred object's type must be defined in the current module, or must have the 'store' type ability\") } }")', crates/sui-framework/build.rs:90:6
  note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
  thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Any { .. }', crates/sui-framework/build.rs:29:10

I will leave it up to @tnowacki to decide if excluding this particular (test-only!) module from verification is acceptable.

[tnowacki]

The test_scenario package is the one that does a bit of magic to emulate transactional execution and thus, I believe, it's not that easy to make it pass bytecode verification. For example, freezing object of the generic type is illegal (though I believe necessary) in the following test_scenario function:

I do not understand this comment. As long as we can ensure that key structs have the first field being id: UID, we should be fine. Could you expand on the issue you see?

[awelc]

I don't understand setting test_mode to true by default, and I think it's going to lead to a lot of unintended consequences.

It indeed leads to a lot of unintended consequences. I tried to explain why this is necessary in the PR description but clearly did not do a good job out of it so let me try again.

Say we set test_mode to true only for the sui move build/test command. Then the bytecode compiled for the user code, including Sui framework and Move stdlib will include test code.

If we only do the above, then the Sui framework code and Move stdlib code bundled with Sui binary will NOT include the test code.

The way we currently detect a version mismatch between a version of Sui framework (and Move stdlib) bundled with Sui binary and a version pulled as a dependency by user code is by comparing bytecode for both these versions. If one includes test code and the other does not, we will get version mismatch error each time.

One way to solve this is to include test code in both versions, as I don't think we can filter "test bytecode" once all code is generated. It does have unintended consequences, though, more than I initially anticipated.

Another tack could be to bundle two versions of the Sui framework and Move stdlib code with Sui binary, one with and one without test code built in, and use the appropriate one based on whether user code is built with or without test_only flag set to true.

[tnowacki]

Maybe we could do something like

let sui_pkg = (... somehow fetch this from the pkg config ...).build();
sui_pkg.verify_framework_version(get_sui_framework(), get_move_stdlib())?;
build_config.test_mode = true;
let _pkg = config.build(path.to_path_buf())?;
sui_framework::run_move_unit_tests(...)