diff --git a/.github/workflows/golangci-lint.yaml b/.github/workflows/golangci-lint.yaml index ce7054a8..85af0445 100644 --- a/.github/workflows/golangci-lint.yaml +++ b/.github/workflows/golangci-lint.yaml @@ -9,7 +9,7 @@ on: permissions: contents: read # Optional: allow read access to pull request. Use with `only-new-issues` option. - # pull-requests: read + pull-requests: read jobs: golangci: name: lint @@ -32,7 +32,7 @@ jobs: # args: --issues-exit-code=0 # Optional: show only new issues if it's a pull request. The default value is `false`. - # only-new-issues: true + only-new-issues: true # Optional: if set to true then the all caching functionality will be complete disabled, # takes precedence over all other caching options. diff --git a/pkg/beacon/checkpoints/majority/majority.go b/pkg/beacon/checkpoints/majority/majority.go index c027be8a..03bf3ed0 100644 --- a/pkg/beacon/checkpoints/majority/majority.go +++ b/pkg/beacon/checkpoints/majority/majority.go @@ -18,12 +18,28 @@ func New() *Decider { } func (m *Decider) Decide(checkpoints []*v1.Finality) (*v1.Finality, error) { + // Checkpoints with missing fields can't be keyed and don't count towards + // the majority threshold, matching how upstreams that fail to return + // finality at all are treated. + valid := make([]*v1.Finality, 0, len(checkpoints)) + + for _, checkpoint := range checkpoints { + if checkpoint == nil || + checkpoint.Finalized == nil || + checkpoint.Justified == nil || + checkpoint.PreviousJustified == nil { + continue + } + + valid = append(valid, checkpoint) + } + common := make(map[string]struct { Finality *v1.Finality Count int - }) + }, len(valid)) - for _, checkpoint := range checkpoints { + for _, checkpoint := range valid { key := eth.RootAsString(checkpoint.Finalized.Root) + "-" + eth.RootAsString(checkpoint.Justified.Root) + "-" + eth.RootAsString(checkpoint.PreviousJustified.Root) @@ -46,7 +62,7 @@ func (m *Decider) Decide(checkpoints []*v1.Finality) (*v1.Finality, error) { } for _, v := range common { - if v.Count > len(checkpoints)/2 { + if v.Count > len(valid)/2 { return v.Finality, nil } } diff --git a/pkg/beacon/checkpoints/majority/majority_test.go b/pkg/beacon/checkpoints/majority/majority_test.go index 111c99d5..fc546855 100644 --- a/pkg/beacon/checkpoints/majority/majority_test.go +++ b/pkg/beacon/checkpoints/majority/majority_test.go @@ -88,3 +88,48 @@ func TestSplitMajority(t *testing.T) { t.Errorf("Expected %v, got %v", ErrNoMajorityFound, err) } } + +func TestNilFinalityIgnored(t *testing.T) { + payload := []*v1.Finality{ + nil, + finalityA, + finalityA, + nil, + } + + finality, err := majority.Decide(payload) + if err != nil { + t.Fatal(err) + } + + if finality.Finalized.Root != finalityA.Finalized.Root { + t.Errorf("Expected %v, got %v", finalityA, finality) + } +} + +func TestNilCheckpointFieldsIgnored(t *testing.T) { + payload := []*v1.Finality{ + {Finalized: nil, Justified: checkpointB, PreviousJustified: checkpointB}, + {Finalized: checkpointA, Justified: nil, PreviousJustified: checkpointB}, + {Finalized: checkpointA, Justified: checkpointB, PreviousJustified: nil}, + finalityA, + } + + finality, err := majority.Decide(payload) + if err != nil { + t.Fatal(err) + } + + if finality.Finalized.Root != finalityA.Finalized.Root { + t.Errorf("Expected %v, got %v", finalityA, finality) + } +} + +func TestAllNilNoMajority(t *testing.T) { + payload := []*v1.Finality{nil, nil} + + _, err := majority.Decide(payload) + if err != ErrNoMajorityFound { + t.Errorf("Expected %v, got %v", ErrNoMajorityFound, err) + } +} diff --git a/pkg/beacon/default.go b/pkg/beacon/default.go index fd456152..46dc40cd 100644 --- a/pkg/beacon/default.go +++ b/pkg/beacon/default.go @@ -485,6 +485,15 @@ func (d *Default) checkFinality(ctx context.Context) error { continue } + if finality == nil || + finality.Finalized == nil || + finality.Justified == nil || + finality.PreviousJustified == nil { + d.log.Infof("Node %s returned incomplete finality", node.Config.Name) + + continue + } + aggFinality = append(aggFinality, finality) } diff --git a/pkg/checkpointz/checkpointz.go b/pkg/checkpointz/checkpointz.go index 375a47ec..23e35283 100644 --- a/pkg/checkpointz/checkpointz.go +++ b/pkg/checkpointz/checkpointz.go @@ -60,6 +60,17 @@ func (s *Server) Start(ctx context.Context) error { router := httprouter.New() + router.PanicHandler = func(w http.ResponseWriter, r *http.Request, rcv any) { + s.log. + WithField("panic", rcv). + WithField("path", r.URL.Path). + Error("Recovered from panic while handling request") + + if err := api.WriteErrorResponse(w, "internal server error", http.StatusInternalServerError); err != nil { + s.log.WithError(err).Error("Failed to write panic error response") + } + } + if err := s.http.Register(ctx, router); err != nil { return err }