Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: call_main utility for integration test #7217

Merged
merged 4 commits into from
Jul 20, 2022
Merged

refactor: call_main utility for integration test #7217

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
91f2588
refactor: `call_main` utility for integration test
jakmeier Jul 19, 2022
6bdfc46
Split `execute_sir_tx` in two.
jakmeier Jul 20, 2022
af6f761
de-duplicate code in test_deploy_cost_increased
jakmeier Jul 20, 2022
8c19dac
Merge branch 'master' into call-main-in-process-blocks
jakmeier Jul 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 52 additions & 11 deletions chain/client/src/test_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ use near_primitives::merkle::{merklize, MerklePath, PartialMerkleTree};
use near_primitives::receipt::Receipt;
use near_primitives::shard_layout::ShardUId;
use near_primitives::sharding::{EncodedShardChunk, PartialEncodedChunk, ReedSolomonWrapper};
use near_primitives::transaction::SignedTransaction;
use near_primitives::transaction::{Action, FunctionCallAction, SignedTransaction};
use near_primitives::types::{
AccountId, Balance, BlockHeight, BlockHeightDelta, EpochId, NumBlocks, NumSeats, NumShards,
ShardId,
Expand Down Expand Up @@ -1505,16 +1505,6 @@ impl TestEnv {
}
}

#[track_caller]
pub fn query_transaction_status(
&mut self,
transaction_hash: &CryptoHash,
) -> FinalExecutionOutcomeView {
self.clients[0].chain.get_final_transaction_result(transaction_hash).unwrap_or_else(|err| {
panic!("failed to get transaction status for {}: {}", transaction_hash, err)
})
}

