-
Notifications
You must be signed in to change notification settings - Fork 137
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
Ssz encoded request #69
Conversation
builder/service.go
Outdated
if cfg.RelayConfigFile != "" { | ||
bytes, err := os.ReadFile(cfg.RelayConfigFile) | ||
if err != nil { | ||
log.Info("Failed to read relay config file, will use default settings", "err", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should the builder not fail here? i.e. if a config file was explicitly requested, it should never fall back to other configuration, as it's not what the user specifically requested 🤔
How does it work when started with a previous configuration (i.e. all relays as CLI args)? Can you add a toml example? |
what do you mean by previous configuration? if started with a previous config the node will need to be restarted to load the new config |
@@ -0,0 +1,34 @@ | |||
package builder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any reason why this is called utils.go
vs ssz.go
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
our SendHTTPRequest
function is in utils.go
as it contains other util functions. I'm hoping this file can be something more generic than specifically for ssz
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that makes sense - I don't feel too strongly about this but I think it would be nice if we determine another name for such a file. utils
is an anti pattern in Go. Many projects still use it but we should try to stick to idioms when they make sense.
https://dave.cheney.net/2019/01/08/avoid-package-names-like-base-util-or-common
@@ -134,7 +139,11 @@ func (r *RemoteRelay) Stop() {} | |||
|
|||
func (r *RemoteRelay) SubmitBlock(msg *boostTypes.BuilderSubmitBlockRequest, _ ValidatorData) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just an observation: submit block should have ctx here. The same ctx that is used for the block building job
no need to fix in thir pr
} | ||
if code > 299 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not your change but I wonder why we use 299 instead of != http.StatusOk
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
had that for a long time, because any 2xx error is okay, and starting with 300 is an error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because any 2xx error is okay
I don't know if this is true. There are certain 2XX responses that yield partial or no content. I don't think it is good practice to handle such events as if they were valid even if the request itself was "successful"
Such conditional also does not check for 1XX responses. It would be better in our application layer to handle these scenarios with more granularity otherwise it can be very difficult to debug and yield misleading results
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense. let's revisit this another time though as it's outside the scope of this PR
just was wondering if this is backwards compatible to any previous configuration (i.e. with the args that worked previously) |
Yes cancellations and ssz flags are backwards compatible, it would just default to json and no gzip if previous configuration is used for relay config. However the default for the builder now is to opt out of cancellations. |
304dffb
to
7bd5156
Compare
As discussed we should ship with cancellations disabled by default |
* SSZ Encoded Builder submissions * add flag to cmd * Upgrade go builder client to 0.3.0 * error handling improvements * enable cancellations * add gzip support * add config to endpoint * linting
📝 Summary
Submit ssz encoded block submissions if ssz is enabled on the relay specified in the config. defaults to json if relay is not listed in config.
📚 References
CONTRIBUTING.md