diff --git a/crates/sui-adapter-transactional-tests/tests/child_count/unwrap_never_stored_transfer.exp b/crates/sui-adapter-transactional-tests/tests/child_count/unwrap_never_stored_transfer.exp index 2020e3b15b829..e099038c53ae3 100644 --- a/crates/sui-adapter-transactional-tests/tests/child_count/unwrap_never_stored_transfer.exp +++ b/crates/sui-adapter-transactional-tests/tests/child_count/unwrap_never_stored_transfer.exp @@ -12,6 +12,5 @@ created: object(107) written: object(106) task 3 'run'. lines 42-42: -created: object(109) -written: object(108) +written: object(108), object(109) deleted: object(107) diff --git a/crates/sui-adapter-transactional-tests/tests/dynamic_fields/unwrap_object.exp b/crates/sui-adapter-transactional-tests/tests/dynamic_fields/unwrap_object.exp index c9e7403c61fe0..46fbd085b75c9 100644 --- a/crates/sui-adapter-transactional-tests/tests/dynamic_fields/unwrap_object.exp +++ b/crates/sui-adapter-transactional-tests/tests/dynamic_fields/unwrap_object.exp @@ -12,12 +12,12 @@ created: object(106), object(107) written: object(105) task 3 'run'. lines 45-45: -created: object(109), object(110) -written: object(106), object(107), object(108) +created: object(110) +written: object(106), object(107), object(108), object(109) task 4 'view-object'. lines 47-47: Owner: Object ID: ( fake(110) ) -Version: 1 +Version: 2 Contents: a::m::Obj {id: sui::object::UID {id: sui::object::ID {bytes: fake(109)}}} task 5 'run'. lines 50-50: @@ -33,10 +33,9 @@ created: object(116), object(117) written: object(115) task 8 'run'. lines 58-58: -created: object(119) -written: object(116), object(117), object(118) +written: object(116), object(117), object(118), object(119) task 9 'view-object'. lines 60-60: Owner: Account Address ( A ) -Version: 1 +Version: 2 Contents: a::m::Obj {id: sui::object::UID {id: sui::object::ID {bytes: fake(119)}}} diff --git a/crates/sui-adapter-transactional-tests/tests/sui/object_basics.exp b/crates/sui-adapter-transactional-tests/tests/sui/object_basics.exp index cc9c57fd4c5be..c83c60c2e0ae1 100644 --- a/crates/sui-adapter-transactional-tests/tests/sui/object_basics.exp +++ b/crates/sui-adapter-transactional-tests/tests/sui/object_basics.exp @@ -29,7 +29,7 @@ created: object(110) written: object(109) task 7 'run'. lines 82-82: -events: CoinBalanceChange { package_id: sui, transaction_module: Identifier("gas"), sender: B, change_type: Gas, owner: AddressOwner(B), coin_type: "sui::sui::SUI", coin_object_id: fake(111), version: SequenceNumber(0), amount: -82 }, MutateObject { package_id: test, transaction_module: Identifier("object_basics"), sender: B, object_type: "test::object_basics::Object", object_id: fake(107), version: SequenceNumber(3) }, MutateObject { package_id: sui, transaction_module: Identifier("unused_input_object"), sender: B, object_type: "test::object_basics::Object", object_id: fake(110), version: SequenceNumber(2) }, MoveEvent { package_id: test, transaction_module: Identifier("object_basics"), sender: B, type_: StructTag { address: test, module: Identifier("object_basics"), name: Identifier("NewValueEvent"), type_params: [] }, contents: [20, 0, 0, 0, 0, 0, 0, 0] } +events: CoinBalanceChange { package_id: sui, transaction_module: Identifier("gas"), sender: B, change_type: Gas, owner: AddressOwner(B), coin_type: "sui::sui::SUI", coin_object_id: fake(111), version: SequenceNumber(0), amount: -82 }, MutateObject { package_id: test, transaction_module: Identifier("object_basics"), sender: B, object_type: "test::object_basics::Object", object_id: fake(107), version: SequenceNumber(3) }, MutateObject { package_id: sui, transaction_module: Identifier("unused_input_object"), sender: B, object_type: "test::object_basics::Object", object_id: fake(110), version: SequenceNumber(3) }, MoveEvent { package_id: test, transaction_module: Identifier("object_basics"), sender: B, type_: StructTag { address: test, module: Identifier("object_basics"), name: Identifier("NewValueEvent"), type_params: [] }, contents: [20, 0, 0, 0, 0, 0, 0, 0] } written: object(107), object(110), object(111) task 8 'run'. lines 84-84: diff --git a/crates/sui-adapter-transactional-tests/tests/sui/unwrap.exp b/crates/sui-adapter-transactional-tests/tests/sui/unwrap.exp index 4220ab10872e8..a41321ec32574 100644 --- a/crates/sui-adapter-transactional-tests/tests/sui/unwrap.exp +++ b/crates/sui-adapter-transactional-tests/tests/sui/unwrap.exp @@ -27,5 +27,5 @@ deleted: object(108) task 6 'view-object'. lines 81-81: Owner: Account Address ( A ) -Version: 2 +Version: 3 Contents: test::object_basics::Object {id: sui::object::UID {id: sui::object::ID {bytes: fake(106)}}, value: 10u64} diff --git a/crates/sui-adapter-transactional-tests/tests/transfer_object/quasi_shared.exp b/crates/sui-adapter-transactional-tests/tests/transfer_object/quasi_shared.exp index 3615a11d60783..a6d09e355ca1c 100644 --- a/crates/sui-adapter-transactional-tests/tests/transfer_object/quasi_shared.exp +++ b/crates/sui-adapter-transactional-tests/tests/transfer_object/quasi_shared.exp @@ -17,7 +17,7 @@ written: object(107), object(108) task 4 'view-object'. lines 33-33: Owner: Object ID: ( fake(107) ) -Version: 1 +Version: 2 Contents: sui::dynamic_field::Field, sui::object::ID> {id: sui::object::UID {id: sui::object::ID {bytes: fake(109)}}, name: sui::dynamic_object_field::Wrapper {name: 0u64}, value: std::option::Option {vec: vector[sui::object::ID {bytes: fake(110)}]}} task 5 'transfer-object'. lines 35-35: @@ -26,5 +26,5 @@ Execution Error: ExecutionError: ExecutionError { inner: ExecutionErrorInner { k task 6 'view-object'. lines 37-37: Owner: Object ID: ( fake(107) ) -Version: 2 +Version: 3 Contents: sui::dynamic_field::Field, sui::object::ID> {id: sui::object::UID {id: sui::object::ID {bytes: fake(109)}}, name: sui::dynamic_object_field::Wrapper {name: 0u64}, value: std::option::Option {vec: vector[sui::object::ID {bytes: fake(110)}]}} diff --git a/crates/sui-adapter-transactional-tests/tests/transfer_object/wrap_unwrap.exp b/crates/sui-adapter-transactional-tests/tests/transfer_object/wrap_unwrap.exp index 3c1efa403d522..d12e36f704d4c 100644 --- a/crates/sui-adapter-transactional-tests/tests/transfer_object/wrap_unwrap.exp +++ b/crates/sui-adapter-transactional-tests/tests/transfer_object/wrap_unwrap.exp @@ -23,7 +23,7 @@ deleted: object(106) task 5 'view-object'. lines 49-49: Owner: Account Address ( A ) -Version: 1 +Version: 2 Contents: a::m::T {id: sui::object::UID {id: sui::object::ID {bytes: fake(108)}}, s: a::m::S {id: sui::object::UID {id: sui::object::ID {bytes: fake(106)}}}} task 6 'run'. lines 51-51: @@ -32,5 +32,5 @@ deleted: object(108) task 7 'view-object'. lines 53-53: Owner: Account Address ( A ) -Version: 2 +Version: 3 Contents: a::m::S {id: sui::object::UID {id: sui::object::ID {bytes: fake(106)}}} diff --git a/crates/sui-adapter/src/adapter.rs b/crates/sui-adapter/src/adapter.rs index 4ceffe3c3bbb7..a1bb3c092a984 100644 --- a/crates/sui-adapter/src/adapter.rs +++ b/crates/sui-adapter/src/adapter.rs @@ -563,7 +563,7 @@ fn process_successful_execution( obj.data .try_as_move_mut() .expect("We previously checked that mutable ref inputs are Move objects") - .update_contents_and_increment_version(new_contents)?; + .update_contents(new_contents)?; changes.insert( obj_id, @@ -572,7 +572,7 @@ fn process_successful_execution( } let tx_digest = ctx.digest(); - for (id, (write_kind, mut recipient, tag, abilities, contents)) in writes { + for (id, (write_kind, recipient, tag, abilities, contents)) in writes { let has_public_transfer = abilities.has_store(); debug_assert_eq!( id, @@ -590,73 +590,28 @@ fn process_successful_execution( old_object_opt.is_none() || loaded_child_version_opt.is_none(), format!("Loaded {id} as a child, but that object was an input object") ); + let old_obj_ver = old_object_opt .map(|(_, version)| *version) .or_else(|| loaded_child_version_opt.copied()); - let (write_kind, version) = match (write_kind, old_obj_ver) { - (_, Some(version)) => { - debug_assert!(write_kind == WriteKind::Mutate); - (WriteKind::Mutate, version) - } - // When an object was wrapped at version `v`, we added a record into `parent_sync` - // with version `v+1` along with OBJECT_DIGEST_WRAPPED. Now when the object is unwrapped, - // it will also have version `v+1`, leading to a violation of the invariant that any - // object_id and version pair must be unique. We use the version from parent_sync and - // increment it (below), so we will have `(v+1)+1`, thus preserving the uniqueness - (WriteKind::Unwrap, None) => match state_view.get_latest_parent_entry_ref(id) { - Ok(Some((_, last_version, _))) => (WriteKind::Unwrap, last_version), - // if the object is not in parent sync, it was wrapped before ever being stored into - // storage. - // we set is_unwrapped to false since the object has never been in storage - // and essentially is being created. Similarly, we add it to the newly_generated_ids - // set - Ok(None) => (WriteKind::Create, SequenceNumber::new()), - _ => { - return Err(ExecutionError::new_with_source( - ExecutionErrorKind::InvariantViolation, - missing_unwrapped_msg(&id), - )); - } - }, - (_, None) => { - debug_assert!(write_kind == WriteKind::Create); - (WriteKind::Create, SequenceNumber::new()) - } - }; + + debug_assert!((write_kind == WriteKind::Mutate) == old_obj_ver.is_some()); + // safe because `has_public_transfer` was properly determined from the abilities - let mut move_obj = - unsafe { MoveObject::new_from_execution(tag, has_public_transfer, version, contents)? }; + let move_obj = unsafe { + MoveObject::new_from_execution( + tag, + has_public_transfer, + old_obj_ver.unwrap_or_else(SequenceNumber::new), + contents, + )? + }; #[cfg(debug_assertions)] { check_transferred_object_invariants(&move_obj, &old_obj_ver) } - // increment the object version. note that if the transferred object was - // freshly created, this means that its version will now be 1. - // thus, all objects in the global object pool have version > 0 - move_obj.increment_version(); - - // Remember the version this object was shared at, if this write was the one that shared it. - if let Owner::Shared { - initial_shared_version, - } = &mut recipient - { - assert_invariant!( - old_obj_ver.is_none(), - "The object should be guaranteed to be new by \ - sui::transfer::share_object, which aborts if it is not new" - ); - // TODO Consider a distinct Recipient enum within ObjectRuntime to enforce this - // invariant at the type level. - assert_eq!( - *initial_shared_version, - SequenceNumber::new(), - "Initial version should be blank before this point", - ); - *initial_shared_version = move_obj.version(); - } - let obj = Object::new_move(move_obj, recipient, tx_digest); if old_obj_ver.is_none() { // Charge extra gas based on object size if we are creating a new object. diff --git a/crates/sui-cluster-test/src/test_case/call_contract_test.rs b/crates/sui-cluster-test/src/test_case/call_contract_test.rs index e19342032d6fc..de2414dc5fc3b 100644 --- a/crates/sui-cluster-test/src/test_case/call_contract_test.rs +++ b/crates/sui-cluster-test/src/test_case/call_contract_test.rs @@ -11,7 +11,6 @@ use tracing::info; use sui::client_commands::{EXAMPLE_NFT_DESCRIPTION, EXAMPLE_NFT_NAME, EXAMPLE_NFT_URL}; use sui_json::SuiJsonValue; use sui_json_rpc_types::SuiEvent; -use sui_types::base_types::SequenceNumber; use sui_types::id::ID; use sui_types::{ base_types::{ObjectID, SuiAddress}, @@ -79,22 +78,28 @@ impl TestCaseImpl for CallContractTest { events.len() ); - events + let new_object_version = events .iter() - .find(|e| { - matches!(e, SuiEvent::NewObject { + .find_map(|e| match e { + SuiEvent::NewObject { package_id, transaction_module, - sender, recipient, object_type, object_id, version - } if - package_id == &SUI_FRAMEWORK_OBJECT_ID + sender, + recipient, + object_type, + object_id, + version, + } if package_id == &SUI_FRAMEWORK_OBJECT_ID && transaction_module == &String::from("devnet_nft") && sender == &signer && recipient == &Owner::AddressOwner(signer) - && object_id == &nft_id && object_type == "0x2::devnet_nft::DevNetNFT" - && version == &SequenceNumber::from_u64(1) - ) + && object_id == &nft_id => + { + Some(*version) + } + + _ => None, }) .unwrap_or_else(|| panic!("Expect such a NewObject in events {:?}", events)); @@ -123,8 +128,7 @@ impl TestCaseImpl for CallContractTest { .await; assert_eq!( - object.reference.version, - SequenceNumber::from_u64(1), + object.reference.version, new_object_version, "Expect sequence number to be 1" ); diff --git a/crates/sui-cluster-test/src/test_case/shared_object_test.rs b/crates/sui-cluster-test/src/test_case/shared_object_test.rs index 29c32f8e77f8e..0446201b1d871 100644 --- a/crates/sui-cluster-test/src/test_case/shared_object_test.rs +++ b/crates/sui-cluster-test/src/test_case/shared_object_test.rs @@ -5,7 +5,6 @@ use crate::{helper::ObjectChecker, TestCaseImpl, TestContext}; use async_trait::async_trait; use sui::client_commands::WalletContext; use sui_json_rpc_types::SuiExecutionStatus; -use sui_types::base_types::SequenceNumber; use sui_types::object::Owner; use test_utils::transaction::{increment_counter, publish_basics_package_and_make_counter}; use tracing::info; @@ -30,7 +29,7 @@ impl TestCaseImpl for SharedCounterTest { let wallet_context: &WalletContext = ctx.get_wallet(); let address = ctx.get_wallet_address(); - let (package_ref, (counter_id, counter_version, _)) = + let (package_ref, (counter_id, initial_counter_version, _)) = publish_basics_package_and_make_counter(wallet_context, address).await; let (tx_cert, effects) = increment_counter(wallet_context, address, None, package_ref, counter_id).await; @@ -40,26 +39,44 @@ impl TestCaseImpl for SharedCounterTest { "Increment counter txn failed: {:?}", effects.status ); + effects .shared_objects .iter() .find(|o| o.object_id == counter_id) .expect("Expect obj {counter_id} in shared_objects"); + let counter_version = effects + .mutated + .iter() + .find_map(|obj| { + let Owner::Shared { initial_shared_version } = obj.owner else { + return None + }; + + if obj.reference.object_id == counter_id + && initial_shared_version == initial_counter_version + { + Some(obj.reference.version) + } else { + None + } + }) + .expect("Expect obj {counter_id} in mutated"); + // Verify fullnode observes the txn ctx.let_fullnode_sync(vec![tx_cert.transaction_digest], 5) .await; let counter_object = ObjectChecker::new(counter_id) .owner(Owner::Shared { - initial_shared_version: counter_version, + initial_shared_version: initial_counter_version, }) .check_into_object(ctx.get_fullnode_client()) .await; assert_eq!( - counter_object.reference.version, - SequenceNumber::from_u64(2), + counter_object.reference.version, counter_version, "Expect sequence number to be 2" ); diff --git a/crates/sui-core/src/authority/authority_store.rs b/crates/sui-core/src/authority/authority_store.rs index 1bce9ebe04e1c..368d5ca7d7d34 100644 --- a/crates/sui-core/src/authority/authority_store.rs +++ b/crates/sui-core/src/authority/authority_store.rs @@ -735,15 +735,51 @@ impl Deserialize<'de>> SuiDataStore { let transaction_digest = certificate.digest(); let mut temporary_store = TemporaryStore::new(Arc::new(&self), input_objects, *transaction_digest); - for (_, (object, kind)) in mutated_objects { + + // TemporaryStore::into_inner will increment object versions, and requires that this + // operation is a true increment. Roll back modified objects' versions so that this remains + // the case. + let prev_versions: HashMap = effects + .data() + .modified_at_versions + .iter() + .copied() + .collect(); + + for (_, (mut object, kind)) in mutated_objects { + let prev_version = match kind { + WriteKind::Create => SequenceNumber::new(), + WriteKind::Mutate | WriteKind::Unwrap => prev_versions[&object.id()], + }; + + if let Owner::Shared { + initial_shared_version, + } = &mut object.owner + { + if WriteKind::Create == kind { + *initial_shared_version = SequenceNumber::new(); + } + } + + if let Some(object) = object.data.try_as_move_mut() { + object.decrement_version_to(prev_version); + } + temporary_store.write_object(&ctx, object, kind); } + for obj_ref in &effects.data().deleted { - temporary_store.delete_object(&ctx, &obj_ref.0, obj_ref.1, DeleteKind::Normal); + let mut v = obj_ref.1; + v.decrement_to(prev_versions[&obj_ref.0]); + temporary_store.delete_object(&ctx, &obj_ref.0, v, DeleteKind::Normal); } + for obj_ref in &effects.data().wrapped { - temporary_store.delete_object(&ctx, &obj_ref.0, obj_ref.1, DeleteKind::Wrap); + let mut v = obj_ref.1; + v.decrement_to(prev_versions[&obj_ref.0]); + temporary_store.delete_object(&ctx, &obj_ref.0, v, DeleteKind::Wrap); } + let (inner_temporary_store, _events) = temporary_store.into_inner(); let mut write_batch = self.perpetual_tables.certificates.batch(); @@ -999,6 +1035,7 @@ impl Deserialize<'de>> SuiDataStore { /// 4. owner_index table change is reverted. pub async fn revert_state_update(&self, tx_digest: &TransactionDigest) -> SuiResult { let effects = self.get_effects(tx_digest)?; + let mut write_batch = self.perpetual_tables.certificates.batch(); write_batch = write_batch.delete_batch(&self.perpetual_tables.certificates, iter::once(tx_digest))?; @@ -1040,43 +1077,36 @@ impl Deserialize<'de>> SuiDataStore { .map(|((id, _, _), owner)| (*owner, *id)); write_batch = write_batch.delete_batch(&self.perpetual_tables.owner_index, owners_to_delete)?; - let mutated_objects = effects - .mutated + + let modified_object_keys = effects + .modified_at_versions .iter() - .map(|(r, _)| r) - .chain(effects.deleted.iter()) - .chain(effects.wrapped.iter()) - .map(|(id, version, _)| { - ObjectKey( - *id, - version - .decrement() - .expect("version revert should never fail"), - ) - }); - let (old_objects, old_locks): (Vec<_>, Vec<_>) = self + .map(|(id, version)| ObjectKey(*id, *version)); + + let (old_modified_objects, old_locks): (Vec<_>, Vec<_>) = self .perpetual_tables .objects - .multi_get(mutated_objects)? + .multi_get(modified_object_keys)? .into_iter() - .map(|obj_opt| { + .filter_map(|obj_opt| { let obj = obj_opt.expect("Older object version not found"); + + if obj.is_immutable() { + return None; + } + let obj_ref = obj.compute_object_reference(); - let lock = if obj.is_address_owned() { - Some(obj_ref) - } else { - None - }; - ( + Some(( ((obj.owner, obj.id()), ObjectInfo::new(&obj_ref, &obj)), - lock, - ) + obj.is_address_owned().then_some(obj_ref), + )) }) .unzip(); let old_locks: Vec<_> = old_locks.into_iter().flatten().collect(); - write_batch = write_batch.insert_batch(&self.perpetual_tables.owner_index, old_objects)?; + write_batch = + write_batch.insert_batch(&self.perpetual_tables.owner_index, old_modified_objects)?; write_batch.write()?; @@ -1227,8 +1257,8 @@ impl Deserialize<'de>> SuiDataStore { let ids = certificate.shared_input_objects().map(|(id, _)| id); let versions = self.epoch_tables().multi_get_next_object_versions(ids)?; + let mut input_object_keys = certificate_input_object_keys(certificate)?; let mut sequenced_to_write = Vec::new(); - let mut schedule_to_write = Vec::new(); for ((id, initial_shared_version), v) in certificate.shared_input_objects().zip(versions.iter()) { @@ -1251,12 +1281,19 @@ impl Deserialize<'de>> SuiDataStore { .unwrap_or_default(), ), }; - let next_version = version.increment(); sequenced_to_write.push(((transaction_digest, *id), version)); - schedule_to_write.push((*id, next_version)); + input_object_keys.push(ObjectKey(*id, version)); } + let next_version = + SequenceNumber::lamport_increment(input_object_keys.iter().map(|obj| obj.1)); + + let schedule_to_write: Vec<_> = sequenced_to_write + .iter() + .map(|((_, id), _)| (*id, next_version)) + .collect(); + trace!(tx_digest = ?transaction_digest, ?sequenced_to_write, ?schedule_to_write, "locking shared objects"); @@ -1557,3 +1594,21 @@ impl EffectsStore for Arc { .collect()) } } + +/// Fetch the `ObjectKey`s (IDs and versions) for non-shared input objects. Includes owned, +/// and immutable objects as well as the gas objects, but not move packages or shared objects. +fn certificate_input_object_keys(certificate: &VerifiedCertificate) -> SuiResult> { + Ok(certificate + .data() + .data + .input_objects()? + .into_iter() + .filter_map(|object| { + use InputObjectKind::*; + match object { + MovePackage(_) | SharedMoveObject { .. } => None, + ImmOrOwnedMoveObject(obj) => Some(obj.into()), + } + }) + .collect()) +} diff --git a/crates/sui-core/src/execution_engine.rs b/crates/sui-core/src/execution_engine.rs index 37827119b1eef..737e49cbd7ba6 100644 --- a/crates/sui-core/src/execution_engine.rs +++ b/crates/sui-core/src/execution_engine.rs @@ -5,6 +5,7 @@ use std::{collections::BTreeSet, sync::Arc}; use move_core_types::language_storage::{ModuleId, StructTag}; use move_vm_runtime::{move_vm::MoveVM, native_functions::NativeFunctionTable}; +use sui_types::base_types::SequenceNumber; use tracing::{debug, instrument}; use sui_adapter::adapter; @@ -19,7 +20,7 @@ use sui_types::messages::ExecutionFailureStatus; #[cfg(test)] use sui_types::messages::InputObjects; use sui_types::messages::{ObjectArg, Pay, PayAllSui, PaySui}; -use sui_types::object::{Data, MoveObject, Owner, OBJECT_START_VERSION}; +use sui_types::object::{Data, MoveObject, Owner}; use sui_types::storage::SingleTxContext; use sui_types::storage::{ChildObjectResolver, DeleteKind, ParentSync, WriteKind}; #[cfg(test)] @@ -314,7 +315,7 @@ fn transfer_object( recipient: SuiAddress, ) -> Result<(), ExecutionError> { object.ensure_public_transfer_eligible()?; - object.transfer_and_increment_version(recipient); + object.transfer(recipient); // This will extract the transfer amount if the object is a Coin of some kind let ctx = SingleTxContext::transfer_object(sender); temporary_store.write_object(&ctx, object, WriteKind::Mutate); @@ -429,7 +430,7 @@ fn debit_coins_and_transfer( let new_coin = Object::new_move( MoveObject::new_coin( coin_type.clone(), - OBJECT_START_VERSION, + SequenceNumber::new(), tx_ctx.fresh_id(), *amount, ), @@ -494,9 +495,7 @@ fn pay( // unwrap safe because we checked that it was a coin object above let move_obj = coin_object.data.try_as_move_mut().unwrap(); // unwrap safe because coin object is always below maximum size - move_obj - .update_contents_and_increment_version(new_contents) - .unwrap(); + move_obj.update_contents(new_contents).unwrap(); temporary_store.write_object(&ctx, coin_object, WriteKind::Mutate); } } @@ -590,15 +589,13 @@ fn transfer_sui( .try_as_move_mut() .expect("Gas object must be Move object"); let new_contents = bcs::to_bytes(&gas_coin).expect("Serializing gas coin can never fail"); - // We do not update the version number yet because gas charge will update it later. // unwrap safe because coin object is always below maximum size - move_object - .update_contents_without_version_change(new_contents) - .unwrap(); + move_object.update_contents(new_contents).unwrap(); - // Creat a new gas coin with the amount. + // Creat a new gas coin with the amount. Set a blank version, to be updated by the store + // when it is committed to effects. let new_object = Object::new_move( - MoveObject::new_gas_coin(OBJECT_START_VERSION, tx_ctx.fresh_id(), amount), + MoveObject::new_gas_coin(SequenceNumber::new(), tx_ctx.fresh_id(), amount), Owner::AddressOwner(recipient), tx_ctx.digest(), ); @@ -606,8 +603,7 @@ fn transfer_sui( Some(amount) } else { // If amount is not specified, we simply transfer the entire coin object. - // We don't want to increment the version number yet because latter gas charge will do it. - object.transfer_without_version_change(recipient); + object.transfer(recipient); Coin::extract_balance_if_coin(&object)? }; diff --git a/crates/sui-core/src/test_utils.rs b/crates/sui-core/src/test_utils.rs index 0bc37f47e20c9..85489cec6a55f 100644 --- a/crates/sui-core/src/test_utils.rs +++ b/crates/sui-core/src/test_utils.rs @@ -170,6 +170,7 @@ pub fn dummy_transaction_effects(tx: &Transaction) -> TransactionEffects { storage_cost: 0, storage_rebate: 0, }, + modified_at_versions: Vec::new(), shared_objects: Vec::new(), transaction_digest: *tx.digest(), created: Vec::new(), diff --git a/crates/sui-core/src/transaction_input_checker.rs b/crates/sui-core/src/transaction_input_checker.rs index 734aa2d75e4e0..1ff5d654fd59d 100644 --- a/crates/sui-core/src/transaction_input_checker.rs +++ b/crates/sui-core/src/transaction_input_checker.rs @@ -254,10 +254,8 @@ fn check_one_object( !object.is_package(), SuiError::MovePackageAsObject { object_id } ); - // wrapped objects that are then deleted will be set to MAX, - // so we need to cap the sequence number at MAX - 1 fp_ensure!( - sequence_number < SequenceNumber::MAX.decrement().unwrap(), + sequence_number < SequenceNumber::MAX, SuiError::InvalidSequenceNumber ); diff --git a/crates/sui-core/src/unit_tests/authority_tests.rs b/crates/sui-core/src/unit_tests/authority_tests.rs index b4d82ec870a60..5d2864a3e10ea 100644 --- a/crates/sui-core/src/unit_tests/authority_tests.rs +++ b/crates/sui-core/src/unit_tests/authority_tests.rs @@ -422,6 +422,7 @@ async fn test_handle_transfer_transaction_ok() { let gas_object_id = ObjectID::random(); let authority_state = init_state_with_ids(vec![(sender, object_id), (sender, gas_object_id)]).await; + let object = authority_state .get_object(&object_id) .await @@ -432,6 +433,13 @@ async fn test_handle_transfer_transaction_ok() { .await .unwrap() .unwrap(); + + let before_object_version = object.version(); + let after_object_version = + SequenceNumber::lamport_increment([object.version(), gas_object.version()]); + + assert!(before_object_version < after_object_version); + let transfer_transaction = init_transfer_transaction( sender, &sender_key, @@ -440,20 +448,14 @@ async fn test_handle_transfer_transaction_ok() { gas_object.compute_object_reference(), ); - let test_object = authority_state - .get_object(&object_id) - .await - .unwrap() - .unwrap(); - // Check the initial state of the locks assert!(authority_state - .get_transaction_lock(&(object_id, 0.into(), test_object.digest()), 0) + .get_transaction_lock(&(object_id, before_object_version, object.digest()), 0) .await .unwrap() .is_none()); assert!(authority_state - .get_transaction_lock(&(object_id, 1.into(), test_object.digest()), 0) + .get_transaction_lock(&(object_id, after_object_version, object.digest()), 0) .await .is_err()); @@ -462,38 +464,26 @@ async fn test_handle_transfer_transaction_ok() { .await .unwrap(); - let object = authority_state - .get_object(&object_id) - .await - .unwrap() - .unwrap(); let pending_confirmation = authority_state .get_transaction_lock(&object.compute_object_reference(), 0) .await .unwrap() .unwrap(); + assert_eq!( account_info.signed_transaction.unwrap(), pending_confirmation ); // Check the final state of the locks - assert!(authority_state - .get_transaction_lock(&(object_id, 0.into(), object.digest()), 0) - .await - .unwrap() - .is_some()); - assert_eq!( - authority_state - .get_transaction_lock(&(object_id, 0.into(), object.digest()), 0) - .await - .unwrap() - .as_ref() - .unwrap() - .data() - .data, - transfer_transaction.data().data - ); + let Some(envelope) = authority_state.get_transaction_lock( + &(object_id, before_object_version, object.digest()), + 0, + ).await.unwrap() else { + panic!("No verified envelope for transaction"); + }; + + assert_eq!(envelope.data().data, transfer_transaction.data().data); } #[tokio::test] @@ -965,15 +955,12 @@ async fn test_handle_confirmation_transaction_bad_sequence_number() { .unwrap(); // Record the old sequence number - let old_seq_num; - { - let old_account = authority_state - .get_object(&object_id) - .await - .unwrap() - .unwrap(); - old_seq_num = old_account.version(); - } + let old_seq_num = authority_state + .get_object(&object_id) + .await + .unwrap() + .unwrap() + .version(); let certified_transfer_transaction = init_certified_transfer_transaction( sender, @@ -992,10 +979,8 @@ async fn test_handle_confirmation_transaction_bad_sequence_number() { .unwrap() .unwrap(); let o = sender_object.data.try_as_move_mut().unwrap(); - let old_contents = o.contents().to_vec(); - // update object contents, which will increment the sequence number - o.update_contents_and_increment_version(old_contents) - .unwrap(); + // increment the sequence number + o.increment_version_to(SequenceNumber::lamport_increment([o.version()])); authority_state.insert_genesis_object(sender_object).await; } @@ -1013,7 +998,7 @@ async fn test_handle_confirmation_transaction_bad_sequence_number() { .await .unwrap() .unwrap(); - assert_eq!(old_seq_num.increment(), new_object.version()); + assert_ne!(old_seq_num, new_object.version()); // No recipient object was created. assert!(authority_state.get_object(&dbg_object_id(2)).await.is_err()); @@ -1082,6 +1067,9 @@ async fn test_handle_confirmation_transaction_ok() { .unwrap() .unwrap(); + let next_sequence_number = + SequenceNumber::lamport_increment([object.version(), gas_object.version()]); + let certified_transfer_transaction = init_certified_transfer_transaction( sender, &sender_key, @@ -1096,8 +1084,6 @@ async fn test_handle_confirmation_transaction_ok() { .await .unwrap() .unwrap(); - let mut next_sequence_number = old_account.version(); - next_sequence_number = next_sequence_number.increment(); let info = authority_state .handle_certificate(&certified_transfer_transaction.clone()) @@ -1363,6 +1349,15 @@ async fn test_move_call_mutable_object_not_mutated() { assert_eq!((effects.created.len(), effects.mutated.len()), (1, 1)); let (new_object_id2, seq2, _) = effects.created[0].0; + let gas_version = authority_state + .get_object(&gas_object_id) + .await + .unwrap() + .unwrap() + .version(); + + let next_object_version = SequenceNumber::lamport_increment([gas_version, seq1, seq2]); + let effects = call_move( &authority_state, &gas_object_id, @@ -1389,7 +1384,7 @@ async fn test_move_call_mutable_object_not_mutated() { .unwrap() .unwrap() .version(), - seq1.increment() + next_object_version ); assert_eq!( authority_state @@ -1398,7 +1393,7 @@ async fn test_move_call_mutable_object_not_mutated() { .unwrap() .unwrap() .version(), - seq2.increment() + next_object_version ); } @@ -1448,6 +1443,7 @@ async fn test_move_call_insufficient_gas() { .unwrap() .into_data(); let gas_used = effects.gas_used.gas_used(); + let obj_ref = authority_state .get_object(&object_id) .await @@ -1455,19 +1451,17 @@ async fn test_move_call_insufficient_gas() { .unwrap() .compute_object_reference(); + let gas_ref = authority_state + .get_object(&gas_object_id2) + .await + .unwrap() + .unwrap() + .compute_object_reference(); + + let next_object_version = SequenceNumber::lamport_increment([obj_ref.1, gas_ref.1]); + // Now we try to construct a transaction with a smaller gas budget than required. - let data = TransactionData::new_transfer( - sender, - obj_ref, - recipient, - authority_state - .get_object(&gas_object_id2) - .await - .unwrap() - .unwrap() - .compute_object_reference(), - gas_used - 5, - ); + let data = TransactionData::new_transfer(sender, obj_ref, recipient, gas_ref, gas_used - 5); let transaction = to_sender_signed_transaction(data, &recipient_key); let tx_digest = *transaction.digest(); @@ -1482,7 +1476,7 @@ async fn test_move_call_insufficient_gas() { .unwrap() .unwrap(); assert_eq!(obj.previous_transaction, tx_digest); - assert_eq!(obj.version(), obj_ref.1.increment()); + assert_eq!(obj.version(), next_object_version); assert_eq!(obj.owner, recipient); } @@ -1584,7 +1578,7 @@ async fn test_get_latest_parent_entry() { ) .await .unwrap(); - let (new_object_id1, _seq1, _) = effects.created[0].0; + let (new_object_id1, seq1, _) = effects.created[0].0; let effects = create_move_object( &pkg_ref, @@ -1595,7 +1589,9 @@ async fn test_get_latest_parent_entry() { ) .await .unwrap(); - let (new_object_id2, _seq2, _) = effects.created[0].0; + let (new_object_id2, seq2, _) = effects.created[0].0; + + let update_version = SequenceNumber::lamport_increment([seq1, seq2, effects.gas_object.0 .1]); let effects = call_move( &authority_state, @@ -1621,9 +1617,11 @@ async fn test_get_latest_parent_entry() { .unwrap() .unwrap(); assert_eq!(obj_ref.0, new_object_id1); - assert_eq!(obj_ref.1, SequenceNumber::from(2)); + assert_eq!(obj_ref.1, update_version); assert_eq!(effects.transaction_digest, tx); + let delete_version = SequenceNumber::lamport_increment([obj_ref.1, effects.gas_object.0 .1]); + let effects = call_move( &authority_state, &gas_object_id, @@ -1659,7 +1657,7 @@ async fn test_get_latest_parent_entry() { .unwrap() .unwrap(); assert_eq!(obj_ref.0, gas_object_id); - assert_eq!(obj_ref.1, SequenceNumber::from(4)); + assert_eq!(obj_ref.1, delete_version); assert_eq!(effects.transaction_digest, tx); // Check entry for deleted object is returned @@ -1669,7 +1667,7 @@ async fn test_get_latest_parent_entry() { .unwrap() .unwrap(); assert_eq!(obj_ref.0, new_object_id1); - assert_eq!(obj_ref.1, SequenceNumber::from(3)); + assert_eq!(obj_ref.1, delete_version); assert_eq!(obj_ref.2, ObjectDigest::OBJECT_DIGEST_DELETED); assert_eq!(effects.transaction_digest, tx); } @@ -1793,7 +1791,7 @@ async fn test_idempotent_reversed_confirmation() { } #[tokio::test] -async fn test_genesis_sui_sysmtem_state_object() { +async fn test_genesis_sui_system_state_object() { // This test verifies that we can read the genesis SuiSystemState object. // And its Move layout matches the definition in Rust (so that we can deserialize it). let authority_state = init_state().await; @@ -1817,13 +1815,8 @@ async fn test_transfer_sui_no_amount() { let init_balance = sui_types::gas::get_gas_balance(&gas_object).unwrap(); let authority_state = init_state_with_objects(vec![gas_object.clone()]).await; - let tx_data = TransactionData::new_transfer_sui( - recipient, - sender, - None, - gas_object.compute_object_reference(), - MAX_GAS, - ); + let gas_ref = gas_object.compute_object_reference(); + let tx_data = TransactionData::new_transfer_sui(recipient, sender, None, gas_ref, MAX_GAS); // Make sure transaction handling works as usual. let transaction = to_sender_signed_transaction(tx_data, &sender_key); @@ -1842,7 +1835,7 @@ async fn test_transfer_sui_no_amount() { // and got transferred. Also check on its version and new balance. assert!(effects.status.is_ok()); assert!(effects.mutated_excluding_gas().next().is_none()); - assert_eq!(effects.gas_object.0 .1, SequenceNumber::new().increment()); + assert!(gas_ref.1 < effects.gas_object.0 .1); assert_eq!(effects.gas_object.1, Owner::AddressOwner(recipient)); let new_balance = sui_types::gas::get_gas_balance( &authority_state @@ -1867,13 +1860,8 @@ async fn test_transfer_sui_with_amount() { let init_balance = sui_types::gas::get_gas_balance(&gas_object).unwrap(); let authority_state = init_state_with_objects(vec![gas_object.clone()]).await; - let tx_data = TransactionData::new_transfer_sui( - recipient, - sender, - Some(500), - gas_object.compute_object_reference(), - MAX_GAS, - ); + let gas_ref = gas_object.compute_object_reference(); + let tx_data = TransactionData::new_transfer_sui(recipient, sender, Some(500), gas_ref, MAX_GAS); let transaction = to_sender_signed_transaction(tx_data, &sender_key); let certificate = init_certified_transaction(transaction, &authority_state); let response = authority_state @@ -1893,7 +1881,7 @@ async fn test_transfer_sui_with_amount() { .unwrap() .unwrap(); assert_eq!(sui_types::gas::get_gas_balance(&new_gas).unwrap(), 500); - assert_eq!(effects.gas_object.0 .1, SequenceNumber::new().increment()); + assert!(gas_ref.1 < effects.gas_object.0 .1); assert_eq!(effects.gas_object.1, Owner::AddressOwner(sender)); let new_balance = sui_types::gas::get_gas_balance( &authority_state 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 a551067e83b1c..e246915e55919 100644 --- a/crates/sui-core/src/unit_tests/move_integration_tests.rs +++ b/crates/sui-core/src/unit_tests/move_integration_tests.rs @@ -21,7 +21,6 @@ use sui_types::{ error::SuiError, event::{Event, EventType}, messages::ExecutionStatus, - object::OBJECT_START_VERSION, }; use std::path::PathBuf; @@ -62,6 +61,9 @@ fn test_object_wrapping_unwrapping() { ) .await; + let gas_version = authority.get_object(&gas).await.unwrap().unwrap().version(); + let create_child_version = SequenceNumber::lamport_increment([gas_version]); + // Create a Child object. let effects = call_move( &authority, @@ -82,7 +84,10 @@ fn test_object_wrapping_unwrapping() { effects.status ); let child_object_ref = effects.created[0].0; - assert_eq!(child_object_ref.1, OBJECT_START_VERSION); + assert_eq!(child_object_ref.1, create_child_version); + + let wrapped_version = + SequenceNumber::lamport_increment([child_object_ref.1, effects.gas_object.0 .1]); // Create a Parent object, by wrapping the child object. let effects = call_move( @@ -115,16 +120,18 @@ fn test_object_wrapping_unwrapping() { let new_child_object_ref = effects.wrapped[0]; let expected_child_object_ref = ( child_object_ref.0, - child_object_ref.1.increment(), + wrapped_version, ObjectDigest::OBJECT_DIGEST_WRAPPED, ); // Make sure that the child's version gets increased after wrapped. assert_eq!(new_child_object_ref, expected_child_object_ref); check_latest_object_ref(&authority, &expected_child_object_ref).await; - let child_object_ref = new_child_object_ref; let parent_object_ref = effects.created[0].0; - assert_eq!(parent_object_ref.1, OBJECT_START_VERSION); + assert_eq!(parent_object_ref.1, wrapped_version); + + let unwrapped_version = + SequenceNumber::lamport_increment([parent_object_ref.1, effects.gas_object.0 .1]); // Extract the child out of the parent. let effects = call_move( @@ -156,10 +163,16 @@ fn test_object_wrapping_unwrapping() { (2, 0, 1) ); // Make sure that version increments again when unwrapped. - assert_eq!(effects.unwrapped[0].0 .1, child_object_ref.1.increment()); + assert_eq!(effects.unwrapped[0].0 .1, unwrapped_version); check_latest_object_ref(&authority, &effects.unwrapped[0].0).await; let child_object_ref = effects.unwrapped[0].0; + let rewrap_version = SequenceNumber::lamport_increment([ + parent_object_ref.1, + child_object_ref.1, + effects.gas_object.0 .1, + ]); + // Wrap the child to the parent again. let effects = call_move( &authority, @@ -187,7 +200,7 @@ fn test_object_wrapping_unwrapping() { assert_eq!((effects.mutated.len(), effects.wrapped.len()), (2, 1)); let expected_child_object_ref = ( child_object_ref.0, - child_object_ref.1.increment(), + rewrap_version, ObjectDigest::OBJECT_DIGEST_WRAPPED, ); assert_eq!(effects.wrapped[0], expected_child_object_ref); @@ -195,6 +208,9 @@ fn test_object_wrapping_unwrapping() { let child_object_ref = effects.wrapped[0]; let parent_object_ref = effects.mutated_excluding_gas().next().unwrap().0; + let deleted_version = + SequenceNumber::lamport_increment([parent_object_ref.1, effects.gas_object.0 .1]); + // Now delete the parent object, which will in turn delete the child object. let effects = call_move( &authority, @@ -215,17 +231,17 @@ fn test_object_wrapping_unwrapping() { effects.status ); assert_eq!(effects.deleted.len(), 2); - // Check that both objects are marked as wrapped in the authority. + // Check that both objects are marked as deleted in the authority. let expected_child_object_ref = ( child_object_ref.0, - child_object_ref.1.increment(), + deleted_version, ObjectDigest::OBJECT_DIGEST_DELETED, ); assert!(effects.deleted.contains(&expected_child_object_ref)); check_latest_object_ref(&authority, &expected_child_object_ref).await; let expected_parent_object_ref = ( parent_object_ref.0, - parent_object_ref.1.increment(), + deleted_version, ObjectDigest::OBJECT_DIGEST_DELETED, ); assert!(effects.deleted.contains(&expected_parent_object_ref)); diff --git a/crates/sui-framework/src/natives/transfer.rs b/crates/sui-framework/src/natives/transfer.rs index 8538acda9665c..a90ef26fdc697 100644 --- a/crates/sui-framework/src/natives/transfer.rs +++ b/crates/sui-framework/src/natives/transfer.rs @@ -76,8 +76,8 @@ pub fn share_object( let obj = args.pop_back().unwrap(); let transfer_result = object_runtime_transfer( context, - // Dummy version, to be filled with the correct initial version when the transaction is - // finalized. + // Dummy version, to be filled with the correct initial version when the effects of the + // transaction are written to storage. Owner::Shared { initial_shared_version: SequenceNumber::new(), }, diff --git a/crates/sui-storage/src/lock_service.rs b/crates/sui-storage/src/lock_service.rs index 5fbde5bf9b8a0..a53b1e89e8c50 100644 --- a/crates/sui-storage/src/lock_service.rs +++ b/crates/sui-storage/src/lock_service.rs @@ -988,7 +988,7 @@ mod tests { ); // Initialize the object's entry to another version - let new_ref2 = (ref2.0, ref2.1.increment(), ref2.2); + let new_ref2 = (ref2.0, SequenceNumber::lamport_increment([ref2.1]), ref2.2); ls.initialize_locks(&[new_ref2], false /* is_force_reset */) .unwrap(); diff --git a/crates/sui-transactional-test-runner/src/test_adapter.rs b/crates/sui-transactional-test-runner/src/test_adapter.rs index 1372cc91f75a5..07449209283c9 100644 --- a/crates/sui-transactional-test-runner/src/test_adapter.rs +++ b/crates/sui-transactional-test-runner/src/test_adapter.rs @@ -500,33 +500,43 @@ impl<'a> SuiTestAdapter<'a> { // TODO: Support different epochs in transactional tests. 0, ); - let created_set: BTreeSet<_> = created.iter().map(|((id, _, _), _)| *id).collect(); - let mut created_ids: Vec<_> = created_set.iter().copied().collect(); - let mut written_ids: Vec<_> = mutated - .iter() - .chain(&unwrapped) - .map(|((id, _, _), _)| *id) - .collect(); + + let mut created_ids: Vec<_> = created.iter().map(|((id, _, _), _)| *id).collect(); + let unwrapped_ids: Vec<_> = unwrapped.iter().map(|((id, _, _), _)| *id).collect(); + let mut written_ids: Vec<_> = mutated.iter().map(|((id, _, _), _)| *id).collect(); let mut deleted_ids: Vec<_> = deleted .iter() .chain(&wrapped) .map(|(id, _, _)| *id) .collect(); + // update storage Arc::get_mut(&mut self.storage) .unwrap() .finish(inner.written, inner.deleted); - // enumerate objects after written to storage, sort by a "stable" sorting as the - // object ID is not stable - let mut created_ids_vec = created_set.iter().collect::>(); - created_ids_vec.sort_by_key(|id| self.get_object_sorting_key(id)); - for id in created_ids_vec { - self.enumerate_fake(*id); + + // make sure objects that have previously not been in storage get assigned a fake id. + let mut might_need_fake_id: Vec<_> = created_ids + .iter() + .chain(unwrapped_ids.iter()) + .copied() + .collect(); + + // Use a stable sort before assigning fake ids, so test output remains stable. + might_need_fake_id.sort_by_key(|id| self.get_object_sorting_key(id)); + for id in might_need_fake_id { + self.enumerate_fake(id); } + + // Treat unwrapped objects as writes (even though sometimes this is the first time we can + // refer to them at their id in storage). + written_ids.extend(unwrapped_ids.into_iter()); + // sort by fake id created_ids.sort_by_key(|id| self.real_to_fake_object_id(id)); written_ids.sort_by_key(|id| self.real_to_fake_object_id(id)); deleted_ids.sort_by_key(|id| self.real_to_fake_object_id(id)); + match status { ExecutionStatus::Success { .. } => Ok(TxnSummary { created: created_ids, diff --git a/crates/sui-types/src/base_types.rs b/crates/sui-types/src/base_types.rs index 5cdfe6fbbf173..92dcc2e41361e 100644 --- a/crates/sui-types/src/base_types.rs +++ b/crates/sui-types/src/base_types.rs @@ -17,6 +17,7 @@ use serde_with::serde_as; use serde_with::Bytes; use sha2::Sha512; use std::borrow::Borrow; +use std::cmp::max; use std::collections::HashMap; use std::convert::{TryFrom, TryInto}; use std::fmt; @@ -701,22 +702,29 @@ impl SequenceNumber { SequenceNumber(u) } + pub fn increment_to(&mut self, next: SequenceNumber) { + debug_assert!(*self < next, "Not an increment: {} to {}", self, next); + *self = next; + } + + pub fn decrement_to(&mut self, prev: SequenceNumber) { + debug_assert!(prev < *self, "Not a decrement: {} to {}", self, prev); + *self = prev; + } + + /// Returns a new sequence number that is greater than all `SequenceNumber`s in `inputs`, + /// assuming this operation will not overflow. #[must_use] - pub fn increment(self) -> SequenceNumber { - // TODO: Ensure this never overflow. + pub fn lamport_increment(inputs: impl IntoIterator) -> SequenceNumber { + let max_input = inputs.into_iter().fold(SequenceNumber::new(), max); + + // TODO: Ensure this never overflows. // Option 1: Freeze the object when sequence number reaches MAX. // Option 2: Reject tx with MAX sequence number. // Issue #182. - debug_assert_ne!(self.0, u64::MAX); - Self(self.0 + 1) - } + assert_ne!(max_input.0, u64::MAX); - pub fn decrement(self) -> Result { - let val = self.0.checked_sub(1); - match val { - None => Err(SuiError::SequenceUnderflow), - Some(val) => Ok(Self(val)), - } + SequenceNumber(max_input.0 + 1) } } diff --git a/crates/sui-types/src/coin.rs b/crates/sui-types/src/coin.rs index 3aae484711e2c..d462d52a737fa 100644 --- a/crates/sui-types/src/coin.rs +++ b/crates/sui-types/src/coin.rs @@ -9,7 +9,8 @@ use move_core_types::{ }; use serde::{Deserialize, Serialize}; -use crate::object::{MoveObject, Owner, OBJECT_START_VERSION}; +use crate::base_types::SequenceNumber; +use crate::object::{MoveObject, Owner}; use crate::storage::{DeleteKind, SingleTxContext, WriteKind}; use crate::temporary_store::TemporaryStore; use crate::{ @@ -157,7 +158,7 @@ pub fn transfer_coin( previous_transaction: TransactionDigest, ) { let new_coin = Object::new_move( - MoveObject::new_coin(coin_type, OBJECT_START_VERSION, *coin.id(), coin.value()), + MoveObject::new_coin(coin_type, SequenceNumber::new(), *coin.id(), coin.value()), Owner::AddressOwner(recipient), previous_transaction, ); @@ -177,15 +178,11 @@ pub fn update_input_coins( let mut gas_coin_obj = coin_objects.remove(0); let new_contents = bcs::to_bytes(gas_coin).expect("Coin serialization should not fail"); // unwrap is safe because we checked that it was a coin object above. - // update_contents_without_version_change b/c this is the gas coin, - // whose version will be bumped upon gas payment. let move_obj = gas_coin_obj.data.try_as_move_mut().unwrap(); // unwrap is safe because size of coin contents should never change - move_obj - .update_contents_without_version_change(new_contents) - .unwrap(); + move_obj.update_contents(new_contents).unwrap(); if let Some(recipient) = recipient { - gas_coin_obj.transfer_without_version_change(recipient); + gas_coin_obj.transfer(recipient); } temporary_store.write_object(ctx, gas_coin_obj, WriteKind::Mutate); diff --git a/crates/sui-types/src/gas.rs b/crates/sui-types/src/gas.rs index 470ae4d082ca7..e9fa4713deb96 100644 --- a/crates/sui-types/src/gas.rs +++ b/crates/sui-types/src/gas.rs @@ -471,9 +471,7 @@ pub fn deduct_gas(gas_object: &mut Object, deduct_amount: u64, rebate_amount: u6 let new_contents = bcs::to_bytes(&new_gas_coin).unwrap(); assert_eq!(move_object.contents().len(), new_contents.len()); // unwrap safe gas object cannot exceed max object size - move_object - .update_contents_and_increment_version(new_contents) - .unwrap(); + move_object.update_contents(new_contents).unwrap(); } pub fn refund_gas(gas_object: &mut Object, amount: u64) { @@ -485,9 +483,7 @@ pub fn refund_gas(gas_object: &mut Object, amount: u64) { // unwrap safe because GasCoin is guaranteed to serialize let new_contents = bcs::to_bytes(&new_gas_coin).unwrap(); // unwrap because safe gas object cannot exceed max object size - move_object - .update_contents_and_increment_version(new_contents) - .unwrap(); + move_object.update_contents(new_contents).unwrap(); } pub fn get_gas_balance(gas_object: &Object) -> SuiResult { diff --git a/crates/sui-types/src/messages.rs b/crates/sui-types/src/messages.rs index b8bb046c1d2b6..c85420c46bf0e 100644 --- a/crates/sui-types/src/messages.rs +++ b/crates/sui-types/src/messages.rs @@ -14,7 +14,7 @@ use crate::message_envelope::{Envelope, Message, TrustedEnvelope, VerifiedEnvelo use crate::messages_checkpoint::{ AuthenticatedCheckpoint, CheckpointSequenceNumber, CheckpointSignatureMessage, }; -use crate::object::{Object, ObjectFormatOptions, Owner, OBJECT_START_VERSION}; +use crate::object::{MoveObject, Object, ObjectFormatOptions, Owner, PACKAGE_VERSION}; use crate::storage::{DeleteKind, WriteKind}; use crate::{SUI_SYSTEM_STATE_OBJECT_ID, SUI_SYSTEM_STATE_OBJECT_SHARED_VERSION}; use byteorder::{BigEndian, ReadBytesExt}; @@ -1722,27 +1722,33 @@ impl From for ExecutionFailureStatus { /// The response from processing a transaction or a certified transaction #[derive(Eq, PartialEq, Clone, Debug, Serialize, Deserialize)] pub struct TransactionEffects { - // The status of the execution + /// The status of the execution pub status: ExecutionStatus, pub gas_used: GasCostSummary, - // The object references of the shared objects used in this transaction. Empty if no shared objects were used. + /// The version that every modified (mutated or deleted) object had before it was modified by + /// this transaction. + pub modified_at_versions: Vec<(ObjectID, SequenceNumber)>, + /// The object references of the shared objects used in this transaction. Empty if no shared objects were used. pub shared_objects: Vec, - // The transaction digest + /// The transaction digest pub transaction_digest: TransactionDigest, - // ObjectRef and owner of new objects created. + + // TODO: All the SequenceNumbers in the ObjectRefs below equal the same value (the lamport + // timestamp of the transaction). Consider factoring this out into one place in the effects. + /// ObjectRef and owner of new objects created. pub created: Vec<(ObjectRef, Owner)>, - // ObjectRef and owner of mutated objects, including gas object. + /// ObjectRef and owner of mutated objects, including gas object. pub mutated: Vec<(ObjectRef, Owner)>, - // ObjectRef and owner of objects that are unwrapped in this transaction. - // Unwrapped objects are objects that were wrapped into other objects in the past, - // and just got extracted out. + /// ObjectRef and owner of objects that are unwrapped in this transaction. + /// Unwrapped objects are objects that were wrapped into other objects in the past, + /// and just got extracted out. pub unwrapped: Vec<(ObjectRef, Owner)>, - // Object Refs of objects now deleted (the old refs). + /// Object Refs of objects now deleted (the old refs). pub deleted: Vec, - // Object refs of objects now wrapped in other objects. + /// Object refs of objects now wrapped in other objects. pub wrapped: Vec, - // The updated gas object reference. Have a dedicated field for convenient access. - // It's also included in mutated. + /// The updated gas object reference. Have a dedicated field for convenient access. + /// It's also included in mutated. pub gas_object: (ObjectRef, Owner), /// The events emitted during execution. Note that only successful transactions emit events pub events: Vec, @@ -1867,6 +1873,7 @@ impl Default for TransactionEffects { storage_cost: 0, storage_rebate: 0, }, + modified_at_versions: Vec::new(), shared_objects: Vec::new(), transaction_digest: TransactionDigest::random(), created: Vec::new(), @@ -1924,7 +1931,7 @@ impl InputObjectKind { pub fn version(&self) -> Option { match self { - Self::MovePackage(..) => Some(OBJECT_START_VERSION), + Self::MovePackage(..) => Some(PACKAGE_VERSION), Self::ImmOrOwnedMoveObject((_, version, _)) => Some(*version), Self::SharedMoveObject { .. } => None, } @@ -2019,6 +2026,17 @@ impl InputObjects { .collect() } + /// The version to set on objects created by the computation that `self` is input to. + /// Guaranteed to be strictly greater than the versions of all input objects. + pub fn lamport_timestamp(&self) -> SequenceNumber { + let input_versions = self + .objects + .iter() + .filter_map(|(_, object)| object.data.try_as_move().map(MoveObject::version)); + + SequenceNumber::lamport_increment(input_versions) + } + pub fn into_object_map(self) -> BTreeMap { self.objects .into_iter() diff --git a/crates/sui-types/src/object.rs b/crates/sui-types/src/object.rs index 7d02b10a972e5..679e758815350 100644 --- a/crates/sui-types/src/object.rs +++ b/crates/sui-types/src/object.rs @@ -153,21 +153,8 @@ impl MoveObject { &self.contents[ID_END_INDEX..] } - /// Update the contents of this object and increment its version - pub fn update_contents_and_increment_version( - &mut self, - new_contents: Vec, - ) -> Result<(), ExecutionError> { - self.update_contents_without_version_change(new_contents)?; - self.increment_version(); - Ok(()) - } - /// Update the contents of this object but does not increment its version - pub fn update_contents_without_version_change( - &mut self, - new_contents: Vec, - ) -> Result<(), ExecutionError> { + pub fn update_contents(&mut self, new_contents: Vec) -> Result<(), ExecutionError> { if new_contents.len() as u64 > MAX_MOVE_OBJECT_SIZE { return Err(ExecutionError::from_kind( ExecutionErrorKind::MoveObjectTooBig { @@ -176,25 +163,25 @@ impl MoveObject { }, )); } + #[cfg(debug_assertions)] let old_id = self.id(); - #[cfg(debug_assertions)] - let old_version = self.version(); - self.contents = new_contents; - #[cfg(debug_assertions)] - { - // caller should never overwrite ID or version - debug_assert_eq!(self.id(), old_id); - debug_assert_eq!(self.version(), old_version); - } + // Update should not modify ID + debug_assert_eq!(self.id(), old_id); + Ok(()) } - /// Increase the version of this object by one - pub fn increment_version(&mut self) { - self.version = self.version.increment(); + /// Sets the version of this object to a new value which is assumed to be higher (and checked to + /// be higher in debug). + pub fn increment_version_to(&mut self, next: SequenceNumber) { + self.version.increment_to(next); + } + + pub fn decrement_version_to(&mut self, prev: SequenceNumber) { + self.version.decrement_to(prev); } pub fn contents(&self) -> &[u8] { @@ -525,20 +512,11 @@ impl Object { meta_data_size + data_size } - /// Change the owner of `self` to `new_owner`. This function does not increase the version - /// number of the object. - pub fn transfer_without_version_change(&mut self, new_owner: SuiAddress) { + /// Change the owner of `self` to `new_owner`. + pub fn transfer(&mut self, new_owner: SuiAddress) { self.owner = Owner::AddressOwner(new_owner); } - /// Change the owner of `self` to `new_owner`. This function will increment the version - /// number of the object after transfer. - pub fn transfer_and_increment_version(&mut self, new_owner: SuiAddress) { - self.transfer_without_version_change(new_owner); - let data = self.data.try_as_move_mut().unwrap(); - data.increment_version(); - } - pub fn immutable_with_id_for_testing(id: ObjectID) -> Self { let data = Data::Move(MoveObject { type_: GasCoin::type_(), diff --git a/crates/sui-types/src/temporary_store.rs b/crates/sui-types/src/temporary_store.rs index 62364a7daf25c..91a0bd8f213c1 100644 --- a/crates/sui-types/src/temporary_store.rs +++ b/crates/sui-types/src/temporary_store.rs @@ -87,6 +87,8 @@ pub struct TemporaryStore { store: S, tx_digest: TransactionDigest, input_objects: BTreeMap, + /// The version to assign to all objects written by the transaction using this store. + lamport_timestamp: SequenceNumber, mutable_input_refs: Vec, // Inputs that are mutable // When an object is being written, we need to ensure that a few invariants hold. // It's critical that we always call write_object to update `written`, instead of writing @@ -104,11 +106,13 @@ impl TemporaryStore { /// initial objects. pub fn new(store: S, input_objects: InputObjects, tx_digest: TransactionDigest) -> Self { let mutable_inputs = input_objects.mutable_inputs(); + let lamport_timestamp = input_objects.lamport_timestamp(); let objects = input_objects.into_object_map(); Self { store, tx_digest, input_objects: objects, + lamport_timestamp, mutable_input_refs: mutable_inputs, written: BTreeMap::new(), deleted: BTreeMap::new(), @@ -154,7 +158,31 @@ impl TemporaryStore { (None, 0) }; - for (id, (ctx, obj, kind)) in self.written { + for (id, (ctx, mut obj, kind)) in self.written { + // Update the version for the written object, as long as it is a move object and not a + // package (whose versions are fixed to 1) + if let Some(obj) = obj.data.try_as_move_mut() { + obj.increment_version_to(self.lamport_timestamp); + } + + // Record the version that the shared object was created at in its owner field. Note, + // this only works because shared objects must be created as shared (not created as + // owned in one transaction and later converted to shared in another). + if let Owner::Shared { + initial_shared_version, + } = &mut obj.owner + { + if kind == WriteKind::Create { + assert_eq!( + *initial_shared_version, + SequenceNumber::new(), + "Initial version should be blank before this point for {id:?}", + ); + *initial_shared_version = self.lamport_timestamp; + } + } + + // Create events for writes let old_obj = self.input_objects.get(&id); let written_events = Self::create_written_events(ctx, kind, id, &obj, old_obj, gas_id, gas_charged); @@ -162,7 +190,10 @@ impl TemporaryStore { written.insert(id, (obj.compute_object_reference(), obj, kind)); } - for (id, (ctx, version, kind)) in self.deleted { + for (id, (ctx, mut version, kind)) in self.deleted { + // Update the version, post-delete. + version.increment_to(self.lamport_timestamp); + // Create events for each deleted changes let deleted_obj = self.input_objects.get(&id); let balance = deleted_obj @@ -370,13 +401,10 @@ impl TemporaryStore { continue; } if !self.written.contains_key(id) && !self.deleted.contains_key(id) { - let mut object = self.input_objects[id].clone(); - // Active input object must be Move object. - object.data.try_as_move_mut().unwrap().increment_version(); - // We cannot update here but have to push to `to_be_updated` and update latter + // We cannot update here but have to push to `to_be_updated` and update later // because the for loop is holding a reference to `self`, and calling // `self.write_object` requires a mutable reference to `self`. - to_be_updated.push(object); + to_be_updated.push(self.input_objects[id].clone()); } } for object in to_be_updated { @@ -455,7 +483,7 @@ impl TemporaryStore { } pub fn to_effects( - self, + mut self, shared_object_refs: Vec, transaction_digest: &TransactionDigest, transaction_dependencies: Vec, @@ -463,37 +491,44 @@ impl TemporaryStore { status: ExecutionStatus, gas_object_ref: ObjectRef, ) -> (InnerTemporaryStore, TransactionEffects) { - let written: BTreeMap = self - .written - .iter() - .map(|(id, (_, obj, write_kind))| { - let obj_ref = obj.compute_object_reference(); - (*id, (obj_ref, obj.owner, *write_kind)) - }) - .collect(); + let mut modified_at_versions = vec![]; + + // Remember the versions objects were updated from in case of rollback. + self.written.iter_mut().for_each(|(id, (_, obj, kind))| { + if *kind == WriteKind::Mutate { + modified_at_versions.push((*id, obj.version())) + } + }); + + self.deleted.iter_mut().for_each(|(id, (_, version, _))| { + modified_at_versions.push((*id, *version)); + }); + + let (inner, events) = self.into_inner(); // In the case of special transactions that don't require a gas object, // we don't really care about the effects to gas, just use the input for it. let updated_gas_object_info = if gas_object_ref.0 == ObjectID::ZERO { (gas_object_ref, Owner::AddressOwner(SuiAddress::default())) } else { - let (obj_ref, owner, _kind) = written[&gas_object_ref.0]; - (obj_ref, owner) + let (obj_ref, object, _kind) = &inner.written[&gas_object_ref.0]; + (*obj_ref, object.owner) }; + let mut mutated = vec![]; let mut created = vec![]; let mut unwrapped = vec![]; - for (_id, (object_ref, owner, kind)) in written { + for (object_ref, object, kind) in inner.written.values() { match kind { - WriteKind::Mutate => mutated.push((object_ref, owner)), - WriteKind::Create => created.push((object_ref, owner)), - WriteKind::Unwrap => unwrapped.push((object_ref, owner)), + WriteKind::Mutate => mutated.push((*object_ref, object.owner)), + WriteKind::Create => created.push((*object_ref, object.owner)), + WriteKind::Unwrap => unwrapped.push((*object_ref, object.owner)), } } let mut deleted = vec![]; let mut wrapped = vec![]; - for (id, (_, version, kind)) in &self.deleted { + for (id, (version, kind)) in &inner.deleted { match kind { DeleteKind::Normal | DeleteKind::UnwrapThenDelete => { deleted.push((*id, *version, ObjectDigest::OBJECT_DIGEST_DELETED)) @@ -503,11 +538,11 @@ impl TemporaryStore { } } } - let (inner, events) = self.into_inner(); let effects = TransactionEffects { status, gas_used: gas_cost_summary, + modified_at_versions, shared_objects: shared_object_refs, transaction_digest: *transaction_digest, created, @@ -577,6 +612,17 @@ impl TemporaryStore { } } + // Created mutable objects' versions are set to the store's lamport timestamp when it is + // committed to effects. Creating an object at a non-zero version risks violating the + // lamport timestamp invariant (that a transaction's lamport timestamp is strictly greater + // than all versions witnessed by the transaction). + debug_assert!( + kind != WriteKind::Create + || object.is_immutable() + || object.version() == SequenceNumber::MIN, + "Created mutable objects should not have a version set", + ); + // The adapter is not very disciplined at filling in the correct // previous transaction digest, so we ensure it is correct here. object.previous_transaction = self.tx_digest; @@ -654,10 +700,10 @@ impl TemporaryStore { panic!("Internal invariant violation: Deleting a read-only object.") } } - // For object deletion, we increment their version so that they will - // eventually show up in the parent_sync table with an updated version. - self.deleted - .insert(*id, (ctx.clone(), version.increment(), kind)); + + // For object deletion, we will increment the version when converting the store to effects + // so the object will eventually show up in the parent_sync table with a new version. + self.deleted.insert(*id, (ctx.clone(), version, kind)); } /// Resets any mutations and deletions recorded in the store. diff --git a/crates/sui-types/src/unit_tests/base_types_tests.rs b/crates/sui-types/src/unit_tests/base_types_tests.rs index d55ac9be8a497..90b132d155cbc 100644 --- a/crates/sui-types/src/unit_tests/base_types_tests.rs +++ b/crates/sui-types/src/unit_tests/base_types_tests.rs @@ -68,7 +68,7 @@ fn test_gas_coin_ser_deser_roundtrip() { } #[test] -fn test_increment_version() { +fn test_update_contents() { let id = ObjectID::random(); let version = SequenceNumber::from(257); let value = 10; @@ -79,14 +79,10 @@ fn test_increment_version() { let mut coin_obj = coin.to_object(version); assert_eq!(&coin_obj.id(), coin.id()); - // update contents, which should increase sequence number, but leave - // everything else the same + // update contents should not touch the version number or ID. let old_contents = coin_obj.contents().to_vec(); let old_type_specific_contents = coin_obj.type_specific_contents().to_vec(); - coin_obj - .update_contents_and_increment_version(old_contents) - .unwrap(); - assert_eq!(coin_obj.version(), version.increment()); + coin_obj.update_contents(old_contents).unwrap(); assert_eq!(&coin_obj.id(), coin.id()); assert_eq!( coin_obj.type_specific_contents(), @@ -95,6 +91,22 @@ fn test_increment_version() { assert_eq!(GasCoin::try_from(&coin_obj).unwrap().value(), coin.value()); } +#[test] +fn test_lamport_increment_version() { + let versions = [ + SequenceNumber::from(1), + SequenceNumber::from(3), + SequenceNumber::from(257), + SequenceNumber::from(42), + ]; + + let incremented = SequenceNumber::lamport_increment(versions); + + for version in versions { + assert!(version < incremented, "Expected: {version} < {incremented}"); + } +} + #[test] fn test_object_id_conversions() {} diff --git a/crates/sui/tests/full_node_tests.rs b/crates/sui/tests/full_node_tests.rs index a2816ccae25de..804df7d679659 100644 --- a/crates/sui/tests/full_node_tests.rs +++ b/crates/sui/tests/full_node_tests.rs @@ -1215,34 +1215,29 @@ async fn test_full_node_transaction_orchestrator_rpc_ok() -> Result<(), anyhow:: async fn get_obj_read_from_node( node: &SuiNode, object_id: ObjectID, - seq_num: Option, ) -> Result<(ObjectRef, Object, Option), anyhow::Error> { - match seq_num { - None => { - let object_read = node.state().get_object_read(&object_id).await?; - match object_read { - ObjectRead::Exists(obj_ref, object, layout) => Ok((obj_ref, object, layout)), - _ => { - anyhow::bail!("Can't find object {object_id:?} on fullnode.") - } - } - } - Some(seq_num) => { - let object_read = node - .state() - .get_past_object_read(&object_id, seq_num) - .await?; - match object_read { - PastObjectRead::VersionFound(obj_ref, object, layout) => { - Ok((obj_ref, object, layout)) - } - _ => { - anyhow::bail!( - "Can't find object {object_id:?} with seq {seq_num:?} on fullnode." - ) - } - } - } + if let ObjectRead::Exists(obj_ref, object, layout) = + node.state().get_object_read(&object_id).await? + { + Ok((obj_ref, object, layout)) + } else { + anyhow::bail!("Can't find object {object_id:?} on fullnode.") + } +} + +async fn get_past_obj_read_from_node( + node: &SuiNode, + object_id: ObjectID, + seq_num: SequenceNumber, +) -> Result<(ObjectRef, Object, Option), anyhow::Error> { + if let PastObjectRead::VersionFound(obj_ref, object, layout) = node + .state() + .get_past_object_read(&object_id, seq_num) + .await? + { + Ok((obj_ref, object, layout)) + } else { + anyhow::bail!("Can't find object {object_id:?} with seq {seq_num:?} on fullnode.") } } @@ -1260,7 +1255,7 @@ async fn test_get_objects_read() -> Result<(), anyhow::Error> { let recipient = context.config.keystore.addresses().get(1).cloned().unwrap(); assert_ne!(sender, recipient); - let (object_ref_v1, object_v1, _) = get_obj_read_from_node(&node, object_id, None).await?; + let (object_ref_v1, object_v1, _) = get_obj_read_from_node(&node, object_id).await?; // Transfer the object from sender to recipient let gas_ref = get_gas_object_with_wallet_context(context, &sender) @@ -1276,7 +1271,7 @@ async fn test_get_objects_read() -> Result<(), anyhow::Error> { context.execute_transaction(nft_transfer_tx).await.unwrap(); sleep(Duration::from_secs(1)).await; - let (object_ref_v2, object_v2, _) = get_obj_read_from_node(&node, object_id, None).await?; + let (object_ref_v2, object_v2, _) = get_obj_read_from_node(&node, object_id).await?; assert_ne!(object_ref_v2, object_ref_v1); // Transfer some SUI to recipient @@ -1297,30 +1292,33 @@ async fn test_get_objects_read() -> Result<(), anyhow::Error> { other => anyhow::bail!("Expect object {object_id:?} deleted but got {other:?}."), }; - let obj_ref_v3 = match node + let read_ref_v3 = match node .state() - .get_past_object_read(&object_id, SequenceNumber::from_u64(3)) + .get_past_object_read(&object_id, object_ref_v3.1) .await? { PastObjectRead::ObjectDeleted(obj_ref) => obj_ref, other => anyhow::bail!("Expect object {object_id:?} deleted but got {other:?}."), }; - assert_eq!(object_ref_v3, obj_ref_v3); - - let (obj_ref_v2, obj_v2, _) = - get_obj_read_from_node(&node, object_id, Some(SequenceNumber::from_u64(2))).await?; - assert_eq!(object_ref_v2, obj_ref_v2); - assert_eq!(object_v2, obj_v2); - assert_eq!(obj_v2.owner, Owner::AddressOwner(recipient)); - let (obj_ref_v1, obj_v1, _) = - get_obj_read_from_node(&node, object_id, Some(SequenceNumber::from_u64(1))).await?; - assert_eq!(object_ref_v1, obj_ref_v1); - assert_eq!(object_v1, obj_v1); - assert_eq!(obj_v1.owner, Owner::AddressOwner(sender)); + assert_eq!(object_ref_v3, read_ref_v3); + + let (read_ref_v2, read_obj_v2, _) = + get_past_obj_read_from_node(&node, object_id, object_ref_v2.1).await?; + assert_eq!(read_ref_v2, object_ref_v2); + assert_eq!(read_obj_v2, object_v2); + assert_eq!(read_obj_v2.owner, Owner::AddressOwner(recipient)); + + let (read_ref_v1, read_obj_v1, _) = + get_past_obj_read_from_node(&node, object_id, object_ref_v1.1).await?; + assert_eq!(read_ref_v1, object_ref_v1); + assert_eq!(read_obj_v1, object_v1); + assert_eq!(read_obj_v1.owner, Owner::AddressOwner(sender)); + + let too_high_version = SequenceNumber::lamport_increment([object_ref_v3.1]); match node .state() - .get_past_object_read(&object_id, SequenceNumber::from_u64(4)) + .get_past_object_read(&object_id, too_high_version) .await? { PastObjectRead::VersionTooHigh { @@ -1329,8 +1327,8 @@ async fn test_get_objects_read() -> Result<(), anyhow::Error> { latest_version, } => { assert_eq!(obj_id, object_id); - assert_eq!(asked_version, SequenceNumber::from_u64(4)); - assert_eq!(latest_version, SequenceNumber::from_u64(3)); + assert_eq!(asked_version, too_high_version); + assert_eq!(latest_version, object_ref_v3.1); } other => anyhow::bail!( "Expect SequenceNumberTooHigh for object {object_id:?} but got {other:?}."