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

Move execution parameter to separate account - V0.37 #6891

Merged

Conversation

janezpodhostnik
Copy link
Contributor

@janezpodhostnik janezpodhostnik commented Jan 15, 2025

work towards: #6507

Read the metering settings from a different account.

Whenever unrelated data would change on the service account, the execution parameters would get evicted from the cache (and since they are a dependency, all programs would also get evicted).

Moving execution parameters to its own dedicated account is a solution for this.

This is a short-medium term solution. The proper solution would be to put the execution parameters into the dynamic protocol state. Which would simplify the programs cache logic and remove the need for the execution parameters to be on a special account.

The newly create account will have the metering settings in storage. They will be copied from the service account with the following transaction:

import "FlowServiceAccount"
				
transaction() {
	prepare(account: auth(Storage) &Account) {

		account.storage.load<{UInt64: UInt64}>(from: /storage/executionEffortWeights)
		account.storage.save(FlowServiceAccount.getExecutionEffortWeights(), to: /storage/executionEffortWeights)

		account.storage.load<UInt64>(from: /storage/executionMemoryLimit)
		account.storage.save(FlowServiceAccount.getExecutionMemoryLimit(), to: /storage/executionMemoryLimit)

                // ExecutionMemoryWeight are not set, so this is skipped because getExecutionMemoryWeights() panics
		// account.storage.load<{UInt64: UInt64}>(from: /storage/executionMemoryWeights)
		// account.storage.save(FlowServiceAccount.getExecutionMemoryWeights(), to: /storage/executionMemoryWeights)

	}
}

This is sufficient for FVM to start consuming this data. For users there are additional steps needed, that are described here: #6894

TODO:

  • create account on mainnet
  • create account on testnet
  • port to master

@janezpodhostnik janezpodhostnik self-assigned this Jan 15, 2025
@j1010001 j1010001 requested a deployment to Production Docker Registry January 15, 2025 18:41 — with GitHub Actions Waiting
@janezpodhostnik janezpodhostnik changed the title move execution parameter to separate account Move execution parameter to separate account - V0.37 Jan 15, 2025
@janezpodhostnik janezpodhostnik marked this pull request as ready for review January 15, 2025 21:10
Comment on lines 107 to 110
// executionParametersAddressTestnet is the address of the Execution Parameters contract on Testnet
executionParametersAddressTestnet = flow.HexToAddress("00000000000")
// executionParametersAddressMainnet is the address of the Execution Parameters contract on Mainnet
executionParametersAddressMainnet = flow.HexToAddress("00000000000")
Copy link
Member

@zhangchiqing zhangchiqing Jan 15, 2025

Choose a reason for hiding this comment

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

why are they 000s?

if becaseu we haven't created them, then better add a TODO here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, we haven't added them yet.
That's not a valid address so its going to break if anyone tries to use it.

I plan to add the testnet address tomorrow, I'll add a todo for the mainnet one

Base automatically changed from janez/v0.37-execution-version-from-snapshot to v0.37 January 16, 2025 10:09
@codecov-commenter
Copy link

codecov-commenter commented Jan 16, 2025

Codecov Report

Attention: Patch coverage is 27.36842% with 69 lines in your changes missing coverage. Please review.

Project coverage is 41.48%. Comparing base (ad39920) to head (25b9987).
Report is 2 commits behind head on v0.37.

Files with missing lines Patch % Lines
state/protocol/mock/snapshot_execution_subset.go 0.00% 50 Missing ⚠️
...rotocol/mock/snapshot_execution_subset_provider.go 0.00% 19 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##            v0.37    #6891      +/-   ##
==========================================
- Coverage   41.51%   41.48%   -0.03%     
==========================================
  Files        2033     2035       +2     
  Lines      181521   181604      +83     
==========================================
- Hits        75360    75342      -18     
- Misses      99926   100019      +93     
- Partials     6235     6243       +8     
Flag Coverage Δ
unittests 41.48% <27.36%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@joshuahannan joshuahannan left a comment

Choose a reason for hiding this comment

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

Are you planning on making a PR to flow-core-contracts to add the other contract or do you need help with that?

fvm/systemcontracts/system_contracts.go Outdated Show resolved Hide resolved
@janezpodhostnik
Copy link
Contributor Author

@janezpodhostnik janezpodhostnik merged commit 4a197c8 into v0.37 Jan 21, 2025
55 checks passed
@janezpodhostnik janezpodhostnik deleted the janez/v0.37-execution-parameters-from-different-account branch January 21, 2025 18:32
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

Successfully merging this pull request may close these issues.

6 participants