When set '0' value in contracts event, may cause Error::ContractTrapped and panic in contract

[atenjin]

When set '0' value in contracts event, may cause Error::ContractTrapped and panic in contract
Setting 0 to strorage value is ok, but set 0 to event value, would cause this ContractTrapped

This is a bug when I try to test contract example like "erc20" or something else.

ink commit: 3803a26
substrate contract module commit:

• substrate tag 2.0.0
• 69198d1e5735798c8baa1921c6e2091c93260752
  (I test this problem in substrate v2.0.0 version and newest substrate version)

Describe the bug
This bug happen when I try to test erc20 example contract. First I run canvas-ui and canvas-node, then I compile erc20 contracts, and use canvas-ui to put-code then deploy contract.

But when I try deploy contract, I pass "0" for initialSupply parameter（meaning init value is zero）,then, deploy process return an Error Error::ContractTrapped.

I use newest substrate node has the same error. Then I try to figure out the reason. I modified the erc20 example, and find that, when I set 0 to the event value, the deploy process must cause this error:
in erc20 contract:

        #[ink(constructor)]
        pub fn new(initial_supply: Balance) -> Self {
          // ... other code
            Self::env().emit_event(Transfer {
                from: None,
                to: Some(caller),
                value: 0_u128.into(),  // here, I set 0 for value field
            });
            ::ink_env::debug_println("Erc20 contract init finish");
            instance
        }

And If set value to other value, like set value: 1_u128.into(), this contract could run normally.

Then I trace in substrate contract module, and find out the error Error::ContractTrapped is from:
primitives/sandbox/with_std.rs ( in Native execution mode)

impl<'a, T> Externals for GuestExternals<'a, T> {
	fn invoke_index(
		&mut self,
		index: usize,
		args: RuntimeArgs,
	) -> Result<Option<RuntimeValue>, Trap> {
// ...
		let result = (self.defined_host_functions.funcs[index])(self.state, &args);
		match result {
			Ok(value) => Ok(match value {
				ReturnValue::Value(v) => Some(v.into()),
				ReturnValue::Unit => None,
			}),
			Err(HostError) => (TrapKind::Host(Box::new(DummyHostError)).into()),  // here return "HostError"
		}
	}

this function return HostError and convert to TrapKind::Host Error.
and then

	pub fn invoke(
		&mut self,
		name: &str,
		args: &[Value],
		state: &mut T,
	) -> Result<ReturnValue, Error> {
// ...
		match result {
			Ok(None) => Ok(ReturnValue::Unit),
			Ok(Some(val)) => Ok(ReturnValue::Value(val.into())),
			Err(_err) => (Error::Execution),
}

convert TrapKind::Host Error to Error::Execution
and finally in pallet-contracts, detail in frame/contracts/src/wasm/runtime.rs L200:

		Err(sp_sandbox::Error::Execution) | Err(sp_sandbox::Error::OutOfBounds) =>
			Err(Error::<E::T>::ContractTrapped)?

convert Error::Execution to Error::<E::T>::ContractTrapped error.

Expected behavior
contracts should run normally.

[athei]

Usually, ContractTapped means that the contract executed an unreachable wasm instruction. However, the error reporting in pallet contracts is not yet completely finished with transitioning to proper module errors and uses ContractTrapped as a fallback whenever an error occurred in some of the supervisor functions that has no explicit error attached. This is bad and will be fixed in the two outstanding PRs for pallet contracts.

All of this said, I could reproduce this error and determined that it is exactly this kind of error that happened. The seal_deposit_event has some error paths that do wrongly throw a ContractTrapped. In this case a duplicate topic was passed to pallet contracts from ink:
https://github.com/paritytech/substrate/blob/a208da1608712c1bec40fb4a3878187b0a5ea404/frame/contracts/src/wasm/runtime.rs#L1188-L1191

I guess the reason is that Option<_>::None and 0u128 have the same encoding in scale and are both declared as topic. Imho ink!'s emit_event should be fallible and error on duplicate topics.

[atenjin]

Oh, I forget to check the process in seal_deposit_event. I just regard this error from wasm execution process.
and for this part:

Usually, ContractTapped means that the contract executed an unreachable wasm instruction. However, the error reporting in pallet contracts is not yet completely finished with transitioning to proper module errors and uses ContractTrapped as a fallback whenever an error occurred in some of the supervisor functions that has no explicit error attached. This is bad and will be fixed in the two outstanding PRs for pallet contracts.

In fact in our project europa version v0.2 plan, https://polkadot.polkassembly.io/motion/28,

Strengthen the error type and error display of the contract module;

we would try to enhance the error display for pallet-contracts.😂😂😂

It seems that we could wait after "two outstanding PRs for pallet contracts", then we can re-plan our design later.

[athei]

I am just gradually improving the on-chain error reporting so that you get a more diverse set of error messages when an extrinsic failed. It is no replacement nor does it collide with a fully fledged debugging VM like you are building. Those are all static error messages (basically glorified integers) that cannot contain any dynamic infos that would help during debugging.

I added a debug_message field to the contracts_call RPC, though. That can be used to output dynamic information to the user when executing off-chain in the future. It is meant as a way for Dapps to output errors to the user when pre-running the transaction before submission to the chain.

[cmichi]

I guess the reason is that Option<_>::None and 0u128 have the same encoding in scale and are both declared as topic. Imho ink!'s emit_event should be fallible and error on duplicate topics.

@athei Thanks a lot for identifying this issue!