move toolchain: fix multiple issues with bytecode dependencies #16523

kklas · 2024-03-04T12:29:48Z

Description

This PR fixes multiple issues caused by bytecode dependencies not being included in CompiledPackage:

multiple functionalities failing due to topo sort in Modules panicking on missing dependencies
running move test on packages with bytecode deps failing due to them not being included in VM storage
source verification failing because bytecode deps aren't handled
commands such as publish and upgrade failing due to bytecode deps not being referenced in the construction of transaction blocks

Summary of changes:

removed move_bytecode_utils::dependency_graph module and instead added a compute_topological_order method to Modules. Replaced all calls to compute_dependency_graph with a direct call to compute_topological_order (2 in total)
added bytecode deps to VM storage for test runs by loading them from ResolvedGraph
include bytecode deps in sui_move_build::CompiledPackage and fix various module fetching methods to return modules from bytecode deps also
include bytecode deps in local_modules function to fix LocalDependencyNotFound errors in source verification

This is part of the work to enable compiling against on-chain dependencies #14178.

cc @rvantonder @amnn

Test Plan

Added unit tests for move test and source verification.

If your changes are not user-facing and do not break anything, you can skip the following section. Otherwise, please briefly describe what has changed under the Release Notes section.

Type of Change (Check all that apply)

protocol change
user-visible impact
breaking change for a client SDKs
breaking change for FNs (FN binary must upgrade)
breaking change for validators or node operators (must upgrade binaries)
breaking change for on-chain data layout
necessitate either a data wipe or data migration

Release notes

vercel · 2024-03-04T12:29:57Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
sui-core	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Sep 11, 2024 0:56am
sui-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Sep 11, 2024 0:56am

3 Skipped Deployments

Name	Status	Preview	Updated (UTC)
multisig-toolkit	⬜️ Ignored (Inspect)	Visit Preview	Sep 11, 2024 0:56am
sui-kiosk	⬜️ Ignored (Inspect)	Visit Preview	Sep 11, 2024 0:56am
sui-typescript-docs	⬜️ Ignored (Inspect)	Visit Preview	Sep 11, 2024 0:56am

crates/sui-move-build/src/lib.rs

rvantonder · 2024-03-04T19:05:24Z

Thanks @kklas! Kicking off CI so long, review to follow!

tnowacki

A few comments but going to tag in @tzakian and @cgswords here too.
Overall, I just want to make sure this feels like something core to the package system and not bolted on top

crates/sui-move-build/src/lib.rs

tnowacki · 2024-03-11T04:21:13Z

crates/sui-move-build/src/lib.rs

+ // collect bytecode dependencies as these are not returned as part of core
+ // `CompiledPackage`


I feel like they should be? If this is an issue

Though needing the full deserialized bytes feels maybe like a Sui specific thing? Unsure

I'm not nearly as familiar with this codebase as any of you are, but I'm also unsure. Though here the only reason I've added these here is because of source verification (to avoid having to hackily load them somehow in the source verification implementation), which is a Sui specific thing.So perhaps it's justifiable keeping them here unless there's another reason to have them in core CompiledPackage?

external-crates/move/crates/move-bytecode-utils/src/lib.rs

external-crates/move/crates/move-unit-test/src/lib.rs

external-crates/move/crates/move-cli/tests/move_unit_tests/bytecode_deps/Move.toml

kklas · 2024-03-13T22:06:31Z

@tnowacki agree with everything. I'm waiting for other comments to do round 2 here.

rvantonder

I am happy to see a move-bytecode-utils file get nuked 😄

See inline comments!

rvantonder · 2024-03-14T22:41:56Z

external-crates/move/crates/move-bytecode-utils/src/lib.rs

