Skip to content

Commit

Permalink
[fix] state accumulator - do not fail when previous obj version is no…
Browse files Browse the repository at this point in the history
…t found (MystenLabs#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
  • Loading branch information
akichidis authored Mar 29, 2023
1 parent ebc4e82 commit a77c739
Show file tree
Hide file tree
Showing 3 changed files with 111 additions and 7 deletions.
16 changes: 9 additions & 7 deletions crates/sui-core/src/state_accumulator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<WrappedObject> = 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();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
95 changes: 95 additions & 0 deletions crates/sui-core/src/unit_tests/move_integration_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<_>>(),
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() {
Expand Down

0 comments on commit a77c739

Please sign in to comment.