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.

Sign up for GitHub

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

Jump to bottom

Substrate Upgrade to V30 #627

Closed
wants to merge 2 commits into from
Closed

Substrate Upgrade to V30 #627

wants to merge 2 commits into from

Conversation

markopoloparadox
Copy link
Contributor

TODO

justinfrevert
justinfrevert reviewed Jun 13, 2023
View reviewed changes
client/Cargo.toml
sp-transaction-storage-proof = { git = "https://github.com/futureversecom/substrate", branch = "polkadot-v0.9.27" }
frame-system = { git = "https://github.com/futureversecom/substrate", branch = "polkadot-v0.9.27" }
pallet-transaction-payment = { git = "https://github.com/futureversecom/substrate" , branch = "polkadot-v0.9.27" }
sc-cli = { git = "https://github.com/paritytech/substrate", features = ["wasmtime"] , branch = "polkadot-v0.9.30" }
Copy link
Contributor

Choose a reason for hiding this comment

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

Why switch from the fork?

justinfrevert
justinfrevert reviewed Jun 13, 2023
View reviewed changes
client/Cargo.toml
fp-consensus = { default-features = false, git = "https://github.com/futureversecom/frontier", branch = "polkadot-v0.9.27-TRN" }
fp-rpc = { default-features = false, git = "https://github.com/futureversecom/frontier", branch = "polkadot-v0.9.27-TRN" }
fp-storage = { default-features = false, git = "https://github.com/futureversecom/frontier", branch = "polkadot-v0.9.27-TRN" }
fc-consensus = { default-features = false, git = "https://github.com/futureversecom/frontier", branch = "polkadot-v0.9.30-TRN" }
Copy link
Contributor

Choose a reason for hiding this comment

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

I noted that the Frontier fork also isn't pointing at the Futureverse Substrate fork

Copy link
Contributor

@surangap surangap Jun 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

do we have custom changes only on the frontier side?
noticed the change from futureversecom/substrate to paritytech/substrate

justinfrevert
justinfrevert suggested changes Jun 13, 2023
View reviewed changes
Copy link
Contributor

@justinfrevert justinfrevert left a comment

Choose a reason for hiding this comment

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

It's mostly inconsequential changes, but I think we need to keep the fork. It's likely we'll use that in the very near future, and using the upstream repo will just cause headaches if we have to switch back

justinfrevert
justinfrevert approved these changes Jun 13, 2023
View reviewed changes
Copy link
Contributor

@justinfrevert justinfrevert left a comment

Choose a reason for hiding this comment

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

Approved on the understanding we can go to v40 right after this on the fork

@ken-futureverse
Copy link
Contributor

@justinfrevert i'm adding do not merge label on this as there is pending discussion on implementing a feature that needs fast-tracking to mainnet

@KarishmaBothara KarishmaBothara mentioned this pull request Jul 4, 2023
Closed
surangap
surangap reviewed Jul 17, 2023
View reviewed changes
Copy link
Contributor

@surangap surangap left a comment

Choose a reason for hiding this comment

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

Extra gas RCA
frontier implements original_storage with this PR -> polkadot-evm/frontier#871
and it creates a different gas cost for opcode sstore. our current version returns gas_sload and new version returns gas_sstore_reset. The diffrence between these two which is 2900 -100 = 2800 exactly matches with the increased amount for TestCall.

the new version seems the right way to go according to the spec docs -> https://ethereum.github.io/execution-specs/autoapi/ethereum/homestead/vm/instructions/storage/index.html#sstore and makes more sense in general.

We should proceed with the increased gas figures.

@ken-futureverse
Copy link
Contributor

excellent finding @surangap, question, in TestCall there are several failed tests, but do they all increase at the exact amount 2800?

@surangap
Copy link
Contributor

excellent finding @surangap, question, in TestCall there are several failed tests, but do they all increase at the exact amount 2800?

v30
43702
    ✔ TestCall:set() estimates and uses ~43_000-46_000 gas (4057ms)
32157
    ✔ TestCallProxy:set() estimates and uses ~28_000-33_000 gas (4057ms)
30701
    1) TestCallProxy:setWithAddress() estimates and uses ~27_000-30_000 gas


current
43702
    ✔ TestCall:set() estimates and uses ~43_000-46_000 gas (4049ms)
29357
    ✔ TestCallProxy:set() estimates and uses ~28_000-33_000 gas (4049ms)
27901
    ✔ TestCallProxy:setWithAddress() estimates and uses ~27_000-30_000 gas (4050ms)

This is used gas figures for TestCall between v30 and current

@surangap
Copy link
Contributor

closing this PR. active PR -> #660

@surangap surangap closed this Jul 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@surangap surangap surangap left review comments

@justinfrevert justinfrevert justinfrevert approved these changes

Assignees
No one assigned
Labels
ops:client-change ops:do-not-merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@markopoloparadox @ken-futureverse @surangap @justinfrevert @KarishmaBothara