Skip to content

Commit

Permalink
[sui/shared] Make shared_object_sync less flaky (#6467)
Browse files Browse the repository at this point in the history
Because not all validators pass transactions on to consensus, the
previous implementation of this test can sometimes fail because the
one validator that was fed the initial transaction does not pass it
along.

The test was carefully set-up so that this doesn't happen, but any
change that affects the Rng seed that decides whether this happens or
not can cause it spuriously fail or hang.

The behaviour under test is that out-of-date authorities can catch up
with the rest because the appropriate certificates get shared through
the consensus layer.

The change is to test this by excluding one validator to begin with,
rather than including only one validator, to guarantee that at least
one will propagate the certificate.

Test Plan:

```
sui$ cargo simtest shared_object_sync
```

Note that running simtest multiple times (see below) didn't seem to
trigger the issue, I think because the logic that decides whether or
not a validator propagates a transaction is driven by ObjectIDs
changing:

```
sui$ env MSIM_TEST_NUM=20 cargo simtest shared_object_sync
```

So instead, this change needs to be tested before and after a change
that required tweaking the validators to make this test work again,
e.g. 6102b24 -- it should work on either side without any material
changes (apart from merge conflict resolution).
  • Loading branch information
amnn authored and Jibz1 committed Dec 6, 2022
1 parent dfaa86c commit 515bc5e
Showing 1 changed file with 32 additions and 20 deletions.
52 changes: 32 additions & 20 deletions crates/sui/tests/shared_objects_tests.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Copyright (c) Mysten Labs, Inc.
// SPDX-License-Identifier: Apache-2.0

use futures::{stream, StreamExt};
use sui_core::authority_client::AuthorityAPI;
use sui_types::messages::{
CallArg, ExecutionStatus, ObjectArg, ObjectInfoRequest, ObjectInfoRequestKind,
Expand Down Expand Up @@ -229,7 +230,7 @@ async fn shared_object_sync() {
let package_ref =
publish_counter_package(gas_objects.pop().unwrap(), configs.validator_set()).await;

// Send a transaction to create a counter, but only to one authority.
// Send a transaction to create a counter, to all but one authority.
let create_counter_transaction = move_transaction(
gas_objects.pop().unwrap(),
"counter",
Expand All @@ -239,11 +240,7 @@ async fn shared_object_sync() {
);
let effects = submit_single_owner_transaction(
create_counter_transaction.clone(),
// this is a bit fragile (see consensus adapter):
// 2022-11-11 huitseeker: validator #2 is one of the two validators that submit this TX.
// 2022-11-25 amnn: For reasons completely unrelated to this test, validator #2 is no
// longer one of the validators that submits this TX, but validator #1 is.
&configs.validator_set()[0..1],
&configs.validator_set()[1..],
)
.await;
assert!(matches!(effects.status, ExecutionStatus::Success { .. }));
Expand All @@ -255,20 +252,33 @@ async fn shared_object_sync() {

// Check that the counter object only exist in one validator, but not the rest.
// count the number of validators that have the counter object.
let mut provisioned_authorities = 0;
for config in configs.validator_set() {
provisioned_authorities += get_client(config)

// Check that the counter object exists in at least one of the validators the transaction was
// sent to.
let has_counter = stream::iter(&configs.validator_set()[1..]).any(|config| async move {
get_client(config)
.handle_object_info_request(ObjectInfoRequest {
object_id: counter_id,
request_kind: ObjectInfoRequestKind::LatestObjectInfo(None),
})
.await
.unwrap()
.object()
.map(|_x| 1)
.unwrap_or_default();
}
assert_eq!(1, provisioned_authorities);
.is_some()
});

assert!(has_counter.await);

// Check that the validator that wasn't sent the transaction is unaware of the counter object
assert!(get_client(&configs.validator_set()[0])
.handle_object_info_request(ObjectInfoRequest {
object_id: counter_id,
request_kind: ObjectInfoRequestKind::LatestObjectInfo(None),
})
.await
.unwrap()
.object()
.is_none());

// Make a transaction to increment the counter.
let increment_counter_transaction = move_transaction(
Expand All @@ -279,21 +289,23 @@ async fn shared_object_sync() {
vec![CallArg::Object(counter_object_arg)],
);

// Let's submit the transaction to just one authority (including only one up-to-date).
// Let's submit the transaction to the original set of validators.
let effects = submit_shared_object_transaction(
increment_counter_transaction.clone(),
&configs.validator_set()[1..4],
&configs.validator_set()[1..],
)
.await
.unwrap();
assert!(matches!(effects.status, ExecutionStatus::Success { .. }));

// Submit transactions to out-of-date authorities.
// Submit transactions to the out-of-date authority.
// It will succeed because we share owned object certificates through narwhal
let effects =
submit_shared_object_transaction(increment_counter_transaction, configs.validator_set())
.await
.unwrap();
let effects = submit_shared_object_transaction(
increment_counter_transaction,
&configs.validator_set()[0..1],
)
.await
.unwrap();
assert!(matches!(effects.status, ExecutionStatus::Success { .. }));
}

Expand Down

0 comments on commit 515bc5e

Please sign in to comment.