fix(verify-source): Detect published-at = 0x0 #18978

amnn · 2024-08-13T20:14:37Z

Description

Detect when the published-at field in Move.toml or Move.lock has been explicitly set to 0x0 and treat that as if it was not set at all.

This is not commonly done by people, but it happens in our test set-up.

This also required converting the field into an ObjectID earlier in the pipeline, which introduced some further changes in the codebase.

Test plan

sui-source-validation$ cargo nextest run

Stack

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.

vercel · 2024-08-13T20:14:44Z

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	Aug 14, 2024 6:00pm

3 Skipped Deployments

Name	Status	Preview	Updated (UTC)
multisig-toolkit	⬜️ Ignored (Inspect)	Visit Preview	Aug 14, 2024 6:00pm
sui-kiosk	⬜️ Ignored (Inspect)	Visit Preview	Aug 14, 2024 6:00pm
sui-typescript-docs	⬜️ Ignored (Inspect)	Visit Preview	Aug 14, 2024 6:00pm

stefan-mysten · 2024-08-13T21:59:37Z

crates/sui-package-management/src/lib.rs

+    let published_id_in_manifest = manifest_published_at(package);
+
+    match published_id_in_manifest {
+        Ok(_) | Err(PublishedAtError::NotPresent) => { /* nop */ }


I feel like this manifest_published_at should be a different function, that checks if it is present or not. Somehow (as I don't know this code at all), it gets confusing that if the function's result is Ok, an error is returned.

stefan-mysten

Not familiar with the code, but looks reasonable. Are these changes covered by tests?

amnn · 2024-08-14T10:05:57Z

Are these changes covered by tests?

Yes, covered by tests further up the stack -- this is actually how I noticed the issue.

## Description Move logic for dealing with toolchain versioning into its own module. ## Test plan ``` sui-source-validation$ cargo nextest run ```

## Description Detect when the `published-at` field in `Move.toml` or `Move.lock` has been explicitly set to `0x0` and treat that as if it was not set at all. This is not commonly done by people, but it happens in our test set-up. This also required converting the field into an `ObjectID` earlier in the pipeline, which introduced some further changes in the codebase. ## Test plan ``` sui-source-validation$ cargo nextest run ```

## Description Fix a bug where publish test transactions always refer to their dependencies at their original IDs. This breaks publishing test packages that depend on upgraded packages. ## Test plan CI -- existing tests using TestTransactionBuilder should still pass. ## Stack - #18956 - #18959 - #18978 --- ## 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): - [ ] Indexer: - [ ] JSON-RPC: - [ ] GraphQL: - [ ] CLI: - [ ] Rust SDK: - [ ] REST API:

## Description Fix a bug in tests that publish packages where the publish transaction was always constructed referring to dependencies at their original IDs, and not their storage IDs. This has not caused a problem to date, meaning these tests may not publish a package that depends on an upgrade package, but to avoid confusion `get_dependency_original_package_ids` has been replaced with `get_dependency_storage_package_ids`. Similarly, `get_package_dependencies_hex` has been replaced with an invocation of `get_dependency_storage_package_ids`. ## Test plan CI +: ``` sui$ cargo build --bin sui sui$ cd tmp sui$ ~/sui/target/debug/sui move new test sui$ # make it possible to compile sui$ ~/sui/target/debug/ui move build --dump-bytecode-as-base64 ``` ## Stack - #18956 - #18959 - #18978 - #18960 --- ## 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): - [ ] Indexer: - [ ] JSON-RPC: - [ ] GraphQL: - [ ] CLI: - [ ] Rust SDK: - [ ] REST API:

## Description Source validation additionally checks that dependencies in the linkage table of the on-chain package match the published dependencies from the source package. Previously it was possible to construct a scenario where source verification would succeed even though a package's representation on-chain did not exactly match, because one of the dependencies mismatched on version but between two versions that were themselves identical. ## Test plan New tests exercising the scenario mentioned above: ``` sui-source-validation$ cargo nextest run -- linkage_differs ``` ## Stack - #18956 - #18959 - #18978 - #18960 - #18962 --- ## 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): - [ ] Indexer: - [ ] JSON-RPC: - [ ] GraphQL: - [x] CLI: `sui client verify-source` now also confirms a package's linkage table matches its source dependencies. - [ ] Rust SDK: - [ ] REST API:

## Description Detect when the `published-at` field in `Move.toml` or `Move.lock` has been explicitly set to `0x0` and treat that as if it was not set at all. This is not commonly done by people, but it happens in our test set-up. This also required converting the field into an `ObjectID` earlier in the pipeline, which introduced some further changes in the codebase. ## Test plan ``` sui-source-validation$ cargo nextest run ``` ## Stack - #18956 - #18959 --- ## 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): - [ ] Indexer: - [ ] JSON-RPC: - [ ] GraphQL: - [x] CLI: Explicitly setting `published-at = "0x0"` is treated as if the `published-at` field was omitted. - [ ] Rust SDK: - [ ] REST API:

## Description Fix a bug where publish test transactions always refer to their dependencies at their original IDs. This breaks publishing test packages that depend on upgraded packages. ## Test plan CI -- existing tests using TestTransactionBuilder should still pass. ## Stack - #18956 - #18959 - #18978 --- ## 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): - [ ] Indexer: - [ ] JSON-RPC: - [ ] GraphQL: - [ ] CLI: - [ ] Rust SDK: - [ ] REST API:

## Description Fix a bug in tests that publish packages where the publish transaction was always constructed referring to dependencies at their original IDs, and not their storage IDs. This has not caused a problem to date, meaning these tests may not publish a package that depends on an upgrade package, but to avoid confusion `get_dependency_original_package_ids` has been replaced with `get_dependency_storage_package_ids`. Similarly, `get_package_dependencies_hex` has been replaced with an invocation of `get_dependency_storage_package_ids`. ## Test plan CI +: ``` sui$ cargo build --bin sui sui$ cd tmp sui$ ~/sui/target/debug/sui move new test sui$ # make it possible to compile sui$ ~/sui/target/debug/ui move build --dump-bytecode-as-base64 ``` ## Stack - #18956 - #18959 - #18978 - #18960 --- ## 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): - [ ] Indexer: - [ ] JSON-RPC: - [ ] GraphQL: - [ ] CLI: - [ ] Rust SDK: - [ ] REST API:

## Description Source validation additionally checks that dependencies in the linkage table of the on-chain package match the published dependencies from the source package. Previously it was possible to construct a scenario where source verification would succeed even though a package's representation on-chain did not exactly match, because one of the dependencies mismatched on version but between two versions that were themselves identical. ## Test plan New tests exercising the scenario mentioned above: ``` sui-source-validation$ cargo nextest run -- linkage_differs ``` ## Stack - #18956 - #18959 - #18978 - #18960 - #18962 --- ## 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): - [ ] Indexer: - [ ] JSON-RPC: - [ ] GraphQL: - [x] CLI: `sui client verify-source` now also confirms a package's linkage table matches its source dependencies. - [ ] Rust SDK: - [ ] REST API:

## Description Detect when the `published-at` field in `Move.toml` or `Move.lock` has been explicitly set to `0x0` and treat that as if it was not set at all. This is not commonly done by people, but it happens in our test set-up. This also required converting the field into an `ObjectID` earlier in the pipeline, which introduced some further changes in the codebase. ## Test plan ``` sui-source-validation$ cargo nextest run ``` ## Stack - #18956 - #18959 --- ## 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): - [ ] Indexer: - [ ] JSON-RPC: - [ ] GraphQL: - [x] CLI: Explicitly setting `published-at = "0x0"` is treated as if the `published-at` field was omitted. - [ ] Rust SDK: - [ ] REST API:

## Description Fix a bug where publish test transactions always refer to their dependencies at their original IDs. This breaks publishing test packages that depend on upgraded packages. ## Test plan CI -- existing tests using TestTransactionBuilder should still pass. ## Stack - #18956 - #18959 - #18978 --- ## 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): - [ ] Indexer: - [ ] JSON-RPC: - [ ] GraphQL: - [ ] CLI: - [ ] Rust SDK: - [ ] REST API:

## Description Fix a bug in tests that publish packages where the publish transaction was always constructed referring to dependencies at their original IDs, and not their storage IDs. This has not caused a problem to date, meaning these tests may not publish a package that depends on an upgrade package, but to avoid confusion `get_dependency_original_package_ids` has been replaced with `get_dependency_storage_package_ids`. Similarly, `get_package_dependencies_hex` has been replaced with an invocation of `get_dependency_storage_package_ids`. ## Test plan CI +: ``` sui$ cargo build --bin sui sui$ cd tmp sui$ ~/sui/target/debug/sui move new test sui$ # make it possible to compile sui$ ~/sui/target/debug/ui move build --dump-bytecode-as-base64 ``` ## Stack - #18956 - #18959 - #18978 - #18960 --- ## 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): - [ ] Indexer: - [ ] JSON-RPC: - [ ] GraphQL: - [ ] CLI: - [ ] Rust SDK: - [ ] REST API:

## Description Source validation additionally checks that dependencies in the linkage table of the on-chain package match the published dependencies from the source package. Previously it was possible to construct a scenario where source verification would succeed even though a package's representation on-chain did not exactly match, because one of the dependencies mismatched on version but between two versions that were themselves identical. ## Test plan New tests exercising the scenario mentioned above: ``` sui-source-validation$ cargo nextest run -- linkage_differs ``` ## Stack - #18956 - #18959 - #18978 - #18960 - #18962 --- ## 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): - [ ] Indexer: - [ ] JSON-RPC: - [ ] GraphQL: - [x] CLI: `sui client verify-source` now also confirms a package's linkage table matches its source dependencies. - [ ] Rust SDK: - [ ] REST API:

amnn requested review from rvantonder and a team August 13, 2024 20:14

amnn self-assigned this Aug 13, 2024

This was referenced Aug 13, 2024

fix: [TransactionBuilder] Publish use storage IDs #18960

Merged

fix(test): publish depending on upgrade #18962

Merged

feat: [Source Validation] Check linkage #18964

Merged

stefan-mysten reviewed Aug 13, 2024

View reviewed changes

stefan-mysten approved these changes Aug 13, 2024

View reviewed changes

vercel bot deployed to Preview – sui-docs August 14, 2024 10:51 View deployment

amnn added 2 commits August 14, 2024 18:11

chore: [Source Validation] Split out toolchain logic

d23cdfa

## Description Move logic for dealing with toolchain versioning into its own module. ## Test plan ``` sui-source-validation$ cargo nextest run ```

amnn force-pushed the amnn/src-verify-0x0 branch from 98316ef to 97cf57e Compare August 14, 2024 17:36

amnn force-pushed the amnn/src-verify-tool branch from 5451e5a to d23cdfa Compare August 14, 2024 17:36

Base automatically changed from amnn/src-verify-tool to amnn/src-valid-err August 14, 2024 17:38

amnn merged commit b90b06c into amnn/src-valid-err Aug 14, 2024
53 of 55 checks passed

amnn deleted the amnn/src-verify-0x0 branch August 14, 2024 17:38

vercel bot deployed to Preview – sui-docs August 14, 2024 18:00 View deployment

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(verify-source): Detect published-at = 0x0 #18978

fix(verify-source): Detect published-at = 0x0 #18978

amnn commented Aug 13, 2024

vercel bot commented Aug 13, 2024 •

edited

Loading

stefan-mysten Aug 13, 2024

stefan-mysten left a comment

amnn commented Aug 14, 2024

fix(verify-source): Detect published-at = 0x0 #18978

fix(verify-source): Detect published-at = 0x0 #18978

Conversation

amnn commented Aug 13, 2024

Description

Test plan

Stack

Release notes

vercel bot commented Aug 13, 2024 • edited Loading

stefan-mysten Aug 13, 2024

Choose a reason for hiding this comment

stefan-mysten left a comment

Choose a reason for hiding this comment

amnn commented Aug 14, 2024

vercel bot commented Aug 13, 2024 •

edited

Loading