Error handling in Substrate (5.4 - Major)

[ansermino]

It is much harder to handle any kind of error in Substrate than it is to do it on Ethereum smart contracts. While on Ethereum one can simple use the revert opcode and be sure that all storage reverts to the previous state the same is not true for Subtrate. All the storage changes that occurred before this error will remain intact.

The best practice in Substrate is to follow the “verify first, write last” principle. During execution you should make all the checks are using the ensure macro and that when you start modifying storage, no error can be thrown after.

While the outlined approach sounds clean, it can get very complicated when inter-pallet communication is thrown into the mix. This is especially true when multiple different external pallets need be called in one function. In such cases it becomes extremely hard to do all the verifications before changing storage.
Examples

There are a few places where this problem may occur:

• transfer_fungible, transfer_nonfungible, transfer_generic functions of the Substrate library are called during the deposit from another pallet. They make use of the ensure macro, so they may potentially fail.

chainbridge/src/lib.rs:L487-L506

   pub fn transfer_fungible(
       dest_id: ChainId,
       resource_id: ResourceId,
       to: Vec<u8>,
       amount: U256,
   ) -> DispatchResult {
       ensure!(
           Self::chain_whitelisted(dest_id),
           Error::<T>::ChainNotWhitelisted
       );
       let nonce = Self::bump_nonce(dest_id);
       Self::deposit_event(RawEvent::FungibleTransfer(
           dest_id,
           nonce,
           resource_id,
           amount,
           to,
       ));
       Ok(())
   }

This means that these functions should be called either before any storage changes (outside of current pallet) or the same checks should be done in the outside pallet, before changing the state. Even though example pallets are out of scope, in this example you can see that the actual transfer is happening before calling the transfer_fungible function:

example-pallet/src/lib.rs:L72-L80

   pub fn transfer_native(origin, amount: BalanceOf<T>, recipient: Vec<u8>, dest_id: bridge::ChainId) -> DispatchResult {
       let source = ensure_signed(origin)?;
       ensure!(<bridge::Module<T>>::chain_whitelisted(dest_id), Error::<T>::InvalidTransfer);
       let bridge_id = <bridge::Module<T>>::account_id();
       T::Currency::transfer(&source, &bridge_id, amount.into(), AllowDeath)?;
  
       let resource_id = T::NativeTokenId::get();
       <bridge::Module<T>>::transfer_fungible(dest_id, resource_id, recipient, U256::from(amount.saturated_into()))
   }

That may lead to tokens actually being transferred to the Bridge, but the event to transfer them outside is not emitted.

• When a proposal passes, its state is saved to storage before calling finalize_execution function.

chainbridge/src/lib.rs:L421-L429

let complete = votes.try_to_complete(<RelayerThreshold>::get(), <RelayerCount>::get());
<Votes<T>>::insert(src_id, (nonce, prop.clone()), votes.clone());
Self::deposit_event(RawEvent::VoteFor(src_id, nonce, who.clone()));

if complete {
    Self::finalize_execution(src_id, nonce, prop)
} else {
    Ok(())
}

inside of finalize_execution function, an external pallet is called (this pallet is not implemented yet) which can potentially also fail, and even if that happens, the proposal will still be considered as executed.

[Polycarpik]

Not actionable.