update to latest polkadot-sdk #467
Conversation
| async fn test_next_fee_multiplier_minimum() { | ||
| // 1e18 denomination. | ||
| let new_base_fee = U256::from(50_123); | ||
| let new_base_fee = U256::from(50_1230); |
There was a problem hiding this comment.
This will not fix the actual issue. You will have to set the GasScale to 1.
Like this: f522500#diff-bf8d1cad61c801a9cfd0ba5917125913b960af9bdbeba4fbfe846bb86e3c3d0dR310-R311
There was a problem hiding this comment.
The default value in test config I think is 10, hence the off by 10
There was a problem hiding this comment.
i'll revert the anvil change in this PR then and cherry-pick your changes then
There was a problem hiding this comment.
You can directly add GasScale I think there is no need to cherry-pick
There was a problem hiding this comment.
The default pallet-revive config for GasScale is 10 based on this PR: https://github.com/paritytech/polkadot-sdk/pull/10393/changes (which also multiplies the next_fee_multiplier by gas scale, not just NATIVE_TO_ETH_RATIO). That's why, given our substrate-runtime not setting it up specifically with this PR, or before it, we get in practice 10 times bigger base fee. For these tests to pass it is enough to configure the GasScale under the pallet-revive config in the anvil-polkadot's substrate-runtime to 1, but that might not do it for certain users tests (outside anvil-polkadot) that rely on a bigger value (e.g. dev-node sets it to 50_000).
crates/revive-env/src/runtime.rs
Outdated
| pub const DepositPerItem: Balance = 1; | ||
| pub const CodeHashLockupDepositPercent: Perbill = Perbill::from_percent(0); | ||
| pub const NativeToEthRatio: u32 = 1_000_000; | ||
| pub const GasScale : u32 = u32::MAX; |
There was a problem hiding this comment.
Isn't this very big? Also, we'd need to capture the value in a pub constant that will be included in the constants mod, so that it can be imported by anvil-polkadot where needed.
base_fee is considered throughout the code as FixedU128::from_rational(<number>, NATIVE_TO_ETH_RATIO). Now we'd need to set it like mainly because we want it to be equal to the result of FixedU128::from_rational(<number>, NATIVE_TO_ETH_RATIO * GAS_SCALE)pallet_revive's evm_base_fee.
There was a problem hiding this comment.
Scratch that, in the storage the base_fee is still stored as FixedU128::from_rational(<number>, NATIVE_TO_ETH_RATIO). Hm.. the only affected parts are related to the code base which calls EthGasPrice, or the pallet-revive's evm_base_fee API. I'll need to look better when that's done.
There was a problem hiding this comment.
(Please ignore where my comment is attached to - the concern is that polkadot-sdk update will impact anvil-polkadot)
Ok, so the way I see it is the following. Calling the SetNextBlockBaseFeePerGas RPC with a certain value, and then expecting to see it after the mining of the next block will not be the case like before after paritytech/polkadot-sdk#10393, if GasScale > 1. This must be double-checked though.
I see a few things that are worth clarifying:
-
(solvable) If we set it to >1, then we'd need to divide what we return to users when calling
EthGasPriceby the gas scale (so that users get same thing they set). -
(issue) With every eth block the base_fee_per_gas field is computed based on pallet-revive's
evm_base_fee, which is multiplied by gas scale, and adjusting this field is not in our control, so what the user would set via the RPC will not match the block value. -
(question) I know this
GasScaleset to something >1 is relevant for a some SCC tests which hardcode the transaction gas limit to a large value IIRC, which will result in the issue at 2. It is not that clear to me what should the default GasScale value be for anvil-polkadot's substrate-runtime so that those tests pass, and whether we're still interested in setting it if 2. pops up.
There was a problem hiding this comment.
Short on time, so I didn't check the code in foundry: why do you even need to bother with the GasScale? If a user overwrites the BaseFeePerGas, then this value should just overwrite the return value of evm_base_fee.
We don't yet have a good answer to question 3. The idea is to scale gas units so that similar transactions require similar gas amounts in revive and in an Ethereum node. That cannot be compared one-by-one, though and depends on other configuration of the chain.
I think that something like 50000 or 10000 could be a good yard stick.
There was a problem hiding this comment.
Short on time, so I didn't check the code in foundry: why do you even need to bother with the
GasScale? If a user overwrites theBaseFeePerGas, then this value should just overwrite the return value ofevm_base_fee.We don't yet have a good answer to question 3. The idea is to scale gas units so that similar transactions require similar gas amounts in revive and in an Ethereum node. That cannot be compared one-by-one, though and depends on other configuration of the chain.
I think that something like 50000 or 10000 could be a good yard stick.
as of right now setting gasScale to 50000 or 10000 still leads to issue of OutOfGas errors with forge test. even setting it to 1_000_000 still doesn't help. also in case of StaticCall if deposit_limit is set to BalanceOf::<Runtime>::MAX we also run out of gas
There was a problem hiding this comment.
why do you even need to bother with the GasScale? If a user overwrites the BaseFeePerGas, then this value should just overwrite the return value of evm_base_fee.
The evm_base_fee overwrite happens in anvil-polkadot by overwriting the next fee multiplier only. We need to bother with GasScale because setting it != 1 results in a certain base_fee_per_gas, as returned by evm_base_fee, which if not interpreted correctly can result in a different value than what users set via SetNextBlockBaseFeePerGas.
We could also "normalize" what users set with FixedU128::from_rational(<number>, NATIVE_TO_ETH_RATIO * GAS_SCALE) (like I initially suggested here, which fixes point 2), but I thought it defeats the purpose of actually using the GasScale in the first place. On anvil-polkadot side setting a certain base_fee_per_gas for next block or for block number 1 can also be done via CLI, so those SCC tests failing at point 3 might be fixable by just starting anvil-polkadot with a certain base_fee_per_gas via CLI. This would be compatible with the idea of not bothering at all with GasScale, and setting it to 1 for anvil-polkadot - and leave for 3 the CLI based tweaking of the base fee per gas, while the other configurations of the chain (which equate basically to anvil-polkadot's substrate-runtime config) to be discovered via experiments.
There was a problem hiding this comment.
I suggest setting the GasScale to 1 for now, to maintain compatibility. If/When there will be a recommended value which production chains will use, we can update the anvil runtime to use that and if the user sets a custom value for the gas price via RPC or CLI, multiply/divide it accordingly to get to their desired value, as @iulianbarbu suggested (my suggestion only applies to anvil, not sure why the out of gas errors reported on forge)
There was a problem hiding this comment.
I think setting to 1 is ok for this PR. We'll have issues with forking though, or these SCC tests that might need a != 1 value, but like you said.. we could multiply/divide where needed at that point.
@pkhry , please consider that this change should be done in crates/anvil-polkadot/substrate-runtime/lib.rs, not in the scope of the file this comment started for. 🙈
No description provided.