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

[frontier] Add support for PoV gas refunding #3036

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Agusrodri
Copy link
Contributor

No description provided.

@@ -88,7 +90,7 @@ describeSuite({
],
privateKey: BALTATHAR_PRIVATE_KEY,
rawTxOnly: true,
gas: expectedGas,
gas: 91_088n,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: this call fails with OOG if we use a gas_limit lower than this number (which is the entire pov_gas without refunding), but the actual gas_used is the expected one (54_168n) and it's correct.

@RomarQ @ahmadkaouk any clue of what could be happening? I guess that the remaining gas should be refunded to the user after the execution, but not sure why we must indicate 91_088n here. Instead, a gas limit of 54_168n should be enough to cover all the transaction cost.

Copy link
Contributor Author

@Agusrodri Agusrodri Nov 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the end this is expected, as we still count for the worst case scenario at the moment of estimating.

Copy link
Contributor

github-actions bot commented Nov 8, 2024

WASM runtime size check:

Compared to target branch

Moonbase runtime: 2260 KB (no changes) ✅

Moonbeam runtime: 2232 KB (no changes) ✅

Moonriver runtime: 2228 KB (no changes) ✅

Compared to latest release (runtime-3300)

Moonbase runtime: 2260 KB (+232 KB compared to latest release) ⚠️

Moonbeam runtime: 2232 KB (+236 KB compared to latest release) ⚠️

Moonriver runtime: 2228 KB (+236 KB compared to latest release) ⚠️

Copy link
Contributor

github-actions bot commented Nov 8, 2024

Coverage Report

@@                       Coverage Diff                       @@
##           master   agustin-frontier-pov-reclaim     +/-   ##
===============================================================
  Coverage   79.00%                         79.00%   0.00%     
  Files         303                            303             
  Lines       88208                          88208             
===============================================================
  Hits        69685                          69685             
  Misses      18523                          18523             
Files Changed Coverage

Coverage generated Wed Nov 20 10:15:55 UTC 2024

@noandrea noandrea added the A8-mergeoncegreen Pull request is reviewed well. label Nov 15, 2024
@crystalin crystalin removed the A8-mergeoncegreen Pull request is reviewed well. label Nov 18, 2024
@RomarQ RomarQ added B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes breaking Needs to be mentioned in breaking changes labels Nov 20, 2024
@crystalin
Copy link
Collaborator

I tested a live transaction (with lazy fork) from BeamSwap using this PR.
The cost goes from 0.48 GLMR to 0.041 GLMR !!!
(current runtime LEFT), (this PR RIGHT)
Capture d'écran 2024-11-20 134217

@crystalin
Copy link
Collaborator

@Agusrodri I can see the ref_time of weight (v2) jumps from 93M to 7M. Is that normal ?

@Agusrodri
Copy link
Contributor Author

@Agusrodri I can see the ref_time of weight (v2) jumps from 93M to 7M. Is that normal ?

Not sure if this is normal to be honest. From my understanding the refund should only take place in the pov size right? Will need to investigate.

@crystalin
Copy link
Collaborator

After more consideration, it is expected to also be lower. For Ethereum transaction, the weight (ref_time) is computed from the gas cost, using a gas to weight ratio. The gas being lower (due to PoV) the ref_time is lower being lower. So all good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes breaking Needs to be mentioned in breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants