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

Upgrade Substrate to stable2409 #3078

Merged
merged 8 commits into from
Oct 2, 2024
Merged

Upgrade Substrate to stable2409 #3078

merged 8 commits into from
Oct 2, 2024

Conversation

nazar-pc
Copy link
Member

This upgrades Substrate to stable2409 upstream release.

A lot of changes are caused by my refactoring of syncing implementation. I didn't take advantage of it yet to reduce the diff.

I have rebased most of the patches we had that are not yet upstreamed, notable exception is Ved's early runtime patch for state version and some of Shamil's patches for Snap sync that are no longer needed (and I have simplified the remaining since they re-exported more than needed with latest upstream refactoring).

On top of stable release I have also backported following PRs that were already merged into master or remain open, but have a good chance of getting in soon-ish:

All of this is in https://github.com/autonomys/polkadot-sdk/tree/subspace-v8, top commit of which is used in this PR.

Frontier has unmerged polkadot-evm/frontier#1504, which I cloned into https://github.com/autonomys/frontier/tree/subspace-v9 and used here.

I recommend ignoring .toml files during review due to trivial changes (mostly just hash updates and minor dependency changes where necessary).

Code contributor checklist:

@nazar-pc
Copy link
Member Author

Removed custom build_network in domains, https://github.com/autonomys/subspace/pull/2999/files#diff-fdc91ceff50eb11bf372dc47250c4ca95059123cc1d381ec5c867e9fa447244c seems to have effectively restored original version of it

@nazar-pc nazar-pc marked this pull request as ready for review September 30, 2024 18:26
@teor2345
Copy link
Contributor

This seems odd and likely not related to this PR?

You have not agreed to the Xcode license. Please resolve this by running:
sudo xcodebuild -license accept

Both macOS jobs are impacted:
https://github.com/autonomys/subspace/actions/runs/11111943805/job/30872989507?pr=3078#step:6:15
https://github.com/autonomys/subspace/actions/runs/11111943805/job/30872990561?pr=3078#step:6:15

@nazar-pc
Copy link
Member Author

@DaMandal0rian I think this is something for you to fix

Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Looks good to me, but I don’t have the background to review some of it in detail.

I had one question about task handling, but it’s not a blocker.

crates/sc-consensus-subspace/src/verifier.rs Show resolved Hide resolved
@teor2345
Copy link
Contributor

teor2345 commented Oct 1, 2024

This seems odd and likely not related to this PR?

You have not agreed to the Xcode license. Please resolve this by running:
sudo xcodebuild -license accept

Both macOS jobs are impacted: autonomys/subspace/actions/runs/11111943805/job/30872989507?pr=3078#step:6:15 autonomys/subspace/actions/runs/11111943805/job/30872990561?pr=3078#step:6:15

Interesting, other PRs passed when I re-ran the tests, maybe it was a temporary issue, or fixed by #3079:
#3079 (review)

@DaMandal0rian
Copy link
Contributor

@nazar-pc @teor2345 the macOS error is fixed already, as I agreed to the license. It was detected when running #3079 checks.

# Conflicts:
#	crates/pallet-offences-subspace/Cargo.toml
#	crates/pallet-subspace/Cargo.toml
#	crates/sc-consensus-subspace/src/verifier.rs
#	crates/subspace-runtime/Cargo.toml
#	test/subspace-test-runtime/Cargo.toml
@nazar-pc
Copy link
Member Author

nazar-pc commented Oct 1, 2024

Merge only had minor Cargo.toml and crates/sc-consensus-subspace/src/verifier.rs conflicts, not sure why it shows so much stuff has changed 🤷

tx_handler_controller,
network_starter,
sync_service,
_block_downloader,
Copy link
Member

Choose a reason for hiding this comment

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

Block downloader will be required in the domain snap-sync.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is still there, just inside build_network. I have no idea why it was surfaced here, it is actually build_network's argument 😕

Copy link
Member

Choose a reason for hiding this comment

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

I meant - domain snap sync requires that variable.

Copy link
Member

@shamil-gadelshin shamil-gadelshin Oct 1, 2024

Choose a reason for hiding this comment

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

This is how we request last confirmed domain block.

Copy link
Member Author

Choose a reason for hiding this comment

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

Then you'll simply have to construct default block downloader explicitly hand it over as an argument here. There is a public function in sc-service now that makes it more convenient to do.

Copy link
Member

Choose a reason for hiding this comment

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

Can you give me more hints or show me the correct function?

Copy link
Member Author

Choose a reason for hiding this comment

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

build_default_block_downloader

Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Merge looks good, even if large

@nazar-pc nazar-pc added this pull request to the merge queue Oct 2, 2024
Merged via the queue into main with commit 9592c9a Oct 2, 2024
11 checks passed
@nazar-pc nazar-pc deleted the upgrade-substrate branch October 2, 2024 03:25
@nazar-pc nazar-pc restored the upgrade-substrate branch October 2, 2024 17:11
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.

4 participants