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

[BUGFIX] Fix small wasm / big args lane assignment #5065

Merged
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
4 changes: 2 additions & 2 deletions node/src/types/transaction/meta_transaction.rs
Original file line number Diff line number Diff line change
Expand Up @@ -353,14 +353,14 @@ mod proptests {
TransactionLaneDefinition {
id: 3,
max_transaction_length: u64::MAX/2,
max_transaction_args_length: 100,
max_transaction_args_length: 10000,
max_transaction_gas_limit: u64::MAX/2,
max_transaction_count: 10,
},
TransactionLaneDefinition {
id: 4,
max_transaction_length: u64::MAX,
max_transaction_args_length: 100,
max_transaction_args_length: 10000,
max_transaction_gas_limit: u64::MAX,
max_transaction_count: 10,
},
Expand Down
37 changes: 30 additions & 7 deletions node/src/types/transaction/meta_transaction/lane_id.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ pub(crate) fn calculate_transaction_lane(
pricing_mode: &PricingMode,
config: &TransactionV1Config,
size_estimation: u64,
runtime_args_size: u64,
) -> Result<u8, InvalidTransactionV1> {
match target {
TransactionTarget::Native => match entry_point {
Expand All @@ -31,9 +32,12 @@ pub(crate) fn calculate_transaction_lane(
}
},
TransactionTarget::Stored { .. } => match entry_point {
TransactionEntryPoint::Custom(_) => {
get_lane_for_non_install_wasm(config, size_estimation, pricing_mode)
}
TransactionEntryPoint::Custom(_) => get_lane_for_non_install_wasm(
config,
size_estimation,
pricing_mode,
runtime_args_size,
),
TransactionEntryPoint::Call
| TransactionEntryPoint::Transfer
| TransactionEntryPoint::AddBid
Expand All @@ -59,7 +63,12 @@ pub(crate) fn calculate_transaction_lane(
if *is_install_upgrade {
Ok(INSTALL_UPGRADE_LANE_ID)
} else {
get_lane_for_non_install_wasm(config, size_estimation, pricing_mode)
get_lane_for_non_install_wasm(
config,
size_estimation,
pricing_mode,
runtime_args_size,
)
}
}
TransactionEntryPoint::Custom(_)
Expand Down Expand Up @@ -87,7 +96,12 @@ pub(crate) fn calculate_transaction_lane(
if *is_install_upgrade {
Ok(INSTALL_UPGRADE_LANE_ID)
} else {
get_lane_for_non_install_wasm(config, size_estimation, pricing_mode)
get_lane_for_non_install_wasm(
config,
size_estimation,
pricing_mode,
runtime_args_size,
)
}
}
TransactionEntryPoint::Transfer
Expand All @@ -112,16 +126,25 @@ fn get_lane_for_non_install_wasm(
config: &TransactionV1Config,
transaction_size: u64,
pricing_mode: &PricingMode,
runtime_args_size: u64,
) -> Result<u8, InvalidTransactionV1> {
match pricing_mode {
PricingMode::PaymentLimited { payment_amount, .. } => config
.get_wasm_lane_id_by_payment_limited(*payment_amount, transaction_size)
.get_wasm_lane_id_by_payment_limited(
*payment_amount,
transaction_size,
runtime_args_size,
)
.ok_or(InvalidTransactionV1::NoWasmLaneMatchesTransaction()),
PricingMode::Fixed {
additional_computation_factor,
..
} => config
.get_wasm_lane_id_by_size(transaction_size, *additional_computation_factor)
.get_wasm_lane_id_by_size(
transaction_size,
*additional_computation_factor,
runtime_args_size,
)
.ok_or(InvalidTransactionV1::NoWasmLaneMatchesTransaction()),
PricingMode::Prepaid { .. } => Err(InvalidTransactionV1::PricingModeNotSupported),
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,12 @@ impl MetaTransactionV1 {
v1: &TransactionV1,
transaction_v1_config: &TransactionV1Config,
) -> Result<MetaTransactionV1, InvalidTransaction> {
let args_binary_len = v1
.payload()
.fields()
.get(&ARGS_MAP_KEY)
.map(|field| field.len())
.unwrap_or(0);
let args: TransactionArgs = v1.deserialize_field(ARGS_MAP_KEY).map_err(|error| {
InvalidTransaction::V1(InvalidTransactionV1::CouldNotDeserializeField { error })
})?;
Expand All @@ -72,13 +78,13 @@ impl MetaTransactionV1 {
let payload_hash = v1.payload_hash()?;
let serialized_length = v1.serialized_length();
let pricing_mode = v1.payload().pricing_mode();

let lane_id = calculate_transaction_lane(
&entry_point,
&target,
pricing_mode,
transaction_v1_config,
serialized_length as u64,
args_binary_len as u64,
)?;
let has_valid_hash = v1.has_valid_hash();
let approvals = v1.approvals().clone();
Expand Down
15 changes: 12 additions & 3 deletions types/src/auction_state.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
use alloc::collections::{btree_map::Entry, BTreeMap};
#![allow(deprecated)]

use alloc::vec::Vec;
use alloc::{
collections::{btree_map::Entry, BTreeMap},
vec::Vec,
};
#[cfg(feature = "json-schema")]
use once_cell::sync::Lazy;
#[cfg(feature = "json-schema")]
Expand Down Expand Up @@ -32,6 +35,7 @@ static ERA_VALIDATORS: Lazy<EraValidators> = Lazy::new(|| {

era_validators
});

#[cfg(feature = "json-schema")]
static AUCTION_INFO: Lazy<AuctionState> = Lazy::new(|| {
use crate::{system::auction::DelegationRate, AccessRights, SecretKey, URef};
Expand Down Expand Up @@ -74,6 +78,7 @@ static AUCTION_INFO: Lazy<AuctionState> = Lazy::new(|| {
#[derive(Debug, Serialize, Deserialize, PartialEq, Eq, Clone)]
#[cfg_attr(feature = "json-schema", derive(JsonSchema))]
#[serde(deny_unknown_fields)]
#[deprecated(since = "5.0.0")]
pub struct JsonValidatorWeights {
public_key: PublicKey,
weight: U512,
Expand All @@ -83,6 +88,7 @@ pub struct JsonValidatorWeights {
#[derive(Debug, Serialize, Deserialize, PartialEq, Eq, Clone)]
#[cfg_attr(feature = "json-schema", derive(JsonSchema))]
#[serde(deny_unknown_fields)]
#[deprecated(since = "5.0.0")]
pub struct JsonEraValidators {
era_id: EraId,
validator_weights: Vec<JsonValidatorWeights>,
Expand All @@ -92,6 +98,7 @@ pub struct JsonEraValidators {
#[derive(Debug, Serialize, Deserialize, PartialEq, Eq, Clone)]
#[cfg_attr(feature = "json-schema", derive(JsonSchema))]
#[serde(deny_unknown_fields)]
#[deprecated(since = "5.0.0")]
pub struct AuctionState {
/// Global state hash.
pub state_root_hash: Digest,
Expand All @@ -106,6 +113,8 @@ pub struct AuctionState {

impl AuctionState {
/// Create new instance of `AuctionState`
/// this logic will retrofit new data into old structure if applicable (it's a lossy
/// conversion).
pub fn new(
state_root_hash: Digest,
block_height: u64,
Expand Down Expand Up @@ -186,7 +195,7 @@ impl AuctionState {
state_root_hash,
block_height,
era_validators: json_era_validators,
bids: BTreeMap::new(), // TODO: figure out if we can retro compat or not
bids,
}
}

Expand Down
80 changes: 51 additions & 29 deletions types/src/chainspec/transaction_config/transaction_v1_config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ pub struct TransactionLaneDefinition {
pub id: u8,
/// The maximum length of a transaction in bytes
pub max_transaction_length: u64,
/// The maximum number of runtime args
/// The max args length size in bytes
pub max_transaction_args_length: u64,
/// The maximum gas limit
pub max_transaction_gas_limit: u64,
Expand Down Expand Up @@ -373,13 +373,17 @@ impl TransactionV1Config {
&self,
transaction_size: u64,
additional_computation_factor: u8,
runtime_args_size: u64,
) -> Option<u8> {
let mut maybe_adequate_lane_index = None;
let buckets = self.get_wasm_lanes_ordered_by_transaction_size();
let number_of_lanes = buckets.len();
for (i, lane) in buckets.iter().enumerate() {
let lane_size = lane.max_transaction_length;
if lane_size >= transaction_size {
let max_transaction_size = lane.max_transaction_length;
let max_runtime_args_size = lane.max_transaction_args_length;
if max_transaction_size >= transaction_size
&& max_runtime_args_size >= runtime_args_size
{
maybe_adequate_lane_index = Some(i);
break;
}
Expand All @@ -397,13 +401,18 @@ impl TransactionV1Config {
&self,
gas_limit: u64,
transaction_size: u64,
runtime_args_size: u64,
) -> Option<u8> {
let mut maybe_adequate_lane_index = None;
let lanes = self.get_wasm_lanes_ordered_by_gas_limit();
for (i, lane) in lanes.iter().enumerate() {
let max_transaction_gas = lane.max_transaction_gas_limit;
let max_transaction_size = lane.max_transaction_length;
if max_transaction_gas >= gas_limit && max_transaction_size >= transaction_size {
let max_runtime_args_size = lane.max_transaction_args_length;
if max_transaction_gas >= gas_limit
&& max_transaction_size >= transaction_size
&& max_runtime_args_size >= runtime_args_size
{
maybe_adequate_lane_index = Some(i);
break;
}
Expand Down Expand Up @@ -654,85 +663,94 @@ mod tests {
#[test]
fn should_get_configuration_for_wasm() {
let config = build_example_transaction_config();
let got = config.get_wasm_lane_id_by_size(100, 0);
let got = config.get_wasm_lane_id_by_size(100, 0, 0);
assert_eq!(got, Some(3));
let config = build_example_transaction_config_reverse_wasm_ids();
let got = config.get_wasm_lane_id_by_size(100, 0);
let got = config.get_wasm_lane_id_by_size(100, 0, 0);
assert_eq!(got, Some(5));
}

#[test]
fn given_too_big_transaction_should_return_none() {
let config = build_example_transaction_config();
let got = config.get_wasm_lane_id_by_size(100000000, 0);
let got = config.get_wasm_lane_id_by_size(100000000, 0, 0);
assert!(got.is_none());
let config = build_example_transaction_config_reverse_wasm_ids();
let got = config.get_wasm_lane_id_by_size(100000000, 0, 0);
assert!(got.is_none());
let config = build_example_transaction_config_reverse_wasm_ids();
let got = config.get_wasm_lane_id_by_size(100000000, 0);
let got = config.get_wasm_lane_id_by_size(1, 0, 100000);
assert!(got.is_none());
}

#[test]
fn given_wasm_should_return_first_fit() {
let config = build_example_transaction_config();

let got = config.get_wasm_lane_id_by_size(660, 0);
let got = config.get_wasm_lane_id_by_size(660, 0, 0);
assert_eq!(got, Some(4));

let got = config.get_wasm_lane_id_by_size(800, 0);
let got = config.get_wasm_lane_id_by_size(800, 0, 0);
assert_eq!(got, Some(5));

let got = config.get_wasm_lane_id_by_size(1, 0);
let got = config.get_wasm_lane_id_by_size(1, 0, 0);
assert_eq!(got, Some(3));

let got = config.get_wasm_lane_id_by_size(800, 0, 6024);
assert_eq!(got, Some(5));

let config = build_example_transaction_config_reverse_wasm_ids();

let got = config.get_wasm_lane_id_by_size(660, 0);
let got = config.get_wasm_lane_id_by_size(660, 0, 0);
assert_eq!(got, Some(4));

let got = config.get_wasm_lane_id_by_size(800, 0);
let got = config.get_wasm_lane_id_by_size(800, 0, 0);
assert_eq!(got, Some(3));

let got = config.get_wasm_lane_id_by_size(1, 0);
let got = config.get_wasm_lane_id_by_size(1, 0, 0);
assert_eq!(got, Some(5));

let got = config.get_wasm_lane_id_by_size(800, 0, 6024);
assert_eq!(got, Some(3));
}

#[test]
fn given_additional_computation_factor_should_be_applied() {
let config = build_example_transaction_config();
let got = config.get_wasm_lane_id_by_size(660, 1);
let got = config.get_wasm_lane_id_by_size(660, 1, 0);
assert_eq!(got, Some(5));

let config = build_example_transaction_config_reverse_wasm_ids();
let got = config.get_wasm_lane_id_by_size(660, 1);
let got = config.get_wasm_lane_id_by_size(660, 1, 0);
assert_eq!(got, Some(3));
}

#[test]
fn given_additional_computation_factor_should_not_overflow() {
let config = build_example_transaction_config();
let got = config.get_wasm_lane_id_by_size(660, 2);
let got = config.get_wasm_lane_id_by_size(660, 2, 0);
assert_eq!(got, Some(5));
let got_2 = config.get_wasm_lane_id_by_size(660, 20);
let got_2 = config.get_wasm_lane_id_by_size(660, 20, 0);
assert_eq!(got_2, Some(5));

let config = build_example_transaction_config_reverse_wasm_ids();
let got = config.get_wasm_lane_id_by_size(660, 2);
let got = config.get_wasm_lane_id_by_size(660, 2, 0);
assert_eq!(got, Some(3));
let got_2 = config.get_wasm_lane_id_by_size(660, 20);
let got_2 = config.get_wasm_lane_id_by_size(660, 20, 0);
assert_eq!(got_2, Some(3));
}

#[test]
fn given_no_wasm_lanes_should_return_none() {
let config = build_example_transaction_config_no_wasms();
let got = config.get_wasm_lane_id_by_size(660, 2);
let got = config.get_wasm_lane_id_by_size(660, 2, 0);
assert!(got.is_none());
let got = config.get_wasm_lane_id_by_size(660, 0);
let got = config.get_wasm_lane_id_by_size(660, 0, 0);
assert!(got.is_none());
let got = config.get_wasm_lane_id_by_size(660, 20);
let got = config.get_wasm_lane_id_by_size(660, 20, 0);
assert!(got.is_none());

let got = config.get_wasm_lane_id_by_payment_limited(100, 1);
let got = config.get_wasm_lane_id_by_payment_limited(100, 1, 0);
assert!(got.is_none());
}

Expand Down Expand Up @@ -760,14 +778,16 @@ mod tests {
TransactionLaneDefinition {
id: 5,
max_transaction_length: 12,
max_transaction_args_length: 1,
max_transaction_args_length: 5,
max_transaction_gas_limit: 155,
max_transaction_count: 1,
},
],
);
let got = config.get_wasm_lane_id_by_payment_limited(54, 1);
let got = config.get_wasm_lane_id_by_payment_limited(54, 1, 0);
assert_eq!(got, Some(4));
let got = config.get_wasm_lane_id_by_payment_limited(54, 10, 3);
assert_eq!(got, Some(5));
}

#[test]
Expand Down Expand Up @@ -800,7 +820,7 @@ mod tests {
},
],
);
let got = config.get_wasm_lane_id_by_payment_limited(54, 12);
let got = config.get_wasm_lane_id_by_payment_limited(54, 12, 0);
assert_eq!(got, Some(5));
}

Expand Down Expand Up @@ -828,13 +848,15 @@ mod tests {
TransactionLaneDefinition {
id: 5,
max_transaction_length: 12,
max_transaction_args_length: 1,
max_transaction_args_length: 5,
max_transaction_gas_limit: 155,
max_transaction_count: 1,
},
],
);
let got = config.get_wasm_lane_id_by_payment_limited(54, 120);
let got = config.get_wasm_lane_id_by_payment_limited(54, 120, 0);
assert_eq!(got, None);
let got = config.get_wasm_lane_id_by_payment_limited(54, 10, 1000);
assert_eq!(got, None);
}

Expand Down
Loading
Loading