feat: Calculate ccm gas limit #3935

syan095 · 2023-09-04T04:06:28Z

Pull Request

Checklist

Please conduct a thorough self-review before opening the PR.

I am confident that the code works.
I have updated documentation where appropriate.

Summary

CCM messages now calculate a "gas_limit", which is used when building the Ethereum Transaction.
gas_limit = gas_budget / (base_gas_price * multiplier + priority_fee)

where gas_budget is the amount of currency used for gas (after swaps are done if needed)
base_gas_price and priority_fee is obtained from EthereumChainTracking
the multiplier is defined through the Config
If any calculation causes an error (e.g. Gas price not set, gas price is 0, overflow etc.), a default gas_limit is used instead.
Currently, the Multiplier is set to 2 and the default gas limit is 400_000.

Added gas limit to CCM messages, calculated based on the current gas price Gas limit is passed into the ExecutexSwapAndCall call and used as gas limit.

…action built.

linear · 2023-09-04T04:06:30Z

PRO-799 Set correct CCM Gas Limit

albert said in PRO-161:

After some pondering here is how this should be implemented.

A CCM swap contains the principal and the gasBudget. The gasBudget is the amount of principal that is to be used for gas on the egress chain. That amount is swapped into egress chain's gas (e.g. ethereum) inside chainflip in parallel to the principal swap itself. CCM egressing works different than normal swaps - in CCM we make a separate call where the recipient can use all the gas in the transaction. This means that we need to cap the amount of gas the broadcaster is willing to use, otherwise we could be griefed. We currently hardcode the gaslimit to 400k (temporal patch) but we should cap it to the amount the user has paid for on the ingress, that is the gasBudget after being swapped into gas.

To ensure we don't spend more than what the user has paid I propose the following:

On the statechain, when crafting the transaction, set an appropiate maxFeePerGas according to the standard maxFeePerGas = 2*baseFee + priorityFee. This should use the Ethereum chain tracking values.

Calculate the maximum gas that can be spent at that gasPrice depending on the gas budget:

gasLimit = egressBudgetAmount / maxFeePerGas;

CFE will do the gas estimates with those values and will fail if the call consumes more gas than the gasLimit, which is correct.

I'm not familiar with the refund tracking to broadcasters, but we will probably need to set the maximum refund amount to egressBudgetAmount, since if someone else were to broadcast it with a lot more gas we don't want to refund more than that. This is out of scope for this task, as this is regarding the refunding logic.

Example:

User swaps 1000 DOT to FLIP specifying 100 DOT for gas.

Chainflip swaps 900 DOT to 900 FLIP and 100 DOT to 0.01 ETH.

According to chain tracking, BaseFee = 10Gwei, PrioFee = 1 Gwei.

maxFeePerGas = 2*10 + 1 = 21 Gwei

gasLimit = 0.01 ETH / 21 Gwei = 476k gas

We are willing to refund 0.01 ETH to anyone that broadcasts the transaction.

For normal swaps, on top of the protocol fee used to buy and burn FLIP, we will be charging and additional socialized fee for broadcasting the batches. In my mind, this solution is elegant enough that it's not necessary to add that here - the gasBudget will contribute to the payment of the transaction broadcasting, as the gasLimit itself contains the overhead of sending a transaction (21k) plus on-chain signature verification (~90k). In other words, that amount (0.01 ETH in the example) can go straight into the funds for paying validators.