+ }
+
+ match petgraph::algo::toposort(&graph, None) {
+ Err(_) => panic!("Circular dependency detected"),


Since we have a Result return type, think it makes sense to trickle this up in stead of panicking? anyhow::bail? The match might also simplify?

It was like that in the old code too. My impression is that you should never be able to end up with a circular dependency on what is essentially a CompiledModule list. Because otherwise these modules wouldn't even compile. So it's not like a user error here but some serious bug somewhere else so a panic seems fine here.

external-crates/move/crates/move-unit-test/src/lib.rs

crates/sui-move-build/src/lib.rs

github-actions · 2024-05-15T01:50:07Z

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days.

kklas · 2024-09-02T08:46:50Z

I apologize for this unreasonably long delay. I have restarted the work on this:

rebase
move bytecode_deps_modules inside TestRunner
use IndexMap in compute_topological_order

@amnn @rvantonder @tnowacki I would appreciate another look

amnn

Thanks for this @kklas ! This looks reasonable to me -- let me give @tnowacki a couple of days into next week to take a look as well, (he's out at the moment), but then I'm happy to stamp.

crates/sui-source-validation/src/tests.rs

external-crates/move/crates/move-unit-test/src/lib.rs

amnn

Let's !

amnn · 2024-09-11T12:00:51Z

@kklas, my hand is hovering over "Squash and Merge" but let me know if you want to do a final rebase, etc before I send it.

…mplementation

kklas · 2024-09-11T12:52:24Z

@amnn rebase done, all good to go!

amnn · 2024-09-11T13:59:10Z

It's in, 🚀 , thanks very much @kklas!

@rvantonder

## Description This PR fixes multiple issues caused by bytecode dependencies not being included in `CompiledPackage`: - multiple functionalities failing due to topo sort in `Modules` panicking on missing dependencies - running `move test` on packages with bytecode deps failing due to them not being included in VM storage - source verification failing because bytecode deps aren't handled - commands such as publish and upgrade failing due to bytecode deps not being referenced in the construction of transaction blocks Summary of changes: - removed `move_bytecode_utils::dependency_graph` module and instead added a `compute_topological_order` method to `Modules`. Replaced all calls to `compute_dependency_graph` with a direct call to `compute_topological_order` (2 in total) - added bytecode deps to VM storage for test runs by loading them from `ResolvedGraph` - include bytecode deps in `sui_move_build::CompiledPackage` and fix various module fetching methods to return modules from bytecode deps also - include bytecode deps in `local_modules` function to fix `LocalDependencyNotFound` errors in source verification This is part of the work to enable compiling against on-chain dependencies #14178. cc @rvantonder @amnn ## Test Plan Added unit tests for `move test` and source verification. --- If your changes are not user-facing and do not break anything, you can skip the following section. Otherwise, please briefly describe what has changed under the Release Notes section. ### Type of Change (Check all that apply) - [ ] protocol change - [ ] user-visible impact - [ ] breaking change for a client SDKs - [ ] breaking change for FNs (FN binary must upgrade) - [ ] breaking change for validators or node operators (must upgrade binaries) - [ ] breaking change for on-chain data layout - [ ] necessitate either a data wipe or data migration ### Release notes

vercel bot deployed to Preview – sui-typescript-docs March 4, 2024 12:30 View deployment

vercel bot deployed to Preview – sui-core March 4, 2024 12:31 View deployment

kklas commented Mar 4, 2024

View reviewed changes

crates/sui-move-build/src/lib.rs Show resolved Hide resolved

rvantonder requested review from amnn, rvantonder and tnowacki March 4, 2024 19:04

vercel bot deployed to Preview – sui-typescript-docs March 5, 2024 12:20 View deployment

vercel bot deployed to Preview – sui-core March 5, 2024 12:23 View deployment

tnowacki reviewed Mar 11, 2024

View reviewed changes

tnowacki requested review from cgswords and tzakian March 11, 2024 04:28

rvantonder reviewed Mar 14, 2024

View reviewed changes

github-actions bot added the Stale label May 15, 2024

kklas force-pushed the bytecode-deps-fixes branch from 82365f6 to 8dc33d3 Compare September 2, 2024 08:39

vercel bot deployed to Preview – sui-docs September 2, 2024 08:44 View deployment

vercel bot deployed to Preview – sui-docs September 5, 2024 18:44 View deployment

amnn reviewed Sep 6, 2024

View reviewed changes

vercel bot deployed to Preview – sui-docs September 9, 2024 10:12 View deployment

amnn approved these changes Sep 11, 2024

View reviewed changes

kklas added 5 commits September 11, 2024 12:48

move-cli: add test for running tests on packages with bytecode deps

075327d

move-bytecode-util: add compute_tobological_order method to Modules

6bac83e

move-cli: add bytecode deps modules to VM storage

8c8c3a7

sui-source-validation: add a test for bytecode deps

99ef0c1

sui-move-build: support bytecode deps in CompiledPackage

9e5416e

kklas added 9 commits September 11, 2024 12:48

sui-source-validation: support validation for bytecode deps

7303a88

return topo sort in correct order

175d9a0

fix sui-source-validation: add a test for bytecode deps

cf090bb

move bytecode_deps_modules field inside TestRunner

db49dab

add newline to bytecode_deps test fixture manifest

85e6dda

use IndexMap instead of two HashMaps in compute_topological_order i…

8991845

…mplementation

fix clippy

cb8e9fa

fix development_mode_not_allowed test

6abc216

simplify successful_verification_with_bytecode_dep test implementation

373355a

kklas force-pushed the bytecode-deps-fixes branch from 6d60803 to 373355a Compare September 11, 2024 12:51

vercel bot deployed to Preview – sui-docs September 11, 2024 12:56 View deployment

amnn merged commit 9a275a4 into MystenLabs:main Sep 11, 2024
45 checks passed

kklas deleted the bytecode-deps-fixes branch September 16, 2024 06:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

move toolchain: fix multiple issues with bytecode dependencies #16523

move toolchain: fix multiple issues with bytecode dependencies #16523

kklas commented Mar 4, 2024 •

edited

Loading

vercel bot commented Mar 4, 2024 •

edited

Loading

rvantonder commented Mar 4, 2024

tnowacki left a comment

tnowacki Mar 11, 2024

tnowacki Mar 11, 2024

kklas Sep 2, 2024

kklas commented Mar 13, 2024

rvantonder left a comment

rvantonder Mar 14, 2024

kklas Mar 14, 2024

github-actions bot commented May 15, 2024

kklas commented Sep 2, 2024

amnn left a comment

amnn left a comment

amnn commented Sep 11, 2024

kklas commented Sep 11, 2024

amnn commented Sep 11, 2024

		// collect bytecode dependencies as these are not returned as part of core
		// `CompiledPackage`

move toolchain: fix multiple issues with bytecode dependencies #16523

move toolchain: fix multiple issues with bytecode dependencies #16523

Conversation

kklas commented Mar 4, 2024 • edited Loading

Description

Test Plan

Type of Change (Check all that apply)

Release notes

vercel bot commented Mar 4, 2024 • edited Loading

rvantonder commented Mar 4, 2024

tnowacki left a comment

Choose a reason for hiding this comment

tnowacki Mar 11, 2024

Choose a reason for hiding this comment

tnowacki Mar 11, 2024

Choose a reason for hiding this comment

kklas Sep 2, 2024

Choose a reason for hiding this comment

kklas commented Mar 13, 2024

rvantonder left a comment

Choose a reason for hiding this comment

rvantonder Mar 14, 2024

Choose a reason for hiding this comment

kklas Mar 14, 2024

Choose a reason for hiding this comment

github-actions bot commented May 15, 2024

kklas commented Sep 2, 2024

amnn left a comment

Choose a reason for hiding this comment

amnn left a comment

Choose a reason for hiding this comment

amnn commented Sep 11, 2024

kklas commented Sep 11, 2024

amnn commented Sep 11, 2024

kklas commented Mar 4, 2024 •

edited

Loading

vercel bot commented Mar 4, 2024 •

edited

Loading