Skip to content
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

fix(errors): Ignore requests Txs + RPC Error Handlings Fixes #2517

Merged
merged 15 commits into from
Feb 18, 2025
2 changes: 1 addition & 1 deletion consensus/cometbft/service/prepare_proposal.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be explicit in the fail case here rather than what we were doing before

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 PrepareProposalResponse to 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

}

return &cmtabci.PrepareProposalResponse{
Expand Down
4 changes: 2 additions & 2 deletions execution/client/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ package client
import (
"context"
"math/big"
"strings"
"sync"
"time"

Expand All @@ -32,6 +31,7 @@ import (
ethclientrpc "github.com/berachain/beacon-kit/execution/client/ethclient/rpc"
"github.com/berachain/beacon-kit/log"
"github.com/berachain/beacon-kit/primitives/math"
"github.com/berachain/beacon-kit/primitives/net/http"
"github.com/berachain/beacon-kit/primitives/net/jwt"
)

Expand Down Expand Up @@ -167,7 +167,7 @@ func (s *EngineClient) verifyChainIDAndConnection(
// After the initial dial, check to make sure the chain ID is correct.
chainID, err = s.Client.ChainID(ctx)
if err != nil {
if strings.Contains(err.Error(), "401 Unauthorized") {
if errors.Is(err, http.ErrUnauthorized) {
// We always log this error as it is a critical error.
s.logger.Error(UnauthenticatedConnectionErrorStr)
}
Expand Down
28 changes: 7 additions & 21 deletions execution/client/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,26 +27,13 @@ import (
jsonrpc "github.com/berachain/beacon-kit/primitives/net/json-rpc"
)

// ErrUnauthenticatedConnection indicates that the connection is not
// authenticated.
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 " +
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused

"is not working. Please ensure you are setting a correct " +
"value for the JWT secret path" +
"is set correctly, or use an IPC " +
"connection if on the same machine."
)

var (
// 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")
Comment on lines -45 to -49
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unusued


// ErrMismatchedEth1ChainID is returned when the chainID does not
// match the expected chain ID.
Expand All @@ -67,15 +54,14 @@ func (s *EngineClient) handleRPCError(
s.metrics.incrementHTTPTimeoutCounter()
return http.ErrTimeout
}

// Check for authorization errors
if errors.Is(err, http.ErrUnauthorized) {
return err
}
// Check for connection errors.
//
//nolint:errorlint // from prysm.
e, ok := err.(jsonrpc.Error)
if !ok {
if jsonrpc.IsUnauthorizedError(e) {
return http.ErrUnauthorized
}
Comment on lines -76 to -78
Copy link
Contributor Author

@rezbera rezbera Feb 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

var e jsonrpc.Error
ok := errors.As(err, &e)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if !ok || e == nil {
return errors.Wrapf(
err,
"got an unexpected server error in JSON-RPC response "+
Expand Down
12 changes: 12 additions & 0 deletions execution/client/ethclient/rpc/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,14 @@ package rpc
import (
"bytes"
"context"
"fmt"
"io"
"net/http"
"sync"
"time"

"github.com/berachain/beacon-kit/primitives/encoding/json"
beaconhttp "github.com/berachain/beacon-kit/primitives/net/http"
"github.com/berachain/beacon-kit/primitives/net/jwt"
)

Expand Down Expand Up @@ -182,6 +184,16 @@ func (rpc *client) callRaw(
return nil, err
}

switch response.StatusCode {
case http.StatusOK:
// OK: just proceed (no return)
case http.StatusUnauthorized:
return nil, beaconhttp.ErrUnauthorized
Comment on lines +188 to +191
Copy link
Contributor Author

@rezbera rezbera Feb 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

default:
// Return a default error
return nil, fmt.Errorf("unexpected status code %d: %s", response.StatusCode, string(data))
}

resp := new(Response)
if err = json.Unmarshal(data, resp); err != nil {
return nil, err
Expand Down
20 changes: 0 additions & 20 deletions primitives/net/json-rpc/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,6 @@
package jsonrpc

import (
"strings"

"github.com/berachain/beacon-kit/errors"
)

Expand Down Expand Up @@ -88,21 +86,3 @@ var (
"an error occurred on the server while parsing the JSON text (code: -32700)",
)
)

// IsUnauthorizedError defines an interface for unauthorized errors.
func IsUnauthorizedError(err error) bool {
if err == nil {
return false
}

//nolint:errorlint // its'ok.
e, ok := err.(Error)
if !ok {
if strings.Contains(
e.Error(), "401 Unauthorized",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

string comparisons is bad

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems again a copy of Prysm

) {
return true
}
}
return false
}
Loading