Merge branch 'implement-acp-77-logging' into implement-acp-77-metrics

chains/manager.go

@@ -13,6 +13,7 @@ import (
 	"sync"
 	"time"
 
+	"github.com/prometheus/client_golang/prometheus"
 	"go.uber.org/zap"
 
 	"github.com/ava-labs/avalanchego/api/health"
@@ -490,19 +491,6 @@ func (m *manager) buildChain(chainParams ChainParameters, sb subnets.Subnet) (*c
 		return nil, fmt.Errorf("error while creating chain's log %w", err)
 	}
 
-	snowmanMetrics, err := metrics.MakeAndRegister(
-		m.snowmanGatherer,
-		primaryAlias,
-	)
-	if err != nil {
-		return nil, err
-	}
-
-	vmMetrics, err := m.getOrMakeVMRegisterer(chainParams.VMID, primaryAlias)
-	if err != nil {
-		return nil, err
-	}
-
 	ctx := &snow.ConsensusContext{
 		Context: &snow.Context{
 			NetworkID:       m.NetworkID,
@@ -520,15 +508,15 @@ func (m *manager) buildChain(chainParams ChainParameters, sb subnets.Subnet) (*c
 			Keystore:     m.Keystore.NewBlockchainKeyStore(chainParams.ID),
 			SharedMemory: m.AtomicMemory.NewSharedMemory(chainParams.ID),
 			BCLookup:     m,
-			Metrics:      vmMetrics,
+			Metrics:      metrics.NewPrefixGatherer(),
 
 			WarpSigner: warp.NewSigner(m.StakingBLSKey, m.NetworkID, chainParams.ID),
 
 			ValidatorState: m.validatorState,
 			ChainDataDir:   chainDataDir,
 		},
 		PrimaryAlias:   primaryAlias,
-		Registerer:     snowmanMetrics,
+		Registerer:     prometheus.NewRegistry(),
 		BlockAcceptor:  m.BlockAcceptorGroup,
 		TxAcceptor:     m.TxAcceptorGroup,
 		VertexAcceptor: m.VertexAcceptorGroup,
@@ -601,7 +589,15 @@ func (m *manager) buildChain(chainParams ChainParameters, sb subnets.Subnet) (*c
 		return nil, err
 	}
 
-	return chain, nil
+	vmGatherer, err := m.getOrMakeVMGatherer(chainParams.VMID)
+	if err != nil {
+		return nil, err
+	}
+
+	return chain, errors.Join(
+		m.snowmanGatherer.Register(primaryAlias, ctx.Registerer),
+		vmGatherer.Register(primaryAlias, ctx.Metrics),
+	)
 }
 
 func (m *manager) AddRegistrant(r Registrant) {
@@ -962,7 +958,6 @@ func (m *manager) createAvalancheChain(
 		StartupTracker:                 startupTracker,
 		Sender:                         snowmanMessageSender,
 		BootstrapTracker:               sb,
-		Timer:                          h,
 		PeerTracker:                    peerTracker,
 		AncestorsMaxContainersReceived: m.BootstrapAncestorsMaxContainersReceived,
 		DB:                             blockBootstrappingDB,
@@ -1357,7 +1352,6 @@ func (m *manager) createSnowmanChain(
 		StartupTracker:                 startupTracker,
 		Sender:                         messageSender,
 		BootstrapTracker:               sb,
-		Timer:                          h,
 		PeerTracker:                    peerTracker,
 		AncestorsMaxContainersReceived: m.BootstrapAncestorsMaxContainersReceived,
 		DB:                             bootstrappingDB,
@@ -1558,26 +1552,22 @@ func (m *manager) getChainConfig(id ids.ID) (ChainConfig, error) {
 	return ChainConfig{}, nil
 }
 
-func (m *manager) getOrMakeVMRegisterer(vmID ids.ID, chainAlias string) (metrics.MultiGatherer, error) {
+func (m *manager) getOrMakeVMGatherer(vmID ids.ID) (metrics.MultiGatherer, error) {
 	vmGatherer, ok := m.vmGatherer[vmID]
-	if !ok {
-		vmName := constants.VMName(vmID)
-		vmNamespace := metric.AppendNamespace(constants.PlatformName, vmName)
-		vmGatherer = metrics.NewLabelGatherer(ChainLabel)
-		err := m.Metrics.Register(
-			vmNamespace,
-			vmGatherer,
-		)
-		if err != nil {
-			return nil, err
-		}
-		m.vmGatherer[vmID] = vmGatherer
+	if ok {
+		return vmGatherer, nil
 	}
 
-	chainReg := metrics.NewPrefixGatherer()
-	err := vmGatherer.Register(
-		chainAlias,
-		chainReg,
+	vmName := constants.VMName(vmID)
+	vmNamespace := metric.AppendNamespace(constants.PlatformName, vmName)
+	vmGatherer = metrics.NewLabelGatherer(ChainLabel)
+	err := m.Metrics.Register(
+		vmNamespace,
+		vmGatherer,
 	)
-	return chainReg, err
+	if err != nil {
+		return nil, err
+	}
+	m.vmGatherer[vmID] = vmGatherer
+	return vmGatherer, nil
 }

database/dbtest/dbtest.go

@@ -22,12 +22,20 @@ import (
 	"github.com/ava-labs/avalanchego/utils/units"
 )
 
+// TestsBasic is a list of all basic database tests that require only
+// a KeyValueReaderWriterDeleter.
+var TestsBasic = map[string]func(t *testing.T, db database.KeyValueReaderWriterDeleter){
+	"SimpleKeyValue":       TestSimpleKeyValue,
+	"OverwriteKeyValue":    TestOverwriteKeyValue,
+	"EmptyKey":             TestEmptyKey,
+	"KeyEmptyValue":        TestKeyEmptyValue,
+	"MemorySafetyDatabase": TestMemorySafetyDatabase,
+	"ModifyValueAfterPut":  TestModifyValueAfterPut,
+	"PutGetEmpty":          TestPutGetEmpty,
+}
+
 // Tests is a list of all database tests
 var Tests = map[string]func(t *testing.T, db database.Database){
-	"SimpleKeyValue":                   TestSimpleKeyValue,
-	"OverwriteKeyValue":                TestOverwriteKeyValue,
-	"EmptyKey":                         TestEmptyKey,
-	"KeyEmptyValue":                    TestKeyEmptyValue,
 	"SimpleKeyValueClosed":             TestSimpleKeyValueClosed,
 	"NewBatchClosed":                   TestNewBatchClosed,
 	"BatchPut":                         TestBatchPut,
@@ -49,23 +57,29 @@ var Tests = map[string]func(t *testing.T, db database.Database){
 	"IteratorError":                    TestIteratorError,
 	"IteratorErrorAfterRelease":        TestIteratorErrorAfterRelease,
 	"CompactNoPanic":                   TestCompactNoPanic,
-	"MemorySafetyDatabase":             TestMemorySafetyDatabase,
 	"MemorySafetyBatch":                TestMemorySafetyBatch,
 	"AtomicClear":                      TestAtomicClear,
 	"Clear":                            TestClear,
 	"AtomicClearPrefix":                TestAtomicClearPrefix,
 	"ClearPrefix":                      TestClearPrefix,
-	"ModifyValueAfterPut":              TestModifyValueAfterPut,
 	"ModifyValueAfterBatchPut":         TestModifyValueAfterBatchPut,
 	"ModifyValueAfterBatchPutReplay":   TestModifyValueAfterBatchPutReplay,
 	"ConcurrentBatches":                TestConcurrentBatches,
 	"ManySmallConcurrentKVPairBatches": TestManySmallConcurrentKVPairBatches,
-	"PutGetEmpty":                      TestPutGetEmpty,
+}
+
+func init() {
+	// Add all basic database tests to the database tests
+	for name, test := range TestsBasic {
+		Tests[name] = func(t *testing.T, db database.Database) {
+			test(t, db)
+		}
+	}
 }
 
 // TestSimpleKeyValue tests to make sure that simple Put + Get + Delete + Has
 // calls return the expected values.
-func TestSimpleKeyValue(t *testing.T, db database.Database) {
+func TestSimpleKeyValue(t *testing.T, db database.KeyValueReaderWriterDeleter) {
 	require := require.New(t)
 
 	key := []byte("hello")
@@ -101,7 +115,7 @@ func TestSimpleKeyValue(t *testing.T, db database.Database) {
 	require.NoError(db.Delete(key))
 }
 
-func TestOverwriteKeyValue(t *testing.T, db database.Database) {
+func TestOverwriteKeyValue(t *testing.T, db database.KeyValueReaderWriterDeleter) {
 	require := require.New(t)
 
 	key := []byte("hello")
@@ -117,7 +131,7 @@ func TestOverwriteKeyValue(t *testing.T, db database.Database) {
 	require.Equal(value2, gotValue)
 }
 
-func TestKeyEmptyValue(t *testing.T, db database.Database) {
+func TestKeyEmptyValue(t *testing.T, db database.KeyValueReaderWriterDeleter) {
 	require := require.New(t)
 
 	key := []byte("hello")
@@ -133,7 +147,7 @@ func TestKeyEmptyValue(t *testing.T, db database.Database) {
 	require.Empty(value)
 }
 
-func TestEmptyKey(t *testing.T, db database.Database) {
+func TestEmptyKey(t *testing.T, db database.KeyValueReaderWriterDeleter) {
 	require := require.New(t)
 
 	var (
@@ -202,7 +216,7 @@ func TestSimpleKeyValueClosed(t *testing.T, db database.Database) {
 
 // TestMemorySafetyDatabase ensures it is safe to modify a key after passing it
 // to Database.Put and Database.Get.
-func TestMemorySafetyDatabase(t *testing.T, db database.Database) {
+func TestMemorySafetyDatabase(t *testing.T, db database.KeyValueReaderWriterDeleter) {
 	require := require.New(t)
 
 	key := []byte("1key")
@@ -211,9 +225,14 @@ func TestMemorySafetyDatabase(t *testing.T, db database.Database) {
 	key2 := []byte("2key")
 	value2 := []byte("value2")
 
-	// Put both K/V pairs in the database
+	// Put key in the database directly
 	require.NoError(db.Put(key, value))
-	require.NoError(db.Put(key2, value2))
+
+	// Put key2 in the database by modifying key, which should be safe
+	// to modify after the Put call
+	key[0] = key2[0]
+	require.NoError(db.Put(key, value2))
+	key[0] = keyCopy[0]
 
 	// Get the value for [key]
 	gotVal, err := db.Get(key)
@@ -1042,7 +1061,7 @@ func testClearPrefix(t *testing.T, db database.Database, clearF func(database.Da
 	require.NoError(db.Close())
 }
 
-func TestModifyValueAfterPut(t *testing.T, db database.Database) {
+func TestModifyValueAfterPut(t *testing.T, db database.KeyValueReaderWriterDeleter) {
 	require := require.New(t)
 
 	key := []byte{1}
@@ -1166,7 +1185,7 @@ func runConcurrentBatches(
 	return eg.Wait()
 }
 
-func TestPutGetEmpty(t *testing.T, db database.Database) {
+func TestPutGetEmpty(t *testing.T, db database.KeyValueReaderWriterDeleter) {
 	require := require.New(t)
 
 	key := []byte("hello")
@@ -1184,7 +1203,7 @@ func TestPutGetEmpty(t *testing.T, db database.Database) {
 	require.Empty(value) // May be nil or empty byte slice.
 }
 
-func FuzzKeyValue(f *testing.F, db database.Database) {
+func FuzzKeyValue(f *testing.F, db database.KeyValueReaderWriterDeleter) {
 	f.Fuzz(func(t *testing.T, key []byte, value []byte) {
 		require := require.New(t)
 

database/linkeddb/db_test.go

@@ -0,0 +1,25 @@
+// Copyright (C) 2019-2024, Ava Labs, Inc. All rights reserved.
+// See the file LICENSE for licensing terms.
+
+package linkeddb
+
+import (
+	"testing"
+
+	"github.com/ava-labs/avalanchego/database/dbtest"
+	"github.com/ava-labs/avalanchego/database/memdb"
+)
+
+func TestInterface(t *testing.T) {
+	for name, test := range dbtest.TestsBasic {
+		t.Run(name, func(t *testing.T) {
+			db := NewDefault(memdb.New())
+			test(t, db)
+		})
+	}
+}
+
+func FuzzKeyValue(f *testing.F) {
+	db := NewDefault(memdb.New())
+	dbtest.FuzzKeyValue(f, db)
+}

database/linkeddb/linkeddb.go

@@ -108,6 +108,8 @@ func (ldb *linkedDB) Put(key, value []byte) error {
 	}
 
 	// The key isn't currently in the list, so we should add it as the head.
+	// Note we will copy the key so it's safe to store references to it.
+	key = slices.Clone(key)
 	newHead := node{Value: slices.Clone(value)}
 	if headKey, err := ldb.getHeadKey(); err == nil {
 		// The list currently has a head, so we need to update the old head.

go.mod

@@ -22,7 +22,6 @@ require (
 	github.com/google/uuid v1.6.0
 	github.com/gorilla/mux v1.8.0
 	github.com/gorilla/rpc v1.2.0
-	github.com/gorilla/websocket v1.5.0
 	github.com/grpc-ecosystem/go-grpc-prometheus v1.2.0
 	github.com/holiman/uint256 v1.2.4
 	github.com/huin/goupnp v1.3.0
@@ -121,6 +120,7 @@ require (
 	github.com/google/gnostic-models v0.6.8 // indirect
 	github.com/google/gofuzz v1.2.0 // indirect
 	github.com/google/pprof v0.0.0-20230207041349-798e818bf904 // indirect
+	github.com/gorilla/websocket v1.5.0 // indirect
 	github.com/grpc-ecosystem/grpc-gateway/v2 v2.16.0 // indirect
 	github.com/hashicorp/go-bexpr v0.1.10 // indirect
 	github.com/hashicorp/golang-lru v0.5.5-0.20210104140557-80c98217689d // indirect

message/internal_msg_builder.go

@@ -16,7 +16,6 @@ import (
 var (
 	disconnected  = &Disconnected{}
 	gossipRequest = &GossipRequest{}
-	timeout       = &Timeout{}
 
 	_ fmt.Stringer    = (*GetStateSummaryFrontierFailed)(nil)
 	_ chainIDGetter   = (*GetStateSummaryFrontierFailed)(nil)
@@ -50,8 +49,6 @@ var (
 	_ fmt.Stringer = (*Disconnected)(nil)
 
 	_ fmt.Stringer = (*GossipRequest)(nil)
-
-	_ fmt.Stringer = (*Timeout)(nil)
 )
 
 type GetStateSummaryFrontierFailed struct {
@@ -391,18 +388,3 @@ func InternalGossipRequest(
 		expiration: mockable.MaxTime,
 	}
 }
-
-type Timeout struct{}
-
-func (Timeout) String() string {
-	return ""
-}
-
-func InternalTimeout(nodeID ids.NodeID) InboundMessage {
-	return &inboundMessage{
-		nodeID:     nodeID,
-		op:         TimeoutOp,
-		message:    timeout,
-		expiration: mockable.MaxTime,
-	}
-}

message/ops.go

@@ -59,7 +59,6 @@ const (
 	DisconnectedOp
 	NotifyOp
 	GossipRequestOp
-	TimeoutOp
 )
 
 var (
@@ -115,7 +114,6 @@ var (
 		DisconnectedOp,
 		NotifyOp,
 		GossipRequestOp,
-		TimeoutOp,
 	}
 	ConsensusOps = append(ConsensusExternalOps, ConsensusInternalOps...)
 
@@ -264,8 +262,6 @@ func (op Op) String() string {
 		return "notify"
 	case GossipRequestOp:
 		return "gossip_request"
-	case TimeoutOp:
-		return "timeout"
 	default:
 		return "unknown"
 	}