Add inactivity penalties to Altair attestation rewards endpoint response

[jimmygchen]

Description

A new inactivity field has been introduced to the attestation rewards endpoint response in #4474 for Phase 0, to include the inactivity penalty which is not part of the head/target/souce rewards. This hasn't been added to Altair and the implementation needs to be updated.

• Consensus spec: link
• Beacon API spec (PR): link

Example response

{
  "ideal_rewards": [
    {
      "effective_balance": "32000000000",
      "head": "212426",
      "target": "212426",
      "source": "212426",
      "inactivity": "0"
    }
  ],
  "total_rewards": [
    {
      "validator_index": "0",
      "head": "212426",
      "target": "212426",
      "source": "212426",
      "inactivity": "0"
    }  
  ]
}

[zack-scott]

@jimmygchen I am seeing inactivity field in the API response for Altair using version 4.5.0 node. Is the inactivity field correctly calculated in Altair and later versions?

[zack-scott]

@jimmygchen Also, I am not seeing inclusion_delay field for anything besides phase0. Not sure if this is expected behavior or needs to be implemented for Altair and later versions?

[jimmygchen]

@zack-scott inclusion_delay is a phase0 component and is not applicable to from altair due to changes in how attestation rewards are calculated, hence you won't see it on altair and later slots.

The inactivity calculation hasn't been implemented on this API for Altair, and is what this issue need to address.

[zack-scott]

@jimmygchen okay, I was just wanting to ensure that the inactivity calculation is missing just on Altair only.

Just to clarify, Phase0, Bellatrix, and Capella versions do have the correct inactivity calculation in place?

This match statement seems to point Altair, Bellatrix, and Capella toward the compute_attestation_rewards_altair function
https://github.com/jimmygchen/lighthouse/blob/stable/beacon_node/beacon_chain/src/attestation_rewards.rs#L54, which would mean everything post Phase0 needs inactivity penalty implemented I believe?

[zack-scott]

@jimmygchen if the above comments sound correct to you, I would like to try and implement this if that is okay?

I may need some guidance though as I am a newcomer to this project, but I believe something like this may work for the this issue:

let inactivity_score = &state.get_inactivity_score(*validator_index)?;
let inactivity_penalty = match state {
    BeaconState::Base(_) => 0i64,
    BeaconState::Altair(_) => {
        (inactivity_score * effective_balance / (4 * 50331648)) as i64
    }
    BeaconState::Capella(_) | BeaconState::Merge(_) => {
        (inactivity_score * effective_balance / (4 * 16777216)) as i64
    }
};

I am using the inactivity penalty formula found in the Eth2 Book

The code snippet above is within the compute_attestation_rewards_altair function, which means the BeaconState::Base(_) shouldn't ever be executed here, but the match statement required for me to insert that arm.

[jimmygchen]

Hi @zack-scott

That would be great! I haven't really look into the Altair calculation in depth, but happy to look into it and try to answer any questions.

which would mean everything post Phase0 needs inactivity penalty implemented I believe

Yes that's right, the inactivity penalty calculation is missing on all post-Altair versions.

We already have a way to calculate the inactivity penalty in a generic way, so you probably won't need to worry about adding this match arm, instead you could use spec.inactivity_penalty_quotient_for_state(state), which is currently used here:

lighthouse/consensus/state_processing/src/per_epoch_processing/altair/rewards_and_penalties.rs

Lines 119 to 128 in f2223fe

	if !matching_target_indices.contains(index)? {
	let penalty_numerator = state
	.get_validator(index)?
	.effective_balance
	.safe_mul(state.get_inactivity_score(index)?)?;
	let penalty_denominator = spec
	.inactivity_score_bias
	.safe_mul(spec.inactivity_penalty_quotient_for_state(state))?;
	delta.penalize(penalty_numerator.safe_div(penalty_denominator)?)?;
	}

If there's a way you could re-use the some of the functionalities in state_processing without modifying it, that would be ideal. e.g. spec.inactivity_penalty_quotient_for_state(state) seems like something you can reuse without having to modify.

It would be nice to verify the calculation results your calculation with a similar test like this one (but for post-altair):

lighthouse/beacon_node/beacon_chain/tests/rewards.rs

Line 171 in fc7f1ba

async fn test_verify_attestation_rewards_base_inactivity_leak() {

Let me know if you have any questions!

[zack-scott]

@jimmygchen thank you, I will work on getting the generic calculation and tests added.

[jimmygchen]

Completed in #4807 🎉 Thank you @zack-scott