Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[cli] New alg for tree shaking #21356

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

stefan-mysten
Copy link
Contributor

@stefan-mysten stefan-mysten commented Feb 26, 2025

Description

This PR updates the tree shaking algorithm to use linkage tables on-chain. We need to use that because implicit dependencies feature that is being worked in #21204 would not work with this algorithm. This is due to the way transitive dependencies are computed in the previous tree shaking algorithm. Implicit dependencies will add those system dependencies to the transitive dependencies, thus system packages will get included in the final list of dependencies in the package that is to be published/upgraded after tree shaking is complete.

Test plan

Existing tests. Updated a test for system packages.

cd crates/sui
cargo nextest run -- cli_tests

Release notes

Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.

For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.

  • Protocol:
  • Nodes (Validators and Full nodes):
  • gRPC:
  • JSON-RPC:
  • GraphQL:
  • CLI:
  • Rust SDK:

@stefan-mysten stefan-mysten self-assigned this Feb 26, 2025
Copy link

vercel bot commented Feb 26, 2025

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

Name Status Preview Comments Updated (UTC)
sui-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 4, 2025 1:33am
2 Skipped Deployments
Name Status Preview Comments Updated (UTC)
multisig-toolkit ⬜️ Ignored (Inspect) Visit Preview Mar 4, 2025 1:33am
sui-kiosk ⬜️ Ignored (Inspect) Visit Preview Mar 4, 2025 1:33am

Copy link
Contributor

@amnn amnn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes around sui move build look good to me, but the new algorithm does not seem quite right. I have suggested a fix and a test that would highlight the issue.

Comment on lines 3160 to 3202
compiled_package
.dependency_ids
.published
.retain(|pkg_name, id| {
linkage_table_ids.contains(id)
|| pkgs.contains_key(pkg_name)
|| pkg_name_to_orig_id
.get(pkg_name)
.is_some_and(|orig_id| pkg_ids.contains(orig_id))
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not seem right:

  • compiled_package.dependency_ids.published is a list of storage IDs (they point to the actual version of the package that you want to use)
  • similarly, pkg_ids is also a list of storage IDs, because it was generated by filtering the same list.
  • linkage_table_ids is a list of original IDs (they point to the first version of the package you want to use, regardless of its version), because that is what the key in the linkage table is.
  • values in pkg_name_to_orig_id are also original IDs.

In this code, we are freely comparing original IDs and storage IDs -- this should only work for packages that have never been upgraded -- have you tested this approach in cases involving upgraded packages? The following example should surface an issue if there is one:

  • B is published depending on C v1
  • A is published depending on B and C v2, but does not directly refer to anything in C v2 (the dependency just serves to upgrade B's dependency).

In that example, A's dependency list should include both B, which it does use, and C v2, which it doesn't use but is an upgrade of one of its immediate dependency's transitive dependencies.

The retention test goes something like this, I think:

compiled_package
    .dependency_ids
    .published
    .retain(|pkg_name, id| {
        pkgs.contains_key(pkg_name)
            || pkg_name_to_orig_id
                .get(pkg_name)
                .is_some_and(|orig_id| linkage_table_original_ids.contains(orig_id))
    })

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this test and fixed this part, I think, but I cannot get the test to work properly. It's working when following the same steps with a CLI build. I think the issue is around publishing without tree shaking using old test-transaction-builder, which does not create a Move.lock file.

Need to think a bit what to do.

Copy link
Contributor Author

@stefan-mysten stefan-mysten Mar 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so here's the new test for it
K, K_v2, L_depends_on_K (with code reference to K), and M_depends_on_L_and_K_v2, with code calling function from L_depends_on_K package, but no code reference to K_v2.

All should work, but I have an intuition that all this breaks if there are no Move.lock files and only Move.toml with published-at field. I need to understand how things worked before automated address mgmt.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants