fix(errors): Ignore requests Txs + RPC Error Handlings Fixes #2517
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2517 +/- ##
=======================================
Coverage 32.30% 32.31%
=======================================
Files 350 350
Lines 15674 15672 -2
Branches 20 20
=======================================
Hits 5064 5064
+ Misses 10247 10245 -2
Partials 363 363
|
Pull request was converted to draft
rezbera
changed the title
| if jsonrpc.IsUnauthorizedError(e) { | ||
| return http.ErrUnauthorized | ||
| } |
There was a problem hiding this comment.
This code was borked. It attempt to use e as the input for IsUnauthorizedError which is nil given that the casting failed and hence ok == false. This meant that IsUnauthorizedError always returned false.
| case http.StatusOK: | ||
| // OK: just proceed (no return) | ||
| case http.StatusUnauthorized: | ||
| return nil, beaconhttp.ErrUnauthorized |
There was a problem hiding this comment.
Rather than attempting to do string comparisons as before - which is extremely flakey as different ELs have slightly different strings, we just check the status.
We do this before we unmarshal as the unmarshalling assumed a success response.
| const ( | ||
| UnauthenticatedConnectionErrorStr = `could not verify execution chain ID as your | ||
| connection is not authenticated. If connecting to your execution client via HTTP, you | ||
| will need to set up JWT authentication...` | ||
|
|
||
| AuthErrMsg = "HTTP authentication to your execution client " + |
| // ErrNotStarted indicates that the execution client is not started. | ||
| ErrNotStarted = errors.New("engine client is not started") | ||
|
|
||
| // ErrFailedToRefreshJWT indicates that the JWT could not be refreshed. | ||
| ErrFailedToRefreshJWT = errors.New("failed to refresh auth token") |
| @@ -77,7 +77,7 @@ func (s *Service) prepareProposal( | |||
| "time", req.Time, | |||
| "err", err, | |||
| ) | |||
| return &cmtabci.PrepareProposalResponse{Txs: req.Txs}, nil | |||
| return &cmtabci.PrepareProposalResponse{Txs: [][]byte{}}, nil | |||
There was a problem hiding this comment.
We should be explicit in the fail case here rather than what we were doing before
There was a problem hiding this comment.
I think strictly speaking this is a different behaviour, but it does not matter in our case. IIUC:
- PrepareProposal passes to the app the transactions CometBFT knows about, that are available in CometBFT mempool (our CometBFT mempool is set to zero, so no txs)
- The app is free to reorder, replace and add any transaction it wants. CometBFT will use the transactions returned in
PrepareProposalResponseto prepare the block - now my guess is that default behaviour for the app, in case of errors is to let CometBFT build a block with whatever txs it has (mempool txs are verified via CheckTx anyhow)
It does not currently matter for us, so I am not opposed to this change.
I would only ask @sbudella-gco to confirm my story
| e, ok := err.(Error) | ||
| if !ok { | ||
| if strings.Contains( | ||
| e.Error(), "401 Unauthorized", |
There was a problem hiding this comment.
string comparisons is bad
There was a problem hiding this comment.
It seems again a copy of Prysm
| return http.ErrUnauthorized | ||
| } | ||
| var e jsonrpc.Error | ||
| ok := errors.As(err, &e) |
There was a problem hiding this comment.
| } | ||
| //nolint:errorlint // by design. | ||
| t, ok := e.(TimeoutError) | ||
| var t TimeoutError |
There was a problem hiding this comment.
| // GetPayloadBuilder returns the block store configuration. | ||
| func (c Config) GetPayloadBuilder() *builder.Config { | ||
| return &c.PayloadBuilder | ||
| } | ||
|
|
||
| // GetBlockStoreService returns the block store configuration. | ||
| func (c Config) GetBlockStoreService() *blockstore.Config { | ||
| return &c.BlockStoreService | ||
| } | ||
|
|
There was a problem hiding this comment.
Added these for consistency with other Getters - not directly related to the goal of this PR but useful in a future one
| @@ -58,16 +58,11 @@ func ExtractBlobsAndBlockFromRequest( | |||
| // UnmarshalBeaconBlockFromABCIRequest extracts a beacon block from an ABCI | |||
| // request. | |||
| func UnmarshalBeaconBlockFromABCIRequest( | |||
| req ABCIRequest, | |||
| txs [][]byte, | |||
There was a problem hiding this comment.
This is also a tangential change not relevant to this PR. However it makes this function easier to test.
It also removes the need for duplicate nil checks on req
Confirmed |
For some reason, in the error case we were returning
req.Txs. That is the only time it is used and from what I can tell - it's always empty. This minor change makes it more clear that we do not use req.TxsOur error handling from the RPC was also borked. We did not check status codes and immediately tried to unmarshal which mean that any unmarshalling errors masked the real issue which was the status code. This occurred most frequently for unauthorized errors. There was also bugs with the handling of Unauthorized Errors