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

Remove auto traits from ICE work-around #26098

Merged
merged 2 commits into from
Jun 23, 2022

Conversation

brooksprumo
Copy link
Contributor

@brooksprumo brooksprumo commented Jun 21, 2022

Problem

#23295 added auto traits to BuiltinFeatureTransition to work around Rust issue rust-lang/rust#92987. This bug was fixed in Rust v1.60.0, which is the version we use, so this work-around can be removed.

// https://github.com/solana-labs/solana/pull/23233 added `BuiltinFeatureTransition`
// to `Bank` which triggers https://github.com/rust-lang/rust/issues/92987 while
// attempting to resolve `Sync` on `BankRc` in `AccountsBackgroundService::new` ala,
//
// query stack during panic:
// #0 [evaluate_obligation] evaluating trait selection obligation `bank::BankRc: core::marker::Sync`
// #1 [typeck] type-checking `accounts_background_service::<impl at runtime/src/accounts_background_service.rs:358:1: 520:2>::new`
// #2 [typeck_item_bodies] type-checking all item bodies
// #3 [analysis] running analysis passes on this crate
// end of query stack
//
// Yoloing a `Sync` onto it avoids the auto trait evaluation and thus the ICE.
//
// We should remove this when upgrading to Rust 1.60.0, where the bug has been
// fixed by https://github.com/rust-lang/rust/pull/93064
unsafe impl Send for BuiltinFeatureTransition {}
unsafe impl Sync for BuiltinFeatureTransition {}

Summary of Changes

Remove work-around from #23295 and #23321.

@brooksprumo brooksprumo marked this pull request as ready for review June 21, 2022 15:50
@brooksprumo brooksprumo requested a review from t-nelson June 21, 2022 15:50
@brooksprumo
Copy link
Contributor Author

brooksprumo commented Jun 21, 2022

Should #23321 be reverted as well?

Edit: Reverting it too.

@brooksprumo brooksprumo merged commit 7d29c26 into solana-labs:master Jun 23, 2022
@brooksprumo brooksprumo deleted the revert-ice-workaround branch June 23, 2022 12:22
mergify bot pushed a commit that referenced this pull request Jun 23, 2022
mergify bot added a commit that referenced this pull request Jun 23, 2022
(cherry picked from commit 7d29c26)

Co-authored-by: Brooks Prumo <[email protected]>
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.

2 participants