Skip to content

Commit

Permalink
improve error msgs (#4548)
Browse files Browse the repository at this point in the history 
* improve error msgs

* improve err msgs

* fix test
  • Loading branch information
@damip
damip authored Nov 21, 2023
1 parent 88cf02e commit 5a165b4
Show file tree
Hide file tree
Showing 8 changed files with 111 additions and 80 deletions.
64 changes: 32 additions & 32 deletions Cargo.lock
View file

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

2 changes: 1 addition & 1 deletion Cargo.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ massa_wallet = { path = "./massa-wallet" }

# Massa projects dependencies
massa-proto-rs = { git = "https://github.com/massalabs/massa-proto-rs", "rev" = "effc0418977cd64402172b7eb749a2fc7537771a" }
massa-sc-runtime = { git = "https://github.com/massalabs/massa-sc-runtime", "rev" = "0be22626a5a873e6a17893e1ad8cc3a44a539308" }
massa-sc-runtime = { git = "https://github.com/massalabs/massa-sc-runtime", "rev" = "32eaf5e65972da79c0357ee9f1827bcaf7b02b94" }
peernet = { git = "https://github.com/massalabs/PeerNet", "branch" = "main" }

# Common dependencies
Expand Down
2 changes: 1 addition & 1 deletion massa-execution-worker/src/tests/scenarios_mandatories.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1814,7 +1814,7 @@ fn not_enough_instance_gas() {
.get_filtered_sc_output_event(EventFilter::default());
assert!(events[0]
.data
.contains("Provided max gas is below the default instance creation cost"));
.contains("is lower than the base instance creation gas"));
}

#[test]
Expand Down
49 changes: 30 additions & 19 deletions massa-module-cache/src/controller.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use massa_hash::Hash;
use massa_models::prehash::BuildHashMapper;
use massa_sc_runtime::{Compiler, RuntimeModule};
use schnellru::{ByLength, LruMap};
use tracing::{debug, info, warn};
use tracing::debug;

use crate::{
config::ModuleCacheConfig, error::CacheError, hd_cache::HDCache, lru_cache::LRUCache,
Expand Down Expand Up @@ -47,8 +47,9 @@ impl ModuleCache {
ModuleInfo::Module(module)
}
Err(e) => {
warn!("compilation of module {} failed with: {}", hash, e);
ModuleInfo::Invalid
let err_msg = format!("compilation of module {} failed: {}", hash, e);
debug!(err_msg);
ModuleInfo::Invalid(err_msg)
}
}
}
Expand Down Expand Up @@ -78,10 +79,10 @@ impl ModuleCache {
}