The only thing worth mentioning with this approach is that the user will most likely be overpaying, as the calculating the gasLimit from the maxGasPerFee, which is significantly higher than the chain gasPrice. We can tweak that in the future if necessary (e.g. `maxFeePerGas = 1,5*baseFee + priorityFee) or consider allocating some of our funds for that to pay for gasPrice fluctuations. We might still need to charge a slightly higher fee somewhere, as CCM swaps are heavier on computation and might contain a large message, but that can be addressed in PRO-717.

daniel

* origin/main: Fix: Correct Select Median Implementation (#3934) fix: independent witnessing startup (#3913) 🍒 cherry-pick: changes in release for CI and chainspec (#3933) refactor: Re-arrange Chains traits for better composability (#3912) fix: log error when we try to transfer *more* than we have fetched (#3930) chore: add checks and increase timeout (#3928) Add `bind_redeem_address` to CLI (#3908) 🍒 cherry-pick: add missing prod dockerfiles (#3926) chore: skip localnet specific tests in devnet 🤫 (#3919) fix: broadcast success should be witnessable after a rotation (#3921) # Conflicts: # state-chain/chains/src/eth/api.rs # state-chain/runtime/src/chainflip.rs

codecov · 2023-09-04T04:56:03Z

Codecov Report

Merging #3935 (048c7a6) into main (74b6263) will increase coverage by 0%.
The diff coverage is 84%.

@@          Coverage Diff           @@
##            main   #3935    +/-   ##
======================================
  Coverage     72%     72%            
======================================
  Files        366     366            
  Lines      57384   57530   +146     
  Branches   57384   57530   +146     
======================================
+ Hits       41442   41577   +135     
- Misses     13851   13862    +11     
  Partials    2091    2091

Files Changed	Coverage Δ
engine/src/eth/retry_rpc.rs	`7% <0%> (-<1%)`	⬇️
engine/src/eth/rpc.rs	`0% <0%> (ø)`
state-chain/chains/src/btc/api.rs	`1% <0%> (-<1%)`	⬇️
state-chain/chains/src/dot/api.rs	`1% <0%> (-<1%)`	⬇️
state-chain/chains/src/evm.rs	`56% <ø> (ø)`
state-chain/chains/src/lib.rs	`40% <0%> (-3%)`	⬇️
state-chain/traits/src/lib.rs	`52% <0%> (ø)`
state-chain/runtime/src/chainflip.rs	`44% <76%> (+1%)`	⬆️
state-chain/traits/src/mocks/egress_handler.rs	`79% <80%> (-1%)`	⬇️
state-chain/cf-integration-tests/src/swapping.rs	`66% <100%> (+4%)`	⬆️
... and 9 more

... and 4 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

state-chain/pallets/cf-swapping/src/lib.rs

albert-llimos · 2023-09-04T12:44:18Z

I am currently testing this changes (running against #3887 test). So far it isn't working but I'm debugging, it might well be a test issue.
EDIT: After some debugging it looks like it might not be directly due to these logic changes but related to other logic. I will investigate further on my side.

Changed GasUnit type to u128 to avoid downcasting

dandanlen

I think we can simplify the PR a lot by focusing on the single use case at hand: ccm gas limits on Ethereum.

I appreciate the desire to generalise this to other chains, but right now I feel like this adds complexity with little benefit (see my comments).

state-chain/primitives/src/lib.rs

state-chain/chains/src/evm/api.rs

state-chain/runtime/src/chainflip.rs

albert-llimos · 2023-09-05T12:45:48Z

I think we can simplify the PR a lot by focusing on the single use case at hand: ccm gas limits on Ethereum.

I appreciate the desire to generalise this to other chains, but right now I feel like this adds complexity with little benefit (see my comments).

While it does make sense that we don't need any of that for BTC/DOT... I'd really push for this implementation to be reusable for other EVM chains (e.g. Arbitrum). Cross-chain messaging is a must have in any EVM chain we add. Not sure if this changes anything for this PR, just thought it is worth mentioning.

…ansaction, instead after CCM. Added an integration test to test the gas limit calculation logic

syan095 · 2023-09-06T05:12:06Z

@dandanlen @albert-llimos
I've made the following changes/refactors:

ExecutexSwapAndCall now contains the gas budget (it's either gas_budget, or a ccm_id that can be used to lookup the gas budget, I thought that using the gas budget directly is more generic/adaptable for other calls in the future).
This value is used when building a signed Transaction.
There's an optional function calculate_gas_limit dedicated to calculating gas_limit, and each chain can have its own implementation (i.e. different EVM chain can have their own ways of calculating this, while other chains can just ignore this).
Constants (such as the base gas fee multiplier and default gas_limit) are defined in calculate_gas_limit`

I think this satisfies both conditions:

Other calls in ethereum can easily implement gas limits - by passing in the gas budget when building the call, and adding itself to the match statement in the calculate_gas_limit function.
Other EVM chains can easily implement gas limits, just make use of the calculate_gas_limit function when building transactions, and chain-specific gas limit calculations can be written per chain.

Please take a look and let me know your thoughts

state-chain/chains/src/lib.rs

dandanlen · 2023-09-07T08:22:42Z

state-chain/pallets/cf-ingress-egress/src/lib.rs

@@ -868,25 +870,28 @@ impl<T: Config<I>, I: 'static> EgressApi<T::TargetChain> for Pallet<T, I> {
 		asset: TargetChainAsset<T, I>,
 		amount: TargetChainAmount<T, I>,
 		destination_address: TargetChainAccount<T, I>,
-		maybe_message: Option<CcmDepositMetadata>,
+		maybe_message: Option<(CcmDepositMetadata, TargetChainAmount<T, I>)>,


This variable will need to be renamed.

state-chain/runtime/src/chainflip.rs

* chore: mainnet version of ccm gaslimit test * chore: restore localnet * chore: rename * chore: fix lint * chore: check gasUsed * chore: working PoC * chore: small fix * chore: update to v0.8.0 * chore: commit full test * chore: some added swaps * chore: shuffle .run.sh order * chore: working test * chore: finalized gaslimit_ccm test * feat: changes to gaslimit logic * chore: lint * chore: fix test and cleanup * chore: trigger ci * chore: ci on push * chore: avoid clone * chore: fix tests * chore: try adding delay * chore: add delay at the end of gasLimit test * chore: cleanup test * chore: more cleanup --------- Co-authored-by: Daniel <daniel@chainflip.io>

AlastairHolmes · 2023-09-11T09:19:57Z

engine/src/eth/retry_rpc.rs

+								access_list: AccessList::default(),
+								from: Some(client.address()),
+								nonce: None,
+							});

 						let estimated_gas = client


