-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Implement ListBeaconCommittees RPC Method #3977
Conversation
* rm'ed in protobuf * build proto * build proto * build proto * fix core package * Gazelle * Fixed all the tests * Fixed static test * Comment out spec test for now * One more skip * fix-roundRobinSync (#3862) * Starting but need new seed function * Revert initial sync * Updated Proposer Slashing * Fixed all tests * Lint * Update inclusion reward * Fill randao mixes with eth1 data hash * Test * Fixing test part1 * All tests passing * One last test * Updated config * Build proto * Proper skip message * Conflict and fmt * Removed crosslinks and shards. Built * Format and gazelle * Fixed all the block package tests * Fixed all the helper tests * All epoch package tests pass * All core package tests pass * Fixed operation tests * Started fixing rpc test * RPC tests passed! * Fixed all init sync tests * All tests pass * Fixed blockchain tests * Lint * Lint * Preston's feedback * Starting * Remove container * Fixed block spec tests * All passing except for block_processing test * Failing block processing test * Starting * Add AggregateAndProof * All mainnet test passes
* Starting * Add AggregateAndProof
* rm'ed in protobuf * build proto * build proto * build proto * fix core package * Gazelle * Fixed all the tests * Fixed static test * Comment out spec test for now * One more skip * fix-roundRobinSync (#3862) * Starting but need new seed function * Revert initial sync * Updated Proposer Slashing * Fixed all tests * Lint * Update inclusion reward * Fill randao mixes with eth1 data hash * Test * Fixing test part1 * All tests passing * One last test * Updated config * Build proto * Proper skip message * Conflict and fmt * Removed crosslinks and shards. Built * Format and gazelle * Fixed all the block package tests * Fixed all the helper tests * All epoch package tests pass * All core package tests pass * Fixed operation tests * Started fixing rpc test * RPC tests passed! * Fixed all init sync tests * All tests pass * Fixed blockchain tests * Lint * Lint * Preston's feedback * Starting * Remove container * Fixed block spec tests * All passing except for block_processing test * Failing block processing test * Starting * Add AggregateAndProof * All mainnet test passes * Unskip block util tests
* Starting * Add AggregateAndProof * Unskip slot processing mainnet test
* Rm outdated interop tests * Rm test runner * Gazelle
* Updated committee cache * Removed shuffled indices cache * Started testing run time * Lint * Fixed test
@@ -69,10 +69,6 @@ func (s *Service) Status() error { | |||
// We archive committee information pertaining to the head state's epoch. | |||
func (s *Service) archiveCommitteeInfo(ctx context.Context, headState *pb.BeaconState) error { | |||
currentEpoch := helpers.SlotToEpoch(headState.Slot) | |||
committeeCount, err := helpers.CommitteeCountAtSlot(headState, helpers.StartSlot(currentEpoch)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is wrong, we shouldn't store this because it can change on a slot by slot basis
ProposerSeed: proposerSeed[:], | ||
AttesterSeed: attesterSeed[:], | ||
CommitteeCount: committeeCount * params.BeaconConfig().SlotsPerEpoch, | ||
ProposerSeed: proposerSeed[:], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two seeds are really all we need to determine historical assignments alongside historical balances
var attesterSeed [32]byte | ||
var activeIndices []uint64 | ||
var err error | ||
if requestingGenesis || helpers.SlotToEpoch(startSlot) < helpers.SlotToEpoch(headState.Slot) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the archival condition
) | ||
} | ||
attesterSeed = bytesutil.ToBytes32(archivedCommitteeInfo.AttesterSeed) | ||
} else if !requestingGenesis && helpers.SlotToEpoch(startSlot) == helpers.SlotToEpoch(headState.Slot) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise if requesting current epoch, return current results
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add these as comments? it seems helpful
…m into beacon-committees-rpc
@@ -42,30 +42,6 @@ func TestLifecycle_OK(t *testing.T) { | |||
|
|||
} | |||
|
|||
func TestRPC_BadEndpoint(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks unrelated, but it was so flaky and annoying that I support removing it in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It also wasn't really testing much - it wasn't really behavior we cared about testing in the first place
) | ||
} | ||
attesterSeed = bytesutil.ToBytes32(archivedCommitteeInfo.AttesterSeed) | ||
} else if !requestingGenesis && helpers.SlotToEpoch(startSlot) == helpers.SlotToEpoch(headState.Slot) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add these as comments? it seems helpful
// | ||
// If no filter criteria is specified, the response returns | ||
// all beacon committees for the current epoch. The results are paginated by default. | ||
func (bs *Server) ListBeaconCommittees( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this we might need more options for the req *ethpb.ListCommitteesRequest
. Along with epoch we also need to give a query option, where we can provide the state root.
In the event of a fork in the network, you could have two chains with different committees at the same epoch. To differentiate on the two sets of committees we would need to be able to identify committees from forked chains, so the state root is a good way to identify them. @shayzluf and I discussed this offline, in the event of a fork in the network , we need to be able to get the appropriate committees from each forked chain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great point, that's a neat suggestion. So what happens if you provide a state root that doesnt match head state?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can do this in a follow-up PR as that will increase the diff quite a lot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it would mean we are asking for the committees from a forked chain :) This would be important for the slasher, as it needs to be able to detect malicious attestations in the event of a fork. Sounds good, we can do it in a follow up PR
Resolves #3969 and #3938
Description
Write why you are making the changes in this pull request
This PR implements the
ListBeaconCommittees
method in our RPC package, allowing for easy retrieval of all committees in an epoch for the usage of slashing detection. Thanks @shayzluf for suggesting this feature. It fetches all beacon committees, paginated by default, based on input parameters of epoch as a query filter.Using this functionality and an attestation's bitfield, we can convert attestations into indexed form, allowing for easier slashing detection!
Link anything that would be helpful or relevant to the reviewers
The request/response type protobuf messages are given below: