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.
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.
any reason why this is called utils.go vs ssz.go ?
There was a problem hiding this comment.
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.
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.
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 r.cancellationsEnabled { | ||
| endpoint = endpoint + "?cancellations=true" | ||
| } | ||
| if code > 299 { |
There was a problem hiding this comment.
This is not your change but I wonder why we use 299 instead of != http.StatusOk
There was a problem hiding this comment.
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.
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.
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. |
avalonche force-pushed the ssz-encoded-request branch from 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
4 participants
📝 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