I think it would be better here to change estimate_gas to take the Eip1559 instead of introduce the generic typed transaction here, and then needing the into(). @kylezs

The ethers-rs estimate_gas call takes a TypedTransaction, so we'd be putting into an EIP1559 type and then pulling it back out. I think this is ok. The signature's of the underlying rpc types ensure they're correct... ideally the ethers interface would allow for what you're suggesting though ofc

Misunderstood what Alastair was suggesting initially, I agree, adding the wrapping with Typed::EIP1559 inside estimate gas would be a bit nicer

I agree it would be a bit nicer but but the ethers api makes it a bit awkward.

~~Only thing there is that we then can't use the set_gas after the gas estimation, which is very clean. I don't have much of an opinion though.~~

dandanlen · 2023-09-11T10:01:12Z

.cargo/config.toml

@@ -1,6 +1,6 @@
 [env]
 CF_ETH_CONTRACT_ABI_ROOT = { value = "eth-contract-abis", relative = true }
-CF_ETH_CONTRACT_ABI_TAG = "perseverance-0.9-rc6"
+CF_ETH_CONTRACT_ABI_TAG = "v0.8.0"


This is a bit confusing... looks like we downgraded the contracts?

yes I understand, but any follow-up tags can't be named perseverance-0.9-xyz as perseverance contracts have been deployed and will not changed. That would be even more confusing/wrong. So the best option to me seems going back to the normal tags and we launch mainnet with v1.0.0.

What about 0.10.0?

dandanlen · 2023-09-11T11:17:47Z

Any objections to merging this as-is without changing the rpc api? (@kylezs @AlastairHolmes )

AlastairHolmes · 2023-09-11T13:16:11Z

Any objections to merging this as-is without changing the rpc api? (@kylezs @AlastairHolmes )

I'll do it, it'll take 3 minutes

linear · 2023-09-12T00:16:18Z

PRO-797 Use integrity test in TestRunner

Right now, for example, if you use the helper methods like execute_at_next_block, there's no good way to add a consistency check like in the validator pallet tests.

There is a hook, IntegrityTest

See:

We already have the other hooks. It should be possible to use this too. This would allow us to always test pallet invariants in every test run.

AlastairHolmes · 2023-09-14T13:55:07Z

Any objections to merging this as-is without changing the rpc api? (@kylezs @AlastairHolmes )

FYI I have added the change already

Co-authored-by: Daniel <daniel@chainflip.io> Co-authored-by: Albert Llimos <53186777+albert-llimos@users.noreply.github.com> Co-authored-by: albert <allimos3@gmail.com> Co-authored-by: Alastair Holmes <holmes.alastair@outlook.com>

syan095 added 2 commits September 1, 2023 17:43

WIP:

7db71ed

Added gas limit to CCM messages, calculated based on the current gas price Gas limit is passed into the ExecutexSwapAndCall call and used as gas limit.

Connected the calculated gas limit for CCM messages to Ethereum Trans…

f2cc9cb

…action built.

syan095 requested review from dandanlen and albert-llimos September 4, 2023 04:06

minor improvement to comments

9e079b1

albert-llimos reviewed Sep 4, 2023

View reviewed changes

state-chain/pallets/cf-swapping/src/lib.rs Outdated Show resolved Hide resolved

syan095 added 2 commits September 5, 2023 12:54

Removed the default gas limit for CCM messages.

68ad01a

Changed GasUnit type to u128 to avoid downcasting

Fixed minor typo

9e32512

syan095 requested a review from martin-chainflip as a code owner September 5, 2023 03:15

dandanlen reviewed Sep 5, 2023

View reviewed changes

state-chain/primitives/src/lib.rs Outdated Show resolved Hide resolved

state-chain/chains/src/evm/api.rs Outdated Show resolved Hide resolved

