Skip to content

Update Substrate#1347

Merged
nazar-pc merged 7 commits into
mainfrom
update-substrate
Apr 3, 2023
Merged

Update Substrate#1347
nazar-pc merged 7 commits into
mainfrom
update-substrate

Conversation

@nazar-pc

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

Copy link
Copy Markdown
Member

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:

nazar-pc added 2 commits April 3, 2023 01:04
…834c7fb` (and ORML vesting to `0085382cf001704d084ac8e97b7bc3e4e8afb8a7`) and apply minimal-ish changes to make things compile and pass tests
@nazar-pc

nazar-pc commented Apr 2, 2023

Copy link
Copy Markdown
Member Author

ORML docs didn't pass tests

@nazar-pc

nazar-pc commented Apr 3, 2023

Copy link
Copy Markdown
Member Author

@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
Copy Markdown
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
Copy Markdown
Contributor

I had no local issues with tests.

Update dependencies, remove now unnecessary override for `chacha20poly1305`
@nazar-pc

nazar-pc commented Apr 3, 2023

Copy link
Copy Markdown
Member Author

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.

i1i1
i1i1 previously approved these changes Apr 3, 2023
Comment thread crates/subspace-fraud-proof/src/invalid_state_transition_proof.rs
@nazar-pc

nazar-pc commented Apr 3, 2023

Copy link
Copy Markdown
Member Author

Pulled in paritytech/substrate#13804

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

Copy link
Copy Markdown
Contributor

Worked for me locally as well for eth relay domain.

@nazar-pc

nazar-pc commented Apr 3, 2023

Copy link
Copy Markdown
Member Author

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?

i1i1
i1i1 previously approved these changes Apr 3, 2023
Comment thread crates/subspace-fraud-proof/src/invalid_state_transition_proof.rs
vedhavyas
vedhavyas previously approved these changes Apr 3, 2023

@vedhavyas vedhavyas left a comment

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
Contributor

@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

nazar-pc commented Apr 3, 2023

Copy link
Copy Markdown
Member Author

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.

@rahulksnv rahulksnv left a comment

Copy link
Copy Markdown
Contributor

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