Skip to content
This repository has been archived by the owner on Jan 13, 2025. It is now read-only.

Ensure stakes are always sorted before generating a leader_schedule #3016

Closed
wants to merge 1 commit into from

Conversation

sagar-solana
Copy link
Contributor

Problem

Stakes need to be sorted, otherwise leader schedule becomes non-deterministic based on when stakes were first added to accounts.

Summary of Changes

Sort stakes before generating a leader schedule.

@codecov
Copy link

codecov bot commented Feb 28, 2019

Codecov Report

Merging #3016 into master will increase coverage by <.1%.
The diff coverage is 100%.

@@           Coverage Diff            @@
##           master   #3016     +/-   ##
========================================
+ Coverage    76.9%   76.9%   +<.1%     
========================================
  Files         130     130             
  Lines       19888   19917     +29     
========================================
+ Hits        15294   15319     +25     
- Misses       4594    4598      +4

Copy link
Contributor

@garious garious left a comment

Choose a reason for hiding this comment

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

Great to see this fixed, but hurts to see an attempt to bring back code from earlier this week without its unit-tests. The original code was commented as well...

fn sort_stakes(stakes: &mut Vec<(Pubkey, u64)>) {
    // Sort first by stake. If stakes are the same, sort by pubkey to ensure a
    // deterministic result.
    // Note: Use unstable sort, because we dedup right after to remove the equal elements.
    stakes.sort_unstable_by(|(pubkey0, stake0), (pubkey1, stake1)| {
        if stake0 == stake1 {
            pubkey0.cmp(&pubkey1)
        } else {
            stake0.cmp(&stake1)
        }
    });

    // Now that it's sorted, we can do an O(n) dedup.
    stakes.dedup();
}

    #[test]
    fn test_sort_stakes_basic() {
        let pubkey0 = Keypair::new().pubkey();
        let pubkey1 = Keypair::new().pubkey();
        let mut stakes = vec![(pubkey0, 2), (pubkey1, 1)];
        sort_stakes(&mut stakes);
        assert_eq!(stakes, vec![(pubkey1, 1), (pubkey0, 2)]);
    }

    #[test]
    fn test_sort_stakes_with_dup() {
        let pubkey0 = Keypair::new().pubkey();
        let pubkey1 = Keypair::new().pubkey();
        let mut stakes = vec![(pubkey0, 1), (pubkey1, 2), (pubkey0, 1)];
        sort_stakes(&mut stakes);
        assert_eq!(stakes, vec![(pubkey0, 1), (pubkey1, 2)]);
    }

    #[test]
    fn test_sort_stakes_with_equal_stakes() {
        let pubkey0 = Pubkey::default();
        let pubkey1 = Keypair::new().pubkey();
        let mut stakes = vec![(pubkey0, 1), (pubkey1, 1)];
        sort_stakes(&mut stakes);
        assert_eq!(stakes, vec![(pubkey0, 1), (pubkey1, 1)]);
    }
}

@sagar-solana
Copy link
Contributor Author

@garious, sorry about that, it was a hotfix pr for something Rob needed :(

@sagar-solana sagar-solana requested a review from garious March 1, 2019 00:21
@sagar-solana
Copy link
Contributor Author

Everything in this PR was already included in #2992

@sagar-solana sagar-solana deleted the sort_stakes branch June 14, 2019 04:58
behzadnouri added a commit to behzadnouri/solana that referenced this pull request Sep 30, 2024
max_bytes for outgoing push messages is pretty outdated and does not
allow gossip to function properly with current testnet cluster size.

In particular it does not allow to clear out queue of pending push
messages unless the new_push_messages function is called very frequently
which involves repeatedly locking/unlocking CRDS table.

Additionally leaving gossip entries in the queue for the next round will
add delay to propagating push messages which can compound as messages go
through several hops.
yihau pushed a commit to yihau/solana that referenced this pull request Oct 2, 2024
…a-labs#3016) (solana-labs#3038)

reworks max number of outgoing push messages (solana-labs#3016)

max_bytes for outgoing push messages is pretty outdated and does not
allow gossip to function properly with current testnet cluster size.

In particular it does not allow to clear out queue of pending push
messages unless the new_push_messages function is called very frequently
which involves repeatedly locking/unlocking CRDS table.

Additionally leaving gossip entries in the queue for the next round will
add delay to propagating push messages which can compound as messages go
through several hops.

(cherry picked from commit 489f483)

Co-authored-by: behzad nouri <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants