Improve various error handling locations (ava-labs#1399)

database/leveldb/db.go

@@ -69,6 +69,9 @@ var (
 	_ database.Database = (*Database)(nil)
 	_ database.Batch    = (*batch)(nil)
 	_ database.Iterator = (*iter)(nil)
+
+	ErrInvalidConfig = errors.New("invalid config")
+	ErrCouldNotOpen  = errors.New("could not open")
 )
 
 // Database is a persistent key-value store. Apart from basic data storage
@@ -198,7 +201,7 @@ func New(file string, configBytes []byte, log logging.Logger, namespace string,
 	}
 	if len(configBytes) > 0 {
 		if err := json.Unmarshal(configBytes, &parsedConfig); err != nil {
-			return nil, fmt.Errorf("failed to parse db config: %w", err)
+			return nil, fmt.Errorf("%w: %s", ErrInvalidConfig, err)
 		}
 	}
 
@@ -228,7 +231,7 @@ func New(file string, configBytes []byte, log logging.Logger, namespace string,
 		db, err = leveldb.RecoverFile(file, nil)
 	}
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("%w: %s", ErrCouldNotOpen, err)
 	}
 
 	wrappedDB := &Database{

database/manager/manager_test.go

@@ -17,6 +17,7 @@ import (
 	"github.com/ava-labs/avalanchego/database/meterdb"
 	"github.com/ava-labs/avalanchego/database/prefixdb"
 	"github.com/ava-labs/avalanchego/utils/logging"
+	"github.com/ava-labs/avalanchego/utils/metric"
 	"github.com/ava-labs/avalanchego/version"
 )
 
@@ -102,7 +103,7 @@ func TestNewInvalidMemberPresent(t *testing.T) {
 	require.NoError(err)
 
 	_, err = NewLevelDB(dir, nil, logging.NoLog{}, v2, "", prometheus.NewRegistry())
-	require.Error(err, "expected to error creating the manager due to an open db")
+	require.ErrorIs(err, leveldb.ErrCouldNotOpen)
 
 	err = db1.Close()
 	require.NoError(err)
@@ -314,7 +315,7 @@ func TestMeterDBManager(t *testing.T) {
 
 	// Confirm that the error from a name conflict is handled correctly
 	_, err = m.NewMeterDBManager("", registry)
-	require.Error(err)
+	require.ErrorIs(err, metric.ErrFailedRegistering)
 }
 
 func TestCompleteMeterDBManager(t *testing.T) {
@@ -363,7 +364,7 @@ func TestCompleteMeterDBManager(t *testing.T) {
 
 	// Confirm that the error from a name conflict is handled correctly
 	_, err = m.NewCompleteMeterDBManager("", registry)
-	require.Error(err)
+	require.ErrorIs(err, metric.ErrFailedRegistering)
 }
 
 func TestNewManagerFromDBs(t *testing.T) {
@@ -414,7 +415,7 @@ func TestNewManagerFromNoDBs(t *testing.T) {
 	require := require.New(t)
 	// Should error if no dbs are given
 	_, err := NewManagerFromDBs(nil)
-	require.Error(err)
+	require.ErrorIs(err, errNoDBs)
 }
 
 func TestNewManagerFromNonUniqueDBs(t *testing.T) {
@@ -447,5 +448,5 @@ func TestNewManagerFromNonUniqueDBs(t *testing.T) {
 				},
 			},
 		})
-	require.Error(err)
+	require.ErrorIs(err, errNonSortedAndUniqueDBs)
 }

indexer/index.go

@@ -23,19 +23,18 @@ import (
 	"github.com/ava-labs/avalanchego/utils/wrappers"
 )
 
-const (
-	// Maximum number of containers IDs that can be fetched at a time
-	// in a call to GetContainerRange
-	MaxFetchedByRange = 1024
-)
+// Maximum number of containers IDs that can be fetched at a time in a call to
+// GetContainerRange
+const MaxFetchedByRange = 1024
 
 var (
 	// Maps to the byte representation of the next accepted index
 	nextAcceptedIndexKey   = []byte{0x00}
 	indexToContainerPrefix = []byte{0x01}
 	containerToIDPrefix    = []byte{0x02}
 	errNoneAccepted        = errors.New("no containers have been accepted")
-	errNumToFetchZero      = fmt.Errorf("numToFetch must be in [1,%d]", MaxFetchedByRange)
+	errNumToFetchInvalid   = fmt.Errorf("numToFetch must be in [1,%d]", MaxFetchedByRange)
+	errNoContainerAtIndex  = errors.New("no container at index")
 
 	_ Index = (*index)(nil)
 )
@@ -195,7 +194,7 @@ func (i *index) GetContainerByIndex(index uint64) (Container, error) {
 func (i *index) getContainerByIndex(index uint64) (Container, error) {
 	lastAcceptedIndex, ok := i.lastAcceptedIndex()
 	if !ok || index > lastAcceptedIndex {
-		return Container{}, fmt.Errorf("no container at index %d", index)
+		return Container{}, fmt.Errorf("%w %d", errNoContainerAtIndex, index)
 	}
 	indexBytes := database.PackUInt64(index)
 	return i.getContainerByIndexBytes(indexBytes)
@@ -224,10 +223,8 @@ func (i *index) getContainerByIndexBytes(indexBytes []byte) (Container, error) {
 // [numToFetch] should be in [0, MaxFetchedByRange]
 func (i *index) GetContainerRange(startIndex, numToFetch uint64) ([]Container, error) {
 	// Check arguments for validity
-	if numToFetch == 0 {
-		return nil, errNumToFetchZero
-	} else if numToFetch > MaxFetchedByRange {
-		return nil, fmt.Errorf("requested %d but maximum page size is %d", numToFetch, MaxFetchedByRange)
+	if numToFetch == 0 || numToFetch > MaxFetchedByRange {
+		return nil, fmt.Errorf("%w but is %d", errNumToFetchInvalid, numToFetch)
 	}
 
 	i.lock.RLock()

indexer/index_test.go

@@ -132,7 +132,7 @@ func TestIndexGetContainerByRangeMaxPageSize(t *testing.T) {
 
 	// Page size too large
 	_, err = idx.GetContainerRange(0, MaxFetchedByRange+1)
-	require.Error(err)
+	require.ErrorIs(err, errNumToFetchInvalid)
 
 	// Make sure data is right
 	containers, err := idx.GetContainerRange(0, MaxFetchedByRange)
@@ -170,7 +170,7 @@ func TestDontIndexSameContainerTwice(t *testing.T) {
 	require.NoError(idx.Accept(ctx, containerID, []byte{1, 2, 3}))
 	require.NoError(idx.Accept(ctx, containerID, []byte{4, 5, 6}))
 	_, err = idx.GetContainerByIndex(1)
-	require.Error(err, "should not have accepted same container twice")
+	require.ErrorIs(err, errNoContainerAtIndex)
 	gotContainer, err := idx.GetContainerByID(containerID)
 	require.NoError(err)
 	require.EqualValues(gotContainer.Bytes, []byte{1, 2, 3}, "should not have accepted same container twice")

network/dialer/dialer_test.go

@@ -64,7 +64,7 @@ func TestDialerCancelDial(t *testing.T) {
 	ctx, cancel := context.WithCancel(context.Background())
 	cancel()
 	_, err = dialer.Dial(ctx, myIP)
-	require.Error(err)
+	require.ErrorIs(err, context.Canceled)
 
 	// Make an outgoing connection with a non-cancelled context
 	conn, err := dialer.Dial(context.Background(), myIP)

network/network_test.go

@@ -6,6 +6,7 @@ package network
 import (
 	"context"
 	"crypto"
+	"crypto/rsa"
 	"net"
 	"sync"
 	"testing"
@@ -420,7 +421,7 @@ func TestTrackVerifiesSignatures(t *testing.T) {
 		Signature: nil,
 	}})
 	// The signature is wrong so this peer tracking info isn't useful.
-	require.Error(err)
+	require.ErrorIs(err, rsa.ErrVerification)
 
 	network.peersLock.RLock()
 	require.Empty(network.trackedIPs)

network/throttling/dial_throttler_test.go

@@ -86,7 +86,7 @@ func TestDialThrottlerCancel(t *testing.T) {
 		// Should block because 5 already taken within last second
 		err := throttler.Acquire(ctx)
 		// Should error because we call cancel() below
-		require.Error(t, err)
+		require.ErrorIs(t, err, context.Canceled)
 		acquiredChan <- struct{}{}
 	}()
 
@@ -95,7 +95,7 @@ func TestDialThrottlerCancel(t *testing.T) {
 	select {
 	case <-acquiredChan:
 	case <-time.After(10 * time.Millisecond):
-		t.Fatal("Acquire should have returned immediately upon context cancelation")
+		t.Fatal("Acquire should have returned immediately upon context cancellation")
 	}
 	close(acquiredChan)
 }

network/throttling/inbound_conn_throttler_test.go

@@ -4,6 +4,7 @@
 package throttling
 
 import (
+	"context"
 	"net"
 	"testing"
 
@@ -44,6 +45,8 @@ func (ml *MockListener) Addr() net.Addr {
 }
 
 func TestInboundConnThrottlerClose(t *testing.T) {
+	require := require.New(t)
+
 	closed := false
 	l := &MockListener{
 		t: t,
@@ -54,8 +57,9 @@ func TestInboundConnThrottlerClose(t *testing.T) {
 	}
 	wrappedL := NewThrottledListener(l, 1)
 	err := wrappedL.Close()
-	require.NoError(t, err)
-	require.True(t, closed)
+	require.NoError(err)
+	require.True(closed)
+
 	select {
 	case <-wrappedL.(*throttledListener).ctx.Done():
 	default:
@@ -64,7 +68,7 @@ func TestInboundConnThrottlerClose(t *testing.T) {
 
 	// Accept() should return an error because the context is cancelled
 	_, err = wrappedL.Accept()
-	require.Error(t, err)
+	require.ErrorIs(err, context.Canceled)
 }
 
 func TestInboundConnThrottlerAddr(t *testing.T) {

utils/metric/averager.go

@@ -4,13 +4,16 @@
 package metric
 
 import (
+	"errors"
 	"fmt"
 
 	"github.com/prometheus/client_golang/prometheus"
 
 	"github.com/ava-labs/avalanchego/utils/wrappers"
 )
 
+var ErrFailedRegistering = errors.New("failed registering metric")
+
 type Averager interface {
 	Observe(float64)
 }
@@ -40,10 +43,12 @@ func NewAveragerWithErrs(namespace, name, desc string, reg prometheus.Registerer
 		}),
 	}
 
-	errs.Add(
-		reg.Register(a.count),
-		reg.Register(a.sum),
-	)
+	if err := reg.Register(a.count); err != nil {
+		errs.Add(fmt.Errorf("%w: %s", ErrFailedRegistering, err))
+	}
+	if err := reg.Register(a.sum); err != nil {
+		errs.Add(fmt.Errorf("%w: %s", ErrFailedRegistering, err))
+	}
 	return &a
 }
 

version/parser.go

@@ -4,14 +4,21 @@
 package version
 
 import (
+	"errors"
 	"fmt"
 	"strconv"
 	"strings"
 )
 
+var (
+	errMissingVersionPrefix     = errors.New("missing required version prefix")
+	errMissingApplicationPrefix = errors.New("missing required application prefix")
+	errMissingVersions          = errors.New("missing version numbers")
+)
+
 func Parse(s string) (*Semantic, error) {
 	if !strings.HasPrefix(s, "v") {
-		return nil, fmt.Errorf("version string %q missing required prefix", s)
+		return nil, fmt.Errorf("%w: %q", errMissingVersionPrefix, s)
 	}
 
 	s = s[1:]
@@ -29,7 +36,7 @@ func Parse(s string) (*Semantic, error) {
 
 func ParseApplication(s string) (*Application, error) {
 	if !strings.HasPrefix(s, "avalanche/") {
-		return nil, fmt.Errorf("application string %q missing required prefix", s)
+		return nil, fmt.Errorf("%w: %q", errMissingApplicationPrefix, s)
 	}
 
 	s = s[10:]
@@ -47,8 +54,8 @@ func ParseApplication(s string) (*Application, error) {
 
 func parseVersions(s string) (int, int, int, error) {
 	splitVersion := strings.SplitN(s, ".", 3)
-	if len(splitVersion) != 3 {
-		return 0, 0, 0, fmt.Errorf("failed to parse %s as a version", s)
+	if numSeperators := len(splitVersion); numSeperators != 3 {
+		return 0, 0, 0, fmt.Errorf("%w: expected 3 only got %d", errMissingVersions, numSeperators)
 	}
 
 	major, err := strconv.Atoi(splitVersion[0])