Skip to content

Substrate Upgrade to V30 #627

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
Closed
markopoloparadox wants to merge 2 commits into from
Closed

Substrate Upgrade to V30 #627

markopoloparadox wants to merge 2 commits into from

Conversation

@markopoloparadox
Copy link
Contributor

@markopoloparadox markopoloparadox commented Jun 13, 2023

TODO

@markopoloparadox markopoloparadox force-pushed the feat/upgrade-substrate-to-v30 branch from cff387c to dd5d4b5 Compare June 13, 2023 12:37
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

@justinfrevert justinfrevert Jun 13, 2023

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

@justinfrevert justinfrevert Jun 13, 2023

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

@markopoloparadox markopoloparadox force-pushed the feat/upgrade-substrate-to-v30 branch from 59320ae to a5e1907 Compare June 13, 2023 23:52
@ken-futureverse
Copy link
Contributor

ken-futureverse commented Jun 20, 2023

@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
@KarishmaBothara KarishmaBothara force-pushed the feat/upgrade-substrate-to-v30 branch from a5e1907 to 5cff571 Compare July 4, 2023 10:15
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

ken-futureverse commented Jul 17, 2023

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

surangap commented Jul 17, 2023

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

surangap commented Jul 31, 2023

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

2 more reviewers

@surangap surangap surangap left review comments

@justinfrevert justinfrevert justinfrevert approved these changes

Reviewers whose approvals may not affect merge requirements

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.

6 participants

@markopoloparadox @ken-futureverse @surangap @justinfrevert @KarishmaBothara