Repository: ObolNetwork/charon

Issue:
Signed block fetcher retries on missed blocks
# 🐞 Bug Report

For our inclusion checker, we need to fetch signed blocks from the beacon node. When a block proposal is missed, the BN returns a 404. We consider that as an error and keep on retrying the request for the said slot. This is incorrect and we are putting unnecessary load to it.

It might be worth not throwing a warning when there is a 404 either. It shouldn't be really a warning.

Hints from issue comments:

Solution patch:
diff --git a/app/eth2wrap/eth2wrap.go b/app/eth2wrap/eth2wrap.go
index 14fb466f..fcdddf43 100644
--- a/app/eth2wrap/eth2wrap.go
+++ b/app/eth2wrap/eth2wrap.go
@@ -281,7 +281,7 @@ func incRequest(endpoint string) {
 func wrapError(ctx context.Context, err error, label string, fields ...z.Field) error {
 	// Decompose go-eth2-client http errors
 	if apiErr := new(eth2api.Error); errors.As(err, &apiErr) {
-		err = errors.New("nok http response",
+		err = errors.Wrap(err, "nok http response",
 			z.Int("status_code", apiErr.StatusCode),
 			z.Str("endpoint", apiErr.Endpoint),
 			z.Str("method", apiErr.Method),
diff --git a/app/eth2wrap/synthproposer.go b/app/eth2wrap/synthproposer.go
index 3024b93f..69df36c9 100644
--- a/app/eth2wrap/synthproposer.go
+++ b/app/eth2wrap/synthproposer.go
@@ -23,7 +23,6 @@ import (
 
 	"github.com/obolnetwork/charon/app/errors"
 	"github.com/obolnetwork/charon/app/log"
-	"github.com/obolnetwork/charon/app/z"
 )
 
 const (
@@ -124,7 +123,9 @@ func (h *synthWrapper) syntheticProposal(ctx context.Context, slot eth2p0.Slot,
 
 		signed, err := h.SignedBeaconBlock(ctx, opts)
 		if err != nil {
-			if z.ContainsField(err, z.Int("status_code", http.StatusNotFound)) {
+			// Check if error is a 404 (block not found)
+			var apiErr *eth2api.Error
+			if errors.As(err, &apiErr) && apiErr.StatusCode == http.StatusNotFound {
 				continue
 			}
 
diff --git a/core/tracker/inclusion.go b/core/tracker/inclusion.go
index cf53e812..ed965ffe 100644
--- a/core/tracker/inclusion.go
+++ b/core/tracker/inclusion.go
@@ -6,6 +6,7 @@ import (
 	"context"
 	"fmt"
 	"maps"
+	"net/http"
 	"slices"
 	"strconv"
 	"sync"
@@ -95,6 +96,13 @@ func inclSupported() map[core.DutyType]bool {
 	return inclSupported
 }
 
+// is404Error returns true if the error is a 404 (Not Found) error from the beacon node.
+// It checks if the error chain contains an eth2api.Error with a 404 status code.
+func is404Error(err error) bool {
+	var apiErr *eth2api.Error
+	return errors.As(err, &apiErr) && apiErr.StatusCode == http.StatusNotFound
+}
+
 // Submitted is called when a duty is submitted to the beacon node.
 // It adds the duty to the list of submitted duties.
 func (i *inclusionCore) Submitted(duty core.Duty, pubkey core.PubKey, data core.SignedData, delay time.Duration) (err error) {
@@ -661,6 +669,12 @@ func (a *InclusionChecker) checkBlock(ctx context.Context, slot uint64, attDutie
 
 	eth2Resp, err := a.eth2Cl.SignedBeaconBlock(ctx, &eth2api.SignedBeaconBlockOpts{Block: strconv.FormatUint(slot, 10)})
 	if err != nil {
+		// 404 means no block was proposed for this slot, which is not an error condition
+		if is404Error(err) {
+			a.checkBlockFunc(ctx, slot, false)
+			return nil
+		}
+
 		return err
 	}
 
@@ -681,6 +695,11 @@ func (a *InclusionChecker) checkBlock(ctx context.Context, slot uint64, attDutie
 func (a *InclusionChecker) checkBlockAndAtts(ctx context.Context, slot uint64, attDuties []*eth2v1.AttesterDuty) error {
 	attsResp, err := a.eth2Cl.BeaconBlockAttestations(ctx, &eth2api.BeaconBlockAttestationsOpts{Block: strconv.FormatUint(slot, 10)})
 	if err != nil {
+		// 404 means no block was proposed for this slot, which is not an error condition
+		if is404Error(err) {
+			return nil
+		}
+
 		return err
 	} else if len(attsResp.Data) == 0 {
 		return nil // No block for this slot



Test patch:
diff --git a/core/tracker/inclusion_internal_test.go b/core/tracker/inclusion_internal_test.go
index ec8d7f33..2037b229 100644
--- a/core/tracker/inclusion_internal_test.go
+++ b/core/tracker/inclusion_internal_test.go
@@ -442,3 +442,136 @@ func TestBlockInclusion(t *testing.T) {
 		require.Empty(t, missed)
 	})
 }
+
+func TestInclusion404Handling(t *testing.T) {
+	ctx := context.Background()
+
+	t.Run("checkBlock handles 404 error gracefully", func(t *testing.T) {
+		bmock, err := beaconmock.New(ctx)
+		require.NoError(t, err)
+
+		// Mock SignedBeaconBlock to return a 404 error
+		bmock.SignedBeaconBlockFunc = func(ctx context.Context, blockID string) (*eth2spec.VersionedSignedBeaconBlock, error) {
+			return nil, &eth2api.Error{
+				StatusCode: 404,
+				Method:     "GET",
+				Endpoint:   "/eth/v2/beacon/blocks/" + blockID,
+				Data:       []byte(`{"code":404,"message":"NOT_FOUND: beacon block not found"}`),
+			}
+		}
+
+		// Wrap beaconmock with eth2wrap to get proper error handling
+		eth2Cl, err := eth2wrap.Instrument([]eth2wrap.Client{bmock}, nil)
+		require.NoError(t, err)
+
+		var (
+			checkBlockCalled bool
+			foundBlock       bool
+		)
+
+		noopTrackerInclFunc := func(duty core.Duty, key core.PubKey, data core.SignedData, err error) {}
+
+		incl, err := NewInclusion(ctx, eth2Cl, noopTrackerInclFunc)
+		require.NoError(t, err)
+
+		// Override checkBlockFunc to capture the result
+		incl.checkBlockFunc = func(ctx context.Context, slot uint64, found bool) {
+			checkBlockCalled = true
+			foundBlock = found
+		}
+
+		// Call checkBlock with a slot that will trigger the 404
+		err = incl.checkBlock(ctx, 12345, nil)
+		require.NoError(t, err, "checkBlock should not return an error for 404")
+		require.True(t, checkBlockCalled, "checkBlockFunc should have been called")
+		require.False(t, foundBlock, "block should be marked as not found")
+	})
+
+	t.Run("checkBlockAndAtts handles 404 error gracefully", func(t *testing.T) {
+		featureset.EnableForT(t, featureset.AttestationInclusion)
+
+		bmock, err := beaconmock.New(ctx)
+		require.NoError(t, err)
+
+		// Mock BeaconBlockAttestations to return a 404 error
+		bmock.BeaconBlockAttestationsFunc = func(ctx context.Context, opts *eth2api.BeaconBlockAttestationsOpts) ([]*eth2spec.VersionedAttestation, error) {
+			return nil, &eth2api.Error{
+				StatusCode: 404,
+				Method:     "GET",
+				Endpoint:   "/eth/v1/beacon/blocks/" + opts.Block + "/attestations",
+				Data:       []byte(`{"code":404,"message":"NOT_FOUND: beacon block not found"}`),
+			}
+		}
+
+		// Wrap beaconmock with eth2wrap to get proper error handling
+		eth2Cl, err := eth2wrap.Instrument([]eth2wrap.Client{bmock}, nil)
+		require.NoError(t, err)
+
+		noopTrackerInclFunc := func(duty core.Duty, key core.PubKey, data core.SignedData, err error) {}
+
+		incl, err := NewInclusion(ctx, eth2Cl, noopTrackerInclFunc)
+		require.NoError(t, err)
+
+		// Call checkBlockAndAtts with a slot that will trigger the 404
+		err = incl.checkBlockAndAtts(ctx, 12345, nil)
+		require.NoError(t, err, "checkBlockAndAtts should not return an error for 404")
+	})
+
+	t.Run("checkBlock returns error for non-404 errors", func(t *testing.T) {
+		bmock, err := beaconmock.New(ctx)
+		require.NoError(t, err)
+
+		// Mock SignedBeaconBlock to return a 500 error
+		bmock.SignedBeaconBlockFunc = func(ctx context.Context, blockID string) (*eth2spec.VersionedSignedBeaconBlock, error) {
+			return nil, &eth2api.Error{
+				StatusCode: 500,
+				Method:     "GET",
+				Endpoint:   "/eth/v2/beacon/blocks/" + blockID,
+				Data:       []byte(`{"code":500,"message":"Internal server error"}`),
+			}
+		}
+
+		// Wrap beaconmock with eth2wrap to get proper error handling
+		eth2Cl, err := eth2wrap.Instrument([]eth2wrap.Client{bmock}, nil)
+		require.NoError(t, err)
+
+		noopTrackerInclFunc := func(duty core.Duty, key core.PubKey, data core.SignedData, err error) {}
+
+		incl, err := NewInclusion(ctx, eth2Cl, noopTrackerInclFunc)
+		require.NoError(t, err)
+
+		// Call checkBlock with a slot that will trigger the 500 error
+		err = incl.checkBlock(ctx, 12345, nil)
+		require.Error(t, err, "checkBlock should return an error for non-404 errors")
+	})
+
+	t.Run("checkBlockAndAtts returns error for non-404 errors", func(t *testing.T) {
+		featureset.EnableForT(t, featureset.AttestationInclusion)
+
+		bmock, err := beaconmock.New(ctx)
+		require.NoError(t, err)
+
+		// Mock BeaconBlockAttestations to return a 500 error
+		bmock.BeaconBlockAttestationsFunc = func(ctx context.Context, opts *eth2api.BeaconBlockAttestationsOpts) ([]*eth2spec.VersionedAttestation, error) {
+			return nil, &eth2api.Error{
+				StatusCode: 500,
+				Method:     "GET",
+				Endpoint:   "/eth/v1/beacon/blocks/" + opts.Block + "/attestations",
+				Data:       []byte(`{"code":500,"message":"Internal server error"}`),
+			}
+		}
+
+		// Wrap beaconmock with eth2wrap to get proper error handling
+		eth2Cl, err := eth2wrap.Instrument([]eth2wrap.Client{bmock}, nil)
+		require.NoError(t, err)
+
+		noopTrackerInclFunc := func(duty core.Duty, key core.PubKey, data core.SignedData, err error) {}
+
+		incl, err := NewInclusion(ctx, eth2Cl, noopTrackerInclFunc)
+		require.NoError(t, err)
+
+		// Call checkBlockAndAtts with a slot that will trigger the 500 error
+		err = incl.checkBlockAndAtts(ctx, 12345, nil)
+		require.Error(t, err, "checkBlockAndAtts should return an error for non-404 errors")
+	})
+}