state-chain/runtime/src/chainflip.rs Outdated Show resolved Hide resolved

Refactored so that the gas limit calculation is done when building Tr…

9a8fdae

…ansaction, instead after CCM. Added an integration test to test the gas limit calculation logic

Merged latest from main.

779563a

dandanlen reviewed Sep 7, 2023

View reviewed changes

albert-llimos requested a review from a team as a code owner September 11, 2023 07:44

albert-llimos requested review from ahasna and tomjohnburton and removed request for a team, ahasna and tomjohnburton September 11, 2023 07:44

albert-llimos added 2 commits September 11, 2023 09:56

chore: merge from main

cab939c

chore: add into() and small refactoring improvement

af9a0fb

AlastairHolmes reviewed Sep 11, 2023

View reviewed changes

chore: add random gas consumption tests

a61d85b

fix: more generic gas limit, avoid panics

8da0758

dandanlen reviewed Sep 11, 2023

View reviewed changes

AlastairHolmes and others added 2 commits September 11, 2023 15:25

directly use Eip1559TransactionRequest

6941f3c

Renamed a parameter

f59c32e

dandanlen approved these changes Sep 14, 2023

View reviewed changes

Merge branch 'main' into feature/ccm-gas-limit

048c7a6

dandanlen changed the title ~~Feature: Calculate ccm gas limit~~ feat: Calculate ccm gas limit Sep 14, 2023

dandanlen enabled auto-merge (squash) September 14, 2023 15:21

dandanlen merged commit 4b2d251 into main Sep 14, 2023
44 checks passed

dandanlen deleted the feature/ccm-gas-limit branch September 14, 2023 15:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Calculate ccm gas limit #3935

feat: Calculate ccm gas limit #3935

syan095 commented Sep 4, 2023 •

edited

Loading

linear bot commented Sep 4, 2023

codecov bot commented Sep 4, 2023 •

edited

Loading

albert-llimos commented Sep 4, 2023 •

edited

Loading

dandanlen left a comment

albert-llimos commented Sep 5, 2023

syan095 commented Sep 6, 2023

dandanlen Sep 7, 2023

AlastairHolmes Sep 11, 2023 •

edited

Loading

kylezs Sep 11, 2023 •

edited

Loading

dandanlen Sep 11, 2023

albert-llimos Sep 11, 2023 •

edited

Loading

dandanlen Sep 11, 2023

albert-llimos Sep 11, 2023

dandanlen Sep 12, 2023 •

edited

Loading

dandanlen commented Sep 11, 2023

AlastairHolmes commented Sep 11, 2023 •

edited

Loading

linear bot commented Sep 12, 2023

AlastairHolmes commented Sep 14, 2023

feat: Calculate ccm gas limit #3935

feat: Calculate ccm gas limit #3935

Conversation

syan095 commented Sep 4, 2023 • edited Loading

Pull Request

Checklist

Summary

linear bot commented Sep 4, 2023

codecov bot commented Sep 4, 2023 • edited Loading

Codecov Report

albert-llimos commented Sep 4, 2023 • edited Loading

dandanlen left a comment

Choose a reason for hiding this comment

albert-llimos commented Sep 5, 2023

syan095 commented Sep 6, 2023

dandanlen Sep 7, 2023

Choose a reason for hiding this comment

AlastairHolmes Sep 11, 2023 • edited Loading

Choose a reason for hiding this comment

kylezs Sep 11, 2023 • edited Loading

Choose a reason for hiding this comment

dandanlen Sep 11, 2023

Choose a reason for hiding this comment

albert-llimos Sep 11, 2023 • edited Loading

Choose a reason for hiding this comment

dandanlen Sep 11, 2023

Choose a reason for hiding this comment

albert-llimos Sep 11, 2023

Choose a reason for hiding this comment

dandanlen Sep 12, 2023 • edited Loading

Choose a reason for hiding this comment

dandanlen commented Sep 11, 2023

AlastairHolmes commented Sep 11, 2023 • edited Loading

linear bot commented Sep 12, 2023

AlastairHolmes commented Sep 14, 2023

syan095 commented Sep 4, 2023 •

edited

Loading

codecov bot commented Sep 4, 2023 •

edited

Loading

albert-llimos commented Sep 4, 2023 •

edited

Loading

AlastairHolmes Sep 11, 2023 •

edited

Loading

kylezs Sep 11, 2023 •

edited

Loading

albert-llimos Sep 11, 2023 •

edited

Loading

dandanlen Sep 12, 2023 •

edited

Loading

AlastairHolmes commented Sep 11, 2023 •

edited

Loading