/// Set a cached module as invalid
pub fn set_invalid(&mut self, bytecode: &[u8]) {
pub fn set_invalid(&mut self, bytecode: &[u8], err_msg: String) {
let hash = Hash::compute_from(bytecode);
self.lru_cache.set_invalid(hash);
self.hd_cache.set_invalid(hash);
self.lru_cache.set_invalid(hash, err_msg.clone());
self.hd_cache.set_invalid(hash, err_msg);
}

/// Load a cached module for execution
Expand All @@ -91,13 +92,19 @@ impl ModuleCache {
/// * `ModuleInfo::Module` if the module is valid and has no delta
/// * `ModuleInfo::ModuleAndDelta` if the module is valid and has a delta
fn load_module_info(&mut self, bytecode: &[u8]) -> ModuleInfo {
if bytecode.is_empty() {
let error_msg = "load_module: bytecode is absent".to_string();
debug!(error_msg);
return ModuleInfo::Invalid(error_msg);
}
if bytecode.len() > self.cfg.max_module_length as usize {
info!(
let error_msg = format!(
"load_module: bytecode length {} exceeds max module length {}",
bytecode.len(),
self.cfg.max_module_length
);
return ModuleInfo::Invalid;
debug!(error_msg);
return ModuleInfo::Invalid(error_msg);
}
let hash = Hash::compute_from(bytecode);
if let Some(lru_module_info) = self.lru_cache.get(hash) {
Expand Down Expand Up @@ -129,22 +136,25 @@ impl ModuleCache {
// This is only supposed to be a check
execution_gas
.checked_sub(self.cfg.gas_costs.max_instance_cost)
.ok_or(CacheError::LoadError(
"Provided max gas is below the default instance creation cost".to_string(),
))?;
.ok_or(CacheError::LoadError(format!(
"Provided gas {} is lower than the base instance creation gas cost {}",
execution_gas, self.cfg.gas_costs.max_instance_cost
)))?;
// TODO: interesting but unimportant optim
// remove max_instance_cost hard check if module is cached and has a delta
let module_info = self.load_module_info(bytecode);
let module = match module_info {
ModuleInfo::Invalid => {
return Err(CacheError::LoadError("Loading invalid module".to_string()));
ModuleInfo::Invalid(err) => {
let err_msg = format!("invalid module: {}", err);
return Err(CacheError::LoadError(err_msg));
}
ModuleInfo::Module(module) => module,
ModuleInfo::ModuleAndDelta((module, delta)) => {
if delta > execution_gas {
return Err(CacheError::LoadError(
"Provided max gas is below the instance creation cost".to_string(),
));
return Err(CacheError::LoadError(format!(
"Provided gas {} is below the gas cost of instance creation ({})",
execution_gas, delta
)));
} else {
module
}
Expand All @@ -167,9 +177,10 @@ impl ModuleCache {
// This is only supposed to be a check
limit
.checked_sub(self.cfg.gas_costs.max_instance_cost)
.ok_or(CacheError::LoadError(
"Provided max gas is below the default instance creation cost".to_string(),
))?;
.ok_or(CacheError::LoadError(format!(
"Provided gas {} is lower than the base instance creation gas cost {}",
limit, self.cfg.gas_costs.max_instance_cost
)))?;
let module = RuntimeModule::new(bytecode, self.cfg.gas_costs.clone(), Compiler::SP)?;
Ok((module, limit))
}
Expand Down
23 changes: 13 additions & 10 deletions massa-module-cache/src/hd_cache.rs
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
use crate::types::{
ModuleInfo, ModuleMetadata, ModuleMetadataDeserializer, ModuleMetadataSerializer,
};
use core::panic;
use massa_hash::Hash;
use massa_sc_runtime::{GasCosts, RuntimeModule};
use massa_serialization::{DeserializeError, Deserializer, Serializer};
Expand Down Expand Up @@ -81,9 +80,9 @@ impl HDCache {

let mut ser_metadata = Vec::new();
let ser_module = match module_info {
ModuleInfo::Invalid => {
ModuleInfo::Invalid(err_msg) => {
self.meta_ser
.serialize(&ModuleMetadata::Invalid, &mut ser_metadata)
.serialize(&ModuleMetadata::Invalid(err_msg), &mut ser_metadata)
.expect(DATA_SER_ERROR);
Vec::new()
}
Expand Down Expand Up @@ -127,10 +126,10 @@ impl HDCache {
}

/// Sets a given module as invalid
pub fn set_invalid(&self, hash: Hash) {
pub fn set_invalid(&self, hash: Hash, err_msg: String) {
let mut ser_metadata = Vec::new();
self.meta_ser
.serialize(&ModuleMetadata::Invalid, &mut ser_metadata)
.serialize(&ModuleMetadata::Invalid(err_msg), &mut ser_metadata)
.expect(DATA_SER_ERROR);
self.db
.put(metadata_key!(hash), ser_metadata)
Expand All @@ -151,14 +150,14 @@ impl HDCache {
.meta_deser
.deserialize::<DeserializeError>(&ser_metadata)
.expect(DATA_DESER_ERROR);
if metadata == ModuleMetadata::Invalid {
return Some(ModuleInfo::Invalid);
if let ModuleMetadata::Invalid(err_msg) = metadata {
return Some(ModuleInfo::Invalid(err_msg));
}
let module =
RuntimeModule::deserialize(&ser_module, gas_costs.max_instance_cost, gas_costs)
.expect(MOD_DESER_ERROR);
let result = match metadata {
ModuleMetadata::Invalid => ModuleInfo::Invalid,
ModuleMetadata::Invalid(err_msg) => ModuleInfo::Invalid(err_msg),
ModuleMetadata::NotExecuted => ModuleInfo::Module(module),
ModuleMetadata::Delta(delta) => ModuleInfo::ModuleAndDelta((module, delta)),
};
Expand Down Expand Up @@ -261,9 +260,13 @@ mod tests {
let cached_module_v2 = cache.get(hash, gas_costs.clone()).unwrap();
assert!(matches!(cached_module_v2, ModuleInfo::ModuleAndDelta(_)));

cache.set_invalid(hash);
let err_msg = "test_error".to_string();
cache.set_invalid(hash, err_msg.clone());
let cached_module_v3 = cache.get(hash, gas_costs).unwrap();
assert!(matches!(cached_module_v3, ModuleInfo::Invalid));
let ModuleInfo::Invalid(res_err) = cached_module_v3 else {
panic!("expected ModuleInfo::Invalid");
};
assert_eq!(res_err, err_msg);
}

#[test]
Expand Down
Loading

0 comments on commit 5a165b4

Please sign in to comment.