From ad4f2cb3c08049f3961124582ddf194d97a7b6ae Mon Sep 17 00:00:00 2001 From: Koute Date: Wed, 9 Feb 2022 18:12:55 +0900 Subject: [PATCH] Add a new host function for reporting fatal errors; make WASM backtraces readable when printing out errors (#10741) * Add a new host function for reporting fatal errors * Fix one of the wasmtime executor tests * Have `#[runtime_interface(wasm_only)]` actually mean WASM-only, and not no_std-only * Print out errors through `Display` instead of `Debug` * Switch one more trait to require `Error` for its error instead of only `Debug` * Align to review comments --- client/authority-discovery/src/error.rs | 2 +- client/authority-discovery/src/worker.rs | 8 +- client/beefy/src/worker.rs | 2 +- client/consensus/aura/src/import_queue.rs | 4 +- client/consensus/babe/rpc/src/lib.rs | 4 +- client/consensus/babe/src/lib.rs | 18 ++-- client/consensus/common/src/import_queue.rs | 17 +++- .../common/src/import_queue/basic_queue.rs | 4 +- .../manual-seal/src/consensus/babe.rs | 2 +- .../manual-seal/src/finalize_block.rs | 2 +- client/consensus/manual-seal/src/rpc.rs | 5 +- .../consensus/manual-seal/src/seal_block.rs | 7 +- client/consensus/pow/src/lib.rs | 24 ++--- client/consensus/pow/src/worker.rs | 4 +- client/consensus/slots/src/lib.rs | 17 ++-- client/consensus/slots/src/slots.rs | 2 +- client/executor/common/src/error.rs | 44 ++++++++- client/executor/runtime-test/Cargo.toml | 2 +- client/executor/runtime-test/src/lib.rs | 8 ++ client/executor/src/integration_tests/mod.rs | 92 +++++++++++-------- client/executor/src/native_executor.rs | 27 +++--- client/executor/src/wasm_runtime.rs | 4 +- client/executor/wasmi/src/lib.rs | 46 ++++++++-- client/executor/wasmtime/src/host.rs | 15 +++ .../executor/wasmtime/src/instance_wrapper.rs | 44 +++++++-- client/executor/wasmtime/src/tests.rs | 14 +-- client/finality-grandpa/src/aux_schema.rs | 2 +- client/finality-grandpa/src/environment.rs | 12 +-- client/finality-grandpa/src/import.rs | 10 +- client/finality-grandpa/src/lib.rs | 23 ++++- client/finality-grandpa/src/observer.rs | 2 +- client/network/src/protocol.rs | 2 +- client/network/src/protocol/sync.rs | 10 +- .../src/protocol/sync/extra_requests.rs | 2 +- client/network/src/protocol/sync/state.rs | 2 +- client/network/src/protocol/sync/warp.rs | 2 +- client/offchain/src/lib.rs | 2 +- client/rpc-api/src/author/error.rs | 2 +- client/rpc-api/src/errors.rs | 6 +- client/service/src/chain_ops/check_block.rs | 2 +- client/service/src/chain_ops/import_blocks.rs | 2 +- client/service/src/client/call_executor.rs | 4 +- client/service/src/client/client.rs | 6 +- client/service/src/client/wasm_override.rs | 2 +- client/service/src/client/wasm_substitutes.rs | 2 +- client/service/src/lib.rs | 2 +- client/tracing/src/block/mod.rs | 2 +- client/transaction-pool/api/src/lib.rs | 2 +- client/transaction-pool/src/api.rs | 12 +-- client/transaction-pool/src/lib.rs | 10 +- client/transaction-pool/src/revalidation.rs | 2 +- frame/contracts/rpc/src/lib.rs | 4 +- frame/merkle-mountain-range/rpc/src/lib.rs | 4 +- frame/transaction-payment/rpc/src/lib.rs | 4 +- .../api/proc-macro/src/impl_runtime_apis.rs | 2 +- primitives/blockchain/src/error.rs | 2 +- primitives/consensus/common/src/lib.rs | 4 +- primitives/io/Cargo.toml | 19 ++++ primitives/io/src/lib.rs | 40 ++++++-- .../bare_function_interface.rs | 39 ++++++-- .../host_function_interface.rs | 4 +- .../src/runtime_interface/trait_decl_impl.rs | 2 +- .../runtime-interface/proc-macro/src/utils.rs | 91 +++++++++++++----- primitives/runtime/src/traits.rs | 2 +- primitives/wasm-interface/src/lib.rs | 23 +++++ utils/frame/benchmarking-cli/src/command.rs | 8 +- utils/frame/rpc/system/src/lib.rs | 4 +- utils/frame/try-runtime/cli/src/lib.rs | 4 +- 68 files changed, 552 insertions(+), 247 deletions(-) diff --git a/client/authority-discovery/src/error.rs b/client/authority-discovery/src/error.rs index bad53e905cb93..bce39069ef7c7 100644 --- a/client/authority-discovery/src/error.rs +++ b/client/authority-discovery/src/error.rs @@ -38,7 +38,7 @@ pub enum Error { #[error("Failed to hash the authority id to be used as a dht key.")] HashingAuthorityId(#[from] libp2p::core::multiaddr::multihash::Error), - #[error("Failed calling into the Substrate runtime.")] + #[error("Failed calling into the Substrate runtime: {0}")] CallingRuntime(#[from] sp_blockchain::Error), #[error("Received a dht record with a key that does not match any in-flight awaited keys.")] diff --git a/client/authority-discovery/src/worker.rs b/client/authority-discovery/src/worker.rs index ee5a15b9533b7..019abaac3cfcb 100644 --- a/client/authority-discovery/src/worker.rs +++ b/client/authority-discovery/src/worker.rs @@ -187,7 +187,7 @@ where Some(registry) => match Metrics::register(®istry) { Ok(metrics) => Some(metrics), Err(e) => { - error!(target: LOG_TARGET, "Failed to register metrics: {:?}", e); + error!(target: LOG_TARGET, "Failed to register metrics: {}", e); None }, }, @@ -242,7 +242,7 @@ where if let Err(e) = self.publish_ext_addresses(only_if_changed).await { error!( target: LOG_TARGET, - "Failed to publish external addresses: {:?}", e, + "Failed to publish external addresses: {}", e, ); } }, @@ -251,7 +251,7 @@ where if let Err(e) = self.refill_pending_lookups_queue().await { error!( target: LOG_TARGET, - "Failed to request addresses of authorities: {:?}", e, + "Failed to request addresses of authorities: {}", e, ); } }, @@ -426,7 +426,7 @@ where metrics.handle_value_found_event_failure.inc(); } - debug!(target: LOG_TARGET, "Failed to handle Dht value found event: {:?}", e); + debug!(target: LOG_TARGET, "Failed to handle Dht value found event: {}", e); } }, DhtEvent::ValueNotFound(hash) => { diff --git a/client/beefy/src/worker.rs b/client/beefy/src/worker.rs index 0c7d8d4ffdc9c..3f23638758eca 100644 --- a/client/beefy/src/worker.rs +++ b/client/beefy/src/worker.rs @@ -378,7 +378,7 @@ where .expect("forwards closure result; the closure always returns Ok; qed."); } }) { - error!(target: "beefy", "🥩 Failed to get hash for block number {}; err: {:?}", + error!(target: "beefy", "🥩 Failed to get hash for block number {}: {}", block_num, err); } diff --git a/client/consensus/aura/src/import_queue.rs b/client/consensus/aura/src/import_queue.rs index 593ea193c3d12..56eb45c621a1b 100644 --- a/client/consensus/aura/src/import_queue.rs +++ b/client/consensus/aura/src/import_queue.rs @@ -205,7 +205,7 @@ where let hash = block.header.hash(); let parent_hash = *block.header.parent_hash(); let authorities = authorities(self.client.as_ref(), &BlockId::Hash(parent_hash)) - .map_err(|e| format!("Could not fetch authorities at {:?}: {:?}", parent_hash, e))?; + .map_err(|e| format!("Could not fetch authorities at {:?}: {}", parent_hash, e))?; let create_inherent_data_providers = self .create_inherent_data_providers @@ -249,7 +249,7 @@ where &BlockId::Hash(parent_hash), |v| v >= 2, ) - .map_err(|e| format!("{:?}", e))? + .map_err(|e| e.to_string())? { self.check_inherents( new_block.clone(), diff --git a/client/consensus/babe/rpc/src/lib.rs b/client/consensus/babe/rpc/src/lib.rs index 462620f26e5bf..88a176e2de10d 100644 --- a/client/consensus/babe/rpc/src/lib.rs +++ b/client/consensus/babe/rpc/src/lib.rs @@ -104,7 +104,7 @@ where let epoch_start = client .runtime_api() .current_epoch_start(&BlockId::Hash(header.hash())) - .map_err(|err| Error::StringError(format!("{:?}", err)))?; + .map_err(|err| Error::StringError(err.to_string()))?; let epoch = epoch_data(&shared_epoch, &client, &babe_config, *epoch_start, &select_chain) .await?; @@ -209,7 +209,7 @@ where slot.into(), |slot| Epoch::genesis(&babe_config, slot), ) - .map_err(|e| Error::Consensus(ConsensusError::ChainLookup(format!("{:?}", e))))? + .map_err(|e| Error::Consensus(ConsensusError::ChainLookup(e.to_string())))? .ok_or(Error::Consensus(ConsensusError::InvalidAuthoritiesSet)) } diff --git a/client/consensus/babe/src/lib.rs b/client/consensus/babe/src/lib.rs index 3e9cf5aab6494..9ad50eb9c0e5c 100644 --- a/client/consensus/babe/src/lib.rs +++ b/client/consensus/babe/src/lib.rs @@ -232,7 +232,7 @@ pub enum Error { #[error("Multiple BABE config change digests, rejecting!")] MultipleConfigChangeDigests, /// Could not extract timestamp and slot - #[error("Could not extract timestamp and slot: {0:?}")] + #[error("Could not extract timestamp and slot: {0}")] Extraction(sp_consensus::Error), /// Could not fetch epoch #[error("Could not fetch epoch at {0:?}")] @@ -274,7 +274,7 @@ pub enum Error { #[error("VRF verification failed: {0:?}")] VRFVerificationFailed(SignatureError), /// Could not fetch parent header - #[error("Could not fetch parent header: {0:?}")] + #[error("Could not fetch parent header: {0}")] FetchParentHeader(sp_blockchain::Error), /// Expected epoch change to happen. #[error("Expected epoch change to happen at {0:?}, s{1}")] @@ -713,7 +713,7 @@ where parent.number().clone(), slot, ) - .map_err(|e| ConsensusError::ChainLookup(format!("{:?}", e)))? + .map_err(|e| ConsensusError::ChainLookup(e.to_string()))? .ok_or(sp_consensus::Error::InvalidAuthoritiesSet) } @@ -1201,7 +1201,7 @@ where ) .await { - warn!(target: "babe", "Error checking/reporting BABE equivocation: {:?}", err); + warn!(target: "babe", "Error checking/reporting BABE equivocation: {}", err); } // if the body is passed through, we need to use the runtime @@ -1551,7 +1551,7 @@ where ) .map_err(|e| { ConsensusError::ClientImport(format!( - "Error importing epoch changes: {:?}", + "Error importing epoch changes: {}", e )) })?; @@ -1559,7 +1559,7 @@ where }; if let Err(e) = prune_and_import() { - debug!(target: "babe", "Failed to launch next epoch: {:?}", e); + debug!(target: "babe", "Failed to launch next epoch: {}", e); *epoch_changes = old_epoch_changes.expect("set `Some` above and not taken; qed"); return Err(e) @@ -1590,7 +1590,7 @@ where parent_weight } else { aux_schema::load_block_weight(&*self.client, last_best) - .map_err(|e| ConsensusError::ChainLookup(format!("{:?}", e)))? + .map_err(|e| ConsensusError::ChainLookup(e.to_string()))? .ok_or_else(|| { ConsensusError::ChainLookup( "No block weight for parent header.".to_string(), @@ -1649,7 +1649,7 @@ where let finalized_slot = { let finalized_header = client .header(BlockId::Hash(info.finalized_hash)) - .map_err(|e| ConsensusError::ClientImport(format!("{:?}", e)))? + .map_err(|e| ConsensusError::ClientImport(e.to_string()))? .expect( "best finalized hash was given by client; finalized headers must exist in db; qed", ); @@ -1666,7 +1666,7 @@ where info.finalized_number, finalized_slot, ) - .map_err(|e| ConsensusError::ClientImport(format!("{:?}", e)))?; + .map_err(|e| ConsensusError::ClientImport(e.to_string()))?; Ok(()) } diff --git a/client/consensus/common/src/import_queue.rs b/client/consensus/common/src/import_queue.rs index f71996fe2b1fa..8b560d0447411 100644 --- a/client/consensus/common/src/import_queue.rs +++ b/client/consensus/common/src/import_queue.rs @@ -161,21 +161,34 @@ pub enum BlockImportStatus { } /// Block import error. -#[derive(Debug)] +#[derive(Debug, thiserror::Error)] pub enum BlockImportError { /// Block missed header, can't be imported + #[error("block is missing a header (origin = {0:?})")] IncompleteHeader(Option), + /// Block verification failed, can't be imported + #[error("block verification failed (origin = {0:?}): {1}")] VerificationFailed(Option, String), + /// Block is known to be Bad + #[error("bad block (origin = {0:?})")] BadBlock(Option), + /// Parent state is missing. + #[error("block is missing parent state")] MissingState, + /// Block has an unknown parent + #[error("block has an unknown parent")] UnknownParent, + /// Block import has been cancelled. This can happen if the parent block fails to be imported. + #[error("import has been cancelled")] Cancelled, + /// Other error. + #[error("consensus error: {0}")] Other(ConsensusError), } @@ -245,7 +258,7 @@ pub(crate) async fn import_single_block_metered< Err(BlockImportError::BadBlock(peer.clone())) }, Err(e) => { - debug!(target: "sync", "Error importing block {}: {:?}: {:?}", number, hash, e); + debug!(target: "sync", "Error importing block {}: {:?}: {}", number, hash, e); Err(BlockImportError::Other(e)) }, }; diff --git a/client/consensus/common/src/import_queue/basic_queue.rs b/client/consensus/common/src/import_queue/basic_queue.rs index 0f23d9b546bd6..5134dc041c26b 100644 --- a/client/consensus/common/src/import_queue/basic_queue.rs +++ b/client/consensus/common/src/import_queue/basic_queue.rs @@ -303,11 +303,11 @@ impl BlockImportWorker { .map_err(|e| { debug!( target: "sync", - "Justification import failed with {:?} for hash: {:?} number: {:?} coming from node: {:?}", - e, + "Justification import failed for hash = {:?} with number = {:?} coming from node = {:?} with error: {}", hash, number, who, + e, ); e }) diff --git a/client/consensus/manual-seal/src/consensus/babe.rs b/client/consensus/manual-seal/src/consensus/babe.rs index 9c2a1638043a7..dd3f9a253478a 100644 --- a/client/consensus/manual-seal/src/consensus/babe.rs +++ b/client/consensus/manual-seal/src/consensus/babe.rs @@ -118,7 +118,7 @@ where pre_digest.slot(), ) .map_err(|e| format!("failed to fetch epoch_descriptor: {}", e))? - .ok_or_else(|| format!("{:?}", sp_consensus::Error::InvalidAuthoritiesSet))?; + .ok_or_else(|| format!("{}", sp_consensus::Error::InvalidAuthoritiesSet))?; // drop the lock drop(epoch_changes); diff --git a/client/consensus/manual-seal/src/finalize_block.rs b/client/consensus/manual-seal/src/finalize_block.rs index bc242ad6085ee..d134ce7734571 100644 --- a/client/consensus/manual-seal/src/finalize_block.rs +++ b/client/consensus/manual-seal/src/finalize_block.rs @@ -48,7 +48,7 @@ where match finalizer.finalize_block(BlockId::Hash(hash), justification, true) { Err(e) => { - log::warn!("Failed to finalize block {:?}", e); + log::warn!("Failed to finalize block {}", e); rpc::send_result(&mut sender, Err(e.into())) }, Ok(()) => { diff --git a/client/consensus/manual-seal/src/rpc.rs b/client/consensus/manual-seal/src/rpc.rs index 7b4063e9b2b1b..4a8dcbc0cb765 100644 --- a/client/consensus/manual-seal/src/rpc.rs +++ b/client/consensus/manual-seal/src/rpc.rs @@ -155,7 +155,10 @@ pub fn send_result( ) { if let Some(sender) = sender.take() { if let Err(err) = sender.send(result) { - log::warn!("Server is shutting down: {:?}", err) + match err { + Ok(value) => log::warn!("Server is shutting down: {:?}", value), + Err(error) => log::warn!("Server is shutting down with error: {}", error), + } } } else { // instant seal doesn't report errors over rpc, simply log them. diff --git a/client/consensus/manual-seal/src/seal_block.rs b/client/consensus/manual-seal/src/seal_block.rs index 99b003c32f136..202b54fe5d0c5 100644 --- a/client/consensus/manual-seal/src/seal_block.rs +++ b/client/consensus/manual-seal/src/seal_block.rs @@ -114,10 +114,7 @@ pub async fn seal_block( let inherent_data = inherent_data_providers.create_inherent_data()?; - let proposer = env - .init(&parent) - .map_err(|err| Error::StringError(format!("{:?}", err))) - .await?; + let proposer = env.init(&parent).map_err(|err| Error::StringError(err.to_string())).await?; let inherents_len = inherent_data.len(); let digest = if let Some(digest_provider) = digest_provider { @@ -133,7 +130,7 @@ pub async fn seal_block( Duration::from_secs(MAX_PROPOSAL_DURATION), None, ) - .map_err(|err| Error::StringError(format!("{:?}", err))) + .map_err(|err| Error::StringError(err.to_string())) .await?; if proposal.block.extrinsics().len() == inherents_len && !create_empty { diff --git a/client/consensus/pow/src/lib.rs b/client/consensus/pow/src/lib.rs index ef81faff46a93..6d0bc3fc5a192 100644 --- a/client/consensus/pow/src/lib.rs +++ b/client/consensus/pow/src/lib.rs @@ -84,24 +84,24 @@ pub enum Error { FailedPreliminaryVerify, #[error("Rejecting block too far in future")] TooFarInFuture, - #[error("Fetching best header failed using select chain: {0:?}")] + #[error("Fetching best header failed using select chain: {0}")] BestHeaderSelectChain(ConsensusError), - #[error("Fetching best header failed: {0:?}")] + #[error("Fetching best header failed: {0}")] BestHeader(sp_blockchain::Error), #[error("Best header does not exist")] NoBestHeader, - #[error("Block proposing error: {0:?}")] + #[error("Block proposing error: {0}")] BlockProposingError(String), - #[error("Fetch best hash failed via select chain: {0:?}")] + #[error("Fetch best hash failed via select chain: {0}")] BestHashSelectChain(ConsensusError), - #[error("Error with block built on {0:?}: {1:?}")] + #[error("Error with block built on {0:?}: {1}")] BlockBuiltError(B::Hash, ConsensusError), #[error("Creating inherents failed: {0}")] CreateInherents(sp_inherents::Error), #[error("Checking inherents failed: {0}")] CheckInherents(sp_inherents::Error), #[error( - "Checking inherents unknown error for identifier: {:?}", + "Checking inherents unknown error for identifier: {}", String::from_utf8_lossy(.0) )] CheckInherentsUnknownError(sp_inherents::InherentIdentifier), @@ -350,7 +350,7 @@ where .select_chain .best_chain() .await - .map_err(|e| format!("Fetch best chain failed via select chain: {:?}", e))?; + .map_err(|e| format!("Fetch best chain failed via select chain: {}", e))?; let best_hash = best_header.hash(); let parent_hash = *block.header.parent_hash(); @@ -565,7 +565,7 @@ where warn!( target: "pow", "Unable to pull new block for authoring. \ - Select best chain error: {:?}", + Select best chain error: {}", err ); continue @@ -596,7 +596,7 @@ where warn!( target: "pow", "Unable to propose new block for authoring. \ - Fetch difficulty failed: {:?}", + Fetch difficulty failed: {}", err, ); continue @@ -612,7 +612,7 @@ where warn!( target: "pow", "Unable to propose new block for authoring. \ - Creating inherent data providers failed: {:?}", + Creating inherent data providers failed: {}", err, ); continue @@ -625,7 +625,7 @@ where warn!( target: "pow", "Unable to propose new block for authoring. \ - Creating inherent data failed: {:?}", + Creating inherent data failed: {}", e, ); continue @@ -661,7 +661,7 @@ where warn!( target: "pow", "Unable to propose new block for authoring. \ - Creating proposal failed: {:?}", + Creating proposal failed: {}", err, ); continue diff --git a/client/consensus/pow/src/worker.rs b/client/consensus/pow/src/worker.rs index 031cf7f6a2940..42f82fb43ef7b 100644 --- a/client/consensus/pow/src/worker.rs +++ b/client/consensus/pow/src/worker.rs @@ -169,7 +169,7 @@ where Err(err) => { warn!( target: "pow", - "Unable to import mined block: {:?}", + "Unable to import mined block: {}", err, ); return false @@ -238,7 +238,7 @@ where Err(err) => { warn!( target: "pow", - "Unable to import mined block: {:?}", + "Unable to import mined block: {}", err, ); false diff --git a/client/consensus/slots/src/lib.rs b/client/consensus/slots/src/lib.rs index a1b335f7a5940..9fc93788a33e9 100644 --- a/client/consensus/slots/src/lib.rs +++ b/client/consensus/slots/src/lib.rs @@ -214,7 +214,7 @@ pub trait SimpleSlotWorker { Err(err) => { warn!( target: logging_target, - "Unable to fetch epoch data at block {:?}: {:?}", + "Unable to fetch epoch data at block {:?}: {}", slot_info.chain_head.hash(), err, ); @@ -274,10 +274,7 @@ pub trait SimpleSlotWorker { let proposer = match self.proposer(&slot_info.chain_head).await { Ok(p) => p, Err(err) => { - warn!( - target: logging_target, - "Unable to author block in slot {:?}: {:?}", slot, err, - ); + warn!(target: logging_target, "Unable to author block in slot {:?}: {}", slot, err,); telemetry!( telemetry; @@ -303,12 +300,12 @@ pub trait SimpleSlotWorker { proposing_remaining_duration.mul_f32(0.98), None, ) - .map_err(|e| sp_consensus::Error::ClientImport(format!("{:?}", e))); + .map_err(|e| sp_consensus::Error::ClientImport(e.to_string())); let proposal = match futures::future::select(proposing, proposing_remaining).await { Either::Left((Ok(p), _)) => p, Either::Left((Err(err), _)) => { - warn!(target: logging_target, "Proposing failed: {:?}", err); + warn!(target: logging_target, "Proposing failed: {}", err); return None }, @@ -353,7 +350,7 @@ pub trait SimpleSlotWorker { { Ok(bi) => bi, Err(err) => { - warn!(target: logging_target, "Failed to create block import params: {:?}", err); + warn!(target: logging_target, "Failed to create block import params: {}", err); return None }, @@ -388,7 +385,7 @@ pub trait SimpleSlotWorker { Err(err) => { warn!( target: logging_target, - "Error with block built on {:?}: {:?}", parent_hash, err, + "Error with block built on {:?}: {}", parent_hash, err, ); telemetry!( @@ -488,7 +485,7 @@ pub async fn start_slot_worker( let slot_info = match slots.next_slot().await { Ok(r) => r, Err(e) => { - warn!(target: "slots", "Error while polling for next slot: {:?}", e); + warn!(target: "slots", "Error while polling for next slot: {}", e); return }, }; diff --git a/client/consensus/slots/src/slots.rs b/client/consensus/slots/src/slots.rs index 2b792af7da8c4..a7b9f3e3ff611 100644 --- a/client/consensus/slots/src/slots.rs +++ b/client/consensus/slots/src/slots.rs @@ -150,7 +150,7 @@ where Err(e) => { log::warn!( target: "slots", - "Unable to author block in slot. No best block header: {:?}", + "Unable to author block in slot. No best block header: {}", e, ); // Let's try at the next slot.. diff --git a/client/executor/common/src/error.rs b/client/executor/common/src/error.rs index 606f97317b9a8..c5de737376c15 100644 --- a/client/executor/common/src/error.rs +++ b/client/executor/common/src/error.rs @@ -31,9 +31,6 @@ pub enum Error { #[error("Unserializable data encountered")] InvalidData(#[from] sp_serializer::Error), - #[error(transparent)] - Trap(#[from] wasmi::Trap), - #[error(transparent)] Wasmi(#[from] wasmi::Error), @@ -108,6 +105,12 @@ pub enum Error { #[error("Invalid initializer expression provided {0}")] InvalidInitializerExpression(String), + + #[error("Execution aborted due to panic: {0}")] + AbortedDueToPanic(MessageWithBacktrace), + + #[error("Execution aborted due to trap: {0}")] + AbortedDueToTrap(MessageWithBacktrace), } impl wasmi::HostError for Error {} @@ -160,3 +163,38 @@ pub enum WasmError { #[error("{0}")] Other(String), } + +/// An error message with an attached backtrace. +#[derive(Debug)] +pub struct MessageWithBacktrace { + /// The error message. + pub message: String, + + /// The backtrace associated with the error message. + pub backtrace: Option, +} + +impl std::fmt::Display for MessageWithBacktrace { + fn fmt(&self, fmt: &mut std::fmt::Formatter) -> std::fmt::Result { + fmt.write_str(&self.message)?; + if let Some(ref backtrace) = self.backtrace { + fmt.write_str("\nWASM backtrace:\n")?; + backtrace.backtrace_string.fmt(fmt)?; + } + + Ok(()) + } +} + +/// A WASM backtrace. +#[derive(Debug)] +pub struct Backtrace { + /// The string containing the backtrace. + pub backtrace_string: String, +} + +impl std::fmt::Display for Backtrace { + fn fmt(&self, fmt: &mut std::fmt::Formatter) -> std::fmt::Result { + fmt.write_str(&self.backtrace_string) + } +} diff --git a/client/executor/runtime-test/Cargo.toml b/client/executor/runtime-test/Cargo.toml index b6fbe8685e35b..ca1746c842e15 100644 --- a/client/executor/runtime-test/Cargo.toml +++ b/client/executor/runtime-test/Cargo.toml @@ -14,7 +14,7 @@ targets = ["x86_64-unknown-linux-gnu"] [dependencies] sp-core = { version = "5.0.0", default-features = false, path = "../../../primitives/core" } -sp-io = { version = "5.0.0", default-features = false, path = "../../../primitives/io" } +sp-io = { version = "5.0.0", default-features = false, path = "../../../primitives/io", features = ["improved_panic_error_reporting"] } sp-runtime = { version = "5.0.0", default-features = false, path = "../../../primitives/runtime" } sp-sandbox = { version = "0.10.0-dev", default-features = false, path = "../../../primitives/sandbox" } sp-std = { version = "4.0.0", default-features = false, path = "../../../primitives/std" } diff --git a/client/executor/runtime-test/src/lib.rs b/client/executor/runtime-test/src/lib.rs index 0655160c1ab7a..bf9f76edd945e 100644 --- a/client/executor/runtime-test/src/lib.rs +++ b/client/executor/runtime-test/src/lib.rs @@ -341,6 +341,14 @@ sp_core::wasm_export_functions! { fn test_take_i8(value: i8) { assert_eq!(value, -66); } + + fn test_abort_on_panic() { + sp_io::panic_handler::abort_on_panic("test_abort_on_panic called"); + } + + fn test_unreachable_intrinsic() { + core::arch::wasm32::unreachable() + } } #[cfg(not(feature = "std"))] diff --git a/client/executor/src/integration_tests/mod.rs b/client/executor/src/integration_tests/mod.rs index 67e9a96cd6bae..462a8ba1b8766 100644 --- a/client/executor/src/integration_tests/mod.rs +++ b/client/executor/src/integration_tests/mod.rs @@ -22,7 +22,7 @@ mod sandbox; use codec::{Decode, Encode}; use hex_literal::hex; -use sc_executor_common::{runtime_blob::RuntimeBlob, wasm_runtime::WasmModule}; +use sc_executor_common::{error::Error, runtime_blob::RuntimeBlob, wasm_runtime::WasmModule}; use sc_runtime_test::wasm_binary_unwrap; use sp_core::{ blake2_128, blake2_256, ed25519, map, @@ -122,7 +122,7 @@ fn call_in_wasm( call_data: &[u8], execution_method: WasmExecutionMethod, ext: &mut E, -) -> Result, String> { +) -> Result, Error> { let executor = crate::WasmExecutor::::new(execution_method, Some(1024), 8, None, 2); executor.uncached_call( @@ -148,25 +148,16 @@ fn call_not_existing_function(wasm_method: WasmExecutionMethod) { let mut ext = TestExternalities::default(); let mut ext = ext.ext(); - match call_in_wasm( - "test_calling_missing_external", - &[], - wasm_method, - &mut ext, - ) { - Ok(_) => panic!("was expected an `Err`"), - Err(e) => { - match wasm_method { - WasmExecutionMethod::Interpreted => assert_eq!( - &format!("{:?}", e), - "\"Trap: Trap { kind: Host(Other(\\\"Function `missing_external` is only a stub. Calling a stub is not allowed.\\\")) }\"" - ), + match call_in_wasm("test_calling_missing_external", &[], wasm_method, &mut ext).unwrap_err() { + Error::AbortedDueToTrap(error) => { + let expected = match wasm_method { + WasmExecutionMethod::Interpreted => "Trap: Host(Other(\"Function `missing_external` is only a stub. Calling a stub is not allowed.\"))", #[cfg(feature = "wasmtime")] - WasmExecutionMethod::Compiled => assert!( - format!("{:?}", e).contains("Wasm execution trapped: call to a missing function env:missing_external") - ), - } - } + WasmExecutionMethod::Compiled => "call to a missing function env:missing_external" + }; + assert_eq!(error.message, expected); + }, + error => panic!("unexpected error: {:?}", error), } } @@ -175,25 +166,18 @@ fn call_yet_another_not_existing_function(wasm_method: WasmExecutionMethod) { let mut ext = TestExternalities::default(); let mut ext = ext.ext(); - match call_in_wasm( - "test_calling_yet_another_missing_external", - &[], - wasm_method, - &mut ext, - ) { - Ok(_) => panic!("was expected an `Err`"), - Err(e) => { - match wasm_method { - WasmExecutionMethod::Interpreted => assert_eq!( - &format!("{:?}", e), - "\"Trap: Trap { kind: Host(Other(\\\"Function `yet_another_missing_external` is only a stub. Calling a stub is not allowed.\\\")) }\"" - ), + match call_in_wasm("test_calling_yet_another_missing_external", &[], wasm_method, &mut ext) + .unwrap_err() + { + Error::AbortedDueToTrap(error) => { + let expected = match wasm_method { + WasmExecutionMethod::Interpreted => "Trap: Host(Other(\"Function `yet_another_missing_external` is only a stub. Calling a stub is not allowed.\"))", #[cfg(feature = "wasmtime")] - WasmExecutionMethod::Compiled => assert!( - format!("{:?}", e).contains("Wasm execution trapped: call to a missing function env:yet_another_missing_external") - ), - } - } + WasmExecutionMethod::Compiled => "call to a missing function env:yet_another_missing_external" + }; + assert_eq!(error.message, expected); + }, + error => panic!("unexpected error: {:?}", error), } } @@ -485,6 +469,7 @@ fn should_trap_when_heap_exhausted(wasm_method: WasmExecutionMethod) { "test_exhaust_heap", &[0], ) + .map_err(|e| e.to_string()) .unwrap_err(); assert!(err.contains("Allocator ran out of space")); @@ -691,7 +676,7 @@ fn panic_in_spawned_instance_panics_on_joining_its_result(wasm_method: WasmExecu let error_result = call_in_wasm("test_panic_in_spawned", &[], wasm_method, &mut ext).unwrap_err(); - assert!(error_result.contains("Spawned task")); + assert!(error_result.to_string().contains("Spawned task")); } test_wasm_execution!(memory_is_cleared_between_invocations); @@ -789,3 +774,32 @@ fn take_i8(wasm_method: WasmExecutionMethod) { call_in_wasm("test_take_i8", &(-66_i8).encode(), wasm_method, &mut ext).unwrap(); } + +test_wasm_execution!(abort_on_panic); +fn abort_on_panic(wasm_method: WasmExecutionMethod) { + let mut ext = TestExternalities::default(); + let mut ext = ext.ext(); + + match call_in_wasm("test_abort_on_panic", &[], wasm_method, &mut ext).unwrap_err() { + Error::AbortedDueToPanic(error) => assert_eq!(error.message, "test_abort_on_panic called"), + error => panic!("unexpected error: {:?}", error), + } +} + +test_wasm_execution!(unreachable_intrinsic); +fn unreachable_intrinsic(wasm_method: WasmExecutionMethod) { + let mut ext = TestExternalities::default(); + let mut ext = ext.ext(); + + match call_in_wasm("test_unreachable_intrinsic", &[], wasm_method, &mut ext).unwrap_err() { + Error::AbortedDueToTrap(error) => { + let expected = match wasm_method { + WasmExecutionMethod::Interpreted => "Trap: Unreachable", + #[cfg(feature = "wasmtime")] + WasmExecutionMethod::Compiled => "wasm trap: wasm `unreachable` instruction executed", + }; + assert_eq!(error.message, expected); + }, + error => panic!("unexpected error: {:?}", error), + } +} diff --git a/client/executor/src/native_executor.rs b/client/executor/src/native_executor.rs index b36d6f1297e91..1bc87840ba353 100644 --- a/client/executor/src/native_executor.rs +++ b/client/executor/src/native_executor.rs @@ -220,7 +220,7 @@ where allow_missing_host_functions: bool, export_name: &str, call_data: &[u8], - ) -> std::result::Result, String> { + ) -> std::result::Result, Error> { let module = crate::wasm_runtime::create_wasm_runtime_with_code::( self.method, self.default_heap_pages, @@ -228,11 +228,10 @@ where allow_missing_host_functions, self.cache_path.as_deref(), ) - .map_err(|e| format!("Failed to create module: {:?}", e))?; + .map_err(|e| format!("Failed to create module: {}", e))?; - let instance = module - .new_instance() - .map_err(|e| format!("Failed to create instance: {:?}", e))?; + let instance = + module.new_instance().map_err(|e| format!("Failed to create instance: {}", e))?; let mut instance = AssertUnwindSafe(instance); let mut ext = AssertUnwindSafe(ext); @@ -243,7 +242,6 @@ where instance.call_export(export_name, call_data) }) .and_then(|r| r) - .map_err(|e| e.to_string()) } } @@ -281,6 +279,7 @@ where "Core_version", &[], ) + .map_err(|e| e.to_string()) } } @@ -456,12 +455,18 @@ impl RuntimeSpawn for RuntimeInstanceSpawn { // pool of instances should be used. // // https://github.com/paritytech/substrate/issues/7354 - let mut instance = - module.new_instance().expect("Failed to create new instance from module"); + let mut instance = match module.new_instance() { + Ok(instance) => instance, + Err(error) => + panic!("failed to create new instance from module: {}", error), + }; - instance + match instance .call(InvokeMethod::TableWithWrapper { dispatcher_ref, func }, &data[..]) - .expect("Failed to invoke instance.") + { + Ok(result) => result, + Err(error) => panic!("failed to invoke instance: {}", error), + } }); match result { @@ -471,7 +476,7 @@ impl RuntimeSpawn for RuntimeInstanceSpawn { Err(error) => { // If execution is panicked, the `join` in the original runtime code will // panic as well, since the sender is dropped without sending anything. - log::error!("Call error in spawned task: {:?}", error); + log::error!("Call error in spawned task: {}", error); }, } }), diff --git a/client/executor/src/wasm_runtime.rs b/client/executor/src/wasm_runtime.rs index 0775755aff7cf..2cccb6f9c38b0 100644 --- a/client/executor/src/wasm_runtime.rs +++ b/client/executor/src/wasm_runtime.rs @@ -105,13 +105,13 @@ impl VersionedRuntime { if new_inst { log::warn!( target: "wasm-runtime", - "Fresh runtime instance failed with {:?}", + "Fresh runtime instance failed with {}", e, ) } else { log::warn!( target: "wasm-runtime", - "Evicting failed runtime instance: {:?}", + "Evicting failed runtime instance: {}", e, ); } diff --git a/client/executor/wasmi/src/lib.rs b/client/executor/wasmi/src/lib.rs index 78fd300e05e3d..f0488972daca9 100644 --- a/client/executor/wasmi/src/lib.rs +++ b/client/executor/wasmi/src/lib.rs @@ -21,7 +21,7 @@ use codec::{Decode, Encode}; use log::{debug, error, trace}; use sc_executor_common::{ - error::{Error, WasmError}, + error::{Error, MessageWithBacktrace, WasmError}, runtime_blob::{DataSegmentsSnapshot, RuntimeBlob}, sandbox, util::MemoryTransfer, @@ -48,6 +48,7 @@ struct FunctionExecutor { host_functions: Arc>, allow_missing_func_imports: bool, missing_functions: Arc>, + panic_message: Option, } impl FunctionExecutor { @@ -69,6 +70,7 @@ impl FunctionExecutor { host_functions, allow_missing_func_imports, missing_functions, + panic_message: None, }) } } @@ -100,7 +102,10 @@ impl<'a> sandbox::SandboxContext for SandboxContext<'a> { match result { Ok(Some(RuntimeValue::I64(val))) => Ok(val), Ok(_) => return Err("Supervisor function returned unexpected result!".into()), - Err(err) => Err(Error::Trap(err)), + Err(err) => Err(Error::AbortedDueToTrap(MessageWithBacktrace { + message: err.to_string(), + backtrace: None, + })), } } @@ -133,6 +138,10 @@ impl FunctionContext for FunctionExecutor { fn sandbox(&mut self) -> &mut dyn Sandbox { self } + + fn register_panic_error_message(&mut self, message: &str) { + self.panic_message = Some(message.to_owned()); + } } impl Sandbox for FunctionExecutor { @@ -502,12 +511,31 @@ fn call_in_wasm_module( let offset = function_executor.allocate_memory(data.len() as u32)?; function_executor.write_memory(offset, data)?; + fn convert_trap(executor: &mut FunctionExecutor, trap: wasmi::Trap) -> Error { + if let Some(message) = executor.panic_message.take() { + Error::AbortedDueToPanic(MessageWithBacktrace { message, backtrace: None }) + } else { + Error::AbortedDueToTrap(MessageWithBacktrace { + message: trap.to_string(), + backtrace: None, + }) + } + } + let result = match method { - InvokeMethod::Export(method) => module_instance.invoke_export( - method, - &[I32(u32::from(offset) as i32), I32(data.len() as i32)], - &mut function_executor, - ), + InvokeMethod::Export(method) => module_instance + .invoke_export( + method, + &[I32(u32::from(offset) as i32), I32(data.len() as i32)], + &mut function_executor, + ) + .map_err(|error| { + if let wasmi::Error::Trap(trap) = error { + convert_trap(&mut function_executor, trap) + } else { + error.into() + } + }), InvokeMethod::Table(func_ref) => { let func = table .ok_or(Error::NoTable)? @@ -518,7 +546,7 @@ fn call_in_wasm_module( &[I32(u32::from(offset) as i32), I32(data.len() as i32)], &mut function_executor, ) - .map_err(Into::into) + .map_err(|trap| convert_trap(&mut function_executor, trap)) }, InvokeMethod::TableWithWrapper { dispatcher_ref, func } => { let dispatcher = table @@ -531,7 +559,7 @@ fn call_in_wasm_module( &[I32(func as _), I32(u32::from(offset) as i32), I32(data.len() as i32)], &mut function_executor, ) - .map_err(Into::into) + .map_err(|trap| convert_trap(&mut function_executor, trap)) }, }; diff --git a/client/executor/wasmtime/src/host.rs b/client/executor/wasmtime/src/host.rs index 5da8ff3259031..b310ada24b629 100644 --- a/client/executor/wasmtime/src/host.rs +++ b/client/executor/wasmtime/src/host.rs @@ -45,6 +45,7 @@ unsafe impl Send for SandboxStore {} pub struct HostState { sandbox_store: SandboxStore, allocator: FreeingBumpHeapAllocator, + panic_message: Option, } impl HostState { @@ -55,8 +56,14 @@ impl HostState { sandbox::SandboxBackend::TryWasmer, )))), allocator, + panic_message: None, } } + + /// Takes the error message out of the host state, leaving a `None` in its place. + pub fn take_panic_message(&mut self) -> Option { + self.panic_message.take() + } } /// A `HostContext` implements `FunctionContext` for making host calls from a Wasmtime @@ -134,6 +141,14 @@ impl<'a> sp_wasm_interface::FunctionContext for HostContext<'a> { fn sandbox(&mut self) -> &mut dyn Sandbox { self } + + fn register_panic_error_message(&mut self, message: &str) { + self.caller + .data_mut() + .host_state_mut() + .expect("host state is not empty when calling a function in wasm; qed") + .panic_message = Some(message.to_owned()); + } } impl<'a> Sandbox for HostContext<'a> { diff --git a/client/executor/wasmtime/src/instance_wrapper.rs b/client/executor/wasmtime/src/instance_wrapper.rs index e27de7817b2bd..896b71cea21dd 100644 --- a/client/executor/wasmtime/src/instance_wrapper.rs +++ b/client/executor/wasmtime/src/instance_wrapper.rs @@ -21,7 +21,7 @@ use crate::runtime::{Store, StoreData}; use sc_executor_common::{ - error::{Error, Result}, + error::{Backtrace, Error, MessageWithBacktrace, Result}, wasm_runtime::InvokeMethod, }; use sp_wasm_interface::{HostFunctions, Pointer, Value, WordSize}; @@ -53,25 +53,51 @@ pub struct EntryPoint { impl EntryPoint { /// Call this entry point. - pub fn call( + pub(crate) fn call( &self, - ctx: impl AsContextMut, + store: &mut Store, data_ptr: Pointer, data_len: WordSize, ) -> Result { let data_ptr = u32::from(data_ptr); let data_len = u32::from(data_len); - fn handle_trap(err: wasmtime::Trap) -> Error { - Error::from(format!("Wasm execution trapped: {}", err)) - } - match self.call_type { EntryPointType::Direct { ref entrypoint } => - entrypoint.call(ctx, (data_ptr, data_len)).map_err(handle_trap), + entrypoint.call(&mut *store, (data_ptr, data_len)), EntryPointType::Wrapped { func, ref dispatcher } => - dispatcher.call(ctx, (func, data_ptr, data_len)).map_err(handle_trap), + dispatcher.call(&mut *store, (func, data_ptr, data_len)), } + .map_err(|trap| { + let host_state = store + .data_mut() + .host_state + .as_mut() + .expect("host state cannot be empty while a function is being called; qed"); + + // The logic to print out a backtrace is somewhat complicated, + // so let's get wasmtime to print it out for us. + let mut backtrace_string = trap.to_string(); + let suffix = "\nwasm backtrace:"; + if let Some(index) = backtrace_string.find(suffix) { + // Get rid of the error message and just grab the backtrace, + // since we're storing the error message ourselves separately. + backtrace_string.replace_range(0..index + suffix.len(), ""); + } + + let backtrace = Backtrace { backtrace_string }; + if let Some(error) = host_state.take_panic_message() { + Error::AbortedDueToPanic(MessageWithBacktrace { + message: error, + backtrace: Some(backtrace), + }) + } else { + Error::AbortedDueToTrap(MessageWithBacktrace { + message: trap.display_reason().to_string(), + backtrace: Some(backtrace), + }) + } + }) } pub fn direct( diff --git a/client/executor/wasmtime/src/tests.rs b/client/executor/wasmtime/src/tests.rs index 773e1d707a354..664d05f5387fc 100644 --- a/client/executor/wasmtime/src/tests.rs +++ b/client/executor/wasmtime/src/tests.rs @@ -17,7 +17,7 @@ // along with this program. If not, see . use codec::{Decode as _, Encode as _}; -use sc_executor_common::{runtime_blob::RuntimeBlob, wasm_runtime::WasmModule}; +use sc_executor_common::{error::Error, runtime_blob::RuntimeBlob, wasm_runtime::WasmModule}; use sc_runtime_test::wasm_binary_unwrap; use std::sync::Arc; @@ -158,11 +158,13 @@ fn test_stack_depth_reaching() { }; let mut instance = runtime.new_instance().expect("failed to instantiate a runtime"); - let err = instance.call_export("test-many-locals", &[]).unwrap_err(); - - assert!(format!("{:?}", err).starts_with( - "Other(\"Wasm execution trapped: wasm trap: wasm `unreachable` instruction executed" - )); + match instance.call_export("test-many-locals", &[]).unwrap_err() { + Error::AbortedDueToTrap(error) => { + let expected = "wasm trap: wasm `unreachable` instruction executed"; + assert_eq!(error.message, expected); + }, + error => panic!("unexpected error: {:?}", error), + } } #[test] diff --git a/client/finality-grandpa/src/aux_schema.rs b/client/finality-grandpa/src/aux_schema.rs index 2ec48a804c936..0ac9ba9e64bd2 100644 --- a/client/finality-grandpa/src/aux_schema.rs +++ b/client/finality-grandpa/src/aux_schema.rs @@ -100,7 +100,7 @@ where // previously we only supported at most one pending change per fork &|_, _| Ok(false), ) { - warn!(target: "afg", "Error migrating pending authority set change: {:?}.", err); + warn!(target: "afg", "Error migrating pending authority set change: {}", err); warn!(target: "afg", "Node is in a potentially inconsistent state."); } } diff --git a/client/finality-grandpa/src/environment.rs b/client/finality-grandpa/src/environment.rs index 81bb24f1a5eed..6ffcdc719a166 100644 --- a/client/finality-grandpa/src/environment.rs +++ b/client/finality-grandpa/src/environment.rs @@ -609,7 +609,7 @@ where let tree_route = match tree_route_res { Ok(tree_route) => tree_route, Err(e) => { - debug!(target: "afg", "Encountered error computing ancestry between block {:?} and base {:?}: {:?}", + debug!(target: "afg", "Encountered error computing ancestry between block {:?} and base {:?}: {}", block, base, e); return Err(GrandpaError::NotDescendent) @@ -1098,7 +1098,7 @@ where ) { warn!(target: "afg", "Detected prevote equivocation in the finality worker: {:?}", equivocation); if let Err(err) = self.report_equivocation(equivocation.into()) { - warn!(target: "afg", "Error reporting prevote equivocation: {:?}", err); + warn!(target: "afg", "Error reporting prevote equivocation: {}", err); } } @@ -1109,7 +1109,7 @@ where ) { warn!(target: "afg", "Detected precommit equivocation in the finality worker: {:?}", equivocation); if let Err(err) = self.report_equivocation(equivocation.into()) { - warn!(target: "afg", "Error reporting precommit equivocation: {:?}", err); + warn!(target: "afg", "Error reporting precommit equivocation: {}", err); } } } @@ -1224,7 +1224,7 @@ where .or_else(|| Some((target_header.hash(), *target_header.number()))) }, Err(e) => { - warn!(target: "afg", "Encountered error finding best chain containing {:?}: {:?}", block, e); + warn!(target: "afg", "Encountered error finding best chain containing {:?}: {}", block, e); None }, }; @@ -1293,7 +1293,7 @@ where ) { if let Some(sender) = justification_sender { if let Err(err) = sender.notify(justification) { - warn!(target: "afg", "Error creating justification for subscriber: {:?}", err); + warn!(target: "afg", "Error creating justification for subscriber: {}", err); } } } @@ -1344,7 +1344,7 @@ where client .apply_finality(import_op, BlockId::Hash(hash), persisted_justification, true) .map_err(|e| { - warn!(target: "afg", "Error applying finality to block {:?}: {:?}", (hash, number), e); + warn!(target: "afg", "Error applying finality to block {:?}: {}", (hash, number), e); e })?; diff --git a/client/finality-grandpa/src/import.rs b/client/finality-grandpa/src/import.rs index 71d74045f4760..ae5839d0c24e6 100644 --- a/client/finality-grandpa/src/import.rs +++ b/client/finality-grandpa/src/import.rs @@ -598,7 +598,7 @@ where Err(e) => { debug!( target: "afg", - "Restoring old authority set after block import error: {:?}", + "Restoring old authority set after block import error: {}", e, ); pending_changes.revert(); @@ -663,8 +663,12 @@ where import_res.unwrap_or_else(|err| { if needs_justification { - debug!(target: "afg", "Imported block #{} that enacts authority set change with \ - invalid justification: {:?}, requesting justification from peers.", number, err); + debug!( + target: "afg", + "Requesting justification from peers due to imported block #{} that enacts authority set change with invalid justification: {}", + number, + err + ); imported_aux.bad_justification = true; imported_aux.needs_justification = true; } diff --git a/client/finality-grandpa/src/lib.rs b/client/finality-grandpa/src/lib.rs index b99f6c0544197..8316e56b5b5e5 100644 --- a/client/finality-grandpa/src/lib.rs +++ b/client/finality-grandpa/src/lib.rs @@ -275,23 +275,38 @@ impl Config { } /// Errors that can occur while voting in GRANDPA. -#[derive(Debug)] +#[derive(Debug, thiserror::Error)] pub enum Error { /// An error within grandpa. + #[error("grandpa error: {0}")] Grandpa(GrandpaError), + /// A network error. + #[error("network error: {0}")] Network(String), + /// A blockchain error. + #[error("blockchain error: {0}")] Blockchain(String), + /// Could not complete a round on disk. + #[error("could not complete a round on disk: {0}")] Client(ClientError), + /// Could not sign outgoing message + #[error("could not sign outgoing message: {0}")] Signing(String), + /// An invariant has been violated (e.g. not finalizing pending change blocks in-order) + #[error("safety invariant has been violated: {0}")] Safety(String), + /// A timer failed to fire. + #[error("a timer failed to fire: {0}")] Timer(io::Error), + /// A runtime api request failed. + #[error("runtime API request failed: {0}")] RuntimeApi(sp_api::ApiError), } @@ -322,7 +337,7 @@ where { fn block_number(&self, hash: Block::Hash) -> Result>, Error> { self.block_number_from_id(&BlockId::Hash(hash)) - .map_err(|e| Error::Blockchain(format!("{:?}", e))) + .map_err(|e| Error::Blockchain(e.to_string())) } } @@ -459,7 +474,7 @@ impl ::std::error::Error for CommandOrError impl fmt::Display for CommandOrError { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match *self { - CommandOrError::Error(ref e) => write!(f, "{:?}", e), + CommandOrError::Error(ref e) => write!(f, "{}", e), CommandOrError::VoterCommand(ref cmd) => write!(f, "{}", cmd), } } @@ -838,7 +853,7 @@ where Ok(()) => error!(target: "afg", "GRANDPA voter future has concluded naturally, this should be unreachable." ), - Err(e) => error!(target: "afg", "GRANDPA voter error: {:?}", e), + Err(e) => error!(target: "afg", "GRANDPA voter error: {}", e), }); // Make sure that `telemetry_task` doesn't accidentally finish and kill grandpa. diff --git a/client/finality-grandpa/src/observer.rs b/client/finality-grandpa/src/observer.rs index ab0c69ef7fc26..a7c951cc33db9 100644 --- a/client/finality-grandpa/src/observer.rs +++ b/client/finality-grandpa/src/observer.rs @@ -203,7 +203,7 @@ where ); let observer_work = observer_work.map_ok(|_| ()).map_err(|e| { - warn!("GRANDPA Observer failed: {:?}", e); + warn!("GRANDPA Observer failed: {}", e); }); Ok(observer_work.map(drop)) diff --git a/client/network/src/protocol.rs b/client/network/src/protocol.rs index 5541a0145366f..b39d0d1b8428b 100644 --- a/client/network/src/protocol.rs +++ b/client/network/src/protocol.rs @@ -886,7 +886,7 @@ impl Protocol { return }, Err(e) => { - warn!("Error reading block header {}: {:?}", hash, e); + warn!("Error reading block header {}: {}", hash, e); return }, }; diff --git a/client/network/src/protocol/sync.rs b/client/network/src/protocol/sync.rs index fbb4e376b1b4f..d98c0d2c04abe 100644 --- a/client/network/src/protocol/sync.rs +++ b/client/network/src/protocol/sync.rs @@ -665,7 +665,7 @@ impl ChainSync { // There is nothing sync can get from the node that has no blockchain data. match self.block_status(&best_hash) { Err(e) => { - debug!(target:"sync", "Error reading blockchain: {:?}", e); + debug!(target:"sync", "Error reading blockchain: {}", e); Err(BadPeer(who, rep::BLOCKCHAIN_READ_ERROR)) }, Ok(BlockStatus::KnownBad) => { @@ -1192,7 +1192,7 @@ impl ChainSync { (_, Err(e)) => { info!( target: "sync", - "❌ Error answering legitimate blockchain query: {:?}", + "❌ Error answering legitimate blockchain query: {}", e, ); return Err(BadPeer(*who, rep::BLOCKCHAIN_READ_ERROR)) @@ -1629,7 +1629,7 @@ impl ChainSync { trace!(target: "sync", "Obsolete block {:?}", hash); }, e @ Err(BlockImportError::UnknownParent) | e @ Err(BlockImportError::Other(_)) => { - warn!(target: "sync", "💔 Error importing block {:?}: {:?}", hash, e); + warn!(target: "sync", "💔 Error importing block {:?}: {}", hash, e.unwrap_err()); self.state_sync = None; self.warp_sync = None; output.extend(self.restart()); @@ -1683,7 +1683,7 @@ impl ChainSync { if let Err(err) = r { warn!( target: "sync", - "💔 Error cleaning up pending extra justification data requests: {:?}", + "💔 Error cleaning up pending extra justification data requests: {}", err, ); } @@ -2081,7 +2081,7 @@ impl ChainSync { ) -> impl Iterator), BadPeer>> + 'a { self.blocks.clear(); if let Err(e) = self.reset_sync_start_point() { - warn!(target: "sync", "💔 Unable to restart sync. :{:?}", e); + warn!(target: "sync", "💔 Unable to restart sync: {}", e); } self.pending_requests.set_all(); debug!(target:"sync", "Restarted with {} ({})", self.best_queued_number, self.best_queued_hash); diff --git a/client/network/src/protocol/sync/extra_requests.rs b/client/network/src/protocol/sync/extra_requests.rs index 224fbd1a1e01a..d0bfebab66010 100644 --- a/client/network/src/protocol/sync/extra_requests.rs +++ b/client/network/src/protocol/sync/extra_requests.rs @@ -108,7 +108,7 @@ impl ExtraRequests { // ignore the `Revert` error. }, Err(err) => { - debug!(target: "sync", "Failed to insert request {:?} into tree: {:?}", request, err); + debug!(target: "sync", "Failed to insert request {:?} into tree: {}", request, err); }, _ => (), } diff --git a/client/network/src/protocol/sync/state.rs b/client/network/src/protocol/sync/state.rs index 3de165b83d9e0..0df862a48333f 100644 --- a/client/network/src/protocol/sync/state.rs +++ b/client/network/src/protocol/sync/state.rs @@ -99,7 +99,7 @@ impl StateSync { Err(e) => { debug!( target: "sync", - "StateResponse failed proof verification: {:?}", + "StateResponse failed proof verification: {}", e, ); return ImportResult::BadResponse diff --git a/client/network/src/protocol/sync/warp.rs b/client/network/src/protocol/sync/warp.rs index 6c51d4b3495f0..f12deb2dbb432 100644 --- a/client/network/src/protocol/sync/warp.rs +++ b/client/network/src/protocol/sync/warp.rs @@ -88,7 +88,7 @@ impl WarpSync { Phase::WarpProof { set_id, authorities, last_hash } => { match self.warp_sync_provider.verify(&response, *set_id, authorities.clone()) { Err(e) => { - log::debug!(target: "sync", "Bad warp proof response: {:?}", e); + log::debug!(target: "sync", "Bad warp proof response: {}", e); return WarpProofImportResult::BadResponse }, Ok(VerificationResult::Partial(new_set_id, new_authorities, new_last_hash)) => { diff --git a/client/offchain/src/lib.rs b/client/offchain/src/lib.rs index cc49c07bffcf4..8d016e945453b 100644 --- a/client/offchain/src/lib.rs +++ b/client/offchain/src/lib.rs @@ -194,7 +194,7 @@ where if let Err(e) = run { tracing::error!( target: LOG_TARGET, - "Error running offchain workers at {:?}: {:?}", + "Error running offchain workers at {:?}: {}", at, e ); diff --git a/client/rpc-api/src/author/error.rs b/client/rpc-api/src/author/error.rs index 5b6bec7ed4bdc..eee77edd5e208 100644 --- a/client/rpc-api/src/author/error.rs +++ b/client/rpc-api/src/author/error.rs @@ -103,7 +103,7 @@ impl From for rpc::Error { Error::Verification(e) => rpc::Error { code: rpc::ErrorCode::ServerError(VERIFICATION_ERROR), message: format!("Verification Error: {}", e).into(), - data: Some(format!("{:?}", e).into()), + data: Some(e.to_string().into()), }, Error::Pool(PoolError::InvalidTransaction(InvalidTransaction::Custom(e))) => rpc::Error { code: rpc::ErrorCode::ServerError(POOL_INVALID_TX), diff --git a/client/rpc-api/src/errors.rs b/client/rpc-api/src/errors.rs index 42e563342fa4d..e59b1b0eda5ce 100644 --- a/client/rpc-api/src/errors.rs +++ b/client/rpc-api/src/errors.rs @@ -18,11 +18,11 @@ use log::warn; -pub fn internal(e: E) -> jsonrpc_core::Error { - warn!("Unknown error: {:?}", e); +pub fn internal(e: E) -> jsonrpc_core::Error { + warn!("Unknown error: {}", e); jsonrpc_core::Error { code: jsonrpc_core::ErrorCode::InternalError, message: "Unknown error occurred".into(), - data: Some(format!("{:?}", e).into()), + data: Some(e.to_string().into()), } } diff --git a/client/service/src/chain_ops/check_block.rs b/client/service/src/chain_ops/check_block.rs index e12766659d9a9..41a6c73c5f473 100644 --- a/client/service/src/chain_ops/check_block.rs +++ b/client/service/src/chain_ops/check_block.rs @@ -46,6 +46,6 @@ where import_blocks(client, import_queue, reader, true, true) }, Ok(None) => Box::pin(future::err("Unknown block".into())), - Err(e) => Box::pin(future::err(format!("Error reading block: {:?}", e).into())), + Err(e) => Box::pin(future::err(format!("Error reading block: {}", e).into())), } } diff --git a/client/service/src/chain_ops/import_blocks.rs b/client/service/src/chain_ops/import_blocks.rs index aa72b745c7cac..9d74fa1c276fa 100644 --- a/client/service/src/chain_ops/import_blocks.rs +++ b/client/service/src/chain_ops/import_blocks.rs @@ -322,7 +322,7 @@ where for result in results { if let (Err(err), hash) = result { - warn!("There was an error importing block with hash {:?}: {:?}", hash, err); + warn!("There was an error importing block with hash {:?}: {}", hash, err); self.has_error = true; break } diff --git a/client/service/src/client/call_executor.rs b/client/service/src/client/call_executor.rs index a806b2dbb6d49..f271b35a69ced 100644 --- a/client/service/src/client/call_executor.rs +++ b/client/service/src/client/call_executor.rs @@ -285,7 +285,7 @@ where state_runtime_code.runtime_code().map_err(sp_blockchain::Error::RuntimeCode)?; self.executor .runtime_version(&mut ext, &runtime_code) - .map_err(|e| sp_blockchain::Error::VersionInvalid(format!("{:?}", e)).into()) + .map_err(|e| sp_blockchain::Error::VersionInvalid(e.to_string()).into()) } fn prove_execution( @@ -340,7 +340,7 @@ where Block: BlockT, { fn runtime_version(&self, at: &BlockId) -> Result { - CallExecutor::runtime_version(self, at).map_err(|e| format!("{:?}", e)) + CallExecutor::runtime_version(self, at).map_err(|e| e.to_string()) } } diff --git a/client/service/src/client/client.rs b/client/service/src/client/client.rs index e8ca5343aa0d2..071af36a23f96 100644 --- a/client/service/src/client/client.rs +++ b/client/service/src/client/client.rs @@ -1116,7 +1116,7 @@ where }; let runtime_version = RuntimeVersionOf::runtime_version(executor, &mut ext, &runtime_code) - .map_err(|e| sp_blockchain::Error::VersionInvalid(format!("{:?}", e)))?; + .map_err(|e| sp_blockchain::Error::VersionInvalid(e.to_string()))?; Ok(runtime_version.state_version()) } else { Err(sp_blockchain::Error::VersionInvalid( @@ -1719,7 +1719,7 @@ where let storage_changes = match self.prepare_block_storage_changes(&mut import_block).map_err(|e| { - warn!("Block prepare storage changes error:\n{:?}", e); + warn!("Block prepare storage changes error: {}", e); ConsensusError::ClientImport(e.to_string()) })? { PrepareStorageChangesResult::Discard(res) => return Ok(res), @@ -1730,7 +1730,7 @@ where self.apply_block(operation, import_block, new_cache, storage_changes) }) .map_err(|e| { - warn!("Block import error:\n{:?}", e); + warn!("Block import error: {}", e); ConsensusError::ClientImport(e.to_string()).into() }) } diff --git a/client/service/src/client/wasm_override.rs b/client/service/src/client/wasm_override.rs index 86365f2d0cab4..267aea0709871 100644 --- a/client/service/src/client/wasm_override.rs +++ b/client/service/src/client/wasm_override.rs @@ -243,7 +243,7 @@ impl WasmOverride { hash: code_hash.into(), }, ) - .map_err(|e| WasmOverrideError::VersionInvalid(format!("{:?}", e)).into()) + .map_err(|e| WasmOverrideError::VersionInvalid(e.to_string()).into()) } } diff --git a/client/service/src/client/wasm_substitutes.rs b/client/service/src/client/wasm_substitutes.rs index a45eefb7b603c..3690672512675 100644 --- a/client/service/src/client/wasm_substitutes.rs +++ b/client/service/src/client/wasm_substitutes.rs @@ -151,6 +151,6 @@ where let mut ext = BasicExternalities::default(); executor .runtime_version(&mut ext, &code.runtime_code(None)) - .map_err(|e| WasmSubstituteError::VersionInvalid(format!("{:?}", e)).into()) + .map_err(|e| WasmSubstituteError::VersionInvalid(e.to_string()).into()) } } diff --git a/client/service/src/lib.rs b/client/service/src/lib.rs index 430a818c0f47c..d158bbc42e947 100644 --- a/client/service/src/lib.rs +++ b/client/service/src/lib.rs @@ -509,7 +509,7 @@ where TransactionImport::Bad }, Err(e) => { - debug!("Error converting pool error: {:?}", e); + debug!("Error converting pool error: {}", e); // it is not bad at least, just some internal node logic error, so peer is // innocent. TransactionImport::KnownGood diff --git a/client/tracing/src/block/mod.rs b/client/tracing/src/block/mod.rs index 067cdafa0ae30..259827e4b47d9 100644 --- a/client/tracing/src/block/mod.rs +++ b/client/tracing/src/block/mod.rs @@ -253,7 +253,7 @@ where self.client.runtime_api().execute_block(&parent_id, block) }) { return Err(Error::Dispatch( - format!("Failed to collect traces and execute block: {:?}", e).to_string(), + format!("Failed to collect traces and execute block: {}", e).to_string(), )) } } diff --git a/client/transaction-pool/api/src/lib.rs b/client/transaction-pool/api/src/lib.rs index 757674a03e850..7c90cd79ccaed 100644 --- a/client/transaction-pool/api/src/lib.rs +++ b/client/transaction-pool/api/src/lib.rs @@ -355,7 +355,7 @@ impl OffchainSubmitTransaction for TP result.map(|_| ()).map_err(|e| { log::warn!( target: "txpool", - "(offchain call) Error submitting a transaction to the pool: {:?}", + "(offchain call) Error submitting a transaction to the pool: {}", e ) }) diff --git a/client/transaction-pool/src/api.rs b/client/transaction-pool/src/api.rs index de1c79534b386..12909f313d100 100644 --- a/client/transaction-pool/src/api.rs +++ b/client/transaction-pool/src/api.rs @@ -167,18 +167,14 @@ where &self, at: &BlockId, ) -> error::Result>> { - self.client - .to_number(at) - .map_err(|e| Error::BlockIdConversion(format!("{:?}", e))) + self.client.to_number(at).map_err(|e| Error::BlockIdConversion(e.to_string())) } fn block_id_to_hash( &self, at: &BlockId, ) -> error::Result>> { - self.client - .to_hash(at) - .map_err(|e| Error::BlockIdConversion(format!("{:?}", e))) + self.client.to_hash(at).map_err(|e| Error::BlockIdConversion(e.to_string())) } fn hash_and_length( @@ -224,7 +220,7 @@ where }?; let block_hash = client.to_hash(at) - .map_err(|e| Error::RuntimeApi(format!("{:?}", e)))? + .map_err(|e| Error::RuntimeApi(e.to_string()))? .ok_or_else(|| Error::RuntimeApi(format!("Could not get hash for block `{:?}`.", at)))?; use sp_api::Core; @@ -237,7 +233,7 @@ where .map_err(|e| Error::RuntimeApi(e.to_string())) } else { let block_number = client.to_number(at) - .map_err(|e| Error::RuntimeApi(format!("{:?}", e)))? + .map_err(|e| Error::RuntimeApi(e.to_string()))? .ok_or_else(|| Error::RuntimeApi(format!("Could not get number for block `{:?}`.", at)) )?; diff --git a/client/transaction-pool/src/lib.rs b/client/transaction-pool/src/lib.rs index 260d938217ad4..ec93d1f7c51fe 100644 --- a/client/transaction-pool/src/lib.rs +++ b/client/transaction-pool/src/lib.rs @@ -534,7 +534,7 @@ async fn prune_known_txs_for_block { - log::debug!(target: "txpool", "Error retrieving header for {:?}: {:?}", block_id, e); + log::debug!(target: "txpool", "Error retrieving header for {:?}: {}", block_id, e); return hashes }, }; if let Err(e) = pool.prune(&block_id, &BlockId::hash(*header.parent_hash()), &extrinsics).await { - log::error!("Cannot prune known in the pool {:?}!", e); + log::error!("Cannot prune known in the pool: {}", e); } hashes @@ -639,7 +639,7 @@ where .block_body(&BlockId::hash(hash)) .await .unwrap_or_else(|e| { - log::warn!("Failed to fetch block body {:?}!", e); + log::warn!("Failed to fetch block body: {}", e); None }) .unwrap_or_default() @@ -685,7 +685,7 @@ where { log::debug!( target: "txpool", - "[{:?}] Error re-submitting transactions: {:?}", + "[{:?}] Error re-submitting transactions: {}", id, e, ) diff --git a/client/transaction-pool/src/revalidation.rs b/client/transaction-pool/src/revalidation.rs index 22b526e9dfc6d..e3641008a7061 100644 --- a/client/transaction-pool/src/revalidation.rs +++ b/client/transaction-pool/src/revalidation.rs @@ -106,7 +106,7 @@ async fn batch_revalidate( Err(validation_err) => { log::debug!( target: "txpool", - "[{:?}]: Error during revalidation: {:?}. Removing.", + "[{:?}]: Removing due to error during revalidation: {}", ext_hash, validation_err ); diff --git a/frame/contracts/rpc/src/lib.rs b/frame/contracts/rpc/src/lib.rs index 580b74b5ca46d..e83e4e6249b92 100644 --- a/frame/contracts/rpc/src/lib.rs +++ b/frame/contracts/rpc/src/lib.rs @@ -302,11 +302,11 @@ where } /// Converts a runtime trap into an RPC error. -fn runtime_error_into_rpc_err(err: impl std::fmt::Debug) -> Error { +fn runtime_error_into_rpc_err(err: impl std::fmt::Display) -> Error { Error { code: ErrorCode::ServerError(RUNTIME_ERROR), message: "Runtime error".into(), - data: Some(format!("{:?}", err).into()), + data: Some(err.to_string().into()), } } diff --git a/frame/merkle-mountain-range/rpc/src/lib.rs b/frame/merkle-mountain-range/rpc/src/lib.rs index b256ccdd7d327..bf3eb3b694e39 100644 --- a/frame/merkle-mountain-range/rpc/src/lib.rs +++ b/frame/merkle-mountain-range/rpc/src/lib.rs @@ -144,11 +144,11 @@ fn mmr_error_into_rpc_error(err: MmrError) -> Error { } /// Converts a runtime trap into an RPC error. -fn runtime_error_into_rpc_error(err: impl std::fmt::Debug) -> Error { +fn runtime_error_into_rpc_error(err: impl std::fmt::Display) -> Error { Error { code: ErrorCode::ServerError(RUNTIME_ERROR), message: "Runtime trapped".into(), - data: Some(format!("{:?}", err).into()), + data: Some(err.to_string().into()), } } diff --git a/frame/transaction-payment/rpc/src/lib.rs b/frame/transaction-payment/rpc/src/lib.rs index b2ff31618f78a..29d94fa260105 100644 --- a/frame/transaction-payment/rpc/src/lib.rs +++ b/frame/transaction-payment/rpc/src/lib.rs @@ -103,7 +103,7 @@ where api.query_info(&at, uxt, encoded_len).map_err(|e| RpcError { code: ErrorCode::ServerError(Error::RuntimeError.into()), message: "Unable to query dispatch info.".into(), - data: Some(format!("{:?}", e).into()), + data: Some(e.to_string().into()), }) } @@ -127,7 +127,7 @@ where let fee_details = api.query_fee_details(&at, uxt, encoded_len).map_err(|e| RpcError { code: ErrorCode::ServerError(Error::RuntimeError.into()), message: "Unable to query fee details.".into(), - data: Some(format!("{:?}", e).into()), + data: Some(e.to_string().into()), })?; let try_into_rpc_balance = |value: Balance| { diff --git a/primitives/api/proc-macro/src/impl_runtime_apis.rs b/primitives/api/proc-macro/src/impl_runtime_apis.rs index 7241a1c2610de..f6de60e1c99b8 100644 --- a/primitives/api/proc-macro/src/impl_runtime_apis.rs +++ b/primitives/api/proc-macro/src/impl_runtime_apis.rs @@ -297,7 +297,7 @@ fn generate_runtime_api_base_structures() -> Result { let state_version = self.call .runtime_version_at(&at) .map(|v| v.state_version()) - .map_err(|e| format!("Failed to get state version: {:?}", e))?; + .map_err(|e| format!("Failed to get state version: {}", e))?; self.changes.replace(Default::default()).into_storage_changes( backend, diff --git a/primitives/blockchain/src/error.rs b/primitives/blockchain/src/error.rs index bbd65e002a3b3..c82fb9bebf4ee 100644 --- a/primitives/blockchain/src/error.rs +++ b/primitives/blockchain/src/error.rs @@ -69,7 +69,7 @@ pub enum Error { ExtrinsicRootInvalid { received: String, expected: String }, // `inner` cannot be made member, since it lacks `std::error::Error` trait bounds. - #[error("Execution failed: {0:?}")] + #[error("Execution failed: {0}")] Execution(Box), #[error("Blockchain")] diff --git a/primitives/consensus/common/src/lib.rs b/primitives/consensus/common/src/lib.rs index 492ad83ddf5bd..edf393fa229ad 100644 --- a/primitives/consensus/common/src/lib.rs +++ b/primitives/consensus/common/src/lib.rs @@ -98,7 +98,7 @@ pub trait Environment { + Unpin + 'static; /// Error which can occur upon creation. - type Error: From + std::fmt::Debug + 'static; + type Error: From + std::error::Error + 'static; /// Initialize the proposal logic on top of a specific header. Provide /// the authorities at that header. @@ -191,7 +191,7 @@ mod private { /// Proposers are generic over bits of "consensus data" which are engine-specific. pub trait Proposer { /// Error type which can occur when proposing or evaluating. - type Error: From + std::fmt::Debug + 'static; + type Error: From + std::error::Error + 'static; /// The transaction type used by the backend. type Transaction: Default + Send + 'static; /// Future that resolves to a committed proposal with an optional proof. diff --git a/primitives/io/Cargo.toml b/primitives/io/Cargo.toml index 207a1a23e81d9..b9cbdbaa70e77 100644 --- a/primitives/io/Cargo.toml +++ b/primitives/io/Cargo.toml @@ -66,3 +66,22 @@ with-tracing = [ disable_panic_handler = [] disable_oom = [] disable_allocator = [] + +# This feature flag controls the runtime's behavior when encountering +# a panic or when it runs out of memory, improving the diagnostics. +# +# When enabled the runtime will marshal the relevant error message +# to the host through the `PanicHandler::abort_on_panic` runtime interface. +# This gives the caller direct programmatic access to the error message. +# +# When disabled the error message will only be printed out in the +# logs, with the caller receving a generic "wasm `unreachable` instruction executed" +# error message. +# +# This has no effect if both `disable_panic_handler` and `disable_oom` +# are enabled. +# +# WARNING: Enabling this feature flag requires the `PanicHandler::abort_on_panic` +# host function to be supported by the host. Do *not* enable it for your +# runtime without first upgrading your host client! +improved_panic_error_reporting = [] diff --git a/primitives/io/src/lib.rs b/primitives/io/src/lib.rs index 76ced407090c3..db86fe0964156 100644 --- a/primitives/io/src/lib.rs +++ b/primitives/io/src/lib.rs @@ -1290,6 +1290,17 @@ pub trait Allocator { } } +/// WASM-only interface which allows for aborting the execution in case +/// of an unrecoverable error. +#[runtime_interface(wasm_only)] +pub trait PanicHandler { + /// Aborts the current execution with the given error message. + #[trap_on_return] + fn abort_on_panic(&mut self, message: &str) { + self.register_panic_error_message(message); + } +} + /// Interface that provides functions for logging from within the runtime. #[runtime_interface] pub trait Logging { @@ -1588,14 +1599,14 @@ pub trait RuntimeTasks { } /// Allocator used by Substrate when executing the Wasm runtime. -#[cfg(not(feature = "std"))] +#[cfg(all(target_arch = "wasm32", not(feature = "std")))] struct WasmAllocator; -#[cfg(all(not(feature = "disable_allocator"), not(feature = "std")))] +#[cfg(all(target_arch = "wasm32", not(feature = "disable_allocator"), not(feature = "std")))] #[global_allocator] static ALLOCATOR: WasmAllocator = WasmAllocator; -#[cfg(not(feature = "std"))] +#[cfg(all(target_arch = "wasm32", not(feature = "std")))] mod allocator_impl { use super::*; use core::alloc::{GlobalAlloc, Layout}; @@ -1617,16 +1628,30 @@ mod allocator_impl { #[no_mangle] pub fn panic(info: &core::panic::PanicInfo) -> ! { let message = sp_std::alloc::format!("{}", info); - logging::log(LogLevel::Error, "runtime", message.as_bytes()); - core::arch::wasm32::unreachable(); + #[cfg(feature = "improved_panic_error_reporting")] + { + panic_handler::abort_on_panic(&message); + } + #[cfg(not(feature = "improved_panic_error_reporting"))] + { + logging::log(LogLevel::Error, "runtime", message.as_bytes()); + core::arch::wasm32::unreachable(); + } } /// A default OOM handler for WASM environment. #[cfg(all(not(feature = "disable_oom"), not(feature = "std")))] #[alloc_error_handler] pub fn oom(_: core::alloc::Layout) -> ! { - logging::log(LogLevel::Error, "runtime", b"Runtime memory exhausted. Aborting"); - core::arch::wasm32::unreachable(); + #[cfg(feature = "improved_panic_error_reporting")] + { + panic_handler::abort_on_panic("Runtime memory exhausted."); + } + #[cfg(not(feature = "improved_panic_error_reporting"))] + { + logging::log(LogLevel::Error, "runtime", b"Runtime memory exhausted. Aborting"); + core::arch::wasm32::unreachable(); + } } /// Type alias for Externalities implementation used in tests. @@ -1646,6 +1671,7 @@ pub type SubstrateHostFunctions = ( crypto::HostFunctions, hashing::HostFunctions, allocator::HostFunctions, + panic_handler::HostFunctions, logging::HostFunctions, sandbox::HostFunctions, crate::trie::HostFunctions, diff --git a/primitives/runtime-interface/proc-macro/src/runtime_interface/bare_function_interface.rs b/primitives/runtime-interface/proc-macro/src/runtime_interface/bare_function_interface.rs index a06a1f9bda73e..b5745e25deb4c 100644 --- a/primitives/runtime-interface/proc-macro/src/runtime_interface/bare_function_interface.rs +++ b/primitives/runtime-interface/proc-macro/src/runtime_interface/bare_function_interface.rs @@ -32,11 +32,12 @@ use crate::utils::{ create_exchangeable_host_function_ident, create_function_ident_with_version, generate_crate_access, get_function_argument_names, get_function_arguments, - get_runtime_interface, + get_runtime_interface, RuntimeInterfaceFunction, }; use syn::{ - parse_quote, spanned::Spanned, FnArg, Ident, ItemTrait, Result, Signature, TraitItemMethod, + parse_quote, spanned::Spanned, FnArg, Ident, ItemTrait, Result, Signature, Token, + TraitItemMethod, }; use proc_macro2::{Span, TokenStream}; @@ -74,14 +75,14 @@ pub fn generate(trait_def: &ItemTrait, is_wasm_only: bool, tracing: bool) -> Res /// Generates the bare function implementation for the given method for the host and wasm side. fn function_for_method( - method: &TraitItemMethod, + method: &RuntimeInterfaceFunction, latest_version: u32, is_wasm_only: bool, ) -> Result { let std_impl = if !is_wasm_only { function_std_latest_impl(method, latest_version)? } else { quote!() }; - let no_std_impl = function_no_std_impl(method)?; + let no_std_impl = function_no_std_impl(method, is_wasm_only)?; Ok(quote! { #std_impl @@ -91,20 +92,46 @@ fn function_for_method( } /// Generates the bare function implementation for `cfg(not(feature = "std"))`. -fn function_no_std_impl(method: &TraitItemMethod) -> Result { +fn function_no_std_impl( + method: &RuntimeInterfaceFunction, + is_wasm_only: bool, +) -> Result { let function_name = &method.sig.ident; let host_function_name = create_exchangeable_host_function_ident(&method.sig.ident); let args = get_function_arguments(&method.sig); let arg_names = get_function_argument_names(&method.sig); - let return_value = &method.sig.output; + let return_value = if method.should_trap_on_return() { + syn::ReturnType::Type( + ]>::default(), + Box::new(syn::TypeNever { bang_token: ::default() }.into()), + ) + } else { + method.sig.output.clone() + }; + let maybe_unreachable = if method.should_trap_on_return() { + quote! { + ; core::arch::wasm32::unreachable(); + } + } else { + quote! {} + }; + let attrs = method.attrs.iter().filter(|a| !a.path.is_ident("version")); + let cfg_wasm_only = if is_wasm_only { + quote! { #[cfg(target_arch = "wasm32")] } + } else { + quote! {} + }; + Ok(quote! { + #cfg_wasm_only #[cfg(not(feature = "std"))] #( #attrs )* pub fn #function_name( #( #args, )* ) #return_value { // Call the host function #host_function_name.get()( #( #arg_names, )* ) + #maybe_unreachable } }) } diff --git a/primitives/runtime-interface/proc-macro/src/runtime_interface/host_function_interface.rs b/primitives/runtime-interface/proc-macro/src/runtime_interface/host_function_interface.rs index 626e309cc0e1c..1566bbf302c3b 100644 --- a/primitives/runtime-interface/proc-macro/src/runtime_interface/host_function_interface.rs +++ b/primitives/runtime-interface/proc-macro/src/runtime_interface/host_function_interface.rs @@ -26,7 +26,7 @@ use crate::utils::{ create_host_function_ident, generate_crate_access, get_function_argument_names, get_function_argument_names_and_types_without_ref, get_function_argument_types, get_function_argument_types_ref_and_mut, get_function_argument_types_without_ref, - get_function_arguments, get_runtime_interface, + get_function_arguments, get_runtime_interface, RuntimeInterfaceFunction, }; use syn::{ @@ -205,7 +205,7 @@ fn generate_host_functions_struct( /// implementation of the function. fn generate_host_function_implementation( trait_name: &Ident, - method: &TraitItemMethod, + method: &RuntimeInterfaceFunction, version: u32, is_wasm_only: bool, ) -> Result<(TokenStream, Ident, TokenStream)> { diff --git a/primitives/runtime-interface/proc-macro/src/runtime_interface/trait_decl_impl.rs b/primitives/runtime-interface/proc-macro/src/runtime_interface/trait_decl_impl.rs index c48da3b788518..0ae0f5260286c 100644 --- a/primitives/runtime-interface/proc-macro/src/runtime_interface/trait_decl_impl.rs +++ b/primitives/runtime-interface/proc-macro/src/runtime_interface/trait_decl_impl.rs @@ -153,7 +153,7 @@ fn impl_trait_for_externalities(trait_def: &ItemTrait, is_wasm_only: bool) -> Re let crate_ = generate_crate_access(); let interface = get_runtime_interface(trait_def)?; let methods = interface.all_versions().map(|(version, method)| { - let mut cloned = method.clone(); + let mut cloned = (*method).clone(); cloned.attrs.retain(|a| !a.path.is_ident("version")); cloned.sig.ident = create_function_ident_with_version(&cloned.sig.ident, version); cloned diff --git a/primitives/runtime-interface/proc-macro/src/utils.rs b/primitives/runtime-interface/proc-macro/src/utils.rs index bc690eb21a9bd..19f7fea023c30 100644 --- a/primitives/runtime-interface/proc-macro/src/utils.rs +++ b/primitives/runtime-interface/proc-macro/src/utils.rs @@ -39,18 +39,64 @@ mod attributes { syn::custom_keyword!(register_only); } +/// A concrete, specific version of a runtime interface function. +pub struct RuntimeInterfaceFunction { + item: TraitItemMethod, + should_trap_on_return: bool, +} + +impl std::ops::Deref for RuntimeInterfaceFunction { + type Target = TraitItemMethod; + fn deref(&self) -> &Self::Target { + &self.item + } +} + +impl RuntimeInterfaceFunction { + fn new(item: &TraitItemMethod) -> Result { + let mut item = item.clone(); + let mut should_trap_on_return = false; + item.attrs.retain(|attr| { + if attr.path.is_ident("trap_on_return") { + should_trap_on_return = true; + false + } else { + true + } + }); + + if should_trap_on_return { + if !matches!(item.sig.output, syn::ReturnType::Default) { + return Err(Error::new( + item.sig.ident.span(), + "Methods marked as #[trap_on_return] cannot return anything", + )) + } + } + + Ok(Self { item, should_trap_on_return }) + } + + pub fn should_trap_on_return(&self) -> bool { + self.should_trap_on_return + } +} + /// Runtime interface function with all associated versions of this function. -pub struct RuntimeInterfaceFunction<'a> { +struct RuntimeInterfaceFunctionSet { latest_version_to_call: Option, - versions: BTreeMap, + versions: BTreeMap, } -impl<'a> RuntimeInterfaceFunction<'a> { - fn new(version: VersionAttribute, trait_item: &'a TraitItemMethod) -> Self { - Self { +impl RuntimeInterfaceFunctionSet { + fn new(version: VersionAttribute, trait_item: &TraitItemMethod) -> Result { + Ok(Self { latest_version_to_call: version.is_callable().then(|| version.version), - versions: BTreeMap::from([(version.version, trait_item)]), - } + versions: BTreeMap::from([( + version.version, + RuntimeInterfaceFunction::new(trait_item)?, + )]), + }) } /// Returns the latest version of this runtime interface function plus the actual function @@ -59,11 +105,11 @@ impl<'a> RuntimeInterfaceFunction<'a> { /// This isn't required to be the latest version, because a runtime interface function can be /// annotated with `register_only` to ensure that the host exposes the host function but it /// isn't used when compiling the runtime. - pub fn latest_version_to_call(&self) -> Option<(u32, &TraitItemMethod)> { + pub fn latest_version_to_call(&self) -> Option<(u32, &RuntimeInterfaceFunction)> { self.latest_version_to_call.map(|v| { ( v, - *self.versions.get(&v).expect( + self.versions.get(&v).expect( "If latest_version_to_call has a value, the key with this value is in the versions; qed", ), ) @@ -74,7 +120,7 @@ impl<'a> RuntimeInterfaceFunction<'a> { fn add_version( &mut self, version: VersionAttribute, - trait_item: &'a TraitItemMethod, + trait_item: &TraitItemMethod, ) -> Result<()> { if let Some(existing_item) = self.versions.get(&version.version) { let mut err = Error::new(trait_item.span(), "Duplicated version attribute"); @@ -86,7 +132,8 @@ impl<'a> RuntimeInterfaceFunction<'a> { return Err(err) } - self.versions.insert(version.version, trait_item); + self.versions + .insert(version.version, RuntimeInterfaceFunction::new(trait_item)?); if self.latest_version_to_call.map_or(true, |v| v < version.version) && version.is_callable() { @@ -98,22 +145,24 @@ impl<'a> RuntimeInterfaceFunction<'a> { } /// All functions of a runtime interface grouped by the function names. -pub struct RuntimeInterface<'a> { - items: BTreeMap>, +pub struct RuntimeInterface { + items: BTreeMap, } -impl<'a> RuntimeInterface<'a> { +impl RuntimeInterface { /// Returns an iterator over all runtime interface function - /// [`latest_version_to_call`](RuntimeInterfaceFunction::latest_version). - pub fn latest_versions_to_call(&self) -> impl Iterator { + /// [`latest_version_to_call`](RuntimeInterfaceFunctionSet::latest_version). + pub fn latest_versions_to_call( + &self, + ) -> impl Iterator { self.items.iter().filter_map(|(_, item)| item.latest_version_to_call()) } - pub fn all_versions(&self) -> impl Iterator { + pub fn all_versions(&self) -> impl Iterator { self.items .iter() .flat_map(|(_, item)| item.versions.iter()) - .map(|(v, i)| (*v, *i)) + .map(|(v, i)| (*v, i)) } } @@ -288,8 +337,8 @@ fn get_item_version(item: &TraitItemMethod) -> Result> } /// Returns all runtime interface members, with versions. -pub fn get_runtime_interface<'a>(trait_def: &'a ItemTrait) -> Result> { - let mut functions: BTreeMap> = BTreeMap::new(); +pub fn get_runtime_interface(trait_def: &ItemTrait) -> Result { + let mut functions: BTreeMap = BTreeMap::new(); for item in get_trait_methods(trait_def) { let name = item.sig.ident.clone(); @@ -301,7 +350,7 @@ pub fn get_runtime_interface<'a>(trait_def: &'a ItemTrait) -> Result { - entry.insert(RuntimeInterfaceFunction::new(version, item)); + entry.insert(RuntimeInterfaceFunctionSet::new(version, item)?); }, Entry::Occupied(mut entry) => { entry.get_mut().add_version(version, item)?; diff --git a/primitives/runtime/src/traits.rs b/primitives/runtime/src/traits.rs index b2e218cb9db73..6a829ea6bba74 100644 --- a/primitives/runtime/src/traits.rs +++ b/primitives/runtime/src/traits.rs @@ -1585,7 +1585,7 @@ impl Printable for Tuple { #[cfg(feature = "std")] pub trait BlockIdTo { /// The error type that will be returned by the functions. - type Error: std::fmt::Debug; + type Error: std::error::Error; /// Convert the given `block_id` to the corresponding block hash. fn to_hash( diff --git a/primitives/wasm-interface/src/lib.rs b/primitives/wasm-interface/src/lib.rs index 21dbf9609ecf3..d57666f126899 100644 --- a/primitives/wasm-interface/src/lib.rs +++ b/primitives/wasm-interface/src/lib.rs @@ -305,6 +305,29 @@ pub trait FunctionContext { fn deallocate_memory(&mut self, ptr: Pointer) -> Result<()>; /// Provides access to the sandbox. fn sandbox(&mut self) -> &mut dyn Sandbox; + + /// Registers a panic error message within the executor. + /// + /// This is meant to be used in situations where the runtime + /// encounters an unrecoverable error and intends to panic. + /// + /// Panicking in WASM is done through the [`unreachable`](https://webassembly.github.io/spec/core/syntax/instructions.html#syntax-instr-control) + /// instruction which causes an unconditional trap and immediately aborts + /// the execution. It does not however allow for any diagnostics to be + /// passed through to the host, so while we do know that *something* went + /// wrong we don't have any direct indication of what *exactly* went wrong. + /// + /// As a workaround we use this method right before the execution is + /// actually aborted to pass an error message to the host so that it + /// can associate it with the next trap, and return that to the caller. + /// + /// A WASM trap should be triggered immediately after calling this method; + /// otherwise the error message might be associated with a completely + /// unrelated trap. + /// + /// It should only be called once, however calling it more than once + /// is harmless and will overwrite the previously set error message. + fn register_panic_error_message(&mut self, message: &str); } /// Sandbox memory identifier. diff --git a/utils/frame/benchmarking-cli/src/command.rs b/utils/frame/benchmarking-cli/src/command.rs index a5b53bcf99c76..0ced8b28ce016 100644 --- a/utils/frame/benchmarking-cli/src/command.rs +++ b/utils/frame/benchmarking-cli/src/command.rs @@ -165,7 +165,7 @@ impl BenchmarkCmd { sp_core::testing::TaskExecutor::new(), ) .execute(strategy.into()) - .map_err(|e| format!("Error getting benchmark list: {:?}", e))?; + .map_err(|e| format!("Error getting benchmark list: {}", e))?; let (list, storage_info) = <(Vec, Vec) as Decode>::decode(&mut &result[..]) @@ -265,7 +265,7 @@ impl BenchmarkCmd { ) .execute(strategy.into()) .map_err(|e| { - format!("Error executing and verifying runtime benchmark: {:?}", e) + format!("Error executing and verifying runtime benchmark: {}", e) })?; } // Do one loop of DB tracking. @@ -290,7 +290,7 @@ impl BenchmarkCmd { sp_core::testing::TaskExecutor::new(), ) .execute(strategy.into()) - .map_err(|e| format!("Error executing runtime benchmark: {:?}", e))?; + .map_err(|e| format!("Error executing runtime benchmark: {}", e))?; let batch = , String> as Decode>::decode( @@ -322,7 +322,7 @@ impl BenchmarkCmd { sp_core::testing::TaskExecutor::new(), ) .execute(strategy.into()) - .map_err(|e| format!("Error executing runtime benchmark: {:?}", e))?; + .map_err(|e| format!("Error executing runtime benchmark: {}", e))?; let batch = , String> as Decode>::decode( diff --git a/utils/frame/rpc/system/src/lib.rs b/utils/frame/rpc/system/src/lib.rs index df24e208b51a4..eb1b258c97ec6 100644 --- a/utils/frame/rpc/system/src/lib.rs +++ b/utils/frame/rpc/system/src/lib.rs @@ -106,7 +106,7 @@ where let nonce = api.account_nonce(&at, account.clone()).map_err(|e| RpcError { code: ErrorCode::ServerError(Error::RuntimeError.into()), message: "Unable to query nonce.".into(), - data: Some(format!("{:?}", e).into()), + data: Some(e.to_string().into()), })?; Ok(adjust_nonce(&*self.pool, account, nonce)) @@ -141,7 +141,7 @@ where let result = api.apply_extrinsic(&at, uxt).map_err(|e| RpcError { code: ErrorCode::ServerError(Error::RuntimeError.into()), message: "Unable to dry run extrinsic.".into(), - data: Some(format!("{:?}", e).into()), + data: Some(e.to_string().into()), })?; Ok(Encode::encode(&result).into()) diff --git a/utils/frame/try-runtime/cli/src/lib.rs b/utils/frame/try-runtime/cli/src/lib.rs index 4c71033288514..ae7a1c3ae87ca 100644 --- a/utils/frame/try-runtime/cli/src/lib.rs +++ b/utils/frame/try-runtime/cli/src/lib.rs @@ -707,7 +707,7 @@ pub(crate) fn state_machine_call(Into::into)?; Ok((changes, encoded_results)) @@ -748,7 +748,7 @@ pub(crate) fn state_machine_call_with_proof(Into::into)?; let proof = proving_backend.extract_proof();