Skip to content

Update frontier to address pov underestimations #3227

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

Merged
merged 25 commits into from
Apr 9, 2025

Conversation

TarekkMA
Copy link
Contributor

@TarekkMA TarekkMA commented Mar 25, 2025

What does it do?

Update frontier to address pov underestimations

What important points should reviewers know?

Is there something left for follow-up PRs?

What alternative implementations were considered?

Are there relevant PRs or issues in other repositories (Substrate, Polkadot, Frontier, Cumulus)?

What value does it bring to the blockchain users?

⚠️ Breaking Changes ⚠️

gas cost has been increased.

@TarekkMA TarekkMA added B5-clientnoteworthy Changes should be mentioned in any downstream projects' release notes B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes and removed B5-clientnoteworthy Changes should be mentioned in any downstream projects' release notes labels Mar 25, 2025
Copy link
Contributor

github-actions bot commented Mar 25, 2025

WASM runtime size check:

Compared to target branch

Moonbase runtime: 2424 KB (+4 KB) 🚨

Moonbeam runtime: 2408 KB (+4 KB) 🚨

Moonriver runtime: 2396 KB (no changes) ✅

Compared to latest release (runtime-3600)

Moonbase runtime: 2424 KB (+468 KB compared to latest release) 🚨

Moonbeam runtime: 2408 KB (+464 KB compared to latest release) 🚨

Moonriver runtime: 2396 KB (+456 KB compared to latest release) ⚠️

Copy link
Contributor

github-actions bot commented Mar 25, 2025

Coverage Report

@@                           Coverage Diff                           @@
##           master   tarekkma/update-frontier-and-fix-pov     +/-   ##
=======================================================================
  Coverage   74.20%                                 74.20%   0.00%     
  Files         383                                    383             
+ Lines       97215                                  97217      +2     
=======================================================================
+ Hits        72133                                  72134      +1     
+ Misses      25082                                  25083      +1     
Files Changed Coverage
/runtime/common/src/apis.rs 28.97% (+0.04%) 🔼

Coverage generated Wed Apr 9 15:31:18 UTC 2025

Copy link
Contributor

@RomarQ RomarQ left a comment

Choose a reason for hiding this comment

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

We should revert the change to the estimated_transaction_len. tried it locally and the value of 258 is correct.

TarekkMA added 13 commits March 25, 2025 12:05
# Conflicts:
#	Cargo.lock
#	test/suites/dev/moonbase/test-fees/test-length-fees2.ts
#	test/suites/dev/moonbase/test-precompile/test-precompile-assets-erc20d.ts
#	test/suites/dev/moonbase/test-precompile/test-precompile-proxy.ts
#	test/suites/dev/moonbase/test-randomness/test-randomness-babe-lottery2.ts
#	test/suites/dev/moonbase/test-randomness/test-randomness-babe-lottery3.ts
#	test/suites/dev/moonbase/test-randomness/test-randomness-vrf-lottery4.ts
#	test/suites/dev/moonbase/test-randomness/test-randomness-vrf-lottery5.ts
#	test/suites/dev/moonbase/test-storage-growth/test-precompile-storage-growth.ts
This reverts commit c364169.
@TarekkMA TarekkMA added breaking Needs to be mentioned in breaking changes D5-nicetohaveaudit⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Mar 28, 2025
@RomarQ RomarQ requested a review from Copilot April 7, 2025 14:14
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (4)

test/suites/dev/moonbase/test-xcm-v3/test-mock-hrmp-transact-ethereum-12.ts:89

  • Verify that the updated multiplier (26000n) correctly reflects the intended gas cost increase and update the inline comment to clarify the rationale behind this change.
const targetXcmWeight = GAS_LIMIT * 26000n + STORAGE_READ_COST + 4_250_000_000n;

test/suites/dev/moonbase/test-storage-growth/test-precompile-storage-growth.ts:40

  • Confirm that the updated inline snapshot value (51888n) for estimated gas is consistent with the new gas cost parameters and that it accurately tests the updated logic.
expect(estimatedGas).toMatchInlineSnapshot(`51888n`);

test/suites/dev/moonbase/test-parameters/test-parameters-runtime-FeesTreasuryProportion.ts:179

  • [nitpick] The revised fee calculation logic now uses a min function and computes tip fees differently; consider adding an inline comment to explain the rationale and handling of edge cases, ensuring the behavior aligns with expected fee structures.
const max_fee_per_gas = GENESIS_BASE_FEE;

test/helpers/block.ts:214

  • [nitpick] The conditional logic adjusting baseFeesPaid and tipAsFeesPaid may not clearly cover all edge cases; consider refactoring or adding documentation to enhance clarity and ensure correctness.
if (actualPaidFees < baseFeesPaid + tipAsFeesPaid) {

@RomarQ RomarQ merged commit 18f7428 into master Apr 9, 2025
37 checks passed
@RomarQ RomarQ deleted the tarekkma/update-frontier-and-fix-pov branch April 9, 2025 20:41
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 D5-nicetohaveaudit⚠️ PR contains trivial changes to logic that should be properly reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants