Remote Signer: Electra #14477

james-prysm · 2024-09-24T18:42:45Z

What type of PR is this?

Feature

What does this PR do? Why is it needed?

updates the remote signing to handle changes from PRs ethereum/remote-signing-api#15 and ethereum/remote-signing-api#17

Kurtosis testing with the following settings

participants:
  - el_type: geth
    el_image: ethpandaops/geth:prague-devnet-5-a193537
    cl_type: prysm
    cl_image: gcr.io/prysmaticlabs/prysm/beacon-chain:latest
    vc_type: prysm
    vc_image: gcr.io/prysmaticlabs/prysm/validator:latest
    use_separate_vc: true
    use_remote_signer: true
  - el_type: geth
    el_image: ethpandaops/geth:prague-devnet-5-a193537
    cl_type: prysm
    cl_image: gcr.io/prysmaticlabs/prysm/beacon-chain:latest
    vc_type: prysm
    vc_image: gcr.io/prysmaticlabs/prysm/validator:latest
    use_separate_vc: true
    use_remote_signer: true
network_params:
  electra_fork_epoch: 1
additional_services:
  - dora

Which issues(s) does this PR fix?

Fixes #

Other notes for review

Acknowledgements

I have read CONTRIBUTING.md.
I have made an appropriate entry to CHANGELOG.md.
I have added a description to this PR with sufficient context for reviewers to understand this PR.

prestonvanloon · 2025-01-02T16:57:03Z

validator/keymanager/remote-web3signer/metrics.go

+	blockElectraSignRequestsTotal = promauto.NewCounter(prometheus.CounterOpts{
+		Name: "remote_web3signer_block_electra_sign_requests_total",
+		Help: "Total number of block electra sign requests",
+	})
+	blindedBlockElectraSignRequestsTotal = promauto.NewCounter(prometheus.CounterOpts{
+		Name: "remote_web3signer_blinded_block_electra_sign_requests_total",
+		Help: "Total number of blinded block electra sign requests",
+	})


This pattern seems bad. Why not just add a label to mention the fork?

blockSignRequestsTotal = promauto.NewCounter(prometheus.CounterOpts{ Name: "remote_web3signer_block_sign_requests_total", Help: "Total number of block sign requests", }) blindedBlockSignRequestsTotal = promauto.NewCounter(prometheus.CounterOpts{ Name: "remote_web3signer_blinded_block_sign_requests_total", Help: "Total number of blinded block sign requests", })

Then use it with appropriate value

blockSignRequestsTotal.WithLabelValues("fork", "electra").Inc()

I think we should do this for the v6 release. There is no reason to report old fork metrics by registering a counter for every fork when we could use labels. Can you add this for Electra, then we can make the breaking change for old metrics in v6?

nalepae · 2025-01-14T10:33:30Z

validator/keymanager/remote-web3signer/types/requests.go

+func GetAggregateAndProofV2ElectraSignRequest(request *validatorpb.SignRequest, genesisValidatorsRoot []byte) (*AggregateAndProofV2ElectraSignRequest, error) {
+	aggregateAttestationAndProof, ok := request.Object.(*validatorpb.SignRequest_AggregateAttestationAndProofElectra)
+	if !ok {
+		return nil, errors.New("failed to cast request object to aggregate attestation and proof")
+	}
+	if aggregateAttestationAndProof == nil {
+		return nil, errors.New("invalid sign request: AggregateAndProof is nil")
+	}
+	fork, err := MapForkInfo(request.SigningSlot, genesisValidatorsRoot)
+	if err != nil {
+		return nil, err
+	}
+	aggregateAndProof, err := MapAggregateAndProofElectra(aggregateAttestationAndProof.AggregateAttestationAndProofElectra)
+	if err != nil {
+		return nil, err
+	}
+	return &AggregateAndProofV2ElectraSignRequest{
+		Type:        "AGGREGATE_AND_PROOF_V2",
+		ForkInfo:    fork,
+		SigningRoot: request.SigningRoot,
+		AggregateAndProof: &AggregateAndProofV2Electra{
+			Version: "ELECTRA",
+			Data:    aggregateAndProof,
+		},
+	}, nil
+}


