feat: add BeaconState functions/mutators #37

varun-doshi · 2025-01-12T19:51:36Z

Added BeaconState mutators and functions

KolbyML · 2025-01-12T19:54:59Z

crates/common/consensus/src/deneb/beacon_state.rs

+    /// Decrease the validator balance at index ``index`` by ``delta`` with underflow protection.
+    pub fn decrease_balance(mut self, index: u64, delta: u64) {
+        if let Some(balance) = self.balances.get_mut(index as usize) {
+            *balance -= delta;


Suggested change

*balance -= delta;

balance.saturating_sub(delta)

the comment says underflow protection, I am assuming that would be a saturating sub?

KolbyML · 2025-01-12T19:55:05Z

crates/common/consensus/src/deneb/beacon_state.rs

+    }
+
+    /// Decrease the validator balance at index ``index`` by ``delta`` with underflow protection.
+    pub fn decrease_balance(mut self, index: u64, delta: u64) {


Suggested change

pub fn decrease_balance(mut self, index: u64, delta: u64) {

pub fn decrease_balance(&mut self, index: u64, delta: u64) {

KolbyML

here is some more feedback

KolbyML · 2025-01-13T21:51:59Z

crates/common/consensus/src/deneb/beacon_state.rs

+        let current_epoch = self.get_current_epoch();
+        let mut exit_queue_epoch = exit_epochs
+            .into_iter()
+            .max()
+            .unwrap_or_else(|| compute_activation_exit_epoch(current_epoch));


Suggested change

let current_epoch = self.get_current_epoch();

let mut exit_queue_epoch = exit_epochs

.into_iter()

.max()

.unwrap_or_else(|| compute_activation_exit_epoch(current_epoch));

let mut exit_queue_epoch = exit_epochs

.into_iter()

.max()

.unwrap_or_else(|| compute_activation_exit_epoch(self.get_current_epoch()));

this isn't reused it should be inlined

KolbyML · 2025-01-13T21:55:46Z

crates/common/consensus/src/deneb/beacon_state.rs

+        let current_epoch = self.get_current_epoch();
+        let mut exit_queue_epoch = exit_epochs
+            .into_iter()
+            .max()
+            .unwrap_or_else(|| compute_activation_exit_epoch(current_epoch));


This doesn't follow the spec no?

Suggested change

let current_epoch = self.get_current_epoch();

let mut exit_queue_epoch = exit_epochs

.into_iter()

.max()

.unwrap_or_else(|| compute_activation_exit_epoch(current_epoch));

let current_epoch = self.get_current_epoch();

let mut exit_queue_epoch = max(exit_epochs, vec![compute_activation_exit_epoch(self.get_current_epoch())]);

I think this would match the spec better

This however returns a Vec instead of just a u64

This feels a more suitable approach:

exit_epochs.push(compute_activation_exit_epoch(self.get_current_epoch())); let mut exit_queue_epoch = *exit_epochs.iter().max().unwrap_or(&0);

sounds good

KolbyML · 2025-01-13T21:57:23Z

crates/common/consensus/src/deneb/beacon_state.rs

+        if index as usize >= self.validators.len() {
+            return;
+        }
+        let exit_epochs: Vec<u64> = self


aren't you missing this check

if validator.exit_epoch != FAR_FUTURE_EPOCH: return

KolbyML · 2025-01-13T22:17:43Z

crates/common/consensus/src/deneb/beacon_state.rs

+        slashed_index: usize,
+        whistleblower_index: Option<usize>,


Suggested change

slashed_index: usize,

whistleblower_index: Option<usize>,

slashed_index: u64,

whistleblower_index: Option<u64>,

these should be u64's as that is how they are defined in the spec, also then there will be less type conversions in this function so it will be cleaner to read

KolbyML · 2025-01-13T22:18:41Z

crates/common/consensus/src/deneb/beacon_state.rs

+    }
+
+    /// Slash the validator with index ``slashed_index``
+    pub fn slash_validator(


This function is outdated, please update it to follow the latest version of the spec https://github.com/ethereum/consensus-specs/blob/dev/specs/bellatrix/beacon-chain.md#modified-slash_validator

KolbyML · 2025-01-13T22:19:25Z

crates/common/consensus/src/fork_choice/helpers/constants.rs

 pub const SLOTS_PER_HISTORICAL_ROOT: u64 = 8192;
-pub const MIN_SEED_LOOKAHEAD: u64 = 1;
-pub const MAX_COMMITTEES_PER_SLOT: u64 = 64;
+pub const WHISTLEBLOWER_REWARD_QUOTIENT: u64 = 512;


WHISTLEBLOWER_REWARD_QUOTIENT should go below TARGET_COMMITTEE_SIZE

KolbyML · 2025-01-15T22:51:58Z

crates/common/consensus/src/deneb/beacon_state.rs

+        let whistleblower_index = whistleblower_index.unwrap_or(proposer_index);
+
+        let whistleblower_reward = validator.effective_balance / WHISTLEBLOWER_REWARD_QUOTIENT;
+        let proposer_reward = (whistleblower_reward * PROPOSER_WEIGHT) / WEIGHT_DENOMINATOR;


Suggested change

let proposer_reward = (whistleblower_reward * PROPOSER_WEIGHT) / WEIGHT_DENOMINATOR;

let proposer_reward = whistleblower_reward * PROPOSER_WEIGHT / WEIGHT_DENOMINATOR;

these brackets aren't needed due to order of operations

KolbyML · 2025-01-15T22:58:33Z

crates/common/consensus/src/deneb/beacon_state.rs

+        // Mutably borrow the slashed validator
+        let mut binding = self.clone();
+        let validator = binding
+            .validators
+            .get_mut(slashed_index as usize)
+            .ok_or_else(|| anyhow::anyhow!("Validator at index {} not found", slashed_index))?;
+
+        validator.slashed = true;
+        validator.withdrawable_epoch = std::cmp::max(
+            validator.withdrawable_epoch,
+            epoch + EPOCHS_PER_SLASHINGS_VECTOR,
+        );
+
+        // Add slashed effective balance to the slashings vector
+        self.slashings[(epoch % EPOCHS_PER_SLASHINGS_VECTOR) as usize] +=
+            validator.effective_balance;
+
+        // Decrease validator balance
+        self.clone().decrease_balance(
+            slashed_index,
+            validator.effective_balance / MIN_SLASHING_PENALTY_QUOTIENT,
+        );


Suggested change

// Mutably borrow the slashed validator

let mut binding = self.clone();

let validator = binding

.validators

.get_mut(slashed_index as usize)

.ok_or_else(|| anyhow::anyhow!("Validator at index {} not found", slashed_index))?;

validator.slashed = true;

validator.withdrawable_epoch = std::cmp::max(

validator.withdrawable_epoch,

epoch + EPOCHS_PER_SLASHINGS_VECTOR,

);

// Add slashed effective balance to the slashings vector

self.slashings[(epoch % EPOCHS_PER_SLASHINGS_VECTOR) as usize] +=

validator.effective_balance;

// Decrease validator balance

self.clone().decrease_balance(

slashed_index,

validator.effective_balance / MIN_SLASHING_PENALTY_QUOTIENT,

);

let validator_effective_balance = if let Some(validator) = self.validators

.get_mut(slashed_index as usize) {

validator.slashed = true;

validator.withdrawable_epoch = std::cmp::max(

validator.withdrawable_epoch,

epoch + EPOCHS_PER_SLASHINGS_VECTOR,

);

validator.effective_balance

} else {

bail!("Validator at index {slashed_index} not found")

};

// Add slashed effective balance to the slashings vector

self.slashings[(epoch % EPOCHS_PER_SLASHINGS_VECTOR) as usize] +=

validator_effective_balance;

// Decrease validator balance

self.decrease_balance(

slashed_index,

validator_effective_balance / MIN_SLASHING_PENALTY_QUOTIENT,

);

You shouldn't be doing clones like this, we aren't using pointers so this code wouldn't work, the cloned object would be different then self. You would be modifying the clone, not self.

KolbyML · 2025-01-16T06:32:20Z

crates/common/consensus/src/deneb/beacon_state.rs

+        let validator_effective_balance =
+            if let Some(validator) = self.validators.get_mut(slashed_index as usize) {
+                validator.slashed = true;
+                validator.withdrawable_epoch = std::cmp::max(
+                    validator.withdrawable_epoch,
+                    epoch + EPOCHS_PER_SLASHINGS_VECTOR,
+                );
+                validator.effective_balance
+            } else {
+                bail!("Validator at index {slashed_index} not found")
+            };
+        // Add slashed effective balance to the slashings vector
+        self.slashings[(epoch % EPOCHS_PER_SLASHINGS_VECTOR) as usize] +=
+            validator_effective_balance;
+        // Decrease validator balance
+        self.decrease_balance(
+            slashed_index,
+            validator_effective_balance / MIN_SLASHING_PENALTY_QUOTIENT,
+        );
+
+        // Apply proposer and whistleblower rewards
+        let proposer_index = self.get_beacon_proposer_index()?;
+        let whistleblower_index = whistleblower_index.unwrap_or(proposer_index);
+
+        let whistleblower_reward =
+            if let Some(validator) = self.validators.get(slashed_index as usize) {
+                validator.effective_balance / WHISTLEBLOWER_REWARD_QUOTIENT
+            } else {
+                bail!("Validator at index {slashed_index} not found")
+            };
+        let proposer_reward = whistleblower_reward * PROPOSER_WEIGHT / WEIGHT_DENOMINATOR;


Suggested change

let validator_effective_balance =

if let Some(validator) = self.validators.get_mut(slashed_index as usize) {

validator.slashed = true;

validator.withdrawable_epoch = std::cmp::max(

validator.withdrawable_epoch,

epoch + EPOCHS_PER_SLASHINGS_VECTOR,

);

validator.effective_balance

} else {

bail!("Validator at index {slashed_index} not found")

};

// Add slashed effective balance to the slashings vector

self.slashings[(epoch % EPOCHS_PER_SLASHINGS_VECTOR) as usize] +=

validator_effective_balance;

// Decrease validator balance

self.decrease_balance(

slashed_index,

validator_effective_balance / MIN_SLASHING_PENALTY_QUOTIENT,

);

// Apply proposer and whistleblower rewards

let proposer_index = self.get_beacon_proposer_index()?;

let whistleblower_index = whistleblower_index.unwrap_or(proposer_index);

let whistleblower_reward =

if let Some(validator) = self.validators.get(slashed_index as usize) {

validator.effective_balance / WHISTLEBLOWER_REWARD_QUOTIENT

} else {

bail!("Validator at index {slashed_index} not found")

};

let proposer_reward = whistleblower_reward * PROPOSER_WEIGHT / WEIGHT_DENOMINATOR;

let validator_effective_balance =

if let Some(validator) = self.validators.get_mut(slashed_index as usize) {

validator.slashed = true;

validator.withdrawable_epoch = std::cmp::max(

validator.withdrawable_epoch,

epoch + EPOCHS_PER_SLASHINGS_VECTOR,

);

validator.effective_balance

} else {

bail!("Validator at index {slashed_index} not found")

};

// Add slashed effective balance to the slashings vector

self.slashings[(epoch % EPOCHS_PER_SLASHINGS_VECTOR) as usize] +=

validator_effective_balance;

// Decrease validator balance

self.decrease_balance(

slashed_index,

validator_effective_balance / MIN_SLASHING_PENALTY_QUOTIENT,

);

// Apply proposer and whistleblower rewards

let proposer_index = self.get_beacon_proposer_index()?;

let whistleblower_index = whistleblower_index.unwrap_or(proposer_index);

let whistleblower_reward = validator_effective_balance / WHISTLEBLOWER_REWARD_QUOTIENT;

let proposer_reward = whistleblower_reward * PROPOSER_WEIGHT / WEIGHT_DENOMINATOR;

the effective balance doesn't change in this function, so we don't need to call self.validators.get() again

KolbyML

looks good

KolbyML reviewed Jan 12, 2025

View reviewed changes

varun-doshi force-pushed the varun/main branch from c90506b to ab3448d Compare January 13, 2025 05:18

KolbyML reviewed Jan 13, 2025

View reviewed changes

varun-doshi force-pushed the varun/main branch from ab3448d to e76b4f3 Compare January 14, 2025 05:15

KolbyML reviewed Jan 15, 2025

View reviewed changes

varun-doshi force-pushed the varun/main branch from e76b4f3 to f1b7ae5 Compare January 16, 2025 06:20

KolbyML reviewed Jan 16, 2025

View reviewed changes

feat: add BeaconState functions/mutators

b7f44b6

varun-doshi force-pushed the varun/main branch from f1b7ae5 to b7f44b6 Compare January 16, 2025 06:39

KolbyML approved these changes Jan 16, 2025

View reviewed changes

KolbyML added this pull request to the merge queue Jan 16, 2025

Merged via the queue into ReamLabs:master with commit f095027 Jan 16, 2025
5 checks passed

	pub fn decrease_balance(mut self, index: u64, delta: u64) {
	pub fn decrease_balance(&mut self, index: u64, delta: u64) {

	let proposer_reward = (whistleblower_reward * PROPOSER_WEIGHT) / WEIGHT_DENOMINATOR;
	let proposer_reward = whistleblower_reward * PROPOSER_WEIGHT / WEIGHT_DENOMINATOR;

feat: add BeaconState functions/mutators #37

feat: add BeaconState functions/mutators #37

Uh oh!

Conversation

varun-doshi commented Jan 12, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KolbyML left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KolbyML Jan 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KolbyML left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

KolbyML Jan 15, 2025 •

edited

Loading