pub fn query_balance(&mut self, account_id: AccountId) -> Balance {
self.query_account(account_id).amount
}
Expand Down Expand Up @@ -1552,6 +1542,57 @@ impl TestEnv {
pub fn get_runtime_config(&self, idx: usize, epoch_id: EpochId) -> RuntimeConfig {
self.clients[idx].runtime_adapter.get_protocol_config(&epoch_id).unwrap().runtime_config
}

/// Create and sign transaction ready for execution.
pub fn tx_from_actions(
&mut self,
actions: Vec<Action>,
signer: &InMemorySigner,
receiver: AccountId,
) -> SignedTransaction {
let tip = self.clients[0].chain.head().unwrap();
SignedTransaction::from_actions(
tip.height + 1,
signer.account_id.clone(),
receiver,
signer,
actions,
tip.last_block_hash,
)
}

/// Process a tx and its receipts, then return the execution outcome.
pub fn execute_tx(&mut self, tx: SignedTransaction) -> FinalExecutionOutcomeView {
let tx_hash = tx.get_hash().clone();
self.clients[0].process_tx(tx, false, false);
let max_iters = 100;
let tip = self.clients[0].chain.head().unwrap();
for i in 0..max_iters {
let block = self.clients[0].produce_block(tip.height + i + 1).unwrap().unwrap();
self.process_block(0, block.clone(), Provenance::PRODUCED);
if let Ok(outcome) = self.clients[0].chain.get_final_transaction_result(&tx_hash) {
return outcome;
}
}
panic!("No transaction outcome found after {max_iters} blocks.")
}

/// Execute a function call transaction that calls main on the `TestEnv`.
///
/// This function assumes that account has been deployed and that
/// `InMemorySigner::from_seed` produces a valid signer that has it's key
/// deployed already.
pub fn call_main(&mut self, account: &AccountId) -> FinalExecutionOutcomeView {
let signer = InMemorySigner::from_seed(account.clone(), KeyType::ED25519, account.as_str());
let actions = vec![Action::FunctionCall(FunctionCallAction {
method_name: "main".to_string(),
args: vec![],
gas: 3 * 10u64.pow(14),
deposit: 0,
})];
let tx = self.tx_from_actions(actions, &signer, signer.account_id.clone());
self.execute_tx(tx)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing I don't like here is that the API is non-compistional. If at some point we introduce some kind of transaction builder which returns a Tx, we'd have to mirror the API here, as there isn't a function which takes a Tx.

This is a non-trivial problem: tx has a nonce, and a block hash, and has to be signed, and we don't want the caller to get the nonce.

So perhaps we need the following, slightly hacky API?

pub fn tx(&mut self, mut tx: Transaction) {
  tx.nonce = tip.height + 1;
  tx.block_hash = tip.hash;
  let signer = Signer::from_seed(tx.signer_id.as_str())
  let tx = tx.sign(&signer)
}

The caller would then basically specify garbage nonce & hash

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I was really hoping to get rid of the caller-site giving garbage nonce and hash.

I have a different suggestion how to make it more compositional, basically split the "create tx" part from the "execute tx" into two functions.

fn tx_from_actions(
    &mut self,
    actions: Vec<Action>,
    signer: &InMemorySigner,
    receiver: AccountId,
) -> SignedTransaction;

pub fn execute_tx(&mut self, tx: SignedTransaction) -> FinalExecutionOutcomeView;

This also opened up more removal of duplicated code in test_deploy_cost_increased. The new interface and this additional refactor are included in the latest two commits.

@matklad Do you think this is better now?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I definitelly like the new API better, in particular, execute_tx(&mut self, tx: SignedTransaction) is very much the right boundary.

I think I'd still prefer garbage nonce approach, but that's debatable. In particular, I feel like that's more of a problem of the split between Tx and SignedTx -- ideally, "what the TX logically is" (list of actions, sender, receiver) should be separate from stuff we need to make cryptograhpy work (nonce, block hash, signature), at least in the testing API. Ideally, we'd have some kind of TxBuilder, but a Tx with Default nonce&block hash can stand in its place. But either of these is a bigger change than warranted in this PR, even if they are good ideas.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the logical piece of work that we generally care about is just a list of actions, not a transaction.

What you describe as

"what the TX logically is" (list of actions, sender, receiver)

in my opinion is not a Transaction. The crypto stuff to define valid placement in the chain very much belongs to a valid transaction definition. The difference between Transaction and SignedTransaction is just how we represent it in memory but logically they describe the same thing.

But I support the idea of a TX builder that prepares actions/signer/receiver and builds into a full TX. But fn tx_from_actions is already quite simple to build a TX. I think to make a builder valuable, it would need to do more than just that. For example, track accounts included in genesis, or help somehow with preparing the receiver with a deployed contract first etc.

}

pub fn create_chunk_on_height_for_shard(
Expand Down
195 changes: 37 additions & 158 deletions integration-tests/src/tests/client/process_blocks.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,23 @@ fn prepare_env_with_congestion(
(env, tx_hashes)
}

/// Create a `TestEnv` with an account and a contract deployed to that account.
fn prepare_env_with_contract(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be great to make it just a method of the builder object! But, with the way real and kv runtimes are setup, that's not going to be trivial, so lgtm.

epoch_length: u64,
protocol_version: u32,
account: AccountId,
contract: Vec<u8>,
) -> TestEnv {
let mut genesis = Genesis::test(vec![account.clone()], 1);
genesis.config.epoch_length = epoch_length;
genesis.config.protocol_version = protocol_version;
let mut env = TestEnv::builder(ChainGenesis::new(&genesis))
.runtime_adapters(create_nightshade_runtimes(&genesis, 1))
.build();
deploy_test_contract(&mut env, account, &contract, epoch_length.clone(), 1);
env
}

/// Runs block producing client and stops after network mock received two blocks.
#[test]
fn produce_two_blocks() {
Expand Down Expand Up @@ -2709,32 +2726,7 @@ fn test_execution_metadata() {
};

// Call the contract and get the execution outcome.
let execution_outcome = {
let tip = env.clients[0].chain.head().unwrap();
let signer = InMemorySigner::from_seed("test0".parse().unwrap(), KeyType::ED25519, "test0");
let tx = Transaction {
signer_id: "test0".parse().unwrap(),
receiver_id: "test0".parse().unwrap(),
public_key: signer.public_key(),
actions: vec![Action::FunctionCall(FunctionCallAction {
method_name: "main".to_string(),
args: Vec::new(),
gas: 100_000_000_000_000,
deposit: 0,
})],

nonce: 10,
block_hash: tip.last_block_hash,
}
.sign(&signer);
let tx_hash = tx.get_hash();

env.clients[0].process_tx(tx, false, false);
for i in 0..3 {
env.produce_block(0, tip.height + i + 1);
}
env.query_transaction_status(&tx_hash)
};
let execution_outcome = env.call_main(&"test0".parse().unwrap());

// Now, let's assert that we get the cost breakdown we expect.
let config = RuntimeConfigStore::test().get_config(PROTOCOL_VERSION).clone();
Expand Down Expand Up @@ -3358,49 +3350,12 @@ fn verify_contract_limits_upgrade(
env
};

let signer = InMemorySigner::from_seed("test0".parse().unwrap(), KeyType::ED25519, "test0");
let tx = Transaction {
signer_id: "test0".parse().unwrap(),
receiver_id: "test0".parse().unwrap(),
public_key: signer.public_key(),
actions: vec![Action::FunctionCall(FunctionCallAction {
method_name: "main".to_string(),
args: Vec::new(),
gas: 100_000_000_000_000,
deposit: 0,
})],

nonce: 0,
block_hash: CryptoHash::default(),
};

// Run the transaction & get tx outcome.
let old_outcome = {
let tip = env.clients[0].chain.head().unwrap();
let signed_transaction =
Transaction { nonce: 10, block_hash: tip.last_block_hash, ..tx.clone() }.sign(&signer);
let tx_hash = signed_transaction.get_hash();
env.clients[0].process_tx(signed_transaction, false, false);
for i in 0..3 {
env.produce_block(0, tip.height + i + 1);
}
env.clients[0].chain.get_final_transaction_result(&tx_hash).unwrap()
};
let account = "test0".parse().unwrap();
let old_outcome = env.call_main(&account);

env.upgrade_protocol(new_protocol_version);

// Re-run the transaction & get tx outcome.
let new_outcome = {
let tip = env.clients[0].chain.head().unwrap();
let signed_transaction =
Transaction { nonce: 11, block_hash: tip.last_block_hash, ..tx }.sign(&signer);
let tx_hash = signed_transaction.get_hash();
env.clients[0].process_tx(signed_transaction, false, false);
for i in 0..3 {
env.produce_block(0, tip.height + i + 1);
}
env.clients[0].chain.get_final_transaction_result(&tx_hash).unwrap()
};
let new_outcome = env.call_main(&account);

assert_matches!(old_outcome.status, FinalExecutionStatus::SuccessValue(_));
let e = match new_outcome.status {
Expand Down Expand Up @@ -3494,33 +3449,15 @@ fn test_deploy_cost_increased() {
};

let signer = InMemorySigner::from_seed("test0".parse().unwrap(), KeyType::ED25519, "test0");
let tx = Transaction {
signer_id: "test0".parse().unwrap(),
receiver_id: "test0".parse().unwrap(),
public_key: signer.public_key(),
actions: vec![Action::DeployContract(DeployContractAction { code: test_contract })],
nonce: 0,
block_hash: CryptoHash::default(),
};
let actions = vec![Action::DeployContract(DeployContractAction { code: test_contract })];

// Run the transaction & get tx outcome in a closure.
let deploy_contract = |env: &mut TestEnv, nonce: u64| {
let tip = env.clients[0].chain.head().unwrap();
let signed_transaction =
Transaction { nonce, block_hash: tip.last_block_hash, ..tx.clone() }.sign(&signer);
let tx_hash = signed_transaction.get_hash();
env.clients[0].process_tx(signed_transaction, false, false);
for i in 0..epoch_length {
env.produce_block(0, tip.height + i + 1);
}
env.clients[0].chain.get_final_transaction_result(&tx_hash).unwrap()
};

let old_outcome = deploy_contract(&mut env, 10);
let tx = env.tx_from_actions(actions.clone(), &signer, signer.account_id.clone());
let old_outcome = env.execute_tx(tx);

env.upgrade_protocol(new_protocol_version);

let new_outcome = deploy_contract(&mut env, 11);
let tx = env.tx_from_actions(actions, &signer, signer.account_id.clone());
let new_outcome = env.execute_tx(tx);

assert_matches!(old_outcome.status, FinalExecutionStatus::SuccessValue(_));
assert_matches!(new_outcome.status, FinalExecutionStatus::SuccessValue(_));
Expand Down Expand Up @@ -4990,7 +4927,6 @@ mod lower_storage_key_limit_test {

mod new_contract_loading_cost {
use super::*;
use near_primitives::views::FinalExecutionOutcomeView;

/// Check that normal execution has the same gas cost after FixContractLoadingCost.
#[test]
Expand All @@ -5005,18 +4941,20 @@ mod new_contract_loading_cost {
let epoch_length: BlockHeight = 5;

let account: AccountId = "test0".parse().unwrap();
let mut env =
test_env_with_contract(epoch_length, old_protocol_version, account.clone(), contract);

let signer = InMemorySigner::from_seed(account.clone(), KeyType::ED25519, account.as_str());
let mut env = prepare_env_with_contract(
epoch_length,
old_protocol_version,
account.clone(),
contract,
);

let old_result = call_main(&mut env, &signer, epoch_length);
let old_result = env.call_main(&account);
let old_gas = old_result.receipts_outcome[0].outcome.gas_burnt;
assert_matches!(old_result.status, FinalExecutionStatus::SuccessValue(_));

env.upgrade_protocol(new_protocol_version);

let new_result = call_main(&mut env, &signer, epoch_length);
let new_result = env.call_main(&account);
let new_gas = new_result.receipts_outcome[0].outcome.gas_burnt;
assert_matches!(new_result.status, FinalExecutionStatus::SuccessValue(_));

Expand All @@ -5036,22 +4974,20 @@ mod new_contract_loading_cost {
let epoch_length: BlockHeight = 5;

let account: AccountId = "test0".parse().unwrap();
let mut env = test_env_with_contract(
let mut env = prepare_env_with_contract(
epoch_length,
old_protocol_version,
account.clone(),
bad_contract,
);

let signer = InMemorySigner::from_seed(account.clone(), KeyType::ED25519, account.as_str());

let old_result = call_main(&mut env, &signer, epoch_length);
let old_result = env.call_main(&account);
let old_gas = old_result.receipts_outcome[0].outcome.gas_burnt;
assert_matches!(old_result.status, FinalExecutionStatus::Failure(_));

env.upgrade_protocol(new_protocol_version);

let new_result = call_main(&mut env, &signer, epoch_length);
let new_result = env.call_main(&account);
let new_gas = new_result.receipts_outcome[0].outcome.gas_burnt;
assert_matches!(new_result.status, FinalExecutionStatus::Failure(_));

Expand All @@ -5063,61 +4999,4 @@ mod new_contract_loading_cost {
let loading_cost = loading_base + contract_size as u64 * loading_byte;
assert_eq!(old_gas + loading_cost, new_gas);
}

/// Create a `TestEnv` with a contract deployed for an account.
fn test_env_with_contract(
epoch_length: u64,
protocol_version: u32,
account: AccountId,
contract: Vec<u8>,
) -> TestEnv {
let mut genesis = Genesis::test(vec![account], 1);
genesis.config.epoch_length = epoch_length;
genesis.config.protocol_version = protocol_version;
let mut env = TestEnv::builder(ChainGenesis::new(&genesis))
.runtime_adapters(create_nightshade_runtimes(&genesis, 1))
.build();
deploy_test_contract(
&mut env,
"test0".parse().unwrap(),
&contract,
epoch_length.clone(),
1,
);
env
}

/// Execute a function call transaction that calls main on the `TestEnv`.
fn call_main(
env: &mut TestEnv,
signer: &InMemorySigner,
epoch_length: u64,
) -> FinalExecutionOutcomeView {
let tx = Transaction {
signer_id: "test0".parse().unwrap(),
receiver_id: "test0".parse().unwrap(),
public_key: signer.public_key(),
actions: vec![Action::FunctionCall(FunctionCallAction {
method_name: "main".to_string(),
args: vec![],
gas: 3 * 10u64.pow(14),
deposit: 0,
})],

nonce: 0,
block_hash: CryptoHash::default(),
};

let tip = env.clients[0].chain.head().unwrap();
let signed_tx =
Transaction { nonce: tip.height + 1, block_hash: tip.last_block_hash, ..tx.clone() }
.sign(signer);
let tx_hash = signed_tx.get_hash().clone();
env.clients[0].process_tx(signed_tx, false, false);
for i in 0..epoch_length {
let block = env.clients[0].produce_block(tip.height + i + 1).unwrap().unwrap();
env.process_block(0, block.clone(), Provenance::PRODUCED);
}
env.clients[0].chain.get_final_transaction_result(&tx_hash).unwrap()
}
}