Repository: ObolNetwork/charon

Issue:
Improve fallback beacon node logic and metric
# 🎯 Problem to be solved
Users have the ability to set a fallback beacon node to be used when requests to the primary beacon node fails. However, a beacon node may fail to respond to a request for multiple reasons that do not demand a retry to the fallback beacon node.
An example of that is having Charon submit a block proposal and receiving a 500 error with `BLOCK_ERROR_ALREADY_KNOWN`. This is an error but does not mean we should retry to the fallback.

A secondary issue is related to the `usingFallbackGauge`. Upon sending a request to the primary beacon node we would set `usingFallback` metric to `0` without checking the response/error. This would cause flickering because, in a situation where beacon node is down, we would set gauge to `0`, fail, send to fallback and set gauge to `1` but on the next request we would try the primary again and set gauge to `0`. The correct behavior should be a constant gauge of `1` since no request to the primary was successful and we always retried to the fallback.

# 🛠️ Proposed solution
- [ ] Core team consensus on the proposed solution

The solution is to filter what types of error demand a retry. We decided that we should retry a request upon a timeout error (beacon node is slow/down) or when the beacon node is syncing and unavailable to answer.
This PR introduces `isTimeoutError` and `isSyncingError` which classify whether an error message is due to a timeout or a beacon node syncing.

This PR also fixes flickering in the `usingFallback` metric by only setting the gauge to `0` if the request didn't go to the fallback node. 

# 🧪 Tests
- [ ] Tested by new automated unit/integration/smoke tests
- [ ] Manually tested on core team/canary/test clusters
- [ ] Manually tested on local compose simnet


Discussion:

Solution patch:
diff --git a/app/eth2wrap/eth2wrap.go b/app/eth2wrap/eth2wrap.go
index c4b2d1db..4cecb0d3 100644
--- a/app/eth2wrap/eth2wrap.go
+++ b/app/eth2wrap/eth2wrap.go
@@ -7,6 +7,7 @@ import (
 	"context"
 	"net"
 	"net/url"
+	"strings"
 	"sync"
 	"time"
 
@@ -163,13 +164,7 @@ func provide[O any](ctx context.Context, clients []Client, fallbacks []Client,
 
 	zero := func() O { var z O; return z }()
 
-	runForkJoin := func(clients []Client, isFallback bool) (O, error) {
-		if isFallback {
-			usingFallbackGauge.Set(1)
-		} else {
-			usingFallbackGauge.Set(0)
-		}
-
+	runForkJoin := func(clients []Client) (O, error) {
 		fork, join, cancel := forkjoin.New(ctx, work,
 			forkjoin.WithoutFailFast(),
 			forkjoin.WithWorkers(len(clients)),
@@ -208,13 +203,29 @@ func provide[O any](ctx context.Context, clients []Client, fallbacks []Client,
 
 		return nokResp.Output, nokResp.Err
 	}
+	output, err := runForkJoin(clients)
 
-	output, err := runForkJoin(clients, false)
-	if err == nil || ctx.Err() != nil || len(fallbacks) == 0 {
-		return output, err
+	// Call fallback nodes when request to beacon node timeout or if it's syncing
+	if err != nil && len(fallbacks) != 0 && (isTimeoutError(err) || isSyncingError(err)) {
+		usingFallbackGauge.Set(1)
+		return runForkJoin(fallbacks)
 	}
 
-	return runForkJoin(fallbacks, true)
+	usingFallbackGauge.Set(0)
+
+	return output, err
+}
+
+// isTimeoutError returns true if error message contains known strings for timeout related issues.
+func isTimeoutError(err error) bool {
+	msg := err.Error()
+	return strings.Contains(msg, "http request timeout") || strings.Contains(msg, "client is not active") || strings.Contains(msg, "context deadline exceeded")
+}
+
+// isSyncingError returns true if error message contains the word "syncing".
+func isSyncingError(err error) bool {
+	msg := err.Error()
+	return strings.Contains(msg, "syncing")
 }
 
 type empty struct{}



Test patch:
diff --git a/app/eth2wrap/eth2wrap_test.go b/app/eth2wrap/eth2wrap_test.go
index e9a71915..cddfccf8 100644
--- a/app/eth2wrap/eth2wrap_test.go
+++ b/app/eth2wrap/eth2wrap_test.go
@@ -147,7 +147,7 @@ func TestMulti(t *testing.T) {
 
 func TestFallback(t *testing.T) {
 	returnValue := &eth2v1.PeerCount{Connected: 42}
-	closedErr := errors.New("error")
+	closedErr := errors.New("context deadline exceeded")
 
 	tests := []struct {
 		name         string