Skip to content

Commit

Permalink
[5/x][lamport] Remove SequenceNumber::decrement and ::increment (#6163)
Browse files Browse the repository at this point in the history
`SequenceNumber::decrement` is used primarily to revert state
updates (e.g. when an update crosses an epoch or configuration change
and then needs to be rolled back).

With the introduction of lamport timestamps, this will no longer be
feasible with decrement, because a transaction can increment an
object's version by an arbitrary amount (not always one), depending on
the versions of other input objects.

This commit re-implements `revert_state_update` to rely on a list of
initial versions for modified inputs that gets stored in
`TransactionEffects` and gets rid of the `decrement` API altogether.

`increment` is replaced by two pieces:

- `SequenceNumber::lamport_increment` to calculate a lamport timestamp.
- `SequenceNumber::increment_to` to increment to a number that should
  be strictly greater than the current version.

And now, rather than objects being incremented when they are modified,
versions are updated as follows:

- Shared object scheduling pre-computes their new versions based on
  the transaction's inputs.
- The `TemporaryStore` is aware of the lamport timestamp for the
  transaction it's serving, and assigns all modified objects (this
  includes inputs and child objects) this version when committing
  modifications to effects.
- We also remember the versions modified and deleted objects had,
  in case we need to revert -- we can't rely solely on the information
  from the certificate for this, because we could have dynamically
  loaded child objects which need to be reverted as well.

Gateway also needs some special support in order to replay
certificates: Previously it took the objects that were indicated as
modified in effects and replayed those modifications into a store, but
new protections that this commit introduces to ensure that all objects
get incremented would complain (because the objects in the effects
come with thei post-modified version), so before replaying the
modifications, the gateway will decrement an object's version to their
pre-modified state, as specified by `modified_at_versions` in effects,
using a new `SequenceNumber::decrement_to` API.
  • Loading branch information
amnn authored Dec 1, 2022
1 parent 689c8e4 commit 15d1e30
Show file tree
Hide file tree
Showing 26 changed files with 494 additions and 403 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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)}}}
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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}
Original file line number Diff line number Diff line change
Expand Up @@ -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::dynamic_object_field::Wrapper<u64>, sui::object::ID> {id: sui::object::UID {id: sui::object::ID {bytes: fake(109)}}, name: sui::dynamic_object_field::Wrapper<u64> {name: 0u64}, value: std::option::Option<sui::object::ID> {vec: vector[sui::object::ID {bytes: fake(110)}]}}

task 5 'transfer-object'. lines 35-35:
Expand All @@ -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::dynamic_object_field::Wrapper<u64>, sui::object::ID> {id: sui::object::UID {id: sui::object::ID {bytes: fake(109)}}, name: sui::dynamic_object_field::Wrapper<u64> {name: 0u64}, value: std::option::Option<sui::object::ID> {vec: vector[sui::object::ID {bytes: fake(110)}]}}
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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)}}}
73 changes: 14 additions & 59 deletions crates/sui-adapter/src/adapter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -563,7 +563,7 @@ fn process_successful_execution<S: Storage + ParentSync>(
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,
Expand All @@ -572,7 +572,7 @@ fn process_successful_execution<S: Storage + ParentSync>(
}
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,
Expand All @@ -590,73 +590,28 @@ fn process_successful_execution<S: Storage + ParentSync>(
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.
Expand Down
28 changes: 16 additions & 12 deletions crates/sui-cluster-test/src/test_case/call_contract_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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},
Expand Down Expand Up @@ -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));

Expand Down Expand Up @@ -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"
);

Expand Down
27 changes: 22 additions & 5 deletions crates/sui-cluster-test/src/test_case/shared_object_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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"
);

Expand Down
Loading

0 comments on commit 15d1e30

Please sign in to comment.