From a77c7398facee030632653094aa1276f0056bf48 Mon Sep 17 00:00:00 2001 From: Anastasios Kichidis Date: Wed, 29 Mar 2023 15:29:35 +0100 Subject: [PATCH] [fix] state accumulator - do not fail when previous obj version is not found (#10083) ## Description It seems that state accumulator is making an assumption on the wrapped object existing in the storage (the previous version - tombstone) when it's unwrapping it. However, if this is a wrapped object that never existed before , we don't expect to find in the storage any tombstone - thus we shouldn't panic. We currently see in DevNet this happening: ``` thread 'tokio-runtime-worker' panicked at 'wrapped tombstone must precede unwrapped object', crates/sui-core/src/state_accumulator.rs:173:30 ``` Thanks @amnn @gdanezis for helping on root causing this! ## Test Plan How did you test the new or updated feature? --- 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) - [ ] 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 --- crates/sui-core/src/state_accumulator.rs | 16 ++-- .../object_owner/sources/object_owner.move | 7 ++ .../src/unit_tests/move_integration_tests.rs | 95 +++++++++++++++++++ 3 files changed, 111 insertions(+), 7 deletions(-) diff --git a/crates/sui-core/src/state_accumulator.rs b/crates/sui-core/src/state_accumulator.rs index 378cf4dd079fb..25fedf1396d63 100644 --- a/crates/sui-core/src/state_accumulator.rs +++ b/crates/sui-core/src/state_accumulator.rs @@ -166,19 +166,21 @@ impl StateAccumulator { // removed as WrappedObject using the last sequence number it was tombstoned // against. Since this happened in a past transaction, and the child object may // have been modified since (and hence its sequence number incremented), we - // seek the version prior to the unwrapped version from the objects table directly + // seek the version prior to the unwrapped version from the objects table directly. + // If the tombstone is not found, then assume this is a newly created wrapped object hence + // we don't expect to find it in the table. let wrapped_objects_to_remove: Vec = all_unwrapped .iter() - .map(|(id, seq_num)| { + .filter_map(|(id, seq_num)| { let objref = self .authority_store .get_object_ref_prior_to_key(id, *seq_num) - .expect("read cannot fail") - .expect("wrapped tombstone must precede unwrapped object"); - - assert!(objref.2.is_wrapped(), "{:?}", objref); + .expect("read cannot fail"); - WrappedObject::new(objref.0, objref.1) + objref.map(|(id, version, digest)| { + assert!(digest.is_wrapped(), "{:?}", id); + WrappedObject::new(id, version) + }) }) .collect(); diff --git a/crates/sui-core/src/unit_tests/data/object_owner/sources/object_owner.move b/crates/sui-core/src/unit_tests/data/object_owner/sources/object_owner.move index e903bb28817dd..3f8427f8d5c36 100644 --- a/crates/sui-core/src/unit_tests/data/object_owner/sources/object_owner.move +++ b/crates/sui-core/src/unit_tests/data/object_owner/sources/object_owner.move @@ -83,6 +83,13 @@ module object_owner::object_owner { transfer::public_transfer(child, tx_context::sender(ctx)); } + public entry fun remove_wrapped_child(parent: &mut Parent, ctx: &mut TxContext) { + let child_id = option::extract(&mut parent.child); + let child: Child = dynamic_field::remove(&mut parent.id, 0); + assert!(object::id(&child) == child_id, 0); + transfer::public_transfer(child, tx_context::sender(ctx)); + } + // Call to delete_child public entry fun delete_child(child: Child) { let Child { id } = child; diff --git a/crates/sui-core/src/unit_tests/move_integration_tests.rs b/crates/sui-core/src/unit_tests/move_integration_tests.rs index 3be8c9b5cc329..6331d36efdde6 100644 --- a/crates/sui-core/src/unit_tests/move_integration_tests.rs +++ b/crates/sui-core/src/unit_tests/move_integration_tests.rs @@ -580,6 +580,101 @@ async fn test_create_then_delete_parent_child_wrap() { ); } +/// We are explicitly testing the case where a parent and child object are created together - where +/// no prior child version exists - and then we remove the child successfully. +#[tokio::test] +#[cfg_attr(msim, ignore)] +async fn test_remove_child_when_no_prior_version_exists() { + let (sender, sender_key): (_, AccountKeyPair) = get_key_pair(); + let gas = ObjectID::random(); + let authority = init_state_with_ids(vec![(sender, gas)]).await; + + let package = build_and_publish_test_package( + &authority, + &sender, + &sender_key, + &gas, + "object_owner", + /* with_unpublished_deps */ false, + ) + .await; + + // Create a parent and a child together + let effects = call_move( + &authority, + &gas, + &sender, + &sender_key, + &package.0, + "object_owner", + "create_parent_and_child_wrapped", + vec![], + vec![], + ) + .await + .unwrap(); + + assert!(effects.status().is_ok()); + // Modifies the gas object + assert_eq!(effects.mutated().len(), 1); + // Creates 3 objects, the parent, a field, and the child + assert_eq!(effects.created().len(), 2); + // not wrapped as it wasn't first created + assert_eq!(effects.wrapped().len(), 0); + + let gas_ref = effects.mutated()[0].0; + + let parent = effects + .created() + .iter() + .find(|(_, owner)| matches!(owner, Owner::AddressOwner(_))) + .unwrap() + .0; + + let field = effects + .created() + .iter() + .find(|((id, _, _), _)| id != &parent.0) + .unwrap() + .0; + + // Delete the child only + let effects = call_move( + &authority, + &gas, + &sender, + &sender_key, + &package.0, + "object_owner", + "remove_wrapped_child", + vec![], + vec![TestCallArg::Object(parent.0)], + ) + .await + .unwrap(); + + assert!(effects.status().is_ok()); + + // The field is considered deleted. The child doesn't count because it wasn't + // considered created in the first place. + assert_eq!(effects.deleted().len(), 1); + // The child was never created so it is not unwrapped. + assert_eq!(effects.unwrapped_then_deleted().len(), 0); + + assert_eq!( + effects + .modified_at_versions() + .iter() + .cloned() + .collect::>(), + HashSet::from([ + (gas_ref.0, gas_ref.1), + (parent.0, parent.1), + (field.0, field.1) + ]), + ); +} + #[tokio::test] #[cfg_attr(msim, ignore)] async fn test_create_then_delete_parent_child_wrap_separate() {