Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support PTC for Duty RPC #14013

Closed
wants to merge 1 commit into from
Closed

Support PTC for Duty RPC #14013

wants to merge 1 commit into from

Conversation

terencechain
Copy link
Member

@terencechain terencechain commented May 17, 2024

This PR adds support for PTC for Duty RPC.

  • Added ptc_slot to DutyResponse protobuf
  • Added PtcSlot to CommitteeAssignmentContainer container for helper function
  • Updated CommitteeAssignments to populate PtcSlot if the validator is part of the PTC of the slot
  • Added a unit test to ensure every single PTC is covered using the GetPayloadTimelinessCommittee helper
  • Added PtcAllocation helper to return the number of beacon committees PTC will borrow from in a slot and the number of validators PTC will borrow from in a beacon committee

⚠️ Note to the reviewer: This PR fixed multiple aspects unit tests:

  1. TestGetAttestationData in Beacon API RPC tests uses the mainnet config. It supplements the minimum genesis validator count (16384) but intentionally leaves the last one inactive to test the unavailable case later. Having fewer than the minimum genesis validator count (16383) breaks the PTC assumption with a count of 512. 16383 translates to 3 committees per slot and 170 validators per committee. Under this case, PTC will borrow from 2 committees, resulting in 256 validators per committee. 256 is greater than 170. To fix this, I modified the unit test to start with 16385 validators instead of 16384. This way, the last validator can remain inactive while maintaining over 16384 validators. Due to this change, the shuffling was affected, so I updated the assertions.

  2. TestGetDuties_OK and others in Prysm API Duty tests use the minimal config, but they use parameters from the beacon config minimal config. PTC config is defined using the fieldparams package. I added //go:build minimal to the top of the file to ensure the PTC config is applied. I also adjusted the PTC minimal config value from 32 to 2 to align with the minimal genesis validator size. The same ratio is maintained: mainnetValidatorCount/mainnetPTCCount == minimalValidatorCount/minimalPTCCount.

  3. I separate out some of the beacon committee assignment test cases to use minimal but some of the beacon committee test cases are challenging due to the validator count being 256 and using mainnet state. I am unsure of the best resolution. Should I rewrite the test cases from scratch?

One solution to avoid revamping most existing unit tests is to accept the following condition in CommitteeAssignments. Instead of:

if committeLength < membersPerCommittee {
    return nil, nil, fmt.Errorf(
        "committee length %d is smaller than member per committee size %d",
        committeLength,
        membersPerCommittee,
    )
}

We simply continue and skip assigning a PTC slot to the CommitteeAssignmentContainer. This may be acceptable since that's only a duty and if we encounter this condition, the state is already bad and PTC responsibility may not matter.

@terencechain terencechain requested a review from a team as a code owner May 17, 2024 03:33
@terencechain terencechain requested review from saolyn, potuz and rkapka and removed request for a team May 17, 2024 03:33
@terencechain terencechain force-pushed the ptc-assignment branch 5 times, most recently from 0c75744 to 8842995 Compare May 18, 2024 00:00
@@ -29,6 +30,13 @@ import (
"github.com/prysmaticlabs/prysm/v5/testing/util"
)

// pubKey is a helper to generate a well-formed public key.
func pubKey(i uint64) []byte {
Copy link
Member Author

Choose a reason for hiding this comment

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

This was originally defined in beacon-chain/rpc/prysm/v1alpha1/validator/duties_test.go, but since we moved that to a minimal test, it needs to be redefined and can no longer be shared.

@terencechain terencechain force-pushed the ptc-assignment branch 4 times, most recently from cf49513 to 1655994 Compare May 18, 2024 02:52
@terencechain
Copy link
Member Author

I'll reopen with a different approach

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant