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

v2.0: banking_stage: do not insert legacy vote ixs, refactor & unstaked (backport of #2888) #2901

Merged
merged 3 commits into from
Sep 30, 2024

Conversation

mergify[bot]
Copy link

@mergify mergify bot commented Sep 11, 2024

Problem

The feature deprecate_legacy_vote_ixs disallows all previous vote instructions in the vote program other than the TowerSync variants. Banking stage should also filter these out.

Summary of Changes

When the feature is active, do not insert votes with these legacy instructions, also some small refactoring of the weighted shuffle and unstaked check.


This is an automatic backport of pull request #2888 done by Mergify.

)

* banking_stage: do not insert legacy vote ixs, refactor & unstaked

* pr feedback: use matches instead of separate fn

(cherry picked from commit 1334fb5)

# Conflicts:
#	core/src/banking_stage/latest_unprocessed_votes.rs
@mergify mergify bot requested a review from a team as a code owner September 11, 2024 19:29
@mergify mergify bot added the conflicts label Sep 11, 2024
Copy link
Author

mergify bot commented Sep 11, 2024

Cherry-pick of 1334fb5 has failed:

On branch mergify/bp/v2.0/pr-2888
Your branch is up to date with 'origin/v2.0'.

You are currently cherry-picking commit 1334fb5248.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	modified:   core/src/banking_stage.rs
	modified:   core/src/banking_stage/forwarder.rs
	modified:   core/src/banking_stage/unprocessed_transaction_storage.rs

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   core/src/banking_stage/latest_unprocessed_votes.rs

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

Comment on lines 856 to 858
pub mod deprecate_legacy_vote_ixs {
solana_program::declare_id!("depVvnQ2UysGrhwdiwU42tCadZL8GcBb1i2GYhMopQv");
}

Choose a reason for hiding this comment

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

it seems odd to me to manually backport the creation of this feature for the filtering, but not the actaul code for disallowing the instructions in the processor.
Seems like we should be backporting #587 if we are using this

Choose a reason for hiding this comment

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

I had to add it as part of fixing conflicts. I would prefer not to backport #587 since it's a huge change and the feature is not planned to be activated till 2.1.0. I can rekey this with a dummy ID if that makes more sense.

Choose a reason for hiding this comment

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

got it - probably fine then, just slightly concerned someone sees the feature is in 2.0, then changes schedule to say it can be activated with 2.0 (which it cannot be).

Choose a reason for hiding this comment

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

i've rekeyed the feature to avoid any mishaps.

@sakridge
Copy link

What's the case for backport? It seems like a new feature essentially to me.

Copy link

@bw-solana bw-solana left a comment

Choose a reason for hiding this comment

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

LGTM overall but left a few questions/suggestions

for vote_source in [VoteSource::Gossip, VoteSource::Tpu] {
for (vote_source, staked) in [VoteSource::Gossip, VoteSource::Tpu]
.into_iter()
.flat_map(|vs| [(vs, true), (vs, false)])

Choose a reason for hiding this comment

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

This looks like a good opportunity to use the iproduct! macro

@@ -6108,6 +6108,15 @@ impl Bank {
Some(vote_account.clone())
}

/// Get the EpochStakes for the current Bank::epoch
pub fn current_epoch_stakes(&self) -> &EpochStakes {

Choose a reason for hiding this comment

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

can this function be private?

Choose a reason for hiding this comment

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

yeah it can, good catch. would you prefer I fix this and iproduct! in a master commit and then backport both commits together? or merge this and then fix in master and backport separately?

Choose a reason for hiding this comment

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

I'm okay with either approach. Neither is a big enough deal that I'd block the PR

Copy link

@bw-solana bw-solana left a comment

Choose a reason for hiding this comment

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

LGTM

@AshwinSekar AshwinSekar merged commit 573db9c into v2.0 Sep 30, 2024
39 checks passed
@AshwinSekar AshwinSekar deleted the mergify/bp/v2.0/pr-2888 branch September 30, 2024 23:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants