Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Different gas value for weight_to_fee() in ink! integration and e2e tests #1985

Closed
arturoBeccar opened this issue Nov 10, 2023 · 6 comments
Closed
Assignees

Comments

@arturoBeccar
Copy link

Different gas value for weight_to_fee() in ink! integration and e2e tests

On our research into integration and end-to-end function implementation differences, we stumbled upon weight_to_fee() not working as expected. Both Integration and E2E environments return a valid gas price (u128) for a given gas amount. However, the price per gas unit differs in both environments: it is 100 in integration tests and always 0 in E2E tests.

Adding to the test-case and documentation we built for analyzing this issue, we explain below the problem, we describe the work we have undertaken to try to identify its source, and propose next steps for resolving it.

The problem

Given a basic contract with this message

#[ink(message)]
pub fn weight_to_fee(&self, gas: u64) -> Balance {
	self.env().weight_to_fee(gas)
}

When we call the function weight_to_fee() from an integration test we see that the fee for 1 unit of gas is 100.

#[ink::test]
fn integration_weight_to_fee_value() {
	// Given
	let contract = WeightToFee::new();
	// When
	let fee = contract.weight_to_fee(1);
	// Then
	assert_eq!(fee, 100); // True
}

When we call the function weight_to_fee() from an end-to-end test, we see that the fee for 1 unit of gas is 0.

#[ink_e2e::test]

async fn end_to_end_weigth_to_fee_1(mut client: ink_e2e::Client<C, E>) -> E2EResult<()> {
	// Given
	let constructor = WeightToFeeRef::new();
	let contract_acc_id = client	
		.instantiate("weight-to-fee", &ink_e2e::alice(), constructor, 0, None)
		.await
		.expect("instantiate failed")
		.account_id;
	
	// When
	let fee = client
		.call(
			&ink_e2e::bob(),
			build_message::<WeightToFeeRef>(contract_acc_id)
			.call(|contract| contract.weight_to_fee(42)), // changing this number doesn't changes the outcome
			0,
			None,
		)
		.await
		.expect("weight-to-fee failed")
		.return_value();
		  
	// Then
	assert_eq!(fee, 0); // True	
	
	Ok(())
}

This behavior is seen in the local development node too, but not in testnets like Aleph Zero testnet.

Looking at the runtime in polkadot-sdk, we found a few implementations of the functions, but modifying them didn't change the outcome of the tests as predicted.
The functions that we modified to check if something changed were:

  1. polkadot-sdk/substrate/frame/contracts/src/exec.rs, the get_weight_price() function.

  2. polkadot-sdk/substrate/frame/transaction-payment/src/lib.rs, the convert() function.

  3. polkadot-sdk/substrate/frame/system/src/limits.rs, the max_block: Weight::zero() inside the builder() function seems to affect at least thepolkadot-sdk/substrate/frame/transaction-payment/src/lib.rs function weight_to_fee():

	/// Compute the unadjusted portion of the weight fee by invoking the configured `WeightToFee`
	/// impl. Note that the input `weight` is capped by the maximum block weight before computation.
	pub fn weight_to_fee(weight: Weight) -> BalanceOf<T> {
		// cap the weight to the maximum defined in runtime, otherwise it will be the
		// `Bounded` maximum of its data type, which is not desired.
		let capped_weight = weight.min(T::BlockWeights::get().max_block);
		T::WeightToFee::weight_to_fee(&capped_weight)
	}

This seems to be always zero, as the max_block field is zero.

Observations

In polkadot-sdk/substrate/frame/contracts/src/wasm/runtime.rs, there are 2 functions:

	fn weight_to_fee(
		ctx: _,
		memory: _,
		gas: u64,
		out_ptr: u32,
		out_len_ptr: u32,
	) -> Result<(), TrapReason> {
		let gas = Weight::from_parts(gas, 0);
		ctx.charge_gas(RuntimeCosts::WeightToFee)?;
		Ok(ctx.write_sandbox_output(
			memory,
			out_ptr,
			out_len_ptr,
			&ctx.ext.get_weight_price(gas).encode(),
			false,
			already_charged,
		)?)
	}
