Refactor beacon attestation validation

[wemeetagain]

Should we move this into a function somewhere and be re-used in gossip + here + state transition? In a way to signal that this logic must be consistent between those three places

Originally posted by @dapplion in #6198 (comment)

We should.

[HiroyukiNaito]

I pick the issue as a good first issue.

[HiroyukiNaito]

I'm locating where should I change.

Should we move this into a function somewhere and be re-used in gossip + here + state transition? In a way to signal that this logic must be consistent between those three places

Is this right place?
https://github.com/ChainSafe/lodestar/blob/unstable/packages/beacon-node/src/chain/validation/attestation.ts#L478

[HiroyukiNaito]

Shamely, I could not find where should I change the state-transition code.
Do you mind if you give me some hints?

[ensi321]

@HiroyukiNaito Hey! Thanks for picking this up.

Just want to share some background about this issue. Feel free to skip to tldr.

The related change is for implementing Deneb EIP EIP-7045, which is to extend the admissible range of attestation from [n - 32, n] slots to [(Math.floor(n / 32) - 1) * 32, n] slots. You can check out our implementation #5731 and also the consensus spec here.

The attestation validation logic in p2p we want to refactoring in Lodestar is to limit to lower bound of the slot in attestation by calculating earliestPermissableSlot. However, during state-transition, specifically during process_attestation, there is already an assertion in the spec and lodestar to limit the attestation to be within current or previous epoch, which has the same effect as setting lower (and upper) bound of the attestation slot. Therefore, the attestation validation logic in p2p is not needed in state-transition.

Shamely, I could not find where should I change the state-transition code.
Do you mind if you give me some hints?

tldr; The original comment is not quite accurate about state-transition. No refactoring needs to be made for state-transition. Only beacon_aggregate_and_proof and beacon_attestation_{subnet_id}

Is this right place?
https://github.com/ChainSafe/lodestar/blob/unstable/packages/beacon-node/src/chain/validation/attestation.ts#L478

Yes. Should be a straightforward change. Let me know if you have further questions

[HiroyukiNaito]

@ensi321 Thank you for describing the technical context of this.
So, I try to crop the codes and functionalize them, and apply the 2 points you suggested.