Skip to content

Commit

Permalink
Make checks for RPG more consistent (#12073) (#12089)
Browse files Browse the repository at this point in the history
Make dev inspect honor RGP and gas price to be more consistent and also
remove as much as possible `RGP = 1` in testing

Added a test for gas price and see what tests fail

---
If your changes are not user-facing and not a breaking change, you can
skip the following section. Otherwise, please indicate what changed, and
then add to the Release Notes section as highlighted during the release
process.

- [x] user-visible impact
- [x] breaking change for a client SDKs
- [x] breaking change for FNs (FN binary must upgrade)
- [ ] breaking change for validators or node operators (must upgrade
binaries)
- [ ] breaking change for on-chain data layout
- [ ] necessitate either a data wipe or data migration

## Description 

Describe the changes or additions included in this PR.

## Test Plan 

How did you test the new or updated feature?

---
If your changes are not user-facing and not a breaking change, you can
skip the following section. Otherwise, please indicate what changed, and
then add to the Release Notes section as highlighted during the release
process.

### Type of Change (Check all that apply)

- [ ] user-visible impact
- [ ] breaking change for a client SDKs
- [ ] breaking change for FNs (FN binary must upgrade)
- [ ] breaking change for validators or node operators (must upgrade
binaries)
- [ ] breaking change for on-chain data layout
- [ ] necessitate either a data wipe or data migration

### Release notes
  • Loading branch information
dariorussi authored May 19, 2023
1 parent 14073f1 commit 536412e
Show file tree
Hide file tree
Showing 7 changed files with 71 additions and 17 deletions.
19 changes: 17 additions & 2 deletions crates/sui-core/src/authority.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1320,8 +1320,10 @@ impl AuthorityState {

transaction_kind.check_version_supported(epoch_store.protocol_config())?;

let reference_gas_price = epoch_store.reference_gas_price();
let protocol_config = epoch_store.protocol_config();
let gas_price = match gas_price {
None => epoch_store.reference_gas_price(),
None => reference_gas_price,
Some(gas) => {
if gas == 0 {
epoch_store.reference_gas_price()
Expand All @@ -1330,7 +1332,20 @@ impl AuthorityState {
}
}
};
let protocol_config = epoch_store.protocol_config();
if gas_price < reference_gas_price {
return Err(UserInputError::GasPriceUnderRGP {
gas_price,
reference_gas_price,
}
.into());
}
if protocol_config.gas_model_version() >= 4 && gas_price >= protocol_config.max_gas_price()
{
return Err(UserInputError::GasPriceTooHigh {
max_gas_price: protocol_config.max_gas_price(),
}
.into());
}
let max_tx_gas = protocol_config.max_tx_gas();

let gas_object_id = ObjectID::random();
Expand Down
1 change: 0 additions & 1 deletion crates/sui-core/src/authority/test_authority_builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,6 @@ impl<'a> TestAuthorityBuilder<'a> {

pub async fn build(self) -> Arc<AuthorityState> {
let local_network_config = sui_config::builder::ConfigBuilder::new_with_temp_dir()
// TODO: change the default to 1000 instead after fixing tests.
.with_reference_gas_price(self.reference_gas_price.unwrap_or(1))
.build();
let genesis = &self.genesis.unwrap_or(&local_network_config.genesis);
Expand Down
54 changes: 47 additions & 7 deletions crates/sui-core/src/unit_tests/authority_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -573,7 +573,7 @@ async fn test_dev_inspect_dynamic_field() {
};
let kind = TransactionKind::programmable(pt);
let DevInspectResults { error, .. } = fullnode
.dev_inspect_transaction_block(sender, kind, Some(1))
.dev_inspect_transaction_block(sender, kind, None)
.await
.unwrap();
// produces an error
Expand Down Expand Up @@ -815,7 +815,7 @@ async fn test_dev_inspect_gas_coin_argument() {
};
let kind = TransactionKind::programmable(pt);
let results = fullnode
.dev_inspect_transaction_block(sender, kind, Some(1))
.dev_inspect_transaction_block(sender, kind, None)
.await
.unwrap()
.results
Expand Down Expand Up @@ -845,6 +845,38 @@ async fn test_dev_inspect_gas_coin_argument() {
assert!(return_values.is_empty());
}

#[tokio::test]
async fn test_dev_inspect_gas_price() {
use sui_types::crypto::get_authority_key_pair;

let fullnode_key_pair = get_authority_key_pair().1;
let fullnode = TestAuthorityBuilder::new()
.with_keypair(&fullnode_key_pair)
.with_reference_gas_price(500)
.build()
.await;

let sender = SuiAddress::random_for_testing_only();
let recipient = SuiAddress::random_for_testing_only();
let amount = 500;
let pt = {
let mut builder = ProgrammableTransactionBuilder::new();
builder.pay_sui(vec![recipient], vec![amount]).unwrap();
builder.finish()
};
let kind = TransactionKind::programmable(pt);
assert!(fullnode
.dev_inspect_transaction_block(sender, kind.clone(), Some(1))
.await
.is_err());
let epoch_store = fullnode.epoch_store_for_testing();
let protocol_config = epoch_store.protocol_config();
assert!(fullnode
.dev_inspect_transaction_block(sender, kind, Some(protocol_config.max_gas_price() + 1))
.await
.is_err());
}

fn check_coin_value(actual_value: &[u8], actual_type: &SuiTypeTag, expected_value: u64) {
let actual_type: TypeTag = actual_type.clone().try_into().unwrap();
assert_eq!(actual_type, TypeTag::Struct(Box::new(GasCoin::type_())));
Expand Down Expand Up @@ -877,7 +909,11 @@ async fn test_dev_inspect_uses_unbound_object() {
let kind = TransactionKind::programmable(pt);

let result = fullnode
.dev_inspect_transaction_block(sender, kind, Some(1))
.dev_inspect_transaction_block(
sender,
kind,
Some(fullnode.reference_gas_price_for_testing().unwrap()),
)
.await;
let Err(err) = result else { panic!() };
assert!(err.to_string().contains("ObjectNotFound"));
Expand Down Expand Up @@ -1015,16 +1051,16 @@ async fn test_dry_run_dev_inspect_dynamic_field_too_new() {
}))],
};
let kind = TransactionKind::programmable(pt.clone());
let rgp = fullnode.reference_gas_price_for_testing().unwrap();
// dev inspect
let DevInspectResults { effects, .. } = fullnode
.dev_inspect_transaction_block(sender, kind, Some(1))
.dev_inspect_transaction_block(sender, kind, Some(rgp))
.await
.unwrap();
assert_eq!(effects.deleted().len(), 1);
let deleted = &effects.deleted()[0];
assert_eq!(field.0, deleted.object_id);
assert_eq!(deleted.version, SequenceNumber::MAX);
let rgp = fullnode.reference_gas_price_for_testing().unwrap();
// dry run
let data = TransactionData::new_programmable(
sender,
Expand Down Expand Up @@ -1076,7 +1112,7 @@ async fn test_dry_run_dev_inspect_max_gas_version() {
let kind = TransactionKind::programmable(pt.clone());
// dev inspect
let DevInspectResults { effects, .. } = fullnode
.dev_inspect_transaction_block(sender, kind, Some(1))
.dev_inspect_transaction_block(sender, kind, Some(rgp + 100))
.await
.unwrap();
assert_eq!(effects.status(), &SuiExecutionStatus::Success);
Expand Down Expand Up @@ -4973,7 +5009,11 @@ async fn test_for_inc_201_dev_inspect() {
));
let kind = TransactionKind::programmable(builder.finish());
let DevInspectResults { events, .. } = fullnode
.dev_inspect_transaction_block(sender, kind, Some(1))
.dev_inspect_transaction_block(
sender,
kind,
Some(fullnode.reference_gas_price_for_testing().unwrap() + 1000),
)
.await
.unwrap();

Expand Down
1 change: 0 additions & 1 deletion crates/sui-core/src/unit_tests/pay_sui_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -425,7 +425,6 @@ async fn execute_pay_all_sui(
) -> PaySuiTransactionBlockExecutionResult {
let dir = tempfile::TempDir::new().unwrap();
let network_config = sui_config::builder::ConfigBuilder::new(&dir)
// TODO: fix numbers in tests to not depend on rgp being 1
.with_reference_gas_price(1)
.with_objects(
input_coin_objects
Expand Down
3 changes: 2 additions & 1 deletion crates/transaction-fuzzer/src/account_universe/account.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@ use sui_types::{

use crate::executor::Executor;

pub const INITIAL_BALANCE: u64 = 10_000_000_000;
pub const INITIAL_BALANCE: u64 = 100_000_000_000_000;
pub const PUBLISH_BUDGET: u64 = 1_000_000_000;
pub const NUM_GAS_OBJECTS: usize = 1;

#[derive(Debug)]
Expand Down
6 changes: 3 additions & 3 deletions crates/transaction-fuzzer/src/executor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ use sui_types::utils::to_sender_signed_transaction;
use sui_types::{error::SuiError, messages::VerifiedTransaction, object::Object};
use tokio::runtime::Runtime;

use crate::account_universe::{AccountCurrent, INITIAL_BALANCE};
use crate::account_universe::{AccountCurrent, PUBLISH_BUDGET};

use std::path::PathBuf;
use sui_move_build::BuildConfig;
Expand Down Expand Up @@ -123,8 +123,8 @@ impl Executor {
gas_object.compute_object_reference(),
modules,
vec![],
INITIAL_BALANCE,
1,
PUBLISH_BUDGET,
1000,
);
let txn = to_sender_signed_transaction(data, &account.initial_data.account.key);
let effects = self
Expand Down
4 changes: 2 additions & 2 deletions crates/transaction-fuzzer/src/type_arg_fuzzer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@ use sui_types::{TypeTag, SUI_FRAMEWORK_OBJECT_ID};
use crate::account_universe::AccountCurrent;
use crate::executor::{assert_is_acceptable_result, Executor};

const GAS: u64 = 1_000_000;
const GAS_PRICE: u64 = 1;
const GAS_PRICE: u64 = 700;
const GAS: u64 = 1_000_000 * GAS_PRICE;

pub fn gen_type_tag() -> impl Strategy<Value = TypeTag> {
prop_oneof![
Expand Down

0 comments on commit 536412e

Please sign in to comment.