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

test: reenable test_state_root_task test #13911

Merged
merged 13 commits into from
Jan 24, 2025

Conversation

rkrasiuk
Copy link
Member

Description

A placeholder PR to figure out why test_state_root_task is failing in CI.

@rkrasiuk rkrasiuk added C-test A change that impacts how or what we test A-trie Related to Merkle Patricia Trie implementation labels Jan 21, 2025
@fgimenez fgimenez force-pushed the rkrasiuk/figure-out-whatsup-with-test branch from 999ab0c to 161bd66 Compare January 23, 2025 11:02
@fgimenez fgimenez marked this pull request as ready for review January 23, 2025 16:35
@fgimenez fgimenez force-pushed the rkrasiuk/figure-out-whatsup-with-test branch from b3457bb to 93d135c Compare January 24, 2025 12:08
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

pending @rkrasiuk for proof fn changes

Comment on lines 48 to 49
pub(crate) fn thread_pool_size() -> usize {
std::thread::available_parallelism().map_or(4, |num| (num.get() / 2).max(4))
Copy link
Collaborator

Choose a reason for hiding this comment

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

this solution should work because it should avoid intra worker thread deadlocks but is not super ideal.

at least for this is a good enough work around, will try to come up with a better solution

crates/engine/tree/src/tree/root.rs Show resolved Hide resolved
Comment on lines 48 to 49
pub(crate) fn thread_pool_size() -> usize {
std::thread::available_parallelism().map_or(4, |num| (num.get() / 2).max(4))
Copy link
Member Author

Choose a reason for hiding this comment

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

does this work with 3 as well?

Copy link
Member

Choose a reason for hiding this comment

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

yep, anything above 2, pushed changes for 3

@fgimenez fgimenez added this pull request to the merge queue Jan 24, 2025
Merged via the queue into main with commit 0cd63cd Jan 24, 2025
43 checks passed
@fgimenez fgimenez deleted the rkrasiuk/figure-out-whatsup-with-test branch January 24, 2025 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-trie Related to Merkle Patricia Trie implementation C-test A change that impacts how or what we test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants