Address @jtraglia's comments regarding Electra code #14833

rkapka · 2025-01-28T09:23:49Z

What type of PR is this?

Other

What does this PR do? Why is it needed?

Addresses @jtraglia 's feedback from https://notes.ethereum.org/@jtraglia/prysm_nits.

Each commit addresses one suggestion:

Acknowledgements

I have read CONTRIBUTING.md.
I have included a uniquely named changelog fragment file.
I have added a description to this PR with sufficient context for reviewers to understand this PR.

api/server/structs/other.go

beacon-chain/core/electra/consolidations.go

james-prysm · 2025-01-28T22:36:30Z

beacon-chain/core/electra/consolidations.go

@@ -207,7 +207,7 @@ func ProcessConsolidationRequests(ctx context.Context, st state.BeaconState, req

 		if npc, err := st.NumPendingConsolidations(); err != nil {
 			return fmt.Errorf("failed to fetch number of pending consolidations: %w", err) // This should never happen.
-		} else if npc >= pcLimit {
+		} else if npc == pcLimit {


wait is this correct? don't we need a test for this?

That's this line in the spec:

https://github.com/ethereum/consensus-specs/blob/8e56aa760d14ae5e596c265b898eb7b68f500dc4/specs/electra/beacon-chain.md?plain=1#L1651-L1653

Using >= is fine but technically it's not what the spec does.

>= may be safer here. Does NumPendingConsolidations ensure linearity?

This will be executed sequentially, so this should always work properly.

I think I would rather have >= here just because it's strictly safer than ==

Are you planning to address this? @rkapka

this change is no longer in the PR

beacon-chain/core/helpers/validator_churn.go

api/server/structs/conversions.go

beacon-chain/rpc/eth/beacon/handlers_pool_test.go

prestonvanloon

Please consider smaller PRs in the future. This will take multiple review passes to ensure that the PR addresses all of the items in the nitpick list as intended by the PR description.

validator/client/beacon-api/test-helpers/electra_beacon_block_test_helpers.go

prestonvanloon · 2025-01-29T19:19:17Z

proto/prysm/v1alpha1/beacon_chain.proto

+
+    // Representing a Fulu block.
+    SignedBeaconBlockFulu fulu_block = 13;
+
+    // Representing a blinded Fulu block.
+    SignedBlindedBeaconBlockFulu blinded_fulu_block = 14;
+    }


I don't see this in the nits doc. Is this related?

no longer in the PR

prestonvanloon · 2025-01-29T19:21:52Z

proto/engine/v1/electra.go

@@ -73,7 +73,7 @@ func unmarshalDeposits(requestListInSSZBytes []byte) ([]*DepositRequest, error)

 func unmarshalWithdrawals(requestListInSSZBytes []byte) ([]*WithdrawalRequest, error) {
 	if len(requestListInSSZBytes) < wrSize {
-		return nil, errors.New("invalid withdrawal request length, requests should be at least the size of 1 request")
+		return nil, errors.New("invalid withdrawal requests length, requests should be at least the size of 1 request")


If you want to nitpick here, then the error should be like this:

Suggested change

return nil, errors.New("invalid withdrawal requests length, requests should be at least the size of 1 request")

return nil, fmt.Errorf("invalid withdrawal requests length, requests should be at least the size of %d requests", wrSize)

Otherwise 1 is a magic number and may not match wrSize

prestonvanloon · 2025-01-29T19:22:03Z

proto/engine/v1/electra.go

@@ -83,7 +83,7 @@ func unmarshalWithdrawals(requestListInSSZBytes []byte) ([]*WithdrawalRequest, e

 func unmarshalConsolidations(requestListInSSZBytes []byte) ([]*ConsolidationRequest, error) {
 	if len(requestListInSSZBytes) < crSize {
-		return nil, errors.New("invalid consolidations request length, requests should be at least the size of 1 request")
+		return nil, errors.New("invalid consolidation requests length, requests should be at least the size of 1 request")


Same feedback here

prestonvanloon · 2025-01-29T19:27:31Z

beacon-chain/core/electra/consolidations.go

@@ -207,7 +207,7 @@ func ProcessConsolidationRequests(ctx context.Context, st state.BeaconState, req

 		if npc, err := st.NumPendingConsolidations(); err != nil {
 			return fmt.Errorf("failed to fetch number of pending consolidations: %w", err) // This should never happen.
-		} else if npc >= pcLimit {
+		} else if npc == pcLimit {


Are you planning to address this? @rkapka

beacon-chain/core/validators/validator.go

api/server/structs/conversions_block.go

prestonvanloon · 2025-01-29T19:37:54Z

changelog/radek_electra-nits.md

+### Changed
+
+- Modified `ProcessConsolidationRequests` to match the specification.
+- Changed order of `aggregate` and `selection_proof` in `AggregateAttestationAndProofElectra` protobuf.


Suggested change

- Changed order of `aggregate` and `selection_proof` in `AggregateAttestationAndProofElectra` protobuf.

- Changed proto field ID of `aggregate` and `selection_proof` in `AggregateAttestationAndProofElectra` protobuf.

I have a new changelog fragment now. Do you want me to mention this item?

prestonvanloon · 2025-01-29T19:38:35Z

beacon-chain/sync/validate_sync_contribution_proof_test.go

@@ -208,7 +208,7 @@ func TestService_ValidateSyncContributionAndProof(t *testing.T) {
 			want: pubsub.ValidationIgnore,
 		},
 		{
-			name: "Invalid Subcommittee Index",
+			name: "Invalid Subcommittee ValidatorIndex",


prestonvanloon · 2025-01-29T19:38:43Z

beacon-chain/sync/validate_bls_to_execution_change_test.go

@@ -199,7 +199,7 @@ func TestService_ValidateBlsToExecutionChange(t *testing.T) {
 			want: pubsub.ValidationAccept,
 		},
 		{
-			name: "Non-existent Validator Index",
+			name: "Non-existent Validator ValidatorIndex",


…lance`

…wal`" This reverts commit 87e4da0.

prestonvanloon

Thanks so much for splitting this PR into individual commits. This was a much better review experience!

validator/client/beacon-api/test-helpers/electra_beacon_block_test_helpers.go

prestonvanloon · 2025-01-31T03:09:45Z

config/params/minimal_config.go

@@ -112,7 +112,7 @@ func MinimalSpecConfig() *BeaconChainConfig {
 	minimalConfig.MaxDepositRequestsPerPayload = 4
 	minimalConfig.PendingPartialWithdrawalsLimit = 64
 	minimalConfig.MaxPendingPartialsPerWithdrawalsSweep = 2
-	minimalConfig.PendingDepositLimit = 134217728


This is the same value as mainnet? This isn't an override so you could just delete this line, if you want.

rkapka requested review from prestonvanloon and a team as code owners January 28, 2025 09:23

rkapka requested review from nisdas and dB2510 January 28, 2025 09:23

rkapka added Electra electra hardfork >1 Approves Required labels Jan 28, 2025

saolyn reviewed Jan 28, 2025

View reviewed changes

api/server/structs/other.go Outdated Show resolved Hide resolved

saolyn reviewed Jan 28, 2025

View reviewed changes

beacon-chain/core/electra/consolidations.go Show resolved Hide resolved

saolyn previously approved these changes Jan 28, 2025

View reviewed changes

james-prysm reviewed Jan 28, 2025

View reviewed changes

beacon-chain/core/helpers/validator_churn.go Show resolved Hide resolved

james-prysm reviewed Jan 28, 2025

View reviewed changes

api/server/structs/conversions.go Outdated Show resolved Hide resolved

james-prysm reviewed Jan 28, 2025

View reviewed changes

api/server/structs/conversions.go Show resolved Hide resolved

james-prysm reviewed Jan 28, 2025

View reviewed changes

beacon-chain/rpc/eth/beacon/handlers_pool_test.go Show resolved Hide resolved

prestonvanloon requested changes Jan 29, 2025

View reviewed changes

rkapka marked this pull request as draft January 29, 2025 20:08

rkapka marked this pull request as ready for review January 29, 2025 20:15

rkapka added 13 commits January 30, 2025 19:54

change field IDs in AggregateAttestationAndProofElectra

fb8f330

fix typo in validator.proto

fe4fc95

correct slashing indices length and shashings length

da6ae7e

check length in indexed attestation's ToConsensus method

cf5be97

use fieldparams.BLSSignatureLength

5c152af

Add length checks for execution request

636c41a

fix typo in beacon_state.proto

84e0fc2

fix typo in ssz_proto_library.bzl

631a987

fix error messages about incorrect types in block factory

7dc1bf7

add Electra case to BeaconBlockContainerToSignedBeaconBlock

ce1326b

move PeerDAS config items to PeerDAS section

fb98ee0

remove redundant params

99dfad2

rename PendingDepositLimit to PendingDepositsLimit

6ef3c6f

rkapka added 6 commits January 30, 2025 20:36

improve requests unmarshaling errors

794bbbb

rename get_validator_max_effective_balance to `get_max_effective_ba…

fdb883e

…lance`

fix typo in consolidations.go

1707624

rename index to validator_index in PendingPartialWithdrawal

87e4da0

rename randomByte to randomBytes in validators.go

124e1de

fix for version in a comment in validator.go

eb40404

rkapka dismissed saolyn’s stale review via eb40404 January 30, 2025 19:58

rkapka force-pushed the electra-nits branch from 31ad399 to eb40404 Compare January 30, 2025 19:58

rkapka added 2 commits January 30, 2025 21:01

changelog <3

7150322

Revert "rename index to validator_index in `PendingPartialWithdra…

b802451

…wal`" This reverts commit 87e4da0.

prestonvanloon approved these changes Jan 31, 2025

View reviewed changes

james-prysm approved these changes Jan 31, 2025

View reviewed changes

rkapka added this pull request to the merge queue Jan 31, 2025

Merged via the queue into develop with commit 4a63a19 Jan 31, 2025
16 checks passed

rkapka deleted the electra-nits branch January 31, 2025 16:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Address @jtraglia's comments regarding Electra code #14833

Address @jtraglia's comments regarding Electra code #14833

rkapka commented Jan 28, 2025 •

edited

Loading

james-prysm Jan 28, 2025

jtraglia Jan 28, 2025

terencechain Jan 29, 2025

jtraglia Jan 29, 2025

rkapka Jan 29, 2025

prestonvanloon Jan 29, 2025

rkapka Jan 30, 2025

prestonvanloon left a comment

prestonvanloon Jan 29, 2025

rkapka Jan 30, 2025

prestonvanloon Jan 29, 2025

rkapka Jan 30, 2025

prestonvanloon Jan 29, 2025

rkapka Jan 30, 2025

prestonvanloon Jan 29, 2025

prestonvanloon Jan 29, 2025

rkapka Jan 30, 2025

prestonvanloon Jan 29, 2025

rkapka Jan 30, 2025

prestonvanloon Jan 29, 2025

rkapka Jan 30, 2025

prestonvanloon left a comment

prestonvanloon Jan 31, 2025

	return nil, errors.New("invalid withdrawal requests length, requests should be at least the size of 1 request")
	return nil, fmt.Errorf("invalid withdrawal requests length, requests should be at least the size of %d requests", wrSize)

	- Changed order of `aggregate` and `selection_proof` in `AggregateAttestationAndProofElectra` protobuf.
	- Changed proto field ID of `aggregate` and `selection_proof` in `AggregateAttestationAndProofElectra` protobuf.

Address @jtraglia's comments regarding Electra code #14833

Address @jtraglia's comments regarding Electra code #14833

Conversation

rkapka commented Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prestonvanloon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prestonvanloon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkapka commented Jan 28, 2025 •

edited

Loading