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

Use VoteAccount::node_pubkey() #26207

Conversation

brooksprumo
Copy link
Contributor

@brooksprumo brooksprumo commented Jun 24, 2022

The original intent of this PR changed. See the discussion (and #26207 (comment) for more info).

Problem

Original:

Clippy was warning about temporary with significant drop in match scrutinee for a bunch of uses of VoteAccounts::vote_state(). These locks are all reader locks, and there will never be another writer (since the one was within a Once), so the clippy warnings are safe to ignore.

See #26207 (comment) for more info.

Related to #24836

Actual:

No problem, just an opportunity to refactor 😐

Summary of Changes

Use VoteAccount::node_pubkey() when possible.

@brooksprumo brooksprumo force-pushed the fix-significant-drop-in-scrutinee/vote_state branch 2 times, most recently from d244d14 to 956587a Compare June 24, 2022 18:54
core/src/consensus.rs Outdated Show resolved Hide resolved
runtime/src/bank.rs Outdated Show resolved Hide resolved
core/src/replay_stage.rs Outdated Show resolved Hide resolved
ledger/src/blockstore_processor.rs Outdated Show resolved Hide resolved
runtime/src/epoch_stakes.rs Outdated Show resolved Hide resolved
core/src/validator.rs Show resolved Hide resolved
runtime/src/bank.rs Outdated Show resolved Hide resolved
runtime/src/stakes.rs Show resolved Hide resolved
@brooksprumo brooksprumo marked this pull request as ready for review June 24, 2022 20:49
@brooksprumo
Copy link
Contributor Author

@buffalu Can't add you to the reviewer's list, but want to make sure you're in the loop.

@buffalu
Copy link
Contributor

buffalu commented Jun 24, 2022

nice find, thanks clippy gods

core/src/replay_stage.rs Outdated Show resolved Hide resolved
@behzadnouri
Copy link
Contributor

behzadnouri commented Jun 25, 2022

This does not seem to be necessary.

The only place that self.0.vote_state is write locked is here:
https://github.com/solana-labs/solana/blob/2efdb965d/runtime/src/vote_account.rs#L82-L89
which is done once and only once the very first time anyone touches VoteAccount::vote_state.
It does not have a lock contention there because it is wrapped in Once::call_once and there is no other lock reference there yet at that point.

All other references are shared-lock, so they can't cause any lock contention.

So, this is a false positive warning, and no need to add manual drops or additional .clone overhead.
Lets just ignore it with #[allow(clippy::...)]

@brooksprumo brooksprumo force-pushed the fix-significant-drop-in-scrutinee/vote_state branch from 956587a to edd1a9d Compare June 26, 2022 03:02
@brooksprumo brooksprumo changed the title Fix callers of VoteAccounts::vote_state() holding the lock too long Ignore clippy warnings on VoteAccounts::vote_state() holding the lock too long Jun 26, 2022
@brooksprumo brooksprumo force-pushed the fix-significant-drop-in-scrutinee/vote_state branch from edd1a9d to 4700099 Compare June 26, 2022 03:09
@brooksprumo
Copy link
Contributor Author

This does not seem to be necessary.

Thanks for the explanation. I agree.

Lets just ignore it with #[allow(clippy::...)]

I tried, but since the lints are in a version newer than 1.60.0, CI checks fails. So this PR has morphed into a tiny refactor to call VoteAccount::node_pubkey().

@mvines
Copy link
Member

mvines commented Jun 26, 2022

This lint occurs all over the code base with recent nightlies, and it seems questionable and/or too aggressive.
In #25489 I globally suppressed it here. That PR is blocked on a the debug of a couple test failures though

@brooksprumo brooksprumo changed the title Ignore clippy warnings on VoteAccounts::vote_state() holding the lock too long Use VoteAccount::node_pubkey() Jun 27, 2022
@brooksprumo brooksprumo merged commit 662818e into solana-labs:master Jun 27, 2022
@brooksprumo brooksprumo deleted the fix-significant-drop-in-scrutinee/vote_state branch June 27, 2022 14:09
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.

5 participants