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.

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

GH-5039: Tweak costs and chainspec settings #5047

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
29 changes: 2 additions & 27 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

45 changes: 34 additions & 11 deletions execution_engine/src/runtime/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -859,8 +859,11 @@ where
mint_runtime.mint_into_existing_purse(existing_purse, amount);
CLValue::from_t(result).map_err(Self::reverter)
})(),

_ => CLValue::from_t(()).map_err(Self::reverter),
_ => {
// Code should never reach this point as existence of the entrypoint is validated
// before reaching this pointt.
Ok(CLValue::unit())
}
};

// Charge just for the amount that particular entry point cost - using gas cost from the
Expand Down Expand Up @@ -954,7 +957,11 @@ where
let maybe_purse = runtime.get_refund_purse().map_err(Self::reverter)?;
CLValue::from_t(maybe_purse).map_err(Self::reverter)
})(),
_ => CLValue::from_t(()).map_err(Self::reverter),
_ => {
// Code should never reach here as existence of the entrypoint is validated before
// reaching this point.
Ok(CLValue::unit())
}
};

self.gas(
Expand Down Expand Up @@ -1290,8 +1297,11 @@ where

CLValue::from_t(()).map_err(Self::reverter)
})(),

_ => CLValue::from_t(()).map_err(Self::reverter),
_ => {
// Code should never reach here as existence of the entrypoint is validated before
// reaching this point.
Ok(CLValue::unit())
}
};

// Charge for the gas spent during execution in an isolated runtime.
Expand Down Expand Up @@ -1693,11 +1703,24 @@ where
return Err(ExecError::InvalidContext);
}

let entry_point = footprint
.entry_points()
.get(entry_point_name)
.cloned()
.ok_or_else(|| ExecError::NoSuchMethod(entry_point_name.to_owned()))?;
let entry_point = match footprint.entry_points().get(entry_point_name) {
Some(entry_point) => entry_point,
None => {
match footprint.entity_kind() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit couldn't this be if let EntityKind::System(_) = ?

EntityKind::System(_) => {
self.charge_system_contract_call(
self.context()
.engine_config()
.system_config()
.no_such_entrypoint(),
)?;
}
EntityKind::Account(_) => {}
EntityKind::SmartContract(_) => {}
}
return Err(ExecError::NoSuchMethod(entry_point_name.to_owned()));
}
};

let entry_point_type = entry_point.entry_point_type();

Expand Down Expand Up @@ -1757,7 +1780,7 @@ where
// if session the caller's context
// else the called contract's context
let context_entity_key =
self.get_context_key_for_contract_call(entity_addr, &entry_point)?;
self.get_context_key_for_contract_call(entity_addr, entry_point)?;

let context_entity_hash = context_entity_key
.into_entity_hash_addr()
Expand Down
31 changes: 4 additions & 27 deletions execution_engine_testing/tests/src/test/regression/ee_1163.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
use casper_engine_test_support::{
LmdbWasmTestBuilder, TransferRequestBuilder, DEFAULT_ACCOUNT_ADDR, LOCAL_GENESIS_REQUEST,
};
use casper_execution_engine::engine_state::{Error, WASMLESS_TRANSFER_FIXED_GAS_PRICE};
use casper_execution_engine::engine_state::Error;
use casper_storage::{data_access_layer::TransferRequest, system::transfer::TransferError};
use casper_types::{
account::AccountHash, system::handle_payment, FeeHandling, Gas, MintCosts, Motes, RuntimeArgs,
SystemConfig, U512,
account::AccountHash, system::handle_payment, Gas, MintCosts, Motes, RuntimeArgs, SystemConfig,
U512,
};