This function is not covered by any test.

oops forgot to add since it was waiting for sammy's pr

nalepae · 2025-01-14T10:37:44Z

validator/keymanager/remote-web3signer/keymanager.go

@@ -482,8 +488,20 @@ func handleAggregateAttestationAndProof(ctx context.Context, validator *validato
 	return json.Marshal(aggregateAndProofSignRequest)
 }

+func handleAggregateAttestationAndProofV2Electra(ctx context.Context, validator *validator.Validate, request *validatorpb.SignRequest, genesisValidatorsRoot []byte) ([]byte, error) {


Not covered by any test.

nalepae · 2025-01-14T10:39:28Z

validator/keymanager/remote-web3signer/metrics.go

@@ -30,6 +27,11 @@ var (
 		Name: "remote_web3signer_attestation_sign_requests_total",
 		Help: "Total number of attestation sign requests",
 	})
+	//TODO: deprecate these fork specific counters in prysm v6...
+	blockSignRequestsTotal = promauto.NewCounter(prometheus.CounterOpts{


Should this one be included in the comment? blockSignRequestsTotal is not fork specific right?

it's phase 0 sign requests

saolyn · 2025-01-15T14:49:43Z

validator/keymanager/remote-web3signer/keymanager.go

@@ -482,8 +488,20 @@ func handleAggregateAttestationAndProof(ctx context.Context, validator *validato
 	return json.Marshal(aggregateAndProofSignRequest)
 }

+func handleAggregateAttestationAndProofV2Electra(ctx context.Context, validator *validator.Validate, request *validatorpb.SignRequest, genesisValidatorsRoot []byte) ([]byte, error) {


Is this meant to be V2 if it's Electra specific and is the only one?

Suggested change

func handleAggregateAttestationAndProofV2Electra(ctx context.Context, validator *validator.Validate, request *validatorpb.SignRequest, genesisValidatorsRoot []byte) ([]byte, error) {

func handleAggregateAttestationAndProofElectra(ctx context.Context, validator *validator.Validate, request *validatorpb.SignRequest, genesisValidatorsRoot []byte) ([]byte, error) {

or just be, as both seems odd

Suggested change

func handleAggregateAttestationAndProofV2Electra(ctx context.Context, validator *validator.Validate, request *validatorpb.SignRequest, genesisValidatorsRoot []byte) ([]byte, error) {

func handleAggregateAttestationAndProofV2(ctx context.Context, validator *validator.Validate, request *validatorpb.SignRequest, genesisValidatorsRoot []byte) ([]byte, error) {

hmm maybe i should just use V2

saolyn · 2025-01-15T14:53:27Z

validator/keymanager/remote-web3signer/metrics.go

@@ -58,6 +60,13 @@ var (
 		Name: "remote_web3signer_blinded_block_deneb_sign_requests_total",
 		Help: "Total number of blinded block deneb sign requests",
 	})
+	/////


Meant to be here?

yeah just to show that it's deprecated to there

nalepae · 2025-01-16T08:30:32Z

validator/keymanager/remote-web3signer/types/requests.go

@@ -88,6 +90,34 @@ func GetAggregateAndProofSignRequest(request *validatorpb.SignRequest, genesisVa
 	}, nil
 }

+// GetAggregateAndProofV2SignRequest maps the request for signing type AGGREGATE_AND_PROOF_V2 on Electra changes.
+func GetAggregateAndProofV2SignRequest(v int, request *validatorpb.SignRequest, genesisValidatorsRoot []byte) (*AggregateAndProofV2SignRequest, error) {


This function is not test covered.

wait it is covered in TestKeymanager_Sign

james-prysm added 2 commits September 23, 2024 17:03

updating blockv2 to handle electra blocks

e70122c

adding aggregate attesation and proof electra

3d809bb

james-prysm added Blocked Blocked by research or external factors Web3Signer Web3Signer related tasks Electra electra hardfork labels Sep 24, 2024

james-prysm and others added 2 commits September 24, 2024 13:43

gaz

6ca4fa6

Merge branch 'develop' into remote-signer-electra

759f0e7

james-prysm changed the title ~~Remote signer electra~~ Remote signer Electra Sep 24, 2024

james-prysm changed the title ~~Remote signer Electra~~ Remote Signer: Electra Sep 24, 2024

james-prysm and others added 3 commits September 25, 2024 09:37

changelogs

bed422a

Merge branch 'develop' into remote-signer-electra

3eb62c9

Merge branch 'develop' into remote-signer-electra

767259f

james-prysm mentioned this pull request Nov 4, 2024

goethereum dependency to v1.14~ #14351

Merged

james-prysm and others added 3 commits December 30, 2024 09:00

Merge branch 'develop' into remote-signer-electra

d61a796

updating web3signer dependency

dd71aae

Merge branch 'develop' into remote-signer-electra

1ba4420

james-prysm marked this pull request as ready for review December 30, 2024 21:21

james-prysm requested a review from a team as a code owner December 30, 2024 21:21

james-prysm requested review from prestonvanloon, potuz and dB2510 December 30, 2024 21:21

james-prysm and others added 4 commits December 30, 2024 15:38

test mock was flipped

4c1e7f0

fixing hex value

182749d

accidently checked in dependency changes

0cd6050

Merge branch 'develop' into remote-signer-electra

e14a335

james-prysm added Ready For Review and removed Blocked Blocked by research or external factors labels Jan 2, 2025

prestonvanloon reviewed Jan 2, 2025

View reviewed changes

rkapka removed the Ready For Review label Jan 3, 2025

Merge branch 'develop' into remote-signer-electra

1a4dcb1

james-prysm added 2 commits January 6, 2025 13:17

preston feedback

5ad06d1

readding old metrics to not break linting

0cb9cf8

james-prysm requested review from prestonvanloon and terencechain January 13, 2025 19:40

Merge branch 'develop' into remote-signer-electra

a2ba49f

nalepae reviewed Jan 14, 2025

View reviewed changes

saolyn reviewed Jan 15, 2025

View reviewed changes

james-prysm and others added 4 commits January 15, 2025 13:28

Merge branch 'develop' into remote-signer-electra

5239021

review feedback and changelog

a8b19b7

Merge branch 'develop' into remote-signer-electra

c4dfb00

gaz

08e5119

nalepae reviewed Jan 16, 2025

View reviewed changes

Merge branch 'develop' into remote-signer-electra

a8d449f

rkapka approved these changes Jan 16, 2025

View reviewed changes

saolyn approved these changes Jan 16, 2025

View reviewed changes

james-prysm added this pull request to the merge queue Jan 16, 2025

Merged via the queue into develop with commit e263687 Jan 16, 2025
15 checks passed

james-prysm deleted the remote-signer-electra branch January 16, 2025 17:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remote Signer: Electra #14477

Remote Signer: Electra #14477

james-prysm commented Sep 24, 2024 •

edited

Loading

prestonvanloon Jan 2, 2025

nalepae Jan 14, 2025

james-prysm Jan 15, 2025

nalepae Jan 14, 2025

nalepae Jan 14, 2025

james-prysm Jan 15, 2025

saolyn Jan 15, 2025

saolyn Jan 15, 2025

james-prysm Jan 15, 2025

saolyn Jan 15, 2025

james-prysm Jan 15, 2025

nalepae Jan 16, 2025

james-prysm Jan 16, 2025

	func handleAggregateAttestationAndProofV2Electra(ctx context.Context, validator validator.Validate, request validatorpb.SignRequest, genesisValidatorsRoot []byte) ([]byte, error) {
	func handleAggregateAttestationAndProofElectra(ctx context.Context, validator validator.Validate, request validatorpb.SignRequest, genesisValidatorsRoot []byte) ([]byte, error) {

Remote Signer: Electra #14477

Remote Signer: Electra #14477

Conversation

james-prysm commented Sep 24, 2024 • 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

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

james-prysm commented Sep 24, 2024 •

edited

Loading