beacon: pass context to methods #2188
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nkryuchkov
requested review from
MatusKysel,
kchojn,
oleg-ssvlabs and
iurii-ssv
This was referenced Apr 29, 2025
# Conflicts: # beacon/goclient/signing.go
MatusKysel
reviewed
May 5, 2025
| @@ -105,6 +105,8 @@ func New(logger *zap.Logger, opts Options, slotTickerProvider slotticker.Provide | |||
|
|
|||
| // Start starts to stream duties and run IBFT instances | |||
| func (n *Node) Start(logger *zap.Logger) error { | |||
| ctx := n.context // TODO: pass it to Start | |||
There was a problem hiding this comment.
what about this todo ? I think it's better to create issue in GH rather then write TODO
MatusKysel
approved these changes
May 5, 2025
There was a problem hiding this comment.
Pull Request Overview
This PR updates the beacon client to pass context parameters explicitly to all methods instead of relying on an internal context. The key changes include updating function signatures across multiple files, removing the Context field from the Options struct, and adjusting tests accordingly.
Reviewed Changes
Copilot reviewed 45 out of 45 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| beacon/goclient/voluntary_exit.go | Updated SubmitVoluntaryExit to take a context parameter. |
| beacon/goclient/validatorliveness.go | Context is now passed to recordRequestDuration. |
| beacon/goclient/validator.go | Function signatures updated to include context parameter in various methods. |
| beacon/goclient/sync_committee_contribution.go | Updated functions to require context parameter. |
| beacon/goclient/sync_committee.go | Updated signature for GetSyncMessageBlockRoot and SubmitSyncMessages. |
| beacon/goclient/spec_test.go, signing_test.go | Updated tests to pass context appropriately. |
| beacon/goclient/signing.go | Updated DomainData to accept context. |
| beacon/goclient/proposer.go | Updated functions to include context in their calls. |
| beacon/goclient/options.go | Removed the Context field from Options. |
| beacon/goclient/goclient.go | Removed internal client context and updated New to accept context. |
| Other test and helper files | Adjusted new client invocations to pass context. |
Comments suppressed due to low confidence (1)
beacon/goclient/options.go:10
- The Context field was removed from the Options struct; please update the documentation and code comments to reflect that the New function now expects a context as its first argument.
Context context.Context
oleg-ssvlabs
reviewed
May 8, 2025
oleg-ssvlabs
approved these changes
May 8, 2025
oleg-ssvlabs
reviewed
May 8, 2025
| @@ -49,7 +120,17 @@ type signer interface { | |||
|
|
|||
| // BeaconNode interface for all beacon duty calls | |||
| type BeaconNode interface { | |||
| specssv.BeaconNode // spec beacon interface | |||
There was a problem hiding this comment.
Is it OK not to use spec definitions?
There was a problem hiding this comment.
IMO, yes. We decided to diverge from spec where we need it to keep spec looking like spec without any overhead
iurii-ssv
reviewed
May 13, 2025
This reverts commit 8915277.
nkryuchkov
added a commit
that referenced
this pull request
Jun 4, 2025
* beacon: pass context to methods * fix context.Background leftovers * fix fee recipient tests * use t.Context instead of context.Background in tests * beacon: delete unused returned errors * Revert "beacon: delete unused returned errors" This reverts commit 8915277. * beacon: delete unused returned errors * spec alignment --------- Co-authored-by: y0sher <lyosher@gmail.com>
nkryuchkov
added a commit
that referenced
this pull request
Jun 4, 2025
* beacon: pass context to methods * fix context.Background leftovers * fix fee recipient tests * use t.Context instead of context.Background in tests * beacon: delete unused returned errors * Revert "beacon: delete unused returned errors" This reverts commit 8915277. * beacon: delete unused returned errors * spec alignment --------- Co-authored-by: y0sher <lyosher@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #2161
Part of PRs that will replace #1308
Currently, the beacon methods aren't cancellable because they use the common beacon client context, which matches the ssv-node lifetime