Add RestartHeaviestFork to Gossip #34161

wen-coding · 2023-11-18T06:11:58Z

Add RestartHeaviestFork to Gossip

codecov · 2023-11-18T07:18:16Z

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (acef7ba) 0.0% compared to head (7c338b0) 81.7%.

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #34161       +/-   ##
===========================================
+ Coverage        0    81.7%    +81.7%     
===========================================
  Files           0      826      +826     
  Lines           0   223236   +223236     
===========================================
+ Hits            0   182485   +182485     
- Misses          0    40751    +40751

behzadnouri · 2023-11-21T00:00:36Z

gossip/src/restart_crds_values.rs

+    // Sum of received heaviest fork validator stake / total stake * u16::MAX.
+    received_heaviest_fork_ratio: u16,


I would probably call this stake_ratio or observed_stake_ratio.
heaviest_fork is already in the name of struct; it is redundant to repeat it here again.

also why this u16 better than sending a f64 or the (stake, total_stake) as a tuple?

The original idea is we can save some bytes, but now you mention it, it's not really worth losing the precision by switching from 16 bytes to 2 bytes, I'm changing to (stake, total_stake) now.

behzadnouri · 2023-11-21T00:02:01Z

gossip/src/restart_crds_values.rs

+    ReceivedHeaviestForkStakeLargerThanTotalStake,
+    #[error("Total stake of received heaviest fork cannot be zero")]
+    ReceivedHeaviestForkStakeZero,


No need to repeat the enum name in the variants.
The variants are already qualified by the enum name, ie. RestartHeaviestForkError::....

behzadnouri · 2023-11-21T00:03:09Z

gossip/src/restart_crds_values.rs

@@ -122,6 +141,64 @@ impl RestartLastVotedForkSlots {
    }
 }

