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

Remote Signer: Electra #14477

Open
wants to merge 14 commits into
base: develop
Choose a base branch
from
Open

Remote Signer: Electra #14477

wants to merge 14 commits into from

Conversation

james-prysm
Copy link
Contributor

@james-prysm james-prysm commented Sep 24, 2024

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

image

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.

@james-prysm james-prysm added Blocked Blocked by research or external factors Web3Signer Web3Signer related tasks Electra electra hardfork labels Sep 24, 2024
@james-prysm james-prysm changed the title Remote signer electra Remote signer Electra Sep 24, 2024
@james-prysm james-prysm changed the title Remote signer Electra Remote Signer: Electra Sep 24, 2024
@james-prysm james-prysm marked this pull request as ready for review December 30, 2024 21:21
@james-prysm james-prysm requested a review from a team as a code owner December 30, 2024 21:21
@james-prysm james-prysm added Ready For Review A pull request ready for code review and removed Blocked Blocked by research or external factors labels Jan 2, 2025
Comment on lines +61 to +68
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",
})
Copy link
Member

Choose a reason for hiding this comment

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

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Electra electra hardfork Ready For Review A pull request ready for code review Web3Signer Web3Signer related tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants