From fdd88bd722572f47d589c9a2e8eadc53e01323d8 Mon Sep 17 00:00:00 2001 From: Jordan Jennings Date: Wed, 30 Oct 2024 19:08:16 -0700 Subject: [PATCH 1/5] [core] dry run should use dev inspect executor --- crates/sui-core/src/authority.rs | 38 ++++++++++++++++---------------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/crates/sui-core/src/authority.rs b/crates/sui-core/src/authority.rs index ef38ca1ac2f56..848b6351c0a92 100644 --- a/crates/sui-core/src/authority.rs +++ b/crates/sui-core/src/authority.rs @@ -1834,25 +1834,25 @@ impl AuthorityState { .expect("Creating an executor should not fail here"); let expensive_checks = false; - let (inner_temp_store, _, effects, _execution_error) = executor - .execute_transaction_to_effects( - self.get_backing_store().as_ref(), - protocol_config, - self.metrics.limits_metrics.clone(), - expensive_checks, - self.config.certificate_deny_config.certificate_deny_set(), - &epoch_store.epoch_start_config().epoch_data().epoch_id(), - epoch_store - .epoch_start_config() - .epoch_data() - .epoch_start_timestamp(), - checked_input_objects, - gas_object_refs, - gas_status, - kind, - signer, - transaction_digest, - ); + let (inner_temp_store, _, effects, _execution_error) = executor.dev_inspect_transaction( + self.get_backing_store().as_ref(), + protocol_config, + self.metrics.limits_metrics.clone(), + expensive_checks, + self.config.certificate_deny_config.certificate_deny_set(), + &epoch_store.epoch_start_config().epoch_data().epoch_id(), + epoch_store + .epoch_start_config() + .epoch_data() + .epoch_start_timestamp(), + checked_input_objects, + gas_object_refs, + gas_status, + kind, + signer, + transaction_digest, + false, + ); let tx_digest = *effects.transaction_digest(); let module_cache = From c601bb32b197d1c0886db6188f2d8cc16a1bce06 Mon Sep 17 00:00:00 2001 From: Jordan Jennings Date: Thu, 31 Oct 2024 11:06:15 -0700 Subject: [PATCH 2/5] pull skip_all_checks into its own variable --- crates/sui-core/src/authority.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/sui-core/src/authority.rs b/crates/sui-core/src/authority.rs index 848b6351c0a92..446215cb844ff 100644 --- a/crates/sui-core/src/authority.rs +++ b/crates/sui-core/src/authority.rs @@ -1834,6 +1834,7 @@ impl AuthorityState { .expect("Creating an executor should not fail here"); let expensive_checks = false; + let skip_all_checks = false; let (inner_temp_store, _, effects, _execution_error) = executor.dev_inspect_transaction( self.get_backing_store().as_ref(), protocol_config, @@ -1851,7 +1852,7 @@ impl AuthorityState { kind, signer, transaction_digest, - false, + skip_all_checks, ); let tx_digest = *effects.transaction_digest(); From 85afc9dd3cb160c8f817c8e3df0427823464a7fa Mon Sep 17 00:00:00 2001 From: Jordan Jennings Date: Wed, 13 Nov 2024 17:12:02 -0800 Subject: [PATCH 3/5] add test to check object ownership on dry run enforced --- .../src/unit_tests/authority_tests.rs | 140 ++++++++++++++++-- 1 file changed, 126 insertions(+), 14 deletions(-) diff --git a/crates/sui-core/src/unit_tests/authority_tests.rs b/crates/sui-core/src/unit_tests/authority_tests.rs index d579ae33d5d3d..0a7369036b774 100644 --- a/crates/sui-core/src/unit_tests/authority_tests.rs +++ b/crates/sui-core/src/unit_tests/authority_tests.rs @@ -174,7 +174,8 @@ async fn construct_shared_object_transaction_with_sequence_number( // Make a sample transaction. let (validator, fullnode, package) = - init_state_with_ids_and_object_basics_with_fullnode(vec![(sender, gas_object_id)]).await; + init_state_with_ids_and_object_basics_with_fullnode(vec![(sender, gas_object_id, None)]) + .await; validator.insert_genesis_object(shared_object.clone()).await; fullnode.insert_genesis_object(shared_object.clone()).await; let rgp = validator.reference_gas_price_for_testing().unwrap(); @@ -264,7 +265,8 @@ async fn test_dry_run_no_gas_big_transfer() { let recipient = dbg_addr(2); let gas_object_id = ObjectID::random(); let (_, fullnode, _) = - init_state_with_ids_and_object_basics_with_fullnode(vec![(sender, gas_object_id)]).await; + init_state_with_ids_and_object_basics_with_fullnode(vec![(sender, gas_object_id, None)]) + .await; let amount = 1_000_000_000u64; let mut builder = ProgrammableTransactionBuilder::new(); @@ -290,12 +292,20 @@ async fn test_dry_run_no_gas_big_transfer() { assert_eq!(*dry_run_res.effects.status(), SuiExecutionStatus::Success); } +// [#tokio::test] +// async fn test_dry_run_no_arbitrary_functions +// private, package(public) and entry from another module tests + +// test_dry_run_no_arbitrary_values +// unowned objects? + #[tokio::test] async fn test_dev_inspect_object_by_bytes() { let (sender, sender_key): (_, AccountKeyPair) = get_key_pair(); let gas_object_id = ObjectID::random(); let (validator, fullnode, object_basics) = - init_state_with_ids_and_object_basics_with_fullnode(vec![(sender, gas_object_id)]).await; + init_state_with_ids_and_object_basics_with_fullnode(vec![(sender, gas_object_id, None)]) + .await; // test normal call let DevInspectResults { @@ -430,7 +440,8 @@ async fn test_dev_inspect_unowned_object() { let (alice, alice_key): (_, AccountKeyPair) = get_key_pair(); let alice_gas_id = ObjectID::random(); let (validator, fullnode, object_basics) = - init_state_with_ids_and_object_basics_with_fullnode(vec![(alice, alice_gas_id)]).await; + init_state_with_ids_and_object_basics_with_fullnode(vec![(alice, alice_gas_id, None)]) + .await; let (bob, _bob_key): (_, AccountKeyPair) = get_key_pair(); // make an object, send it to bob @@ -497,8 +508,12 @@ async fn test_dev_inspect_dynamic_field() { let (sender, sender_key): (_, AccountKeyPair) = get_key_pair(); let gas_object_id = ObjectID::random(); let (validator, fullnode, object_basics) = - init_state_with_ids_and_object_basics_with_fullnode(vec![(sender, gas_object_id)]) - .await; + init_state_with_ids_and_object_basics_with_fullnode(vec![( + sender, + gas_object_id, + None, + )]) + .await; macro_rules! mk_obj { () => {{ let effects = call_move_( @@ -536,7 +551,8 @@ async fn test_dev_inspect_dynamic_field() { let (sender, _sender_key): (_, AccountKeyPair) = get_key_pair(); let gas_object_id = ObjectID::random(); let (_validator, fullnode, object_basics) = - init_state_with_ids_and_object_basics_with_fullnode(vec![(sender, gas_object_id)]).await; + init_state_with_ids_and_object_basics_with_fullnode(vec![(sender, gas_object_id, None)]) + .await; // add a dynamic field to itself let pt = ProgrammableTransaction { @@ -604,7 +620,8 @@ async fn test_dev_inspect_return_values() { let (sender, sender_key): (_, AccountKeyPair) = get_key_pair(); let gas_object_id = ObjectID::random(); let (validator, fullnode, object_basics) = - init_state_with_ids_and_object_basics_with_fullnode(vec![(sender, gas_object_id)]).await; + init_state_with_ids_and_object_basics_with_fullnode(vec![(sender, gas_object_id, None)]) + .await; // make an object let init_value = 16_u64; @@ -889,7 +906,8 @@ async fn test_dev_inspect_uses_unbound_object() { let (sender, _sender_key): (_, AccountKeyPair) = get_key_pair(); let gas_object_id = ObjectID::random(); let (_validator, fullnode, object_basics) = - init_state_with_ids_and_object_basics_with_fullnode(vec![(sender, gas_object_id)]).await; + init_state_with_ids_and_object_basics_with_fullnode(vec![(sender, gas_object_id, None)]) + .await; let pt = { let mut builder = ProgrammableTransactionBuilder::new(); @@ -1132,6 +1150,67 @@ async fn test_dry_run_dev_inspect_max_gas_version() { assert_eq!(effects.status(), &SuiExecutionStatus::Success); } +// Dry run should behave the same as normal mode where object ownership rules are maintained +#[tokio::test] +async fn test_dry_run_invalid_object_ownership() { + // User transaction that attempts to mutate an object it does not own will fail to sign. + let (sender, sender_key): (_, AccountKeyPair) = get_key_pair(); + let (invalid_owner, _): (_, AccountKeyPair) = get_key_pair(); + + let recipient = dbg_addr(2); + let gas_object_id = ObjectID::random(); + let gas_object = Object::with_id_owner_for_testing(gas_object_id, sender); + + let invalid_ownership_object_id = ObjectID::random(); + let invalid_ownership_object = + Object::with_id_owner_for_testing(invalid_ownership_object_id, invalid_owner); + + // used to be init_state_with_objects + let (_, authority_state, _) = init_state_with_ids_and_object_basics_with_fullnode(vec![ + (sender, gas_object_id.clone(), Some(gas_object.clone())), + ( + sender, + invalid_ownership_object_id.clone(), + Some(invalid_ownership_object.clone()), + ), + ]) + .await; + let rgp = authority_state.reference_gas_price_for_testing().unwrap(); + + let gas_ref = gas_object.compute_object_reference(); + let invalid_ownership_object_ref = invalid_ownership_object.compute_object_reference(); + + let transfer_transaction = init_transfer_transaction( + &authority_state, + sender, + &sender_key, + recipient, + invalid_ownership_object_ref, + gas_ref, + rgp * TEST_ONLY_GAS_UNIT_FOR_TRANSFER, + rgp, + ); + + let digest = transfer_transaction.digest(); + + let Err(e) = authority_state + .dry_exec_transaction( + transfer_transaction.transaction_data().clone(), + digest.clone(), + ) + // .handle_transaction(&epoch_store, transfer_transaction.clone()) + .await + else { + panic!("Expected handling transaction to fail due to IncorrectUserSignature."); + }; + println!("error: {:?}", e); + + assert_eq!( + UserInputError::try_from(e).unwrap(), + UserInputError::IncorrectUserSignature { error: format!("Object {:?} is owned by account address {:?}, but given owner/signer address is {:?}", invalid_ownership_object_id, invalid_owner, sender)} + ); +} + #[tokio::test] async fn test_handle_transfer_transaction_bad_signature() { let (sender, sender_key): (_, AccountKeyPair) = get_key_pair(); @@ -2998,6 +3077,37 @@ async fn test_invalid_object_ownership() { UserInputError::try_from(e).unwrap(), UserInputError::IncorrectUserSignature { error: format!("Object {:?} is owned by account address {:?}, but given owner/signer address is {:?}", invalid_ownership_object_id, invalid_owner, sender)} ); + + // now dev inspect + // + // let transfer_transaction = init_transfer_transaction( + // &authority_state, + // sender, + // &sender_key, + // recipient, + // gas_ref, + // gas_ref, + // rgp * TEST_ONLY_GAS_UNIT_FOR_TRANSFER, + // rgp, + // ); + // + // let pt = ProgrammableTransaction { + // inputs: vec![ + // CallArg::Object(ObjectArg::ImmOrOwnedObject(invalid_ownership_object_ref)), + // CallArg::Pure(recipient.to_vec()), + // ], + // commands: vec![Command::TransferObjects( + // vec![Argument::Input(0)], + // Argument::Input(1), + // )], + // }; + // let kind = TransactionKind::ProgrammableTransaction(pt); + // let DevInspectResults { error, .. } = authority_state + // .dev_inspect_transaction_block(sender, kind, None, None, None, None, None, Some(false)) + // .await + // .unwrap(); + // + // println!("error: {:?}", error); } #[tokio::test] @@ -4059,15 +4169,15 @@ pub async fn publish_object_basics(state: Arc) -> (Arc, + I: IntoIterator)>, >( objects: I, ) -> (Arc, Arc, ObjectRef) { use sui_move_build::BuildConfig; let (validator, fullnode) = init_state_validator_with_fullnode().await; - for (address, object_id) in objects { - let obj = Object::with_id_owner_for_testing(object_id, address); + for (address, object_id, obj) in objects { + let obj = obj.unwrap_or(Object::with_id_owner_for_testing(object_id, address)); validator.insert_genesis_object(obj.clone()).await; fullnode.insert_genesis_object(obj).await; } @@ -5230,7 +5340,8 @@ async fn test_for_inc_201_dev_inspect() { let (sender, _sender_key): (_, AccountKeyPair) = get_key_pair(); let gas_object_id = ObjectID::random(); let (_, fullnode, _) = - init_state_with_ids_and_object_basics_with_fullnode(vec![(sender, gas_object_id)]).await; + init_state_with_ids_and_object_basics_with_fullnode(vec![(sender, gas_object_id, None)]) + .await; // Module bytes let mut path = PathBuf::from(env!("CARGO_MANIFEST_DIR")); @@ -5275,7 +5386,8 @@ async fn test_for_inc_201_dry_run() { let (sender, sender_key): (_, AccountKeyPair) = get_key_pair(); let gas_object_id = ObjectID::random(); let (_, fullnode, _) = - init_state_with_ids_and_object_basics_with_fullnode(vec![(sender, gas_object_id)]).await; + init_state_with_ids_and_object_basics_with_fullnode(vec![(sender, gas_object_id, None)]) + .await; // Module bytes let mut path = PathBuf::from(env!("CARGO_MANIFEST_DIR")); From ed9b3e3373eb16b23b4ab3e4beac5a6c0dcab319 Mon Sep 17 00:00:00 2001 From: Jordan Jennings Date: Thu, 14 Nov 2024 13:43:56 -0800 Subject: [PATCH 4/5] no arbitrary function and publish dry run tests --- .../src/unit_tests/authority_tests.rs | 171 +++++++++++++++++- 1 file changed, 163 insertions(+), 8 deletions(-) diff --git a/crates/sui-core/src/unit_tests/authority_tests.rs b/crates/sui-core/src/unit_tests/authority_tests.rs index 0a7369036b774..238d16b9ff55b 100644 --- a/crates/sui-core/src/unit_tests/authority_tests.rs +++ b/crates/sui-core/src/unit_tests/authority_tests.rs @@ -292,13 +292,6 @@ async fn test_dry_run_no_gas_big_transfer() { assert_eq!(*dry_run_res.effects.status(), SuiExecutionStatus::Success); } -// [#tokio::test] -// async fn test_dry_run_no_arbitrary_functions -// private, package(public) and entry from another module tests - -// test_dry_run_no_arbitrary_values -// unowned objects? - #[tokio::test] async fn test_dev_inspect_object_by_bytes() { let (sender, sender_key): (_, AccountKeyPair) = get_key_pair(); @@ -1150,6 +1143,125 @@ async fn test_dry_run_dev_inspect_max_gas_version() { assert_eq!(effects.status(), &SuiExecutionStatus::Success); } +// private, package(public) and entry from another module tests +#[tokio::test] +async fn test_dry_run_no_arbitrary_functions() { + let (sender, sender_key): (_, AccountKeyPair) = get_key_pair(); + let gas_object_id = ObjectID::random(); + let gas_object = Object::with_id_owner_for_testing(gas_object_id, sender); + let (validator, fullnode, object_basics) = init_state_with_ids_and_object_basics_with_fullnode( + vec![(sender, gas_object_id, Some(gas_object.clone()))], + ) + .await; + + // make an object + let init_value = 16_u64; + let effects = call_move_( + &validator, + Some(&fullnode), + &gas_object_id, + &sender, + &sender_key, + &object_basics.0, + "object_basics", + "create", + vec![], + vec![ + TestCallArg::Pure(bcs::to_bytes(&(init_value)).unwrap()), + TestCallArg::Pure(bcs::to_bytes(&sender).unwrap()), + ], + false, + ) + .await + .unwrap(); + let created_object_id = effects.created()[0].0 .0; + let created_object = validator + .get_object(&created_object_id) + .await + .unwrap() + .unwrap(); + let created_object_bytes = created_object + .data + .try_as_move() + .unwrap() + .contents() + .to_vec(); + + match call_dry_run( + &fullnode, + &sender, + &object_basics.0, + "object_basics", + "get_value", + vec![], + vec![TestCallArg::Pure(created_object_bytes.clone())], + gas_object.compute_object_reference(), + ) + .await + .unwrap() + .0 + .effects + { + SuiTransactionBlockEffects::V1(effects) => { + assert!(effects.status.is_err()); + assert_eq!( + effects.status, + SuiExecutionStatus::Failure { + error: "NonEntryFunctionInvoked in command 0".to_string(), + } + ); + } + }; +} + +#[tokio::test] +async fn test_dry_run_publish() { + let (sender, sender_key): (_, AccountKeyPair) = get_key_pair(); + let (_, authority, _) = init_state_with_ids_and_object_basics_with_fullnode(vec![]).await; + + let rgp = authority.reference_gas_price_for_testing().unwrap(); + let gas_payment_object_id = ObjectID::random(); + // Use the max budget to avoid running out of gas. + let gas_balance = { + let epoch_store = authority.epoch_store_for_testing(); + let protocol_config = epoch_store.protocol_config(); + protocol_config.max_tx_gas() + }; + let gas_payment_object = + Object::with_id_owner_gas_for_testing(gas_payment_object_id, sender, gas_balance); + let gas_payment_object_ref = gas_payment_object.compute_object_reference(); + authority.insert_genesis_object(gas_payment_object).await; + + let module = file_format::empty_module(); + let mut module_bytes = Vec::new(); + module + .serialize_with_version(module.version, &mut module_bytes) + .unwrap(); + let module_bytes = vec![module_bytes]; + let dependencies = vec![]; // no dependencies + let data = TransactionData::new_module( + sender, + gas_payment_object_ref, + module_bytes, + dependencies, + rgp * TEST_ONLY_GAS_UNIT_FOR_PUBLISH, + rgp, + ); + let transaction = to_sender_signed_transaction(data, &sender_key); + let transaction_data = transaction.transaction_data().clone(); + let transaction_digest = transaction.digest().clone(); + + let result = authority + .dry_exec_transaction(transaction_data, transaction_digest) + .await + .expect("publish failed"); + match result.0.effects { + SuiTransactionBlockEffects::V1(effects) => { + assert_eq!(effects.status, SuiExecutionStatus::Success); + } + } +} + // Dry run should behave the same as normal mode where object ownership rules are maintained #[tokio::test] async fn test_dry_run_invalid_object_ownership() { @@ -1198,7 +1310,6 @@ async fn test_dry_run_invalid_object_ownership() { transfer_transaction.transaction_data().clone(), digest.clone(), ) - // .handle_transaction(&epoch_store, transfer_transaction.clone()) .await else { panic!("Expected handling transaction to fail due to IncorrectUserSignature."); @@ -4536,6 +4647,50 @@ pub async fn call_dev_inspect( .await } +pub async fn call_dry_run( + authority: &AuthorityState, + sender: &SuiAddress, + package: &ObjectID, + module: &str, + function: &str, + type_arguments: Vec, + test_args: Vec, + gas_object_ref: ObjectRef, +) -> SuiResult<( + DryRunTransactionBlockResponse, + BTreeMap, + TransactionEffects, + Option, +)> { + let mut builder = ProgrammableTransactionBuilder::new(); + let mut arguments = Vec::with_capacity(test_args.len()); + for a in test_args { + arguments.push(a.to_call_arg(&mut builder, authority).await) + } + + builder.command(Command::move_call( + *package, + Identifier::new(module).unwrap(), + Identifier::new(function).unwrap(), + type_arguments, + arguments, + )); + + let rgp = authority.reference_gas_price_for_testing().unwrap(); + + let tx = TransactionData::new_programmable( + *sender, + vec![gas_object_ref], + builder.finish(), + rgp * TEST_ONLY_GAS_UNIT_FOR_OBJECT_BASICS, + rgp, + ); + + let digest = tx.digest(); + + authority.dry_exec_transaction(tx, digest).await +} + /// This function creates a transaction that calls a 0x02::object_basics::set_value function. /// Usually we need to publish this package first, but in these test files we often don't do that. /// Then the tx would fail with `VMVerificationOrDeserializationError` (Linker error, module not found), From 3f0e2dbee0365fdf757f1ab65924ef367389944a Mon Sep 17 00:00:00 2001 From: Jordan Jennings Date: Thu, 14 Nov 2024 15:55:57 -0800 Subject: [PATCH 5/5] fix comments --- .../src/unit_tests/authority_tests.rs | 35 ++----------------- 1 file changed, 2 insertions(+), 33 deletions(-) diff --git a/crates/sui-core/src/unit_tests/authority_tests.rs b/crates/sui-core/src/unit_tests/authority_tests.rs index 238d16b9ff55b..22ac30bca4698 100644 --- a/crates/sui-core/src/unit_tests/authority_tests.rs +++ b/crates/sui-core/src/unit_tests/authority_tests.rs @@ -1143,7 +1143,7 @@ async fn test_dry_run_dev_inspect_max_gas_version() { assert_eq!(effects.status(), &SuiExecutionStatus::Success); } -// private, package(public) and entry from another module tests +// ensure that dry run cannot call private functions #[tokio::test] async fn test_dry_run_no_arbitrary_functions() { let (sender, sender_key): (_, AccountKeyPair) = get_key_pair(); @@ -1262,7 +1262,7 @@ async fn test_dry_run_publish() { } } -// Dry run should behave the same as normal mode where object ownership rules are maintained +// dry run should not allow violations of ownership #[tokio::test] async fn test_dry_run_invalid_object_ownership() { // User transaction that attempts to mutate an object it does not own will fail to sign. @@ -3188,37 +3188,6 @@ async fn test_invalid_object_ownership() { UserInputError::try_from(e).unwrap(), UserInputError::IncorrectUserSignature { error: format!("Object {:?} is owned by account address {:?}, but given owner/signer address is {:?}", invalid_ownership_object_id, invalid_owner, sender)} ); - - // now dev inspect - // - // let transfer_transaction = init_transfer_transaction( - // &authority_state, - // sender, - // &sender_key, - // recipient, - // gas_ref, - // gas_ref, - // rgp * TEST_ONLY_GAS_UNIT_FOR_TRANSFER, - // rgp, - // ); - // - // let pt = ProgrammableTransaction { - // inputs: vec![ - // CallArg::Object(ObjectArg::ImmOrOwnedObject(invalid_ownership_object_ref)), - // CallArg::Pure(recipient.to_vec()), - // ], - // commands: vec![Command::TransferObjects( - // vec![Argument::Input(0)], - // Argument::Input(1), - // )], - // }; - // let kind = TransactionKind::ProgrammableTransaction(pt); - // let DevInspectResults { error, .. } = authority_state - // .dev_inspect_transaction_block(sender, kind, None, None, None, None, None, Some(false)) - // .await - // .unwrap(); - // - // println!("error: {:?}", error); } #[tokio::test]