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

Update Substrate #1347

Merged
merged 7 commits into from
Apr 3, 2023
Merged

Update Substrate #1347

merged 7 commits into from
Apr 3, 2023

Conversation

nazar-pc
Copy link
Member

@nazar-pc nazar-pc commented Apr 2, 2023

Upstream changes I found relevant/important/interesting:

With this release LLVM version shouldn't matter anymore on macOS or other platforms.

A lot of moving parts have changed because of above PRs, I had to re-apply our Substrate tweaks on top of upstream and created https://github.com/subspace/substrate/tree/subspace-v4 branch for that. Changes were caused by networking refactoring, but after applying tweaks one by one it compiled and should work fine (I'm successfully syncing with this branch onto Gemini 3d).

Follow-ups required for:

Code contributor checklist:

…834c7fb` (and ORML vesting to `0085382cf001704d084ac8e97b7bc3e4e8afb8a7`) and apply minimal-ish changes to make things compile and pass tests
@nazar-pc
Copy link
Member Author

nazar-pc commented Apr 2, 2023

ORML docs didn't pass tests

@nazar-pc
Copy link
Member Author

nazar-pc commented Apr 3, 2023

@liuchengxu @NingLin-P test failed with:

thread 'tests::test_executor_full_node_catching_up' panicked at 'The test took too long!', domains/client/domain-executor/src/tests.rs:28:1

https://github.com/subspace/subspace/actions/runs/4591054185/jobs/8106976976?pr=1347

Works for me locally though, must be performance sensitive?

@liuchengxu
Copy link
Contributor

Works for me locally as well. It looks more like an interesting glitch somewhere, with some debugging in CI, I found that the ack of slot notification somehow never resolves, https://github.com/subspace/subspace/blob/0043ed4726/test/subspace-test-service/src/mock.rs#L182-L184, still working on it.

@shamil-gadelshin
Copy link
Member

I had no local issues with tests.

Update dependencies, remove now unnecessary override for `chacha20poly1305`
@nazar-pc
Copy link
Member Author

nazar-pc commented Apr 3, 2023

CI in #1348 (built on top of this) passed for some reason. Let's see if it happens again and if so we can merge it.

@nazar-pc
Copy link
Member Author

nazar-pc commented Apr 3, 2023

Pulled in paritytech/substrate#13804

@nazar-pc nazar-pc requested a review from i1i1 April 3, 2023 09:12
@ParthDesai
Copy link
Contributor

Worked for me locally as well for eth relay domain.

@nazar-pc
Copy link
Member Author

nazar-pc commented Apr 3, 2023

tests::invalid_execution_proof_should_not_work hangs for me locally sometimes when running with cargo nextest run, but not when running it individually.

Should I ignore it for now so we can make progress here?

vedhavyas
vedhavyas previously approved these changes Apr 3, 2023
Copy link
Contributor

@vedhavyas vedhavyas left a comment

Choose a reason for hiding this comment

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

Nice work!

I think it is totally OK to skip the failing tests and let the Execution team handle it. If you can create an issue, would be great.

@liuchengxu
Copy link
Contributor

@nazar-pc Let's ignore the failing test for now. I reproduced it locally with a busy machine but haven't got a clue.

@NingLin-P It can be reproduced with busy CPUs. For example, run openssl speed -multi 24 rsa, and then run the test indivisually. You may need to have multiple runs as it sometimes passes even with 100% CPU usage on my machine.

liuchengxu
liuchengxu previously approved these changes Apr 3, 2023
@NingLin-P
Copy link
Member

@NingLin-P It can be reproduced with busy CPUs. For example, run openssl speed -multi 24 rsa, and then run the test indivisually. You may need to have multiple runs as it sometimes passes even with 100% CPU usage on my machine.

Thanks for the clue! I will look into it.

@nazar-pc
Copy link
Member Author

nazar-pc commented Apr 3, 2023

Now test_executor_full_node_catching_up timed out, which is annoying. I guess tokio wrapper has some timeout for tests that we might need to increase. Will try to ignore this test too, but this is a really slow dev cycle to run it through CI every time.

Though judging by #1345 (comment) it just gets stuck and increasing timeout will not help.

Copy link
Contributor

@rahulksnv rahulksnv left a comment

Choose a reason for hiding this comment

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

Thanks. The chain sync changes appear to be mostly code moved around, I will resync autonomys/substrate#46 after this is merged

@nazar-pc nazar-pc merged commit 165c163 into main Apr 3, 2023
@nazar-pc nazar-pc deleted the update-substrate branch April 3, 2023 15:31
i1i1 added a commit to autonomys/subspace-pulsar-sdk that referenced this pull request Apr 4, 2023
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.

8 participants