Skip to content

Commit

Permalink
A few simplifications in the sui-executor (MystenLabs#13140)
Browse files Browse the repository at this point in the history 
## Description 

1. In the circular ownership check, input objects cannot have object
owner, so they don't need to be involved at all. Remove them. @tnowacki
please double check if this is correct.
2. Rename ObjectStore to ChildObjectStore to improve accuracy
3. Fix a few indentations

## Test Plan 

CI

---
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)

- [ ] protocol change
- [ ] 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
@lxfind
lxfind authored Jul 28, 2023
1 parent da719a2 commit 0014233
Show file tree
Hide file tree
Showing 3 changed files with 69 additions and 79 deletions.
105 changes: 53 additions & 52 deletions sui-execution/latest/sui-adapter/src/programmable_transactions/context.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -157,17 +157,18 @@ mod checked {
// so to mimic this "off limits" behavior, we act as if the coin has less balance than
// it really does
let Some(Value::Object(ObjectValue {
contents: ObjectContents::Coin(coin),
..
})) = &mut gas.inner.value else {
invariant_violation!("Gas object should be a populated coin")
};
contents: ObjectContents::Coin(coin),
..
})) = &mut gas.inner.value else {
invariant_violation!("Gas object should be a populated coin")
};

let max_gas_in_balance = gas_charger.gas_budget();
let Some(new_balance) = coin.balance.value().checked_sub(max_gas_in_balance) else {
invariant_violation!(
"Transaction input checker should check that there is enough gas"
);
};
invariant_violation!(
"Transaction input checker should check that there is enough gas"
);
};
coin.balance = Balance::new(new_balance);
gas
} else {
Expand Down Expand Up @@ -326,8 +327,8 @@ mod checked {
.type_to_type_layout(&ty)
.map_err(|e| self.convert_vm_error(e))?;
let Some(bytes) = value.simple_serialize(&layout) else {
invariant_violation!("Failed to deserialize already serialized Move value");
};
invariant_violation!("Failed to deserialize already serialized Move value");
};
Ok((module_id.clone(), tag, bytes))
})
.collect::<Result<Vec<_>, ExecutionError>>()?;
Expand Down Expand Up @@ -479,8 +480,8 @@ mod checked {
);
// restore is exclusively used for mut
let Ok((_, value_opt)) = self.borrow_mut_impl(arg, None) else {
invariant_violation!("Should be able to borrow argument to restore it")
};
invariant_violation!("Should be able to borrow argument to restore it")
};
let old_value = value_opt.replace(value);
assert_invariant!(
old_value.is_none() || old_value.unwrap().is_copyable(),
Expand Down Expand Up @@ -588,9 +589,9 @@ mod checked {
let id = object_metadata.id;
input_object_metadata.insert(object_metadata.id, object_metadata);
let Some(Value::Object(object_value)) = value else {
by_value_inputs.insert(id);
return Ok(())
};
by_value_inputs.insert(id);
return Ok(())
};
if is_mutable_input {
add_additional_write(&mut additional_writes, owner, object_value)?;
}
Expand Down Expand Up @@ -750,8 +751,8 @@ mod checked {
.type_to_type_layout(&ty)
.map_err(|e| convert_vm_error(e, vm, tmp_session.get_resolver()))?;
let Some(bytes) = value.simple_serialize(&layout) else {
invariant_violation!("Failed to deserialize already serialized Move value");
};
invariant_violation!("Failed to deserialize already serialized Move value");
};
// safe because has_public_transfer has been determined by the abilities
let move_object = unsafe {
create_written_object(
Expand Down Expand Up @@ -780,20 +781,20 @@ mod checked {
let delete_kind_with_seq = match delete_kind {
DeleteKind::Normal | DeleteKind::Wrap => {
let old_version = match input_object_metadata.get(&id) {
Some(metadata) => {
assert_invariant!(
!matches!(metadata.owner, Owner::Immutable),
"Attempting to delete immutable object {id} via delete kind {delete_kind}"
);
metadata.version
}
None => {
match loaded_child_objects.get(&id) {
Some(version) => *version,
None => invariant_violation!("Deleted/wrapped object {id} must be either in input or loaded child objects")
Some(metadata) => {
assert_invariant!(
!matches!(metadata.owner, Owner::Immutable),
"Attempting to delete immutable object {id} via delete kind {delete_kind}"
);
metadata.version
}
}
};
None => {
match loaded_child_objects.get(&id) {
Some(version) => *version,
None => invariant_violation!("Deleted/wrapped object {id} must be either in input or loaded child objects")
}
}
};
if delete_kind == DeleteKind::Normal {
DeleteKindWithOldVersion::Normal(old_version)
} else {
Expand Down Expand Up @@ -897,23 +898,23 @@ mod checked {
}
Argument::Result(i) => {
let Some(command_result) = self.results.get_mut(i as usize) else {
return Err(CommandArgumentError::IndexOutOfBounds { idx: i });
};
return Err(CommandArgumentError::IndexOutOfBounds { idx: i });
};
if command_result.len() != 1 {
return Err(CommandArgumentError::InvalidResultArity { result_idx: i });
}
(None, &mut command_result[0])
}
Argument::NestedResult(i, j) => {
let Some(command_result) = self.results.get_mut(i as usize) else {
return Err(CommandArgumentError::IndexOutOfBounds { idx: i });
};
return Err(CommandArgumentError::IndexOutOfBounds { idx: i });
};
let Some(result_value) = command_result.get_mut(j as usize) else {
return Err(CommandArgumentError::SecondaryIndexOutOfBounds {
result_idx: i,
secondary_idx: j,
});
};
return Err(CommandArgumentError::SecondaryIndexOutOfBounds {
result_idx: i,
secondary_idx: j,
});
};
(None, result_value)
}
};
Expand Down Expand Up @@ -1273,20 +1274,20 @@ mod checked {
gas_id: ObjectID,
) -> Result<(), ExecutionError> {
let Some(AdditionalWrite { bytes,.. }) = additional_writes.get_mut(&gas_id) else {
invariant_violation!("Gas object cannot be wrapped or destroyed")
};
invariant_violation!("Gas object cannot be wrapped or destroyed")
};
let Ok(mut coin) = Coin::from_bcs_bytes(bytes) else {
invariant_violation!("Gas object must be a coin")
};
let Some(new_balance) = coin
.balance
.value()
.checked_add(gas_charger.gas_budget()) else {
return Err(ExecutionError::new_with_source(
ExecutionErrorKind::CoinBalanceOverflow,
"Gas coin too large after returning the max gas budget",
));
invariant_violation!("Gas object must be a coin")
};
let Some(new_balance) = coin
.balance
.value()
.checked_add(gas_charger.gas_budget()) else {
return Err(ExecutionError::new_with_source(
ExecutionErrorKind::CoinBalanceOverflow,
"Gas coin too large after returning the max gas budget",
));
};
coin.balance = Balance::new(new_balance);
*bytes = coin.to_bcs_bytes();
Ok(())
Expand Down
39 changes: 14 additions & 25 deletions sui-execution/latest/sui-move-natives/src/object_runtime/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ use sui_types::{

pub(crate) mod object_store;

use object_store::ObjectStore;
use object_store::ChildObjectStore;

use self::object_store::{ChildObjectEffect, ObjectResult};

Expand Down Expand Up @@ -131,7 +131,7 @@ impl LocalProtocolConfig {

#[derive(Tid)]
pub struct ObjectRuntime<'a> {
object_store: ObjectStore<'a>,
child_object_store: ChildObjectStore<'a>,
// inventories for test scenario
pub(crate) test_inventories: TestInventories,
// the internal state
Expand Down Expand Up @@ -184,7 +184,7 @@ impl<'a> ObjectRuntime<'a> {
}
}
Self {
object_store: ObjectStore::new(
child_object_store: ChildObjectStore::new(
object_resolver,
root_version,
is_metered,
Expand Down Expand Up @@ -329,7 +329,7 @@ impl<'a> ObjectRuntime<'a> {
parent: ObjectID,
child: ObjectID,
) -> PartialVMResult<bool> {
self.object_store.object_exists(parent, child)
self.child_object_store.object_exists(parent, child)
}

pub(crate) fn child_object_exists_and_has_type(
Expand All @@ -338,7 +338,7 @@ impl<'a> ObjectRuntime<'a> {
child: ObjectID,
child_type: &MoveObjectType,
) -> PartialVMResult<bool> {
self.object_store
self.child_object_store
.object_exists_and_has_type(parent, child, child_type)
}

Expand All @@ -351,7 +351,7 @@ impl<'a> ObjectRuntime<'a> {
child_fully_annotated_layout: &MoveTypeLayout,
child_move_type: MoveObjectType,
) -> PartialVMResult<ObjectResult<&mut GlobalValue>> {
let res = self.object_store.get_or_fetch_object(
let res = self.child_object_store.get_or_fetch_object(
parent,
child,
child_ty,
Expand All @@ -373,7 +373,7 @@ impl<'a> ObjectRuntime<'a> {
child_move_type: MoveObjectType,
child_value: Value,
) -> PartialVMResult<()> {
self.object_store
self.child_object_store
.add_object(parent, child, child_ty, child_move_type, child_value)
}

Expand All @@ -387,7 +387,7 @@ impl<'a> ObjectRuntime<'a> {
by_value_inputs: BTreeSet<ObjectID>,
external_transfers: BTreeSet<ObjectID>,
) -> Result<RuntimeResults, ExecutionError> {
let (loaded_child_objects, child_effects) = self.object_store.take_effects();
let (loaded_child_objects, child_effects) = self.child_object_store.take_effects();
self.state.finish(
by_value_inputs,
external_transfers,
Expand All @@ -399,11 +399,11 @@ impl<'a> ObjectRuntime<'a> {
pub(crate) fn all_active_child_objects(
&self,
) -> impl Iterator<Item = (&ObjectID, &Type, Value)> {
self.object_store.all_active_objects()
self.child_object_store.all_active_objects()
}

pub fn loaded_child_objects(&self) -> BTreeMap<ObjectID, LoadedChildObjectMetadata> {
self.object_store
self.child_object_store
.cached_objects()
.iter()
.filter_map(|(id, obj_opt)| {
Expand Down Expand Up @@ -498,20 +498,9 @@ impl ObjectRuntimeState {
events: user_events,
total_events_size: _,
} = self;
let input_owner_map = input_objects
.iter()
.filter_map(|(id, owner)| match owner {
Owner::AddressOwner(_) | Owner::Shared { .. } | Owner::Immutable => None,
Owner::ObjectOwner(parent) => Some((*id, (*parent).into())),
})
.collect();
// update the input owners with the new owners from transfers
// reports an error on cycles
// Check new owners from transfers, reports an error on cycles.
// TODO can we have cycles in the new system?
update_owner_map(
input_owner_map,
transfers.iter().map(|(id, (owner, _, _))| (*id, *owner)),
)?;
check_circular_ownership(transfers.iter().map(|(id, (owner, _, _))| (*id, *owner)))?;
// determine write kinds
let writes: LinkedHashMap<_, _> = transfers
.into_iter()
Expand Down Expand Up @@ -580,10 +569,10 @@ impl ObjectRuntimeState {
}
}

fn update_owner_map(
mut object_owner_map: BTreeMap<ObjectID, ObjectID>,
fn check_circular_ownership(
transfers: impl IntoIterator<Item = (ObjectID, Owner)>,
) -> Result<(), ExecutionError> {
let mut object_owner_map = BTreeMap::new();
for (id, recipient) in transfers {
object_owner_map.remove(&id);
match recipient {
Expand Down
4 changes: 2 additions & 2 deletions sui-execution/latest/sui-move-natives/src/object_runtime/object_store.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ struct Inner<'a> {

// maintains the runtime GlobalValues for child objects and manages the fetching of objects
// from storage, through the `ChildObjectResolver`
pub(super) struct ObjectStore<'a> {
pub(super) struct ChildObjectStore<'a> {
// contains object resolver and object cache
// kept as a separate struct to deal with lifetime issues where the `store` is accessed
// at the same time as the `cached_objects` is populated
Expand Down Expand Up @@ -235,7 +235,7 @@ impl<'a> Inner<'a> {
}
}

impl<'a> ObjectStore<'a> {
impl<'a> ChildObjectStore<'a> {
pub(super) fn new(
resolver: &'a dyn ChildObjectResolver,
root_version: BTreeMap<ObjectID, SequenceNumber>,
Expand Down

0 comments on commit 0014233

Please sign in to comment.