//**---**//
	#[unstable]
	fn weight_to_fee(
		ctx: _,
		memory: _,
		ref_time_limit: u64,
		proof_size_limit: u64,
		out_ptr: u32,
		out_len_ptr: u32,
	) -> Result<(), TrapReason> {
		let weight = Weight::from_parts(ref_time_limit, proof_size_limit);
		ctx.charge_gas(RuntimeCosts::WeightToFee)?;
		Ok(ctx.write_sandbox_output(
			memory,
			out_ptr,
			out_len_ptr,
			&ctx.ext.get_weight_price(weight).encode(),
			false,
			already_charged,
		)?)
	}

Where the first one has a hardcoded zero. Is that intentional as "working as intended", or is that a workaround for something else?

Next Steps

We propose to make the following changes to weight_to_fee() in ink! e2e tests. Before doing that, we need assistance in identifying all the files relevant to its implementation. Particularly, to identify which code is responible for the function always returning 0.

  • Make the gas value in e2e the same as in integration tests.
  • Make a setter like env().setGasPrice(...) to set the value in the tests (with a default value).
  • Make the gas value an arbitrary X and document it.

Any of this decisions should be properly documented.

@smiasojed
Copy link
Collaborator

Regarding the observation you made: The 0 is intentional, as the comment indicates, "Equivalent to the newer [seal1][super::api_doc::Version2::weight_to_fee] version but works with ref_time Weight only." (proof_size is set to 0).
The issue you observed is related to the runtime configuration. By changing type FeeMultiplierUpdate = () to type FeeMultiplierUpdate = SlowAdjustingFeeUpdate<Self> in pallet_transaction_payment::Config for Runtime, you can make it work.

@smiasojed
Copy link
Collaborator

The code responsible for the function always returning 0:

	fn convert(weight: Weight) -> BalanceOf<T> {
		<NextFeeMultiplier<T>>::get().saturating_mul_int(Self::weight_to_fee(weight))
	}

athei pushed a commit to paritytech/substrate-contracts-node that referenced this issue Nov 28, 2023
It has been reported an issue
(use-ink/ink#1985) that weight_to_fee
always returns zero in end-to-end tests.
This pull request adds `type FeeMultiplierUpdate =
SlowAdjustingFeeUpdate<Self>` to make it work.
@smiasojed
Copy link
Collaborator

@arturoBeccar, please advise if it works for you, and can we close the issue?

@arturoBeccar
Copy link
Author

Hi @smiasojed, recompiling the node on commit f1d8389 , and calling a contract with this function:

 #[ink(message)]
        pub fn weight_to_fee(&self, gas: u64) -> Balance {
            self.env().weight_to_fee(gas)
        }

We still obtain 0 with any gas number in e2e.

weight_to_fee_error_e2e_20231204

Any idea on why this is happening?

@smiasojed
Copy link
Collaborator

smiasojed commented Dec 4, 2023

Hi @arturoBeccar,
This is how I tested it:

  1. I recompiled the substrate-contract-node on commit 'f1d8389.'
  2. I started the node.
  3. I added the 'weight_to_fee' message to the Flipper contract.
  4. I instantiated it.
  5. I called 'weight_to_fee':
    cargo contract call --suri //Alice --manifest-path ../ink/integration-tests/flipper/Cargo.toml --contract 5D4NHEJyj56T7J9TXZwUjmuWBzecFvEunX51GmRmcv1igd8s --message weight_to_fee --args 10
    Result Ok(9)
    Do you verify it in the same manner?

@arturoBeccar
Copy link
Author

Hi @smiasojed. I tried the suggested steps and it worked correctly. I confirm the issue is now resolved in PR #219 to substrate-contract-node and can be closed. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants