Skip to content

Commit

Permalink
Do not use VerifiedTransaction type on clients (MystenLabs#12757)
Browse files Browse the repository at this point in the history
This PR paves the way for moving to stateful authenticators, which are
generally difficult or impossible to use correctly on clients until we
have better light client infrastructure.

VerifiedTransaction is intended to prevent unverified transactions from
entering the validator core accidentally, so their use on clients was
unnecessary to begin with.

Note that TransactionOrchestrator continues to use VerifiedTransaction -
this is for two reasons:
- it has an AuthorityState handle so it is easy for it to do so even
with a stateful authenticator
- it should catch bad signatures early rather than relying on validators
to do that
  • Loading branch information
mystenmark authored Jul 6, 2023
1 parent 68b28fb commit 33e1e14
Show file tree
Hide file tree
Showing 60 changed files with 370 additions and 381 deletions.
14 changes: 4 additions & 10 deletions crates/sui-benchmark/src/bank.rs
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ impl BenchmarkBank {
amounts[0],
);

let verified_tx = make_pay_sui_transaction(
let tx = make_pay_sui_transaction(
init_coin.0,
vec![],
recipient_addresses,
Expand All @@ -121,10 +121,7 @@ impl BenchmarkBank {
MAX_BUDGET,
);

let effects = self
.proxy
.execute_transaction_block(verified_tx.into())
.await?;
let effects = self.proxy.execute_transaction_block(tx).await?;

if !effects.is_ok() {
effects.print_gas_summary();
Expand Down Expand Up @@ -166,7 +163,7 @@ impl BenchmarkBank {
async fn create_init_coin(&mut self, amount: u64, gas_price: u64) -> Result<Gas> {
info!("Creating initilization coin of value {amount}...");

let verified_tx = make_transfer_sui_transaction(
let tx = make_transfer_sui_transaction(
self.primary_coin.0,
self.primary_coin.1,
Some(amount),
Expand All @@ -175,10 +172,7 @@ impl BenchmarkBank {
gas_price,
);

let effects = self
.proxy
.execute_transaction_block(verified_tx.into())
.await?;
let effects = self.proxy.execute_transaction_block(tx).await?;

if !effects.is_ok() {
effects.print_gas_summary();
Expand Down
8 changes: 4 additions & 4 deletions crates/sui-benchmark/src/drivers/bench_driver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ use crate::ValidatorProxy;
use std::collections::{BTreeMap, VecDeque};
use std::sync::Arc;
use std::time::Duration;
use sui_types::transaction::{TransactionDataAPI, VerifiedTransaction};
use sui_types::transaction::{Transaction, TransactionDataAPI};
use sysinfo::{CpuExt, System, SystemExt};
use tokio::sync::Barrier;
use tokio::{time, time::Instant};
Expand Down Expand Up @@ -149,7 +149,7 @@ struct Stats {
pub bench_stats: BenchmarkStats,
}

type RetryType = Box<(VerifiedTransaction, Box<dyn Payload>)>;
type RetryType = Box<(Transaction, Box<dyn Payload>)>;

enum NextOp {
Response {
Expand Down Expand Up @@ -385,7 +385,7 @@ impl Driver<(BenchmarkStats, StressStats)> for BenchDriver {
let committee_cloned = Arc::new(worker.proxy.clone_committee());
let start = Arc::new(Instant::now());
let res = worker.proxy
.execute_transaction_block(b.0.clone().into())
.execute_transaction_block(b.0.clone())
.then(|res| async move {
match res {
Ok(effects) => {
Expand Down Expand Up @@ -441,7 +441,7 @@ impl Driver<(BenchmarkStats, StressStats)> for BenchDriver {
// TODO: clone committee for each request is not ideal.
let committee_cloned = Arc::new(worker.proxy.clone_committee());
let res = worker.proxy
.execute_transaction_block(tx.clone().into())
.execute_transaction_block(tx.clone())
.then(|res| async move {
match res {
Ok(effects) => {
Expand Down
10 changes: 5 additions & 5 deletions crates/sui-benchmark/src/in_memory_wallet.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use sui_types::{
base_types::{ObjectID, ObjectRef, SuiAddress},
crypto::AccountKeyPair,
object::Owner,
transaction::{CallArg, TransactionData, TransactionDataAPI, VerifiedTransaction},
transaction::{CallArg, Transaction, TransactionData, TransactionDataAPI},
utils::to_sender_signed_transaction,
};

Expand Down Expand Up @@ -124,7 +124,7 @@ impl InMemoryWallet {
self.accounts.get(addr).map(|a| a.owned.values())
}

pub fn create_tx(&self, data: TransactionData) -> VerifiedTransaction {
pub fn create_tx(&self, data: TransactionData) -> Transaction {
let sender = data.sender();
to_sender_signed_transaction(data, self.accounts.get(&sender).unwrap().key.as_ref())
}
Expand All @@ -139,7 +139,7 @@ impl InMemoryWallet {
arguments: Vec<CallArg>,
gas_budget: u64,
gas_price: u64,
) -> VerifiedTransaction {
) -> Transaction {
let account = self.account(&sender).unwrap();
let data = TransactionData::new_move_call(
sender,
Expand All @@ -166,7 +166,7 @@ impl InMemoryWallet {
arguments: Vec<BenchMoveCallArg>,
gas_budget: u64,
gas_price: u64,
) -> VerifiedTransaction {
) -> Transaction {
let account = self.account(&sender).unwrap();
move_call_pt_impl(
sender,
Expand Down Expand Up @@ -214,7 +214,7 @@ pub fn move_call_pt_impl(
gas_ref: &ObjectRef,
gas_budget: u64,
gas_price: u64,
) -> VerifiedTransaction {
) -> Transaction {
let mut builder = ProgrammableTransactionBuilder::new();
let args = convert_move_call_args(&arguments, &mut builder);

Expand Down
2 changes: 0 additions & 2 deletions crates/sui-benchmark/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -359,7 +359,6 @@ impl ValidatorProxy for LocalValidatorAggregatorProxy {
return self.execute_bench_transaction(tx).await;
}
let tx_digest = *tx.digest();
let tx = tx.verify()?;
let mut retry_cnt = 0;
while retry_cnt < 3 {
let ticket = self.qd.submit_transaction(tx.clone()).await?;
Expand Down Expand Up @@ -721,7 +720,6 @@ impl ValidatorProxy for FullNodeProxy {

async fn execute_transaction_block(&self, tx: Transaction) -> anyhow::Result<ExecutionEffects> {
let tx_digest = *tx.digest();
let tx = tx.verify()?;
let mut retry_cnt = 0;
while retry_cnt < 10 {
// Fullnode could time out after WAIT_FOR_FINALITY_TIMEOUT (30s) in TransactionOrchestrator
Expand Down
11 changes: 3 additions & 8 deletions crates/sui-benchmark/src/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,7 @@ use sui_test_transaction_builder::TestTransactionBuilder;
use sui_types::base_types::ObjectRef;
use sui_types::crypto::{AccountKeyPair, KeypairTraits};
use sui_types::object::Owner;
use sui_types::transaction::{
TransactionData, VerifiedTransaction, TEST_ONLY_GAS_UNIT_FOR_TRANSFER,
};
use sui_types::transaction::{Transaction, TransactionData, TEST_ONLY_GAS_UNIT_FOR_TRANSFER};
use sui_types::utils::to_sender_signed_transaction;
use sui_types::{base_types::SuiAddress, crypto::SuiKeyPair};

Expand Down Expand Up @@ -41,7 +39,7 @@ pub fn make_pay_tx(
gas: ObjectRef,
keypair: &AccountKeyPair,
gas_price: u64,
) -> Result<VerifiedTransaction> {
) -> Result<Transaction> {
let pay = TransactionData::new_pay(
sender,
input_coins,
Expand All @@ -64,10 +62,7 @@ pub async fn publish_basics_package(
let transaction = TestTransactionBuilder::new(sender, gas, gas_price)
.publish_examples("basics")
.build_and_sign(keypair);
let effects = proxy
.execute_transaction_block(transaction.into())
.await
.unwrap();
let effects = proxy.execute_transaction_block(transaction).await.unwrap();
effects
.created()
.iter()
Expand Down
16 changes: 5 additions & 11 deletions crates/sui-benchmark/src/workloads/adversarial.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ use sui_types::effects::TransactionEffectsAPI;
use sui_types::transaction::Command;
use sui_types::transaction::{CallArg, ObjectArg};
use sui_types::{base_types::ObjectID, object::Owner};
use sui_types::{base_types::SuiAddress, crypto::get_key_pair, transaction::VerifiedTransaction};
use sui_types::{base_types::SuiAddress, crypto::get_key_pair, transaction::Transaction};
use sui_types::{transaction::TransactionData, utils::to_sender_signed_transaction};
use tracing::debug;

Expand Down Expand Up @@ -176,7 +176,7 @@ impl Payload for AdversarialTestPayload {
self.state.update(effects);
}

fn make_transaction(&mut self) -> VerifiedTransaction {
fn make_transaction(&mut self) -> Transaction {
let payload_type = self.adversarial_payload_cfg.payload_type;

self.create_transaction(
Expand All @@ -203,7 +203,7 @@ impl AdversarialTestPayload {
&self,
payload_type: &AdversarialPayloadType,
protocol_config: &ProtocolConfig,
) -> VerifiedTransaction {
) -> Transaction {
let args = self.get_payload_args(payload_type, protocol_config);
let module_name = "adversarial";
let account = self.state.account(&self.sender).unwrap();
Expand Down Expand Up @@ -464,10 +464,7 @@ impl Workload<dyn Payload> for AdversarialWorkload {
let transaction = TestTransactionBuilder::new(gas.1, gas.0, reference_gas_price)
.publish(path)
.build_and_sign(gas.2.as_ref());
let effects = proxy
.execute_transaction_block(transaction.into())
.await
.unwrap();
let effects = proxy.execute_transaction_block(transaction).await.unwrap();
let created = effects.created();
// should only create the package object, upgrade cap, dynamic field top level obj, and NUM_DYNAMIC_FIELDS df objects. otherwise, there are some object initializers running and we will need to disambiguate
assert_eq!(
Expand Down Expand Up @@ -514,10 +511,7 @@ impl Workload<dyn Payload> for AdversarialWorkload {
reference_gas_price,
);

let effects = proxy
.execute_transaction_block(transaction.into())
.await
.unwrap();
let effects = proxy.execute_transaction_block(transaction).await.unwrap();

let created = effects.created();
assert_eq!(created.len() as u64, num_shared_objs);
Expand Down
4 changes: 2 additions & 2 deletions crates/sui-benchmark/src/workloads/batch_payment.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ use sui_types::object::Owner;
use sui_types::{
base_types::{ObjectRef, SuiAddress},
crypto::get_key_pair,
transaction::VerifiedTransaction,
transaction::Transaction,
};
use tracing::{debug, error};

Expand Down Expand Up @@ -70,7 +70,7 @@ impl Payload for BatchPaymentTestPayload {
self.num_payments += self.state.num_addresses();
}

fn make_transaction(&mut self) -> VerifiedTransaction {
fn make_transaction(&mut self) -> Transaction {
let addrs = self.state.addresses().cloned().collect::<Vec<SuiAddress>>();
let num_recipients = addrs.len();
let sender = if self.num_payments == 0 {
Expand Down
4 changes: 2 additions & 2 deletions crates/sui-benchmark/src/workloads/delegation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ use sui_test_transaction_builder::TestTransactionBuilder;
use sui_types::base_types::{ObjectRef, SuiAddress};
use sui_types::crypto::{get_key_pair, AccountKeyPair};
use sui_types::gas_coin::MIST_PER_SUI;
use sui_types::transaction::VerifiedTransaction;
use sui_types::transaction::Transaction;
use tracing::error;

#[derive(Debug)]
Expand Down Expand Up @@ -54,7 +54,7 @@ impl Payload for DelegationTestPayload {
/// delegation flow is split into two phases
/// first `make_transaction` call creates separate coin object for future delegation
/// followup call creates delegation transaction itself
fn make_transaction(&mut self) -> VerifiedTransaction {
fn make_transaction(&mut self) -> Transaction {
match self.coin {
Some(coin) => TestTransactionBuilder::new(
self.sender,
Expand Down
4 changes: 2 additions & 2 deletions crates/sui-benchmark/src/workloads/payload.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,13 @@

use crate::ExecutionEffects;
use std::fmt::Display;
use sui_types::transaction::VerifiedTransaction;
use sui_types::transaction::Transaction;

/// A Payload is a transaction wrapper of a particular type (transfer object, shared counter, etc).
/// Calling `make_transaction()` on a payload produces the transaction it is wrapping. Once that
/// transaction is returned with effects (by quorum driver), a new payload can be generated with that
/// effect by invoking `make_new_payload(effects)`
pub trait Payload: Send + Sync + std::fmt::Debug + Display {
fn make_new_payload(&mut self, effects: &ExecutionEffects);
fn make_transaction(&mut self) -> VerifiedTransaction;
fn make_transaction(&mut self) -> Transaction;
}
9 changes: 3 additions & 6 deletions crates/sui-benchmark/src/workloads/shared_counter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use sui_test_transaction_builder::TestTransactionBuilder;
use sui_types::crypto::get_key_pair;
use sui_types::{
base_types::{ObjectDigest, ObjectID, SequenceNumber},
transaction::VerifiedTransaction,
transaction::Transaction,
};
use tracing::{debug, error, info};

Expand Down Expand Up @@ -51,7 +51,7 @@ impl Payload for SharedCounterTestPayload {
}
self.gas.0 = effects.gas_object().0;
}
fn make_transaction(&mut self) -> VerifiedTransaction {
fn make_transaction(&mut self) -> Transaction {
let rgp = self
.system_state_observer
.state
Expand Down Expand Up @@ -220,10 +220,7 @@ impl Workload<dyn Payload> for SharedCounterWorkload {
.build_and_sign(keypair.as_ref());
let proxy_ref = proxy.clone();
futures.push(async move {
if let Ok(effects) = proxy_ref
.execute_transaction_block(transaction.into())
.await
{
if let Ok(effects) = proxy_ref.execute_transaction_block(transaction).await {
effects.created()[0].0
} else {
panic!("Failed to create shared counter!");
Expand Down
4 changes: 2 additions & 2 deletions crates/sui-benchmark/src/workloads/transfer_object.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use sui_core::test_utils::make_transfer_object_transaction;
use sui_types::{
base_types::{ObjectRef, SuiAddress},
crypto::{get_key_pair, AccountKeyPair},
transaction::VerifiedTransaction,
transaction::Transaction,
};

/// TODO: This should be the amount that is being transferred instead of MAX_GAS.
Expand Down Expand Up @@ -65,7 +65,7 @@ impl Payload for TransferObjectTestPayload {
self.transfer_to = recipient;
self.gas = updated_gas;
}
fn make_transaction(&mut self) -> VerifiedTransaction {
fn make_transaction(&mut self) -> Transaction {
let (gas_obj, _, keypair) = self.gas.iter().find(|x| x.1 == self.transfer_from).unwrap();
make_transfer_object_transaction(
self.transfer_object,
Expand Down
8 changes: 3 additions & 5 deletions crates/sui-cluster-test/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ use sui_sdk::SuiClient;
use sui_types::gas_coin::GasCoin;
use sui_types::{
base_types::SuiAddress,
transaction::{Transaction, TransactionData, VerifiedTransaction},
transaction::{Transaction, TransactionData},
};
use test_case::{
coin_index_test::CoinIndexTest, coin_merge_split_test::CoinMergeSplitTest,
Expand Down Expand Up @@ -127,7 +127,7 @@ impl TestContext {

/// See `make_transactions_with_wallet_context` for potential caveats
/// of this helper function.
pub async fn make_transactions(&self, max_txn_num: usize) -> Vec<VerifiedTransaction> {
pub async fn make_transactions(&self, max_txn_num: usize) -> Vec<Transaction> {
self.get_wallet()
.batch_make_transfer_transactions(max_txn_num)
.await
Expand Down Expand Up @@ -155,9 +155,7 @@ impl TestContext {
.get_fullnode_client()
.quorum_driver_api()
.execute_transaction_block(
Transaction::from_data(txn_data, Intent::sui_transaction(), vec![signature])
.verify()
.unwrap(),
Transaction::from_data(txn_data, Intent::sui_transaction(), vec![signature]),
SuiTransactionBlockResponseOptions::new()
.with_object_changes()
.with_balance_changes()
Expand Down
9 changes: 9 additions & 0 deletions crates/sui-core/src/authority.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4052,6 +4052,15 @@ impl AuthorityState {
Ok(new_epoch_store)
}

// TODO: when we add stateful authenticators, we may need to take care that this function is
// reconfig safe (i.e. cannot be called concurrently with reconfiguration).
pub fn verify_transaction(&self, tx: Transaction) -> SuiResult<VerifiedTransaction> {
self.load_epoch_store_one_call_per_task()
.signature_verifier
.verify_tx(tx.data())
.map(|_| VerifiedTransaction::new_from_verified(tx))
}

#[cfg(test)]
pub(crate) fn iter_live_object_set_for_testing(
&self,
Expand Down
Loading

0 comments on commit 33e1e14

Please sign in to comment.