+impl Sanitize for RestartHeaviestFork {
+    fn sanitize(&self) -> Result<(), SanitizeError> {


can we sanitize the wallclock like the other crds values?
also probably that should be done for the previous restart crds value as well.

behzadnouri · 2023-11-21T00:03:56Z

gossip/src/restart_crds_values.rs

+        if self.received_heaviest_fork_ratio == 0 {
+            // this should at least include its own stake.
+            return Err(SanitizeError::ValueOutOfBounds);
+        }


what if node_stake / total_stake * u16::MAX < 1?

behzadnouri · 2023-11-21T00:05:39Z

gossip/src/restart_crds_values.rs

+    pub wallclock: u64,
+    pub last_slot: Slot,
+    pub last_slot_hash: Hash,
+    // Sum of received heaviest fork validator stake / total stake * u16::MAX.


What is "total stake"?
Is that the epoch stake or the stake observed in gossip?

behzadnouri · 2023-11-29T14:32:41Z

gossip/src/restart_crds_values.rs

+    pub last_slot: Slot,
+    pub last_slot_hash: Hash,
+    pub observed_stake: u64,
+    pub total_epoch_stake: u64,


why do we need this?
Isn't the epoch_stakes already known by all the nodes in the cluster?
https://github.com/solana-labs/solana/blob/1f00f8e29/runtime/src/bank.rs#L7744-L7746

I thought it might be nice to distribute total_epoch_stake just in case some calculation is off. But you are right, if this stake is wrong we have bigger problems. Removed.

behzadnouri · 2023-11-30T16:47:04Z

gossip/src/restart_crds_values.rs

+#[derive(Debug, Error, PartialEq)]
+pub enum RestartHeaviestForkError {
+    #[error("Total stake of received heaviest fork cannot be zero")]
+    ZeroObservedStake,
+}
+


This seems unnecessary and just adds verbosity.
Zero stake is practically not any different from stake of only 1 sol.
In addition a malicious node can just fake any number here.

behzadnouri · 2023-11-30T16:47:43Z

gossip/src/restart_crds_values.rs

+        if self.observed_stake == 0 {
+            // this should at least include its own stake.
+            return Err(SanitizeError::ValueOutOfBounds);
+        }


Similarly here, regarding stake of 0.

behzadnouri · 2023-11-30T16:48:02Z

gossip/src/restart_crds_values.rs

+        if observed_stake == 0 {
+            return Err(RestartHeaviestForkError::ZeroObservedStake);
+        }


again, here regarding zero stake.

behzadnouri · 2024-01-19T15:42:07Z

gossip/src/restart_crds_values.rs

+    pub fn new(
+        from: Pubkey,
+        now: u64,
+        last_slot: Slot,
+        last_slot_hash: Hash,
+        observed_stake: u64,
+        shred_version: u16,
+    ) -> Self {
+        Self {
+            from,
+            wallclock: now,
+            last_slot,
+            last_slot_hash,
+            observed_stake,
+            shred_version,
+        }
+    }


Why do we need this? all fields are public.
Either remove fields from public interface or remove this redundant method.

behzadnouri · 2024-01-19T15:44:24Z

gossip/src/restart_crds_values.rs

+        }
+    }
+
+    pub fn new_rand<R: Rng>(rng: &mut R, pubkey: Option<Pubkey>) -> Self {


Does this need to be pub?
I feel like every review I have to ask not to pollute public interface.

Can you go over all pubs in this code and remove those not needed?
or at least change them to pub(crate).

This is used in tests, and I don't see any benchmarks needing it now. So I changed it to pub(crate).

wen-coding and others added 2 commits November 17, 2023 22:10

Add RestartHeaviestFork to Gossip.

a2e4a5e

Merge branch 'solana-labs:master' into add_restart_heaviest_fork

ed573d9

wen-coding self-assigned this Nov 18, 2023

Add a test for out of bound value.

e2ad04d

wen-coding requested a review from behzadnouri November 18, 2023 23:14

behzadnouri reviewed Nov 21, 2023

View reviewed changes

wen-coding and others added 2 commits November 20, 2023 16:38

Merge branch 'master' into add_restart_heaviest_fork

30540d0

Send observed_stake and total_epoch_stake in ResatartHeaviestFork.

35f9c3e

behzadnouri reviewed Nov 29, 2023

View reviewed changes

wen-coding and others added 4 commits November 29, 2023 08:53

Remove total_epoch_stake from RestartHeaviestFork.

0fc9e08

Merge branch 'master' into add_restart_heaviest_fork

62f13a0

Forgot to update ABI digest.

dbd5159

Merge branch 'master' into add_restart_heaviest_fork

7c244a7

behzadnouri reviewed Nov 30, 2023

View reviewed changes

Remove checking of whether stake is zero.

8aedf03

github-actions bot added stale [bot only] Added to stale content; results in auto-close after a week. and removed stale [bot only] Added to stale content; results in auto-close after a week. labels Dec 15, 2023

github-actions bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Jan 2, 2024

Merge branch 'master' into add_restart_heaviest_fork

a976ca4

wen-coding removed the stale [bot only] Added to stale content; results in auto-close after a week. label Jan 4, 2024

wen-coding requested a review from behzadnouri January 4, 2024 19:03

github-actions bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Jan 19, 2024

behzadnouri removed the stale [bot only] Added to stale content; results in auto-close after a week. label Jan 19, 2024

behzadnouri reviewed Jan 19, 2024

View reviewed changes

wen-coding and others added 2 commits January 19, 2024 10:49

Merge branch 'solana-labs:master' into add_restart_heaviest_fork

2e2b84e

Remove unnecessary new function and make new_rand pub(crate).

7c338b0

behzadnouri approved these changes Jan 19, 2024

View reviewed changes

wen-coding merged commit 4a2871f into solana-labs:master Jan 19, 2024
35 checks passed

wen-coding deleted the add_restart_heaviest_fork branch January 19, 2024 21:59

willhickey mentioned this pull request Mar 28, 2024

v1.18 commits - please ignore anza-xyz/agave#475

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add RestartHeaviestFork to Gossip #34161

Add RestartHeaviestFork to Gossip #34161

wen-coding commented Nov 18, 2023

codecov bot commented Nov 18, 2023 •

edited

Loading

behzadnouri Nov 21, 2023

behzadnouri Nov 21, 2023

wen-coding Nov 21, 2023

behzadnouri Nov 21, 2023

wen-coding Nov 21, 2023

behzadnouri Nov 21, 2023

wen-coding Nov 21, 2023

behzadnouri Nov 21, 2023

wen-coding Nov 21, 2023

behzadnouri Nov 21, 2023

wen-coding Nov 21, 2023

behzadnouri Nov 29, 2023

wen-coding Nov 29, 2023

behzadnouri Nov 30, 2023

wen-coding Nov 30, 2023

behzadnouri Nov 30, 2023

wen-coding Nov 30, 2023

behzadnouri Nov 30, 2023

wen-coding Nov 30, 2023

behzadnouri Jan 19, 2024

wen-coding Jan 19, 2024

behzadnouri Jan 19, 2024

wen-coding Jan 19, 2024

		// Sum of received heaviest fork validator stake / total stake * u16::MAX.
		received_heaviest_fork_ratio: u16,

Add RestartHeaviestFork to Gossip #34161

Add RestartHeaviestFork to Gossip #34161

Conversation

wen-coding commented Nov 18, 2023

codecov bot commented Nov 18, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 18, 2023 •

edited

Loading