op-txpool: external validating proxy for conditional transactions #11223
Conversation
![semgrep-app[bot]](https://avatars.githubusercontent.com/in/60555?s=60&v=4){kind=small}
| return common.Hash{}, invalidAuthenticationErr | ||
| } | ||
|
|
||
| caller, signature := common.HexToAddress(authElems[0]), common.Hex2Bytes(authElems[1]) |
There was a problem hiding this comment.
Found banned use of common.Hex2Bytes. Use common.FromHex instead.
| caller, signature := common.HexToAddress(authElems[0]), common.Hex2Bytes(authElems[1]) | |
| caller, signature := common.HexToAddress(authElems[0]), common.FromHex(authElems[1]) |
| hash, err := s.sendCondTx(ctx, caller, txBytes, cond) | ||
| if err != nil { | ||
| s.failures.WithLabelValues(err.Error()).Inc() | ||
| s.log.Error("failed transaction conditional", "caller", caller.String(), "hash", hash.String(), "err", err) | ||
| } |
There was a problem hiding this comment.
Variable hash is likely modified and later used on error. In some cases this could result in panics due to a nil dereference
| require.NoError(t, err) | ||
| defer c.Close() | ||
|
|
||
| c.SetHeader(authHeaderKey, fmt.Sprintf("%s:%s", common.HexToAddress("0xa"), "foobar")) |
There was a problem hiding this comment.
use net.JoinHostPort instead of fmt.Sprintf("foobar", common.HexToAddress("0xa"))
Ignore this finding from sprintf-host-port.
| addr = common.Address{19: 1} | ||
| } | ||
|
|
||
| req.Header.Set(authHeaderKey, fmt.Sprintf("%s:%s", addr, hex.EncodeToString(sig))) |
There was a problem hiding this comment.
use net.JoinHostPort instead of fmt.Sprintf(hex.EncodeToString(sig), addr)
Ignore this finding from sprintf-host-port.
| ) | ||
|
|
||
| var ( | ||
| authHeaderKey = "x-optimism-signature" |
There was a problem hiding this comment.
We should be mindful of the existing tooling that just works with the flashbots auth header key, now new tooling must be built to support our small diff
| }, nil | ||
| } | ||
|
|
||
| func (s *ConditionalTxService) SendRawTransactionConditional(ctx context.Context, txBytes hexutil.Bytes, cond *types.TransactionConditional) (common.Hash, error) { |
There was a problem hiding this comment.
I thought this service was just supposed to verify the auth header and not do anything else?
| s.failures.WithLabelValues("missing auth").Inc() | ||
| return common.Hash{}, invalidAuthenticationErr | ||
| } | ||
| authElems := strings.Split(authHeader, ":") |
There was a problem hiding this comment.
Implementing this under SendRawTransactionConditional means it cannot easily be used for any other RPC endpoints. I feel like it should be more generic and not coupled this this RPC endpoint
|
I don't think this is a tx pool, its more of a passthru proxy? I also think this should live outside of the protocol monorepo as it will result in maintenance overhead when updating versions of |
Yea not managing txs. I'll think of a better name. Perhaps this service better lives in ethereum-optimism/infra since it's supporting infrastructure? I would expect operators that enable the endpoint in op-geth to also need this service which is why I opened the PR here since it has an op-geth dependency. But the infra repo also works |
I think it would make more sense to go in the infra repo since its infra. It could also go in its own repo. |
|
@hamdiallam Is it ok if we close this? |
|
Closing this here and will re-open in infra |
Requires
This PR introduces a txpool service to proxy certain rpcs prior to reaching mempool. Primarily just the
sendRawTransactionConditionalendpoint. This implements the validation and requirements described in the design docNote: While this service fans out to the list of block builders specified in the config, proxyd will support this broadcast functionality