Skip to content

Commit

Permalink
[source-validation] Support validating upgraded deps (MystenLabs#10387)
Browse files Browse the repository at this point in the history
## Description 

Make source validation aware that packages could be upgraded, meaning
that the address they are published at is different from their module
self-addresses.

## Test Plan 

New test for source validation + upgrades

```
sui-source-validation$ cargo nextest -- successful_verification_upgrades
```

(+ Updating all existing tests to reference the `published-at` address
of a package.

---
If your changes are not user-facing and not a breaking change, you can
skip the following section. Otherwise, please indicate what changed, and
then add to the Release Notes section as highlighted during the release
process.

### Type of Change (Check all that apply)

- [x] 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

Unblocks publishing a package that depends on an upgraded package, and
use of `sui client verify-source` on an upgraded package.
  • Loading branch information
amnn authored Apr 5, 2023
1 parent c34761b commit 870927d
Show file tree
Hide file tree
Showing 22 changed files with 533 additions and 236 deletions.
2 changes: 1 addition & 1 deletion crates/sui-core/src/test_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ pub fn compile_nfts_package() -> CompiledPackage {
}

pub fn compile_example_package(relative_path: &str) -> CompiledPackage {
move_package::package_hooks::register_package_hooks(Box::new(SuiPackageHooks {}));
move_package::package_hooks::register_package_hooks(Box::new(SuiPackageHooks));
let mut path = PathBuf::from(env!("CARGO_MANIFEST_DIR"));
path.push(relative_path);

Expand Down
4 changes: 2 additions & 2 deletions crates/sui-core/src/unit_tests/move_package_publish_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ use sui_types::{

use move_package::source_package::manifest_parser;
use sui_framework_build::compiled_package::{
check_unpublished_dependencies, gather_dependencies, BuildConfig,
check_unpublished_dependencies, gather_published_ids, BuildConfig,
};
use sui_types::{
crypto::{get_key_pair, AccountKeyPair},
Expand Down Expand Up @@ -281,7 +281,7 @@ async fn test_custom_property_check_unpublished_dependencies() {
.expect("Could not build resolution graph.");

let SuiError::ModulePublishFailure { error } =
check_unpublished_dependencies(&gather_dependencies(&resolution_graph).unpublished)
check_unpublished_dependencies(&gather_published_ids(&resolution_graph).1.unpublished)
.err()
.unwrap()
else {
Expand Down
95 changes: 60 additions & 35 deletions crates/sui-framework-build/src/compiled_package.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ use move_core_types::{
account_address::AccountAddress,
language_storage::{ModuleId, StructTag, TypeTag},
};
use move_package::source_package::parsed_manifest::CustomDepInfo;
use move_package::{
compilation::{
build_plan::BuildPlan, compiled_package::CompiledPackage as MoveCompiledPackage,
Expand All @@ -36,6 +35,9 @@ use move_package::{
resolution::resolution_graph::ResolvedGraph,
BuildConfig as MoveBuildConfig,
};
use move_package::{
resolution::resolution_graph::Package, source_package::parsed_manifest::CustomDepInfo,
};
use move_symbol_pool::Symbol;
use serde_reflection::Registry;
use sui_types::{
Expand All @@ -49,8 +51,11 @@ use sui_verifier::verifier as sui_bytecode_verifier;
use crate::{MOVE_STDLIB_PACKAGE_NAME, SUI_PACKAGE_NAME, SUI_SYSTEM_PACKAGE_NAME};

/// Wrapper around the core Move `CompiledPackage` with some Sui-specific traits and info
#[derive(Debug)]
pub struct CompiledPackage {
pub package: MoveCompiledPackage,
/// Address the package is recorded as being published at.
pub published_at: Result<ObjectID, PublishedAtError>,
/// The dependency IDs of this package
pub dependency_ids: PackageDependencies,
/// Path to the Move package (i.e., where the Move.toml file is)
Expand Down Expand Up @@ -165,7 +170,8 @@ pub fn build_from_resolution_graph(
run_bytecode_verifier: bool,
print_diags_to_stderr: bool,
) -> SuiResult<CompiledPackage> {
let dependency_ids = gather_dependencies(&resolution_graph);
let (published_at, dependency_ids) = gather_published_ids(&resolution_graph);

let result = if print_diags_to_stderr {
BuildConfig::compile_package(resolution_graph, &mut std::io::stderr())
} else {
Expand Down Expand Up @@ -195,6 +201,7 @@ pub fn build_from_resolution_graph(
}
Ok(CompiledPackage {
package,
published_at,
dependency_ids,
path,
})
Expand Down Expand Up @@ -608,7 +615,7 @@ impl GetModule for CompiledPackage {

pub const PUBLISHED_AT_MANIFEST_FIELD: &str = "published-at";

pub struct SuiPackageHooks {}
pub struct SuiPackageHooks;

impl PackageHooks for SuiPackageHooks {
fn custom_package_info_fields(&self) -> Vec<String> {
Expand All @@ -628,6 +635,7 @@ impl PackageHooks for SuiPackageHooks {
}
}

#[derive(Debug)]
pub struct PackageDependencies {
/// Set of published dependencies (name and address).
pub published: BTreeMap<Symbol, ObjectID>,
Expand All @@ -637,52 +645,69 @@ pub struct PackageDependencies {
pub invalid: BTreeMap<Symbol, String>,
}

/// Gather transitive package dependencies, partitioned into two sets:
/// - published dependencies (which contain `published-at` address in manifest); and
/// - unpublished dependencies (no `published-at` address in manifest).
pub fn gather_dependencies(resolution_graph: &ResolvedGraph) -> PackageDependencies {
#[derive(Debug)]
pub enum PublishedAtError {
Invalid(String),
NotPresent,
}

/// Partition packages in `resolution_graph` into one of four groups:
/// - The ID that the package itself is published at (if it is published)
/// - The IDs of dependencies that have been published
/// - The names of packages that have not been published on chain.
/// - The names of packages that have a `published-at` field that isn't filled with a valid address.
pub fn gather_published_ids(
resolution_graph: &ResolvedGraph,
) -> (Result<ObjectID, PublishedAtError>, PackageDependencies) {
let root = resolution_graph.root_package();

let mut published = BTreeMap::new();
let mut unpublished = BTreeSet::new();
let mut invalid = BTreeMap::new();
let mut published_at = Err(PublishedAtError::NotPresent);

for (name, package) in &resolution_graph.package_table {
let property = published_at_property(package);
if name == &root {
// Separate out the root package as a special case
published_at = property;
continue;
}

for name in resolution_graph.graph.package_table.keys() {
if let Some(package) = &resolution_graph.package_table.get(name) {
let value = package
.source_package
.package
.custom_properties
.get(&Symbol::from(PUBLISHED_AT_MANIFEST_FIELD));

if value.is_none() {
match property {
Ok(id) => {
published.insert(*name, id);
}
Err(PublishedAtError::NotPresent) => {
unpublished.insert(*name);
continue;
}

if let Some(id) = value.and_then(|v| ObjectID::from_str(v.as_str()).ok()) {
published.insert(*name, id);
} else {
invalid.insert(*name, value.unwrap().to_string());
Err(PublishedAtError::Invalid(value)) => {
invalid.insert(*name, value);
}
}
};
}

PackageDependencies {
published,
unpublished,
invalid,
}
(
published_at,
PackageDependencies {
published,
unpublished,
invalid,
},
)
}

pub fn root_published_at(resolution_graph: &ResolvedGraph) -> Option<ObjectID> {
let published_at = resolution_graph
.package_table
.get(&resolution_graph.graph.root_package)
.unwrap()
pub fn published_at_property(package: &Package) -> Result<ObjectID, PublishedAtError> {
let Some(value) = package
.source_package
.package
.custom_properties
.get(&Symbol::from(PUBLISHED_AT_MANIFEST_FIELD))?;
ObjectID::from_str(published_at.as_str()).ok()
.get(&Symbol::from(PUBLISHED_AT_MANIFEST_FIELD))
else {
return Err(PublishedAtError::NotPresent);
};

ObjectID::from_str(value.as_str()).map_err(|_| PublishedAtError::Invalid(value.to_owned()))
}

pub fn check_unpublished_dependencies(unpublished: &BTreeSet<Symbol>) -> Result<(), SuiError> {
Expand Down
2 changes: 1 addition & 1 deletion crates/sui-framework/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ const DOCS_DIR: &str = "docs";

/// Save revision info to environment variable
fn main() {
move_package::package_hooks::register_package_hooks(Box::new(SuiPackageHooks {}));
move_package::package_hooks::register_package_hooks(Box::new(SuiPackageHooks));
let out_dir = PathBuf::from(env::var("OUT_DIR").unwrap());
let packages_path = Path::new(env!("CARGO_MANIFEST_DIR")).join("packages");

Expand Down
37 changes: 36 additions & 1 deletion crates/sui-json-rpc-types/src/object_changes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use move_core_types::language_storage::StructTag;
use schemars::JsonSchema;
use serde::{Deserialize, Serialize};
use serde_with::serde_as;
use sui_types::base_types::{ObjectDigest, ObjectID, SequenceNumber, SuiAddress};
use sui_types::base_types::{ObjectDigest, ObjectID, ObjectRef, SequenceNumber, SuiAddress};
use sui_types::object::Owner;
use sui_types::sui_serde::SuiStructTag;

Expand Down Expand Up @@ -93,6 +93,41 @@ impl ObjectChange {
}
}

pub fn object_ref(&self) -> ObjectRef {
match self {
ObjectChange::Published {
package_id,
version,
digest,
..
} => (*package_id, *version, *digest),
ObjectChange::Transferred {
object_id,
version,
digest,
..
}
| ObjectChange::Mutated {
object_id,
version,
digest,
..
}
| ObjectChange::Created {
object_id,
version,
digest,
..
} => (*object_id, *version, *digest),
ObjectChange::Deleted {
object_id, version, ..
} => (*object_id, *version, ObjectDigest::OBJECT_DIGEST_DELETED),
ObjectChange::Wrapped {
object_id, version, ..
} => (*object_id, *version, ObjectDigest::OBJECT_DIGEST_WRAPPED),
}
}

pub fn mask_for_test(&mut self, new_version: SequenceNumber, new_digest: ObjectDigest) {
match self {
ObjectChange::Published {
Expand Down
4 changes: 4 additions & 0 deletions crates/sui-source-validation/fixture/a/Move.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
[package]
name = "a"
version = "0.0.1"
published-at = "$STORAGE_ID"

[dependencies]
b = { local = "../b" }

[addresses]
a = "$RUNTIME_ID"
7 changes: 7 additions & 0 deletions crates/sui-source-validation/fixture/b-v2/Move.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[package]
name = "b"
version = "0.0.2"
published-at = "$STORAGE_ID"

[addresses]
b = "$RUNTIME_ID"
12 changes: 12 additions & 0 deletions crates/sui-source-validation/fixture/b-v2/sources/b.move
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// Copyright (c) Mysten Labs, Inc.
// SPDX-License-Identifier: Apache-2.0

module b::b {
public fun b(): u64 {
42
}

public fun c(): u64 {
b() + 1
}
}
8 changes: 8 additions & 0 deletions crates/sui-source-validation/fixture/b-v2/sources/c.move
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
// Copyright (c) Mysten Labs, Inc.
// SPDX-License-Identifier: Apache-2.0

module b::c {
public fun c(): u64 {
43
}
}
8 changes: 8 additions & 0 deletions crates/sui-source-validation/fixture/b-v2/sources/d.move
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
// Copyright (c) Mysten Labs, Inc.
// SPDX-License-Identifier: Apache-2.0

module b::d {
public fun d(): u64 {
44
}
}
4 changes: 4 additions & 0 deletions crates/sui-source-validation/fixture/b/Move.toml
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
[package]
name = "b"
version = "0.0.1"
published-at = "$STORAGE_ID"

[addresses]
b = "$RUNTIME_ID"
4 changes: 4 additions & 0 deletions crates/sui-source-validation/fixture/c/Move.toml
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
[package]
name = "c"
version = "0.0.1"
published-at = "$STORAGE_ID"

[addresses]
c = "$RUNTIME_ID"
4 changes: 4 additions & 0 deletions crates/sui-source-validation/fixture/d/Move.toml
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
[package]
name = "d"
version = "0.0.1"
published-at = "$STORAGE_ID"

[dependencies]
b = { local = "../b" }
c = { local = "../c" }

[addresses]
d = "$RUNTIME_ID"
10 changes: 10 additions & 0 deletions crates/sui-source-validation/fixture/e/Move.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
[package]
name = "e"
version = "0.0.1"
published-at = "$STORAGE_ID"

[dependencies]
b = { local = "../b-v2" }

[addresses]
e = "$RUNTIME_ID"
11 changes: 11 additions & 0 deletions crates/sui-source-validation/fixture/e/sources/e.move
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// Copyright (c) Mysten Labs, Inc.
// SPDX-License-Identifier: Apache-2.0

module e::e {
use b::b::b;
use b::b::c;

public fun e() : u64 {
b() + c()
}
}
4 changes: 4 additions & 0 deletions crates/sui-source-validation/fixture/z/Move.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
[package]
name = "z"
version = "0.0.1"
published-at = "$STORAGE_ID"

[dependencies]
Sui = { local = "$REPO_ROOT/crates/sui-framework/packages/sui-framework" }

[addresses]
z = "$RUNTIME_ID"
Loading

0 comments on commit 870927d

Please sign in to comment.