const ACCOUNT_1_ADDR: AccountHash = AccountHash::new([1u8; 32]);
Expand All @@ -21,38 +21,15 @@ fn should_enforce_limit_for_user_error(
request: TransferRequest,
) -> Error {
let transfer_cost = Gas::from(SystemConfig::default().mint_costs().transfer);
let transfer_cost_motes =
Motes::from_gas(transfer_cost, WASMLESS_TRANSFER_FIXED_GAS_PRICE).expect("gas overflow");

let default_account = builder
.get_entity_by_account_hash(*DEFAULT_ACCOUNT_ADDR)
.expect("should have default account");
let main_purse = default_account.main_purse();
let purse_balance_before = builder.get_purse_balance(main_purse);
let proposer_purse_balance_before = builder.get_proposer_purse_balance();

builder.transfer_and_commit(request);

let purse_balance_after = builder.get_purse_balance(main_purse);
let proposer_purse_balance_after = builder.get_proposer_purse_balance();

let response = builder
.get_exec_result_owned(0)
.expect("should have result");

assert_eq!(response.limit(), transfer_cost);
let fee_handling = builder.chainspec().core_config.fee_handling;
if fee_handling == FeeHandling::PayToProposer {
assert_eq!(
purse_balance_before - transfer_cost_motes.value(),
purse_balance_after
);
assert_eq!(
proposer_purse_balance_before + transfer_cost_motes.value(),
proposer_purse_balance_after
);
}
// Verify handle payment postconditions
assert_eq!(response.consumed(), transfer_cost);

let handle_payment = builder.get_handle_payment_contract();
let payment_purse = handle_payment
Expand Down
55 changes: 29 additions & 26 deletions execution_engine_testing/tests/src/test/system_costs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -625,10 +625,6 @@ fn should_charge_for_erroneous_system_contract_calls() {
let mint_hash = builder.get_mint_contract_hash();
let handle_payment_hash = builder.get_handle_payment_contract_hash();

let account = builder
.get_entity_by_account_hash(*DEFAULT_ACCOUNT_ADDR)
.expect("should have account");

let system_config = builder.chainspec().system_costs_config;

// Entrypoints that could fail early due to missing arguments
Expand All @@ -641,37 +637,37 @@ fn should_charge_for_erroneous_system_contract_calls() {
(
auction_hash,
auction::METHOD_WITHDRAW_BID,
system_config.auction_costs().withdraw_bid.into(),
system_config.auction_costs().withdraw_bid,
),
(
auction_hash,
auction::METHOD_DELEGATE,
system_config.auction_costs().delegate.into(),
system_config.auction_costs().delegate,
),
(
auction_hash,
auction::METHOD_UNDELEGATE,
system_config.auction_costs().undelegate.into(),
system_config.auction_costs().undelegate,
),
(
auction_hash,
auction::METHOD_REDELEGATE,
system_config.auction_costs().redelegate.into(),
system_config.auction_costs().redelegate,
),
(
auction_hash,
auction::METHOD_RUN_AUCTION,
system_config.auction_costs().run_auction.into(),
system_config.auction_costs().run_auction,
),
(
auction_hash,
auction::METHOD_SLASH,
system_config.auction_costs().slash.into(),
system_config.auction_costs().slash,
),
(
auction_hash,
auction::METHOD_DISTRIBUTE,
system_config.auction_costs().distribute.into(),
system_config.auction_costs().distribute,
),
(
mint_hash,
Expand Down Expand Up @@ -703,6 +699,21 @@ fn should_charge_for_erroneous_system_contract_calls() {
// handle_payment::METHOD_FINALIZE_PAYMENT,
// system_config.handle_payment_costs().finalize_payment,
// ),
(
auction_hash,
"this_entrypoint_does_not_exists_1",
system_config.no_such_entrypoint(),
),
(
mint_hash,
"this_entrypoint_does_not_exists_2",
system_config.no_such_entrypoint(),
),
(
handle_payment_hash,
"this_entrypoint_does_not_exists_3",
system_config.no_such_entrypoint(),
),
];

for (contract_hash, entrypoint, expected_cost) in entrypoint_calls {
Expand All @@ -714,31 +725,22 @@ fn should_charge_for_erroneous_system_contract_calls() {
)
.build();

let balance_before = builder.get_purse_balance(account.main_purse());

let proposer_reward_starting_balance = builder.get_proposer_purse_balance();

builder.exec(exec_request).commit();

let _error = builder
.get_last_exec_result()
.expect("should have results")
.error()
.cloned()
.unwrap_or_else(|| panic!("should have error while executing {}", entrypoint));

let transaction_fee =
builder.get_proposer_purse_balance() - proposer_reward_starting_balance;

let balance_after = builder.get_purse_balance(account.main_purse());
// assert!(matches!(
// error,
// Error::Exec(ExecError::NoSuchMethod(ref no_such_method)) if no_such_method ==
// entrypoint), "{:?}", error);

let call_cost = U512::from(expected_cost);
assert_eq!(
balance_after,
balance_before - transaction_fee,
"Calling a failed entrypoint {} does not incur expected cost of {}",
entrypoint,
expected_cost,
);

assert_eq!(
builder.last_exec_gas_consumed().value(),
call_cost,
Expand Down Expand Up @@ -856,6 +858,7 @@ fn should_verify_wasm_add_bid_wasm_cost_is_not_recursive() {
let new_handle_payment_costs = HandlePaymentCosts::default();

let system_costs_config = SystemConfig::new(
1,
new_auction_costs,
new_mint_costs,
new_handle_payment_costs,
Expand Down
2 changes: 1 addition & 1 deletion node/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ tokio-serde = { version = "0.8.0", features = ["bincode"] }
tokio-stream = { version = "0.1.4", features = ["sync"] }
tokio-util = { version = "0.6.4", features = ["codec"] }
mio = "0.8.11"
toml = { version = "0.7.8", features = ["preserve_order"] }
toml = { version = "0.8.19", features = ["preserve_order"] }
tower = { version = "0.4.6", features = ["limit"] }
tracing = "0.1.18"
tracing-futures = "0.2.5"
Expand Down
10 changes: 8 additions & 2 deletions node/src/reactor/main_reactor/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3283,8 +3283,14 @@ async fn run_gas_price_scenario(gas_price_scenario: GasPriceScenario) {
.highest_complete_block()
.maybe_current_gas_price()
.expect("must have gas price");
let cost =
fixture.chainspec.system_costs_config.mint_costs().transfer * (current_gas_price as u32);

let cost = match fixture.chainspec.core_config.pricing_handling {
PricingHandling::Classic => 0,
PricingHandling::Fixed => {
fixture.chainspec.system_costs_config.mint_costs().transfer * (current_gas_price as u32)
}
};

Comment on lines +3286 to +3293
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand why Classic is set to 0 here...in classic pricing the imputed amount should be used and 0 is not a valid imputed amount

assert_eq!(holds_after, holds_before + U512::from(cost));

// Run the network at zero load and ensure the value falls back to the floor.
Expand Down
Loading
Loading