Post merge polish #4127
Closed
Post merge polish #4127
Conversation
This file contains 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
…ecution/consensus terminology
|
TODO overlap with #4126 @arnetheduck |
tersec
reviewed
Sep 16, 2022
| @@ -580,45 +580,46 @@ proc exchangeTransitionConfiguration*(p: Eth1Monitor): Future[EtcStatus] {.async | |||
| consensusCfg), | |||
| timeout = 1.seconds) | |||
| except CatchableError as err: | |||
| error "Failed to exchange transition configuration", err = err.msg | |||
| error "Failed to exchange transition configuration witrh Execution Client", err = err.msg | |||
There was a problem hiding this comment.
Suggested change
| error "Failed to exchange transition configuration witrh Execution Client", err = err.msg | |
| error "Failed to exchange transition configuration with execution client", err = err.msg |
|
In general, are "execution layer" and "execution client" treated as proper nouns by the spec? I agree with using that rather than engine API, but I'm not sure about the capitalization here. |
|
If I were changing the strings, I'd probably hint about |
|
#4126 needs to go in before this one though - until then, we're simply not sending the right messages at the right time and whatever we print here will not be right |
|
these changes have been, or are being, incorporated in various other PR:s, closing for now |
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.
Some post-merge polish after looking at some log files:
One thing that was recurring as well is the people not updating --web3-url=http://localhost:8545 to --web3-url=http://localhost:8551 (or websocket).
I think we need to add a log whether we are connected as a mere web3 (to monitor deposits), or as a fully-fledged CL. Though now that mainnet is switch to PoS, maybe there is no interest at all in pre-PoS with deposit monitoring as all chains of interest have moved to PoS? Or use a different consensus algorithms already integrated within an EL (for rollups for example).
Notably missing: do we send a stub to the ELs on mainnet or do we like today send the actual expected values?